diff --git a/controllers/configmap_test.go b/controllers/configmap_test.go index d6bce90..024be22 100644 --- a/controllers/configmap_test.go +++ b/controllers/configmap_test.go @@ -9,7 +9,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" appsv1 "k8s.io/api/apps/v1" - v1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -19,7 +18,7 @@ var _ = Describe("ConfigMap Test", func() { var a v1alpha1.RolloutManager var r *RolloutManagerReconciler var sa *corev1.ServiceAccount - var existingDeployment *v1.Deployment + var existingDeployment *appsv1.Deployment const trafficrouterPluginLocation = "https://custom-traffic-plugin-location" const metricPluginLocation = "https://custom-metric-plugin-location" @@ -38,7 +37,11 @@ var _ = Describe("ConfigMap Test", func() { } Expect(r.Client.Create(ctx, sa)).To(Succeed()) - existingDeployment = deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin-test", "tmp-test"}, "linux-test", sa.Name, a) + var err error + + existingDeployment, err = deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin-test", "tmp-test"}, "linux-test", sa.Name, a) + Expect(err).ToNot(HaveOccurred()) + Expect(r.Client.Create(ctx, existingDeployment)).To(Succeed()) }) diff --git a/controllers/deployment.go b/controllers/deployment.go index 1a4b5f7..75e7fd3 100644 --- a/controllers/deployment.go +++ b/controllers/deployment.go @@ -16,7 +16,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) -func generateDesiredRolloutsDeployment(cr rolloutsmanagerv1alpha1.RolloutManager, sa corev1.ServiceAccount) appsv1.Deployment { +func generateDesiredRolloutsDeployment(cr rolloutsmanagerv1alpha1.RolloutManager, sa corev1.ServiceAccount) (appsv1.Deployment, error) { // NOTE: When updating this function, ensure that normalizeDeployment is updated as well. See that function for details. @@ -111,8 +111,12 @@ func generateDesiredRolloutsDeployment(cr rolloutsmanagerv1alpha1.RolloutManager desiredPodSpec.ServiceAccountName = sa.ObjectMeta.Name + rolloutsCont, err := rolloutsContainer(cr) + if err != nil { + return appsv1.Deployment{}, err + } desiredPodSpec.Containers = []corev1.Container{ - rolloutsContainer(cr), + rolloutsCont, } desiredPodSpec.Volumes = []corev1.Volume{ @@ -130,13 +134,16 @@ func generateDesiredRolloutsDeployment(cr rolloutsmanagerv1alpha1.RolloutManager }, } - return desiredDeployment + return desiredDeployment, nil } // Reconcile the Rollouts controller deployment. func (r *RolloutManagerReconciler) reconcileRolloutsDeployment(ctx context.Context, cr rolloutsmanagerv1alpha1.RolloutManager, sa corev1.ServiceAccount) error { - desiredDeployment := generateDesiredRolloutsDeployment(cr, sa) + desiredDeployment, err := generateDesiredRolloutsDeployment(cr, sa) + if err != nil { + return err + } normalizedDesiredDeployment, err := normalizeDeployment(desiredDeployment, cr) if err != nil { @@ -292,7 +299,7 @@ func defaultRolloutsContainerResources() corev1.ResourceRequirements { } } -func rolloutsContainer(cr rolloutsmanagerv1alpha1.RolloutManager) corev1.Container { +func rolloutsContainer(cr rolloutsmanagerv1alpha1.RolloutManager) (corev1.Container, error) { // NOTE: When updating this function, ensure that normalizeDeployment is updated as well. See that function for details. @@ -308,8 +315,13 @@ func rolloutsContainer(cr rolloutsmanagerv1alpha1.RolloutManager) corev1.Contain containerResources = &defaultContainerResources } + commandArgs, err := getRolloutsCommandArgs(cr) + if err != nil { + return corev1.Container{}, err + } + return corev1.Container{ - Args: getRolloutsCommandArgs(cr), + Args: commandArgs, Env: rolloutsEnv, Image: getRolloutsContainerImage(cr), ImagePullPolicy: corev1.PullAlways, @@ -374,7 +386,7 @@ func rolloutsContainer(cr rolloutsmanagerv1alpha1.RolloutManager) corev1.Contain }, }, Resources: *containerResources, - } + }, nil } @@ -661,7 +673,7 @@ func getRolloutsContainerImage(cr rolloutsmanagerv1alpha1.RolloutManager) string } // getRolloutsCommand will return the command for the Rollouts controller component. -func getRolloutsCommandArgs(cr rolloutsmanagerv1alpha1.RolloutManager) []string { +func getRolloutsCommandArgs(cr rolloutsmanagerv1alpha1.RolloutManager) ([]string, error) { args := make([]string, 0) if cr.Spec.NamespaceScoped { @@ -675,9 +687,9 @@ func getRolloutsCommandArgs(cr rolloutsmanagerv1alpha1.RolloutManager) []string extraArgs := cr.Spec.ExtraCommandArgs err := isMergable(extraArgs, args) if err != nil { - return args + return args, err } args = append(args, extraArgs...) - return args + return args, nil } diff --git a/controllers/deployment_test.go b/controllers/deployment_test.go index b186192..3d214e9 100644 --- a/controllers/deployment_test.go +++ b/controllers/deployment_test.go @@ -49,7 +49,8 @@ var _ = Describe("Deployment Test", func() { fetchedDeployment := &appsv1.Deployment{} Expect(fetchObject(ctx, r.Client, a.Namespace, DefaultArgoRolloutsResourceName, fetchedDeployment)).To(Succeed()) - expectedDeployment := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin", "tmp"}, "linux", DefaultArgoRolloutsResourceName, a) + expectedDeployment, err := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin", "tmp"}, "linux", DefaultArgoRolloutsResourceName, a) + Expect(err).ToNot(HaveOccurred()) By("verify that the fetched Deployment matches the desired one") Expect(fetchedDeployment.Name).To(Equal(expectedDeployment.Name)) @@ -67,7 +68,8 @@ var _ = Describe("Deployment Test", func() { When("Rollouts Deployment already exists, but then is modified away from default values", func() { It("should update the Deployment back to default values, but preserve any added annotations/labels", func() { By("create a new Deployment with custom values") - existingDeployment := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin-test", "tmp-test"}, "linux-test", sa.Name, a) + existingDeployment, err := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin-test", "tmp-test"}, "linux-test", sa.Name, a) + Expect(err).ToNot(HaveOccurred()) existingDeployment.Labels["new-label"] = "new-label-value" existingDeployment.Annotations["new-annotation"] = "new-annotation-value" @@ -81,7 +83,8 @@ var _ = Describe("Deployment Test", func() { fetchedDeployment := &appsv1.Deployment{} Expect(fetchObject(ctx, r.Client, a.Namespace, DefaultArgoRolloutsResourceName, fetchedDeployment)).To(Succeed()) - expectedDeployment := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin", "tmp"}, "linux", sa.Name, a) + expectedDeployment, err := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin", "tmp"}, "linux", sa.Name, a) + Expect(err).ToNot(HaveOccurred()) By("verifing that the Deployment has been reconciled back to default values") Expect(fetchedDeployment.Name).To(Equal(expectedDeployment.Name)) @@ -194,7 +197,8 @@ var _ = Describe("Deployment Test", func() { It("should cause the existing Deployment to be deleted, and a new Deployment to be created with the updated .spec.selector", func() { By("create a basic Rollout Deployment") - existingDeployment := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin", "tmp"}, "linux", DefaultArgoRolloutsResourceName, a) + existingDeployment, err := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin", "tmp"}, "linux", DefaultArgoRolloutsResourceName, a) + Expect(err).ToNot(HaveOccurred()) By("assigning a fake UID to the original deployment, so we can detected when it is deleted/recreated") existingDeployment.ObjectMeta.UID = "original-deployment" @@ -224,7 +228,8 @@ var _ = Describe("Deployment Test", func() { Expect(fetchObject(ctx, r.Client, a.Namespace, DefaultArgoRolloutsResourceName, fetchedDeployment)).To(Succeed()) Expect(fetchedDeployment.ObjectMeta.UID).To(Equal(types.UID("")), "UID should be empty, because the original Deployment was deleted and recreated") - expectedDeployment := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin", "tmp"}, "linux", sa.Name, a) + expectedDeployment, err := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin", "tmp"}, "linux", sa.Name, a) + Expect(err).ToNot(HaveOccurred()) By("verifying that the Deployment has been reconciled back to default labels") Expect(fetchedDeployment.Name).To(Equal(expectedDeployment.Name)) @@ -240,10 +245,12 @@ var _ = Describe("Deployment Test", func() { When("the deployment has not changed", func() { It("should not report any difference, both before and after normalization", func() { - expectedDeployment := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin", "tmp"}, "linux", DefaultArgoRolloutsResourceName, a) + expectedDeployment, err := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin", "tmp"}, "linux", DefaultArgoRolloutsResourceName, a) + Expect(err).ToNot(HaveOccurred()) Expect(identifyDeploymentDifference(*expectedDeployment, *expectedDeployment)).To(Equal(""), "comparing the object with itself should always report no differences") - expectedDeployment = deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin", "tmp"}, "linux", DefaultArgoRolloutsResourceName, a) + expectedDeployment, err = deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin", "tmp"}, "linux", DefaultArgoRolloutsResourceName, a) + Expect(err).ToNot(HaveOccurred()) expectedDeploymentNormalized, err := normalizeDeployment(*expectedDeployment, a) Expect(err).To(Succeed()) @@ -258,7 +265,8 @@ var _ = Describe("Deployment Test", func() { It("should ensure the user-defiend labels/annotations are be removed by called to normalizeDeployment, while preserving the values contributed by the operation", func() { - originalDeployment := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin", "tmp"}, "linux", DefaultArgoRolloutsResourceName, a) + originalDeployment, err := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin", "tmp"}, "linux", DefaultArgoRolloutsResourceName, a) + Expect(err).ToNot(HaveOccurred()) By("creating a new object with user added labels/annotations") new := originalDeployment.DeepCopy() @@ -308,9 +316,11 @@ var _ = Describe("Deployment Test", func() { DescribeTable("controller should detect and revert the change", func(fxn func(deployment *appsv1.Deployment)) { By("ensuring that deploymentCR properly detects the change") - defaultDeployment := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin", "tmp"}, "linux", DefaultArgoRolloutsResourceName, a) + defaultDeployment, err := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin", "tmp"}, "linux", DefaultArgoRolloutsResourceName, a) + Expect(err).ToNot(HaveOccurred()) - defaultDeploymentModified := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin", "tmp"}, "linux", DefaultArgoRolloutsResourceName, a) + defaultDeploymentModified, err := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin", "tmp"}, "linux", DefaultArgoRolloutsResourceName, a) + Expect(err).ToNot(HaveOccurred()) Expect(identifyDeploymentDifference(*defaultDeployment, *defaultDeploymentModified)).To(BeEmpty(), "they should be the same before one is modified") @@ -455,7 +465,8 @@ var _ = Describe("Deployment Test", func() { fetchedDeployment := &appsv1.Deployment{} Expect(fetchObject(ctx, r.Client, a.Namespace, DefaultArgoRolloutsResourceName, fetchedDeployment)).To(Succeed()) - expectedDeployment := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin", "tmp"}, "linux", DefaultArgoRolloutsResourceName, a) + expectedDeployment, err := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin", "tmp"}, "linux", DefaultArgoRolloutsResourceName, a) + Expect(err).ToNot(HaveOccurred()) By("verify that the fetched Deployment matches the desired one") Expect(fetchedDeployment.Name).To(Equal(expectedDeployment.Name)) @@ -528,7 +539,8 @@ var _ = Describe("generateDesiredRolloutsDeployment tests", func() { Context("when generating the desired deployment", func() { It("should set the correct metadata on the deployment", func() { - deployment := generateDesiredRolloutsDeployment(cr, sa) + deployment, err := generateDesiredRolloutsDeployment(cr, sa) + Expect(err).ToNot(HaveOccurred()) Expect(deployment.ObjectMeta.Name).To(Equal(DefaultArgoRolloutsResourceName)) Expect(deployment.ObjectMeta.Namespace).To(Equal(cr.Namespace)) @@ -538,7 +550,9 @@ var _ = Describe("generateDesiredRolloutsDeployment tests", func() { }) It("should set the NodeSelector and tolerations if NodePlacement is provided", func() { - deployment := generateDesiredRolloutsDeployment(cr, sa) + deployment, err := generateDesiredRolloutsDeployment(cr, sa) + Expect(err).ToNot(HaveOccurred()) + Expect(deployment.Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"kubernetes.io/os": "linux", "key1": "value1"})) Expect(deployment.Spec.Template.Spec.Tolerations).To(ContainElement(corev1.Toleration{ Key: "key1", @@ -548,18 +562,21 @@ var _ = Describe("generateDesiredRolloutsDeployment tests", func() { It("should set the default node selector if NodePlacement is not provided", func() { cr.Spec.NodePlacement = nil - deployment := generateDesiredRolloutsDeployment(cr, sa) + deployment, err := generateDesiredRolloutsDeployment(cr, sa) + Expect(err).ToNot(HaveOccurred()) Expect(deployment.Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"kubernetes.io/os": "linux"})) Expect(deployment.Spec.Template.Spec.Tolerations).To(BeNil()) }) It("should set the service account name", func() { - deployment := generateDesiredRolloutsDeployment(cr, sa) + deployment, err := generateDesiredRolloutsDeployment(cr, sa) + Expect(err).ToNot(HaveOccurred()) Expect(deployment.Spec.Template.Spec.ServiceAccountName).To(Equal(sa.ObjectMeta.Name)) }) It("should add the correct volumes", func() { - deployment := generateDesiredRolloutsDeployment(cr, sa) + deployment, err := generateDesiredRolloutsDeployment(cr, sa) + Expect(err).ToNot(HaveOccurred()) Expect(deployment.Spec.Template.Spec.Volumes).To(HaveLen(2)) Expect(deployment.Spec.Template.Spec.Volumes).To(ContainElement(corev1.Volume{ Name: "plugin-bin", @@ -587,7 +604,9 @@ var _ = Describe("normalizeDeployment tests to verify that an error is returned" a = *makeTestRolloutManager() // Set up a valid deployment object - deployment = deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin", "tmp"}, "linux", DefaultArgoRolloutsResourceName, a) + var err error + deployment, err = deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin", "tmp"}, "linux", DefaultArgoRolloutsResourceName, a) + Expect(err).ToNot(HaveOccurred()) }) DescribeTable("should return an error when", @@ -790,7 +809,8 @@ var _ = Describe("rolloutsContainer tests", func() { } By("Call rolloutsContainer function") - container := rolloutsContainer(cr) + container, err := rolloutsContainer(cr) + Expect(err).ToNot(HaveOccurred()) By("Verify the environment variables") expectedEnvVars := map[string]string{ @@ -809,7 +829,7 @@ var _ = Describe("rolloutsContainer tests", func() { }) }) -func deploymentCR(name string, namespace string, rolloutsSelectorLabel string, volumeNames []string, nodeSelector string, serviceAccount string, rolloutManager v1alpha1.RolloutManager) *appsv1.Deployment { +func deploymentCR(name string, namespace string, rolloutsSelectorLabel string, volumeNames []string, nodeSelector string, serviceAccount string, rolloutManager v1alpha1.RolloutManager) (*appsv1.Deployment, error) { runAsNonRoot := true deploymentCR := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ @@ -822,6 +842,12 @@ func deploymentCR(name string, namespace string, rolloutsSelectorLabel string, v if rolloutManager.Spec.HA != nil && rolloutManager.Spec.HA.Enabled { replicas = 2 } + + rolloutsContainer, err := rolloutsContainer(rolloutManager) + if err != nil { + return nil, err + } + deploymentCR.Spec = appsv1.DeploymentSpec{ Replicas: &replicas, Selector: &metav1.LabelSelector{ @@ -855,7 +881,7 @@ func deploymentCR(name string, namespace string, rolloutsSelectorLabel string, v "kubernetes.io/os": nodeSelector, }, Containers: []corev1.Container{ - rolloutsContainer(rolloutManager), + rolloutsContainer, }, ServiceAccountName: serviceAccount, SecurityContext: &corev1.PodSecurityContext{ @@ -865,6 +891,6 @@ func deploymentCR(name string, namespace string, rolloutsSelectorLabel string, v }, } - return deploymentCR + return deploymentCR, nil } diff --git a/controllers/utils.go b/controllers/utils.go index 4a08a8a..6e1278f 100644 --- a/controllers/utils.go +++ b/controllers/utils.go @@ -20,7 +20,7 @@ const ( UnsupportedRolloutManagerConfiguration = "when there exists a cluster-scoped RolloutManager on the cluster, there may not exist another: only a single cluster-scoped RolloutManager is supported" UnsupportedRolloutManagerClusterScoped = "when Subscription has environment variable NAMESPACE_SCOPED_ARGO_ROLLOUTS set to True, there may not exist any cluster-scoped RolloutManagers: in this case, only namespace-scoped RolloutManager resources are supported" UnsupportedRolloutManagerNamespaceScoped = "when Subscription has environment variable NAMESPACE_SCOPED_ARGO_ROLLOUTS set to False, there may not exist any namespace-scoped RolloutManagers: only a single cluster-scoped RolloutManager is supported" - UnsupportedRolloutManagerClusterScopedNamespace = "Namespace is not specified in CLUSTER_SCOPED_ARGO_ROLLOUTS_NAMESPACES environment variable of Subscription resource. If you wish to install a cluster-scoped Argo Rollouts instance outside the default namespace, ensure it is defined in CLUSTER_SCOPED_ARGO_ROLLOUTS_NAMESPACES" + UnsupportedRolloutManagerClusterScopedNamespace = "namespace is not specified in CLUSTER_SCOPED_ARGO_ROLLOUTS_NAMESPACES environment variable of Subscription resource. If you wish to install a cluster-scoped Argo Rollouts instance outside the default namespace, ensure it is defined in CLUSTER_SCOPED_ARGO_ROLLOUTS_NAMESPACES" ) // pluginItem is a clone of PluginItem from "github.com/argoproj/argo-rollouts/utils/plugin/types"