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]>

bundle/manifests/authorino.kuadrant.io_authconfigs.yaml

@@ -2606,8 +2606,13 @@ spec:
             type: object
           spec:
             description: Specifies the desired state of the AuthConfig resource, i.e.
+<<<<<<< HEAD
               the authentication/authorization scheme to be applied to protect the
               matching service hosts.
+=======
+              the authentication/authorization scheme to be applied to protect the matching
+              service hosts.
+>>>>>>> f003c58 (fix(labels): propagates labels to Deployment's spec.template)
             properties:
               authentication:
                 additionalProperties:

charts/authorino-operator/templates/manifests.yaml

@@ -71,8 +71,13 @@ spec:
             type: object
           spec:
             description: Specifies the desired state of the AuthConfig resource, i.e.
+<<<<<<< HEAD
               the authentication/authorization scheme to be applied to protect the
               matching service hosts.
+=======
+              the authentication/authorization scheme to be applied to protect the matching
+              service hosts.
+>>>>>>> f003c58 (fix(labels): propagates labels to Deployment's spec.template)
             properties:
               authentication:
                 additionalProperties:

config/deploy/manifests.yaml

@@ -78,8 +78,13 @@ spec:
             type: object
           spec:
             description: Specifies the desired state of the AuthConfig resource, i.e.
+<<<<<<< HEAD
               the authentication/authorization scheme to be applied to protect the
               matching service hosts.
+=======
+              the authentication/authorization scheme to be applied to protect the matching
+              service hosts.
+>>>>>>> f003c58 (fix(labels): propagates labels to Deployment's spec.template)
             properties:
               authentication:
                 additionalProperties:

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,11 +132,29 @@ func (r *AuthorinoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		// Deployment created successfully - return and requeue
 		return ctrl.Result{Requeue: true}, nil
 	} else {
+
+		if !deploymentAvailable(existingDeployment) {
+			// Deployment not ready – return and requeue
+			err = updateStatusConditions(logger, authorinoInstance, r.Client, statusNotReady(statusDeploymentNotReady, "Authorino Deployment resource not ready"))
+			return ctrl.Result{RequeueAfter: time.Second}, err
+		}
+
 		// 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)
+		// 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 {
+			if desiredValue, exists := (desiredDeployment.Spec.Template.Labels)[existingKey]; !exists || desiredValue != existingValue {
+				desiredDeployment.Spec.Template.Labels[existingKey] = existingValue
+			}
+		}
+		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)
 		if changed := r.authorinoDeploymentChanges(existingDeployment, desiredDeployment); changed {
 			if err := r.Update(ctx, desiredDeployment); err != nil {
 				return ctrl.Result{}, r.wrapErrorWithStatusUpdate(
@@ -142,12 +166,6 @@ func (r *AuthorinoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 			err = updateStatusConditions(logger, authorinoInstance, r.Client, statusNotReady(statusUpdated, "Authorino Deployment resource updated"))
 			return ctrl.Result{RequeueAfter: time.Second}, err
 		}
-
-		if !deploymentAvailable(existingDeployment) {
-			// Deployment not ready – return and requeue
-			err = updateStatusConditions(logger, authorinoInstance, r.Client, statusNotReady(statusDeploymentNotReady, "Authorino Deployment resource not ready"))
-			return ctrl.Result{RequeueAfter: time.Second}, err
-		}
 	}
 
 	// Updates the status conditions to provisioned
@@ -188,7 +206,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 +277,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 +288,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)
@@ -305,11 +323,10 @@ func (r *AuthorinoReconciler) buildAuthorinoDeployment(authorino *api.Authorino)
 		replicas,
 		containers,
 		volumes,
-		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 +442,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 +571,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

@@ -133,8 +133,15 @@ var _ = Describe("Authorino controller", func() {
 			image := DefaultAuthorinoImage
 			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).Should(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).Should(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 {
 					Expect(container.Image).Should(Equal(image))
@@ -170,16 +177,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 +195,64 @@ 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())
+
+			nsdName := namespacedName(authorinoInstance.GetNamespace(), authorinoInstance.GetName())
+
+			Eventually(func() bool {
+				var authorino api.Authorino
+				err := k8sClient.Get(context.TODO(), nsdName, &authorino)
+				return err == nil && authorinoInstance.Status.Ready()
+			}, testTimeout, testInterval).Should(BeFalse())
+		})
+
+		It("Should not have the label removed", func() {
+			desiredDeploymentList := &k8sapps.DeploymentList{}
+
+			Eventually(func() error {
+				return k8sClient.List(context.TODO(),
+					desiredDeploymentList,
+					&client.ListOptions{
+						Namespace: authorinoInstance.Namespace,
+					})
+			}, testTimeout, testInterval).Should(Succeed())
+
+			for _, deployment := range desiredDeploymentList.Items {
+				if "authorino" == deployment.Name {
+					deployment.Spec.Template.Labels["user-added-label"] = "value"
+					Expect(k8sClient.Update(context.TODO(), &deployment)).Should(Succeed())
+				}
+			}
+
+			replicas := int32(testAuthorinoReplicas + 1)
+			authorinoInstance.Spec.Replicas = &replicas
+			Expect(k8sClient.Update(context.TODO(), authorinoInstance)).Should(Succeed())
+
+			updatedDeployment := &k8sapps.Deployment{}
+
+			Eventually(func() error {
+				return k8sClient.List(context.TODO(),
+					desiredDeploymentList,
+					&client.ListOptions{
+						Namespace: authorinoInstance.Namespace,
+					})
+			}, testTimeout, testInterval).Should(Succeed())
+
+			for _, deployment := range desiredDeploymentList.Items {
+				if "authorino" == deployment.Name {
+					Expect(updatedDeployment.Spec.Template.Labels).Should(HaveKeyWithValue("user-added-label", "value"))
+				}
+			}
+		})
+	})
+
 	Context("Deploy an old version of Authorino", func() {
 		var authorinoInstance *api.Authorino
 

controllers/constants.go

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

pkg/resources/k8s_deployment.go

@@ -6,9 +6,9 @@ import (
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-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)
+func GetDeployment(name, namespace, saName string, replicas *int32, containers []k8score.Container, vol []k8score.Volume) *k8sapps.Deployment {
+	authorinoLabels := defaultAuthorinoLabels(name)
+	objMeta := getObjectMeta(namespace, name, authorinoLabels)
 
 	return &k8sapps.Deployment{
 		ObjectMeta: objMeta,
@@ -75,3 +75,16 @@ func GetTlsVolume(certName, secretName string) k8score.Volume {
 		},
 	}
 }
+
+func MapUpdateNeeded(existing map[string]string, desired map[string]string) bool {
+	if existing == nil {
+		existing = map[string]string{}
+	}
+
+	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,