Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

getRolloutsCommandArgs should return an error from the call to isMergable, rather than burying the error #100

Merged
merged 1 commit into from
Nov 7, 2024
+76 −35
Merged

getRolloutsCommandArgs should return an error from the call to isMergable, rather than burying the error #100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions controllers/configmap_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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"

Expand All @@ -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())

})
Expand Down
32 changes: 22 additions & 10 deletions controllers/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down Expand Up @@ -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{
Expand All @@ -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 {
Expand Down Expand Up @@ -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.

Expand All @@ -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,
Expand Down Expand Up @@ -374,7 +386,7 @@ func rolloutsContainer(cr rolloutsmanagerv1alpha1.RolloutManager) corev1.Contain
},
},
Resources: *containerResources,
}
}, nil

}

Expand Down Expand Up @@ -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 {
Expand All @@ -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
}
68 changes: 47 additions & 21 deletions controllers/deployment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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"
Expand All @@ -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))
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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))
Expand All @@ -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())

Expand All @@ -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()
Expand Down Expand Up @@ -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")

Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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))

Expand All @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand Down Expand Up @@ -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{
Expand All @@ -865,6 +891,6 @@ func deploymentCR(name string, namespace string, rolloutsSelectorLabel string, v
},
}

return deploymentCR
return deploymentCR, nil

}
2 changes: 1 addition & 1 deletion controllers/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
Loading