OCPBUGS-23221: internalServiceChanged: Fix target port logic

pkg/manifests/assets/router/deployment.yaml

@@ -31,20 +31,33 @@ spec:
           - name: DEFAULT_DESTINATION_CA_PATH
             value: /var/run/configmaps/service-ca/service-ca.crt
           livenessProbe:
+            failureThreshold: 3
             httpGet:
               path: /healthz
               port: 1936
+              scheme: HTTP
+            periodSeconds: 10
+            successThreshold: 1
+            timeoutSeconds: 1
             terminationGracePeriodSeconds: 10
           readinessProbe:
+            failureThreshold: 3
             httpGet:
               path: /healthz/ready
               port: 1936
+              scheme: HTTP
+            periodSeconds: 10
+            successThreshold: 1
+            timeoutSeconds: 1
           startupProbe:
             failureThreshold: 120
             httpGet:
               path: /healthz/ready
               port: 1936
+              scheme: HTTP
             periodSeconds: 1
+            successThreshold: 1
+            timeoutSeconds: 1
           resources:
             requests:
               cpu: 100m
@@ -63,9 +76,9 @@ spec:
           # SecretName is set at run-time.
       - name: service-ca-bundle
         configMap:
+          defaultMode: 420
           items:
           - key: service-ca.crt
             path: service-ca.crt
           name: service-ca-bundle
           optional: false
-        defaultMode: 420

pkg/manifests/assets/router/service-cloud.yaml

@@ -24,3 +24,4 @@ spec:
     protocol: TCP
     port: 443
     targetPort: https
+  sessionAffinity: None

pkg/manifests/assets/router/service-internal.yaml

@@ -19,3 +19,4 @@ spec:
     port: 1936
     protocol: TCP
     targetPort: metrics
+  sessionAffinity: None

pkg/operator/controller/canary/daemonset_test.go

@@ -200,6 +200,9 @@ func Test_canaryDaemonsetChanged(t *testing.T) {
 			if changed, updated := canaryDaemonSetChanged(original, mutated); changed != tc.expect {
 				t.Errorf("expect canaryDaemonSetChanged to be %t, got %t", tc.expect, changed)
 			} else if changed {
+				if updatedChanged, _ := canaryDaemonSetChanged(original, updated); !updatedChanged {
+					t.Error("canaryDaemonSetChanged reported changes but did not make any update")
+				}
 				if changedAgain, _ := canaryDaemonSetChanged(mutated, updated); changedAgain {
 					t.Error("canaryDaemonSetChanged does not behave as a fixed point function")
 				}

pkg/operator/controller/canary/namespace_test.go

@@ -38,6 +38,9 @@ func Test_canaryNamespaceChanged(t *testing.T) {
 			if changed, updated := canaryNamespaceChanged(original, mutated); changed != tc.expect {
 				t.Errorf("expect canaryNamespaceChanged to be %t, got %t", tc.expect, changed)
 			} else if changed {
+				if updatedChanged, _ := canaryNamespaceChanged(original, updated); !updatedChanged {
+					t.Error("canaryNamespaceChanged reported changes but did not make any update")
+				}
 				if changedAgain, _ := canaryNamespaceChanged(mutated, updated); changedAgain {
 					t.Error("canaryNamespaceChanged does not behave as a fixed point function")
 				}

pkg/operator/controller/canary/route_test.go

@@ -134,6 +134,9 @@ func Test_canaryRouteChanged(t *testing.T) {
 			if changed, updated := canaryRouteChanged(original, mutated); changed != tc.expect {
 				t.Errorf("expected canaryRouteChanged to be %t, got %t", tc.expect, changed)
 			} else if changed {
+				if updatedChanged, _ := canaryRouteChanged(original, updated); !updatedChanged {
+					t.Error("canaryRouteChanged reported changes but did not make any update")
+				}
 				if changedAgain, _ := canaryRouteChanged(mutated, updated); changedAgain {
 					t.Error("canaryRouteChanged does not behave as a fixed point function")
 				}

pkg/operator/controller/gatewayapi/crds.go

@@ -114,7 +114,9 @@ func (r *reconciler) updateCRD(ctx context.Context, current, desired *apiextensi
 // the expected spec and if not returns an updated one.
 func crdChanged(current, expected *apiextensionsv1.CustomResourceDefinition) (bool, *apiextensionsv1.CustomResourceDefinition) {
 	crdCmpOpts := []cmp.Option{
-		// Ignore fields that the API may have modified.
+		// Ignore fields that the API may have modified.  Note: This
+		// list must be kept consistent with the updated.Spec.Foo =
+		// current.Spec.Foo assignments below!
 		cmpopts.IgnoreFields(apiextensionsv1.CustomResourceDefinitionSpec{}, "Conversion"),
 		cmpopts.EquateEmpty(),
 	}
@@ -124,6 +126,10 @@ func crdChanged(current, expected *apiextensionsv1.CustomResourceDefinition) (bo
 
 	updated := current.DeepCopy()
 	updated.Spec = expected.Spec
+	// Preserve fields that the API, other controllers, or user may have
+	// modified.  Note: This list must be kept consistent with crdCmpOpts
+	// above!
+	updated.Spec.Conversion = current.Spec.Conversion
 
 	return true, updated
 }

pkg/operator/controller/ingress/deployment.go

@@ -32,6 +32,7 @@ import (
 
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/rand"
+	"k8s.io/utils/ptr"
 
 	configv1 "github.com/openshift/api/config/v1"
 )
@@ -432,7 +433,8 @@ func desiredRouterDeployment(ci *operatorv1.IngressController, ingressController
 		Name: statsVolumeName,
 		VolumeSource: corev1.VolumeSource{
 			Secret: &corev1.SecretVolumeSource{
-				SecretName: statsSecretName,
+				DefaultMode: ptr.To[int32](0644),
+				SecretName:  statsSecretName,
 			},
 		},
 	}
@@ -459,7 +461,8 @@ func desiredRouterDeployment(ci *operatorv1.IngressController, ingressController
 		Name: certsVolumeName,
 		VolumeSource: corev1.VolumeSource{
 			Secret: &corev1.SecretVolumeSource{
-				SecretName: certsSecretName,
+				DefaultMode: ptr.To[int32](0644),
+				SecretName:  certsSecretName,
 			},
 		},
 	}
@@ -478,6 +481,7 @@ func desiredRouterDeployment(ci *operatorv1.IngressController, ingressController
 			Name: "error-pages",
 			VolumeSource: corev1.VolumeSource{
 				ConfigMap: &corev1.ConfigMapVolumeSource{
+					DefaultMode: ptr.To[int32](0644),
 					LocalObjectReference: corev1.LocalObjectReference{
 						Name: configmapName.Name,
 					},
@@ -770,6 +774,7 @@ func desiredRouterDeployment(ci *operatorv1.IngressController, ingressController
 				Name: "rsyslog-config",
 				VolumeSource: corev1.VolumeSource{
 					ConfigMap: &corev1.ConfigMapVolumeSource{
+						DefaultMode: ptr.To[int32](0644),
 						LocalObjectReference: corev1.LocalObjectReference{
 							Name: controller.RsyslogConfigMapName(ci).Name,
 						},
@@ -1064,6 +1069,7 @@ func desiredRouterDeployment(ci *operatorv1.IngressController, ingressController
 				Name: clientCAVolumeName,
 				VolumeSource: corev1.VolumeSource{
 					ConfigMap: &corev1.ConfigMapVolumeSource{
+						DefaultMode: ptr.To[int32](0644),
 						LocalObjectReference: corev1.LocalObjectReference{
 							Name: clientCAConfigmapName.Name,
 						},
@@ -1514,7 +1520,7 @@ func hashableProbe(probe *corev1.Probe) *corev1.Probe {
 
 	var hashableProbe corev1.Probe
 
-	copyProbe(probe, &hashableProbe)
+	copyProbe(probe, &hashableProbe, false)
 
 	return &hashableProbe
 }
@@ -1615,9 +1621,9 @@ func deploymentConfigChanged(current, expected *appsv1.Deployment) (bool, *appsv
 	updated.Spec.Template.Spec.Containers[0].SecurityContext = expected.Spec.Template.Spec.Containers[0].SecurityContext
 	updated.Spec.Template.Spec.Containers[0].Env = expected.Spec.Template.Spec.Containers[0].Env
 	updated.Spec.Template.Spec.Containers[0].Image = expected.Spec.Template.Spec.Containers[0].Image
-	copyProbe(expected.Spec.Template.Spec.Containers[0].LivenessProbe, updated.Spec.Template.Spec.Containers[0].LivenessProbe)
-	copyProbe(expected.Spec.Template.Spec.Containers[0].ReadinessProbe, updated.Spec.Template.Spec.Containers[0].ReadinessProbe)
-	copyProbe(expected.Spec.Template.Spec.Containers[0].StartupProbe, updated.Spec.Template.Spec.Containers[0].StartupProbe)
+	copyProbe(expected.Spec.Template.Spec.Containers[0].LivenessProbe, updated.Spec.Template.Spec.Containers[0].LivenessProbe, true)
+	copyProbe(expected.Spec.Template.Spec.Containers[0].ReadinessProbe, updated.Spec.Template.Spec.Containers[0].ReadinessProbe, true)
+	copyProbe(expected.Spec.Template.Spec.Containers[0].StartupProbe, updated.Spec.Template.Spec.Containers[0].StartupProbe, true)
 	updated.Spec.Template.Spec.Containers[0].VolumeMounts = expected.Spec.Template.Spec.Containers[0].VolumeMounts
 	updated.Spec.Template.Spec.Containers[0].Ports = expected.Spec.Template.Spec.Containers[0].Ports
 	updated.Spec.Template.Spec.Tolerations = expected.Spec.Template.Spec.Tolerations
@@ -1633,12 +1639,16 @@ func deploymentConfigChanged(current, expected *appsv1.Deployment) (bool, *appsv
 }
 
 // copyProbe copies probe parameters that the operator manages from probe a to
-// probe b.
-func copyProbe(a, b *corev1.Probe) {
+// probe b.  If a field in probe a has the default value, then the value is
+// copied to probe b only if copyDefaultValues is true.
+func copyProbe(a, b *corev1.Probe, copyDefaultValues bool) {
 	if a == nil || b == nil {
 		return
 	}
 
+	// Always copy values that differ from the default values, as well as
+	// values where the default value is the same as the zero value.
+
 	if a.ProbeHandler.HTTPGet != nil {
 		b.ProbeHandler.HTTPGet = &corev1.HTTPGetAction{
 			Path: a.ProbeHandler.HTTPGet.Path,
@@ -1655,7 +1665,6 @@ func copyProbe(a, b *corev1.Probe) {
 
 	// Users are permitted to modify the timeout, so *don't* copy it.
 
-	// Don't copy default values that the API set.
 	if a.PeriodSeconds != int32(10) {
 		b.PeriodSeconds = a.PeriodSeconds
 	}
@@ -1665,6 +1674,17 @@ func copyProbe(a, b *corev1.Probe) {
 	if a.FailureThreshold != int32(3) {
 		b.FailureThreshold = a.FailureThreshold
 	}
+
+	// If copyDefaultValues is true, copy values even if they are the
+	// default values that the API sets.
+	if copyDefaultValues {
+		if a.ProbeHandler.HTTPGet != nil {
+			b.ProbeHandler.HTTPGet.Scheme = a.ProbeHandler.HTTPGet.Scheme
+		}
+		b.PeriodSeconds = a.PeriodSeconds
+		b.SuccessThreshold = a.SuccessThreshold
+		b.FailureThreshold = a.FailureThreshold
+	}
 }
 
 // clipHAProxyTimeoutValue prevents the HAProxy config file from using