From c9fab435c10e8e150f978a43b27c6066c7852b7d Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Fri, 31 Jan 2020 15:48:24 -0800 Subject: [PATCH] pkg/cvo: Fetch proxy CA certs from openshift-config-managed/trusted-ca-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 5968cdf933 (pkg: switch to openshift-config for proxy CA, 2019-08-07, #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 --- pkg/cvo/cvo.go | 22 +++++++++++----------- pkg/cvo/cvo_test.go | 1 + pkg/cvo/egress.go | 25 ++++++++++++------------- pkg/internal/constants.go | 7 ++++--- 4 files changed, 28 insertions(+), 27 deletions(-) diff --git a/pkg/cvo/cvo.go b/pkg/cvo/cvo.go index a941a3ace..ebd0e41c7 100644 --- a/pkg/cvo/cvo.go +++ b/pkg/cvo/cvo.go @@ -115,11 +115,12 @@ type Operator struct { // syncBackoff allows the tests to use a quicker backoff syncBackoff wait.Backoff - cvLister configlistersv1.ClusterVersionLister - coLister configlistersv1.ClusterOperatorLister - cmConfigLister listerscorev1.ConfigMapNamespaceLister - proxyLister configlistersv1.ProxyLister - cacheSynced []cache.InformerSynced + cvLister configlistersv1.ClusterVersionLister + coLister configlistersv1.ClusterOperatorLister + cmConfigLister listerscorev1.ConfigMapNamespaceLister + cmConfigManagedLister listerscorev1.ConfigMapNamespaceLister + proxyLister configlistersv1.ProxyLister + cacheSynced []cache.InformerSynced // queue tracks applying updates to a cluster. queue workqueue.RateLimitingInterface @@ -212,6 +213,7 @@ func New( optr.proxyLister = proxyInformer.Lister() optr.cmConfigLister = cmConfigInformer.Lister().ConfigMaps(internal.ConfigNamespace) + optr.cmConfigManagedLister = cmConfigInformer.Lister().ConfigMaps(internal.ConfigManagedNamespace) // make sure this is initialized after all the listers are initialized optr.upgradeableChecks = optr.defaultUpgradeableChecks() @@ -743,17 +745,15 @@ func (optr *Operator) HTTPClient() (*http.Client, error) { // getTransportOpts retrieves the URL of the cluster proxy and the CA // trust, if they exist. func (optr *Operator) getTransportOpts() (*url.URL, *tls.Config, error) { - proxyURL, cmNameRef, err := optr.getHTTPSProxyURL() + proxyURL, err := optr.getHTTPSProxyURL() if err != nil { return nil, nil, err } var tlsConfig *tls.Config - if cmNameRef != "" { - tlsConfig, err = optr.getTLSConfig(cmNameRef) - if err != nil { - return nil, nil, err - } + tlsConfig, err = optr.getTLSConfig() + if err != nil { + return nil, nil, err } return proxyURL, tlsConfig, nil } diff --git a/pkg/cvo/cvo_test.go b/pkg/cvo/cvo_test.go index 9388e709b..465272cfc 100644 --- a/pkg/cvo/cvo_test.go +++ b/pkg/cvo/cvo_test.go @@ -2628,6 +2628,7 @@ func TestOperator_availableUpdatesSync(t *testing.T) { optr.proxyLister = &clientProxyLister{client: optr.client} optr.coLister = &clientCOLister{client: optr.client} optr.cvLister = &clientCVLister{client: optr.client} + optr.cmConfigManagedLister = &cmConfigLister{} optr.eventRecorder = record.NewFakeRecorder(100) if tt.handler != nil { diff --git a/pkg/cvo/egress.go b/pkg/cvo/egress.go index 75cfa607c..fa5260fd8 100644 --- a/pkg/cvo/egress.go +++ b/pkg/cvo/egress.go @@ -11,39 +11,38 @@ import ( // getHTTPSProxyURL returns a url.URL object for the configured // https proxy only. It can be nil if does not exist or there is an error. -func (optr *Operator) getHTTPSProxyURL() (*url.URL, string, error) { +func (optr *Operator) getHTTPSProxyURL() (*url.URL, error) { proxy, err := optr.proxyLister.Get("cluster") if errors.IsNotFound(err) { - return nil, "", nil + return nil, nil } if err != nil { - return nil, "", err + return nil, err } if &proxy.Spec != nil { if proxy.Spec.HTTPSProxy != "" { proxyURL, err := url.Parse(proxy.Spec.HTTPSProxy) if err != nil { - return nil, "", err + return nil, err } - return proxyURL, proxy.Spec.TrustedCA.Name, nil + return proxyURL, nil } } - return nil, "", nil + return nil, nil } -func (optr *Operator) getTLSConfig(cmNameRef string) (*tls.Config, error) { - cm, err := optr.cmConfigLister.Get(cmNameRef) - +func (optr *Operator) getTLSConfig() (*tls.Config, error) { + cm, err := optr.cmConfigManagedLister.Get("trusted-ca-bundle") + if errors.IsNotFound(err) { + return nil, nil + } if err != nil { return nil, err } - certPool, _ := x509.SystemCertPool() - if certPool == nil { - certPool = x509.NewCertPool() - } + certPool := x509.NewCertPool() if cm.Data["ca-bundle.crt"] != "" { if ok := certPool.AppendCertsFromPEM([]byte(cm.Data["ca-bundle.crt"])); !ok { diff --git a/pkg/internal/constants.go b/pkg/internal/constants.go index df5f417bb..b651a52bb 100644 --- a/pkg/internal/constants.go +++ b/pkg/internal/constants.go @@ -1,7 +1,8 @@ package internal const ( - ConfigNamespace = "openshift-config" - InstallerConfigMap = "openshift-install" - ManifestsConfigMap = "openshift-install-manifests" + ConfigNamespace = "openshift-config" + ConfigManagedNamespace = "openshift-config-managed" + InstallerConfigMap = "openshift-install" + ManifestsConfigMap = "openshift-install-manifests" )