fix(labels): do not overwrite user added labels from Deployment's spe…

…c.template

PR #91 brought the ability to propagate labels defined for Authorino CR
down to Deployment. However, more often than not, it's more desired to
have those labels propagated to template and therefore pods which are
which are part of the deploymenit. This change adds the ability for a
user added label to not be overwritten by the authorino-operator

Signed-off-by: Laura Fitzgerald <[email protected]>
Co-authored-by: bartoszmajsak <[email protected]>

controllers/authorino_controller.go

@@ -66,13 +66,12 @@ type AuthorinoReconciler struct {
 // +kubebuilder:rbac:groups="",resources=events,verbs=create;patch;
 // +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch
 // +kubebuilder:rbac:groups="authorino.kuadrant.io",resources=authconfigs,verbs=create;delete;get;list;patch;update;watch
-// +kubebuilder:rbac:groups="authorino.kuadrant.io",resources=authconfigs,verbs=create;delete;get;list;patch;update;watch
 // +kubebuilder:rbac:groups="authorino.kuadrant.io",resources=authconfigs/status,verbs=get;patch;update
 // +kubebuilder:rbac:groups="coordination.k8s.io",resources=leases,verbs=get;list;create;update;
 
 // Reconcile deploys an instance of authorino depending on the settings
-// defined in the API, any change applied to the existings CRs will trigger
-// a new reconcilation to apply the required changes
+// defined in the API, any change applied to the existing CRs will trigger
+// a new reconciliation to apply the required changes
 func (r *AuthorinoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	logger := r.Log.WithValues("authorino", req.NamespacedName)
 
@@ -106,14 +105,21 @@ func (r *AuthorinoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 	}
 
 	// Gets Deployment resource for the authorino instance
-	if existingDeployment, err := r.getAuthorinoDeployment(authorinoInstance); err != nil {
+	existingDeployment, err := r.getAuthorinoDeployment(authorinoInstance)
+	if err != nil {
 		return ctrl.Result{}, r.wrapErrorWithStatusUpdate(logger, authorinoInstance, r.setStatusFailed(statusUnableToGetDeployment),
 			fmt.Errorf("failed to get %s Deployment resource, err: %v", authorinoInstance.Name, err),
 		)
-	} else if existingDeployment == nil {
+	}
+	if existingDeployment == nil {
 		// Creates a new deployment resource to deploy the new authorino instance
-		newDeployment := r.buildAuthorinoDeployment(authorinoInstance)
-		if err := r.Client.Create(context.TODO(), newDeployment); err != nil {
+		newDeployment, err := r.buildAuthorinoDeployment(authorinoInstance)
+		if err != nil {
+			return ctrl.Result{}, r.wrapErrorWithStatusUpdate(logger, authorinoInstance, r.setStatusFailed(statusUnableToBuildDeploymentObject),
+				fmt.Errorf("failed to build %s Deployment resource for creation, err: %v", authorinoInstance.Name, err),
+			)
+		}
+		if err := r.Client.Create(ctx, newDeployment); err != nil {
 			return ctrl.Result{}, r.wrapErrorWithStatusUpdate(
 				logger, authorinoInstance, r.setStatusFailed(statusUnableToCreateDeployment),
 				fmt.Errorf("failed to create %s Deployment resource, err: %v", newDeployment.Name, err),
@@ -126,13 +132,23 @@ func (r *AuthorinoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		// Deployment created successfully - return and requeue
 		return ctrl.Result{Requeue: true}, nil
 	} else {
+
 		// deployment already exists, then build a new resource with the desired changes
 		// and compare them, if changes are encountered apply the desired changes
-		desiredDeployment := r.buildAuthorinoDeployment(authorinoInstance)
-		logger.Info("desiredDeployment", "deployment", desiredDeployment)
-		logger.Info("existingDeployment", "deployment", existingDeployment)
+		desiredDeployment, err := r.buildAuthorinoDeployment(authorinoInstance)
+		if err != nil {
+			return ctrl.Result{}, r.wrapErrorWithStatusUpdate(logger, authorinoInstance, r.setStatusFailed(statusUnableToBuildDeploymentObject),
+				fmt.Errorf("failed to build %s Deployment resource for updating, err: %v", authorinoInstance.Name, err),
+			)
+		}
+		logger.V(1).Info("desiredDeployment", "deployment", desiredDeployment)
+		logger.V(1).Info("existingDeployment", "deployment", existingDeployment)
+		// merge existing key/value pairs back into the desiredDeployment Spec Template Labels so as not to overwrite those.
+		for existingKey, existingValue := range existingDeployment.Spec.Template.Labels {
+			desiredDeployment.Spec.Template.Labels[existingKey] = existingValue
+		}
 		if changed := r.authorinoDeploymentChanges(existingDeployment, desiredDeployment); changed {
-			if err := r.Update(ctx, desiredDeployment); err != nil {
+			if err = r.Update(ctx, desiredDeployment); err != nil {
 				return ctrl.Result{}, r.wrapErrorWithStatusUpdate(
 					logger, authorinoInstance, r.setStatusFailed(statusUnableToUpdateDeployment),
 					fmt.Errorf("failed to update %s Deployment resource, err: %v", desiredDeployment.Name, err),
@@ -188,7 +204,7 @@ func (r *AuthorinoReconciler) getAuthorinoDeployment(authorino *api.Authorino) (
 	return deployment, nil
 }
 
-func (r *AuthorinoReconciler) buildAuthorinoDeployment(authorino *api.Authorino) *k8sapps.Deployment {
+func (r *AuthorinoReconciler) buildAuthorinoDeployment(authorino *api.Authorino) (*k8sapps.Deployment, error) {
 	var containers []k8score.Container
 	var saName = authorino.Name + "-authorino"
 
@@ -259,7 +275,7 @@ func (r *AuthorinoReconciler) buildAuthorinoDeployment(authorino *api.Authorino)
 		volumes = append(volumes, authorinoResources.GetTlsVolume(authorinoTlsCertVolumeName, secretName))
 	}
 
-	// if an external OIDC server is enable mounts a volume to the container
+	// if an external OIDC server is enabled mounts a volume to the container
 	// by using the secret with the certs
 	if enabled := authorino.Spec.OIDCServer.Tls.Enabled; enabled == nil || *enabled {
 		secretName := authorino.Spec.OIDCServer.Tls.CertSecret.Name
@@ -270,7 +286,7 @@ func (r *AuthorinoReconciler) buildAuthorinoDeployment(authorino *api.Authorino)
 	args := r.buildAuthorinoArgs(authorino)
 	var envs []k8score.EnvVar
 
-	// [DEPRECATED] configure authorino using env vars (only for old Authorino versions)
+	// Deprecated: configure authorino using env vars (only for old Authorino versions)
 	authorinoVersion := authorinoVersionFromImageTag(image)
 	if detectEnvVarAuthorinoVersion(authorinoVersion) {
 		envs = r.buildAuthorinoEnv(authorino)
@@ -308,8 +324,8 @@ func (r *AuthorinoReconciler) buildAuthorinoDeployment(authorino *api.Authorino)
 		authorino.Labels,
 	)
 
-	_ = ctrl.SetControllerReference(authorino, deployment, r.Scheme)
-	return deployment
+	err := ctrl.SetControllerReference(authorino, deployment, r.Scheme)
+	return deployment, err
 }
 
 func (r *AuthorinoReconciler) buildAuthorinoArgs(authorino *api.Authorino) []string {
@@ -425,7 +441,7 @@ func (r *AuthorinoReconciler) buildAuthorinoArgs(authorino *api.Authorino) []str
 	return args
 }
 
-// [DEPRECATED] Configures Authorino by defining environment variables (instead of command-line args)
+// Deprecated: Configures Authorino by defining environment variables (instead of command-line args)
 // Kept for backward compatibility with older versions of Authorino (<= v0.10.x)
 func (r *AuthorinoReconciler) buildAuthorinoEnv(authorino *api.Authorino) []k8score.EnvVar {
 	envVar := []k8score.EnvVar{}
@@ -554,6 +570,16 @@ func (r *AuthorinoReconciler) authorinoDeploymentChanges(existingDeployment, des
 		return true
 	}
 
+	// check the labels here to see if there needs to be a change
+	// selector labels are immutable so don't need to check those.
+	if authorinoResources.MapUpdateNeeded(existingDeployment.Labels, desiredDeployment.Labels) {
+		return true
+	}
+
+	if authorinoResources.MapUpdateNeeded(existingDeployment.Spec.Template.Labels, desiredDeployment.Spec.Template.Labels) {
+		return true
+	}
+
 	existingContainer := existingDeployment.Spec.Template.Spec.Containers[0]
 	desiredContainer := desiredDeployment.Spec.Template.Spec.Containers[0]
 

controllers/authorino_controller_test.go

@@ -3,6 +3,7 @@ package controllers
 import (
 	"context"
 	"fmt"
+	"k8s.io/utils/env"
 	"strings"
 	"time"
 
@@ -54,14 +55,6 @@ var _ = Describe("Authorino controller", func() {
 
 			authorinoInstance = newFullAuthorinoInstance()
 			Expect(k8sClient.Create(ctx, authorinoInstance)).To(Succeed())
-
-			nsdName := namespacedName(authorinoInstance.GetNamespace(), authorinoInstance.GetName())
-
-			Eventually(func(ctx context.Context) bool {
-				var authorino api.Authorino
-				err := k8sClient.Get(ctx, nsdName, &authorino)
-				return err == nil && authorinoInstance.Status.Ready()
-			}).WithContext(ctx).Should(BeFalse())
 		})
 
 		It("Should create authorino required services", func(ctx context.Context) {
@@ -73,10 +66,16 @@ var _ = Describe("Authorino controller", func() {
 
 			for _, service := range desiredServices {
 				nsdName := namespacedName(service.GetNamespace(), service.GetName())
-
+				clusterService := &k8score.Service{}
 				Eventually(func(ctx context.Context) error {
-					return k8sClient.Get(ctx, nsdName, &k8score.Service{})
+					return k8sClient.Get(ctx, nsdName, clusterService)
 				}).WithContext(ctx).Should(Succeed())
+
+				Expect(clusterService.Labels).ShouldNot(HaveKeyWithValue("control-plane", "controller-manager"))
+				Expect(clusterService.Labels).ShouldNot(HaveKeyWithValue("authorino-resource", authorinoInstance.Name))
+
+				Expect(clusterService.Spec.Selector).Should(HaveKeyWithValue("control-plane", "controller-manager"))
+				Expect(clusterService.Spec.Selector).Should(HaveKeyWithValue("authorino-resource", authorinoInstance.Name))
 			}
 		})
 
@@ -130,13 +129,23 @@ var _ = Describe("Authorino controller", func() {
 			}).WithContext(ctx).Should(Succeed())
 
 			replicas := int32(testAuthorinoReplicas)
-			image := DefaultAuthorinoImage
+			image := authorinoInstance.Spec.Image
 			existContainer := false
 
-			Expect(deployment.Spec.Replicas).Should(Equal(&replicas))
-			Expect(deployment.Labels).Should(Equal(map[string]string{"thisLabel": "willPropagate"}))
+			Expect(deployment.Spec.Replicas).To(Equal(&replicas))
+
+			Expect(deployment.Labels).ShouldNot(HaveKeyWithValue("control-plane", "controller-manager"))
+			Expect(deployment.Spec.Selector.MatchLabels).Should(HaveKeyWithValue("control-plane", "controller-manager"))
+			Expect(deployment.Spec.Template.Labels).Should(HaveKeyWithValue("control-plane", "controller-manager"))
+
+			Expect(deployment.Labels).ShouldNot(HaveKeyWithValue("authorino-resource", authorinoInstance.Name))
+			Expect(deployment.Spec.Selector.MatchLabels).Should(HaveKeyWithValue("authorino-resource", authorinoInstance.Name))
+			Expect(deployment.Spec.Template.Labels).Should(HaveKeyWithValue("authorino-resource", authorinoInstance.Name))
 			for _, container := range deployment.Spec.Template.Spec.Containers {
 				if container.Name == authorinoContainerName {
+					if image == "" {
+						image = env.GetString("RELATED_IMAGE_AUTHORINO", DefaultAuthorinoImage)
+					}
 					Expect(container.Image).Should(Equal(image))
 					Expect(container.ImagePullPolicy).Should(Equal(k8score.PullAlways))
 					checkAuthorinoArgs(authorinoInstance, container.Args)
@@ -170,16 +179,16 @@ var _ = Describe("Authorino controller", func() {
 			existingAuthorinoInstance.Spec.LogLevel = "debug"
 			Expect(k8sClient.Update(context.TODO(), existingAuthorinoInstance)).Should(Succeed())
 
-			desiredDevelopment := &k8sapps.Deployment{}
+			desiredDeployment := &k8sapps.Deployment{}
 
 			Eventually(func(ctx context.Context) error {
 				return k8sClient.Get(ctx,
 					nsdName,
-					desiredDevelopment)
+					desiredDeployment)
 			}).WithContext(ctx).Should(Succeed())
 
-			Expect(desiredDevelopment.Spec.Replicas).Should(Equal(&replicas))
-			for _, container := range desiredDevelopment.Spec.Template.Spec.Containers {
+			Expect(desiredDeployment.Spec.Replicas).Should(Equal(&replicas))
+			for _, container := range desiredDeployment.Spec.Template.Spec.Containers {
 				if container.Name == authorinoContainerName {
 					checkAuthorinoArgs(existingAuthorinoInstance, container.Args)
 					Expect(container.Env).To(BeEmpty())
@@ -188,6 +197,71 @@ var _ = Describe("Authorino controller", func() {
 		})
 	})
 
+	Context("Updating the labels on the deployment", func() {
+		var authorinoInstance *api.Authorino
+
+		BeforeEach(func() {
+			_ = k8sClient.Create(context.TODO(), newExtServerConfigMap())
+
+			authorinoInstance = newFullAuthorinoInstance()
+			Expect(k8sClient.Create(context.TODO(), authorinoInstance)).Should(Succeed())
+		})
+
+		It("Should not have the label removed", func() {
+
+			desiredDeployment := &k8sapps.Deployment{}
+			nsdName := namespacedName(testAuthorinoNamespace, authorinoInstance.Name)
+
+			Eventually(func(ctx context.Context) error {
+				return k8sClient.Get(ctx,
+					nsdName,
+					desiredDeployment)
+			}).WithContext(ctx).Should(Succeed())
+			Expect(desiredDeployment.Spec.Template.Labels).ShouldNot(HaveKeyWithValue("user-added-label", "value"))
+			Expect(desiredDeployment.Labels).ShouldNot(HaveKeyWithValue("user-added-label", "value"))
+			Expect(desiredDeployment.Spec.Selector.MatchLabels).ShouldNot(HaveKeyWithValue("user-added-label", "value"))
+			Expect(desiredDeployment.Labels).ShouldNot(HaveKeyWithValue("control-plane", "controller-manager"))
+			Expect(desiredDeployment.Spec.Selector.MatchLabels).Should(HaveKeyWithValue("control-plane", "controller-manager"))
+			Expect(desiredDeployment.Spec.Template.Labels).Should(HaveKeyWithValue("control-plane", "controller-manager"))
+
+			Expect(desiredDeployment.Labels).ShouldNot(HaveKeyWithValue("authorino-resource", authorinoInstance.Name))
+			Expect(desiredDeployment.Spec.Selector.MatchLabels).Should(HaveKeyWithValue("authorino-resource", authorinoInstance.Name))
+			Expect(desiredDeployment.Spec.Template.Labels).Should(HaveKeyWithValue("authorino-resource", authorinoInstance.Name))
+
+			desiredDeployment.Spec.Template.Labels["user-added-label"] = "value"
+			Expect(k8sClient.Update(context.TODO(), desiredDeployment)).Should(Succeed())
+
+			Eventually(func(ctx context.Context) error {
+				return k8sClient.Get(ctx,
+					nsdName,
+					authorinoInstance)
+			}).WithContext(ctx).Should(Succeed())
+			replicas := int32(testAuthorinoReplicas + 1)
+			authorinoInstance.Spec.Replicas = &replicas
+			Expect(k8sClient.Update(context.TODO(), authorinoInstance)).Should(Succeed())
+
+			updatedDeployment := &k8sapps.Deployment{}
+
+			Eventually(func(ctx context.Context) error {
+				return k8sClient.Get(ctx,
+					nsdName,
+					updatedDeployment)
+			}).WithContext(ctx).Should(Succeed())
+			Expect(updatedDeployment.Spec.Template.Labels).Should(HaveKeyWithValue("user-added-label", "value"))
+			Expect(updatedDeployment.Labels).ShouldNot(HaveKeyWithValue("user-added-label", "value"))
+			Expect(updatedDeployment.Spec.Selector.MatchLabels).ShouldNot(HaveKeyWithValue("user-added-label", "value"))
+
+			Expect(updatedDeployment.Labels).ShouldNot(HaveKeyWithValue("control-plane", "controller-manager"))
+			Expect(updatedDeployment.Spec.Selector.MatchLabels).Should(HaveKeyWithValue("control-plane", "controller-manager"))
+			Expect(updatedDeployment.Spec.Template.Labels).Should(HaveKeyWithValue("control-plane", "controller-manager"))
+
+			Expect(updatedDeployment.Labels).ShouldNot(HaveKeyWithValue("authorino-resource", authorinoInstance.Name))
+			Expect(updatedDeployment.Spec.Selector.MatchLabels).Should(HaveKeyWithValue("authorino-resource", authorinoInstance.Name))
+			Expect(updatedDeployment.Spec.Template.Labels).Should(HaveKeyWithValue("authorino-resource", authorinoInstance.Name))
+
+		})
+	})
+
 	Context("Deploy an old version of Authorino", func() {
 		var authorinoInstance *api.Authorino
 
@@ -258,7 +332,6 @@ func newFullAuthorinoInstance() *api.Authorino {
 		ObjectMeta: v1.ObjectMeta{
 			Name:      name,
 			Namespace: testAuthorinoNamespace,
-			Labels:    map[string]string{"thisLabel": "willPropagate"},
 		},
 		Spec: api.AuthorinoSpec{
 			Image:           image,

controllers/constants.go

@@ -89,6 +89,7 @@ const (
 	statusTlsSecretNotProvided                    = "TlsSecretNotProvided"
 	statusUnableToUpdateDeployment                = "UnableToUpdateDeployment"
 	statusDeploymentNotReady                      = "DeploymentNotReady"
+	statusUnableToBuildDeploymentObject           = "UnableToBuildDeploymentObject"
 )
 
 // ldflags

pkg/resources/k8s_deployment.go

@@ -8,18 +8,17 @@ import (
 
 func GetDeployment(name, namespace, saName string, replicas *int32, containers []k8score.Container, vol []k8score.Volume, labels map[string]string) *k8sapps.Deployment {
 	objMeta := getObjectMeta(namespace, name, labels)
-	authorinoLabels := labelsForAuthorino(name)
 
 	return &k8sapps.Deployment{
 		ObjectMeta: objMeta,
 		Spec: k8sapps.DeploymentSpec{
 			Replicas: replicas,
 			Selector: &v1.LabelSelector{
-				MatchLabels: authorinoLabels,
+				MatchLabels: defaultAuthorinoLabels(name),
 			},
 			Template: k8score.PodTemplateSpec{
 				ObjectMeta: v1.ObjectMeta{
-					Labels: authorinoLabels,
+					Labels: defaultAuthorinoLabels(name),
 				},
 				Spec: k8score.PodSpec{
 					ServiceAccountName: saName,
@@ -75,3 +74,12 @@ func GetTlsVolume(certName, secretName string) k8score.Volume {
 		},
 	}
 }
+
+func MapUpdateNeeded(existing map[string]string, desired map[string]string) bool {
+	for k, v := range desired {
+		if existingVal, exists := (existing)[k]; !exists || v != existingVal {
+			return true
+		}
+	}
+	return false
+}

pkg/resources/k8s_services.go

@@ -57,7 +57,7 @@ func newService(serviceName, serviceNamespace, authorinoName string, labels map[
 	objMeta := getObjectMeta(serviceNamespace, authorinoName+"-"+serviceName, labels)
 	return &k8score.Service{
 		ObjectMeta: objMeta,
-		Spec:       newServiceSpec(labelsForAuthorino(authorinoName), servicePorts...),
+		Spec:       newServiceSpec(defaultAuthorinoLabels(authorinoName), servicePorts...),
 	}
 }
 

pkg/resources/k8s_util.go

@@ -10,7 +10,7 @@ func getObjectMeta(namespace, name string, labels map[string]string) v1.ObjectMe
 	return v1.ObjectMeta{Name: name, Namespace: namespace, Labels: labels}
 }
 
-func labelsForAuthorino(name string) map[string]string {
+func defaultAuthorinoLabels(name string) map[string]string {
 	return map[string]string{
 		"control-plane":      "controller-manager",
 		"authorino-resource": name,