fix: Incorrect comparison logic when comparing lists

controllers/consoleplugin.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"os"
 	"reflect"
+	"sort"
 
 	argocommon "github.com/argoproj-labs/argocd-operator/common"
 	argocdutil "github.com/argoproj-labs/argocd-operator/controllers/argoutil"
@@ -16,6 +17,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 
 	"github.com/redhat-developer/gitops-operator/common"
+	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/errors"
 	resourcev1 "k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -267,6 +269,74 @@ func pluginConfigMap() *corev1.ConfigMap {
 	return cm
 }
 
+// normalizeContainerDefaults sets Kubernetes default values for container fields that are
+// automatically populated by the API server. This ensures consistent comparison between
+// existing containers (from etcd) and new containers (from operator).
+func normalizeContainerDefaults(container *corev1.Container) {
+	if container.TerminationMessagePath == "" {
+		container.TerminationMessagePath = "/dev/termination-log"
+	}
+	if container.TerminationMessagePolicy == "" {
+		container.TerminationMessagePolicy = corev1.TerminationMessageReadFile
+	}
+}
+
+// sortContainers creates a sorted copy of containers by name, and nested fields
+// (Env, Ports, VolumeMounts) to handle non-deterministic ordering from etcd
+func sortContainers(containers []corev1.Container) []corev1.Container {
+	if len(containers) == 0 {
+		return containers
+	}
+	sorted := make([]corev1.Container, len(containers))
+	for i := range containers {
+		sorted[i] = *containers[i].DeepCopy()
+		normalizeContainerDefaults(&sorted[i])
+		sort.Slice(sorted[i].Env, func(a, b int) bool {
+			return sorted[i].Env[a].Name < sorted[i].Env[b].Name
+		})
+		sort.Slice(sorted[i].Ports, func(a, b int) bool {
+			if sorted[i].Ports[a].ContainerPort != sorted[i].Ports[b].ContainerPort {
+				return sorted[i].Ports[a].ContainerPort < sorted[i].Ports[b].ContainerPort
+			}
+			return sorted[i].Ports[a].Name < sorted[i].Ports[b].Name
+		})
+		sort.Slice(sorted[i].VolumeMounts, func(a, b int) bool {
+			return sorted[i].VolumeMounts[a].Name < sorted[i].VolumeMounts[b].Name
+		})
+	}
+	sort.Slice(sorted, func(i, j int) bool {
+		return sorted[i].Name < sorted[j].Name
+	})
+	return sorted
+}
+
+// sortVolumes creates a sorted copy of volumes by name
+func sortVolumes(volumes []corev1.Volume) []corev1.Volume {
+	sorted := make([]corev1.Volume, len(volumes))
+	copy(sorted, volumes)
+	sort.Slice(sorted, func(i, j int) bool {
+		return sorted[i].Name < sorted[j].Name
+	})
+	return sorted
+}
+
+// sortTolerations creates a sorted copy of tolerations by key, operator, and effect
+func sortTolerations(tolerations []corev1.Toleration) []corev1.Toleration {
+	sorted := make([]corev1.Toleration, len(tolerations))
+	copy(sorted, tolerations)
+	sort.Slice(sorted, func(i, j int) bool {
+		a, b := sorted[i], sorted[j]
+		if a.Key != b.Key {
+			return a.Key < b.Key
+		}
+		if a.Operator != b.Operator {
+			return string(a.Operator) < string(b.Operator)
+		}
+		return string(a.Effect) < string(b.Effect)
+	})
+	return sorted
+}
+
 func (r *ReconcileGitopsService) reconcileDeployment(cr *pipelinesv1alpha1.GitopsService, request reconcile.Request) (reconcile.Result, error) {
 	reqLogger := logs.WithValues("Request.Namespace", request.Namespace, "Request.Name", request.Name)
 	newPluginDeployment := pluginDeployment(cr.Spec.ImagePullPolicy)
@@ -309,17 +379,19 @@ func (r *ReconcileGitopsService) reconcileDeployment(cr *pipelinesv1alpha1.Gitop
 	} else {
 		existingSpecTemplate := &existingPluginDeployment.Spec.Template
 		newSpecTemplate := newPluginDeployment.Spec.Template
-		changed := !reflect.DeepEqual(existingPluginDeployment.Labels, newPluginDeployment.Labels) ||
-			!reflect.DeepEqual(existingPluginDeployment.Spec.Replicas, newPluginDeployment.Spec.Replicas) ||
-			!reflect.DeepEqual(existingPluginDeployment.Spec.Selector, newPluginDeployment.Spec.Selector) ||
-			!reflect.DeepEqual(existingSpecTemplate.Labels, newSpecTemplate.Labels) ||
-			!reflect.DeepEqual(existingSpecTemplate.Spec.Containers, newSpecTemplate.Spec.Containers) ||
-			!reflect.DeepEqual(existingSpecTemplate.Spec.Volumes, newSpecTemplate.Spec.Volumes) ||
-			!reflect.DeepEqual(existingSpecTemplate.Spec.RestartPolicy, newSpecTemplate.Spec.RestartPolicy) ||
-			!reflect.DeepEqual(existingSpecTemplate.Spec.DNSPolicy, newSpecTemplate.Spec.DNSPolicy) ||
-			!reflect.DeepEqual(existingPluginDeployment.Spec.Template.Spec.NodeSelector, newPluginDeployment.Spec.Template.Spec.NodeSelector) ||
-			!reflect.DeepEqual(existingPluginDeployment.Spec.Template.Spec.Tolerations, newPluginDeployment.Spec.Template.Spec.Tolerations) ||
-			!reflect.DeepEqual(existingSpecTemplate.Spec.SecurityContext, existingSpecTemplate.Spec.SecurityContext) || !reflect.DeepEqual(existingSpecTemplate.Spec.Containers[0].Resources, newSpecTemplate.Spec.Containers[0].Resources)
+		// Sort list fields before comparing to handle non-deterministic ordering
+		changed := !equality.Semantic.DeepEqual(existingPluginDeployment.Labels, newPluginDeployment.Labels) ||
+			!equality.Semantic.DeepEqual(existingPluginDeployment.Spec.Replicas, newPluginDeployment.Spec.Replicas) ||
+			!equality.Semantic.DeepEqual(existingPluginDeployment.Spec.Selector, newPluginDeployment.Spec.Selector) ||
+			!equality.Semantic.DeepEqual(existingSpecTemplate.Labels, newSpecTemplate.Labels) ||
+			!equality.Semantic.DeepEqual(sortContainers(existingSpecTemplate.Spec.Containers), sortContainers(newSpecTemplate.Spec.Containers)) ||
+			!equality.Semantic.DeepEqual(sortVolumes(existingSpecTemplate.Spec.Volumes), sortVolumes(newSpecTemplate.Spec.Volumes)) ||
+			!equality.Semantic.DeepEqual(existingSpecTemplate.Spec.RestartPolicy, newSpecTemplate.Spec.RestartPolicy) ||
+			!equality.Semantic.DeepEqual(existingSpecTemplate.Spec.DNSPolicy, newSpecTemplate.Spec.DNSPolicy) ||
+			!equality.Semantic.DeepEqual(existingPluginDeployment.Spec.Template.Spec.NodeSelector, newPluginDeployment.Spec.Template.Spec.NodeSelector) ||
+			!equality.Semantic.DeepEqual(sortTolerations(existingPluginDeployment.Spec.Template.Spec.Tolerations), sortTolerations(newPluginDeployment.Spec.Template.Spec.Tolerations)) ||
+			!equality.Semantic.DeepEqual(existingSpecTemplate.Spec.SecurityContext, newSpecTemplate.Spec.SecurityContext) ||
+			!equality.Semantic.DeepEqual(existingSpecTemplate.Spec.Containers[0].Resources, newSpecTemplate.Spec.Containers[0].Resources)
 
 		if changed {
 			reqLogger.Info("Reconciling plugin deployment", "Namespace", existingPluginDeployment.Namespace, "Name", existingPluginDeployment.Name)

controllers/consoleplugin_test.go

@@ -1656,3 +1656,207 @@ func TestPlug_reconcileConfigMap_changedLabels(t *testing.T) {
 		})
 	}
 }
+
+// Tests that reconciliation does NOT trigger an update when containers are returned in different order from etcd.
+func TestReconcileDeployment_NoUpdateWhenContainersOrderDiffers(t *testing.T) {
+	s := scheme.Scheme
+	addKnownTypesToScheme(s)
+
+	fakeClient := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(newGitopsService()).Build()
+	reconciler := newReconcileGitOpsService(fakeClient, s)
+	instance := &pipelinesv1alpha1.GitopsService{}
+
+	// Create deployment
+	_, err := reconciler.reconcileDeployment(instance, newRequest(serviceNamespace, gitopsPluginName))
+	assertNoError(t, err)
+
+	// Get the deployment and capture initial ResourceVersion and Generation
+	deployment := &appsv1.Deployment{}
+	err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: gitopsPluginName, Namespace: serviceNamespace}, deployment)
+	assertNoError(t, err)
+	initialRV := deployment.ResourceVersion
+	initialGen := deployment.Generation
+
+	// Simulate etcd returning containers in different order
+	if len(deployment.Spec.Template.Spec.Containers) >= 2 {
+		containers := deployment.Spec.Template.Spec.Containers
+		reversedContainers := make([]corev1.Container, len(containers))
+		for i := range containers {
+			reversedContainers[len(containers)-1-i] = containers[i]
+		}
+		deployment.Spec.Template.Spec.Containers = reversedContainers
+		err = fakeClient.Update(context.TODO(), deployment)
+		assertNoError(t, err)
+	}
+
+	// Reconcile again - should NOT trigger an update
+	_, err = reconciler.reconcileDeployment(instance, newRequest(serviceNamespace, gitopsPluginName))
+	assertNoError(t, err)
+
+	// Verify no update was triggered
+	err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: gitopsPluginName, Namespace: serviceNamespace}, deployment)
+	assertNoError(t, err)
+
+	assert.Equal(t, deployment.ResourceVersion, initialRV,
+		"ResourceVersion should NOT change when only container order differs")
+	assert.Equal(t, deployment.Generation, initialGen,
+		"Generation should NOT change when only container order differs")
+}
+
+// Tests that reconciliation does NOT trigger an update when volumes are returned in different order from etcd.
+func TestReconcileDeployment_NoUpdateWhenVolumesOrderDiffers(t *testing.T) {
+	s := scheme.Scheme
+	addKnownTypesToScheme(s)
+
+	fakeClient := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(newGitopsService()).Build()
+	reconciler := newReconcileGitOpsService(fakeClient, s)
+	instance := &pipelinesv1alpha1.GitopsService{}
+
+	// Create deployment
+	_, err := reconciler.reconcileDeployment(instance, newRequest(serviceNamespace, gitopsPluginName))
+	assertNoError(t, err)
+
+	// Get the deployment
+	deployment := &appsv1.Deployment{}
+	err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: gitopsPluginName, Namespace: serviceNamespace}, deployment)
+	assertNoError(t, err)
+
+	// Simulate etcd returning volumes in different order
+	if len(deployment.Spec.Template.Spec.Volumes) >= 2 {
+		volumes := deployment.Spec.Template.Spec.Volumes
+		reversedVolumes := make([]corev1.Volume, len(volumes))
+		for i := range volumes {
+			reversedVolumes[len(volumes)-1-i] = volumes[i]
+		}
+		deployment.Spec.Template.Spec.Volumes = reversedVolumes
+		err = fakeClient.Update(context.TODO(), deployment)
+		assertNoError(t, err)
+
+		err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: gitopsPluginName, Namespace: serviceNamespace}, deployment)
+		assertNoError(t, err)
+		rvAfterManualUpdate := deployment.ResourceVersion
+		genAfterManualUpdate := deployment.Generation
+
+		// Reconcile again - should NOT trigger an update
+		_, err = reconciler.reconcileDeployment(instance, newRequest(serviceNamespace, gitopsPluginName))
+		assertNoError(t, err)
+
+		// Verify no update was triggered
+		err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: gitopsPluginName, Namespace: serviceNamespace}, deployment)
+		assertNoError(t, err)
+
+		assert.Equal(t, deployment.ResourceVersion, rvAfterManualUpdate,
+			"ResourceVersion should NOT change when only volume order differs")
+		assert.Equal(t, deployment.Generation, genAfterManualUpdate,
+			"Generation should NOT change when only volume order differs")
+	} else {
+		t.Skip("Skipping test: deployment has less than 2 volumes, cannot test order difference")
+	}
+}
+
+// Tests that reconciliation does NOT trigger an update when tolerations are returned in different order from etcd.
+func TestReconcileDeployment_NoUpdateWhenTolerationsOrderDiffers(t *testing.T) {
+	s := scheme.Scheme
+	addKnownTypesToScheme(s)
+
+	// Create GitopsService with tolerations
+	gitopsService := &pipelinesv1alpha1.GitopsService{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: serviceName,
+		},
+		Spec: pipelinesv1alpha1.GitopsServiceSpec{
+			Tolerations: []corev1.Toleration{
+				{
+					Key:      "key-a",
+					Operator: corev1.TolerationOpEqual,
+					Effect:   corev1.TaintEffectNoSchedule,
+				},
+				{
+					Key:      "key-b",
+					Operator: corev1.TolerationOpEqual,
+					Effect:   corev1.TaintEffectNoSchedule,
+				},
+			},
+		},
+	}
+
+	fakeClient := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(gitopsService).Build()
+	reconciler := newReconcileGitOpsService(fakeClient, s)
+
+	// Create deployment
+	_, err := reconciler.reconcileDeployment(gitopsService, newRequest(serviceNamespace, gitopsPluginName))
+	assertNoError(t, err)
+
+	// Get the deployment
+	deployment := &appsv1.Deployment{}
+	err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: gitopsPluginName, Namespace: serviceNamespace}, deployment)
+	assertNoError(t, err)
+
+	// Simulate etcd returning tolerations in different order
+	if len(deployment.Spec.Template.Spec.Tolerations) >= 2 {
+		tolerations := deployment.Spec.Template.Spec.Tolerations
+		reversedTolerations := make([]corev1.Toleration, len(tolerations))
+		for i := range tolerations {
+			reversedTolerations[len(tolerations)-1-i] = tolerations[i]
+		}
+		deployment.Spec.Template.Spec.Tolerations = reversedTolerations
+		err = fakeClient.Update(context.TODO(), deployment)
+		assertNoError(t, err)
+
+		err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: gitopsPluginName, Namespace: serviceNamespace}, deployment)
+		assertNoError(t, err)
+		rvAfterManualUpdate := deployment.ResourceVersion
+		genAfterManualUpdate := deployment.Generation
+
+		// Reconcile again - should NOT trigger an update
+		_, err = reconciler.reconcileDeployment(gitopsService, newRequest(serviceNamespace, gitopsPluginName))
+		assertNoError(t, err)
+
+		// Verify no update was triggered
+		err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: gitopsPluginName, Namespace: serviceNamespace}, deployment)
+		assertNoError(t, err)
+
+		assert.Equal(t, deployment.ResourceVersion, rvAfterManualUpdate,
+			"ResourceVersion should NOT change when only toleration order differs")
+		assert.Equal(t, deployment.Generation, genAfterManualUpdate,
+			"Generation should NOT change when only toleration order differs")
+	} else {
+		t.Skip("Skipping test: deployment has less than 2 tolerations, cannot test order difference")
+	}
+}
+
+// Tests that legitimate changes do trigger updates.
+func TestReconcileDeployment_UpdateWhenActualChange(t *testing.T) {
+	s := scheme.Scheme
+	addKnownTypesToScheme(s)
+
+	fakeClient := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(newGitopsService()).Build()
+	reconciler := newReconcileGitOpsService(fakeClient, s)
+	instance := &pipelinesv1alpha1.GitopsService{}
+
+	// Create deployment
+	_, err := reconciler.reconcileDeployment(instance, newRequest(serviceNamespace, gitopsPluginName))
+	assertNoError(t, err)
+
+	// Get the deployment and capture initial ResourceVersion and Generation
+	deployment := &appsv1.Deployment{}
+	err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: gitopsPluginName, Namespace: serviceNamespace}, deployment)
+	assertNoError(t, err)
+	initialRV := deployment.ResourceVersion
+
+	// Make an actual change
+	deployment.Spec.Template.Spec.Containers[0].Image = "different-image:tag"
+	err = fakeClient.Update(context.TODO(), deployment)
+	assertNoError(t, err)
+
+	// Reconcile again - should trigger an update
+	_, err = reconciler.reconcileDeployment(instance, newRequest(serviceNamespace, gitopsPluginName))
+	assertNoError(t, err)
+
+	// Verify update was triggered
+	err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: gitopsPluginName, Namespace: serviceNamespace}, deployment)
+	assertNoError(t, err)
+
+	assert.Assert(t, deployment.ResourceVersion != initialRV,
+		"ResourceVersion SHOULD change when actual change is made")
+}