-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BUG 1684206: *: store etcd CA and client certs in cluster #1363
Conversation
…and certs in the cluster The CA are stored as configmap for the purpose of establishing trust. The CAs and client certs are stored as secrets for use inside the cluster.
rebase around #1291 |
/retest |
apiVersion: v1 | ||
kind: Secret | ||
metadata: | ||
name: etcd-client-ca-deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we opening with a deprecated object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we opening with a deprecated object?
no sure what you mean by that.
This mimics the deprecation comment on https://github.com/openshift/installer/blob/c52894b2c2064e5cb8a6d736ccdbe29279d18ceb/pkg/asset/tls/etcd.go#L11
anf https://github.com/openshift/installer/blob/c52894b2c2064e5cb8a6d736ccdbe29279d18ceb/pkg/asset/manifests/operators.go#L168
@@ -9,12 +9,12 @@ import ( | |||
) | |||
|
|||
const ( | |||
pullFileName = "pull.yaml.template" | |||
pullFileName = "pull.json.template" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unrelated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see from the commit message:
fixes the template for pull as the contents were json but file was names yaml
But JSON is a subset of YAML, so .yaml
was accurate, but less specific. Anyhow, I'm fine with this rename if you don't want to bother splitting it out ;).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have mentioned that in c52894b
modifies the manifest target to use the filename from template assets, rather than duplicating the filename.
fixes the template for pull as the contents were json but file was names yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for consistency between the name of the template and the name of the file generated.
pkg/asset/manifests/operators.go
Outdated
} { | ||
dependencies.Get(a) | ||
for _, f := range a.Files() { | ||
assetData[strings.TrimSuffix(filepath.Base(f.Filename), ".template")] = applyTemplateData(f.Data, templateData) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a welcome change over what was being done. We don't need the assetData
map as a temporary home. We can add the file directly to a *assetFile
slice here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modifies the manifest asset to use the filename from template assets, rather than duplicating the filename. fixes the template for pull as the contents were json but file was named .yaml
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, staebler The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Catching up with 53c6fc3 (assets/manifests: push etcd configmaps and secrets into cluster, 2019-03-04, openshift#1363). Generated with: $ openshift-install graph | dot -Tsvg >docs/design/resource_dep.svg using: $ dot -V dot - graphviz version 2.30.1 (20170916.1124)
xref: https://bugzilla.redhat.com/show_bug.cgi?id=1684206
This pushes the etcd ca into cluster to sign future etcd client CSR requests.
/cc @csrwng