Bug 2017565: [OCPCLOUD-1306] Add user defined trusted CA from cloud-config to ccm-trusted-ca bundle

docs/dev/trusted_ca_bundle_sync.md

@@ -19,21 +19,27 @@ which would not be successful without configured trust to platform endpoint cert
 
 For solving this 'chicken-and-egg' problem with a minimum amount of risk, a separate `trusted_ca_bundle_controller` was introduced.
 
+Moreover, [historically](https://github.com/openshift/installer/pull/5251) `additionalTrustBundle` from installer-config
+does not always end up in the **_Proxy_** object, instead this CA goes to cloud-config for future consuming by cloud provider.
+
 ## Implementation Description
 
 Implementation mostly replicates logic from `cluster-network-operator`.
 
 The controller has been [introduced](https://github.com/openshift/cluster-cloud-controller-manager-operator/pull/136) as a part of `config-sync-controllers` binary in CCCMO pod and lives as separate control loop along with [cloud-config-sync](cloud-config-sync.md) controller. 
 
 The controller performs sync and merges CA from user defined ConfigMap
-(located in `openshift-config` and referenced by cluster scoped Proxy resource) with system bundle.
+(located in `openshift-config` and referenced by cluster scoped Proxy resource) and `ca-bundle.pem` key of [synced
+cloud-config configmap](cloud-config-sync.md) with the system bundle.
 Merged CA bundle will be written to `ccm-trusted-ca` ConfigMap in `openshift-cloud-controller-manager` namespace and intended to be mounted in all CCM pods.
 
 Top-level overview:
 - In case when Proxy resource contains the `trustedCA` parameter in its spec, user's CA will be taken from a config map with a name specified by `trustedCA` parameter.
-- In case if Proxy resource does not contain the `trustedCA` parameter, only the system bundle from the CCCMO pod will be used.
-- In case if user defined CA is invalid (PEM can not be parsed, ConfigMap format is unexpected) only the system bundle from the CCCMO pod will be used
+- In case if `ca-bundle.pem` key is presented in `cloud-config` ConfigMap within CCMs namespace, it would be added to merged CA as well.
+- In case if Proxy resource does not contain the `trustedCA` parameter, CA bundle from `cloud-config` pod will be used along with system one.
+- In case if user defined CAs is invalid (PEM can not be parsed, ConfigMap format is unexpected) or not presented only the system bundle from the CCCMO pod will be used
 
 # Links
 - [cluster-network-operator implementation](https://github.com/openshift/cluster-network-operator/blob/master/pkg/controller/proxyconfig/controller.go#L91)
-- [related openshift documentation](https://docs.openshift.com/container-platform/4.8/networking/configuring-a-custom-pki.html)
+- [related openshift documentation](https://docs.openshift.com/container-platform/4.8/networking/configuring-a-custom-pki.html)
+- [installer part with cloud-config shaping](https://github.com/openshift/installer/blob/master/pkg/asset/manifests/cloudproviderconfig.go#L99)

pkg/controllers/cloud_config_sync_controller.go

@@ -22,8 +22,7 @@ import (
 const (
 	managedCloudConfigMapName = "kube-cloud-config"
 
-	cloudConfigMapName = "cloud-conf"
-	defaultConfigKey   = "cloud.conf"
+	defaultConfigKey = "cloud.conf"
 )
 
 type CloudConfigReconciler struct {
@@ -68,7 +67,7 @@ func (r *CloudConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	targetCM := &corev1.ConfigMap{}
 	targetConfigMapKey := client.ObjectKey{
 		Namespace: r.TargetNamespace,
-		Name:      cloudConfigMapName,
+		Name:      syncedCloudConfigMapName,
 	}
 
 	// If the config does not exist, it will be created later, so we can ignore a Not Found error
@@ -116,7 +115,7 @@ func (r *CloudConfigReconciler) isCloudConfigEqual(source *corev1.ConfigMap, tar
 }
 
 func (r *CloudConfigReconciler) syncCloudConfigData(ctx context.Context, source *corev1.ConfigMap, target *corev1.ConfigMap) error {
-	target.SetName(cloudConfigMapName)
+	target.SetName(syncedCloudConfigMapName)
 	target.SetNamespace(r.TargetNamespace)
 	target.Data = source.Data
 	target.BinaryData = source.BinaryData

pkg/controllers/cloud_config_sync_controller_test.go

@@ -100,6 +100,16 @@ var _ = Describe("prepareSourceConfigMap reconciler method", func() {
 		Expect(err).Should(Not(Succeed()))
 		Expect(err.Error()).Should(BeEquivalentTo("key foo specified in infra resource does not found in source configmap openshift-config/test-config"))
 	})
+
+	It("config preparation should not touch extra fields in infra ConfigMap", func() {
+		extendedInfraConfig := infraCloudConfig.DeepCopy()
+		extendedInfraConfig.Data = map[string]string{infraCloudConfKey: "bar", "bar": "baz"}
+		preparedConfig, err := reconciler.prepareSourceConfigMap(extendedInfraConfig, infra)
+		Expect(err).Should(Succeed())
+		_, ok := preparedConfig.Data[defaultConfigKey]
+		Expect(ok).Should(BeTrue())
+		Expect(len(preparedConfig.Data)).Should(BeEquivalentTo(2))
+	})
 })
 
 var _ = Describe("Cloud config sync controller", func() {
@@ -113,11 +123,10 @@ var _ = Describe("Cloud config sync controller", func() {
 
 	var infraCloudConfig *corev1.ConfigMap
 	var managedCloudConfig *corev1.ConfigMap
-	var syncedCloudConfigMap *corev1.ConfigMap
 
 	var reconciler *CloudConfigReconciler
 
-	syncedConfigMapKey := client.ObjectKey{Namespace: targetNamespaceName, Name: cloudConfigMapName}
+	syncedConfigMapKey := client.ObjectKey{Namespace: targetNamespaceName, Name: syncedCloudConfigMapName}
 
 	BeforeEach(func() {
 		By("Setting up a new manager")
@@ -164,30 +173,17 @@ var _ = Describe("Cloud config sync controller", func() {
 			GracePeriodSeconds: pointer.Int64(0),
 		}
 
-		if infraCloudConfig != nil {
-			Expect(cl.Delete(ctx, infraCloudConfig, deleteOptions)).To(Succeed())
-			Eventually(
-				apierrors.IsNotFound(cl.Get(ctx, client.ObjectKeyFromObject(infraCloudConfig), &corev1.ConfigMap{})),
-			).Should(BeTrue())
-		}
-
-		if managedCloudConfig != nil {
-			Expect(cl.Delete(ctx, managedCloudConfig, deleteOptions)).To(Succeed())
-			Eventually(
-				apierrors.IsNotFound(cl.Get(ctx, client.ObjectKeyFromObject(managedCloudConfig), &corev1.ConfigMap{})),
-			).Should(BeTrue())
-		}
-
-		if syncedCloudConfigMap != nil {
-			Expect(cl.Delete(ctx, syncedCloudConfigMap, deleteOptions)).To(Succeed())
+		allCMs := &corev1.ConfigMapList{}
+		Expect(cl.List(ctx, allCMs)).To(Succeed())
+		for _, cm := range allCMs.Items {
+			Expect(cl.Delete(ctx, cm.DeepCopy(), deleteOptions)).To(Succeed())
 			Eventually(
-				apierrors.IsNotFound(cl.Get(ctx, client.ObjectKeyFromObject(syncedCloudConfigMap), &corev1.Namespace{})),
+				apierrors.IsNotFound(cl.Get(ctx, client.ObjectKeyFromObject(cm.DeepCopy()), &corev1.ConfigMap{})),
 			).Should(BeTrue())
 		}
 
 		infraCloudConfig = nil
 		managedCloudConfig = nil
-		syncedCloudConfigMap = nil
 
 		infra := makeInfrastructureResource()
 		Expect(cl.Delete(ctx, infra)).To(Succeed())
@@ -224,7 +220,9 @@ var _ = Describe("Cloud config sync controller", func() {
 
 	It("config should be synced up if own cloud-config deleted or changed", func() {
 		syncedCloudConfigMap := &corev1.ConfigMap{}
-		Expect(cl.Get(ctx, syncedConfigMapKey, syncedCloudConfigMap)).Should(Succeed())
+		Eventually(func() {
+			Expect(cl.Get(ctx, syncedConfigMapKey, syncedCloudConfigMap)).Should(Succeed())
+		}).Should(Succeed())
 
 		syncedCloudConfigMap.Data = map[string]string{"foo": "baz"}
 		Expect(cl.Update(ctx, syncedCloudConfigMap)).To(Succeed())
@@ -248,7 +246,9 @@ var _ = Describe("Cloud config sync controller", func() {
 
 	It("config should not be updated if source and target config content are identical", func() {
 		syncedCloudConfigMap := &corev1.ConfigMap{}
-		Expect(cl.Get(ctx, syncedConfigMapKey, syncedCloudConfigMap)).Should(Succeed())
+		Eventually(func() {
+			Expect(cl.Get(ctx, syncedConfigMapKey, syncedCloudConfigMap)).Should(Succeed())
+		}).Should(Succeed())
 		initialCMresourceVersion := syncedCloudConfigMap.ResourceVersion
 
 		request := reconcile.Request{NamespacedName: client.ObjectKey{Name: "foo", Namespace: "bar"}}
@@ -276,4 +276,19 @@ var _ = Describe("Cloud config sync controller", func() {
 			return syncedCloudConfigMap.Data[defaultConfigKey] == "infra one changed", nil
 		}).Should(BeTrue())
 	})
+
+	It("all keys from cloud-config should be synced", func() {
+		changedManagedConfig := managedCloudConfig.DeepCopy()
+		changedManagedConfig.Data = map[string]string{
+			infraCloudConfKey: "infra config", cloudProviderConfigCABundleConfigMapKey: "some pem there",
+			"baz": "fizz",
+		}
+		Expect(cl.Update(ctx, changedManagedConfig)).Should(Succeed())
+
+		Eventually(func() {
+			syncedCloudConfigMap := &corev1.ConfigMap{}
+			Expect(cl.Get(ctx, syncedConfigMapKey, syncedCloudConfigMap)).Should(Succeed())
+			Expect(len(syncedCloudConfigMap.Data)).Should(BeEquivalentTo(3))
+		}).Should(Succeed())
+	})
 })

pkg/controllers/common_consts.go

@@ -8,5 +8,7 @@ const (
 	OpenshiftConfigNamespace        = "openshift-config"
 	OpenshiftManagedConfigNamespace = "openshift-config-managed"
 
+	syncedCloudConfigMapName = "cloud-conf"
+
 	proxyResourceName = "cluster"
 )

pkg/controllers/trusted_ca_bundle_controller.go

@@ -1,6 +1,7 @@
 package controllers
 
 import (
+	"bytes"
 	"context"
 	"crypto/x509"
 	"fmt"
@@ -28,7 +29,11 @@ import (
 const (
 	trustedCAConfigMapName      = "ccm-trusted-ca"
 	trustedCABundleConfigMapKey = "ca-bundle.crt"
-	systemTrustBundlePath       = "/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem"
+	// key in cloud-provider config is different for some reason.
+	// https://github.com/openshift/installer/blob/master/pkg/asset/manifests/cloudproviderconfig.go#L41
+	// https://github.com/openshift/installer/blob/master/pkg/asset/manifests/cloudproviderconfig.go#L99
+	cloudProviderConfigCABundleConfigMapKey = "ca-bundle.pem"
+	systemTrustBundlePath                   = "/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem"
 )
 
 type TrustedCABundleReconciler struct {
@@ -59,7 +64,8 @@ func (r *TrustedCABundleReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		return reconcile.Result{}, fmt.Errorf("failed to get proxy '%s': %v", req.Name, err)
 	}
 
-	// check if changed config map in 'openshift-config' namespace is proxy trusted ca
+	// Check if changed config map in 'openshift-config' namespace is proxy trusted ca.
+	// If not, return early
 	if req.Namespace == OpenshiftConfigNamespace {
 		if proxyConfig.Spec.TrustedCA.Name != req.Name {
 			klog.Infof("changed config map %s is not a proxy trusted ca, skipping", req)
@@ -71,36 +77,91 @@ func (r *TrustedCABundleReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 	if err != nil {
 		return reconcile.Result{}, fmt.Errorf("failed to get system trust bundle: %v", err)
 	}
-	ccmTrustedConfigMap := r.makeCABundleConfigMap(systemTrustBundle)
 
-	if isSpecTrustedCASet(&proxyConfig.Spec) {
-		userCABundle, err := r.getUserCABundle(ctx, proxyConfig.Spec.TrustedCA.Name)
-		if err != nil {
-			klog.Warningf("failed to get user defined trust bundle, system CA will be used: %v", err)
-		} else {
-			mergedTrustBundle, err := r.mergeCABundles(userCABundle, systemTrustBundle)
-			if err != nil {
-				return reconcile.Result{}, fmt.Errorf("can not merge system and user trust bundles: %v", err)
-			}
-			ccmTrustedConfigMap = r.makeCABundleConfigMap(mergedTrustBundle)
-		}
+	proxyCABundle, mergedTrustBundle, err := r.addProxyCABundle(ctx, proxyConfig, systemTrustBundle)
+	if err != nil {
+		return reconcile.Result{}, fmt.Errorf("can not check and add proxy CA to merged bundle: %v", err)
+	}
+
+	_, mergedTrustBundle, err = r.addCloudConfigCABundle(ctx, proxyCABundle, mergedTrustBundle)
+	if err != nil {
+		return reconcile.Result{}, fmt.Errorf("can not check and add cloud-config CA to merged bundle: %v", err)
 	}
 
+	ccmTrustedConfigMap := r.makeCABundleConfigMap(mergedTrustBundle)
 	if err := r.createOrUpdateConfigMap(ctx, ccmTrustedConfigMap); err != nil {
 		return reconcile.Result{}, fmt.Errorf("can not update target trust bundle configmap: %v", err)
 	}
 
 	return ctrl.Result{}, nil
 }
 
-func (r *TrustedCABundleReconciler) getUserCABundle(ctx context.Context, trustedCA string) ([]byte, error) {
+// addProxyCABundle checks ca bundle referred by Proxy resource and adds it to passed bundle
+// in case if proxy one is valid.
+// This function returns added bundle as first value, result as second and an error if it was occurred.
+func (r *TrustedCABundleReconciler) addProxyCABundle(ctx context.Context, proxyConfig *configv1.Proxy, originalCABundle []byte) ([]byte, []byte, error) {
+	if isSpecTrustedCASet(&proxyConfig.Spec) {
+		userProxyCABundle, err := r.getUserProxyCABundle(ctx, proxyConfig.Spec.TrustedCA.Name)
+		if err != nil {
+			klog.Warningf("failed to get user defined proxy trust bundle, system CA will be used: %v", err)
+			return nil, originalCABundle, nil
+		}
+		resultCABundle, err := r.mergeCABundles(userProxyCABundle, originalCABundle)
+		if err != nil {
+			return userProxyCABundle, nil, fmt.Errorf("can not merge system and user trust bundles: %v", err)
+		}
+		return userProxyCABundle, resultCABundle, nil
+	}
+	return nil, originalCABundle, nil
+}
+
+// addCloudConfigCABundle checks cloud-config for additional CA bundle presence and adds it to passed bundle
+// in case found one is valid.
+// This function returns added bundle as first value, result as second and an error if it was occurred.
+// Note: missed cloud-config not considered an error, because no cloud-config is expected on some platforms (AWS)
+func (r *TrustedCABundleReconciler) addCloudConfigCABundle(ctx context.Context, proxyCABundle []byte, originalCABundle []byte) ([]byte, []byte, error) {
+	// Due to installer implementation nuances, 'additionalTrustBundle' does not always end up in Proxy object.
+	// For handling this situation we have to check synced cloud-config for additional CA bundle presence.
+	// See https://github.com/openshift/installer/pull/5251#issuecomment-932622321 and
+	// https://github.com/openshift/installer/pull/5248 for additional context.
+	// However, some platforms might not have cloud-config at all (AWS), so missed cloud config is not an error.
+	ccmSyncedCloudConfig := &corev1.ConfigMap{}
+	syncedCloudConfigObjectKey := types.NamespacedName{Name: syncedCloudConfigMapName, Namespace: r.TargetNamespace}
+	if err := r.Get(ctx, syncedCloudConfigObjectKey, ccmSyncedCloudConfig); err != nil {
+		klog.Infof("cloud-config was not found: %v", err)
+		return nil, originalCABundle, nil
+	}
+
+	_, found := ccmSyncedCloudConfig.Data[cloudProviderConfigCABundleConfigMapKey]
+	if found {
+		klog.Infof("additional CA bundle key found in cloud-config")
+		_, cloudConfigCABundle, err := r.getCABundleConfigMapData(ccmSyncedCloudConfig, cloudProviderConfigCABundleConfigMapKey)
+		if err != nil {
+			klog.Warningf("failed to parse additional CA bundle from cloud-config, system and proxy CAs will be used: %v", err)
+			return nil, originalCABundle, nil
+		}
+		if bytes.Equal(proxyCABundle, cloudConfigCABundle) {
+			klog.Infof("proxy CA and cloud-config CA bundles are equal, no need to merge")
+			return nil, originalCABundle, nil
+		}
+		klog.Infof("proxy CA and cloud-config CA bundles are not equal, merging")
+		mergedCABundle, err := r.mergeCABundles(cloudConfigCABundle, originalCABundle)
+		if err != nil {
+			return cloudConfigCABundle, nil, fmt.Errorf("can not merge system and user trust bundle from cloud-config: %v", err)
+		}
+		return cloudConfigCABundle, mergedCABundle, nil
+	}
+	return nil, originalCABundle, nil
+}
+
+func (r *TrustedCABundleReconciler) getUserProxyCABundle(ctx context.Context, trustedCA string) ([]byte, error) {
 	cfgMap, err := r.getUserCABundleConfigMap(ctx, trustedCA)
 	if err != nil {
 		return nil, fmt.Errorf("failed to validate configmap reference for proxy trustedCA '%s': %v",
 			trustedCA, err)
 	}
 
-	_, bundleData, err := r.getCABundleConfigMapData(cfgMap)
+	_, bundleData, err := r.getCABundleConfigMapData(cfgMap, trustedCABundleConfigMapKey)
 	if err != nil {
 		return nil, fmt.Errorf("failed to validate trust bundle for proxy trustedCA '%s': %v",
 			trustedCA, err)
@@ -118,8 +179,8 @@ func (r *TrustedCABundleReconciler) getUserCABundleConfigMap(ctx context.Context
 	return cfgMap, nil
 }
 
-func (r *TrustedCABundleReconciler) getCABundleConfigMapData(cfgMap *corev1.ConfigMap) ([]*x509.Certificate, []byte, error) {
-	certBundle, bundleData, err := util.TrustBundleConfigMap(cfgMap)
+func (r *TrustedCABundleReconciler) getCABundleConfigMapData(cfgMap *corev1.ConfigMap, caBundleKey string) ([]*x509.Certificate, []byte, error) {
+	certBundle, bundleData, err := util.TrustBundleConfigMap(cfgMap, caBundleKey)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -201,6 +262,7 @@ func (r *TrustedCABundleReconciler) SetupWithManager(mgr ctrl.Manager) error {
 				predicate.Or(
 					openshiftConfigNamespacedPredicate(),
 					ccmTrustedCABundleConfigMapPredicates(r.TargetNamespace),
+					ownCloudConfigPredicate(r.TargetNamespace),
 				),
 			),
 		).