diff --git a/Makefile b/Makefile index 4c308ca0..31e19889 100644 --- a/Makefile +++ b/Makefile @@ -41,7 +41,9 @@ GOLANGCI_LINT_VER := v1.55.2 GOLANGCI_LINT_BIN := golangci-lint GOLANGCI_LINT := $(BIN_DIR)/$(GOLANGCI_LINT_BIN) -TEST_TIMEOUT := 20m +# Let's use a generous timeout for integration tests because GitHub workers can +# be slow +TEST_TIMEOUT := 30m all: build diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 047a13f1..4a0180f4 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -11,10 +11,9 @@ rules: - mutatingwebhookconfigurations verbs: - create - - get + - delete - list - patch - - update - watch - apiGroups: - admissionregistration.k8s.io @@ -22,19 +21,21 @@ rules: - validatingwebhookconfigurations verbs: - create - - get + - delete - list - patch - - update - watch - apiGroups: - policies.kubewarden.io resources: - admissionpolicies verbs: + - create - delete - get - list + - patch + - update - watch - apiGroups: - policies.kubewarden.io @@ -55,9 +56,12 @@ rules: resources: - clusteradmissionpolicies verbs: + - create - delete - get - list + - patch + - update - watch - apiGroups: - policies.kubewarden.io diff --git a/controllers/admissionpolicy_controller.go b/controllers/admissionpolicy_controller.go index fe4685ef..57663de8 100644 --- a/controllers/admissionpolicy_controller.go +++ b/controllers/admissionpolicy_controller.go @@ -42,7 +42,7 @@ import ( // // We need access to these resources inside of all the namespaces -> a ClusterRole // is needed -//+kubebuilder:rbac:groups=policies.kubewarden.io,resources=admissionpolicies,verbs=get;list;watch;delete +//+kubebuilder:rbac:groups=policies.kubewarden.io,resources=admissionpolicies,verbs=create;delete;get;list;patch;update;watch //+kubebuilder:rbac:groups=policies.kubewarden.io,resources=admissionpolicies/status,verbs=get;update;patch //+kubebuilder:rbac:groups=policies.kubewarden.io,resources=admissionpolicies/finalizers,verbs=update // diff --git a/controllers/clusteradmissionpolicy_controller.go b/controllers/clusteradmissionpolicy_controller.go index 6fcc0194..e352829e 100644 --- a/controllers/clusteradmissionpolicy_controller.go +++ b/controllers/clusteradmissionpolicy_controller.go @@ -42,7 +42,7 @@ import ( // // We need access to these resources inside of all the namespaces -> a ClusterRole // is needed -//+kubebuilder:rbac:groups=policies.kubewarden.io,resources=clusteradmissionpolicies,verbs=get;list;watch;delete +//+kubebuilder:rbac:groups=policies.kubewarden.io,resources=clusteradmissionpolicies,verbs=create;delete;get;list;patch;update;watch //+kubebuilder:rbac:groups=policies.kubewarden.io,resources=clusteradmissionpolicies/status,verbs=get;update;patch //+kubebuilder:rbac:groups=policies.kubewarden.io,resources=clusteradmissionpolicies/finalizers,verbs=update diff --git a/controllers/policyserver_controller_test.go b/controllers/policyserver_controller_test.go index e15bfc1d..13bf964b 100644 --- a/controllers/policyserver_controller_test.go +++ b/controllers/policyserver_controller_test.go @@ -1,5 +1,5 @@ /* -Copyright 2022. +copyright 2022. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -17,6 +17,7 @@ limitations under the License. package controllers import ( + "encoding/json" "fmt" . "github.com/onsi/ginkgo/v2" //nolint:revive @@ -24,9 +25,11 @@ import ( . "github.com/onsi/gomega/gstruct" //nolint:revive corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "github.com/kubewarden/kubewarden-controller/internal/pkg/admission" "github.com/kubewarden/kubewarden-controller/internal/pkg/constants" policiesv1 "github.com/kubewarden/kubewarden-controller/pkg/apis/policies/v1" k8spoliciesv1 "k8s.io/api/policy/v1" @@ -41,6 +44,7 @@ var _ = Describe("PolicyServer controller", func() { }) When("deleting a PolicyServer", func() { + BeforeEach(func() { createPolicyServerAndWaitForItsService(policyServerFactory(policyServerName)) }) @@ -142,50 +146,249 @@ var _ = Describe("PolicyServer controller", func() { }) }) - When("creating a new PolicyServer", func() { - var policyServer *policiesv1.PolicyServer + When("creating a PolicyServer", func() { + + It("should use the policy server affinity configuration in the policy server deployment", func() { + policyServer := policyServerFactory(policyServerName) + policyServer.Spec.Affinity = corev1.Affinity{ + NodeAffinity: &corev1.NodeAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: &corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{ + { + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: "label", + Operator: corev1.NodeSelectorOpIn, + Values: []string{"nodename"}, + }, + }, + }, + }, + }, + }, + } + createPolicyServerAndWaitForItsService(policyServer) + deployment, err := getTestPolicyServerDeployment(policyServerName) + Expect(err).ToNot(HaveOccurred()) + Expect(deployment.Spec.Template.Spec.Affinity).To(PointTo(MatchFields(IgnoreExtras, Fields{ + "NodeAffinity": PointTo(MatchFields(IgnoreExtras, Fields{ + "RequiredDuringSchedulingIgnoredDuringExecution": PointTo(MatchFields(IgnoreExtras, Fields{ + "NodeSelectorTerms": ContainElement(MatchFields(IgnoreExtras, Fields{ + "MatchExpressions": ContainElement(MatchAllFields(Fields{ + "Key": Equal("label"), + "Operator": Equal(corev1.NodeSelectorOpIn), + "Values": Equal([]string{"nodename"}), + })), + })), + })), + })), + }))) + }) - BeforeEach(func() { - policyServer = policyServerFactory(policyServerName) + It("should create policy server deployment with some default configuration", func() { + policyServer := policyServerFactory(policyServerName) + createPolicyServerAndWaitForItsService(policyServer) + deployment, err := getTestPolicyServerDeployment(policyServerName) + Expect(err).ToNot(HaveOccurred()) + By("checking the deployment container security context") + Expect(deployment.Spec.Template.Spec.Containers).Should(ContainElement(MatchFields(IgnoreExtras, Fields{ + "SecurityContext": PointTo(MatchFields(IgnoreExtras, Fields{ + "RunAsNonRoot": PointTo(BeTrue()), + "AllowPrivilegeEscalation": PointTo(BeFalse()), + "Privileged": PointTo(BeFalse()), + "ReadOnlyRootFilesystem": PointTo(BeTrue()), + "Capabilities": PointTo(MatchAllFields(Fields{ + "Add": BeNil(), + "Drop": Equal([]corev1.Capability{"all"}), + })), + "SELinuxOptions": BeNil(), + "WindowsOptions": BeNil(), + "RunAsUser": BeNil(), + "RunAsGroup": BeNil(), + "ProcMount": BeNil(), + "SeccompProfile": BeNil(), + }))}))) + By("checking the deployment pod security context") + Expect(deployment.Spec.Template.Spec.SecurityContext).To(PointTo(MatchFields(IgnoreExtras, Fields{ + "SELinuxOptions": BeNil(), + "WindowsOptions": BeNil(), + "RunAsUser": BeNil(), + "RunAsGroup": BeNil(), + "RunAsNonRoot": BeNil(), + "SupplementalGroups": BeNil(), + "FSGroup": BeNil(), + "Sysctls": BeNil(), + "FSGroupChangePolicy": BeNil(), + "SeccompProfile": BeNil()}))) + + By("checking the deployment affinity") + Expect(deployment.Spec.Template.Spec.Affinity).To(BeNil()) + }) + + It("should create the policy server deployment and use the user defined security contexts", func() { + policyServer := policyServerFactory(policyServerName) + runAsUser := int64(1000) + privileged := true + runAsNonRoot := false + policyServer.Spec.SecurityContexts = policiesv1.PolicyServerSecurity{ + Container: &corev1.SecurityContext{ + RunAsUser: &runAsUser, + Privileged: &privileged, + RunAsNonRoot: &runAsNonRoot, + }, + Pod: &corev1.PodSecurityContext{ + RunAsUser: &runAsUser, + RunAsNonRoot: &runAsNonRoot, + }, + } + createPolicyServerAndWaitForItsService(policyServer) + deployment, err := getTestPolicyServerDeployment(policyServerName) + Expect(err).ToNot(HaveOccurred()) + Expect(deployment.Spec.Template.Spec.Containers).Should(ContainElement(MatchFields(IgnoreExtras, Fields{ + "SecurityContext": PointTo(MatchFields(IgnoreExtras, Fields{ + "RunAsNonRoot": PointTo(BeFalse()), + "AllowPrivilegeEscalation": BeNil(), + "Privileged": PointTo(BeTrue()), + "ReadOnlyRootFilesystem": BeNil(), + "Capabilities": BeNil(), + "SELinuxOptions": BeNil(), + "WindowsOptions": BeNil(), + "RunAsUser": PointTo(BeNumerically("==", 1000)), + "RunAsGroup": BeNil(), + "ProcMount": BeNil(), + "SeccompProfile": BeNil(), + }))}))) + Expect(deployment.Spec.Template.Spec.SecurityContext).To(PointTo(MatchFields(IgnoreExtras, Fields{ + "SELinuxOptions": BeNil(), + "WindowsOptions": BeNil(), + "RunAsUser": PointTo(BeNumerically("==", 1000)), + "RunAsGroup": BeNil(), + "RunAsNonRoot": PointTo(BeFalse()), + "SupplementalGroups": BeNil(), + "FSGroup": BeNil(), + "Sysctls": BeNil(), + "FSGroupChangePolicy": BeNil(), + "SeccompProfile": BeNil()}))) + }) + + It("should create the policy server configmap empty if no policies are assigned ", func() { + policyServer := policyServerFactory(policyServerName) + createPolicyServerAndWaitForItsService(policyServer) + configmap, err := getTestPolicyServerConfigMap(policyServerName) + Expect(err).ToNot(HaveOccurred()) + Expect(configmap).To(PointTo(MatchFields(IgnoreExtras, Fields{ + "Data": MatchAllKeys(Keys{ + constants.PolicyServerConfigPoliciesEntry: Equal("{}"), + constants.PolicyServerConfigSourcesEntry: Equal("{}"), + }), + }))) }) - It("it should creates the policy server configmap empty when no policy is assigned", func() { + It("should create the policy server configmap with the assigned policies", func() { + policyServer := policyServerFactory(policyServerName) createPolicyServerAndWaitForItsService(policyServer) + policyName := newName("policy") + policy := clusterAdmissionPolicyFactory(policyName, policyServerName, false) + Expect(k8sClient.Create(ctx, policy)).To(Succeed()) + + policiesMap := admission.PolicyConfigEntryMap{} + policiesMap[policy.GetUniqueName()] = admission.PolicyServerConfigEntry{ + NamespacedName: types.NamespacedName{ + Namespace: policy.GetNamespace(), + Name: policy.GetName(), + }, + URL: policy.GetModule(), + PolicyMode: string(policy.GetPolicyMode()), + AllowedToMutate: policy.IsMutating(), + Settings: policy.GetSettings(), + ContextAwareResources: policy.GetContextAwareResources(), + } + policies, err := json.Marshal(policiesMap) + Expect(err).ToNot(HaveOccurred()) + + Eventually(func() error { + _, err := getTestClusterAdmissionPolicy(policyName) + return err + }, timeout, pollInterval).Should(Succeed()) Eventually(func() error { _, err := getTestPolicyServerConfigMap(policyServerName) return err }, timeout, pollInterval).Should(Succeed()) - configmap, err := getTestPolicyServerConfigMap(policyServerName) + configMap, err := getTestPolicyServerConfigMap(policyServerName) Expect(err).ToNot(HaveOccurred()) - Expect(configmap).To(PointTo(MatchFields(IgnoreExtras, Fields{ + Expect(configMap).To(PointTo(MatchFields(IgnoreExtras, Fields{ "Data": MatchAllKeys(Keys{ - constants.PolicyServerConfigPoliciesEntry: Equal("{}"), + constants.PolicyServerConfigPoliciesEntry: MatchJSON(policies), constants.PolicyServerConfigSourcesEntry: Equal("{}"), }), }))) }) - It("it should create a PDB when policy server is defined with MinAvailable", func() { + It("should create the policy server configmap with the sources authorities", func() { + policyServer := policyServerFactory(policyServerName) + policyServer.Spec.InsecureSources = []string{"localhost:5000"} + policyServer.Spec.SourceAuthorities = map[string][]string{ + "myprivateregistry:5000": {"cert1", "cert2"}, + } + createPolicyServerAndWaitForItsService(policyServer) + sourceAuthoriries := map[string][]map[string]string{} + for uri, certificates := range policyServer.Spec.SourceAuthorities { + certs := []map[string]string{} + for _, cert := range certificates { + certs = append(certs, map[string]string{ + "type": "Data", + "data": cert, + }) + } + sourceAuthoriries[uri] = certs + } + sources, err := json.Marshal(map[string]interface{}{ + "insecure_sources": policyServer.Spec.InsecureSources, + "source_authorities": sourceAuthoriries, + }) + Expect(err).ToNot(HaveOccurred()) + + Eventually(func() error { + _, err := getTestPolicyServerConfigMap(policyServerName) + return err + }, timeout, pollInterval).Should(Succeed()) + configMap, err := getTestPolicyServerConfigMap(policyServerName) + Expect(err).ToNot(HaveOccurred()) + Expect(configMap).To(PointTo(MatchFields(IgnoreExtras, Fields{ + "Data": MatchAllKeys(Keys{ + constants.PolicyServerConfigPoliciesEntry: Equal("{}"), + constants.PolicyServerConfigSourcesEntry: MatchJSON(sources), + }), + }))) + }) + + It("should create PodDisruptionBudget when policy server has MinAvailable configuration set", func() { + + policyServer := policyServerFactory(policyServerName) minAvailable := intstr.FromInt(2) policyServer.Spec.MinAvailable = &minAvailable createPolicyServerAndWaitForItsService(policyServer) + Eventually(func() *k8spoliciesv1.PodDisruptionBudget { pdb, _ := getPolicyServerPodDisruptionBudget(policyServerName) return pdb }, timeout, pollInterval).Should(policyServerPodDisruptionBudgetMatcher(policyServer, &minAvailable, nil)) }) - It("should create a PodDisruptionBudget when policy server is defined with MaxUnavailable", func() { + It("should create PodDisruptionBudget when policy server has MaxUnavailable configuration set", func() { + policyServer := policyServerFactory(policyServerName) maxUnavailable := intstr.FromInt(2) policyServer.Spec.MaxUnavailable = &maxUnavailable createPolicyServerAndWaitForItsService(policyServer) + Eventually(func() *k8spoliciesv1.PodDisruptionBudget { pdb, _ := getPolicyServerPodDisruptionBudget(policyServerName) return pdb }, timeout, pollInterval).Should(policyServerPodDisruptionBudgetMatcher(policyServer, nil, &maxUnavailable)) }) - It("it should not create PDB when policy server is defined with no PodDisruptionBudget", func() { + It("should not create PodDisruptionBudget when policy server has no PDB configuration", func() { + policyServer := policyServerFactory(policyServerName) createPolicyServerAndWaitForItsService(policyServer) Consistently(func() error { _, err := getPolicyServerPodDisruptionBudget(policyServerName) @@ -193,8 +396,50 @@ var _ = Describe("PolicyServer controller", func() { }, consistencyTimeout, pollInterval).ShouldNot(Succeed()) }) - It("should create a PDB if the policy server definition is updated with a PodDisruptionBudget configuration", func() { + It("should create the PolicyServer pod with the limits and the requests", func() { + policyServer := policyServerFactory(policyServerName) + policyServer.Spec.Limits = corev1.ResourceList{ + "cpu": resource.MustParse("100m"), + "memory": resource.MustParse("1Gi"), + } createPolicyServerAndWaitForItsService(policyServer) + By("creating a deployment with limits and requests set") + Eventually(func() error { + deployment, err := getTestPolicyServerDeployment(policyServerName) + if err != nil { + return err + } + Expect(deployment.Spec.Template.Spec.Containers[0].Resources.Limits).To(Equal(policyServer.Spec.Limits)) + return nil + }, timeout, pollInterval).Should(Succeed()) + + By("creating a pod with limit and request set") + Eventually(func() error { + pod, err := getTestPolicyServerPod(policyServerName) + if err != nil { + return err + } + + Expect(pod.Spec.Containers[0].Resources.Limits).To(Equal(policyServer.Spec.Limits)) + + By("setting the requests to the same value as the limits") + Expect(pod.Spec.Containers[0].Resources.Requests).To(Equal(policyServer.Spec.Limits)) + + return nil + }, timeout, pollInterval).Should(Succeed()) + }) + + }) + + When("updating the PolicyServer", func() { + var policyServer *policiesv1.PolicyServer + + BeforeEach(func() { + policyServer = policyServerFactory(policyServerName) + createPolicyServerAndWaitForItsService(policyServer) + }) + + It("should create a PDB if the policy server definition is updated with a PodDisruptionBudget configuration", func() { Consistently(func() error { _, err := getPolicyServerPodDisruptionBudget(policyServerName) return err @@ -215,72 +460,250 @@ var _ = Describe("PolicyServer controller", func() { }, timeout, pollInterval).Should(policyServerPodDisruptionBudgetMatcher(policyServer, nil, &maxUnavailable)) }) - Context("with requests and no limits", func() { + It("should update deployment when policy server image change", func() { + deployment, err := getTestPolicyServerDeployment(policyServerName) + Expect(err).ToNot(HaveOccurred()) + oldImage := deployment.Spec.Template.Spec.Containers[0].Image + Eventually(func() error { + policyServer, err := getTestPolicyServer(policyServerName) + if err != nil { + return err + } + policyServer.Spec.Image = "new-image" + return k8sClient.Update(ctx, policyServer) + }).Should(Succeed()) + + Eventually(func() string { + deployment, err := getTestPolicyServerDeployment(policyServerName) + if err != nil { + return "" + } + return deployment.Spec.Template.Spec.Containers[0].Image + }).Should(And(Not(Equal(oldImage)), Equal("new-image"))) + }) - BeforeEach(func() { - policyServer.Spec.Limits = corev1.ResourceList{ - "cpu": resource.MustParse("100m"), - "memory": resource.MustParse("1Gi"), + It("should update deployment when policy server replica size change", func() { + deployment, err := getTestPolicyServerDeployment(policyServerName) + Expect(err).ToNot(HaveOccurred()) + oldReplica := deployment.Spec.Replicas + Eventually(func() error { + policyServer, err := getTestPolicyServer(policyServerName) + if err != nil { + return err } - createPolicyServerAndWaitForItsService(policyServer) - }) + policyServer.Spec.Replicas = 2 + return k8sClient.Update(ctx, policyServer) + }).Should(Succeed()) + + Eventually(func() int32 { + deployment, err := getTestPolicyServerDeployment(policyServerName) + if err != nil { + return 0 + } + return *deployment.Spec.Replicas + }).Should(And(Not(Equal(oldReplica)), Equal(int32(2)))) + }) - It("should create the PolicyServer pod with the limits and the requests", func() { - By("creating a deployment with limits and requests set") - Eventually(func() error { - deployment, err := getTestPolicyServerDeployment(policyServerName) - if err != nil { - return err - } - Expect(deployment.Spec.Template.Spec.Containers[0].Resources.Limits).To(Equal(policyServer.Spec.Limits)) + It("should update deployment when policy server service account change", func() { + deployment, err := getTestPolicyServerDeployment(policyServerName) + Expect(err).ToNot(HaveOccurred()) + oldServiceAccount := deployment.Spec.Template.Spec.ServiceAccountName + Eventually(func() error { + policyServer, err := getTestPolicyServer(policyServerName) + if err != nil { + return err + } + policyServer.Spec.ServiceAccountName = "new-service-account" + return k8sClient.Update(ctx, policyServer) + }).Should(Succeed()) + + Eventually(func() string { + deployment, err := getTestPolicyServerDeployment(policyServerName) + if err != nil { + return "" + } + return deployment.Spec.Template.Spec.ServiceAccountName + }).Should(And(Not(Equal(oldServiceAccount)), Equal("new-service-account"))) + }) + + It("should update deployment when policy server security context change", func() { + deployment, err := getTestPolicyServerDeployment(policyServerName) + Expect(err).ToNot(HaveOccurred()) + oldSecurityContext := deployment.Spec.Template.Spec.SecurityContext + newUser := int64(1000) + Eventually(func() error { + policyServer, err := getTestPolicyServer(policyServerName) + if err != nil { + return err + } + policyServer.Spec.SecurityContexts.Pod = &corev1.PodSecurityContext{ + RunAsUser: &newUser, + } + return k8sClient.Update(ctx, policyServer) + }).Should(Succeed()) + + Eventually(func() *corev1.PodSecurityContext { + deployment, err := getTestPolicyServerDeployment(policyServerName) + if err != nil { return nil - }, timeout, pollInterval).Should(Succeed()) + } + return deployment.Spec.Template.Spec.SecurityContext + }).Should(And(PointTo(MatchFields(IgnoreExtras, Fields{ + "RunAsUser": PointTo(BeNumerically("==", newUser)), + })), Not(PointTo(Equal(oldSecurityContext))))) + }) - By("creating a pod with limit and request set") - Eventually(func() error { - pod, err := getTestPolicyServerPod(policyServerName) - if err != nil { - return err - } + It("should update deployment when policy server affinity configuration change", func() { + deployment, err := getTestPolicyServerDeployment(policyServerName) + Expect(err).ToNot(HaveOccurred()) + oldAffinity := deployment.Spec.Template.Spec.Affinity + newAffinity := corev1.Affinity{ + NodeAffinity: &corev1.NodeAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: &corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{ + { + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: "label", + Operator: corev1.NodeSelectorOpIn, + Values: []string{"nodename"}, + }, + }, + }, + }, + }, + }, + } + Eventually(func() error { + policyServer, err := getTestPolicyServer(policyServerName) + if err != nil { + return err + } + policyServer.Spec.Affinity = newAffinity + return k8sClient.Update(ctx, policyServer) + }).Should(Succeed()) - Expect(pod.Spec.Containers[0].Resources.Limits).To(Equal(policyServer.Spec.Limits)) + Eventually(func() *corev1.Affinity { + deployment, err := getTestPolicyServerDeployment(policyServerName) + if err != nil { + return nil + } + return deployment.Spec.Template.Spec.Affinity + }).Should(And(Not(PointTo(Equal(oldAffinity))), PointTo(Equal(newAffinity)))) + }) - By("setting the requests to the same value as the limits") - Expect(pod.Spec.Containers[0].Resources.Requests).To(Equal(policyServer.Spec.Limits)) + It("should update deployment when policy server annotations change", func() { + deployment, err := getTestPolicyServerDeployment(policyServerName) + Expect(err).ToNot(HaveOccurred()) + oldAnnotations := deployment.Spec.Template.Annotations + newAnnotations := map[string]string{ + "new-annotation": "new-value", + } + Eventually(func() error { + policyServer, err := getTestPolicyServer(policyServerName) + if err != nil { + return err + } + policyServer.Spec.Annotations = newAnnotations + return k8sClient.Update(ctx, policyServer) + }).Should(Succeed()) + Eventually(func() map[string]string { + deployment, err := getTestPolicyServerDeployment(policyServerName) + if err != nil { return nil - }, timeout, pollInterval).Should(Succeed()) - }) + } + return deployment.Spec.Template.Annotations + }).Should(And(Not(Equal(oldAnnotations)), Equal(newAnnotations))) + }) - It("when the requests are updated should update the PolicyServer pod with the new requests", func() { - By("updating the PolicyServer requests") - updatedRequestsResources := corev1.ResourceList{ - "cpu": resource.MustParse("50m"), - "memory": resource.MustParse("500Mi"), + It("should update deployment when policy server resources limits change", func() { + deployment, err := getTestPolicyServerDeployment(policyServerName) + Expect(err).ToNot(HaveOccurred()) + oldContainers := deployment.Spec.Template.Spec.Containers + newResourceLimits := corev1.ResourceList{ + "cpu": resource.MustParse("100m"), + "memory": resource.MustParse("1Gi"), + } + Eventually(func() error { + policyServer, err := getTestPolicyServer(policyServerName) + if err != nil { + return err } - Eventually(func() error { - policyServer, err := getTestPolicyServer(policyServerName) - if err != nil { - return err - } - policyServer.Spec.Requests = updatedRequestsResources - return k8sClient.Update(ctx, policyServer) - }).Should(Succeed()) + policyServer.Spec.Limits = newResourceLimits + return k8sClient.Update(ctx, policyServer) + }).Should(Succeed()) - By("updating the pod with the new requests") - Eventually(func() (*corev1.Container, error) { - pod, err := getTestPolicyServerPod(policyServerName) - if err != nil { - return nil, err - } - return &pod.Spec.Containers[0], nil - }, timeout, pollInterval).Should( - And( - HaveField("Resources.Requests", Equal(updatedRequestsResources)), - HaveField("Resources.Limits", Equal(policyServer.Spec.Limits)), - ), - ) - }) + Eventually(func() []corev1.Container { + deployment, err := getTestPolicyServerDeployment(policyServerName) + if err != nil { + return nil + } + return deployment.Spec.Template.Spec.Containers + }).Should(And(ContainElement(MatchFields(IgnoreExtras, Fields{ + "Resources": MatchFields(IgnoreExtras, Fields{ + "Limits": Equal(newResourceLimits), + }), + })), Not(Equal(oldContainers)))) + }) + + It("should update deployment when policy server environment variables change", func() { + deployment, err := getTestPolicyServerDeployment(policyServerName) + Expect(err).ToNot(HaveOccurred()) + oldContainers := deployment.Spec.Template.Spec.Containers + newEnvironmentVariable := corev1.EnvVar{ + Name: "NEW_ENV", + Value: "new-value", + } + Eventually(func() error { + policyServer, err := getTestPolicyServer(policyServerName) + if err != nil { + return err + } + policyServer.Spec.Env = []corev1.EnvVar{newEnvironmentVariable} + return k8sClient.Update(ctx, policyServer) + }).Should(Succeed()) + + Eventually(func() []corev1.Container { + deployment, err := getTestPolicyServerDeployment(policyServerName) + if err != nil { + return nil + } + return deployment.Spec.Template.Spec.Containers + }).Should(And(ContainElement(MatchFields(IgnoreExtras, Fields{ + "Env": ContainElement(Equal(newEnvironmentVariable)), + })), Not(Equal(oldContainers)))) }) + + It("should update the PolicyServer pod with the new requests when the requests are updated", func() { + By("updating the PolicyServer requests") + updatedRequestsResources := corev1.ResourceList{ + "cpu": resource.MustParse("50m"), + "memory": resource.MustParse("500Mi"), + } + Eventually(func() error { + policyServer, err := getTestPolicyServer(policyServerName) + if err != nil { + return err + } + policyServer.Spec.Requests = updatedRequestsResources + return k8sClient.Update(ctx, policyServer) + }).Should(Succeed()) + + By("updating the pod with the new requests") + Eventually(func() (*corev1.Container, error) { + pod, err := getTestPolicyServerPod(policyServerName) + if err != nil { + return nil, err + } + return &pod.Spec.Containers[0], nil + }, timeout, pollInterval).Should( + And( + HaveField("Resources.Requests", Equal(updatedRequestsResources)), + HaveField("Resources.Limits", Equal(policyServer.Spec.Limits)), + ), + ) + }) + }) }) diff --git a/internal/pkg/admission/mutating-webhook.go b/internal/pkg/admission/mutating-webhook.go index a15ab3e4..d9ea6753 100644 --- a/internal/pkg/admission/mutating-webhook.go +++ b/internal/pkg/admission/mutating-webhook.go @@ -4,20 +4,18 @@ import ( "context" "fmt" "path/filepath" - "reflect" - "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" policiesv1 "github.com/kubewarden/kubewarden-controller/pkg/apis/policies/v1" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/kubewarden/kubewarden-controller/internal/pkg/constants" ) -//+kubebuilder:rbac:groups=admissionregistration.k8s.io,resources=mutatingwebhookconfigurations,verbs=get;list;watch;create;update;patch +//+kubebuilder:rbac:groups=admissionregistration.k8s.io,resources=mutatingwebhookconfigurations,verbs=create;delete;list;patch;watch func (r *Reconciler) ReconcileMutatingWebhookConfiguration( ctx context.Context, @@ -25,97 +23,43 @@ func (r *Reconciler) ReconcileMutatingWebhookConfiguration( admissionSecret *corev1.Secret, policyServerNameWithPrefix string, ) error { - webhook := r.mutatingWebhookConfiguration(policy, admissionSecret, policyServerNameWithPrefix) - err := r.Client.Create(ctx, webhook) - if err == nil { - return nil - } - if apierrors.IsAlreadyExists(err) { - return r.updateMutatingWebhook(ctx, policy, webhook) - } - return fmt.Errorf("cannot reconcile mutating webhook: %w", err) -} - -func (r *Reconciler) updateMutatingWebhook(ctx context.Context, - policy policiesv1.Policy, - newWebhook *admissionregistrationv1.MutatingWebhookConfiguration, -) error { - var originalWebhook admissionregistrationv1.MutatingWebhookConfiguration - - err := r.Client.Get(ctx, client.ObjectKey{ - Name: policy.GetUniqueName(), - }, &originalWebhook) - if err != nil && apierrors.IsNotFound(err) { - return fmt.Errorf("cannot retrieve mutating webhook: %w", err) - } - - patch := originalWebhook.DeepCopy() - - if patch.ObjectMeta.Labels == nil { - patch.ObjectMeta.Labels = make(map[string]string) - } - for key, value := range newWebhook.ObjectMeta.Labels { - patch.ObjectMeta.Labels[key] = value - } - - if patch.ObjectMeta.Annotations == nil { - patch.ObjectMeta.Annotations = make(map[string]string) - } - for key, value := range newWebhook.ObjectMeta.Annotations { - patch.ObjectMeta.Annotations[key] = value - } - - if !reflect.DeepEqual(originalWebhook.Webhooks, newWebhook.Webhooks) { - patch.Webhooks = newWebhook.Webhooks - } - - err = r.Client.Patch(ctx, patch, client.MergeFrom(&originalWebhook)) - if err != nil { - return fmt.Errorf("cannot patch mutating webhook: %w", err) - } - - return nil -} - -func (r *Reconciler) mutatingWebhookConfiguration( - policy policiesv1.Policy, - admissionSecret *corev1.Secret, - policyServerName string, -) *admissionregistrationv1.MutatingWebhookConfiguration { - admissionPath := filepath.Join("/validate", policy.GetUniqueName()) - admissionPort := int32(constants.PolicyServerPort) - - service := admissionregistrationv1.ServiceReference{ - Namespace: r.DeploymentsNamespace, - Name: policyServerName, - Path: &admissionPath, - Port: &admissionPort, - } - - sideEffects := policy.GetSideEffects() - if sideEffects == nil { - noneSideEffects := admissionregistrationv1.SideEffectClassNone - sideEffects = &noneSideEffects - } - - policyScope := "namespace" - if policy.GetNamespace() == "" { - policyScope = "cluster" - } - - return &admissionregistrationv1.MutatingWebhookConfiguration{ + webhook := &admissionregistrationv1.MutatingWebhookConfiguration{ ObjectMeta: metav1.ObjectMeta{ Name: policy.GetUniqueName(), - Labels: map[string]string{ - "kubewarden": "true", - constants.WebhookConfigurationPolicyScopeLabelKey: policyScope, - }, - Annotations: map[string]string{ - constants.WebhookConfigurationPolicyNameAnnotationKey: policy.GetName(), - constants.WebhookConfigurationPolicyNamespaceAnnotationKey: policy.GetNamespace(), - }, }, - Webhooks: []admissionregistrationv1.MutatingWebhook{ + } + _, err := controllerutil.CreateOrUpdate(ctx, r.Client, webhook, func() error { + admissionPath := filepath.Join("/validate", policy.GetUniqueName()) + admissionPort := int32(constants.PolicyServerPort) + + service := admissionregistrationv1.ServiceReference{ + Namespace: r.DeploymentsNamespace, + Name: policyServerNameWithPrefix, + Path: &admissionPath, + Port: &admissionPort, + } + + sideEffects := policy.GetSideEffects() + if sideEffects == nil { + noneSideEffects := admissionregistrationv1.SideEffectClassNone + sideEffects = &noneSideEffects + } + + policyScope := "namespace" + if policy.GetNamespace() == "" { + policyScope = "cluster" + } + + webhook.Name = policy.GetUniqueName() + webhook.Labels = map[string]string{ + "kubewarden": "true", + constants.WebhookConfigurationPolicyScopeLabelKey: policyScope, + } + webhook.Annotations = map[string]string{ + constants.WebhookConfigurationPolicyNameAnnotationKey: policy.GetName(), + constants.WebhookConfigurationPolicyNamespaceAnnotationKey: policy.GetNamespace(), + } + webhook.Webhooks = []admissionregistrationv1.MutatingWebhook{ { Name: fmt.Sprintf("%s.kubewarden.admission", policy.GetUniqueName()), ClientConfig: admissionregistrationv1.WebhookClientConfig{ @@ -131,6 +75,11 @@ func (r *Reconciler) mutatingWebhookConfiguration( TimeoutSeconds: policy.GetTimeoutSeconds(), AdmissionReviewVersions: []string{"v1"}, }, - }, + } + return nil + }) + if err != nil { + return fmt.Errorf("cannot reconcile mutating webhook: %w", err) } + return nil } diff --git a/internal/pkg/admission/policy-server-configmap.go b/internal/pkg/admission/policy-server-configmap.go index aef450de..2144484d 100644 --- a/internal/pkg/admission/policy-server-configmap.go +++ b/internal/pkg/admission/policy-server-configmap.go @@ -4,17 +4,16 @@ import ( "context" "encoding/json" "fmt" - "reflect" 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" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "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" @@ -41,7 +40,7 @@ type policyServerSourceAuthority struct { } //nolint:tagliatelle -type policyServerSourcesEntry struct { +type PolicyServerSourcesEntry struct { InsecureSources []string `json:"insecure_sources,omitempty"` SourceAuthorities map[string][]policyServerSourceAuthority `json:"source_authorities,omitempty"` } @@ -52,96 +51,30 @@ func (r *Reconciler) reconcilePolicyServerConfigMap( policyServer *policiesv1.PolicyServer, policies []policiesv1.Policy, ) error { - cfg := &corev1.ConfigMap{} - err := r.Client.Get(ctx, client.ObjectKey{ - Namespace: r.DeploymentsNamespace, - Name: policyServer.NameWithPrefix(), - }, cfg) - if err != nil { - if apierrors.IsNotFound(err) { - return r.createPolicyServerConfigMap(ctx, policyServer, policies) - } - return fmt.Errorf("cannot lookup Policy server ConfigMap: %w", err) - } - - return r.updateIfNeeded(ctx, cfg, policies, policyServer) -} - -func (r *Reconciler) updateIfNeeded(ctx context.Context, cfg *corev1.ConfigMap, - policies []policiesv1.Policy, - policyServer *policiesv1.PolicyServer) error { - newPoliciesMap := r.createPoliciesMap(policies) - newSourcesList := r.createSourcesMap(policyServer) - - var ( - shouldUpdatePolicies, shouldUpdateSources bool - err error - ) - if shouldUpdatePolicies, err = shouldUpdatePolicyMap(cfg.Data[constants.PolicyServerConfigPoliciesEntry], newPoliciesMap); err != nil { - return fmt.Errorf("cannot compare policies: %w", err) - } - if shouldUpdateSources, err = shouldUpdateSourcesList(cfg.Data[constants.PolicyServerConfigSourcesEntry], - newSourcesList); err != nil { - return fmt.Errorf("cannot compare insecureSources: %w", err) - } - if !(shouldUpdatePolicies || shouldUpdateSources) { - return nil - } - - patch := cfg.DeepCopy() - if shouldUpdatePolicies { - newPoliciesYML, err := json.Marshal(newPoliciesMap) - if err != nil { - return fmt.Errorf("cannot marshal policies: %w", err) - } - patch.Data[constants.PolicyServerConfigPoliciesEntry] = string(newPoliciesYML) - } - if shouldUpdateSources { - newSourcesYML, err := json.Marshal(newSourcesList) - if err != nil { - return fmt.Errorf("cannot marshal insecureSources: %w", err) - } - patch.Data[constants.PolicyServerConfigSourcesEntry] = string(newSourcesYML) + cfg := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: policyServer.NameWithPrefix(), + Namespace: r.DeploymentsNamespace, + }, } - err = r.Client.Patch(ctx, patch, client.MergeFrom(cfg)) + _, err := controllerutil.CreateOrUpdate(ctx, r.Client, cfg, func() error { + return r.updateConfigMapData(cfg, policyServer, policies) + }) if err != nil { - return fmt.Errorf("cannot patch PolicyServer Configmap: %w", err) + return fmt.Errorf("cannot create or update PolicyServer ConfigMap: %w", err) } - return nil } -func shouldUpdatePolicyMap(currentPoliciesYML string, newPoliciesMap PolicyConfigEntryMap) (bool, error) { - var currentPoliciesMap PolicyConfigEntryMap - - if err := json.Unmarshal([]byte(currentPoliciesYML), ¤tPoliciesMap); err != nil { - return false, fmt.Errorf("cannot unmarshal policies: %w", err) - } - - return !reflect.DeepEqual(currentPoliciesMap, newPoliciesMap), nil -} - -func shouldUpdateSourcesList(currentSourcesYML string, newSources policyServerSourcesEntry) (bool, error) { - var currentSources policyServerSourcesEntry - if err := json.Unmarshal([]byte(currentSourcesYML), ¤tSources); err != nil { - return false, fmt.Errorf("cannot unmarshal insecureSources: %w", err) - } - - return !reflect.DeepEqual(currentSources, newSources), nil -} - -func (r *Reconciler) createPolicyServerConfigMap( - ctx context.Context, - policyServer *policiesv1.PolicyServer, - policies []policiesv1.Policy, -) error { - policiesMap := r.createPoliciesMap(policies) +// Function used to update the ConfigMap data when creating or updating it +func (r *Reconciler) updateConfigMapData(cfg *corev1.ConfigMap, policyServer *policiesv1.PolicyServer, policies []policiesv1.Policy) error { + policiesMap := buildPoliciesMap(policies) policiesYML, err := json.Marshal(policiesMap) if err != nil { return fmt.Errorf("cannot marshal policies: %w", err) } - sources := r.createSourcesMap(policyServer) + sources := buildSourcesMap(policyServer) sourcesYML, err := json.Marshal(sources) if err != nil { return fmt.Errorf("cannot marshal insecureSources: %w", err) @@ -152,19 +85,11 @@ func (r *Reconciler) createPolicyServerConfigMap( constants.PolicyServerConfigSourcesEntry: string(sourcesYML), } - cfg := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: policyServer.NameWithPrefix(), - Namespace: r.DeploymentsNamespace, - Labels: map[string]string{ - constants.PolicyServerLabelKey: policyServer.ObjectMeta.Name, - }, - }, - Data: data, + cfg.Data = data + cfg.ObjectMeta.Labels = map[string]string{ + constants.PolicyServerLabelKey: policyServer.ObjectMeta.Name, } - - //nolint:wrapcheck - return r.Client.Create(ctx, cfg) + return nil } type PolicyConfigEntryMap map[string]PolicyServerConfigEntry @@ -200,7 +125,7 @@ func (policyConfigEntryMap PolicyConfigEntryMap) ToClusterAdmissionPolicyReconci return res } -func (r *Reconciler) createPoliciesMap(admissionPolicies []policiesv1.Policy) PolicyConfigEntryMap { +func buildPoliciesMap(admissionPolicies []policiesv1.Policy) PolicyConfigEntryMap { policies := PolicyConfigEntryMap{} for _, admissionPolicy := range admissionPolicies { policies[admissionPolicy.GetUniqueName()] = PolicyServerConfigEntry{ @@ -218,8 +143,8 @@ func (r *Reconciler) createPoliciesMap(admissionPolicies []policiesv1.Policy) Po return policies } -func (r *Reconciler) createSourcesMap(policyServer *policiesv1.PolicyServer) policyServerSourcesEntry { - sourcesEntry := policyServerSourcesEntry{} +func buildSourcesMap(policyServer *policiesv1.PolicyServer) PolicyServerSourcesEntry { + sourcesEntry := PolicyServerSourcesEntry{} sourcesEntry.InsecureSources = policyServer.Spec.InsecureSources if sourcesEntry.InsecureSources == nil { sourcesEntry.InsecureSources = make([]string, 0) diff --git a/internal/pkg/admission/policy-server-configmap_test.go b/internal/pkg/admission/policy-server-configmap_test.go index a9cf9ad3..4baf065d 100644 --- a/internal/pkg/admission/policy-server-configmap_test.go +++ b/internal/pkg/admission/policy-server-configmap_test.go @@ -1,7 +1,6 @@ package admission import ( - "encoding/json" "reflect" "testing" @@ -10,71 +9,18 @@ import ( "k8s.io/apimachinery/pkg/types" ) -func TestArePoliciesEqual(t *testing.T) { - tests := []struct { - name string - newPoliciesYML string - currentPoliciesYML string - expect bool - }{{"same nil settings", - "{\"privileged-pods\":{\"url\":\"registry://ghcr.io/kubewarden/policies/pod-privileged:v0.1.5\",\"settings\":null}}", - "{\"privileged-pods\":{\"url\":\"registry://ghcr.io/kubewarden/policies/pod-privileged:v0.1.5\",\"settings\":null}}", - false}, - {"same empty", - "{}", - "{}", - false}, - {"same with settings", - "{\"privileged-pods\":{\"url\":\"registry://ghcr.io/kubewarden/policies/pod-privileged:v0.1.5\",\"settings\":{\"name\":\"test\", \"list\":[\"one\",\"two\"]}}}", - "{\"privileged-pods\":{\"url\":\"registry://ghcr.io/kubewarden/policies/pod-privileged:v0.1.5\",\"settings\":{\"name\":\"test\", \"list\":[\"one\",\"two\"]}}}", - false}, - {"same with settings different order", - "{\"privileged-pods\":{\"url\":\"registry://ghcr.io/kubewarden/policies/pod-privileged:v0.1.5\",\"settings\":{\"name\":\"test\", \"list\":[\"one\",\"two\"]}}}", - "{\"privileged-pods\":{\"settings\":{\"name\":\"test\", \"list\":[\"one\",\"two\"]},\"url\":\"registry://ghcr.io/kubewarden/policies/pod-privileged:v0.1.5\"}}", - false}, - {"2 policies same different order", - "{\"privileged-pods\":{\"url\":\"registry://ghcr.io/kubewarden/policies/pod-privileged:v0.1.5\",\"settings\":null},\"psp-capabilities\":{\"url\":\"registry://ghcr.io/kubewarden/policies/psp-capabilities:v0.1.5\",\"settings\":null}}", - "{\"psp-capabilities\":{\"url\":\"registry://ghcr.io/kubewarden/policies/psp-capabilities:v0.1.5\",\"settings\":null},\"privileged-pods\":{\"url\":\"registry://ghcr.io/kubewarden/policies/pod-privileged:v0.1.5\",\"settings\":null}}", - false}, - {"different", - "{\"privileged-pods\":{\"url\":\"registry://ghcr.io/kubewarden/policies/pod-privileged:v0.1.5\",\"settings\":null}}", - "{\"psp-capabilities\":{\"url\":\"registry://ghcr.io/kubewarden/policies/psp-capabilities:v0.1.5\",\"settings\":null},\"privileged-pods\":{\"url\":\"registry://ghcr.io/kubewarden/policies/pod-privileged:v0.1.5\",\"settings\":null}}", - true}, - {"different settings", - "{\"privileged-pods\":{\"url\":\"registry://ghcr.io/kubewarden/policies/pod-privileged:v0.1.5\",\"settings\":{\"name\":\"test\", \"list\":[\"one\",\"two\"]}}}", - "{\"privileged-pods\":{\"url\":\"registry://ghcr.io/kubewarden/policies/pod-privileged:v0.1.5\",\"settings\":{\"name\":\"test\", \"list\":[\"one\"]}}}", - 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) { - var currentPoliciesMap PolicyConfigEntryMap - if err := json.Unmarshal([]byte(ttest.newPoliciesYML), ¤tPoliciesMap); err != nil { - t.Errorf("unexpected error %s", err.Error()) - } - got, err := shouldUpdatePolicyMap(ttest.currentPoliciesYML, currentPoliciesMap) - if err != nil { - t.Errorf("unexpected error %s", err.Error()) - } - if got != ttest.expect { - t.Errorf("got %t, want %t", got, ttest.expect) - } - }) - } -} - func TestCreatePoliciesMap(t *testing.T) { - reconciler, validationPolicy, mutatingPolicy, contextAwarePolicy := createReconciler() + _, validationPolicy, mutatingPolicy, contextAwarePolicy := createReconciler() var policies PolicyConfigEntryMap clusterAdmissionPolicies := []policiesv1.Policy{} - policies = reconciler.createPoliciesMap(clusterAdmissionPolicies) + policies = buildPoliciesMap(clusterAdmissionPolicies) if len(policies) != 0 { t.Error("Empty ClusterAdmissionPolicyList should generate empty policies map") } clusterAdmissionPolicies = []policiesv1.Policy{&validationPolicy, &mutatingPolicy, &contextAwarePolicy} - policies = reconciler.createPoliciesMap(clusterAdmissionPolicies) + policies = buildPoliciesMap(clusterAdmissionPolicies) if len(policies) != 3 { t.Error("Policy map must has 3 entries") } @@ -118,86 +64,3 @@ func TestCreatePoliciesMap(t *testing.T) { } } } - -func TestShouldUpdateSourcesList(t *testing.T) { - tests := []struct { - name string - currentSourcesYML string - newSourcesList policyServerSourcesEntry - expect bool - }{ - { - "empty sources", - "{}", - policyServerSourcesEntry{}, - false, - }, - { - "add insecure_sources", - "{}", - policyServerSourcesEntry{InsecureSources: []string{"localhost:5000"}}, - true, - }, - { - "remove insecure_sources", - "{\"insecure_sources\":[\"localhost:5000\"]}", - policyServerSourcesEntry{}, - true, - }, - { - "same insecure_sources", - "{\"insecure_sources\":[\"localhost:5000\"]}", - policyServerSourcesEntry{InsecureSources: []string{"localhost:5000"}}, - false, - }, - { - "add source_authorities", - "{}", - policyServerSourcesEntry{ - InsecureSources: []string{}, - SourceAuthorities: map[string][]policyServerSourceAuthority{ - "host.k3d.internal:5000": { - policyServerSourceAuthority{ - Type: "Data", - Data: "pem cert 1", - }, - }, - }, - }, - true, - }, - { - "remove source_authorities", - "{\"source_authorities\":{\"host.k3d.internal:5000\":[{\"type\": \"Data\",\"data\":\"pem cert 1\"}]}}", - policyServerSourcesEntry{}, - true, - }, - { - "same source_authorities", - "{\"source_authorities\":{\"host.k3d.internal:5000\":[{\"type\": \"Data\",\"data\":\"pem cert 1\"}]}}", - policyServerSourcesEntry{ - SourceAuthorities: map[string][]policyServerSourceAuthority{ - "host.k3d.internal:5000": { - policyServerSourceAuthority{ - Type: "Data", - Data: "pem cert 1", - }, - }, - }, - }, - false, - }, - } - for _, test := range tests { - ttest := test // ensure ttest is correctly scoped when used in function literal - t.Run(ttest.name, func(t *testing.T) { - got, err := shouldUpdateSourcesList(ttest.currentSourcesYML, ttest.newSourcesList) - if err != nil { - t.Errorf("unexpected error %s", err.Error()) - } - if got != ttest.expect { - t.Errorf("got %t, want %t", got, ttest.expect) - } - }) - } -} diff --git a/internal/pkg/admission/policy-server-deployment.go b/internal/pkg/admission/policy-server-deployment.go index ee86e4eb..6d9e8339 100644 --- a/internal/pkg/admission/policy-server-deployment.go +++ b/internal/pkg/admission/policy-server-deployment.go @@ -4,16 +4,14 @@ import ( "context" "fmt" "path/filepath" - "reflect" "strconv" policiesv1 "github.com/kubewarden/kubewarden-controller/pkg/apis/policies/v1" appsv1 "k8s.io/api/apps/v1" 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/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,96 +49,173 @@ func (r *Reconciler) reconcilePolicyServerDeployment(ctx context.Context, policy } } - deployment := r.deployment(configMapVersion, policyServer) - err = r.Client.Create(ctx, deployment) - if err == nil { - return nil + policyServerDeployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: policyServer.NameWithPrefix(), + Namespace: r.DeploymentsNamespace, + }, } - if !apierrors.IsAlreadyExists(err) { + _, err = controllerutil.CreateOrUpdate(ctx, r.Client, policyServerDeployment, func() error { + return r.updatePolicyServerDeployment(policyServer, policyServerDeployment, configMapVersion) + }) + if err != nil { return fmt.Errorf("error reconciling policy-server deployment: %w", err) } - - return r.updatePolicyServerDeployment(ctx, policyServer, deployment) + 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{ - Namespace: r.DeploymentsNamespace, - Name: policyServer.NameWithPrefix(), - }, originalDeployment) - if err != nil { - return fmt.Errorf("cannot retrieve existing policy-server Deployment: %w", err) +func (r *Reconciler) updatePolicyServerDeployment(policyServer *policiesv1.PolicyServer, policyServerDeployment *appsv1.Deployment, configMapVersion string) error { + admissionContainer := getPolicyServerContainer(policyServer) + if r.AlwaysAcceptAdmissionReviewsInDeploymentsNamespace { + admissionContainer.Env = append(admissionContainer.Env, corev1.EnvVar{ + Name: "KUBEWARDEN_ALWAYS_ACCEPT_ADMISSION_REVIEWS_ON_NAMESPACE", + Value: r.DeploymentsNamespace, + }) + } + if policyServer.Spec.VerificationConfig != "" { + admissionContainer.VolumeMounts = append(admissionContainer.VolumeMounts, + corev1.VolumeMount{ + Name: verificationConfigVolumeName, + ReadOnly: true, + MountPath: constants.PolicyServerVerificationConfigContainerPath, + }) + admissionContainer.Env = append(admissionContainer.Env, + corev1.EnvVar{ + Name: "KUBEWARDEN_VERIFICATION_CONFIG_PATH", + Value: filepath.Join(constants.PolicyServerVerificationConfigContainerPath, verificationFilename), + }) + } + if policyServer.Spec.ImagePullSecret != "" { + admissionContainer.VolumeMounts = append(admissionContainer.VolumeMounts, + corev1.VolumeMount{ + Name: imagePullSecretVolumeName, + ReadOnly: true, + MountPath: dockerConfigJSONPolicyServerPath, + }) + admissionContainer.Env = append(admissionContainer.Env, + corev1.EnvVar{ + Name: "KUBEWARDEN_DOCKER_CONFIG_JSON_PATH", + Value: dockerConfigJSONPolicyServerPath, + }) + } + if len(policyServer.Spec.InsecureSources) > 0 || len(policyServer.Spec.SourceAuthorities) > 0 { + admissionContainer.VolumeMounts = append(admissionContainer.VolumeMounts, + corev1.VolumeMount{ + Name: sourcesVolumeName, + ReadOnly: true, + MountPath: constants.PolicyServerSourcesConfigContainerPath, + }) + admissionContainer.Env = append(admissionContainer.Env, + corev1.EnvVar{ + Name: "KUBEWARDEN_SOURCES_PATH", + Value: filepath.Join(constants.PolicyServerSourcesConfigContainerPath, sourcesFilename), + }) } - shouldUpdate, err := shouldUpdatePolicyServerDeployment(policyServer, originalDeployment, newDeployment) - if err != nil { - return fmt.Errorf("cannot check if it is necessary to update the policy server deployment: %w", err) + podSecurityContext := &corev1.PodSecurityContext{} + if policyServer.Spec.SecurityContexts.Pod != nil { + podSecurityContext = policyServer.Spec.SecurityContexts.Pod } - if shouldUpdate { - patch := originalDeployment.DeepCopy() - patch.Spec.Replicas = newDeployment.Spec.Replicas - patch.Spec.Template = newDeployment.Spec.Template - patch.ObjectMeta.Annotations[constants.PolicyServerDeploymentConfigVersionAnnotation] = newDeployment.Annotations[constants.PolicyServerDeploymentConfigVersionAnnotation] - err = r.Client.Patch(ctx, patch, client.MergeFrom(originalDeployment)) - if err != nil { - return fmt.Errorf("cannot patch policy-server Deployment: %w", err) - } - r.Log.Info("deployment patched") + if policyServer.Spec.SecurityContexts.Container != nil { + admissionContainer.SecurityContext = policyServer.Spec.SecurityContexts.Container + } else { + admissionContainer.SecurityContext = defaultContainerSecurityContext() } - return nil -} + templateAnnotations := policyServer.Spec.Annotations + if templateAnnotations == nil { + templateAnnotations = make(map[string]string) + } -func getPolicyServerImageFromDeployment(policyServer *policiesv1.PolicyServer, deployment *appsv1.Deployment) (string, error) { - for containerIndex := range deployment.Spec.Template.Spec.Containers { - container := &deployment.Spec.Template.Spec.Containers[containerIndex] - if container.Name == policyServer.NameWithPrefix() { - return container.Image, nil + if r.MetricsEnabled { + templateAnnotations[constants.OptelInjectAnnotation] = "true" + + 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) } } - return "", fmt.Errorf("cannot find policy server container") -} -func isPolicyServerImageChanged(policyServer *policiesv1.PolicyServer, originalDeployment *appsv1.Deployment, newDeployment *appsv1.Deployment) (bool, error) { - var oldImage, newImage string - var err error - if oldImage, err = getPolicyServerImageFromDeployment(policyServer, originalDeployment); err != nil { - return false, err - } - if newImage, err = getPolicyServerImageFromDeployment(policyServer, newDeployment); err != nil { - return false, err - } - return oldImage != newImage, nil -} + if r.TracingEnabled { + templateAnnotations[constants.OptelInjectAnnotation] = "true" -func shouldUpdatePolicyServerDeployment(policyServer *policiesv1.PolicyServer, originalDeployment *appsv1.Deployment, newDeployment *appsv1.Deployment) (bool, error) { - containerImageChanged, err := isPolicyServerImageChanged(policyServer, originalDeployment, newDeployment) - if err != nil { - return false, err + 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 *originalDeployment.Spec.Replicas != *newDeployment.Spec.Replicas || - containerImageChanged || - originalDeployment.Spec.Template.Spec.ServiceAccountName != newDeployment.Spec.Template.Spec.ServiceAccountName || - originalDeployment.Spec.Template.Spec.SecurityContext != newDeployment.Spec.Template.Spec.SecurityContext || - originalDeployment.Annotations[constants.PolicyServerDeploymentConfigVersionAnnotation] != newDeployment.Annotations[constants.PolicyServerDeploymentConfigVersionAnnotation] || - !reflect.DeepEqual(originalDeployment.Spec.Template.Spec.Containers[0].Env, newDeployment.Spec.Template.Spec.Containers[0].Env) || - !reflect.DeepEqual(originalDeployment.Spec.Template.Spec.Containers[0].Resources, newDeployment.Spec.Template.Spec.Containers[0].Resources) || - !reflect.DeepEqual(originalDeployment.Spec.Template.Spec.Affinity, newDeployment.Spec.Template.Spec.Affinity) || - !haveEqualAnnotationsWithoutRestart(originalDeployment, newDeployment), nil -} -func haveEqualAnnotationsWithoutRestart(originalDeployment *appsv1.Deployment, newDeployment *appsv1.Deployment) bool { - if originalDeployment.Spec.Template.Annotations == nil && newDeployment.Spec.Template.Annotations == nil { - return true + policyServerDeployment.Annotations = map[string]string{ + constants.PolicyServerDeploymentConfigVersionAnnotation: configMapVersion, } - annotationsWithoutRestart := make(map[string]string) - for k, v := range originalDeployment.Spec.Template.Annotations { - if k != constants.PolicyServerDeploymentRestartAnnotation { - annotationsWithoutRestart[k] = v - } + policyServerDeployment.Labels = map[string]string{ + constants.AppLabelKey: policyServer.AppLabel(), + constants.PolicyServerLabelKey: policyServer.Name, + } + policyServerDeployment.Spec = appsv1.DeploymentSpec{ + Replicas: &policyServer.Spec.Replicas, + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + constants.AppLabelKey: policyServer.AppLabel(), + }, + }, + Strategy: appsv1.DeploymentStrategy{ + Type: appsv1.RollingUpdateDeploymentStrategyType, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + constants.AppLabelKey: policyServer.AppLabel(), + constants.PolicyServerDeploymentPodSpecConfigVersionLabel: configMapVersion, + constants.PolicyServerLabelKey: policyServer.Name, + }, + Annotations: templateAnnotations, + }, + Spec: corev1.PodSpec{ + SecurityContext: podSecurityContext, + Containers: []corev1.Container{admissionContainer}, + ServiceAccountName: policyServer.Spec.ServiceAccountName, + Volumes: []corev1.Volume{ + { + Name: policyStoreVolume, + VolumeSource: corev1.VolumeSource{ + EmptyDir: &corev1.EmptyDirVolumeSource{}, + }, + }, + { + Name: certsVolumeName, + VolumeSource: corev1.VolumeSource{ + Secret: &corev1.SecretVolumeSource{ + SecretName: policyServer.NameWithPrefix(), + }, + }, + }, + { + Name: policiesVolumeName, + VolumeSource: corev1.VolumeSource{ + ConfigMap: &corev1.ConfigMapVolumeSource{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: policyServer.NameWithPrefix(), + }, + Items: []corev1.KeyToPath{ + { + Key: constants.PolicyServerConfigPoliciesEntry, + Path: policiesFilename, + }, + }, + }, + }, + }, + }, + }, + }, } - return reflect.DeepEqual(annotationsWithoutRestart, newDeployment.Spec.Template.Annotations) + r.adaptDeploymentSettingsForPolicyServer(policyServerDeployment, policyServer) + return nil } func (r *Reconciler) adaptDeploymentSettingsForPolicyServer(policyServerDeployment *appsv1.Deployment, policyServer *policiesv1.PolicyServer) { @@ -213,8 +288,27 @@ func envVarsContainVariable(envVars []corev1.EnvVar, envVarName string) int { return -1 } -func (r *Reconciler) deployment(configMapVersion string, policyServer *policiesv1.PolicyServer) *appsv1.Deployment { - admissionContainer := corev1.Container{ +func defaultContainerSecurityContext() *corev1.SecurityContext { + enableReadOnlyFilesystem := true + privileged := false + runAsNonRoot := true + allowPrivilegeEscalation := false + capabilities := corev1.Capabilities{ + Add: []corev1.Capability{}, + Drop: []corev1.Capability{"all"}, + } + admissionContainerSecurityContext := corev1.SecurityContext{ + ReadOnlyRootFilesystem: &enableReadOnlyFilesystem, + Privileged: &privileged, + AllowPrivilegeEscalation: &allowPrivilegeEscalation, + Capabilities: &capabilities, + RunAsNonRoot: &runAsNonRoot, + } + return &admissionContainerSecurityContext +} + +func getPolicyServerContainer(policyServer *policiesv1.PolicyServer) corev1.Container { + return corev1.Container{ Name: policyServer.NameWithPrefix(), Image: policyServer.Spec.Image, VolumeMounts: []corev1.VolumeMount{ @@ -273,187 +367,4 @@ func (r *Reconciler) deployment(configMapVersion string, policyServer *policiesv Limits: policyServer.Spec.Limits, }, } - if r.AlwaysAcceptAdmissionReviewsInDeploymentsNamespace { - admissionContainer.Env = append(admissionContainer.Env, corev1.EnvVar{ - Name: "KUBEWARDEN_ALWAYS_ACCEPT_ADMISSION_REVIEWS_ON_NAMESPACE", - Value: r.DeploymentsNamespace, - }) - } - if policyServer.Spec.VerificationConfig != "" { - admissionContainer.VolumeMounts = append(admissionContainer.VolumeMounts, - corev1.VolumeMount{ - Name: verificationConfigVolumeName, - ReadOnly: true, - MountPath: constants.PolicyServerVerificationConfigContainerPath, - }, - ) - admissionContainer.Env = append(admissionContainer.Env, - corev1.EnvVar{ - Name: "KUBEWARDEN_VERIFICATION_CONFIG_PATH", - Value: filepath.Join(constants.PolicyServerVerificationConfigContainerPath, verificationFilename), - }, - ) - } - if policyServer.Spec.ImagePullSecret != "" { - admissionContainer.VolumeMounts = append(admissionContainer.VolumeMounts, - corev1.VolumeMount{ - Name: imagePullSecretVolumeName, - ReadOnly: true, - MountPath: dockerConfigJSONPolicyServerPath, - }, - ) - admissionContainer.Env = append(admissionContainer.Env, - corev1.EnvVar{ - Name: "KUBEWARDEN_DOCKER_CONFIG_JSON_PATH", - Value: dockerConfigJSONPolicyServerPath, - }, - ) - } - if len(policyServer.Spec.InsecureSources) > 0 || len(policyServer.Spec.SourceAuthorities) > 0 { - admissionContainer.VolumeMounts = append(admissionContainer.VolumeMounts, - corev1.VolumeMount{ - Name: sourcesVolumeName, - ReadOnly: true, - MountPath: constants.PolicyServerSourcesConfigContainerPath, - }, - ) - admissionContainer.Env = append(admissionContainer.Env, - corev1.EnvVar{ - Name: "KUBEWARDEN_SOURCES_PATH", - Value: filepath.Join(constants.PolicyServerSourcesConfigContainerPath, sourcesFilename), - }, - ) - } - - podSecurityContext := &corev1.PodSecurityContext{} - if policyServer.Spec.SecurityContexts.Pod != nil { - podSecurityContext = policyServer.Spec.SecurityContexts.Pod - } - if policyServer.Spec.SecurityContexts.Container != nil { - admissionContainer.SecurityContext = policyServer.Spec.SecurityContexts.Container - } else { - 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) - } - } - - policyServerDeployment := &appsv1.Deployment{ - ObjectMeta: metav1.ObjectMeta{ - Name: policyServer.NameWithPrefix(), - Namespace: r.DeploymentsNamespace, - Annotations: map[string]string{ - constants.PolicyServerDeploymentConfigVersionAnnotation: configMapVersion, - }, - Labels: map[string]string{ - constants.AppLabelKey: policyServer.AppLabel(), - constants.PolicyServerLabelKey: policyServer.Name, - }, - }, - Spec: appsv1.DeploymentSpec{ - Replicas: &policyServer.Spec.Replicas, - Selector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - constants.AppLabelKey: policyServer.AppLabel(), - }, - }, - Strategy: appsv1.DeploymentStrategy{ - Type: appsv1.RollingUpdateDeploymentStrategyType, - }, - Template: corev1.PodTemplateSpec{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - constants.AppLabelKey: policyServer.AppLabel(), - constants.PolicyServerDeploymentPodSpecConfigVersionLabel: configMapVersion, - constants.PolicyServerLabelKey: policyServer.Name, - }, - Annotations: templateAnnotations, - }, - Spec: corev1.PodSpec{ - SecurityContext: podSecurityContext, - Containers: []corev1.Container{admissionContainer}, - ServiceAccountName: policyServer.Spec.ServiceAccountName, - Volumes: []corev1.Volume{ - { - Name: policyStoreVolume, - VolumeSource: corev1.VolumeSource{ - EmptyDir: &corev1.EmptyDirVolumeSource{}, - }, - }, - { - Name: certsVolumeName, - VolumeSource: corev1.VolumeSource{ - Secret: &corev1.SecretVolumeSource{ - SecretName: policyServer.NameWithPrefix(), - }, - }, - }, - { - Name: policiesVolumeName, - VolumeSource: corev1.VolumeSource{ - ConfigMap: &corev1.ConfigMapVolumeSource{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: policyServer.NameWithPrefix(), - }, - Items: []corev1.KeyToPath{ - { - Key: constants.PolicyServerConfigPoliciesEntry, - Path: policiesFilename, - }, - }, - }, - }, - }, - }, - }, - }, - }, - } - - r.adaptDeploymentSettingsForPolicyServer(policyServerDeployment, policyServer) - - return policyServerDeployment -} - -func defaultContainerSecurityContext() *corev1.SecurityContext { - enableReadOnlyFilesystem := true - privileged := false - runAsNonRoot := true - allowPrivilegeEscalation := false - capabilities := corev1.Capabilities{ - Add: []corev1.Capability{}, - Drop: []corev1.Capability{"all"}, - } - admissionContainerSecurityContext := corev1.SecurityContext{ - ReadOnlyRootFilesystem: &enableReadOnlyFilesystem, - Privileged: &privileged, - AllowPrivilegeEscalation: &allowPrivilegeEscalation, - Capabilities: &capabilities, - RunAsNonRoot: &runAsNonRoot, - } - return &admissionContainerSecurityContext } diff --git a/internal/pkg/admission/policy-server-deployment_test.go b/internal/pkg/admission/policy-server-deployment_test.go index 708128bd..1caa4472 100644 --- a/internal/pkg/admission/policy-server-deployment_test.go +++ b/internal/pkg/admission/policy-server-deployment_test.go @@ -1,454 +1,19 @@ package admission import ( - "path/filepath" "testing" "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" ) const ( - policyServerName = "testing" - policyServerContainerName = "policy-server-testing" - invalidPolicyServerName = "invalid" - dropCapabilityAll = "all" + dropCapabilityAll = "all" ) -func TestShouldUpdatePolicyServerDeployment(t *testing.T) { - deployment := createDeployment(1, "sa", "", "image", nil, []corev1.EnvVar{{Name: "env1"}}, nil, map[string]string{}, nil) - tests := []struct { - name string - original *appsv1.Deployment - new *appsv1.Deployment - expect bool - }{ - {"equal deployments", deployment, createDeployment(1, "sa", "", "image", nil, []corev1.EnvVar{{Name: "env1"}}, nil, map[string]string{}, nil), false}, - {"different replicas", deployment, createDeployment(2, "sa", "", "image", nil, []corev1.EnvVar{{Name: "env1"}}, nil, map[string]string{}, nil), true}, - {"different image", deployment, createDeployment(1, "sa", "", "test", nil, []corev1.EnvVar{{Name: "env1"}}, nil, map[string]string{}, nil), true}, - {"different serviceAccount", deployment, createDeployment(1, "serviceAccount", "", "image", nil, []corev1.EnvVar{{Name: "env1"}}, nil, map[string]string{}, nil), true}, - {"different podSecurityContext", deployment, createDeployment(1, "sa", "", "image", nil, []corev1.EnvVar{{Name: "env1"}}, - &corev1.PodSecurityContext{ - RunAsNonRoot: &[]bool{true}[0], - }, map[string]string{}, nil), true}, - {"new imagePullSecret", deployment, createDeployment(1, "sa", "regcred", "image", nil, []corev1.EnvVar{{Name: "env1"}}, nil, map[string]string{}, nil), true}, - {"different imagePullSecret", createDeployment(1, "sa", "regcred", "image", nil, nil, nil, map[string]string{}, nil), createDeployment(1, "sa", "regcred2", "image", nil, nil, nil, map[string]string{}, nil), false}, - {"new insecureSources", deployment, createDeployment(1, "sa", "regcred", "image", []string{"localhost:5000"}, []corev1.EnvVar{{Name: "env1"}}, nil, map[string]string{}, nil), true}, - {"different insecureSources", createDeployment(1, "sa", "regcred", "image", []string{"localhost:4000"}, nil, nil, map[string]string{}, nil), createDeployment(1, "sa", "regcred2", "image", []string{"localhost:9999"}, nil, nil, map[string]string{}, nil), false}, - {"different env", deployment, createDeployment(1, "sa", "", "image", nil, []corev1.EnvVar{{Name: "env1"}, {Name: "env2"}}, nil, map[string]string{}, nil), true}, - {"different annotation", deployment, createDeployment(1, "sa", "", "image", nil, []corev1.EnvVar{{Name: "env1"}}, nil, map[string]string{"key": "val"}, nil), true}, - {"same nil env", createDeployment(1, "sa", "", "image", nil, nil, nil, map[string]string{}, nil), createDeployment(1, "sa", "", "image", nil, nil, nil, map[string]string{}, nil), false}, - {"same nil annotation", createDeployment(1, "sa", "", "image", nil, []corev1.EnvVar{{Name: "env1"}}, nil, nil, nil), createDeployment(1, "sa", "", "image", nil, []corev1.EnvVar{{Name: "env1"}}, nil, nil, nil), false}, - {`new affinity`, deployment, createDeployment(1, "sa", "", "image", nil, []corev1.EnvVar{{}}, nil, map[string]string{}, createAffinity("nodename")), true}, - { - `different affinity`, - createDeployment(1, "sa", "", "image", nil, []corev1.EnvVar{{}}, nil, map[string]string{}, - createAffinity("nodename"), - ), - createDeployment(1, "sa", "", "image", nil, []corev1.EnvVar{{}}, nil, map[string]string{}, - createAffinity("othernodename"), - ), - true, - }, - } - - policyServer := &policiesv1.PolicyServer{ - Spec: policiesv1.PolicyServerSpec{ - Image: "image", - }, - } - policyServer.Name = policyServerName - for _, test := range tests { - tt := test // ensure tt is correctly scoped when used in function literal - t.Run(tt.name, func(t *testing.T) { - got, _ := shouldUpdatePolicyServerDeployment(policyServer, tt.original, tt.new) - if got != tt.expect { - t.Errorf("got %t, want %t", got, tt.expect) - } - }) - } -} - -func createDeployment(replicasInt int, serviceAccount, imagePullSecret, image string, - insecureSources []string, - env []corev1.EnvVar, - podSecurityContext *corev1.PodSecurityContext, - annotations map[string]string, - affinity *corev1.Affinity, -) *appsv1.Deployment { - replicas := int32(replicasInt) - const ( - imagePullSecretVolumeName = "imagepullsecret" - dockerConfigJSONPolicyServerPath = "/home/kubewarden/.docker" - sourcesVolumeName = "sources" - sourcesConfigContainerPath = "/sources" - sourcesFilename = "sources.yml" - ) - container := corev1.Container{ - Image: image, - Env: env, - Name: policyServerContainerName, - } - if imagePullSecret != "" { - container.VolumeMounts = append(container.VolumeMounts, - corev1.VolumeMount{ - Name: imagePullSecretVolumeName, - ReadOnly: true, - MountPath: dockerConfigJSONPolicyServerPath, - }, - ) - container.Env = append(container.Env, - corev1.EnvVar{ - Name: "KUBEWARDEN_DOCKER_CONFIG_JSON_PATH", - Value: filepath.Join(dockerConfigJSONPolicyServerPath, ".dockerconfigjson"), - }, - ) - } - if len(insecureSources) > 0 { - container.VolumeMounts = append(container.VolumeMounts, - corev1.VolumeMount{ - Name: sourcesVolumeName, - ReadOnly: true, - MountPath: sourcesConfigContainerPath, - }, - ) - container.Env = append(container.Env, - corev1.EnvVar{ - Name: "KUBEWARDEN_SOURCES_PATH", - Value: filepath.Join(sourcesConfigContainerPath, sourcesFilename), - }, - ) - } - - policyServerDeployment := &appsv1.Deployment{ - TypeMeta: metav1.TypeMeta{}, - ObjectMeta: metav1.ObjectMeta{}, - Spec: appsv1.DeploymentSpec{ - Replicas: &replicas, - Selector: nil, - Template: corev1.PodTemplateSpec{ - ObjectMeta: metav1.ObjectMeta{Annotations: annotations}, - Spec: corev1.PodSpec{ - SecurityContext: podSecurityContext, - Containers: []corev1.Container{container}, - ServiceAccountName: serviceAccount, - Affinity: affinity, - }, - }, - Strategy: appsv1.DeploymentStrategy{}, - MinReadySeconds: 0, - RevisionHistoryLimit: nil, - Paused: false, - ProgressDeadlineSeconds: nil, - }, - Status: appsv1.DeploymentStatus{}, - } - if imagePullSecret != "" { - policyServerDeployment.Spec.Template.Spec.Volumes = append( - policyServerDeployment.Spec.Template.Spec.Volumes, - corev1.Volume{ - Name: imagePullSecretVolumeName, - VolumeSource: corev1.VolumeSource{ - Secret: &corev1.SecretVolumeSource{ - SecretName: imagePullSecret, - }, - }, - }, - ) - } - - return policyServerDeployment -} - -func createAffinity(nodeName string) *corev1.Affinity { - return &corev1.Affinity{ - NodeAffinity: &corev1.NodeAffinity{ - RequiredDuringSchedulingIgnoredDuringExecution: &corev1.NodeSelector{ - NodeSelectorTerms: []corev1.NodeSelectorTerm{ - { - MatchExpressions: []corev1.NodeSelectorRequirement{ - { - Key: "label", - Operator: corev1.NodeSelectorOpIn, - Values: []string{nodeName}, - }, - }, - }, - }, - }, - }, - } -} - -func insertContainer(deployment *appsv1.Deployment) { - container := corev1.Container{ - Name: "container0", - Image: "container0image:latest", - } - containers := []corev1.Container{container} - containers = append(containers, deployment.Spec.Template.Spec.Containers...) - deployment.Spec.Template.Spec.Containers = containers -} - -func TestGetPolicyServeImageFromDeployment(t *testing.T) { - policyServer := policiesv1.PolicyServer{ - Spec: policiesv1.PolicyServerSpec{ - Image: "image", - }, - } - policyServer.Name = policyServerName - deployment := createDeployment(1, "sa", "", "image", nil, []corev1.EnvVar{}, nil, map[string]string{}, nil) - image, err := getPolicyServerImageFromDeployment(&policyServer, deployment) - if err != nil || image != "image" { - t.Errorf("The function cannot find the right container image for the policy server container. Expected: 'image', Got: %s", image) - } - deployment.Spec.Template.Spec.Containers[0].Name = "policy-server-default" - image, err = getPolicyServerImageFromDeployment(&policyServer, deployment) - if err == nil || image != "" { - t.Error("The function should not be able to find the container image. Because there is no container with the policy server name") - } -} - -func TestIfPolicyServerImageChanged(t *testing.T) { - policyServer := &policiesv1.PolicyServer{ - Spec: policiesv1.PolicyServerSpec{ - Image: "image", - }, - } - policyServer.Name = policyServerName - oldDeployment := createDeployment(1, "sa", "", "image", nil, []corev1.EnvVar{}, nil, map[string]string{}, nil) - newDeployment := createDeployment(1, "sa", "", "image", nil, []corev1.EnvVar{}, nil, map[string]string{}, nil) - oldDeployment.Spec.Template.Spec.Containers[0].Name = policyServerContainerName - newDeployment.Spec.Template.Spec.Containers[0].Name = policyServerContainerName - - changed, err := isPolicyServerImageChanged(policyServer, oldDeployment, newDeployment) - if changed || err != nil { - t.Errorf("Function should not detect change in the container image. changed: %v, err: %v", changed, err) - return - } - insertContainer(oldDeployment) - changed, err = isPolicyServerImageChanged(policyServer, oldDeployment, newDeployment) - if changed || err != nil { - t.Errorf("Function should not detect change in the container image. changed: %v, err: %v", changed, err) - return - } - insertContainer(newDeployment) - changed, err = isPolicyServerImageChanged(policyServer, oldDeployment, newDeployment) - if changed || err != nil { - t.Errorf("Function should not detect change in the container image. changed: %v, err: %v", changed, err) - return - } - newDeployment.Spec.Template.Spec.Containers[1].Image = "image2" - changed, err = isPolicyServerImageChanged(policyServer, oldDeployment, newDeployment) - if changed == false || err != nil { - t.Errorf("Function should detect change in the container image. changed: %v, err: %s", changed, err) - return - } - - policyServer.Name = invalidPolicyServerName - _, err = isPolicyServerImageChanged(policyServer, oldDeployment, newDeployment) - if err == nil { - t.Errorf("Function should fail to find the container image. err: %v", err) - } -} - -func TestPolicyServerWithContainerSecurityContext(t *testing.T) { - reconciler := Reconciler{ - Client: nil, - DeploymentsNamespace: "kubewarden", - } - readOnlFileSystem := false - privileged := true - runAsRoot := false - allowPrivilegeEscalation := true - capabilities := corev1.Capabilities{ - Drop: []corev1.Capability{corev1.Capability(dropCapabilityAll)}, - } - containerSecurity := corev1.SecurityContext{ - ReadOnlyRootFilesystem: &readOnlFileSystem, - Privileged: &privileged, - RunAsNonRoot: &runAsRoot, - AllowPrivilegeEscalation: &allowPrivilegeEscalation, - Capabilities: &capabilities, - } - policyServer := &policiesv1.PolicyServer{ - Spec: policiesv1.PolicyServerSpec{ - Image: "image", - SecurityContexts: policiesv1.PolicyServerSecurity{ - Container: &containerSecurity, - }, - }, - } - deployment := reconciler.deployment("v1", policyServer) - - if deployment.Spec.Template.Spec.Containers[0].SecurityContext.ReadOnlyRootFilesystem == nil || - *deployment.Spec.Template.Spec.Containers[0].SecurityContext.ReadOnlyRootFilesystem != readOnlFileSystem { - t.Error("Policy server container ReadOnlyRootFilesystem diverge from the expected value") - } - if deployment.Spec.Template.Spec.Containers[0].SecurityContext.Privileged == nil || - *deployment.Spec.Template.Spec.Containers[0].SecurityContext.Privileged != privileged { - t.Error("Policy server container Privileged diverge from the expected value") - } - if deployment.Spec.Template.Spec.Containers[0].SecurityContext.RunAsNonRoot == nil || - *deployment.Spec.Template.Spec.Containers[0].SecurityContext.RunAsNonRoot != runAsRoot { - t.Error("Policy server container RunAsNonRoot diverges from the expected value") - } - if deployment.Spec.Template.Spec.Containers[0].SecurityContext.AllowPrivilegeEscalation == nil || - *deployment.Spec.Template.Spec.Containers[0].SecurityContext.AllowPrivilegeEscalation != allowPrivilegeEscalation { - t.Error("Policy server container AllowPrivilegeEscalation diverge from the expected value") - } - if deployment.Spec.Template.Spec.Containers[0].SecurityContext.Capabilities == nil || - len(deployment.Spec.Template.Spec.Containers[0].SecurityContext.Capabilities.Add) > 0 || - len(deployment.Spec.Template.Spec.Containers[0].SecurityContext.Capabilities.Drop) != 1 || - deployment.Spec.Template.Spec.Containers[0].SecurityContext.Capabilities.Drop[0] != dropCapabilityAll { - t.Error("Policy server container Capabilities diverge from the expected value") - } -} - -func TestPolicyServerWithPodSecurityContext(t *testing.T) { - reconciler := Reconciler{ - Client: nil, - DeploymentsNamespace: "kubewarden", - } - var user, group int64 - user = 1000 - group = 2000 - podSecurity := corev1.PodSecurityContext{ - RunAsUser: &user, - RunAsGroup: &group, - } - policyServer := &policiesv1.PolicyServer{ - Spec: policiesv1.PolicyServerSpec{ - Image: "image", - SecurityContexts: policiesv1.PolicyServerSecurity{ - Pod: &podSecurity, - }, - }, - } - deployment := reconciler.deployment("v1", policyServer) - - if deployment.Spec.Template.Spec.SecurityContext == nil { - t.Error("Pod securityContext should be defined ") - } - - if deployment.Spec.Template.Spec.SecurityContext != nil { - if *deployment.Spec.Template.Spec.SecurityContext.RunAsUser != user { - t.Error("Pod RunAsUser diverges from the expected value") - } - if *deployment.Spec.Template.Spec.SecurityContext.RunAsGroup != group { - t.Error("Pod RunAsGroup diverges from the expected value") - } - } -} - -func TestPolicyServerWithoutSecurityContext(t *testing.T) { - reconciler := Reconciler{ - Client: nil, - DeploymentsNamespace: "kubewarden", - } - policyServer := &policiesv1.PolicyServer{ - Spec: policiesv1.PolicyServerSpec{ - Image: "image", - SecurityContexts: policiesv1.PolicyServerSecurity{}, - }, - } - deployment := reconciler.deployment("v1", policyServer) - containerDefaultSecurityContext := defaultContainerSecurityContext() - - if *deployment.Spec.Template.Spec.Containers[0].SecurityContext.ReadOnlyRootFilesystem != *containerDefaultSecurityContext.ReadOnlyRootFilesystem { - t.Error("Policy server container ReadOnlyRootFilesystem diverge from the expected value") - } - if *deployment.Spec.Template.Spec.Containers[0].SecurityContext.Privileged != *containerDefaultSecurityContext.Privileged { - t.Error("Policy server container Privileged diverge from the expected value") - } - if *deployment.Spec.Template.Spec.Containers[0].SecurityContext.RunAsNonRoot != *containerDefaultSecurityContext.RunAsNonRoot { - t.Error("Policy server container RunAsNonRoot diverges from the expected value") - } - if *deployment.Spec.Template.Spec.Containers[0].SecurityContext.AllowPrivilegeEscalation != *containerDefaultSecurityContext.AllowPrivilegeEscalation { - t.Error("Policy server container AllowPrivilegeEscalation diverge from the expected value") - } - if deployment.Spec.Template.Spec.Containers[0].SecurityContext.Capabilities == containerDefaultSecurityContext.Capabilities { - t.Error("Policy server container should have capabilities defined") - } -} - -func TestPolicyServerWithPodAndContainerSecurityContext(t *testing.T) { - reconciler := Reconciler{ - Client: nil, - DeploymentsNamespace: "kubewarden", - } - readOnlFileSystem := false - privileged := true - runAsRoot := false - allowPrivilegeEscalation := true - capabilities := corev1.Capabilities{ - Drop: []corev1.Capability{corev1.Capability(dropCapabilityAll)}, - } - var user, group int64 - user = 1000 - group = 2000 - podSecurity := corev1.PodSecurityContext{ - RunAsUser: &user, - RunAsGroup: &group, - } - containerSecurity := corev1.SecurityContext{ - ReadOnlyRootFilesystem: &readOnlFileSystem, - Privileged: &privileged, - RunAsNonRoot: &runAsRoot, - AllowPrivilegeEscalation: &allowPrivilegeEscalation, - Capabilities: &capabilities, - } - policyServer := &policiesv1.PolicyServer{ - Spec: policiesv1.PolicyServerSpec{ - Image: "image", - SecurityContexts: policiesv1.PolicyServerSecurity{ - Container: &containerSecurity, - Pod: &podSecurity, - }, - }, - } - deployment := reconciler.deployment("v1", policyServer) - - if *deployment.Spec.Template.Spec.Containers[0].SecurityContext.ReadOnlyRootFilesystem != readOnlFileSystem { - t.Error("Policy server container ReadOnlyRootFilesystem diverge from the expected value") - } - if *deployment.Spec.Template.Spec.Containers[0].SecurityContext.Privileged != privileged { - t.Error("Policy server container Privileged diverge from the expected value") - } - if *deployment.Spec.Template.Spec.Containers[0].SecurityContext.RunAsNonRoot != runAsRoot { - t.Error("Policy server container RunAsNonRoot diverges from the expected value") - } - if *deployment.Spec.Template.Spec.Containers[0].SecurityContext.AllowPrivilegeEscalation != allowPrivilegeEscalation { - t.Error("Policy server container AllowPrivilegeEscalation diverge from the expected value") - } - if deployment.Spec.Template.Spec.Containers[0].SecurityContext.Capabilities == nil { - t.Error("Policy server container should have capabilities defined") - } else { - if len(deployment.Spec.Template.Spec.Containers[0].SecurityContext.Capabilities.Add) > 0 { - t.Error("Policy server container should not have 'Add' capabilities defined") - } - if len(deployment.Spec.Template.Spec.Containers[0].SecurityContext.Capabilities.Drop) != 1 || - deployment.Spec.Template.Spec.Containers[0].SecurityContext.Capabilities.Drop[0] != dropCapabilityAll { - t.Error("Policy server container Capabilities should have only one 'All' drop capability") - } - } - - if deployment.Spec.Template.Spec.SecurityContext == nil { - t.Error("Pod securityContext should be defined ") - return - } - - if *deployment.Spec.Template.Spec.SecurityContext.RunAsUser != user { - t.Error("Pod RunAsUser diverges from the expected value") - } - if *deployment.Spec.Template.Spec.SecurityContext.RunAsGroup != group { - t.Error("Pod RunAsGroup diverges from the expected value") - } -} - func TestDefaultContainerSecurityContext(t *testing.T) { securityContext := defaultContainerSecurityContext() @@ -493,129 +58,47 @@ func TestMetricAndLogFmtEnvVarsDetection(t *testing.T) { } } -func TestPolicyServerDeploymentMetricConfigurationWithValueDefinedByUser(t *testing.T) { //nolint:dupl - reconciler := Reconciler{ - Client: nil, - DeploymentsNamespace: "kubewarden", - MetricsEnabled: true, - } - policyServer := &policiesv1.PolicyServer{ - Spec: policiesv1.PolicyServerSpec{ - Image: "image", - Env: []corev1.EnvVar{{Name: constants.PolicyServerEnableMetricsEnvVar, Value: "false"}, {Name: constants.PolicyServerLogFmtEnvVar, Value: "invalid"}}, - }, +func TestDeploymentMetricsConfiguration(t *testing.T) { + tests := []struct { + name string + metricsEnabled bool + tracingEnabled bool + }{ + {"with metrics enabled", true, false}, + {"with tracing enabled", false, true}, + {"with metrics and tracing enabled", true, true}, + {"with metrics and tracing disabled", false, false}, } - deployment := reconciler.deployment("v1", policyServer) - hasMetricEnvvar := false - for _, envvar := range deployment.Spec.Template.Spec.Containers[0].Env { - if envvar.Name == constants.PolicyServerEnableMetricsEnvVar { - hasMetricEnvvar = true - if envvar.Value != "true" { - t.Error("Present but not reconciled {} value", constants.PolicyServerEnableMetricsEnvVar) + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + reconciler := newReconciler(nil, test.metricsEnabled, test.tracingEnabled) + deployment := &appsv1.Deployment{} + policyServer := &policiesv1.PolicyServer{} + err := reconciler.updatePolicyServerDeployment(policyServer, deployment, "") + require.NoError(t, err) + if test.metricsEnabled || test.tracingEnabled { + require.Contains(t, deployment.Spec.Template.GetAnnotations(), constants.OptelInjectAnnotation, "Deployment pod spec should have OpenTelemetry annotations") + require.Equal(t, "true", deployment.Spec.Template.GetAnnotations()[constants.OptelInjectAnnotation], "Deployment pod spec should have OpenTelemetry annotations value equal to 'true'") } - } - } - if !hasMetricEnvvar { - t.Error("Missing {} environment variable", constants.PolicyServerEnableMetricsEnvVar) - } - - value, hasAnnotation := deployment.Spec.Template.Annotations["sidecar.opentelemetry.io/inject"] - if !hasAnnotation { - t.Error("Missing OTEL annotation") - } - if value != "true" { - t.Error("OTEL annotation invalid value") - } -} - -func TestPolicyServerDeploymentTracingConfigurationWithValueDefinedByUser(t *testing.T) { //nolint:dupl - reconciler := Reconciler{ - Client: nil, - DeploymentsNamespace: "kubewarden", - TracingEnabled: 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) - hasLogFmtEnvvar := false - for _, envvar := range deployment.Spec.Template.Spec.Containers[0].Env { - if envvar.Name == constants.PolicyServerLogFmtEnvVar { - hasLogFmtEnvvar = true - if envvar.Value != "otlp" { - t.Error("Present but not reconciled {} value", constants.PolicyServerLogFmtEnvVar) + if !test.metricsEnabled && !test.tracingEnabled { + require.NotContains(t, deployment.Spec.Template.GetAnnotations(), constants.OptelInjectAnnotation, "Deployment pod spec should not have OpenTelemetry annotations") } - } - } - if !hasLogFmtEnvvar { - t.Error("Missing {} environment variable", constants.PolicyServerLogFmtEnvVar) - } - - value, hasAnnotation := deployment.Spec.Template.Annotations["sidecar.opentelemetry.io/inject"] - if !hasAnnotation { - t.Error("Missing OTEL annotation") - } - if value != "true" { - t.Error("OTEL annotation invalid value") - } -} - -func TestPolicyServerDeploymentMetricConfigurationWithNoValueDefinedByUSer(t *testing.T) { - reconciler := Reconciler{ - Client: nil, - DeploymentsNamespace: "kubewarden", - MetricsEnabled: false, - } - policyServer := &policiesv1.PolicyServer{ - Spec: policiesv1.PolicyServerSpec{ - Image: "image", - Env: []corev1.EnvVar{}, - }, - } - deployment := reconciler.deployment("v1", policyServer) - hasMetricEnvvar := false - hasLogFmtEnvvar := false - for _, envvar := range deployment.Spec.Template.Spec.Containers[0].Env { - if envvar.Name == constants.PolicyServerEnableMetricsEnvVar { - hasMetricEnvvar = true - } - if envvar.Name == constants.PolicyServerLogFmtEnvVar { - hasLogFmtEnvvar = true - } - } - if hasMetricEnvvar { - t.Error("{} should not be set", constants.PolicyServerEnableMetricsEnvVar) - } - if hasLogFmtEnvvar { - t.Error("{} should not be set", constants.PolicyServerLogFmtEnvVar) - } - _, hasAnnotation := deployment.Spec.Template.Annotations["sidecar.opentelemetry.io/inject"] - if hasAnnotation { - t.Error("OTEL annotation should not be set") - } -} + if test.metricsEnabled { + require.Contains(t, deployment.Spec.Template.Spec.Containers[0].Env, + corev1.EnvVar{Name: constants.PolicyServerEnableMetricsEnvVar, Value: "true"}, "Policy server container should have metrics environment variable") + } else { + require.NotContains(t, deployment.Spec.Template.Spec.Containers[0].Env, + corev1.EnvVar{Name: constants.PolicyServerEnableMetricsEnvVar, Value: "true"}, "Policy server container should not have metrics environment variable") + } -func TestPolicyServerDeploymentWithAffinity(t *testing.T) { - reconciler := Reconciler{ - Client: nil, - DeploymentsNamespace: "kubewarden", - MetricsEnabled: false, - } - policyServer := &policiesv1.PolicyServer{ - Spec: policiesv1.PolicyServerSpec{ - Image: "image", - Env: []corev1.EnvVar{}, - Affinity: *createAffinity("nodename"), - }, - } - deployment := reconciler.deployment("v1", policyServer) - if deployment.Spec.Template.Spec.Affinity. - NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution. - NodeSelectorTerms[0].MatchExpressions[0].Values[0] != "nodename" { - t.Error("missing affinity") + if test.tracingEnabled { + require.Contains(t, deployment.Spec.Template.Spec.Containers[0].Env, + corev1.EnvVar{Name: constants.PolicyServerLogFmtEnvVar, Value: "otlp"}, "Policy server container should have tracing environment variable") + } else { + require.NotContains(t, deployment.Spec.Template.Spec.Containers[0].Env, + corev1.EnvVar{Name: constants.PolicyServerLogFmtEnvVar, Value: "otlp"}, "Policy server container should not have tracing environment variable") + } + }) } } 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 697f9ba8..7f70241d 100644 --- a/internal/pkg/admission/policy-server-pod-disruption-budget_test.go +++ b/internal/pkg/admission/policy-server-pod-disruption-budget_test.go @@ -28,7 +28,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", @@ -134,7 +134,7 @@ func TestPDBUpdate(t *testing.T) { IntVal: int32(*test.oldMaxUnavailable), } } - reconciler := newReconciler([]client.Object{oldPDB}, false) + reconciler := newReconciler([]client.Object{oldPDB}, false, false) err := reconciler.reconcilePolicyServerPodDisruptionBudget(context.Background(), policyServer) require.NoError(t, err) @@ -187,7 +187,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..6d43f9d6 100644 --- a/internal/pkg/admission/policy-server-service.go +++ b/internal/pkg/admission/policy-server-service.go @@ -8,9 +8,9 @@ import ( 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" "k8s.io/apimachinery/pkg/util/intstr" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "github.com/kubewarden/kubewarden-controller/internal/pkg/constants" ) @@ -35,39 +35,40 @@ func init() { } func (r *Reconciler) reconcilePolicyServerService(ctx context.Context, policyServer *policiesv1.PolicyServer) error { - service := r.service(policyServer) - err := r.Client.Create(ctx, service) - - if err != nil && apierrors.IsAlreadyExists(err) { - err = r.Client.Update(ctx, service) + svc := corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: policyServer.NameWithPrefix(), + Namespace: r.DeploymentsNamespace, + }, } - if err == nil { + _, err := controllerutil.CreateOrUpdate(ctx, r.Client, &svc, func() error { + r.updateService(&svc, policyServer) return nil + }) + + if err != nil { + return fmt.Errorf("cannot reconcile policy-server service: %w", err) } - return fmt.Errorf("cannot reconcile policy-server service: %w", err) + return nil } -func (r *Reconciler) service(policyServer *policiesv1.PolicyServer) *corev1.Service { - svc := corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: policyServer.NameWithPrefix(), - Namespace: r.DeploymentsNamespace, - Labels: map[string]string{ - constants.AppLabelKey: policyServer.AppLabel(), +func (r *Reconciler) updateService(svc *corev1.Service, policyServer *policiesv1.PolicyServer) { + svc.Name = policyServer.NameWithPrefix() + svc.Namespace = r.DeploymentsNamespace + svc.Labels = map[string]string{ + constants.AppLabelKey: policyServer.AppLabel(), + } + svc.Spec = corev1.ServiceSpec{ + Ports: []corev1.ServicePort{ + { + Name: "policy-server", + Port: constants.PolicyServerPort, + TargetPort: intstr.FromInt(constants.PolicyServerPort), + Protocol: corev1.ProtocolTCP, }, }, - Spec: corev1.ServiceSpec{ - Ports: []corev1.ServicePort{ - { - Name: "policy-server", - Port: constants.PolicyServerPort, - TargetPort: intstr.FromInt(constants.PolicyServerPort), - Protocol: corev1.ProtocolTCP, - }, - }, - Selector: map[string]string{ - constants.AppLabelKey: policyServer.AppLabel(), - }, + Selector: map[string]string{ + constants.AppLabelKey: policyServer.AppLabel(), }, } if r.MetricsEnabled { @@ -80,5 +81,4 @@ func (r *Reconciler) service(policyServer *policiesv1.PolicyServer) *corev1.Serv }, ) } - return &svc } diff --git a/internal/pkg/admission/policy-server-service_test.go b/internal/pkg/admission/policy-server-service_test.go index f5cd6141..9c1e3908 100644 --- a/internal/pkg/admission/policy-server-service_test.go +++ b/internal/pkg/admission/policy-server-service_test.go @@ -5,24 +5,53 @@ import ( "github.com/kubewarden/kubewarden-controller/internal/pkg/constants" policiesv1 "github.com/kubewarden/kubewarden-controller/pkg/apis/policies/v1" - "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" ) -func TestMetricsEnabled(t *testing.T) { - reconciler := newReconciler([]client.Object{}, true) - if !reconciler.MetricsEnabled { - t.Fatal("Metric not enabled") +func TestServiceConfiguration(t *testing.T) { + tests := []struct { + name string + metricsEnabled bool + }{ + {"with metrics enabled", true}, + {"with metrics disabled", false}, } - policyServer := &policiesv1.PolicyServer{ - Spec: policiesv1.PolicyServerSpec{ - Image: "image", - }, - } - service := reconciler.service(policyServer) - for _, port := range service.Spec.Ports { - if port.Port == constants.PolicyServerMetricsPort { - return - } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + reconciler := newReconciler(nil, test.metricsEnabled, true) + service := &corev1.Service{} + policyServer := &policiesv1.PolicyServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + } + reconciler.updateService(service, policyServer) + require.Equal(t, policyServer.AppLabel(), service.Labels[constants.AppLabelKey]) + require.Equal(t, policyServer.NameWithPrefix(), service.Name) + require.Equal(t, reconciler.DeploymentsNamespace, service.Namespace) + require.Contains(t, service.Spec.Ports, corev1.ServicePort{ + Name: "policy-server", + Port: constants.PolicyServerPort, + TargetPort: intstr.FromInt(constants.PolicyServerPort), + Protocol: corev1.ProtocolTCP, + }) + require.Equal(t, map[string]string{ + constants.AppLabelKey: policyServer.AppLabel(), + }, service.Spec.Selector) + + metricsPort := corev1.ServicePort{ + Name: "metrics", + Port: int32(metricsPort), + Protocol: corev1.ProtocolTCP, + } + if test.metricsEnabled { + require.Contains(t, service.Spec.Ports, metricsPort) + } else { + require.NotContains(t, service.Spec.Ports, metricsPort) + } + }) } - t.Error("Policy Server service is missing metrics port") } 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/validating-webhook.go b/internal/pkg/admission/validating-webhook.go index 68af63ef..ef9785f7 100644 --- a/internal/pkg/admission/validating-webhook.go +++ b/internal/pkg/admission/validating-webhook.go @@ -4,11 +4,9 @@ import ( "context" "fmt" "path/filepath" - "reflect" policiesv1 "github.com/kubewarden/kubewarden-controller/pkg/apis/policies/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" - "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" corev1 "k8s.io/api/core/v1" @@ -17,7 +15,7 @@ import ( "github.com/kubewarden/kubewarden-controller/internal/pkg/constants" ) -//+kubebuilder:rbac:groups=admissionregistration.k8s.io,resources=validatingwebhookconfigurations,verbs=get;list;watch;create;update;patch +//+kubebuilder:rbac:groups=admissionregistration.k8s.io,resources=validatingwebhookconfigurations,verbs=create;delete;list;patch;watch func (r *Reconciler) ReconcileValidatingWebhookConfiguration( ctx context.Context, @@ -25,96 +23,42 @@ func (r *Reconciler) ReconcileValidatingWebhookConfiguration( admissionSecret *corev1.Secret, policyServerNameWithPrefix string, ) error { - webhook := r.validatingWebhookConfiguration(policy, admissionSecret, policyServerNameWithPrefix) - err := r.Client.Create(ctx, webhook) - if err == nil { - return nil - } - if apierrors.IsAlreadyExists(err) { - return r.updateValidatingWebhook(ctx, policy, webhook) - } - return fmt.Errorf("cannot reconcile validating webhook: %w", err) -} - -func (r *Reconciler) updateValidatingWebhook(ctx context.Context, - policy policiesv1.Policy, - newWebhook *admissionregistrationv1.ValidatingWebhookConfiguration, -) error { - var originalWebhook admissionregistrationv1.ValidatingWebhookConfiguration - - err := r.Client.Get(ctx, client.ObjectKey{ - Name: policy.GetUniqueName(), - }, &originalWebhook) - if err != nil && apierrors.IsNotFound(err) { - return fmt.Errorf("cannot retrieve validating webhook: %w", err) - } - - patch := originalWebhook.DeepCopy() - - if patch.ObjectMeta.Labels == nil { - patch.ObjectMeta.Labels = make(map[string]string) - } - for key, value := range newWebhook.ObjectMeta.Labels { - patch.ObjectMeta.Labels[key] = value - } - - if patch.ObjectMeta.Annotations == nil { - patch.ObjectMeta.Annotations = make(map[string]string) - } - for key, value := range newWebhook.ObjectMeta.Annotations { - patch.ObjectMeta.Annotations[key] = value - } - if !reflect.DeepEqual(originalWebhook.Webhooks, newWebhook.Webhooks) { - patch.Webhooks = newWebhook.Webhooks - } - - err = r.Client.Patch(ctx, patch, client.MergeFrom(&originalWebhook)) - if err != nil { - return fmt.Errorf("cannot patch validating webhook: %w", err) - } - - return nil -} - -func (r *Reconciler) validatingWebhookConfiguration( - policy policiesv1.Policy, - admissionSecret *corev1.Secret, - policyServerName string, -) *admissionregistrationv1.ValidatingWebhookConfiguration { - admissionPath := filepath.Join("/validate", policy.GetUniqueName()) - admissionPort := int32(constants.PolicyServerPort) - - service := admissionregistrationv1.ServiceReference{ - Namespace: r.DeploymentsNamespace, - Name: policyServerName, - Path: &admissionPath, - Port: &admissionPort, - } - - sideEffects := policy.GetSideEffects() - if sideEffects == nil { - noneSideEffects := admissionregistrationv1.SideEffectClassNone - sideEffects = &noneSideEffects - } - - policyScope := "namespace" - if policy.GetNamespace() == "" { - policyScope = "cluster" - } - - return &admissionregistrationv1.ValidatingWebhookConfiguration{ + webhook := &admissionregistrationv1.ValidatingWebhookConfiguration{ ObjectMeta: metav1.ObjectMeta{ Name: policy.GetUniqueName(), - Labels: map[string]string{ - "kubewarden": "true", - constants.WebhookConfigurationPolicyScopeLabelKey: policyScope, - }, - Annotations: map[string]string{ - constants.WebhookConfigurationPolicyNameAnnotationKey: policy.GetName(), - constants.WebhookConfigurationPolicyNamespaceAnnotationKey: policy.GetNamespace(), - }, }, - Webhooks: []admissionregistrationv1.ValidatingWebhook{ + } + _, err := controllerutil.CreateOrUpdate(ctx, r.Client, webhook, func() error { + admissionPath := filepath.Join("/validate", policy.GetUniqueName()) + admissionPort := int32(constants.PolicyServerPort) + + service := admissionregistrationv1.ServiceReference{ + Namespace: r.DeploymentsNamespace, + Name: policyServerNameWithPrefix, + Path: &admissionPath, + Port: &admissionPort, + } + + sideEffects := policy.GetSideEffects() + if sideEffects == nil { + noneSideEffects := admissionregistrationv1.SideEffectClassNone + sideEffects = &noneSideEffects + } + + policyScope := "namespace" + if policy.GetNamespace() == "" { + policyScope = "cluster" + } + webhook.Name = policy.GetUniqueName() + webhook.Labels = map[string]string{ + "kubewarden": "true", + constants.WebhookConfigurationPolicyScopeLabelKey: policyScope, + } + webhook.Annotations = map[string]string{ + constants.WebhookConfigurationPolicyNameAnnotationKey: policy.GetName(), + constants.WebhookConfigurationPolicyNamespaceAnnotationKey: policy.GetNamespace(), + } + webhook.Webhooks = []admissionregistrationv1.ValidatingWebhook{ { Name: fmt.Sprintf("%s.kubewarden.admission", policy.GetUniqueName()), ClientConfig: admissionregistrationv1.WebhookClientConfig{ @@ -130,6 +74,11 @@ func (r *Reconciler) validatingWebhookConfiguration( TimeoutSeconds: policy.GetTimeoutSeconds(), AdmissionReviewVersions: []string{"v1"}, }, - }, + } + return nil + }) + if err != nil { + return fmt.Errorf("cannot reconcile validating webhook: %w", err) } + return nil }