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
```

Refs:
- https://docs.openshift.com/container-platform/4.15/security/certificates/service-serving-certificate.html
- https://issues.redhat.com/browse/OCPBUGS-32139

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

controllers/sriovoperatorconfig_controller_test.go

@@ -21,6 +21,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"
 )
 
@@ -327,5 +328,59 @@ var _ = Describe("SriovOperatorConfig controller", Ordered, func() {
 			})
 			Expect(err).ToNot(HaveOccurred())
 		})
+
+		Context("On OpenShift clusters", Ordered, func() {
+			BeforeAll(func() {
+				By("Temporary set cluster type to openshift")
+				DeferCleanup(func(prev string) {
+					By("Restore global vars.ClusterType to " + prev)
+					vars.ClusterType = prev
+
+					err := util.TriggerSriovOperatorConfigReconcile(k8sClient, testNamespace)
+					Expect(err).NotTo(HaveOccurred())
+				}, vars.ClusterType)
+
+				vars.ClusterType = "openshift"
+
+				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.Annotations).ToNot(BeNil())
+					g.Expect(validateCfg.Annotations["service.beta.openshift.io/inject-cabundle"]).ToNot(BeEmpty())
+				}).Should(Succeed())
+			})
+
+			// 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
+			It("should not remove the field Spec.ClientConfig.CABundle from webhook configuration when reconciling", func() {
+				By("Simulate a third party component updating the webhook CABundle")
+				validateCfg := &admv1.ValidatingWebhookConfiguration{}
+				err := k8sClient.Get(ctx, types.NamespacedName{Namespace: testNamespace, Name: "sriov-operator-webhook-config"}, validateCfg)
+				Expect(err).NotTo(HaveOccurred())
+
+				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
@@ -82,7 +83,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,103 @@ 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
+	}
+
+	current.GetAnnotations()
+	currentAnnotations := current.GetAnnotations()
+	if currentAnnotations == nil {
+		return nil
+	}
+
+	injectCABundle, ok := currentAnnotations["service.beta.openshift.io/inject-cabundle"]
+	if !ok || injectCABundle != "true" {
+		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] {