From e83dc472e2ec07d33874bbbe9c44c28760a10314 Mon Sep 17 00:00:00 2001 From: Philip Laine Date: Thu, 19 Mar 2026 15:48:38 +0100 Subject: [PATCH] Remove webhook for setup key resource (#148) The webhook for the NBSetupKey does a mix of runtime validation and configuration validation. The validation of secret key ref has been switched to using CEL rules to achieve the same thing. The other logic is just being removed as it is goes against common practice in Kubernetes. We should not block secret deletion if a pod is using the secret. Existing pods will keep running while new pods will not be able to be created. This is expected behavior in other tools and should be dealt with during reconcile. Checking that secrets exist and other dependency problems should be done during reconcile and then bubbled up with conditions instead. Signed-off-by: Philip Laine --- api/v1/nbsetupkey_types.go | 1 + cmd/main.go | 5 - .../crds/netbird.io_nbsetupkeys.yaml | 5 + .../templates/webhook.yaml | 42 +--- internal/webhook/v1/nbsetupkey_webhook.go | 103 --------- .../webhook/v1/nbsetupkey_webhook_test.go | 210 ------------------ internal/webhook/v1/webhook_suite_test.go | 3 - test/e2e/e2e_test.go | 14 -- 8 files changed, 7 insertions(+), 376 deletions(-) delete mode 100644 internal/webhook/v1/nbsetupkey_webhook.go delete mode 100644 internal/webhook/v1/nbsetupkey_webhook_test.go diff --git a/api/v1/nbsetupkey_types.go b/api/v1/nbsetupkey_types.go index 11bf1eb..679aa9e 100644 --- a/api/v1/nbsetupkey_types.go +++ b/api/v1/nbsetupkey_types.go @@ -33,6 +33,7 @@ const ( // NBSetupKeySpec defines the desired state of NBSetupKey. type NBSetupKeySpec struct { // SecretKeyRef is a reference to the secret containing the setup key + // +kubebuilder:validation:XValidation:rule="self.name.size() > 0",reason="FieldValueRequired",message="secret name needs to be set",fieldPath=".name" SecretKeyRef corev1.SecretKeySelector `json:"secretKeyRef"` // ManagementURL optional, override operator management URL ManagementURL string `json:"managementURL,omitempty"` diff --git a/cmd/main.go b/cmd/main.go index 5fbbf21..fa1ba85 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -208,11 +208,6 @@ func main() { setupLog.Error(err, "unable to create webhook", "webhook", "Pod") os.Exit(1) } - - if err = webhooknetbirdiov1.SetupNBSetupKeyWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create webhook", "webhook", "NBSetupKey") - os.Exit(1) - } } if len(netbirdAPIKey) > 0 { diff --git a/helm/kubernetes-operator/crds/netbird.io_nbsetupkeys.yaml b/helm/kubernetes-operator/crds/netbird.io_nbsetupkeys.yaml index f730c43..46b33a3 100644 --- a/helm/kubernetes-operator/crds/netbird.io_nbsetupkeys.yaml +++ b/helm/kubernetes-operator/crds/netbird.io_nbsetupkeys.yaml @@ -67,6 +67,11 @@ spec: - key type: object x-kubernetes-map-type: atomic + x-kubernetes-validations: + - fieldPath: .name + message: secret name needs to be set + reason: FieldValueRequired + rule: self.name.size() > 0 volumeMounts: description: VolumeMounts optional, additional volumeMounts for NetBird container diff --git a/helm/kubernetes-operator/templates/webhook.yaml b/helm/kubernetes-operator/templates/webhook.yaml index e96d8bc..9209de9 100644 --- a/helm/kubernetes-operator/templates/webhook.yaml +++ b/helm/kubernetes-operator/templates/webhook.yaml @@ -51,46 +51,6 @@ webhooks: resources: - pods sideEffects: None ---- -apiVersion: admissionregistration.k8s.io/v1 -kind: ValidatingWebhookConfiguration -metadata: -{{- if $.Values.webhook.enableCertManager }} - annotations: - cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/{{ template "kubernetes-operator.fullname" . }}-serving-cert -{{- end }} - name: {{ include "kubernetes-operator.fullname" . }}-vnbsetupkey-webhook - labels: - {{- include "kubernetes-operator.labels" . | nindent 4 }} -webhooks: -- clientConfig: - {{- if not $.Values.webhook.enableCertManager }} - caBundle: {{ $tls.caCert }} - {{ end }} - service: - name: {{ template "kubernetes-operator.webhookService" . }} - namespace: {{ $.Release.Namespace }} - path: /validate-netbird-io-v1-nbsetupkey - failurePolicy: {{ .Values.webhook.failurePolicy }} - name: vnbsetupkey-v1.netbird.io - admissionReviewVersions: - - v1 - {{- if .Values.webhook.namespaceSelectors }} - namespaceSelector: - matchExpressions: - {{ toYaml .Values.webhook.namespaceSelectors | nindent 4 }} - {{ end }} - rules: - - apiGroups: - - netbird.io - apiVersions: - - v1 - operations: - - CREATE - - UPDATE - resources: - - "nbsetupkeys" - sideEffects: None {{- if and $.Values.ingress.enabled (or .Values.netbirdAPI.key .Values.netbirdAPI.keyFromSecret) }} --- apiVersion: admissionregistration.k8s.io/v1 @@ -172,4 +132,4 @@ metadata: {{ include "kubernetes-operator.labels" . | indent 4 }} spec: selfSigned: {} -{{- end }} \ No newline at end of file +{{- end }} diff --git a/internal/webhook/v1/nbsetupkey_webhook.go b/internal/webhook/v1/nbsetupkey_webhook.go deleted file mode 100644 index 0945156..0000000 --- a/internal/webhook/v1/nbsetupkey_webhook.go +++ /dev/null @@ -1,103 +0,0 @@ -package v1 - -import ( - "context" - "fmt" - "strings" - - "github.com/google/uuid" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/types" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - logf "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - - netbirdiov1 "github.com/netbirdio/kubernetes-operator/api/v1" -) - -// nolint:unused -// log is for logging in this package. -var nbsetupkeylog = logf.Log.WithName("nbsetupkey-resource") - -// SetupNBSetupKeyWebhookWithManager registers the webhook for NBSetupKey in the manager. -func SetupNBSetupKeyWebhookWithManager(mgr ctrl.Manager) error { - return ctrl.NewWebhookManagedBy(mgr, &netbirdiov1.NBSetupKey{}). - WithValidator(&NBSetupKeyCustomValidator{client: mgr.GetClient()}). - Complete() -} - -// NBSetupKeyCustomValidator struct is responsible for validating the NBSetupKey resource -// when it is created, updated, or deleted. -type NBSetupKeyCustomValidator struct { - client client.Client -} - -var _ admission.Validator[*netbirdiov1.NBSetupKey] = &NBSetupKeyCustomValidator{} - -// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type NBSetupKey. -func (v *NBSetupKeyCustomValidator) ValidateCreate(ctx context.Context, nbSetupKey *netbirdiov1.NBSetupKey) (admission.Warnings, error) { - nbsetupkeylog.Info("Validating NBSetupKey", "namespace", nbSetupKey.Namespace, "name", nbSetupKey.Name) - - if nbSetupKey.Spec.SecretKeyRef.Name == "" { - return nil, fmt.Errorf("spec.secretKeyRef.name is required") - } - - if nbSetupKey.Spec.SecretKeyRef.Key == "" { - return nil, fmt.Errorf("spec.secretKeyRef.key is required") - } - - var secret corev1.Secret - err := v.client.Get(ctx, types.NamespacedName{Namespace: nbSetupKey.Namespace, Name: nbSetupKey.Spec.SecretKeyRef.Name}, &secret) - if err != nil { - if errors.IsNotFound(err) { - return admission.Warnings{fmt.Sprintf("secret %s/%s not found", nbSetupKey.Namespace, nbSetupKey.Spec.SecretKeyRef.Name)}, nil - } - return nil, err - } - - uuidBytes, ok := secret.Data[nbSetupKey.Spec.SecretKeyRef.Key] - if !ok { - return admission.Warnings{fmt.Sprintf("key %s in secret %s/%s not found", nbSetupKey.Spec.SecretKeyRef.Key, nbSetupKey.Namespace, nbSetupKey.Spec.SecretKeyRef.Name)}, nil - } - - _, err = uuid.Parse(string(uuidBytes)) - if err != nil { - return admission.Warnings{fmt.Sprintf("setupkey %s in secret %s/%s is not a valid setup key", nbSetupKey.Spec.SecretKeyRef.Key, nbSetupKey.Namespace, nbSetupKey.Spec.SecretKeyRef.Name)}, nil - } - - return nil, nil -} - -// ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type NBSetupKey. -func (v *NBSetupKeyCustomValidator) ValidateUpdate(ctx context.Context, old, new *netbirdiov1.NBSetupKey) (admission.Warnings, error) { - return v.ValidateCreate(ctx, new) -} - -// ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the type NBSetupKey. -func (v *NBSetupKeyCustomValidator) ValidateDelete(ctx context.Context, nbSetupKey *netbirdiov1.NBSetupKey) (admission.Warnings, error) { - nbsetupkeylog.Info("Validating NBSetupKey deletion", "namespace", nbSetupKey.Namespace, "name", nbSetupKey.Name) - - var pods corev1.PodList - err := v.client.List(ctx, &pods, client.InNamespace(nbSetupKey.Namespace)) - if err != nil { - return nil, err - } - - //nolint:prealloc - var invalidPods []string - for _, p := range pods.Items { - // If annotation doesn't exist, or doesn't match NBSetupKey being deleted, ignore - if v, ok := p.Annotations[setupKeyAnnotation]; !ok || v != nbSetupKey.Name { - continue - } - invalidPods = append(invalidPods, p.Name) - } - - if len(invalidPods) > 0 { - return nil, fmt.Errorf("NBSetupKey is in-use by %d pods: %s", len(invalidPods), strings.Join(invalidPods, ",")) - } - - return nil, nil -} diff --git a/internal/webhook/v1/nbsetupkey_webhook_test.go b/internal/webhook/v1/nbsetupkey_webhook_test.go deleted file mode 100644 index e7b9309..0000000 --- a/internal/webhook/v1/nbsetupkey_webhook_test.go +++ /dev/null @@ -1,210 +0,0 @@ -package v1 - -import ( - "context" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" - - netbirdiov1 "github.com/netbirdio/kubernetes-operator/api/v1" -) - -var _ = Describe("NBSetupKey Webhook", func() { - var ( - obj *netbirdiov1.NBSetupKey - validator NBSetupKeyCustomValidator - resourceName = "test" - secret *corev1.Secret - ) - - BeforeEach(func() { - obj = &netbirdiov1.NBSetupKey{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - Namespace: "default", - }, - } - validator = NBSetupKeyCustomValidator{ - client: k8sClient, - } - }) - - Context("When creating or updating NBSetupKey under Validating Webhook", func() { - When("secretKeyRef is empty", func() { - It("Should fail", func() { - obj.Spec = netbirdiov1.NBSetupKeySpec{} - warnings, err := validator.ValidateCreate(context.Background(), obj) - Expect(err).To(HaveOccurred()) - Expect(warnings).To(BeEmpty()) - }) - }) - - When("secret doesn't exist", func() { - It("Should fail", func() { - obj.Spec = netbirdiov1.NBSetupKeySpec{ - SecretKeyRef: corev1.SecretKeySelector{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: resourceName, - }, - Key: "setupkey", - }, - } - warnings, err := validator.ValidateCreate(context.Background(), obj) - Expect(err).NotTo(HaveOccurred()) - Expect(warnings).NotTo(BeEmpty()) - }) - }) - - Context("secret exists", Ordered, func() { - createSecret := func(secretkey, setupkey string) { - resource := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "default", - Name: resourceName, - }, - Data: map[string][]byte{ - secretkey: []byte(setupkey), - }, - } - - secret = &corev1.Secret{} - err := k8sClient.Get(ctx, types.NamespacedName{Namespace: "default", Name: resourceName}, secret) - if err == nil { - Expect(k8sClient.Delete(ctx, secret)).To(Succeed()) - } - Expect(k8sClient.Create(ctx, resource)).To(Succeed()) - } - - BeforeEach(func() { - obj.Spec = netbirdiov1.NBSetupKeySpec{ - SecretKeyRef: corev1.SecretKeySelector{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: resourceName, - }, - Key: "setupkey", - }, - } - }) - - When("secret key doesn't exist", func() { - It("Should fail", func() { - createSecret("wrongkey", "EEEEEEEE-EEEE-EEEE-EEEE-EEEEEEEEEEEE") - warnings, err := validator.ValidateCreate(context.Background(), obj) - Expect(err).NotTo(HaveOccurred()) - Expect(warnings).NotTo(BeEmpty()) - }) - }) - When("setup key is invalid", func() { - It("Should fail", func() { - createSecret("setupkey", "EEEEEEEE") - warnings, err := validator.ValidateCreate(context.Background(), obj) - Expect(err).NotTo(HaveOccurred()) - Expect(warnings).NotTo(BeEmpty()) - }) - }) - When("setup key is valid", func() { - It("Should allow creation", func() { - createSecret("setupkey", "EEEEEEEE-EEEE-EEEE-EEEE-EEEEEEEEEEEE") - warnings, err := validator.ValidateCreate(context.Background(), obj) - Expect(err).NotTo(HaveOccurred()) - Expect(warnings).To(BeEmpty()) - }) - }) - }) - - Context("Delete", func() { - When("No pods exist with annotation", func() { - BeforeEach(func() { - pod := &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "default", - Name: "notannotated", - }, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - {Name: "test", Image: "test"}, - }, - }, - } - Expect(k8sClient.Create(ctx, pod)).To(Succeed()) - }) - - AfterEach(func() { - pod := &corev1.Pod{} - err := k8sClient.Get(ctx, types.NamespacedName{Namespace: "default", Name: "notannotated"}, pod) - if !errors.IsNotFound(err) { - Expect(err).NotTo(HaveOccurred()) - if len(pod.Finalizers) > 0 { - pod.Finalizers = nil - Expect(k8sClient.Update(ctx, pod)).To(Succeed()) - } - err = k8sClient.Delete(ctx, pod) - if !errors.IsNotFound(err) { - Expect(err).NotTo(HaveOccurred()) - } - } - }) - - It("should allow delete", func() { - obj = &netbirdiov1.NBSetupKey{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - Namespace: "default", - }, - } - Expect(validator.ValidateDelete(ctx, obj)).Error().NotTo(HaveOccurred()) - }) - }) - When("Pods exist with annotation", func() { - BeforeEach(func() { - pod := &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "default", - Name: "annotated", - Annotations: map[string]string{ - setupKeyAnnotation: "test", - }, - }, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - {Name: "test", Image: "test"}, - }, - }, - } - Expect(k8sClient.Create(ctx, pod)).To(Succeed()) - }) - - AfterEach(func() { - pod := &corev1.Pod{} - err := k8sClient.Get(ctx, types.NamespacedName{Namespace: "default", Name: "annotated"}, pod) - if !errors.IsNotFound(err) { - Expect(err).NotTo(HaveOccurred()) - if len(pod.Finalizers) > 0 { - pod.Finalizers = nil - Expect(k8sClient.Update(ctx, pod)).To(Succeed()) - } - err = k8sClient.Delete(ctx, pod) - if !errors.IsNotFound(err) { - Expect(err).NotTo(HaveOccurred()) - } - } - }) - - It("should deny delete", func() { - obj = &netbirdiov1.NBSetupKey{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - Namespace: "default", - }, - } - Expect(validator.ValidateDelete(ctx, obj)).Error().To(HaveOccurred()) - }) - }) - }) - }) - -}) diff --git a/internal/webhook/v1/webhook_suite_test.go b/internal/webhook/v1/webhook_suite_test.go index 457ca5e..59c3a8a 100644 --- a/internal/webhook/v1/webhook_suite_test.go +++ b/internal/webhook/v1/webhook_suite_test.go @@ -121,9 +121,6 @@ var _ = BeforeSuite(func() { err = SetupPodWebhookWithManager(mgr, "", "") Expect(err).NotTo(HaveOccurred()) - err = SetupNBSetupKeyWebhookWithManager(mgr) - Expect(err).NotTo(HaveOccurred()) - err = SetupNBGroupWebhookWithManager(mgr) Expect(err).NotTo(HaveOccurred()) diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 20c569c..0a98dd3 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -264,20 +264,6 @@ var _ = Describe("Manager", Ordered, func() { Eventually(verifyCAInjection).Should(Succeed()) }) - It("should have CA injection for validating webhooks", func() { - By("checking CA injection for validating webhooks") - verifyCAInjection := func(g Gomega) { - cmd := exec.Command("kubectl", "get", - "validatingwebhookconfigurations.admissionregistration.k8s.io", - "kubernetes-operator-vnbsetupkey-webhook", - "-o", "go-template={{ range .webhooks }}{{ .clientConfig.caBundle }}{{ end }}") - vwhOutput, err := utils.Run(cmd) - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(len(vwhOutput)).To(BeNumerically(">", 10)) - } - Eventually(verifyCAInjection).Should(Succeed()) - }) - Context("NBSetupKey", Ordered, func() { Describe("Basic functionality", Ordered, func() { BeforeAll(func() {