diff --git a/controllers/policyserver_controller.go b/controllers/policyserver_controller.go index 3516beae..e732695f 100644 --- a/controllers/policyserver_controller.go +++ b/controllers/policyserver_controller.go @@ -107,10 +107,6 @@ func (r *PolicyServerReconciler) reconcileDeletion(ctx context.Context, policySe return r.deletePoliciesAndRequeue(ctx, policyServer, policies) } - if err := r.Reconciler.ReconcileDeletion(ctx, policyServer); err != nil { - return ctrl.Result{}, errors.Join(errors.New("could not reconcile policy server deletion"), err) - } - controllerutil.RemoveFinalizer(policyServer, constants.KubewardenFinalizer) if err := r.Update(ctx, policyServer); err != nil { // return if PolicyServer was previously deleted diff --git a/controllers/policyserver_controller_test.go b/controllers/policyserver_controller_test.go index 48a952d1..750d001d 100644 --- a/controllers/policyserver_controller_test.go +++ b/controllers/policyserver_controller_test.go @@ -20,8 +20,9 @@ import ( "fmt" "time" - . "github.com/onsi/ginkgo/v2" //nolint:revive - . "github.com/onsi/gomega" //nolint:revive + . "github.com/onsi/ginkgo/v2" //nolint:revive + . "github.com/onsi/gomega" //nolint:revive + . "github.com/onsi/gomega/gstruct" //nolint:revive corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/util/intstr" @@ -47,6 +48,94 @@ var _ = Describe("PolicyServer controller", func() { }, timeout, pollInterval).Should(Succeed()) }) + It("should create deployment with owner reference", func() { + Eventually(func() error { + deployment, err := getTestPolicyServerDeployment(policyServerName) + if err != nil { + return err + } + policyServer, err := getTestPolicyServer(policyServerName) + if err != nil { + return err + } + Expect(deployment.OwnerReferences).To(ContainElement( + MatchFields(IgnoreExtras, Fields{ + "UID": Equal(policyServer.GetUID()), + "Name": Equal(policyServer.GetName()), + "Kind": Equal(policyServer.GetObjectKind().GroupVersionKind().Kind), + "APIVersion": Equal(policyServer.GetObjectKind().GroupVersionKind().GroupVersion().String()), + }), + )) + return nil + }).Should(Succeed()) + }) + + It("should create configmap with owner reference", func() { + Eventually(func() error { + configmap, err := getTestPolicyServerConfigMap(policyServerName) + if err != nil { + return err + } + policyServer, err := getTestPolicyServer(policyServerName) + if err != nil { + return err + } + Expect(configmap.OwnerReferences).To(ContainElement( + MatchFields(IgnoreExtras, Fields{ + "UID": Equal(policyServer.GetUID()), + "Name": Equal(policyServer.GetName()), + "Kind": Equal(policyServer.GetObjectKind().GroupVersionKind().Kind), + "APIVersion": Equal(policyServer.GetObjectKind().GroupVersionKind().GroupVersion().String()), + }), + )) + return nil + }).Should(Succeed()) + }) + + It("should create service with owner reference", func() { + Eventually(func() error { + service, err := getTestPolicyServerService(policyServerName) + if err != nil { + return err + } + policyServer, err := getTestPolicyServer(policyServerName) + if err != nil { + return err + } + Expect(service.OwnerReferences).To(ContainElement( + MatchFields(IgnoreExtras, Fields{ + "UID": Equal(policyServer.GetUID()), + "Name": Equal(policyServer.GetName()), + "Kind": Equal(policyServer.GetObjectKind().GroupVersionKind().Kind), + "APIVersion": Equal(policyServer.GetObjectKind().GroupVersionKind().GroupVersion().String()), + }), + )) + return nil + }).Should(Succeed()) + }) + + It("should create secret with owner reference", func() { + Eventually(func() error { + secret, err := getTestPolicyServerSecret(policyServerName) + if err != nil { + return err + } + policyServer, err := getTestPolicyServer(policyServerName) + if err != nil { + return err + } + Expect(secret.OwnerReferences).To(ContainElement( + MatchFields(IgnoreExtras, Fields{ + "UID": Equal(policyServer.GetUID()), + "Name": Equal(policyServer.GetName()), + "Kind": Equal(policyServer.GetObjectKind().GroupVersionKind().Kind), + "APIVersion": Equal(policyServer.GetObjectKind().GroupVersionKind().GroupVersion().String()), + }), + )) + return nil + }).Should(Succeed()) + }) + Context("with no assigned policies", func() { It("should get its finalizer removed", func() { By("deleting the policy server") @@ -137,16 +226,47 @@ var _ = Describe("PolicyServer controller", func() { return err }, timeout, pollInterval).ShouldNot(Succeed()) - Eventually(func() error { - _, err := getTestPolicyServerService(policyServerName) - return err - }, timeout, pollInterval).ShouldNot(Succeed()) - Eventually(func() (*policiesv1.PolicyServer, error) { return getTestPolicyServer(policyServerName) }, timeout, pollInterval).ShouldNot( HaveField("Finalizers", ContainElement(constants.KubewardenFinalizer)), ) + + }) + + It("policy server resources should be gone after it being deleted", func() { + // It's necessary remove the test finalizer to make the + // Kubernetes garbage collector to remove the resources + policyServer, err := getTestPolicyServer(policyServerName) + Expect(err).Should(Succeed()) + controllerutil.RemoveFinalizer(policyServer, IntegrationTestsFinalizer) + err = reconciler.Client.Update(ctx, policyServer) + Expect(err).ToNot(HaveOccurred()) + + Eventually(func() error { + _, err := getTestPolicyServer(policyServerName) + return err + }, timeout, pollInterval).ShouldNot(Succeed()) + + Eventually(func() error { + _, err := getTestPolicyServerService(policyServerName) + return err + }, timeout, pollInterval).ShouldNot(Succeed()) + + Eventually(func() error { + _, err := getTestPolicyServerSecret(policyServerName) + return err + }, timeout, pollInterval).ShouldNot(Succeed()) + + Eventually(func() error { + _, err := getTestPolicyServerDeployment(policyServerName) + return err + }, timeout, pollInterval).ShouldNot(Succeed()) + + Eventually(func() error { + _, err := getTestPolicyServerConfigMap(policyServerName) + return err + }, timeout, pollInterval).ShouldNot(Succeed()) }) }) }) diff --git a/controllers/utils_test.go b/controllers/utils_test.go index f7afac1e..ca363782 100644 --- a/controllers/utils_test.go +++ b/controllers/utils_test.go @@ -155,13 +155,7 @@ func getTestPolicyServer(name string) (*policiesv1.PolicyServer, error) { } func getTestPolicyServerService(policyServerName string) (*corev1.Service, error) { - policyServer := policiesv1.PolicyServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: policyServerName, - }, - } - serviceName := policyServer.NameWithPrefix() - + serviceName := getPolicyServerNameWithPrefix(policyServerName) service := corev1.Service{} if err := reconciler.APIReader.Get(ctx, client.ObjectKey{Name: serviceName, Namespace: DeploymentsNamespace}, &service); err != nil { return nil, errors.Join(errors.New("could not find Service owned by PolicyServer"), err) @@ -169,14 +163,17 @@ func getTestPolicyServerService(policyServerName string) (*corev1.Service, error return &service, nil } -func getTestPolicyServerDeployment(policyServerName string) (*appsv1.Deployment, error) { - policyServer := policiesv1.PolicyServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: policyServerName, - }, +func getTestPolicyServerSecret(policyServerName string) (*corev1.Secret, error) { + secretName := getPolicyServerNameWithPrefix(policyServerName) + secret := corev1.Secret{} + if err := reconciler.APIReader.Get(ctx, client.ObjectKey{Name: secretName, Namespace: DeploymentsNamespace}, &secret); err != nil { + return nil, errors.Join(errors.New("could not find secret owned by PolicyServer"), err) } - deploymentName := policyServer.NameWithPrefix() + return &secret, nil +} +func getTestPolicyServerDeployment(policyServerName string) (*appsv1.Deployment, error) { + deploymentName := getPolicyServerNameWithPrefix(policyServerName) deployment := appsv1.Deployment{} if err := reconciler.APIReader.Get(ctx, client.ObjectKey{Name: deploymentName, Namespace: DeploymentsNamespace}, &deployment); err != nil { return nil, errors.Join(errors.New("could not find Deployment owned by PolicyServer"), err) @@ -184,6 +181,25 @@ func getTestPolicyServerDeployment(policyServerName string) (*appsv1.Deployment, return &deployment, nil } +func getTestPolicyServerConfigMap(policyServerName string) (*corev1.ConfigMap, error) { + configMapName := getPolicyServerNameWithPrefix(policyServerName) + + configmap := corev1.ConfigMap{} + if err := reconciler.APIReader.Get(ctx, client.ObjectKey{Name: configMapName, Namespace: DeploymentsNamespace}, &configmap); err != nil { + return nil, errors.Join(errors.New("could not find ConfigMap owned by PolicyServer"), err) + } + return &configmap, nil +} + +func getPolicyServerNameWithPrefix(policyServerName string) string { + policyServer := policiesv1.PolicyServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: policyServerName, + }, + } + return policyServer.NameWithPrefix() +} + func getTestPolicyServerPod(policyServerName string) (*corev1.Pod, error) { podList := corev1.PodList{} if err := reconciler.APIReader.List(ctx, &podList, client.MatchingLabels{ diff --git a/internal/pkg/admission/policy-server-ca-secret.go b/internal/pkg/admission/policy-server-ca-secret.go index be52e273..938accf7 100644 --- a/internal/pkg/admission/policy-server-ca-secret.go +++ b/internal/pkg/admission/policy-server-ca-secret.go @@ -4,12 +4,15 @@ import ( "context" "crypto/rsa" "crypto/x509" + "errors" "fmt" + policiesv1 "github.com/kubewarden/kubewarden-controller/pkg/apis/policies/v1" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "github.com/kubewarden/kubewarden-controller/internal/pkg/admissionregistration" "github.com/kubewarden/kubewarden-controller/internal/pkg/constants" @@ -28,16 +31,16 @@ func (r *Reconciler) reconcileCASecret(ctx context.Context, secret *corev1.Secre return fmt.Errorf("error reconciling policy-server CA Secret: %w", err) } -func (r *Reconciler) fetchOrInitializePolicyServerCASecret(ctx context.Context, policyServerName string, caSecret *corev1.Secret, generateCert generateCertFunc) (*corev1.Secret, error) { +func (r *Reconciler) fetchOrInitializePolicyServerCASecret(ctx context.Context, policyServer *policiesv1.PolicyServer, caSecret *corev1.Secret, generateCert generateCertFunc) (*corev1.Secret, error) { policyServerSecret := corev1.Secret{} err := r.Client.Get( ctx, client.ObjectKey{ Namespace: r.DeploymentsNamespace, - Name: policyServerName}, + Name: policyServer.NameWithPrefix()}, &policyServerSecret) if err != nil && apierrors.IsNotFound(err) { - secret, err := r.buildPolicyServerCASecret(policyServerName, caSecret, generateCert) + secret, err := r.buildPolicyServerCASecret(policyServer, caSecret, generateCert) if err != nil { return secret, fmt.Errorf("cannot fetch or initialize Policy Server CA secret: %w", err) } @@ -53,31 +56,36 @@ func (r *Reconciler) fetchOrInitializePolicyServerCASecret(ctx context.Context, return &policyServerSecret, nil } -func (r *Reconciler) buildPolicyServerCASecret(policyServerName string, caSecret *corev1.Secret, generateCert generateCertFunc) (*corev1.Secret, error) { +func (r *Reconciler) buildPolicyServerCASecret(policyServer *policiesv1.PolicyServer, caSecret *corev1.Secret, generateCert generateCertFunc) (*corev1.Secret, error) { admissionregCA, err := extractCaFromSecret(caSecret) if err != nil { return nil, err } servingCert, servingKey, err := generateCert( admissionregCA.CaCert, - fmt.Sprintf("%s.%s.svc", policyServerName, r.DeploymentsNamespace), - []string{fmt.Sprintf("%s.%s.svc", policyServerName, r.DeploymentsNamespace)}, + fmt.Sprintf("%s.%s.svc", policyServer.NameWithPrefix(), r.DeploymentsNamespace), + []string{fmt.Sprintf("%s.%s.svc", policyServer.NameWithPrefix(), r.DeploymentsNamespace)}, admissionregCA.CaPrivateKey) if err != nil { - return nil, fmt.Errorf("cannot generate policy-server %s certificate: %w", policyServerName, err) + return nil, fmt.Errorf("cannot generate policy-server %s certificate: %w", policyServer.NameWithPrefix(), err) } secretContents := map[string]string{ constants.PolicyServerTLSCert: string(servingCert), constants.PolicyServerTLSKey: string(servingKey), } - return &corev1.Secret{ + secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: policyServerName, + Name: policyServer.NameWithPrefix(), Namespace: r.DeploymentsNamespace, }, StringData: secretContents, Type: corev1.SecretTypeOpaque, - }, nil + } + if err := controllerutil.SetOwnerReference(policyServer, secret, r.Client.Scheme()); err != nil { + return nil, errors.Join(fmt.Errorf("failed to set policy server secret owner reference"), err) + } + + return secret, nil } func extractCaFromSecret(caSecret *corev1.Secret) (*admissionregistration.CA, error) { diff --git a/internal/pkg/admission/policy-server-ca-secret_test.go b/internal/pkg/admission/policy-server-ca-secret_test.go index bb9609ad..82c0e4c2 100644 --- a/internal/pkg/admission/policy-server-ca-secret_test.go +++ b/internal/pkg/admission/policy-server-ca-secret_test.go @@ -12,9 +12,10 @@ import ( "github.com/google/go-cmp/cmp" "github.com/kubewarden/kubewarden-controller/internal/pkg/admissionregistration" "github.com/kubewarden/kubewarden-controller/internal/pkg/constants" + policiesv1 "github.com/kubewarden/kubewarden-controller/pkg/apis/policies/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client" ) func TestFetchOrInitializePolicyServerCARootSecret(t *testing.T) { @@ -49,7 +50,7 @@ func TestFetchOrInitializePolicyServerCARootSecret(t *testing.T) { generateCACalled bool }{ {"Existing CA", createReconcilerWithExistingCA(), nil, mockSecretContents, false}, - {"CA does not exist", createReconcilerWithEmptyClient(), nil, caSecretContents, true}, + {"CA does not exist", newReconciler(nil, false, false), nil, caSecretContents, true}, } for _, test := range tests { @@ -98,13 +99,18 @@ func TestFetchOrInitializePolicyServerSecret(t *testing.T) { generateCertCalled bool }{ {"Existing cert", createReconcilerWithExistingCert(), nil, mockSecretCert, false}, - {"cert does not exist", createReconcilerWithEmptyClient(), nil, caSecretContents, true}, + {"cert does not exist", newReconciler(nil, false, false), nil, caSecretContents, true}, } for _, test := range tests { ttest := test // ensure tt is correctly scoped when used in function literal t.Run(ttest.name, func(t *testing.T) { - secret, err := ttest.r.fetchOrInitializePolicyServerCASecret(context.Background(), "policyServer", caSecret, generateCertFunc) + policyServer := &policiesv1.PolicyServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "policyServer", + }, + } + secret, err := ttest.r.fetchOrInitializePolicyServerCASecret(context.Background(), policyServer, caSecret, generateCertFunc) if diff := cmp.Diff(secret.StringData, ttest.secretContents); diff != "" { t.Errorf("got an unexpected secret, diff %s", diff) } @@ -135,12 +141,7 @@ func createReconcilerWithExistingCA() Reconciler { Type: corev1.SecretTypeOpaque, } - // Create a fake client to mock API calls. It will return the mock secret - cl := fake.NewClientBuilder().WithObjects(mockSecret).Build() - return Reconciler{ - Client: cl, - DeploymentsNamespace: namespace, - } + return newReconciler([]client.Object{mockSecret}, false, false) } var mockSecretCert = map[string]string{"cert": "certString"} @@ -148,26 +149,12 @@ var mockSecretCert = map[string]string{"cert": "certString"} func createReconcilerWithExistingCert() Reconciler { mockSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "policyServer", + Name: "policy-server-policyServer", Namespace: namespace, }, StringData: mockSecretCert, Type: corev1.SecretTypeOpaque, } - // Create a fake client to mock API calls. It will return the mock secret - cl := fake.NewClientBuilder().WithObjects(mockSecret).Build() - return Reconciler{ - Client: cl, - DeploymentsNamespace: namespace, - } -} - -func createReconcilerWithEmptyClient() Reconciler { - // Create a fake client to mock API calls. - cl := fake.NewClientBuilder().WithObjects().Build() - return Reconciler{ - Client: cl, - DeploymentsNamespace: namespace, - } + return newReconciler([]client.Object{mockSecret}, false, false) } diff --git a/internal/pkg/admission/policy-server-configmap.go b/internal/pkg/admission/policy-server-configmap.go index aef450de..b2c15cc0 100644 --- a/internal/pkg/admission/policy-server-configmap.go +++ b/internal/pkg/admission/policy-server-configmap.go @@ -3,6 +3,7 @@ package admission import ( "context" "encoding/json" + "errors" "fmt" "reflect" @@ -15,6 +16,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/kubewarden/kubewarden-controller/internal/pkg/constants" @@ -162,6 +164,9 @@ func (r *Reconciler) createPolicyServerConfigMap( }, Data: data, } + if err := controllerutil.SetOwnerReference(policyServer, cfg, r.Client.Scheme()); err != nil { + return errors.Join(fmt.Errorf("failed to set policy server configmap owner reference"), err) + } //nolint:wrapcheck return r.Client.Create(ctx, cfg) diff --git a/internal/pkg/admission/policy-server-deployment.go b/internal/pkg/admission/policy-server-deployment.go index ee86e4eb..82c5d134 100644 --- a/internal/pkg/admission/policy-server-deployment.go +++ b/internal/pkg/admission/policy-server-deployment.go @@ -2,6 +2,7 @@ package admission import ( "context" + "errors" "fmt" "path/filepath" "reflect" @@ -14,6 +15,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "github.com/kubewarden/kubewarden-controller/internal/pkg/constants" "github.com/kubewarden/kubewarden-controller/internal/pkg/policyserver" @@ -51,7 +53,10 @@ func (r *Reconciler) reconcilePolicyServerDeployment(ctx context.Context, policy } } - deployment := r.deployment(configMapVersion, policyServer) + deployment, err := r.deployment(configMapVersion, policyServer) + if err != nil { + return err + } err = r.Client.Create(ctx, deployment) if err == nil { return nil @@ -213,7 +218,7 @@ func envVarsContainVariable(envVars []corev1.EnvVar, envVarName string) int { return -1 } -func (r *Reconciler) deployment(configMapVersion string, policyServer *policiesv1.PolicyServer) *appsv1.Deployment { +func (r *Reconciler) deployment(configMapVersion string, policyServer *policiesv1.PolicyServer) (*appsv1.Deployment, error) { admissionContainer := corev1.Container{ Name: policyServer.NameWithPrefix(), Image: policyServer.Spec.Image, @@ -335,32 +340,7 @@ func (r *Reconciler) deployment(configMapVersion string, policyServer *policiesv admissionContainer.SecurityContext = defaultContainerSecurityContext() } - templateAnnotations := policyServer.Spec.Annotations - if templateAnnotations == nil { - templateAnnotations = make(map[string]string) - } - - if r.MetricsEnabled { - templateAnnotations[constants.OptelInjectAnnotation] = "true" //nolint:goconst - - envvar := corev1.EnvVar{Name: constants.PolicyServerEnableMetricsEnvVar, Value: "true"} - if index := envVarsContainVariable(admissionContainer.Env, constants.PolicyServerEnableMetricsEnvVar); index >= 0 { - admissionContainer.Env[index] = envvar - } else { - admissionContainer.Env = append(admissionContainer.Env, envvar) - } - } - - if r.TracingEnabled { - templateAnnotations[constants.OptelInjectAnnotation] = "true" - - logFmtEnvVar := corev1.EnvVar{Name: constants.PolicyServerLogFmtEnvVar, Value: "otlp"} - if index := envVarsContainVariable(admissionContainer.Env, constants.PolicyServerLogFmtEnvVar); index >= 0 { - admissionContainer.Env[index] = logFmtEnvVar - } else { - admissionContainer.Env = append(admissionContainer.Env, logFmtEnvVar) - } - } + templateAnnotations := setupMetricsAndTracing(policyServer, &admissionContainer, r.TracingEnabled, r.MetricsEnabled) policyServerDeployment := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ @@ -435,8 +415,41 @@ func (r *Reconciler) deployment(configMapVersion string, policyServer *policiesv } r.adaptDeploymentSettingsForPolicyServer(policyServerDeployment, policyServer) + if err := controllerutil.SetOwnerReference(policyServer, policyServerDeployment, r.Client.Scheme()); err != nil { + return nil, errors.Join(fmt.Errorf("failed to set policy server deployment owner reference"), err) + } - return policyServerDeployment + return policyServerDeployment, nil +} + +func setupMetricsAndTracing(policyServer *policiesv1.PolicyServer, admissionContainer *corev1.Container, tracingEnabled bool, metricsEnabled bool) map[string]string { + templateAnnotations := policyServer.Spec.Annotations + if templateAnnotations == nil { + templateAnnotations = make(map[string]string) + } + + if metricsEnabled { + templateAnnotations[constants.OptelInjectAnnotation] = "true" //nolint:goconst + + envvar := corev1.EnvVar{Name: constants.PolicyServerEnableMetricsEnvVar, Value: "true"} + if index := envVarsContainVariable(admissionContainer.Env, constants.PolicyServerEnableMetricsEnvVar); index >= 0 { + admissionContainer.Env[index] = envvar + } else { + admissionContainer.Env = append(admissionContainer.Env, envvar) + } + } + + if tracingEnabled { + templateAnnotations[constants.OptelInjectAnnotation] = "true" + + logFmtEnvVar := corev1.EnvVar{Name: constants.PolicyServerLogFmtEnvVar, Value: "otlp"} + if index := envVarsContainVariable(admissionContainer.Env, constants.PolicyServerLogFmtEnvVar); index >= 0 { + admissionContainer.Env[index] = logFmtEnvVar + } else { + admissionContainer.Env = append(admissionContainer.Env, logFmtEnvVar) + } + } + return templateAnnotations } func defaultContainerSecurityContext() *corev1.SecurityContext { diff --git a/internal/pkg/admission/policy-server-deployment_test.go b/internal/pkg/admission/policy-server-deployment_test.go index 708128bd..aa1a6d4c 100644 --- a/internal/pkg/admission/policy-server-deployment_test.go +++ b/internal/pkg/admission/policy-server-deployment_test.go @@ -6,9 +6,11 @@ import ( "github.com/kubewarden/kubewarden-controller/internal/pkg/constants" policiesv1 "github.com/kubewarden/kubewarden-controller/pkg/apis/policies/v1" + "github.com/stretchr/testify/require" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client/apiutil" ) const ( @@ -256,10 +258,7 @@ func TestIfPolicyServerImageChanged(t *testing.T) { } func TestPolicyServerWithContainerSecurityContext(t *testing.T) { - reconciler := Reconciler{ - Client: nil, - DeploymentsNamespace: "kubewarden", - } + reconciler := newReconciler(nil, false, false) readOnlFileSystem := false privileged := true runAsRoot := false @@ -282,7 +281,8 @@ func TestPolicyServerWithContainerSecurityContext(t *testing.T) { }, }, } - deployment := reconciler.deployment("v1", policyServer) + deployment, err := reconciler.deployment("v1", policyServer) + require.NoError(t, err) if deployment.Spec.Template.Spec.Containers[0].SecurityContext.ReadOnlyRootFilesystem == nil || *deployment.Spec.Template.Spec.Containers[0].SecurityContext.ReadOnlyRootFilesystem != readOnlFileSystem { @@ -309,10 +309,7 @@ func TestPolicyServerWithContainerSecurityContext(t *testing.T) { } func TestPolicyServerWithPodSecurityContext(t *testing.T) { - reconciler := Reconciler{ - Client: nil, - DeploymentsNamespace: "kubewarden", - } + reconciler := newReconciler(nil, false, false) var user, group int64 user = 1000 group = 2000 @@ -328,7 +325,8 @@ func TestPolicyServerWithPodSecurityContext(t *testing.T) { }, }, } - deployment := reconciler.deployment("v1", policyServer) + deployment, err := reconciler.deployment("v1", policyServer) + require.NoError(t, err) if deployment.Spec.Template.Spec.SecurityContext == nil { t.Error("Pod securityContext should be defined ") @@ -345,17 +343,15 @@ func TestPolicyServerWithPodSecurityContext(t *testing.T) { } func TestPolicyServerWithoutSecurityContext(t *testing.T) { - reconciler := Reconciler{ - Client: nil, - DeploymentsNamespace: "kubewarden", - } + reconciler := newReconciler(nil, false, false) policyServer := &policiesv1.PolicyServer{ Spec: policiesv1.PolicyServerSpec{ Image: "image", SecurityContexts: policiesv1.PolicyServerSecurity{}, }, } - deployment := reconciler.deployment("v1", policyServer) + deployment, err := reconciler.deployment("v1", policyServer) + require.NoError(t, err) containerDefaultSecurityContext := defaultContainerSecurityContext() if *deployment.Spec.Template.Spec.Containers[0].SecurityContext.ReadOnlyRootFilesystem != *containerDefaultSecurityContext.ReadOnlyRootFilesystem { @@ -376,10 +372,7 @@ func TestPolicyServerWithoutSecurityContext(t *testing.T) { } func TestPolicyServerWithPodAndContainerSecurityContext(t *testing.T) { - reconciler := Reconciler{ - Client: nil, - DeploymentsNamespace: "kubewarden", - } + reconciler := newReconciler(nil, false, false) readOnlFileSystem := false privileged := true runAsRoot := false @@ -410,7 +403,8 @@ func TestPolicyServerWithPodAndContainerSecurityContext(t *testing.T) { }, }, } - deployment := reconciler.deployment("v1", policyServer) + deployment, err := reconciler.deployment("v1", policyServer) + require.NoError(t, err) if *deployment.Spec.Template.Spec.Containers[0].SecurityContext.ReadOnlyRootFilesystem != readOnlFileSystem { t.Error("Policy server container ReadOnlyRootFilesystem diverge from the expected value") @@ -494,18 +488,15 @@ func TestMetricAndLogFmtEnvVarsDetection(t *testing.T) { } func TestPolicyServerDeploymentMetricConfigurationWithValueDefinedByUser(t *testing.T) { //nolint:dupl - reconciler := Reconciler{ - Client: nil, - DeploymentsNamespace: "kubewarden", - MetricsEnabled: true, - } + reconciler := newReconciler(nil, true, false) policyServer := &policiesv1.PolicyServer{ Spec: policiesv1.PolicyServerSpec{ Image: "image", Env: []corev1.EnvVar{{Name: constants.PolicyServerEnableMetricsEnvVar, Value: "false"}, {Name: constants.PolicyServerLogFmtEnvVar, Value: "invalid"}}, }, } - deployment := reconciler.deployment("v1", policyServer) + deployment, err := reconciler.deployment("v1", policyServer) + require.NoError(t, err) hasMetricEnvvar := false for _, envvar := range deployment.Spec.Template.Spec.Containers[0].Env { if envvar.Name == constants.PolicyServerEnableMetricsEnvVar { @@ -529,18 +520,15 @@ func TestPolicyServerDeploymentMetricConfigurationWithValueDefinedByUser(t *test } func TestPolicyServerDeploymentTracingConfigurationWithValueDefinedByUser(t *testing.T) { //nolint:dupl - reconciler := Reconciler{ - Client: nil, - DeploymentsNamespace: "kubewarden", - TracingEnabled: true, - } + reconciler := newReconciler(nil, false, true) policyServer := &policiesv1.PolicyServer{ Spec: policiesv1.PolicyServerSpec{ Image: "image", Env: []corev1.EnvVar{{Name: constants.PolicyServerEnableMetricsEnvVar, Value: "false"}, {Name: constants.PolicyServerLogFmtEnvVar, Value: "invalid"}}, }, } - deployment := reconciler.deployment("v1", policyServer) + deployment, err := reconciler.deployment("v1", policyServer) + require.NoError(t, err) hasLogFmtEnvvar := false for _, envvar := range deployment.Spec.Template.Spec.Containers[0].Env { if envvar.Name == constants.PolicyServerLogFmtEnvVar { @@ -564,18 +552,15 @@ func TestPolicyServerDeploymentTracingConfigurationWithValueDefinedByUser(t *tes } func TestPolicyServerDeploymentMetricConfigurationWithNoValueDefinedByUSer(t *testing.T) { - reconciler := Reconciler{ - Client: nil, - DeploymentsNamespace: "kubewarden", - MetricsEnabled: false, - } + reconciler := newReconciler(nil, false, false) policyServer := &policiesv1.PolicyServer{ Spec: policiesv1.PolicyServerSpec{ Image: "image", Env: []corev1.EnvVar{}, }, } - deployment := reconciler.deployment("v1", policyServer) + deployment, err := reconciler.deployment("v1", policyServer) + require.NoError(t, err) hasMetricEnvvar := false hasLogFmtEnvvar := false for _, envvar := range deployment.Spec.Template.Spec.Containers[0].Env { @@ -600,11 +585,7 @@ func TestPolicyServerDeploymentMetricConfigurationWithNoValueDefinedByUSer(t *te } func TestPolicyServerDeploymentWithAffinity(t *testing.T) { - reconciler := Reconciler{ - Client: nil, - DeploymentsNamespace: "kubewarden", - MetricsEnabled: false, - } + reconciler := newReconciler(nil, false, false) policyServer := &policiesv1.PolicyServer{ Spec: policiesv1.PolicyServerSpec{ Image: "image", @@ -612,10 +593,31 @@ func TestPolicyServerDeploymentWithAffinity(t *testing.T) { Affinity: *createAffinity("nodename"), }, } - deployment := reconciler.deployment("v1", policyServer) + deployment, err := reconciler.deployment("v1", policyServer) + require.NoError(t, err) if deployment.Spec.Template.Spec.Affinity. NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution. NodeSelectorTerms[0].MatchExpressions[0].Values[0] != "nodename" { t.Error("missing affinity") } } + +func TestPolicyServerDeploymentOwnerReference(t *testing.T) { + reconciler := newReconciler(nil, false, false) + policyServer := policiesv1.PolicyServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + UID: "test-uid", + }, + } + gvk, err := apiutil.GVKForObject(&policyServer, reconciler.Client.Scheme()) + require.NoError(t, err) + + deployment, err := reconciler.deployment("v1", &policyServer) + + require.NoError(t, err) + require.Equal(t, policyServer.GetName(), deployment.OwnerReferences[0].Name) + require.Equal(t, policyServer.GetUID(), deployment.OwnerReferences[0].UID) + require.Equal(t, gvk.GroupVersion().String(), deployment.OwnerReferences[0].APIVersion) + require.Equal(t, gvk.Kind, deployment.OwnerReferences[0].Kind) +} diff --git a/internal/pkg/admission/policy-server-pod-disruption-budget_test.go b/internal/pkg/admission/policy-server-pod-disruption-budget_test.go index fcce5df8..e46fa2db 100644 --- a/internal/pkg/admission/policy-server-pod-disruption-budget_test.go +++ b/internal/pkg/admission/policy-server-pod-disruption-budget_test.go @@ -27,7 +27,7 @@ func TestPDBCreation(t *testing.T) { } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - reconciler := newReconciler(nil, false) + reconciler := newReconciler(nil, false, false) policyServer := &policiesv1.PolicyServer{ ObjectMeta: metav1.ObjectMeta{ UID: "uid", @@ -120,7 +120,7 @@ func TestPDBUpdate(t *testing.T) { }, } - reconciler := newReconciler([]client.Object{oldPDB}, false) + reconciler := newReconciler([]client.Object{oldPDB}, false, false) err := reconciler.reconcilePolicyServerPodDisruptionBudget(context.Background(), policyServer) require.NoError(t, err) @@ -173,7 +173,7 @@ func TestPDBDelete(t *testing.T) { }, }, } - reconciler := newReconciler([]client.Object{oldPDB}, false) + reconciler := newReconciler([]client.Object{oldPDB}, false, false) err := reconciler.reconcilePolicyServerPodDisruptionBudget(context.Background(), policyServer) require.NoError(t, err) diff --git a/internal/pkg/admission/policy-server-service.go b/internal/pkg/admission/policy-server-service.go index 5222cb4f..8b84e2ad 100644 --- a/internal/pkg/admission/policy-server-service.go +++ b/internal/pkg/admission/policy-server-service.go @@ -2,6 +2,7 @@ package admission import ( "context" + "errors" "fmt" "os" "strconv" @@ -11,6 +12,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "github.com/kubewarden/kubewarden-controller/internal/pkg/constants" ) @@ -35,8 +37,11 @@ func init() { } func (r *Reconciler) reconcilePolicyServerService(ctx context.Context, policyServer *policiesv1.PolicyServer) error { - service := r.service(policyServer) - err := r.Client.Create(ctx, service) + service, err := r.service(policyServer) + if err != nil { + return err + } + err = r.Client.Create(ctx, service) if err != nil && apierrors.IsAlreadyExists(err) { err = r.Client.Update(ctx, service) @@ -47,7 +52,7 @@ func (r *Reconciler) reconcilePolicyServerService(ctx context.Context, policySer return fmt.Errorf("cannot reconcile policy-server service: %w", err) } -func (r *Reconciler) service(policyServer *policiesv1.PolicyServer) *corev1.Service { +func (r *Reconciler) service(policyServer *policiesv1.PolicyServer) (*corev1.Service, error) { svc := corev1.Service{ ObjectMeta: metav1.ObjectMeta{ Name: policyServer.NameWithPrefix(), @@ -80,5 +85,10 @@ func (r *Reconciler) service(policyServer *policiesv1.PolicyServer) *corev1.Serv }, ) } - return &svc + + if err := controllerutil.SetOwnerReference(policyServer, &svc, r.Client.Scheme()); err != nil { + return nil, errors.Join(fmt.Errorf("failed to set policy server configmap owner reference"), err) + } + + return &svc, nil } diff --git a/internal/pkg/admission/policy-server-service_test.go b/internal/pkg/admission/policy-server-service_test.go index f5cd6141..da2aeedd 100644 --- a/internal/pkg/admission/policy-server-service_test.go +++ b/internal/pkg/admission/policy-server-service_test.go @@ -5,11 +5,34 @@ import ( "github.com/kubewarden/kubewarden-controller/internal/pkg/constants" policiesv1 "github.com/kubewarden/kubewarden-controller/pkg/apis/policies/v1" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/apiutil" ) +func TestServiceOwnerReference(t *testing.T) { + reconciler := newReconciler([]client.Object{}, false, false) + policyServer := &policiesv1.PolicyServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "policy-server", + UID: "uid", + }, + } + gvk, err := apiutil.GVKForObject(policyServer, reconciler.Client.Scheme()) + require.NoError(t, err) + + service, err := reconciler.service(policyServer) + + require.NoError(t, err) + require.Equal(t, policyServer.GetName(), service.OwnerReferences[0].Name) + require.Equal(t, policyServer.GetUID(), service.OwnerReferences[0].UID) + require.Equal(t, gvk.GroupVersion().String(), service.OwnerReferences[0].APIVersion) + require.Equal(t, gvk.Kind, service.OwnerReferences[0].Kind) +} + func TestMetricsEnabled(t *testing.T) { - reconciler := newReconciler([]client.Object{}, true) + reconciler := newReconciler([]client.Object{}, true, false) if !reconciler.MetricsEnabled { t.Fatal("Metric not enabled") } @@ -18,7 +41,8 @@ func TestMetricsEnabled(t *testing.T) { Image: "image", }, } - service := reconciler.service(policyServer) + service, err := reconciler.service(policyServer) + require.NoError(t, err) for _, port := range service.Spec.Ports { if port.Port == constants.PolicyServerMetricsPort { return diff --git a/internal/pkg/admission/policy-server_integration_test.go b/internal/pkg/admission/policy-server_integration_test.go index ee94975e..0958b103 100644 --- a/internal/pkg/admission/policy-server_integration_test.go +++ b/internal/pkg/admission/policy-server_integration_test.go @@ -18,7 +18,7 @@ const port = "8181" func TestCAAndCertificateCreationInAHttpsServer(t *testing.T) { const domain = "localhost" const maxRetries = 10 - r := createReconcilerWithEmptyClient() + r := newReconciler(nil, false, false) // create CA caSecret, err := r.buildPolicyServerCARootSecret(admissionregistration.GenerateCA, admissionregistration.PemEncodeCertificate) if err != nil { diff --git a/internal/pkg/admission/reconciler.go b/internal/pkg/admission/reconciler.go index f4f25a2b..7a12f2f3 100644 --- a/internal/pkg/admission/reconciler.go +++ b/internal/pkg/admission/reconciler.go @@ -3,16 +3,13 @@ package admission import ( "context" "fmt" - "strings" "github.com/go-logr/logr" "github.com/kubewarden/kubewarden-controller/internal/pkg/admissionregistration" "github.com/kubewarden/kubewarden-controller/internal/pkg/constants" policiesv1 "github.com/kubewarden/kubewarden-controller/pkg/apis/policies/v1" - appsv1 "k8s.io/api/apps/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" - corev1 "k8s.io/api/core/v1" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -28,101 +25,6 @@ type Reconciler struct { TracingEnabled bool } -type reconcilerErrors []error - -func (errorList reconcilerErrors) Error() string { - errors := []string{} - for _, error := range errorList { - errors = append(errors, error.Error()) - } - return strings.Join(errors, ", ") -} - -func (r *Reconciler) ReconcileDeletion( - ctx context.Context, - policyServer *policiesv1.PolicyServer, -) error { - errors := reconcilerErrors{} - - deployment := &appsv1.Deployment{ - ObjectMeta: metav1.ObjectMeta{ - Name: policyServer.NameWithPrefix(), - Namespace: r.DeploymentsNamespace, - }, - } - err := r.Client.Delete(ctx, deployment) - if err == nil { - setFalseConditionType( - &policyServer.Status.Conditions, - string(policiesv1.PolicyServerDeploymentReconciled), - "Policy Server has been deleted", - ) - } else if !apierrors.IsNotFound(err) { - r.Log.Error(err, "ReconcileDeletion: cannot delete PolicyServer Deployment "+policyServer.Name) - errors = append(errors, err) - } - - certificateSecret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: policyServer.NameWithPrefix(), - Namespace: r.DeploymentsNamespace, - }, - } - err = r.Client.Delete(ctx, certificateSecret) - if err == nil { - setFalseConditionType( - &policyServer.Status.Conditions, - string(policiesv1.PolicyServerCASecretReconciled), - "Policy Server has been deleted", - ) - } else if !apierrors.IsNotFound(err) { - r.Log.Error(err, "ReconcileDeletion: cannot delete PolicyServer Certificate Secret "+policyServer.Name) - errors = append(errors, err) - } - - service := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: policyServer.NameWithPrefix(), - Namespace: r.DeploymentsNamespace, - }, - } - err = r.Client.Delete(ctx, service) - if err == nil { - setFalseConditionType( - &policyServer.Status.Conditions, - string(policiesv1.PolicyServerServiceReconciled), - "Policy Server has been deleted", - ) - } else if !apierrors.IsNotFound(err) { - r.Log.Error(err, "ReconcileDeletion: cannot delete PolicyServer Service "+policyServer.Name) - errors = append(errors, err) - } - - cfg := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: policyServer.NameWithPrefix(), - Namespace: r.DeploymentsNamespace, - }, - } - err = r.Client.Delete(ctx, cfg) - if err == nil { - setFalseConditionType( - &policyServer.Status.Conditions, - string(policiesv1.PolicyServerConfigMapReconciled), - "Policy Server has been deleted", - ) - } else if !apierrors.IsNotFound(err) { - r.Log.Error(err, "ReconcileDeletion: cannot delete PolicyServer ConfigMap "+policyServer.Name) - errors = append(errors, err) - } - - if len(errors) == 0 { - return nil - } - - return errors -} - func setFalseConditionType( conditions *[]metav1.Condition, conditionType string, @@ -179,7 +81,7 @@ func (r *Reconciler) Reconcile( string(policiesv1.PolicyServerCARootSecretReconciled), ) - policyServerCASecret, err := r.fetchOrInitializePolicyServerCASecret(ctx, policyServer.NameWithPrefix(), policyServerCARootSecret, admissionregistration.GenerateCert) + policyServerCASecret, err := r.fetchOrInitializePolicyServerCASecret(ctx, policyServer, policyServerCARootSecret, admissionregistration.GenerateCert) if err != nil { setFalseConditionType( &policyServer.Status.Conditions, diff --git a/internal/pkg/admission/reconciler_test.go b/internal/pkg/admission/reconciler_test.go index 787a49e6..ee2f8ab3 100644 --- a/internal/pkg/admission/reconciler_test.go +++ b/internal/pkg/admission/reconciler_test.go @@ -75,7 +75,7 @@ func TestGetPolicies(t *testing.T) { for _, test := range tests { ttest := test // ensure ttest is correctly scoped when used in function literal t.Run(ttest.name, func(t *testing.T) { - reconciler := newReconciler(ttest.policies, false) + reconciler := newReconciler(ttest.policies, false, false) policies, err := reconciler.GetPolicies(context.Background(), &policiesv1.PolicyServer{ ObjectMeta: metav1.ObjectMeta{Name: policyServer}, }) @@ -89,14 +89,21 @@ func TestGetPolicies(t *testing.T) { } } -func newReconciler(policies []client.Object, metricsEnabled bool) Reconciler { +func newReconciler(objects []client.Object, metricsEnabled bool, tracingEnabled bool) Reconciler { customScheme := scheme.Scheme - customScheme.AddKnownTypes(schema.GroupVersion{Group: "policies.kubewarden.io", Version: "v1"}, &policiesv1.ClusterAdmissionPolicy{}, &policiesv1.AdmissionPolicy{}, &policiesv1.ClusterAdmissionPolicyList{}, &policiesv1.AdmissionPolicyList{}, &policiesv1.PolicyServer{}, &policiesv1.PolicyServerList{}) - cl := fake.NewClientBuilder().WithScheme(customScheme).WithObjects(policies...).Build() + customScheme.AddKnownTypes(schema.GroupVersion{Group: "policies.kubewarden.io", Version: "v1"}, + &policiesv1.ClusterAdmissionPolicy{}, + &policiesv1.AdmissionPolicy{}, + &policiesv1.ClusterAdmissionPolicyList{}, + &policiesv1.AdmissionPolicyList{}, + &policiesv1.PolicyServer{}, + &policiesv1.PolicyServerList{}) + cl := fake.NewClientBuilder().WithScheme(customScheme).WithObjects(objects...).Build() return Reconciler{ Client: cl, DeploymentsNamespace: namespace, MetricsEnabled: metricsEnabled, + TracingEnabled: tracingEnabled, } } diff --git a/internal/pkg/admission/testing_common.go b/internal/pkg/admission/testing_common.go index f9451337..f83f9640 100644 --- a/internal/pkg/admission/testing_common.go +++ b/internal/pkg/admission/testing_common.go @@ -67,6 +67,7 @@ func createReconciler() (Reconciler, policiesv1.ClusterAdmissionPolicy, policies customScheme := scheme.Scheme customScheme.AddKnownTypes(schema.GroupVersion{Group: "policies.kubewarden.io", Version: "v1"}, &validationPolicy) + customScheme.AddKnownTypes(schema.GroupVersion{Group: "policies.kubewarden.io", Version: "v1"}, &policiesv1.PolicyServer{}) cl := fake.NewClientBuilder().WithScheme(customScheme).WithObjects(validatingWebhook, mutatingWebhook, contextAwareWebhook, &validationPolicy, &mutatingPolicy, &contextAwarePolicy).Build() reconciler := Reconciler{ Client: cl,