Merge pull request #243 from Kuadrant/label-propagation-fix

fix(labels): do not remove user added labels from Deployment's spec.template

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,27 @@ 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.Spec.Replicas).To(Equal(&replicas))
+
 			Expect(deployment.Labels).Should(Equal(map[string]string{"thisLabel": "willPropagate"}))
+			Expect(deployment.Spec.Selector.MatchLabels).ShouldNot(HaveKeyWithValue("thisLabel", "willPropagate"))
+			Expect(deployment.Spec.Template.Labels).ShouldNot(HaveKeyWithValue("thisLabel", "willPropagate"))
+
+			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 +183,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 +201,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
 

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,