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 <philip.laine@gmail.com>

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

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 {

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

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 }}
+{{- end }}

internal/webhook/v1/nbsetupkey_webhook.go

@@ -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
-}

internal/webhook/v1/nbsetupkey_webhook_test.go

@@ -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())
-				})
-			})
-		})
-	})
-
-})

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())
 

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() {