Merge pull request #530 from fabriziosestito/feat/validate-imagepulls…

…ecret-policyserver

feat: validate PolicyServer and imagePullSecret

config/webhook/manifests.yaml

@@ -117,3 +117,23 @@ webhooks:
     resources:
     - clusteradmissionpolicies
   sideEffects: None
+- admissionReviewVersions:
+  - v1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: system
+      path: /validate-policies-kubewarden-io-v1-policyserver
+  failurePolicy: Fail
+  name: vpolicyserver.kb.io
+  rules:
+  - apiGroups:
+    - policies.kubewarden.io
+    apiVersions:
+    - v1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - policyservers
+  sideEffects: None

internal/pkg/admission/policy-server-deployment.go

@@ -11,13 +11,11 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/kubewarden/kubewarden-controller/internal/pkg/constants"
+	"github.com/kubewarden/kubewarden-controller/internal/pkg/policyserver"
 )
 
 const (
@@ -46,9 +44,9 @@ func (r *Reconciler) reconcilePolicyServerDeployment(ctx context.Context, policy
 	}
 
 	if policyServer.Spec.ImagePullSecret != "" {
-		err := r.policyServerImagePullSecretPresent(ctx, policyServer)
+		err = policyserver.ValidateImagePullSecret(ctx, r.Client, policyServer.Spec.ImagePullSecret, r.DeploymentsNamespace)
 		if err != nil {
-			return err
+			r.Log.Error(err, "error while validating policy-server imagePullSecret")
 		}
 	}
 
@@ -64,35 +62,6 @@ func (r *Reconciler) reconcilePolicyServerDeployment(ctx context.Context, policy
 	return r.updatePolicyServerDeployment(ctx, policyServer, deployment)
 }
 
-func (r *Reconciler) policyServerImagePullSecretPresent(ctx context.Context, policyServer *policiesv1.PolicyServer) error {
-	// By using Unstructured data we force the client to fetch fresh, uncached
-	// data from the API server
-	unstructuredObj := &unstructured.Unstructured{}
-	unstructuredObj.SetGroupVersionKind(schema.GroupVersionKind{
-		Kind:    "Secret",
-		Version: "v1",
-	})
-	err := r.Client.Get(ctx, client.ObjectKey{
-		Namespace: r.DeploymentsNamespace,
-		Name:      policyServer.Spec.ImagePullSecret,
-	}, unstructuredObj)
-	if err != nil {
-		return fmt.Errorf("cannot get spec.ImagePullSecret: %w", err)
-	}
-
-	var secret corev1.Secret
-	err = runtime.DefaultUnstructuredConverter.
-		FromUnstructured(unstructuredObj.UnstructuredContent(), &secret)
-	if err != nil {
-		return fmt.Errorf("spec.ImagePullSecret is not of Kind Secret: %w", err)
-	}
-
-	if secret.Type != "kubernetes.io/dockerconfigjson" {
-		return fmt.Errorf("spec.ImagePullSecret secret \"%s\" is not of type kubernetes.io/dockerconfigjson", secret.Name)
-	}
-	return nil
-}
-
 func (r *Reconciler) updatePolicyServerDeployment(ctx context.Context, policyServer *policiesv1.PolicyServer, newDeployment *appsv1.Deployment) error {
 	originalDeployment := &appsv1.Deployment{}
 	err := r.Client.Get(ctx, client.ObjectKey{

internal/pkg/policyserver/validation.go

@@ -0,0 +1,27 @@
+package policyserver
+
+import (
+	"context"
+	"fmt"
+
+	corev1 "k8s.io/api/core/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+// ValidateImagePullSecret validates that the specified PolicyServer imagePullSecret exists and is of type kubernetes.io/dockerconfigjson
+func ValidateImagePullSecret(ctx context.Context, k8sClient client.Client, imagePullSecret string, deploymentsNamespace string) error {
+	secret := &corev1.Secret{}
+	err := k8sClient.Get(ctx, client.ObjectKey{
+		Namespace: deploymentsNamespace,
+		Name:      imagePullSecret,
+	}, secret)
+	if err != nil {
+		return fmt.Errorf("cannot get spec.ImagePullSecret: %w", err)
+	}
+
+	if secret.Type != "kubernetes.io/dockerconfigjson" {
+		return fmt.Errorf("spec.ImagePullSecret secret \"%s\" is not of type kubernetes.io/dockerconfigjson", secret.Name)
+	}
+
+	return nil
+}

internal/pkg/policyserver/validation_test.go

@@ -0,0 +1,71 @@
+package policyserver
+
+import (
+	"context"
+	"testing"
+
+	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+)
+
+func TestValidateImagePullSecret(t *testing.T) {
+	ctx := context.Background()
+	deploymentsNamespace := "test"
+
+	tests := []struct {
+		name            string
+		imagePullSecret string
+		secret          *v1.Secret
+		valid           bool
+	}{
+		{
+			"non existing secret",
+			"test",
+			nil,
+			false,
+		},
+		{
+			"secret of wrong type",
+			"test",
+			&v1.Secret{
+				Type: "Opaque",
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+				},
+			},
+			false,
+		},
+		{
+			"valid secret",
+			"test",
+			&v1.Secret{
+				Type: "kubernetes.io/dockerconfigjson",
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+				},
+			},
+			false,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			k8sClient := fake.NewClientBuilder().Build()
+
+			if test.secret != nil {
+				err := k8sClient.Create(ctx, test.secret)
+				if err != nil {
+					t.Errorf("failed to create secret: %s", err.Error())
+				}
+			}
+
+			err := ValidateImagePullSecret(ctx, k8sClient, test.imagePullSecret, deploymentsNamespace)
+			if err != nil && test.valid {
+				t.Errorf("unexpected error: %s", err.Error())
+			} else if err == nil && !test.valid {
+				t.Errorf("expected error, got nil")
+			}
+		})
+	}
+}

main.go

@@ -181,7 +181,7 @@ func main() {
 		setupLog,
 		environment.developmentMode,
 		environment.webhookHostAdvertise,
-		webhooks(),
+		webhooks(deploymentsNamespace),
 	)
 	if err != nil {
 		setupLog.Error(err, "unable to start manager")
@@ -252,10 +252,10 @@ func main() {
 	}
 }
 
-func webhooks() []webhookwrapper.WebhookRegistrator {
+func webhooks(deploymentsNamespace string) []webhookwrapper.WebhookRegistrator {
 	return []webhookwrapper.WebhookRegistrator{
 		{
-			Registrator: (&policiesv1.PolicyServer{}).SetupWebhookWithManager,
+			Registrator: policiesv1.SetupPolicyServerWebhookWithManager(deploymentsNamespace),
 			Name:        "mutate-policyservers.kubewarden.dev",
 			RulesWithOperations: []admissionregistrationv1.RuleWithOperations{
 				{
@@ -273,6 +273,25 @@ func webhooks() []webhookwrapper.WebhookRegistrator {
 			WebhookPath: "/mutate-policies-kubewarden-io-v1-policyserver",
 			Mutating:    true,
 		},
+		{
+			Registrator: policiesv1.SetupPolicyServerWebhookWithManager(deploymentsNamespace),
+			Name:        "validate-policyservers.kubewarden.dev",
+			RulesWithOperations: []admissionregistrationv1.RuleWithOperations{
+				{
+					Operations: []admissionregistrationv1.OperationType{
+						admissionregistrationv1.Create,
+						admissionregistrationv1.Update,
+					},
+					Rule: admissionregistrationv1.Rule{
+						APIGroups:   []string{policiesv1.GroupVersion.Group},
+						APIVersions: []string{policiesv1.GroupVersion.Version},
+						Resources:   []string{"policyservers"},
+					},
+				},
+			},
+			WebhookPath: "/validate-policies-kubewarden-io-v1-policyserver",
+			Mutating:    true,
+		},
 		{
 			Registrator: (&policiesv1.ClusterAdmissionPolicy{}).SetupWebhookWithManager,
 			Name:        "mutate-clusteradmissionpolicies.kubewarden.dev",

pkg/apis/policies/v1/clusteradmissionpolicy_types_test.go

@@ -6,19 +6,19 @@ import (
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
+const kubewardenNamespace = "kubewarden"
+
 func TestGetNamespaceSelectorWithEmptyNamespaceSelector(t *testing.T) {
-	kubewardenNs := "kubewarden"
 	c := ClusterAdmissionPolicy{}
-	nsSelector := c.GetUpdatedNamespaceSelector(kubewardenNs)
-	isKubewardenNsFound := isNamespaceFoundInSelector(nsSelector, kubewardenNs)
+	nsSelector := c.GetUpdatedNamespaceSelector(kubewardenNamespace)
+	isKubewardenNsFound := isNamespaceFoundInSelector(nsSelector, kubewardenNamespace)
 
 	if !isKubewardenNsFound {
 		t.Errorf("Kubewarden namespace not added to namespace selector")
 	}
 }
 
 func TestGetNamespaceSelectorWithExistingMatchExpressions(t *testing.T) {
-	kubewardenNs := "kubewarden"
 	policy := ClusterAdmissionPolicy{
 		Spec: ClusterAdmissionPolicySpec{
 			NamespaceSelector: &v1.LabelSelector{
@@ -32,8 +32,8 @@ func TestGetNamespaceSelectorWithExistingMatchExpressions(t *testing.T) {
 			},
 		},
 	}
-	nsSelector := policy.GetUpdatedNamespaceSelector(kubewardenNs)
-	isKubewardenNsFound := isNamespaceFoundInSelector(nsSelector, kubewardenNs)
+	nsSelector := policy.GetUpdatedNamespaceSelector(kubewardenNamespace)
+	isKubewardenNsFound := isNamespaceFoundInSelector(nsSelector, kubewardenNamespace)
 
 	if !isKubewardenNsFound {
 		t.Errorf("Kubewarden namespace not added to namespace selector")

pkg/apis/policies/v1/policyserver_webhook.go

@@ -17,10 +17,15 @@ limitations under the License.
 package v1
 
 import (
+	"context"
 	"fmt"
 
 	"github.com/kubewarden/kubewarden-controller/internal/pkg/constants"
+	"github.com/kubewarden/kubewarden-controller/internal/pkg/policyserver"
+	"k8s.io/apimachinery/pkg/runtime"
+	validationutils "k8s.io/apimachinery/pkg/util/validation"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
@@ -29,17 +34,25 @@ import (
 // log is for logging in this package.
 var policyserverlog = logf.Log.WithName("policyserver-resource")
 
-func (ps *PolicyServer) SetupWebhookWithManager(mgr ctrl.Manager) error {
-	err := ctrl.NewWebhookManagedBy(mgr).
-		For(ps).
-		Complete()
-	if err != nil {
-		return fmt.Errorf("failed enrolling webhook with manager: %w", err)
+// SetupPolicyServerWebhookWithManager returns a function that can be used to setup the PolicyServer webhook with the manager.
+// This is needed since we are using github.com/ereslibre/kube-webhook-wrapper and will be removed once we no longer use it.
+func SetupPolicyServerWebhookWithManager(deploymentsNamespace string) func(mgr ctrl.Manager) error {
+	return func(mgr ctrl.Manager) error {
+		ps := &PolicyServer{}
+
+		err := ctrl.NewWebhookManagedBy(mgr).
+			For(ps).
+			WithValidator(&policyServerValidator{k8sClient: mgr.GetClient(), deploymentsNamespace: deploymentsNamespace}).
+			Complete()
+		if err != nil {
+			return fmt.Errorf("failed enrolling webhook with manager: %w", err)
+		}
+
+		return nil
 	}
-	return nil
 }
 
-//+kubebuilder:webhook:path=/mutate-policies-kubewarden-io-v1-policyserver,mutating=true,failurePolicy=fail,sideEffects=None,groups=policies.kubewarden.io,resources=policyservers,verbs=create;update,versions=v1,name=mpolicyserver.kb.io,admissionReviewVersions={v1,v1beta1}
+// +kubebuilder:webhook:path=/mutate-policies-kubewarden-io-v1-policyserver,mutating=true,failurePolicy=fail,sideEffects=None,groups=policies.kubewarden.io,resources=policyservers,verbs=create;update,versions=v1,name=mpolicyserver.kb.io,admissionReviewVersions={v1,v1beta1}
 
 var _ webhook.Defaulter = &PolicyServer{}
 
@@ -50,3 +63,44 @@ func (ps *PolicyServer) Default() {
 		controllerutil.AddFinalizer(ps, constants.KubewardenFinalizer)
 	}
 }
+
+// +kubebuilder:webhook:path=/validate-policies-kubewarden-io-v1-policyserver,mutating=false,failurePolicy=fail,sideEffects=None,groups=policies.kubewarden.io,resources=policyservers,verbs=create;update,versions=v1,name=vpolicyserver.kb.io,admissionReviewVersions=v1
+
+// polyServerValidator validates PolicyServers
+type policyServerValidator struct {
+	k8sClient            client.Client
+	deploymentsNamespace string
+}
+
+func (v *policyServerValidator) validate(ctx context.Context, obj runtime.Object) error {
+	policyServer, ok := obj.(*PolicyServer)
+	if !ok {
+		return fmt.Errorf("expected a PolicyServer object, got %T", obj)
+	}
+
+	// The PolicyServer name must be maximum 63 like all Kubernetes objects to fit in a DNS subdomain name
+	if len(policyServer.GetName()) > validationutils.DNS1035LabelMaxLength {
+		return fmt.Errorf("the PolicyServer name cannot be longer than %d characters", validationutils.DNS1035LabelMaxLength)
+	}
+
+	if policyServer.Spec.ImagePullSecret != "" {
+		err := policyserver.ValidateImagePullSecret(ctx, v.k8sClient, policyServer.Spec.ImagePullSecret, v.deploymentsNamespace)
+		if err != nil {
+			return fmt.Errorf("spec.ImagePullSecret is invalid: %w", err)
+		}
+	}
+
+	return nil
+}
+
+func (v *policyServerValidator) ValidateCreate(ctx context.Context, obj runtime.Object) error {
+	return v.validate(ctx, obj)
+}
+
+func (v *policyServerValidator) ValidateUpdate(ctx context.Context, _, obj runtime.Object) error {
+	return v.validate(ctx, obj)
+}
+
+func (v *policyServerValidator) ValidateDelete(_ context.Context, _ runtime.Object) error {
+	return nil
+}