-
Notifications
You must be signed in to change notification settings - Fork 235
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
Adds Trust Bundle Publishing to Proxy Controller #271
Adds Trust Bundle Publishing to Proxy Controller #271
Conversation
1cddc5c
to
d333e71
Compare
d333e71
to
d5f3e80
Compare
// encoded certificates, embeds the merged byte slice into a configmap | ||
// named "proxy-ca-bundle" in namespace "openshift-config-managed" and | ||
// returns the configmap. | ||
func (r *ConfigMapReconciler) ensureMergedConfigMap(additionalData, systemData []byte) (*corev1.ConfigMap, error) { |
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.
Is it a problem if the additional CA is already present in the system trust? Will that break expectations?
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.
it's just going to result in 2 copies of that cert in the bundle, right?
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.
Yup. The question is: does that break anything? Does the bundle need to be unique? (I have no idea)
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'd be surprised but it would definitely be a good thing to test.
d5f3e80
to
59f1bcc
Compare
59f1bcc
to
2590627
Compare
2590627
to
9cf327d
Compare
|
||
// Reconcile expects request to refer to a proxy object named "cluster" | ||
// in the default namespace or to a configmap object named | ||
// "user-ca-bundle" in namespace "openshift-config-managed", and will |
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 comment is wrong... i'd expect this controller to be watching 3 things:
- the trust-bundle configmap in openshift-config-managed (this CM has a fixed name), because you need to stomp it back to the correct value if it gets updated by something else
- the additional CA bundle configmap in the openshift-config namespace (this configmap name can change, so you need to detect if it changes, by watching the proxy config resource), because if the bundle changes, you need to re-merge the values w/ the system values and publish it to the CM in (1).
- the proxyconfig resource (so you can determine the name of the configmap in (2) and perform the merge action on it)
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.
9cf327d
to
5a0adf0
Compare
5a0adf0
to
d68c724
Compare
2960d45
to
2a0c159
Compare
2a0c159
to
b0e2c1c
Compare
/lgtm |
/test e2e-aws-upgrade |
/test e2e-aws-upgrade |
b0e2c1c
to
46e0ac1
Compare
/lgtm |
/approve |
aws-e2e-upgrade job failure due to: I found that I am removing the following from
|
46e0ac1
to
6de48c6
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bparees, danehans, knobunc 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 |
Depends on #245
ConfigMap
object.Data
of "openshift-config-managed/user-ca-bundle"ConfigMap
object.ConfigMap
object.Jira: SDN-501
PTAL @squeed @danwinship @dcbw @bparees @JacobTanenbaum