chore: Add logic and test for validation webhook

Signed-off-by: Teddy Andrieux <teddy.andrieux@scality.com>

Author: TeddyAndrieux

config/manager/kustomization.yaml

@@ -1,2 +1,8 @@
 resources:
 - manager.yaml
+apiVersion: kustomize.config.k8s.io/v1beta1
+kind: Kustomization
+images:
+- name: controller
+  newName: example.com/crl-operator
+  newTag: v0.0.1

internal/webhook/v1alpha1/managedcrl_webhook.go

@@ -20,12 +20,15 @@ import (
 	"context"
 	"fmt"
 
+	cmv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	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"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
+	"github.com/go-logr/logr"
 	crloperatorv1alpha1 "github.com/scality/crl-operator/api/v1alpha1"
 )
 
@@ -36,7 +39,9 @@ var managedcrllog = logf.Log.WithName("managedcrl-resource")
 // SetupManagedCRLWebhookWithManager registers the webhook for ManagedCRL in the manager.
 func SetupManagedCRLWebhookWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewWebhookManagedBy(mgr).For(&crloperatorv1alpha1.ManagedCRL{}).
-		WithValidator(&ManagedCRLCustomValidator{}).
+		WithValidator(&ManagedCRLCustomValidator{
+			client: mgr.GetClient(),
+		}).
 		Complete()
 }
 
@@ -53,35 +58,34 @@ func SetupManagedCRLWebhookWithManager(mgr ctrl.Manager) error {
 // NOTE: The +kubebuilder:object:generate=false marker prevents controller-gen from generating DeepCopy methods,
 // as this struct is used only for temporary operations and does not need to be deeply copied.
 type ManagedCRLCustomValidator struct {
-	// TODO(user): Add more fields as needed for validation
+	client client.Client
 }
 
 var _ webhook.CustomValidator = &ManagedCRLCustomValidator{}
 
 // ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type ManagedCRL.
-func (v *ManagedCRLCustomValidator) ValidateCreate(_ context.Context, obj runtime.Object) (admission.Warnings, error) {
+func (v *ManagedCRLCustomValidator) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
 	managedcrl, ok := obj.(*crloperatorv1alpha1.ManagedCRL)
 	if !ok {
 		return nil, fmt.Errorf("expected a ManagedCRL object but got %T", obj)
 	}
-	managedcrllog.Info("Validation for ManagedCRL upon creation", "name", managedcrl.GetName())
+	logger := managedcrllog.WithValues("name", managedcrl.GetName()).WithValues("namespace", managedcrl.GetNamespace())
 
-	// TODO(user): fill in your validation logic upon object creation.
+	logger.Info("Validation for ManagedCRL upon creation")
 
-	return nil, nil
+	return nil, validationManagedCRL(logger, ctx, v.client, managedcrl)
 }
 
 // ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type ManagedCRL.
-func (v *ManagedCRLCustomValidator) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
+func (v *ManagedCRLCustomValidator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
 	managedcrl, ok := newObj.(*crloperatorv1alpha1.ManagedCRL)
 	if !ok {
 		return nil, fmt.Errorf("expected a ManagedCRL object for the newObj but got %T", newObj)
 	}
-	managedcrllog.Info("Validation for ManagedCRL upon update", "name", managedcrl.GetName())
-
-	// TODO(user): fill in your validation logic upon object update.
+	logger := managedcrllog.WithValues("name", managedcrl.GetName()).WithValues("namespace", managedcrl.GetNamespace())
+	logger.Info("Validation for ManagedCRL upon update")
 
-	return nil, nil
+	return nil, validationManagedCRL(logger, ctx, v.client, managedcrl)
 }
 
 // ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the type ManagedCRL.
@@ -96,3 +100,49 @@ func (v *ManagedCRLCustomValidator) ValidateDelete(ctx context.Context, obj runt
 
 	return nil, nil
 }
+
+// validationManagedCRL validates the ManagedCRL fields.
+func validationManagedCRL(logger logr.Logger, ctx context.Context, c client.Client, managedcrl *crloperatorv1alpha1.ManagedCRL) error {
+	managedcrl.WithDefaults()
+	if err := managedcrl.Validate(); err != nil {
+		logger.Error(err, "Validation failed")
+		return err
+	}
+
+	// Ensure the specified Issuer or ClusterIssuer exists
+	issuerLogger := logger.WithValues("issuer_name", managedcrl.Spec.IssuerRef.Name, "issuer_kind", managedcrl.Spec.IssuerRef.Kind)
+	issuerRef := managedcrl.Spec.IssuerRef
+	switch issuerRef.Kind {
+	case "Issuer":
+		var issuer cmv1.Issuer
+		err := c.Get(ctx, client.ObjectKey{Namespace: managedcrl.Namespace, Name: issuerRef.Name}, &issuer)
+		if err != nil {
+			issuerLogger.Error(err, "Issuer not found")
+			return fmt.Errorf("issuer %s not found in namespace %s", issuerRef.Name, managedcrl.Namespace)
+		}
+		if issuer.Spec.CA == nil || issuer.Spec.CA.SecretName == "" {
+			err := fmt.Errorf("issuer %s in namespace %s is not a CA issuer", issuerRef.Name, managedcrl.Namespace)
+			issuerLogger.Error(err, "Issuer is not a CA issuer")
+			return err
+		}
+	case "ClusterIssuer":
+		var issuer cmv1.ClusterIssuer
+		err := c.Get(ctx, client.ObjectKey{Name: issuerRef.Name}, &issuer)
+		if err != nil {
+			issuerLogger.Error(err, "ClusterIssuer not found")
+			return fmt.Errorf("clusterissuer %s not found", issuerRef.Name)
+		}
+		if issuer.Spec.CA == nil || issuer.Spec.CA.SecretName == "" {
+			err := fmt.Errorf("clusterissuer %s is not a CA issuer", issuerRef.Name)
+			issuerLogger.Error(err, "Issuer is not a CA issuer")
+			return err
+		}
+	default:
+		err := fmt.Errorf("invalid IssuerRef kind: %s", issuerRef.Kind)
+		issuerLogger.Error(err, "IssuerRef kind must be either 'Issuer' or 'ClusterIssuer'")
+		return err
+	}
+
+	logger.Info("Validation successful")
+	return nil
+}

test/integration/managedcrl_controller_test.go

@@ -21,6 +21,7 @@ import (
 	"crypto/x509"
 	"fmt"
 	"math/big"
+	"strings"
 	"time"
 
 	. "github.com/onsi/ginkgo/v2"
@@ -32,6 +33,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	cmv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
+	cmmetav1 "github.com/cert-manager/cert-manager/pkg/apis/meta/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
@@ -42,6 +44,7 @@ type mcrlTestCase struct {
 	name                  string
 	spec                  crloperatorv1alpha1.ManagedCRLSpec
 	shouldError           bool
+	errorMessage          string
 	shouldExposePod       bool
 	shouldExposeIngress   bool
 	shouldConfigureIssuer bool
@@ -177,6 +180,61 @@ var (
 			shouldExposePod:       true,
 			shouldExposeIngress:   true,
 			shouldConfigureIssuer: true,
+		}, {
+			name: "error-invalid-issuer-kind",
+			spec: crloperatorv1alpha1.ManagedCRLSpec{
+				IssuerRef: cmmetav1.IssuerReference{
+					Kind: "InvalidKind",
+				},
+			},
+			shouldError:  true,
+			errorMessage: "issuerRef kind must be either 'Issuer' or 'ClusterIssuer', got 'InvalidKind'",
+		}, {
+			name: "error-non-existent-issuer",
+			spec: crloperatorv1alpha1.ManagedCRLSpec{
+				IssuerRef: cmmetav1.IssuerReference{
+					Name: "non-existent-issuer",
+				},
+			},
+			shouldError:  true,
+			errorMessage: "issuer non-existent-issuer not found",
+		}, {
+			name: "error-non-ca-issuer",
+			spec: crloperatorv1alpha1.ManagedCRLSpec{
+				IssuerRef: cmmetav1.IssuerReference{
+					Name: "test-issuer-non-ca",
+				},
+			},
+			shouldError:  true,
+			errorMessage: "issuer test-issuer-non-ca .*is not a CA issuer",
+		}, {
+			name: "error-too-small-duration",
+			spec: crloperatorv1alpha1.ManagedCRLSpec{
+				Duration: &metav1.Duration{Duration: time.Hour},
+			},
+			shouldError:  true,
+			errorMessage: "duration must be at least 24h",
+		}, {
+			name: "error-invalid-serial-number",
+			spec: crloperatorv1alpha1.ManagedCRLSpec{
+				Revocations: []crloperatorv1alpha1.RevocationSpec{
+					{
+						SerialNumber: "invalid-serial",
+					},
+				},
+			},
+			shouldError:  true,
+			errorMessage: "invalid serial number: invalid-serial",
+		}, {
+			name: "error-empty-ingress",
+			spec: crloperatorv1alpha1.ManagedCRLSpec{
+				Expose: &crloperatorv1alpha1.CRLExposeSpec{
+					Enabled: true,
+					Ingress: &crloperatorv1alpha1.IngressSpec{},
+				},
+			},
+			shouldError:  true,
+			errorMessage: "invalid ingress configuration: either hostname or ipAddresses must be specified",
 		},
 	}
 )
@@ -186,15 +244,19 @@ func toTableEntry(tcs []mcrlTestCase) []TableEntry {
 	for _, tc := range tcs {
 		// Always add one entry for Issuer and one for ClusterIssuer
 		name := tc.name
-		tc.spec.IssuerRef.Name = "test-issuer"
-
-		tc.name = fmt.Sprintf("%s-issuer", name)
-		tc.spec.IssuerRef.Kind = "Issuer"
-		entries = append(entries, Entry(fmt.Sprintf("ManagedCRL %s", tc.name), tc))
+		if tc.spec.IssuerRef.Name == "" {
+			tc.spec.IssuerRef.Name = "test-issuer"
+		}
+		issuerKindToTest := []string{"Issuer", "ClusterIssuer"}
+		if tc.spec.IssuerRef.Kind != "" {
+			issuerKindToTest = []string{tc.spec.IssuerRef.Kind}
+		}
 
-		tc.name = fmt.Sprintf("%s-clusterissuer", name)
-		tc.spec.IssuerRef.Kind = "ClusterIssuer"
-		entries = append(entries, Entry(fmt.Sprintf("ManagedCRL %s", tc.name), tc))
+		for _, kind := range issuerKindToTest {
+			tc.name = fmt.Sprintf("%s-%s", name, strings.ToLower(kind))
+			tc.spec.IssuerRef.Kind = kind
+			entries = append(entries, Entry(fmt.Sprintf("ManagedCRL %s", tc.name), tc))
+		}
 	}
 	return entries
 }
@@ -231,6 +293,19 @@ var _ = Describe("ManagedCRL Controller", func() {
 					},
 				},
 			)).To(Succeed())
+			Expect(k8sClient.Create(
+				ctx,
+				&cmv1.ClusterIssuer{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test-issuer-non-ca",
+					},
+					Spec: cmv1.IssuerSpec{
+						IssuerConfig: cmv1.IssuerConfig{
+							SelfSigned: &cmv1.SelfSignedIssuer{},
+						},
+					},
+				},
+			)).To(Succeed())
 			Expect(k8sClient.Create(
 				ctx,
 				&corev1.Secret{
@@ -261,6 +336,20 @@ var _ = Describe("ManagedCRL Controller", func() {
 					},
 				},
 			)).To(Succeed())
+			Expect(k8sClient.Create(
+				ctx,
+				&cmv1.Issuer{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "test-issuer-non-ca",
+						Namespace: testNamespace,
+					},
+					Spec: cmv1.IssuerSpec{
+						IssuerConfig: cmv1.IssuerConfig{
+							SelfSigned: &cmv1.SelfSignedIssuer{},
+						},
+					},
+				},
+			)).To(Succeed())
 		})
 
 		AfterEach(func() {
@@ -275,6 +364,11 @@ var _ = Describe("ManagedCRL Controller", func() {
 					Name: "test-issuer",
 				},
 			})).To(Succeed())
+			Expect(k8sClient.Delete(ctx, &cmv1.ClusterIssuer{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-issuer-non-ca",
+				},
+			})).To(Succeed())
 		})
 
 		DescribeTableSubtree("should reconcile various ManagedCRL resources as expected", func(tc mcrlTestCase) {
@@ -295,6 +389,7 @@ var _ = Describe("ManagedCRL Controller", func() {
 				err := k8sClient.Create(ctx, managedcrl)
 				if tc.shouldError {
 					Expect(err).To(HaveOccurred())
+					Expect(err).To(MatchError(MatchRegexp(tc.errorMessage)))
 					return
 				}
 				Expect(err).ToNot(HaveOccurred())

test/integration/suite_test.go

@@ -24,6 +24,7 @@ import (
 	"os"
 	"path/filepath"
 	"testing"
+	"time"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -38,9 +39,11 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
+	"sigs.k8s.io/controller-runtime/pkg/webhook"
 
 	crloperatorv1alpha1 "github.com/scality/crl-operator/api/v1alpha1"
 	"github.com/scality/crl-operator/internal/controller"
+	webhookv1alpha1 "github.com/scality/crl-operator/internal/webhook/v1alpha1"
 	// +kubebuilder:scaffold:imports
 )
 
@@ -137,6 +140,9 @@ var _ = BeforeSuite(func() {
 			filepath.Join("..", "..", "testdata", "crds"),
 		},
 		ErrorIfCRDPathMissing: true,
+		WebhookInstallOptions: envtest.WebhookInstallOptions{
+			Paths: []string{filepath.Join("..", "..", "config", "webhook")},
+		},
 	}
 
 	// Retrieve the first found binary directory to allow running tests from IDEs
@@ -155,8 +161,14 @@ var _ = BeforeSuite(func() {
 
 	k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{
 		Scheme: scheme.Scheme,
+		WebhookServer: webhook.NewServer(webhook.Options{
+			Host:    testEnv.WebhookInstallOptions.LocalServingHost,
+			Port:    testEnv.WebhookInstallOptions.LocalServingPort,
+			CertDir: testEnv.WebhookInstallOptions.LocalServingCertDir,
+		}),
 	})
 	Expect(err).ToNot(HaveOccurred())
+	Expect(webhookv1alpha1.SetupManagedCRLWebhookWithManager(k8sManager)).To(Succeed())
 
 	// Create default cert-manager namespace
 	Expect(k8sClient.Create(
@@ -234,6 +246,16 @@ var _ = BeforeSuite(func() {
 		err = k8sManager.Start(ctx)
 		Expect(err).ToNot(HaveOccurred())
 	}()
+
+	By("waiting for webhook server to be ready")
+	Eventually(func() error {
+		webhookServer := k8sManager.GetWebhookServer()
+		err := webhookServer.StartedChecker()(nil)
+		if err != nil {
+			return fmt.Errorf("webhook server not started: %w", err)
+		}
+		return nil
+	}, 10*time.Second, time.Second).Should(Succeed())
 })
 
 var _ = AfterSuite(func() {