Bug 1936030: Fix spurious reconciliation of NodePort services

pkg/operator/controller/canary/route.go

@@ -75,10 +75,12 @@ func (r *reconciler) updateCanaryRoute(current, desired *routev1.Route) (bool, e
 		return false, nil
 	}
 
+	// Diff before updating because the client may mutate the object.
+	diff := cmp.Diff(current, updated, cmpopts.EquateEmpty())
 	if err := r.client.Update(context.TODO(), updated); err != nil {
 		return false, fmt.Errorf("failed to update canary route %s/%s: %v", updated.Namespace, updated.Name, err)
 	}
-	log.Info("updated canary route", "namespace", updated.Namespace, "name", updated.Name)
+	log.Info("updated canary route", "namespace", updated.Namespace, "name", updated.Name, "diff", diff)
 	return true, nil
 }
 

pkg/operator/controller/ingress/cluster-role.go

@@ -5,6 +5,9 @@ import (
 	"fmt"
 	"reflect"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
@@ -45,7 +48,6 @@ func (r *reconciler) ensureClusterRole() (bool, *rbacv1.ClusterRole, error) {
 		if updated, err := r.updateClusterRole(current, desired); err != nil {
 			return true, current, fmt.Errorf("failed to update cluster role: %v", err)
 		} else if updated {
-			log.Info("updated cluster role", "desired", desired)
 			return r.currentClusterRole()
 		}
 	}
@@ -77,11 +79,14 @@ func (r *reconciler) updateClusterRole(current, desired *rbacv1.ClusterRole) (bo
 	}
 	updated := current.DeepCopy()
 	updated.Rules = desired.Rules
+	// Diff before updating because the client may mutate the object.
+	diff := cmp.Diff(current, updated, cmpopts.EquateEmpty())
 	if err := r.client.Update(context.TODO(), updated); err != nil {
 		if errors.IsAlreadyExists(err) {
 			return false, nil
 		}
 		return false, err
 	}
+	log.Info("updated cluster role", "namespace", updated.Namespace, "name", updated.Name, "diff", diff)
 	return true, nil
 }

pkg/operator/controller/ingress/dns.go

@@ -51,7 +51,6 @@ func (r *reconciler) ensureWildcardDNSRecord(ic *operatorv1.IngressController, s
 		if updated, err := r.updateDNSRecord(current, desired); err != nil {
 			return true, current, fmt.Errorf("failed to update dnsrecord %s/%s: %v", desired.Namespace, desired.Name, err)
 		} else if updated {
-			log.Info("updated dnsrecord", "dnsrecord", desired)
 			return r.currentWildcardDNSRecord(ic)
 		}
 	}
@@ -170,9 +169,12 @@ func (r *reconciler) updateDNSRecord(current, desired *iov1.DNSRecord) (bool, er
 		return false, nil
 	}
 
+	// Diff before updating because the client may mutate the object.
+	diff := cmp.Diff(current, updated, cmpopts.EquateEmpty())
 	if err := r.client.Update(context.TODO(), updated); err != nil {
 		return false, err
 	}
+	log.Info("updated dnsrecord", "namespace", updated.Namespace, "name", updated.Name, "diff", diff)
 	return true, nil
 }
 

pkg/operator/controller/ingress/load_balancer_service.go

@@ -4,6 +4,9 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+
 	operatorv1 "github.com/openshift/api/operator/v1"
 	"github.com/openshift/cluster-ingress-operator/pkg/manifests"
 	"github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
@@ -160,7 +163,6 @@ func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController,
 		if updated, err := r.updateLoadBalancerService(currentLBService, desiredLBService, platform); err != nil {
 			return true, currentLBService, fmt.Errorf("failed to update load balancer service: %v", err)
 		} else if updated {
-			log.Info("updated load balancer service", "namespace", desiredLBService.Namespace, "name", desiredLBService.Name)
 			return r.currentLoadBalancerService(ci)
 		}
 	}
@@ -299,9 +301,12 @@ func (r *reconciler) updateLoadBalancerService(current, desired *corev1.Service,
 	if !changed {
 		return false, nil
 	}
+	// Diff before updating because the client may mutate the object.
+	diff := cmp.Diff(current, updated, cmpopts.EquateEmpty())
 	if err := r.client.Update(context.TODO(), updated); err != nil {
 		return false, err
 	}
+	log.Info("updated load balancer service", "namespace", updated.Namespace, "name", updated.Name, "diff", diff)
 	return true, nil
 }
 

pkg/operator/controller/ingress/monitoring.go

@@ -5,6 +5,9 @@ import (
 	"fmt"
 	"reflect"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+
 	operatorcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
 
 	operatorv1 "github.com/openshift/api/operator/v1"
@@ -41,7 +44,6 @@ func (r *reconciler) ensureServiceMonitor(ic *operatorv1.IngressController, svc
 		if updated, err := r.updateServiceMonitor(current, desired); err != nil {
 			return true, current, fmt.Errorf("failed to update servicemonitor %s/%s: %v", desired.GetNamespace(), desired.GetName(), err)
 		} else if updated {
-			log.Info("updated servicemonitor", "namespace", desired.GetNamespace(), "name", desired.GetName())
 			return r.currentServiceMonitor(ic)
 		}
 	}
@@ -131,9 +133,12 @@ func (r *reconciler) updateServiceMonitor(current, desired *unstructured.Unstruc
 		return false, nil
 	}
 
+	// Diff before updating because the client may mutate the object.
+	diff := cmp.Diff(current, updated, cmpopts.EquateEmpty())
 	if err := r.client.Update(context.TODO(), updated); err != nil {
 		return false, err
 	}
+	log.Info("updated servicemonitor", "namespace", updated.GetNamespace(), "name", updated.GetName(), "diff", diff)
 	return true, nil
 }
 

pkg/operator/controller/ingress/nodeport_service.go

@@ -53,7 +53,6 @@ func (r *reconciler) ensureNodePortService(ic *operatorv1.IngressController, dep
 		if updated, err := r.updateNodePortService(current, desired); err != nil {
 			return true, current, fmt.Errorf("failed to update NodePort service: %v", err)
 		} else if updated {
-			log.Info("updated NodePort service", "service", desired)
 			return r.currentNodePortService(ic)
 		}
 	}
@@ -126,9 +125,12 @@ func (r *reconciler) updateNodePortService(current, desired *corev1.Service) (bo
 		return false, nil
 	}
 
+	// Diff before updating because the client may mutate the object.
+	diff := cmp.Diff(current, updated, cmpopts.EquateEmpty())
 	if err := r.client.Update(context.TODO(), updated); err != nil {
 		return false, err
 	}
+	log.Info("updated NodePort service", "namespace", updated.Namespace, "name", updated.Name, "diff", diff)
 	return true, nil
 }
 
@@ -139,7 +141,7 @@ func nodePortServiceChanged(current, expected *corev1.Service) (bool, *corev1.Se
 		// Ignore fields that the API, other controllers, or user may
 		// have modified.
 		cmpopts.IgnoreFields(corev1.ServicePort{}, "NodePort"),
-		cmpopts.IgnoreFields(corev1.ServiceSpec{}, "ClusterIP", "ExternalIPs", "HealthCheckNodePort"),
+		cmpopts.IgnoreFields(corev1.ServiceSpec{}, "ClusterIP", "ClusterIPs", "ExternalIPs", "HealthCheckNodePort"),
 		cmp.Comparer(cmpServiceAffinity),
 		cmpopts.EquateEmpty(),
 	}

pkg/operator/controller/ingress/nodeport_service_test.go

@@ -75,6 +75,7 @@ func TestNodePortServiceChanged(t *testing.T) {
 			description: "if .spec.clusterIP changes",
 			mutate: func(svc *corev1.Service) {
 				svc.Spec.ClusterIP = "2.3.4.5"
+				svc.Spec.ClusterIPs = []string{"2.3.4.5"}
 			},
 			expect: false,
 		},

pkg/operator/controller/ingress/poddisruptionbudget.go

@@ -53,7 +53,6 @@ func (r *reconciler) ensureRouterPodDisruptionBudget(ic *operatorv1.IngressContr
 		if updated, err := r.updateRouterPodDisruptionBudget(current, desired); err != nil {
 			return true, current, fmt.Errorf("failed to update pod disruption budget: %v", err)
 		} else if updated {
-			log.Info("updated pod disruption budget", "poddisruptionbudget", desired)
 			return r.currentRouterPodDisruptionBudget(ic)
 		}
 	}
@@ -115,9 +114,12 @@ func (r *reconciler) updateRouterPodDisruptionBudget(current, desired *policyv1b
 		return false, nil
 	}
 
+	// Diff before updating because the client may mutate the object.
+	diff := cmp.Diff(current, updated, cmpopts.EquateEmpty())
 	if err := r.client.Update(context.TODO(), updated); err != nil {
 		return false, err
 	}
+	log.Info("updated pod disruption budget", "namespace", updated.Namespace, "name", updated.Name, "diff", diff)
 	return true, nil
 }
 

pkg/operator/controller/ingress/rsyslog_configmap.go

@@ -5,6 +5,9 @@ import (
 	"fmt"
 	"reflect"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+
 	configv1 "github.com/openshift/api/config/v1"
 	operatorv1 "github.com/openshift/api/operator/v1"
 	"github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
@@ -60,7 +63,6 @@ func (r *reconciler) ensureRsyslogConfigMap(ic *operatorv1.IngressController, de
 		if updated, err := r.updateRsyslogConfigMap(current, desired); err != nil {
 			return true, current, fmt.Errorf("failed to update configmap: %v", err)
 		} else if updated {
-			log.Info("updated configmap", "configmap", desired)
 			return r.currentRsyslogConfigMap(ic)
 		}
 	}
@@ -114,12 +116,15 @@ func (r *reconciler) updateRsyslogConfigMap(current, desired *corev1.ConfigMap)
 	}
 	updated := current.DeepCopy()
 	updated.Data = desired.Data
+	// Diff before updating because the client may mutate the object.
+	diff := cmp.Diff(current, updated, cmpopts.EquateEmpty())
 	if err := r.client.Update(context.TODO(), updated); err != nil {
 		if errors.IsAlreadyExists(err) {
 			return false, nil
 		}
 		return false, err
 	}
+	log.Info("updated configmap", "namespace", updated.Namespace, "name", updated.Name, "diff", diff)
 	return true, nil
 }
 

pkg/operator/controller/ingress/serviceca_configmap.go

@@ -4,6 +4,9 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+
 	"github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
 
 	corev1 "k8s.io/api/core/v1"
@@ -47,7 +50,6 @@ func (r *reconciler) ensureServiceCAConfigMap() (bool, *corev1.ConfigMap, error)
 		if updated, err := r.updateServiceCAConfigMap(current, desired); err != nil {
 			return true, current, fmt.Errorf("failed to update configmap: %v", err)
 		} else if updated {
-			log.Info("updated configmap", "configmap", desired)
 			return r.currentServiceCAConfigMap()
 		}
 	}
@@ -101,11 +103,14 @@ func (r *reconciler) updateServiceCAConfigMap(current, desired *corev1.ConfigMap
 
 	updated := current.DeepCopy()
 	updated.Annotations["service.beta.openshift.io/inject-cabundle"] = "true"
+	// Diff before updating because the client may mutate the object.
+	diff := cmp.Diff(current, updated, cmpopts.EquateEmpty())
 	if err := r.client.Update(context.TODO(), updated); err != nil {
 		if errors.IsAlreadyExists(err) {
 			return false, nil
 		}
 		return false, err
 	}
+	log.Info("updated configmap", "namespace", updated.Namespace, "name", updated.Name, "diff", diff)
 	return true, nil
 }