Merge pull request #711 from zeeke/OCPBUGS-32139

Avoid reconciling field `Webhook.ClientConfig.CABundle`

controllers/sriovoperatorconfig_controller_test.go

@@ -2,6 +2,7 @@ package controllers
 
 import (
 	"context"
+	"os"
 	"strings"
 	"sync"
 
@@ -22,6 +23,7 @@ import (
 	"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/featuregate"
 	mock_platforms "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/platforms/mock"
 	"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/platforms/openshift"
+	"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/vars"
 	util "github.com/k8snetworkplumbingwg/sriov-network-operator/test/util"
 )
 
@@ -366,5 +368,64 @@ var _ = Describe("SriovOperatorConfig controller", Ordered, func() {
 			Expect(err).ToNot(HaveOccurred())
 		})
 
+		// This test verifies that the CABundle field in the webhook configuration  added by third party components is not
+		// removed during the reconciliation loop. This is important when dealing with OpenShift certificate mangement:
+		// https://docs.openshift.com/container-platform/4.15/security/certificates/service-serving-certificate.html
+		// and when CertManager is used
+		It("should not remove the field Spec.ClientConfig.CABundle from webhook configuration when reconciling", func() {
+			validateCfg := &admv1.ValidatingWebhookConfiguration{}
+			err := util.WaitForNamespacedObject(validateCfg, k8sClient, testNamespace, "sriov-operator-webhook-config", util.RetryInterval, util.APITimeout*3)
+			Expect(err).NotTo(HaveOccurred())
+
+			By("Simulate a third party component updating the webhook CABundle")
+			validateCfg.Webhooks[0].ClientConfig.CABundle = []byte("some-base64-ca-bundle-value")
+
+			err = k8sClient.Update(ctx, validateCfg)
+			Expect(err).NotTo(HaveOccurred())
+
+			By("Trigger a controller reconciliation")
+			err = util.TriggerSriovOperatorConfigReconcile(k8sClient, testNamespace)
+			Expect(err).NotTo(HaveOccurred())
+
+			By("Verify the operator did not remove the CABundle from the webhook configuration")
+			Consistently(func(g Gomega) {
+				err = k8sClient.Get(ctx, types.NamespacedName{Namespace: testNamespace, Name: "sriov-operator-webhook-config"}, validateCfg)
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(validateCfg.Webhooks[0].ClientConfig.CABundle).To(Equal([]byte("some-base64-ca-bundle-value")))
+			}, "1s").Should(Succeed())
+		})
+
+		It("should update the webhook CABundle if `ADMISSION_CONTROLLERS_CERTIFICATES environment variable are set` ", func() {
+			DeferCleanup(os.Setenv, "ADMISSION_CONTROLLERS_CERTIFICATES_OPERATOR_CA_CRT", os.Getenv("ADMISSION_CONTROLLERS_CERTIFICATES_OPERATOR_CA_CRT"))
+			// echo "ca-bundle-1" | base64 -w 0
+			os.Setenv("ADMISSION_CONTROLLERS_CERTIFICATES_OPERATOR_CA_CRT", "Y2EtYnVuZGxlLTEK")
+
+			DeferCleanup(os.Setenv, "ADMISSION_CONTROLLERS_CERTIFICATES_INJECTOR_CA_CRT", os.Getenv("ADMISSION_CONTROLLERS_CERTIFICATES_INJECTOR_CA_CRT"))
+			// echo "ca-bundle-2" | base64 -w 0
+			os.Setenv("ADMISSION_CONTROLLERS_CERTIFICATES_INJECTOR_CA_CRT", "Y2EtYnVuZGxlLTIK")
+
+			DeferCleanup(func(old string) { vars.ClusterType = old }, vars.ClusterType)
+			vars.ClusterType = consts.ClusterTypeKubernetes
+
+			err := util.TriggerSriovOperatorConfigReconcile(k8sClient, testNamespace)
+			Expect(err).NotTo(HaveOccurred())
+
+			Eventually(func(g Gomega) {
+				validateCfg := &admv1.ValidatingWebhookConfiguration{}
+				err = k8sClient.Get(ctx, types.NamespacedName{Namespace: testNamespace, Name: "sriov-operator-webhook-config"}, validateCfg)
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(validateCfg.Webhooks[0].ClientConfig.CABundle).To(Equal([]byte("ca-bundle-1\n")))
+
+				mutateCfg := &admv1.MutatingWebhookConfiguration{}
+				err = k8sClient.Get(ctx, types.NamespacedName{Namespace: testNamespace, Name: "sriov-operator-webhook-config"}, mutateCfg)
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(mutateCfg.Webhooks[0].ClientConfig.CABundle).To(Equal([]byte("ca-bundle-1\n")))
+
+				injectorCfg := &admv1.MutatingWebhookConfiguration{}
+				err = k8sClient.Get(ctx, types.NamespacedName{Namespace: testNamespace, Name: "network-resources-injector-config"}, injectorCfg)
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(injectorCfg.Webhooks[0].ClientConfig.CABundle).To(Equal([]byte("ca-bundle-2\n")))
+			}, "1s").Should(Succeed())
+		})
 	})
 })

go.mod

@@ -11,6 +11,7 @@ require (
 	github.com/go-logr/logr v1.2.4
 	github.com/golang/mock v1.4.4
 	github.com/google/go-cmp v0.6.0
+	github.com/google/uuid v1.3.1
 	github.com/hashicorp/go-retryablehttp v0.7.7
 	github.com/jaypipes/ghw v0.9.0
 	github.com/jaypipes/pcidb v1.0.0
@@ -88,7 +89,6 @@ require (
 	github.com/google/gofuzz v1.2.0 // indirect
 	github.com/google/pprof v0.0.0-20210720184732-4bb14d4b1be1 // indirect
 	github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 // indirect
-	github.com/google/uuid v1.3.1 // indirect
 	github.com/gregjones/httpcache v0.0.0-20190611155906-901d90724c79 // indirect
 	github.com/hashicorp/go-cleanhttp v0.5.2 // indirect
 	github.com/huandu/xstrings v1.3.2 // indirect

pkg/apply/merge.go

@@ -1,6 +1,8 @@
 package apply
 
 import (
+	"fmt"
+
 	"github.com/pkg/errors"
 
 	uns "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -35,6 +37,10 @@ func MergeObjectForUpdate(current, updated *uns.Unstructured) error {
 		return err
 	}
 
+	if err := MergeWebhookForUpdate(current, updated); err != nil {
+		return err
+	}
+
 	// For all object types, merge metadata.
 	// Run this last, in case any of the more specific merge logic has
 	// changed "updated"
@@ -116,6 +122,91 @@ func MergeServiceAccountForUpdate(current, updated *uns.Unstructured) error {
 	return nil
 }
 
+// MergeWebhookForUpdate ensures the Webhook.ClientConfig.CABundle is never removed from a webhook
+func MergeWebhookForUpdate(current, updated *uns.Unstructured) error {
+	gvk := updated.GroupVersionKind()
+	if gvk.Group != "admissionregistration.k8s.io" {
+		return nil
+	}
+
+	if gvk.Kind != "ValidatingWebhookConfiguration" && gvk.Kind != "MutatingWebhookConfiguration" {
+		return nil
+	}
+
+	updatedWebhooks, ok, err := uns.NestedSlice(updated.Object, "webhooks")
+	if err != nil {
+		return err
+	}
+	if !ok {
+		return nil
+	}
+
+	currentWebhooks, ok, err := uns.NestedSlice(current.Object, "webhooks")
+	if err != nil {
+		return err
+	}
+	if !ok {
+		return nil
+	}
+
+	for _, updatedWebhook := range updatedWebhooks {
+		updateWebhookMap := updatedWebhook.(map[string]interface{})
+		caBundle, ok, err := uns.NestedString(updateWebhookMap, "clientConfig", "caBundle")
+		if err != nil {
+			return nil
+		}
+
+		// if the updated object already contains a CABundle, leave it as is. If it's nil, update its value with the current one
+		if ok && caBundle != "" {
+			continue
+		}
+
+		webhookName, ok, err := uns.NestedString(updateWebhookMap, "name")
+		if err != nil {
+			return err
+		}
+
+		if !ok {
+			return fmt.Errorf("webhook name not found in %v", updateWebhookMap)
+		}
+
+		currentWebhook := findByName(currentWebhooks, webhookName)
+		if currentWebhook == nil {
+			// Webhook not yet present in the cluster
+			continue
+		}
+
+		currentCABundle, ok, err := uns.NestedString(*currentWebhook, "clientConfig", "caBundle")
+		if err != nil {
+			return err
+		}
+
+		if !ok {
+			// Cluster webook does not have a CABundle
+			continue
+		}
+
+		uns.SetNestedField(updateWebhookMap, currentCABundle, "clientConfig", "caBundle")
+	}
+
+	err = uns.SetNestedSlice(updated.Object, updatedWebhooks, "webhooks")
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func findByName(objList []interface{}, name string) *map[string]interface{} {
+	for _, obj := range objList {
+		objMap := obj.(map[string]interface{})
+		if objMap["name"] == name {
+			return &objMap
+		}
+	}
+	return nil
+}
+
 // mergeAnnotations copies over any annotations from current to updated,
 // with updated winning if there's a conflict
 func mergeAnnotations(current, updated *uns.Unstructured) {

pkg/apply/merge_test.go

@@ -261,6 +261,70 @@ metadata:
 	g.Expect(s).To(ConsistOf("foo"))
 }
 
+func TestMergeWebHookCABundle(t *testing.T) {
+	g := NewGomegaWithT(t)
+
+	cur := UnstructuredFromYaml(t, `
+apiVersion: admissionregistration.k8s.io/v1
+kind: ValidatingWebhookConfiguration
+metadata:
+  name: webhook-1
+  annotations:
+    service.beta.openshift.io/inject-cabundle: "true"
+webhooks:
+  - name: webhook-name-1
+    sideEffects: None
+    admissionReviewVersions: ["v1", "v1beta1"]
+    failurePolicy: Fail
+    clientConfig:
+      service:
+        name: service1
+        namespace: ns1
+        path: "/path"
+      caBundle: BASE64CABUNDLE
+    rules:
+      - operations: [ "CREATE", "UPDATE", "DELETE" ]
+        apiGroups: ["sriovnetwork.openshift.io"]
+        apiVersions: ["v1"]`)
+
+	upd := UnstructuredFromYaml(t, `
+apiVersion: admissionregistration.k8s.io/v1
+kind: ValidatingWebhookConfiguration
+metadata:
+  name: webhook-1
+  annotations:
+    service.beta.openshift.io/inject-cabundle: "true"
+webhooks:
+  - name: webhook-name-1
+    sideEffects: None
+    admissionReviewVersions: ["v1", "v1beta1"]
+    failurePolicy: Fail
+    clientConfig:
+      service:
+        name: service1
+        namespace: ns1
+        path: "/path"
+    rules:
+      - operations: [ "CREATE", "UPDATE", "DELETE" ]
+        apiGroups: ["sriovnetwork.openshift.io"]
+        apiVersions: ["v1"]`)
+
+	err := MergeObjectForUpdate(cur, upd)
+	g.Expect(err).NotTo(HaveOccurred())
+
+	webhooks, ok, err := uns.NestedSlice(upd.Object, "webhooks")
+	g.Expect(err).NotTo(HaveOccurred())
+	g.Expect(ok).To(BeTrue())
+	g.Expect(len(webhooks)).To(Equal(1))
+
+	webhook0, ok := webhooks[0].(map[string]interface{})
+	g.Expect(ok).To(BeTrue())
+	caBundle, ok, err := uns.NestedString(webhook0, "clientConfig", "caBundle")
+	g.Expect(err).NotTo(HaveOccurred())
+	g.Expect(ok).To(BeTrue())
+	g.Expect(caBundle).To(Equal("BASE64CABUNDLE"))
+}
+
 // UnstructuredFromYaml creates an unstructured object from a raw yaml string
 func UnstructuredFromYaml(t *testing.T, obj string) *uns.Unstructured {
 	t.Helper()

test/util/util.go

@@ -10,7 +10,9 @@ import (
 	// "testing"
 	"time"
 
+	"github.com/google/uuid"
 	dptypes "github.com/k8snetworkplumbingwg/sriov-network-device-plugin/pkg/types"
+
 	// "github.com/operator-framework/operator-sdk/pkg/test/e2eutil"
 	appsv1 "k8s.io/api/apps/v1"
 	// corev1 "k8s.io/api/core/v1"
@@ -198,6 +200,23 @@ func ValidateDevicePluginConfig(nps []*sriovnetworkv1.SriovNetworkNodePolicy, ra
 	return nil
 }
 
+// TriggerSriovOperatorConfigReconcile edits a test label of the default SriovOperatorConfig object to
+// trigger the reconciliation logic of the controller.
+func TriggerSriovOperatorConfigReconcile(client client.Client, operatorNamespace string) error {
+	config := &sriovnetworkv1.SriovOperatorConfig{}
+	err := client.Get(goctx.Background(), types.NamespacedName{Name: "default", Namespace: operatorNamespace}, config)
+	if err != nil {
+		return err
+	}
+
+	if config.ObjectMeta.Labels == nil {
+		config.ObjectMeta.Labels = make(map[string]string)
+	}
+
+	config.ObjectMeta.Labels["trigger-test"] = uuid.NewString()
+	return client.Update(goctx.Background(), config)
+}
+
 func validateSelector(rc *dptypes.NetDeviceSelectors, ns *sriovnetworkv1.SriovNetworkNicSelector) bool {
 	if ns.DeviceID != "" {
 		if len(rc.Devices) != 1 || ns.DeviceID != rc.Devices[0] {