-
Notifications
You must be signed in to change notification settings - Fork 191
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 1797123: pkg/cvo: Fetch proxy CA certs from openshift-config-managed/trusted-ca-bundle #311
Bug 1797123: pkg/cvo: Fetch proxy CA certs from openshift-config-managed/trusted-ca-bundle #311
Conversation
@wking: This pull request references Bugzilla bug 1797123, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/bugzilla refresh |
@wking: This pull request references Bugzilla bug 1797123, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
If the operator that creates the config map in that managed namespace is created by CVO, there is a cycle of dependencies.. |
Is that a problem? We shouldn't need either Cincy graph fetches or signature pulls until the network operator comes up. |
Yeah, i don't think we should add this cycle. |
I can't imagine a case where the CVO would need to make external HTTP(S) requests before the network operator comes up. And I like having the CVO continue to be able to make HTTP(S) requests via a proxy even in the event that a cluster-admin borks the ConfigMap they control (because the network operator will refuse to copy the broken CAs into the openshift-config-managed ConfigMap that it controls). But whatever, I don't care enough to push this through ;). I'm happy to reopen and rebase if there's a consensus to pick this approach back up later, just let me know. Until then. /close |
@wking: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@wking: This pull request references Bugzilla bug 1797123, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/bugzilla refresh |
@wking: This pull request references Bugzilla bug 1797123, which is valid. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
02fd56f
to
f2c34cf
Compare
cm, err := optr.cmConfigLister.Get(cmNameRef) | ||
|
||
func (optr *Operator) getTLSConfig() (*tls.Config, error) { | ||
cm, err := optr.cmConfigManagedLister.Get("trusted-ca-bundle") |
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.
We are hard coding the name here i.e. "trusted-ca-bundle"
. Is there any way we can avoid 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.
It is part of the adopted enhancement. I only use it in this one place, so it doesn't seem useful to create a local const
for it. But I can if you want one.
…a-bundle The API docs [1] recommend avoiding trustedCA (unless you happen to be the "proxy validator") and instead pulling the trust bundle from this managed namespace. The docs also explain that the trusted-ca-bundle ConfigMap has already been merged with the system certificates, so we don't need to inject those locally. If trusted-ca-bundle doesn't exist, we'll fall back to our local system store. Our logic was touched most recently in 5968cdf (pkg: switch to openshift-config for proxy CA, 2019-08-07, openshift#231), which resolved the "looking in the wrong place" issue by looking at the trustedCA source (which feeds the proxy validator). With this commit we switch that around and look at the proxy validator's output. [1]: https://github.com/openshift/api/blob/f2a771e1a90ceb4e65f1ca2c8b11fc1ac6a66da8/config/v1/types_proxy.go#L44-L52
1b3ae21
to
c9fab43
Compare
e2e-upgrade died on /retest |
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.
/lgtm
/retest |
/hold |
/hold cancel |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, LalatenduMohanty, vrutkovs, wking 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 |
@wking: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/retest |
@wking: All pull requests linked via external trackers have merged: openshift/cluster-version-operator#311. Bugzilla bug 1797123 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
With c9fab43 (pkg/cvo: Fetch proxy CA certs from openshift-config-managed/trusted-ca-bundle, 2020-01-31, openshift#311), I attempted to pivot to loading the TLS configration from the openshift-config-managed namespace. But I hadn't realized that cmConfigInformer was scoped to the openshift-config namespace, so attempts to retrieve trusted-ca-bundle failed via the "not found" no-op path regardless of whether the ConfigMap actually existed. With this commit, I create a new informer for the openshift-config-managed namespace, so we can successfully retrieve the ConfigMap when it exists. An alternative approach would be to drop the informers.WithNamespace filter. My impression is that having two namespace-filtered informers will be more efficient than a single unfiltered informer, but I'm not really sure.
Since 4.2's ea5e3bc (Add http transport for cincinnati to enable proxy, 2019-07-16, openshift#219), the CVO has been loading proxy config from the spec property. We should be loading from status instead, so we benefit from the network operator's validation. Risk is small, because unlike some other in-cluster components, the CVO is unlikely to break things if it is temporarily consuming a broken proxy configuration. This is similar to c9fab43 (pkg/cvo: Fetch proxy CA certs from openshift-config-managed/trusted-ca-bundle, 2020-01-31, openshift#311), where we moved our trusted CA source from the user-configured ConfigMap to the network-operator-validated ConfigMap.
Since 4.2's ea5e3bc (Add http transport for cincinnati to enable proxy, 2019-07-16, openshift#219), the CVO has been loading proxy config from the spec property. We should be loading from status instead, so we benefit from the network operator's validation. Risk is small, because unlike some other in-cluster components, the CVO is unlikely to break things if it is temporarily consuming a broken proxy configuration. This is similar to c9fab43 (pkg/cvo: Fetch proxy CA certs from openshift-config-managed/trusted-ca-bundle, 2020-01-31, openshift#311), where we moved our trusted CA source from the user-configured ConfigMap to the network-operator-validated ConfigMap.
Since 4.2's ea5e3bc (Add http transport for cincinnati to enable proxy, 2019-07-16, openshift#219), the CVO has been loading proxy config from the spec property. We should be loading from status instead, so we benefit from the network operator's validation. Risk is small, because unlike some other in-cluster components, the CVO is unlikely to break things if it is temporarily consuming a broken proxy configuration. This is similar to c9fab43 (pkg/cvo: Fetch proxy CA certs from openshift-config-managed/trusted-ca-bundle, 2020-01-31, openshift#311), where we moved our trusted CA source from the user-configured ConfigMap to the network-operator-validated ConfigMap.
Since 4.2's ea5e3bc (Add http transport for cincinnati to enable proxy, 2019-07-16, openshift#219), the CVO has been loading proxy config from the spec property. We should be loading from status instead, so we benefit from the network operator's validation. Risk is small, because unlike some other in-cluster components, the CVO is unlikely to break things if it is temporarily consuming a broken proxy configuration. This is similar to c9fab43 (pkg/cvo: Fetch proxy CA certs from openshift-config-managed/trusted-ca-bundle, 2020-01-31, openshift#311), where we moved our trusted CA source from the user-configured ConfigMap to the network-operator-validated ConfigMap.
Since 4.2's ea5e3bc (Add http transport for cincinnati to enable proxy, 2019-07-16, openshift#219), the CVO has been loading proxy config from the spec property. We should be loading from status instead, so we benefit from the network operator's validation. Risk is small, because unlike some other in-cluster components, the CVO is unlikely to break things if it is temporarily consuming a broken proxy configuration. This is similar to c9fab43 (pkg/cvo: Fetch proxy CA certs from openshift-config-managed/trusted-ca-bundle, 2020-01-31, openshift#311), where we moved our trusted CA source from the user-configured ConfigMap to the network-operator-validated ConfigMap.
The API docs recommend avoiding
trustedCA
(unless you happen to be the "proxy validator") and instead pulling the trust bundle from this managed namespace. The docs also explain that thetrusted-ca-bundle
ConfigMap has already been merged with the system certificates, so we don't need to inject those locally. Iftrusted-ca-bundle
doesn't exist, we'll fall back to our local system store.Our logic was touched most recently in 5968cdf (#231), which resolved the "looking in the wrong place" issue by looking at the
trustedCA
source (which feeds the proxy validator). With this commit we switch that around and look at the proxy validator's output.