Avoid reconciling field Webhook.ClientConfig.CABundle

Webhook resources (`ValidatingWebhookConfiguration` and `MutatingWebhookConfiguration`) in OpenShift
are configured with `service.beta.openshift.io/inject-cabundle` in a way that
a third component fills the ClientConfig.CABundle field of the webhook.
When reconciling webhooks, do not override the field and avoid a flakiness, as
there might be a time slot in which the API server is not configured with a valid
client certificate:

```
Error from server (InternalError): error when creating "policies": Internal error occurred: failed calling webhook "operator-webhook.sriovnetwork.openshift.io": failed to call webhook: Post "https://operator-webhook-service.openshift-sriov-network-operator.svc:443/mutating-custom-resource?timeout=10s": tls: failed to verify certificate: x509: certificate signed by unknown authority
```

The same behavior also happens when using CertManager

Refs:
- https://docs.openshift.com/container-platform/4.15/security/certificates/service-serving-certificate.html
- https://issues.redhat.com/browse/OCPBUGS-32139
- https://cert-manager.io/docs/concepts/ca-injector/

Signed-off-by: Andrea Panattoni <[email protected]>

controllers/sriovoperatorconfig_controller_test.go

@@ -327,5 +327,33 @@ 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())
+		})
 	})
 })

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.0
 	github.com/jaypipes/ghw v0.9.0
 	github.com/k8snetworkplumbingwg/network-attachment-definition-client v1.4.0
@@ -86,7 +87,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/hashicorp/go-hclog v1.2.0 // 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,92 @@ func MergeServiceAccountForUpdate(current, updated *uns.Unstructured) error {
 	return nil
 }
 
+// MergeWebhookForUpdate ensures the Webhook.ClientConfig.CABundle is never removed from a webhook
+// if the resource has the `service.beta.openshift.io/inject-cabundle` annotation
+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] {