From 42905ded9829876e5fa340f1029e59ff9553a5da Mon Sep 17 00:00:00 2001 From: Andrea Panattoni Date: Thu, 13 Jun 2024 10:44:57 +0200 Subject: [PATCH] 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 --- .../sriovoperatorconfig_controller_test.go | 62 +++++++++++++ go.mod | 4 +- pkg/apply/merge.go | 91 +++++++++++++++++++ pkg/apply/merge_test.go | 64 +++++++++++++ test/util/util.go | 19 ++++ 5 files changed, 238 insertions(+), 2 deletions(-) diff --git a/controllers/sriovoperatorconfig_controller_test.go b/controllers/sriovoperatorconfig_controller_test.go index d7cc7fcb9..3432d92cf 100644 --- a/controllers/sriovoperatorconfig_controller_test.go +++ b/controllers/sriovoperatorconfig_controller_test.go @@ -2,6 +2,7 @@ package controllers import ( "context" + "os" "strings" "sync" @@ -21,6 +22,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 +329,65 @@ 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()) + }) }) }) diff --git a/go.mod b/go.mod index fca66390c..8ba0d84b3 100644 --- a/go.mod +++ b/go.mod @@ -11,7 +11,8 @@ 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/hashicorp/go-retryablehttp v0.7.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/k8snetworkplumbingwg/network-attachment-definition-client v1.4.0 github.com/k8snetworkplumbingwg/sriov-network-device-plugin v0.0.0-20221127172732-a5a7395122e3 @@ -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 diff --git a/pkg/apply/merge.go b/pkg/apply/merge.go index a0bbfd146..d2ed6d4fb 100644 --- a/pkg/apply/merge.go +++ b/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) { diff --git a/pkg/apply/merge_test.go b/pkg/apply/merge_test.go index 8079c91ae..f6ad89289 100644 --- a/pkg/apply/merge_test.go +++ b/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() diff --git a/test/util/util.go b/test/util/util.go index 096962b3c..5103af78f 100644 --- a/test/util/util.go +++ b/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] {