diff --git a/pkg/manifests/assets/router/deployment.yaml b/pkg/manifests/assets/router/deployment.yaml index 0042633286..25cfae777c 100644 --- a/pkg/manifests/assets/router/deployment.yaml +++ b/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 diff --git a/pkg/manifests/assets/router/service-cloud.yaml b/pkg/manifests/assets/router/service-cloud.yaml index fa618a6170..9f5a072b7f 100644 --- a/pkg/manifests/assets/router/service-cloud.yaml +++ b/pkg/manifests/assets/router/service-cloud.yaml @@ -24,3 +24,4 @@ spec: protocol: TCP port: 443 targetPort: https + sessionAffinity: None diff --git a/pkg/manifests/assets/router/service-internal.yaml b/pkg/manifests/assets/router/service-internal.yaml index 411de483a5..126f6ba273 100644 --- a/pkg/manifests/assets/router/service-internal.yaml +++ b/pkg/manifests/assets/router/service-internal.yaml @@ -19,3 +19,4 @@ spec: port: 1936 protocol: TCP targetPort: metrics + sessionAffinity: None diff --git a/pkg/operator/controller/canary/daemonset_test.go b/pkg/operator/controller/canary/daemonset_test.go index 5bc180483d..957edbd2c4 100644 --- a/pkg/operator/controller/canary/daemonset_test.go +++ b/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") } diff --git a/pkg/operator/controller/canary/namespace_test.go b/pkg/operator/controller/canary/namespace_test.go index 586f9e2b5b..d6da51bb16 100644 --- a/pkg/operator/controller/canary/namespace_test.go +++ b/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") } diff --git a/pkg/operator/controller/canary/route_test.go b/pkg/operator/controller/canary/route_test.go index 5a62010ada..bc99d6c760 100644 --- a/pkg/operator/controller/canary/route_test.go +++ b/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") } diff --git a/pkg/operator/controller/gatewayapi/crds.go b/pkg/operator/controller/gatewayapi/crds.go index f88370326f..b7ac6b62ce 100644 --- a/pkg/operator/controller/gatewayapi/crds.go +++ b/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 } diff --git a/pkg/operator/controller/ingress/deployment.go b/pkg/operator/controller/ingress/deployment.go index 62b4aea791..113c56bb4c 100644 --- a/pkg/operator/controller/ingress/deployment.go +++ b/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 diff --git a/pkg/operator/controller/ingress/deployment_test.go b/pkg/operator/controller/ingress/deployment_test.go index 418515b04b..b1639b150c 100644 --- a/pkg/operator/controller/ingress/deployment_test.go +++ b/pkg/operator/controller/ingress/deployment_test.go @@ -9,6 +9,8 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" + configv1 "github.com/openshift/api/config/v1" operatorv1 "github.com/openshift/api/operator/v1" @@ -16,7 +18,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" - "k8s.io/utils/pointer" + "k8s.io/utils/ptr" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -468,6 +470,70 @@ func Test_desiredRouterDeployment(t *testing.T) { checkDeploymentHasEnvSorted(t, deployment) } +// assertHasVolumes asserts that the given slice of volumes has all of the +// named volumes. +func assertHasVolumes(t *testing.T, volumes []corev1.Volume, expectedVolumeNames []string) { + t.Helper() + + actualVolumeNames := []string{} + for _, volume := range volumes { + actualVolumeNames = append(actualVolumeNames, volume.Name) + } + assert.Equal(t, len(expectedVolumeNames), len(actualVolumeNames)) + for _, volume := range expectedVolumeNames { + assert.Contains(t, actualVolumeNames, volume) + } +} + +// assertVolumeHasDefaultMode asserts that the given int32 pointer points to a +// value that is equal to the given int32 value. +func assertVolumeHasDefaultMode(t *testing.T, expected int32, actual *int32, volumeName string) { + t.Helper() + + if assert.NotNil(t, actual, "router deployment volume %s does not specify defaultMode", volumeName) { + assert.Equal(t, expected, *actual, "router deployment volume %s has unexpected defaultMode; expected: %O, got %O", volumeName, expected, *actual) + } +} + +// checkProbes asserts that the given container specifies liveness, readiness, +// and startup probes, and that these probes have the expected parameters. If +// useLocalhost is true, checkProbes asserts that the probe's HTTP action +// specifies host "localhost"; else, checkProbes asserts that the probe does not +// specify a host. +func checkProbes(t *testing.T, container *corev1.Container, useLocalhost bool) { + t.Helper() + + checkHandler := func(probe *corev1.Probe) { + if assert.NotNil(t, probe.HTTPGet) { + assert.Equal(t, corev1.URIScheme("HTTP"), probe.HTTPGet.Scheme) + if useLocalhost { + assert.Equal(t, "localhost", probe.HTTPGet.Host) + } else { + assert.Empty(t, probe.HTTPGet.Host) + } + } + } + + assert.Equal(t, int32(3), container.LivenessProbe.FailureThreshold) + assert.Equal(t, int32(10), container.LivenessProbe.PeriodSeconds) + assert.Equal(t, int32(1), container.LivenessProbe.SuccessThreshold) + assert.Equal(t, int32(1), container.LivenessProbe.TimeoutSeconds) + checkHandler(container.LivenessProbe) + assert.NotNil(t, container.LivenessProbe.TerminationGracePeriodSeconds, "expected liveness probe's termination grace period to be set") + + assert.Equal(t, int32(3), container.ReadinessProbe.FailureThreshold) + assert.Equal(t, int32(10), container.ReadinessProbe.PeriodSeconds) + assert.Equal(t, int32(1), container.ReadinessProbe.SuccessThreshold) + assert.Equal(t, int32(1), container.ReadinessProbe.TimeoutSeconds) + checkHandler(container.ReadinessProbe) + + assert.Equal(t, int32(120), container.StartupProbe.FailureThreshold) + assert.Equal(t, int32(1), container.StartupProbe.PeriodSeconds) + assert.Equal(t, int32(1), container.StartupProbe.SuccessThreshold) + assert.Equal(t, int32(1), container.StartupProbe.TimeoutSeconds) + checkHandler(container.StartupProbe) +} + func TestDesiredRouterDeploymentSpecTemplate(t *testing.T) { ic, ingressConfig, infraConfig, apiConfig, networkConfig, proxyNeeded, clusterProxyConfig := getRouterDeploymentComponents(t) @@ -481,12 +547,25 @@ func TestDesiredRouterDeploymentSpecTemplate(t *testing.T) { "metrics-certs": fmt.Sprintf("router-metrics-certs-%s", ic.Name), "stats-auth": fmt.Sprintf("router-stats-%s", ic.Name), } + expectedVolumes := []string{ + "default-certificate", + "metrics-certs", + "stats-auth", + "service-ca-bundle", + } + assertHasVolumes(t, deployment.Spec.Template.Spec.Volumes, expectedVolumes) for _, volume := range deployment.Spec.Template.Spec.Volumes { if secretName, ok := expectedVolumeSecretPairs[volume.Name]; ok { if volume.Secret.SecretName != secretName { t.Errorf("router Deployment expected volume %s to have secret %s, got %s", volume.Name, secretName, volume.Secret.SecretName) } - } else if volume.Name != "service-ca-bundle" { + assertVolumeHasDefaultMode(t, int32(0644), volume.Secret.DefaultMode, volume.Name) + continue + } + switch volume.Name { + case "service-ca-bundle": + assertVolumeHasDefaultMode(t, int32(0644), volume.ConfigMap.DefaultMode, volume.Name) + default: t.Errorf("router deployment has unexpected volume %s", volume.Name) } } @@ -513,18 +592,7 @@ func TestDesiredRouterDeploymentSpecTemplate(t *testing.T) { t.Errorf("expected dnsPolicy to be %s, got %s", corev1.DNSClusterFirst, deployment.Spec.Template.Spec.DNSPolicy) } - if len(deployment.Spec.Template.Spec.Containers[0].LivenessProbe.ProbeHandler.HTTPGet.Host) != 0 { - t.Errorf("expected empty liveness probe host, got %q", deployment.Spec.Template.Spec.Containers[0].LivenessProbe.ProbeHandler.HTTPGet.Host) - } - if deployment.Spec.Template.Spec.Containers[0].LivenessProbe.TerminationGracePeriodSeconds == nil { - t.Error("expected liveness probe's termination grace period to be set") - } - if len(deployment.Spec.Template.Spec.Containers[0].ReadinessProbe.ProbeHandler.HTTPGet.Host) != 0 { - t.Errorf("expected empty readiness probe host, got %q", deployment.Spec.Template.Spec.Containers[0].ReadinessProbe.ProbeHandler.HTTPGet.Host) - } - if len(deployment.Spec.Template.Spec.Containers[0].StartupProbe.ProbeHandler.HTTPGet.Host) != 0 { - t.Errorf("expected empty startup probe host, got %q", deployment.Spec.Template.Spec.Containers[0].StartupProbe.ProbeHandler.HTTPGet.Host) - } + checkProbes(t, &deployment.Spec.Template.Spec.Containers[0], false) checkDeploymentHasContainer(t, deployment, operatorv1.ContainerLoggingSidecarContainerName, false) @@ -614,20 +682,29 @@ func TestDesiredRouterDeploymentSpecAndNetwork(t *testing.T) { if deployment.Spec.Template.Spec.DNSPolicy != corev1.DNSClusterFirst { t.Errorf("expected dnsPolicy to be %s, got %s", corev1.DNSClusterFirst, deployment.Spec.Template.Spec.DNSPolicy) } - if len(deployment.Spec.Template.Spec.Containers[0].LivenessProbe.ProbeHandler.HTTPGet.Host) != 0 { - t.Errorf("expected empty liveness probe host, got %q", deployment.Spec.Template.Spec.Containers[0].LivenessProbe.ProbeHandler.HTTPGet.Host) - } - if len(deployment.Spec.Template.Spec.Containers[0].ReadinessProbe.ProbeHandler.HTTPGet.Host) != 0 { - t.Errorf("expected empty readiness probe host, got %q", deployment.Spec.Template.Spec.Containers[0].ReadinessProbe.ProbeHandler.HTTPGet.Host) - } - if len(deployment.Spec.Template.Spec.Containers[0].StartupProbe.ProbeHandler.HTTPGet.Host) != 0 { - t.Errorf("expected empty startup probe host, got %q", deployment.Spec.Template.Spec.Containers[0].StartupProbe.ProbeHandler.HTTPGet.Host) - } - if len(deployment.Spec.Template.Spec.Containers[0].VolumeMounts) <= 4 || deployment.Spec.Template.Spec.Containers[0].VolumeMounts[4].Name != "error-pages" { - t.Errorf("deployment.Spec.Template.Spec.Containers[0].VolumeMounts[4].Name %v", deployment.Spec.Template.Spec.Containers[0].VolumeMounts) - //log.Info(fmt.Sprintf("deployment.Spec.Template.Spec.Containers[0].VolumeMounts[4].Name %v", deployment.Spec.Template.Spec.Containers[0])) - t.Error("router Deployment is missing error code pages volume mount") + checkProbes(t, &deployment.Spec.Template.Spec.Containers[0], false) + + expectedVolumes := []string{ + "default-certificate", + "metrics-certs", + "stats-auth", + "service-ca-bundle", + "error-pages", + "rsyslog-config", + "rsyslog-socket", + } + assertHasVolumes(t, deployment.Spec.Template.Spec.Volumes, expectedVolumes) + for _, volume := range deployment.Spec.Template.Spec.Volumes { + switch volume.Name { + case "default-certificate", "metrics-certs", "stats-auth": + assertVolumeHasDefaultMode(t, int32(0644), volume.Secret.DefaultMode, volume.Name) + case "error-pages", "rsyslog-config", "service-ca-bundle": + assertVolumeHasDefaultMode(t, int32(0644), volume.ConfigMap.DefaultMode, volume.Name) + case "rsyslog-socket": + default: + t.Errorf("router deployment has unexpected volume %s", volume.Name) + } } checkDeploymentHasContainer(t, deployment, operatorv1.ContainerLoggingSidecarContainerName, true) @@ -696,15 +773,8 @@ func TestDesiredRouterDeploymentSpecAndNetwork(t *testing.T) { if deployment.Spec.Template.Spec.HostNetwork != false { t.Error("expected host network to be false") } - if len(deployment.Spec.Template.Spec.Containers[0].LivenessProbe.ProbeHandler.HTTPGet.Host) != 0 { - t.Errorf("expected empty liveness probe host, got %q", deployment.Spec.Template.Spec.Containers[0].LivenessProbe.ProbeHandler.HTTPGet.Host) - } - if len(deployment.Spec.Template.Spec.Containers[0].ReadinessProbe.ProbeHandler.HTTPGet.Host) != 0 { - t.Errorf("expected empty readiness probe host, got %q", deployment.Spec.Template.Spec.Containers[0].ReadinessProbe.ProbeHandler.HTTPGet.Host) - } - if len(deployment.Spec.Template.Spec.Containers[0].StartupProbe.ProbeHandler.HTTPGet.Host) != 0 { - t.Errorf("expected empty startup probe host, got %q", deployment.Spec.Template.Spec.Containers[0].StartupProbe.ProbeHandler.HTTPGet.Host) - } + + checkProbes(t, &deployment.Spec.Template.Spec.Containers[0], false) tests = []envData{ {"ROUTER_HAPROXY_CONFIG_MANAGER", true, "true"}, @@ -835,28 +905,32 @@ func TestDesiredRouterDeploymentVariety(t *testing.T) { t.Errorf("expected dnsPolicy to be %s, got %s", corev1.DNSClusterFirstWithHostNet, deployment.Spec.Template.Spec.DNSPolicy) } - if deployment.Spec.Template.Spec.Containers[0].LivenessProbe.ProbeHandler.HTTPGet.Host != "localhost" { - t.Errorf("expected liveness probe host to be \"localhost\", got %q", deployment.Spec.Template.Spec.Containers[0].LivenessProbe.ProbeHandler.HTTPGet.Host) - } - if deployment.Spec.Template.Spec.Containers[0].ReadinessProbe.ProbeHandler.HTTPGet.Host != "localhost" { - t.Errorf("expected readiness probe host to be \"localhost\", got %q", deployment.Spec.Template.Spec.Containers[0].ReadinessProbe.ProbeHandler.HTTPGet.Host) - } - if deployment.Spec.Template.Spec.Containers[0].StartupProbe.ProbeHandler.HTTPGet.Host != "localhost" { - t.Errorf("expected startup probe host to be \"localhost\", got %q", deployment.Spec.Template.Spec.Containers[0].StartupProbe.ProbeHandler.HTTPGet.Host) - } + checkProbes(t, &deployment.Spec.Template.Spec.Containers[0], true) expectedVolumeSecretPairs := map[string]string{ "default-certificate": secretName, "metrics-certs": fmt.Sprintf("router-metrics-certs-%s", ic.Name), "stats-auth": fmt.Sprintf("router-stats-%s", ic.Name), } - + expectedVolumes := []string{ + "default-certificate", + "metrics-certs", + "stats-auth", + "service-ca-bundle", + } + assertHasVolumes(t, deployment.Spec.Template.Spec.Volumes, expectedVolumes) for _, volume := range deployment.Spec.Template.Spec.Volumes { if secretName, ok := expectedVolumeSecretPairs[volume.Name]; ok { if volume.Secret.SecretName != secretName { t.Errorf("router Deployment expected volume %s to have secret %s, got %s", volume.Name, secretName, volume.Secret.SecretName) } - } else if volume.Name != "service-ca-bundle" && volume.Name != "error-pages" { + assertVolumeHasDefaultMode(t, int32(0644), volume.Secret.DefaultMode, volume.Name) + continue + } + switch volume.Name { + case "service-ca-bundle": + assertVolumeHasDefaultMode(t, int32(0644), volume.ConfigMap.DefaultMode, volume.Name) + default: t.Errorf("router deployment has unexpected volume %s", volume.Name) } } @@ -965,6 +1039,54 @@ func TestDesiredRouterDeploymentSingleReplica(t *testing.T) { } } +// TestDesiredRouterDeploymentClientTLS verifies that desiredRouterDeployment +// returns the expected deployment when client TLS is enabled. +func TestDesiredRouterDeploymentClientTLS(t *testing.T) { + ic, ingressConfig, infraConfig, apiConfig, networkConfig, _, clusterProxyConfig := getRouterDeploymentComponents(t) + ic.Spec.ClientTLS = operatorv1.ClientTLS{ + AllowedSubjectPatterns: []string{"foo", "bar"}, + ClientCertificatePolicy: "Optional", + ClientCA: configv1.ConfigMapNameReference{ + Name: "baz", + }, + } + clientCAConfigmap := &corev1.ConfigMap{} + deployment, err := desiredRouterDeployment(ic, ingressControllerImage, ingressConfig, infraConfig, apiConfig, networkConfig, false, true, clientCAConfigmap, clusterProxyConfig, false) + if err != nil { + t.Fatalf("invalid router deployment: %v", err) + } + + env := []envData{ + {"ROUTER_MUTUAL_TLS_AUTH", true, "optional"}, + {"ROUTER_MUTUAL_TLS_AUTH_CA", true, "/etc/pki/tls/client-ca/ca-bundle.pem"}, + {"ROUTER_MUTUAL_TLS_AUTH_FILTER", true, `(?:foo|bar)`}, + } + if err := checkDeploymentEnvironment(t, deployment, env); err != nil { + t.Error(err) + } + expectedVolumes := []string{ + "default-certificate", + "metrics-certs", + "stats-auth", + "service-ca-bundle", + "client-ca", + } + assertHasVolumes(t, deployment.Spec.Template.Spec.Volumes, expectedVolumes) + for _, volume := range deployment.Spec.Template.Spec.Volumes { + switch volume.Name { + case "default-certificate", "metrics-certs", "stats-auth": + assertVolumeHasDefaultMode(t, int32(0644), volume.Secret.DefaultMode, volume.Name) + case "service-ca-bundle": + assertVolumeHasDefaultMode(t, int32(0644), volume.ConfigMap.DefaultMode, volume.Name) + case "client-ca": + assertVolumeHasDefaultMode(t, int32(0644), volume.ConfigMap.DefaultMode, volume.Name) + assert.Equal(t, "router-client-ca-default", volume.VolumeSource.ConfigMap.LocalObjectReference.Name) + default: + t.Errorf("router deployment has unexpected volume %s", volume.Name) + } + } +} + func checkContainerPort(t *testing.T, d *appsv1.Deployment, portName string, port int32) { t.Helper() for _, p := range d.Spec.Template.Spec.Containers[0].Ports { @@ -1577,14 +1699,14 @@ func Test_deploymentConfigChanged(t *testing.T) { { description: "if the router container .securityContext.allowPrivilegeEscalation changes", mutate: func(deployment *appsv1.Deployment) { - deployment.Spec.Template.Spec.Containers[0].SecurityContext.AllowPrivilegeEscalation = pointer.Bool(false) + deployment.Spec.Template.Spec.Containers[0].SecurityContext.AllowPrivilegeEscalation = ptr.To[bool](false) }, expect: true, }, { description: "if the router container .securityContext.readOnlyRootFilesystem changes", mutate: func(deployment *appsv1.Deployment) { - deployment.Spec.Template.Spec.Containers[0].SecurityContext.ReadOnlyRootFilesystem = pointer.Bool(true) + deployment.Spec.Template.Spec.Containers[0].SecurityContext.ReadOnlyRootFilesystem = ptr.To[bool](true) }, expect: true, }, @@ -1773,8 +1895,8 @@ func Test_deploymentConfigChanged(t *testing.T) { }, }, SecurityContext: &corev1.SecurityContext{ - AllowPrivilegeEscalation: pointer.Bool(true), - ReadOnlyRootFilesystem: pointer.Bool(false), + AllowPrivilegeEscalation: ptr.To[bool](true), + ReadOnlyRootFilesystem: ptr.To[bool](false), }, }, }, @@ -1869,6 +1991,9 @@ func Test_deploymentConfigChanged(t *testing.T) { if changed, updated := deploymentConfigChanged(&original, mutated); changed != tc.expect { t.Errorf("expect deploymentConfigChanged to be %t, got %t", tc.expect, changed) } else if changed { + if updatedChanged, _ := deploymentConfigChanged(&original, updated); !updatedChanged { + t.Error("deploymentConfigChanged reported changes but did not make any update") + } if changedAgain, _ := deploymentConfigChanged(mutated, updated); changedAgain { t.Error("deploymentConfigChanged does not behave as a fixed point function") } diff --git a/pkg/operator/controller/ingress/internal_service.go b/pkg/operator/controller/ingress/internal_service.go index 36a2f441f6..f1891dd955 100644 --- a/pkg/operator/controller/ingress/internal_service.go +++ b/pkg/operator/controller/ingress/internal_service.go @@ -118,7 +118,8 @@ func internalServiceChanged(current, expected *corev1.Service) (bool, *corev1.Se serviceCmpOpts := []cmp.Option{ // Ignore fields that the API, other controllers, or user may - // have modified. + // have modified. Note: This list must be kept consistent with + // the updated.Spec.Foo = current.Spec.Foo assignments below! cmpopts.IgnoreFields(corev1.ServicePort{}, "NodePort"), cmpopts.IgnoreFields(corev1.ServiceSpec{}, "ClusterIP", "ClusterIPs", @@ -163,14 +164,18 @@ func internalServiceChanged(current, expected *corev1.Service) (bool, *corev1.Se } } // Preserve fields that the API, other controllers, or user may have - // modified. + // modified. Note: This list must be kept consistent with + // serviceCmpOpts above! updated.Spec.ClusterIP = current.Spec.ClusterIP + updated.Spec.ClusterIPs = current.Spec.ClusterIPs updated.Spec.ExternalIPs = current.Spec.ExternalIPs updated.Spec.HealthCheckNodePort = current.Spec.HealthCheckNodePort + updated.Spec.IPFamilies = current.Spec.IPFamilies + updated.Spec.IPFamilyPolicy = current.Spec.IPFamilyPolicy for i, updatedPort := range updated.Spec.Ports { for _, currentPort := range current.Spec.Ports { if currentPort.Name == updatedPort.Name { - updated.Spec.Ports[i].TargetPort = currentPort.TargetPort + updated.Spec.Ports[i].NodePort = currentPort.NodePort } } } diff --git a/pkg/operator/controller/ingress/internal_service_test.go b/pkg/operator/controller/ingress/internal_service_test.go index 729bda5ece..fe130634c3 100644 --- a/pkg/operator/controller/ingress/internal_service_test.go +++ b/pkg/operator/controller/ingress/internal_service_test.go @@ -13,7 +13,7 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" ) -// test_desiredInternalIngressControllerService verifies that +// Test_desiredInternalIngressControllerService verifies that // desiredInternalIngressControllerService returns the expected service. func Test_desiredInternalIngressControllerService(t *testing.T) { ic := &operatorv1.IngressController{ @@ -53,6 +53,7 @@ func Test_desiredInternalIngressControllerService(t *testing.T) { Port: int32(1936), TargetPort: intstr.FromString("metrics"), }}, svc.Spec.Ports) + assert.Equal(t, "None", string(svc.Spec.SessionAffinity)) } // Test_internalServiceChanged verifies that internalServiceChanged properly @@ -125,6 +126,14 @@ func Test_internalServiceChanged(t *testing.T) { }, expect: true, }, + { + description: "if .spec.ports[*].nodePort changes", + mutate: func(svc *corev1.Service) { + svc.Spec.Ports[0].NodePort = int32(33337) + svc.Spec.Ports[1].NodePort = int32(33338) + }, + expect: false, + }, { description: "if .spec.ports changes by changing the metrics port's target port from an integer to a string", mutate: func(svc *corev1.Service) { @@ -227,6 +236,9 @@ func Test_internalServiceChanged(t *testing.T) { if changed, updated := internalServiceChanged(&original, mutated); changed != tc.expect { t.Errorf("expected internalServiceChanged to be %t, got %t", tc.expect, changed) } else if changed { + if updatedChanged, _ := internalServiceChanged(&original, updated); !updatedChanged { + t.Error("internalServiceChanged reported changes but did not make any update") + } if changedAgain, _ := internalServiceChanged(mutated, updated); changedAgain { t.Error("internalServiceChanged does not behave as a fixed point function") } diff --git a/pkg/operator/controller/ingress/load_balancer_service_test.go b/pkg/operator/controller/ingress/load_balancer_service_test.go index 3cf487d3cb..e68711ecfc 100644 --- a/pkg/operator/controller/ingress/load_balancer_service_test.go +++ b/pkg/operator/controller/ingress/load_balancer_service_test.go @@ -82,19 +82,21 @@ func Test_desiredLoadBalancerService(t *testing.T) { value string } testCases := []struct { - description string - strategy *operatorv1.EndpointPublishingStrategy - proxyNeeded bool - expectService bool - expectedServiceAnnotations map[string]annotationExpectation - platformStatus *configv1.PlatformStatus + description string + strategy *operatorv1.EndpointPublishingStrategy + proxyNeeded bool + expectService bool + expectedServiceAnnotations map[string]annotationExpectation + expectedExternalTrafficPolicy corev1.ServiceExternalTrafficPolicy + platformStatus *configv1.PlatformStatus }{ { - description: "external classic load balancer with scope for aws platform", - platformStatus: platformStatus(configv1.AWSPlatformType), - strategy: lbs(operatorv1.ExternalLoadBalancer), - proxyNeeded: true, - expectService: true, + description: "external classic load balancer with scope for aws platform", + platformStatus: platformStatus(configv1.AWSPlatformType), + strategy: lbs(operatorv1.ExternalLoadBalancer), + proxyNeeded: true, + expectService: true, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, expectedServiceAnnotations: map[string]annotationExpectation{ awsInternalLBAnnotation: {false, ""}, awsLBAdditionalResourceTags: {false, ""}, @@ -107,10 +109,11 @@ func Test_desiredLoadBalancerService(t *testing.T) { }, }, { - description: "external classic load balancer with scope for aws platform and custom user tags", - strategy: lbs(operatorv1.ExternalLoadBalancer), - proxyNeeded: true, - expectService: true, + description: "external classic load balancer with scope for aws platform and custom user tags", + strategy: lbs(operatorv1.ExternalLoadBalancer), + proxyNeeded: true, + expectService: true, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, expectedServiceAnnotations: map[string]annotationExpectation{ awsInternalLBAnnotation: {false, ""}, awsLBAdditionalResourceTags: {true, "classic-load-balancer-key-with-value=100,classic-load-balancer-key-with-empty-value="}, @@ -137,11 +140,12 @@ func Test_desiredLoadBalancerService(t *testing.T) { }, }, { - description: "external classic load balancer without LoadBalancerStrategy for aws platform", - platformStatus: platformStatus(configv1.AWSPlatformType), - strategy: lbs(""), - proxyNeeded: true, - expectService: true, + description: "external classic load balancer without LoadBalancerStrategy for aws platform", + platformStatus: platformStatus(configv1.AWSPlatformType), + strategy: lbs(""), + proxyNeeded: true, + expectService: true, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, expectedServiceAnnotations: map[string]annotationExpectation{ awsInternalLBAnnotation: {false, ""}, awsLBAdditionalResourceTags: {false, ""}, @@ -154,11 +158,12 @@ func Test_desiredLoadBalancerService(t *testing.T) { }, }, { - description: "internal classic load balancer for aws platform", - platformStatus: platformStatus(configv1.AWSPlatformType), - strategy: lbs(operatorv1.InternalLoadBalancer), - proxyNeeded: true, - expectService: true, + description: "internal classic load balancer for aws platform", + platformStatus: platformStatus(configv1.AWSPlatformType), + strategy: lbs(operatorv1.InternalLoadBalancer), + proxyNeeded: true, + expectService: true, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, expectedServiceAnnotations: map[string]annotationExpectation{ awsInternalLBAnnotation: {true, "true"}, awsLBAdditionalResourceTags: {false, ""}, @@ -171,11 +176,12 @@ func Test_desiredLoadBalancerService(t *testing.T) { }, }, { - description: "external network load balancer without scope for aws platform", - platformStatus: platformStatus(configv1.AWSPlatformType), - strategy: nlb(""), - proxyNeeded: false, - expectService: true, + description: "external network load balancer without scope for aws platform", + platformStatus: platformStatus(configv1.AWSPlatformType), + strategy: nlb(""), + proxyNeeded: false, + expectService: true, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, expectedServiceAnnotations: map[string]annotationExpectation{ awsInternalLBAnnotation: {false, ""}, awsLBAdditionalResourceTags: {false, ""}, @@ -189,11 +195,12 @@ func Test_desiredLoadBalancerService(t *testing.T) { }, }, { - description: "external network load balancer with scope for aws platform", - platformStatus: platformStatus(configv1.AWSPlatformType), - strategy: nlb(operatorv1.ExternalLoadBalancer), - proxyNeeded: false, - expectService: true, + description: "external network load balancer with scope for aws platform", + platformStatus: platformStatus(configv1.AWSPlatformType), + strategy: nlb(operatorv1.ExternalLoadBalancer), + proxyNeeded: false, + expectService: true, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, expectedServiceAnnotations: map[string]annotationExpectation{ awsInternalLBAnnotation: {false, ""}, awsLBAdditionalResourceTags: {false, ""}, @@ -225,6 +232,7 @@ func Test_desiredLoadBalancerService(t *testing.T) { }}, }, }, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, expectedServiceAnnotations: map[string]annotationExpectation{ awsInternalLBAnnotation: {false, ""}, awsLBAdditionalResourceTags: {true, "network-load-balancer-key-with-value=200,network-load-balancer-key-with-empty-value="}, @@ -244,66 +252,73 @@ func Test_desiredLoadBalancerService(t *testing.T) { expectService: false, }, { - description: "external load balancer for ibm platform", - platformStatus: platformStatus(configv1.IBMCloudPlatformType), - strategy: lbs(operatorv1.ExternalLoadBalancer), - expectService: true, + description: "external load balancer for ibm platform", + platformStatus: platformStatus(configv1.IBMCloudPlatformType), + strategy: lbs(operatorv1.ExternalLoadBalancer), + expectService: true, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyCluster, expectedServiceAnnotations: map[string]annotationExpectation{ iksLBScopeAnnotation: {true, iksLBScopePublic}, }, }, { - description: "internal load balancer for ibm platform", - platformStatus: platformStatus(configv1.IBMCloudPlatformType), - strategy: lbs(operatorv1.InternalLoadBalancer), - expectService: true, + description: "internal load balancer for ibm platform", + platformStatus: platformStatus(configv1.IBMCloudPlatformType), + strategy: lbs(operatorv1.InternalLoadBalancer), + expectService: true, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyCluster, expectedServiceAnnotations: map[string]annotationExpectation{ iksLBScopeAnnotation: {true, iksLBScopePrivate}, }, }, { - description: "external load balancer for Power VS platform", - platformStatus: platformStatus(configv1.PowerVSPlatformType), - strategy: lbs(operatorv1.ExternalLoadBalancer), - expectService: true, + description: "external load balancer for Power VS platform", + platformStatus: platformStatus(configv1.PowerVSPlatformType), + strategy: lbs(operatorv1.ExternalLoadBalancer), + expectService: true, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyCluster, expectedServiceAnnotations: map[string]annotationExpectation{ iksLBScopeAnnotation: {true, iksLBScopePublic}, }, }, { - description: "internal load balancer for Power VS platform", - platformStatus: platformStatus(configv1.PowerVSPlatformType), - strategy: lbs(operatorv1.InternalLoadBalancer), - expectService: true, + description: "internal load balancer for Power VS platform", + platformStatus: platformStatus(configv1.PowerVSPlatformType), + strategy: lbs(operatorv1.InternalLoadBalancer), + expectService: true, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyCluster, expectedServiceAnnotations: map[string]annotationExpectation{ iksLBScopeAnnotation: {true, iksLBScopePrivate}, }, }, { - description: "external load balancer for azure platform", - platformStatus: platformStatus(configv1.AzurePlatformType), - strategy: lbs(operatorv1.ExternalLoadBalancer), - expectService: true, + description: "external load balancer for azure platform", + platformStatus: platformStatus(configv1.AzurePlatformType), + strategy: lbs(operatorv1.ExternalLoadBalancer), + expectService: true, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, expectedServiceAnnotations: map[string]annotationExpectation{ azureInternalLBAnnotation: {false, ""}, localWithFallbackAnnotation: {true, ""}, }, }, { - description: "internal load balancer for azure platform", - platformStatus: platformStatus(configv1.AzurePlatformType), - strategy: lbs(operatorv1.InternalLoadBalancer), - expectService: true, + description: "internal load balancer for azure platform", + platformStatus: platformStatus(configv1.AzurePlatformType), + strategy: lbs(operatorv1.InternalLoadBalancer), + expectService: true, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, expectedServiceAnnotations: map[string]annotationExpectation{ azureInternalLBAnnotation: {true, "true"}, localWithFallbackAnnotation: {true, ""}, }, }, { - description: "external load balancer for gcp platform", - platformStatus: platformStatus(configv1.GCPPlatformType), - strategy: lbs(operatorv1.ExternalLoadBalancer), - expectService: true, + description: "external load balancer for gcp platform", + platformStatus: platformStatus(configv1.GCPPlatformType), + strategy: lbs(operatorv1.ExternalLoadBalancer), + expectService: true, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, expectedServiceAnnotations: map[string]annotationExpectation{ GCPGlobalAccessAnnotation: {false, ""}, gcpLBTypeAnnotation: {false, ""}, @@ -311,10 +326,11 @@ func Test_desiredLoadBalancerService(t *testing.T) { }, }, { - description: "internal load balancer for gcp platform", - platformStatus: platformStatus(configv1.GCPPlatformType), - strategy: lbs(operatorv1.InternalLoadBalancer), - expectService: true, + description: "internal load balancer for gcp platform", + platformStatus: platformStatus(configv1.GCPPlatformType), + strategy: lbs(operatorv1.InternalLoadBalancer), + expectService: true, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, expectedServiceAnnotations: map[string]annotationExpectation{ GCPGlobalAccessAnnotation: {false, ""}, gcpLBTypeAnnotation: {true, "Internal"}, @@ -322,10 +338,11 @@ func Test_desiredLoadBalancerService(t *testing.T) { }, }, { - description: "internal load balancer for gcp platform with global ClientAccess", - platformStatus: platformStatus(configv1.GCPPlatformType), - strategy: gcpLB(operatorv1.InternalLoadBalancer, operatorv1.GCPGlobalAccess), - expectService: true, + description: "internal load balancer for gcp platform with global ClientAccess", + platformStatus: platformStatus(configv1.GCPPlatformType), + strategy: gcpLB(operatorv1.InternalLoadBalancer, operatorv1.GCPGlobalAccess), + expectService: true, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, expectedServiceAnnotations: map[string]annotationExpectation{ GCPGlobalAccessAnnotation: {true, "true"}, gcpLBTypeAnnotation: {true, "Internal"}, @@ -333,10 +350,11 @@ func Test_desiredLoadBalancerService(t *testing.T) { }, }, { - description: "internal load balancer for gcp platform with local ClientAccess", - platformStatus: platformStatus(configv1.GCPPlatformType), - strategy: gcpLB(operatorv1.InternalLoadBalancer, operatorv1.GCPLocalAccess), - expectService: true, + description: "internal load balancer for gcp platform with local ClientAccess", + platformStatus: platformStatus(configv1.GCPPlatformType), + strategy: gcpLB(operatorv1.InternalLoadBalancer, operatorv1.GCPLocalAccess), + expectService: true, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, expectedServiceAnnotations: map[string]annotationExpectation{ GCPGlobalAccessAnnotation: {true, "false"}, gcpLBTypeAnnotation: {true, "Internal"}, @@ -344,40 +362,44 @@ func Test_desiredLoadBalancerService(t *testing.T) { }, }, { - description: "external load balancer for openstack platform", - platformStatus: platformStatus(configv1.OpenStackPlatformType), - strategy: lbs(operatorv1.ExternalLoadBalancer), - expectService: true, + description: "external load balancer for openstack platform", + platformStatus: platformStatus(configv1.OpenStackPlatformType), + strategy: lbs(operatorv1.ExternalLoadBalancer), + expectService: true, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, expectedServiceAnnotations: map[string]annotationExpectation{ openstackInternalLBAnnotation: {false, ""}, localWithFallbackAnnotation: {true, ""}, }, }, { - description: "internal load balancer for openstack platform", - platformStatus: platformStatus(configv1.OpenStackPlatformType), - strategy: lbs(operatorv1.InternalLoadBalancer), - expectService: true, + description: "internal load balancer for openstack platform", + platformStatus: platformStatus(configv1.OpenStackPlatformType), + strategy: lbs(operatorv1.InternalLoadBalancer), + expectService: true, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, expectedServiceAnnotations: map[string]annotationExpectation{ openstackInternalLBAnnotation: {true, "true"}, localWithFallbackAnnotation: {true, ""}, }, }, { - description: "external load balancer for alibaba platform", - platformStatus: platformStatus(configv1.AlibabaCloudPlatformType), - strategy: lbs(operatorv1.ExternalLoadBalancer), - expectService: true, + description: "external load balancer for alibaba platform", + platformStatus: platformStatus(configv1.AlibabaCloudPlatformType), + strategy: lbs(operatorv1.ExternalLoadBalancer), + expectService: true, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, expectedServiceAnnotations: map[string]annotationExpectation{ alibabaCloudLBAddressTypeAnnotation: {true, alibabaCloudLBAddressTypeInternet}, localWithFallbackAnnotation: {true, ""}, }, }, { - description: "internal load balancer for alibaba platform", - platformStatus: platformStatus(configv1.AlibabaCloudPlatformType), - strategy: lbs(operatorv1.InternalLoadBalancer), - expectService: true, + description: "internal load balancer for alibaba platform", + platformStatus: platformStatus(configv1.AlibabaCloudPlatformType), + strategy: lbs(operatorv1.InternalLoadBalancer), + expectService: true, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, expectedServiceAnnotations: map[string]annotationExpectation{ alibabaCloudLBAddressTypeAnnotation: {true, alibabaCloudLBAddressTypeIntranet}, localWithFallbackAnnotation: {true, ""}, @@ -448,6 +470,21 @@ func Test_desiredLoadBalancerService(t *testing.T) { t.Errorf("service has unexpected annotation: %s=%s", k, v) } } + assert.Equal(t, "LoadBalancer", string(svc.Spec.Type)) + assert.Equal(t, tc.expectedExternalTrafficPolicy, svc.Spec.ExternalTrafficPolicy) + assert.Equal(t, "Cluster", string(*svc.Spec.InternalTrafficPolicy)) + assert.Equal(t, []corev1.ServicePort{{ + Name: "http", + Protocol: "TCP", + Port: int32(80), + TargetPort: intstr.FromString("http"), + }, { + Name: "https", + Protocol: "TCP", + Port: int32(443), + TargetPort: intstr.FromString("https"), + }}, svc.Spec.Ports) + assert.Equal(t, "None", string(svc.Spec.SessionAffinity)) }) } } @@ -872,6 +909,9 @@ func Test_loadBalancerServiceChanged(t *testing.T) { if changed, updated := loadBalancerServiceChanged(&original, mutated); changed != tc.expect { t.Errorf("expected loadBalancerServiceChanged to be %t, got %t", tc.expect, changed) } else if changed { + if updatedChanged, _ := loadBalancerServiceChanged(&original, updated); !updatedChanged { + t.Error("loadBalancerServiceChanged reported changes but did not make any update") + } if changedAgain, _ := loadBalancerServiceChanged(mutated, updated); changedAgain { t.Error("loadBalancerServiceChanged does not behave as a fixed point function") } @@ -970,6 +1010,9 @@ func Test_loadBalancerServiceAnnotationsChanged(t *testing.T) { if changed, updated := loadBalancerServiceAnnotationsChanged(¤t, &expected, tc.managedAnnotations); changed != tc.expect { t.Errorf("expected loadBalancerServiceAnnotationsChanged to be %t, got %t", tc.expect, changed) } else if changed { + if updatedChanged, _ := loadBalancerServiceAnnotationsChanged(¤t, updated, tc.managedAnnotations); !updatedChanged { + t.Error("loadBalancerServiceAnnotationsChanged reported changes but did not make any update") + } if changedAgain, _ := loadBalancerServiceAnnotationsChanged(&expected, updated, tc.managedAnnotations); changedAgain { t.Error("loadBalancerServiceAnnotationsChanged does not behave as a fixed point function") } diff --git a/pkg/operator/controller/ingress/monitoring_test.go b/pkg/operator/controller/ingress/monitoring_test.go index bfa2dac1e4..975df9ec42 100644 --- a/pkg/operator/controller/ingress/monitoring_test.go +++ b/pkg/operator/controller/ingress/monitoring_test.go @@ -41,7 +41,12 @@ func Test_serviceMonitorChanged(t *testing.T) { } if changed, sm3 := serviceMonitorChanged(sm1, sm2); !changed { t.Fatal("expected changed to be true after clearing servicemonitor's selector") - } else if changedAgain, _ := serviceMonitorChanged(sm2, sm3); changedAgain { - t.Fatal("serviceMonitorChanged does not behave as a fixed-point function") + } else { + if updatedChanged, _ := serviceMonitorChanged(sm1, sm3); !updatedChanged { + t.Error("serviceMonitorChanged reported changes but did not make any update") + } + if changedAgain, _ := serviceMonitorChanged(sm2, sm3); changedAgain { + t.Fatal("serviceMonitorChanged does not behave as a fixed-point function") + } } } diff --git a/pkg/operator/controller/ingress/namespace_test.go b/pkg/operator/controller/ingress/namespace_test.go index e0335cc9d8..99155c592c 100644 --- a/pkg/operator/controller/ingress/namespace_test.go +++ b/pkg/operator/controller/ingress/namespace_test.go @@ -58,6 +58,9 @@ func Test_routerNamespaceChanged(t *testing.T) { if changed, updated := routerNamespaceChanged(mutated, desired); changed != tc.expect { t.Errorf("expect routerNamespaceChanged to be %t, got %t", tc.expect, changed) } else if changed { + if updatedChanged, _ := routerNamespaceChanged(mutated, updated); !updatedChanged { + t.Error("routerNamespaceChanged reported changes but did not make any update") + } if changedAgain, _ := routerNamespaceChanged(desired, updated); changedAgain { t.Error("routerNamespaceChanged does not behave as a fixed point function") } diff --git a/pkg/operator/controller/ingress/nodeport_service.go b/pkg/operator/controller/ingress/nodeport_service.go index a74284a2dd..2f7719f21f 100644 --- a/pkg/operator/controller/ingress/nodeport_service.go +++ b/pkg/operator/controller/ingress/nodeport_service.go @@ -134,8 +134,9 @@ func desiredNodePortService(ic *operatorv1.IngressController, deploymentRef meta TargetPort: intstr.FromString("metrics"), }, }, - Selector: controller.IngressControllerDeploymentPodSelector(ic).MatchLabels, - Type: corev1.ServiceTypeNodePort, + Selector: controller.IngressControllerDeploymentPodSelector(ic).MatchLabels, + SessionAffinity: corev1.ServiceAffinityNone, + Type: corev1.ServiceTypeNodePort, }, } if !wantMetricsPort { @@ -195,7 +196,8 @@ func nodePortServiceChanged(current, expected *corev1.Service) (bool, *corev1.Se serviceCmpOpts := []cmp.Option{ // Ignore fields that the API, other controllers, or user may - // have modified. + // have modified. Note: This list must be kept consistent with + // the updated.Spec.Foo = current.Spec.Foo assignments below! cmpopts.IgnoreFields(corev1.ServicePort{}, "NodePort"), cmpopts.IgnoreFields(corev1.ServiceSpec{}, "ClusterIP", "ClusterIPs", @@ -240,10 +242,14 @@ func nodePortServiceChanged(current, expected *corev1.Service) (bool, *corev1.Se } } // Preserve fields that the API, other controllers, or user may have - // modified. + // modified. Note: This list must be kept consistent with + // serviceCmpOpts above! updated.Spec.ClusterIP = current.Spec.ClusterIP + updated.Spec.ClusterIPs = current.Spec.ClusterIPs updated.Spec.ExternalIPs = current.Spec.ExternalIPs updated.Spec.HealthCheckNodePort = current.Spec.HealthCheckNodePort + updated.Spec.IPFamilies = current.Spec.IPFamilies + updated.Spec.IPFamilyPolicy = current.Spec.IPFamilyPolicy for i, updatedPort := range updated.Spec.Ports { for _, currentPort := range current.Spec.Ports { if currentPort.Name == updatedPort.Name { diff --git a/pkg/operator/controller/ingress/nodeport_service_test.go b/pkg/operator/controller/ingress/nodeport_service_test.go index 2d39d6c815..4611a405f3 100644 --- a/pkg/operator/controller/ingress/nodeport_service_test.go +++ b/pkg/operator/controller/ingress/nodeport_service_test.go @@ -72,7 +72,8 @@ func Test_desiredNodePortService(t *testing.T) { Selector: map[string]string{ "ingresscontroller.operator.openshift.io/deployment-ingresscontroller": "default", }, - Type: "NodePort", + SessionAffinity: corev1.ServiceAffinityNone, + Type: "NodePort", }, }, }, @@ -120,7 +121,8 @@ func Test_desiredNodePortService(t *testing.T) { Selector: map[string]string{ "ingresscontroller.operator.openshift.io/deployment-ingresscontroller": "default", }, - Type: "NodePort", + SessionAffinity: corev1.ServiceAffinityNone, + Type: "NodePort", }, }, }, @@ -332,6 +334,9 @@ func Test_nodePortServiceChanged(t *testing.T) { if changed, updated := nodePortServiceChanged(&original, mutated); changed != tc.expect { t.Errorf("expect nodePortServiceChanged to be %t, got %t", tc.expect, changed) } else if changed { + if updatedChanged, _ := nodePortServiceChanged(&original, updated); !updatedChanged { + t.Error("nodePortServiceChanged reported changes but did not make any update") + } if changedAgain, _ := nodePortServiceChanged(mutated, updated); changedAgain { t.Error("nodePortServiceChanged does not behave as a fixed point function") } diff --git a/pkg/operator/controller/ingress/poddisruptionbudget_test.go b/pkg/operator/controller/ingress/poddisruptionbudget_test.go index a80b941937..e47e41952c 100644 --- a/pkg/operator/controller/ingress/poddisruptionbudget_test.go +++ b/pkg/operator/controller/ingress/poddisruptionbudget_test.go @@ -148,6 +148,9 @@ func Test_podDisruptionBudgetChange(t *testing.T) { if changed, updatedPdb := podDisruptionBudgetChanged(originalPdb, mutatedPdb); changed != tc.expect { t.Errorf("%s, expect podDisruptionBudgetChanged to be %t, got %t", tc.description, tc.expect, changed) } else if changed { + if updatedChanged, _ := podDisruptionBudgetChanged(originalPdb, updatedPdb); !updatedChanged { + t.Error("podDisruptionBudgetChanged reported changes but did not make any update") + } if changedAgain, _ := podDisruptionBudgetChanged(mutatedPdb, updatedPdb); changedAgain { t.Errorf("%s, podDisruptionBudgetChanged does not behave as a fixed point function", tc.description) } diff --git a/pkg/operator/controller/ingressclass/ingressclass_test.go b/pkg/operator/controller/ingressclass/ingressclass_test.go index f51ff50540..fb313a989a 100644 --- a/pkg/operator/controller/ingressclass/ingressclass_test.go +++ b/pkg/operator/controller/ingressclass/ingressclass_test.go @@ -182,6 +182,9 @@ func Test_ingressClassChanged(t *testing.T) { if changed, updated := ingressClassChanged(&original, mutated); changed != tc.expect { t.Errorf("expect ingressClassChanged to be %t, got %t", tc.expect, changed) } else if changed { + if updatedChanged, _ := ingressClassChanged(&original, updated); !updatedChanged { + t.Error("ingressClassChanged reported changes but did not make any update") + } if changedAgain, _ := ingressClassChanged(mutated, updated); changedAgain { t.Error("ingressClassChanged does not behave as a fixed point function") } diff --git a/test/e2e/all_test.go b/test/e2e/all_test.go index 14ab641b82..f1019d1b7f 100644 --- a/test/e2e/all_test.go +++ b/test/e2e/all_test.go @@ -81,6 +81,7 @@ func TestAll(t *testing.T) { t.Run("TestRouteMetricsControllerRouteAndNamespaceSelector", TestRouteMetricsControllerRouteAndNamespaceSelector) t.Run("TestSetIngressControllerResponseHeaders", TestSetIngressControllerResponseHeaders) t.Run("TestSetRouteResponseHeaders", TestSetRouteResponseHeaders) + t.Run("TestReconcileInternalService", TestReconcileInternalService) }) t.Run("serial", func(t *testing.T) { diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index 00a0aaac6b..f0cd9cce80 100644 --- a/test/e2e/operator_test.go +++ b/test/e2e/operator_test.go @@ -40,6 +40,7 @@ import ( "github.com/aws/aws-sdk-go/aws/endpoints" "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/config" @@ -55,6 +56,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/utils/pointer" @@ -970,23 +972,23 @@ func TestHostNetworkEndpointPublishingStrategy(t *testing.T) { // TestHostNetworkPortBinding creates two ingresscontrollers on the same node // with different port bindings and verifies that both routers are available. func TestHostNetworkPortBinding(t *testing.T) { - // deploy first ingresscontroller with the default port bindings + t.Log("creating an ingresscontroller with the default port bindings") name1 := types.NamespacedName{Namespace: operatorNamespace, Name: "hostnetworkportbinding"} ing1 := newHostNetworkController(name1, name1.Name+"."+dnsConfig.Spec.BaseDomain) if err := kclient.Create(context.TODO(), ing1); err != nil { t.Fatalf("failed to create the first ingresscontroller: %v", err) } - defer assertIngressControllerDeleted(t, kclient, ing1) + t.Cleanup(func() { assertIngressControllerDeleted(t, kclient, ing1) }) err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, name1, availableConditionsForIngressControllerWithHostNetwork...) if err != nil { t.Errorf("failed to observe expected conditions for the first ingresscontroller: %v", err) } - // get first router's single replica + t.Log("getting pod list for the first ingresscontroller") pods := &corev1.PodList{} if err := kclient.List(context.TODO(), pods, client.InNamespace(operandNamespace)); err != nil { - t.Fatalf("failed to list the first ingresscontroller's PODs: %v", err) + t.Fatalf("failed to list the first ingresscontroller's pods: %v", err) } var pod1 *corev1.Pod for _, p := range pods.Items { @@ -996,7 +998,7 @@ func TestHostNetworkPortBinding(t *testing.T) { } } if pod1 == nil { - t.Fatal("failed to find the first ingresscontroller's POD") + t.Fatal("failed to find the first ingresscontroller's pod") } routerContainer := pod1.Spec.Containers[0] @@ -1007,14 +1009,40 @@ func TestHostNetworkPortBinding(t *testing.T) { t.Errorf("pod %s is not running on the host's network", pod1.Name) } - // create second ingresscontroller on the same node but with different port bindings + t.Log("verifying that the first ingresscontroller's internal service has the expected ports") + ing1ServiceName := controller.InternalIngressControllerServiceName(ing1) + ing1Service := &corev1.Service{} + if err := kclient.Get(context.TODO(), ing1ServiceName, ing1Service); err != nil { + t.Fatal(err) + } + expectedInternalServicePorts := []corev1.ServicePort{{ + Name: "http", + Protocol: "TCP", + Port: int32(80), + TargetPort: intstr.FromString("http"), + }, { + Name: "https", + Protocol: "TCP", + Port: int32(443), + TargetPort: intstr.FromString("https"), + }, { + Name: "metrics", + Protocol: "TCP", + Port: int32(1936), + TargetPort: intstr.FromString("metrics"), + }} + assert.Equal(t, expectedInternalServicePorts, ing1Service.Spec.Ports) + + t.Log("creating a second ingresscontroller on the same node but with different port bindings") name2 := types.NamespacedName{Namespace: operatorNamespace, Name: "samehost"} strategy := &operatorv1.HostNetworkStrategy{ HTTPPort: 9080, HTTPSPort: 9443, StatsPort: 9936, } - // take the node placement of the first router + // Take the node name of the first ingresscontroller and configure a + // node selector on the second ingresscontroller to force the router + // pods all to land on the same node host. placement := &operatorv1.NodePlacement{ Tolerations: pod1.Spec.Tolerations, NodeSelector: &metav1.LabelSelector{ @@ -1028,15 +1056,16 @@ func TestHostNetworkPortBinding(t *testing.T) { if err := kclient.Create(context.TODO(), ing2); err != nil { t.Fatalf("failed to create the second ingresscontroller: %v", err) } - defer assertIngressControllerDeleted(t, kclient, ing2) + t.Cleanup(func() { assertIngressControllerDeleted(t, kclient, ing2) }) err = waitForIngressControllerCondition(t, kclient, 5*time.Minute, name2, availableConditionsForIngressControllerWithHostNetwork...) if err != nil { t.Errorf("failed to observe expected conditions for the second ingresscontroller: %v", err) } + t.Log("getting pod list for the second ingresscontroller") if err := kclient.List(context.TODO(), pods, client.InNamespace(operandNamespace)); err != nil { - t.Fatalf("failed to list the first ingresscontroller's PODs: %v", err) + t.Fatalf("failed to list the first ingresscontroller's pods: %v", err) } var pod2 *corev1.Pod for _, p := range pods.Items { @@ -1046,7 +1075,7 @@ func TestHostNetworkPortBinding(t *testing.T) { } } if pod2 == nil { - t.Fatalf("failed to find the second ingresscontroller's POD") + t.Fatalf("failed to find the second ingresscontroller's pod") } routerContainer = pod2.Spec.Containers[0] assertContainerHasPort(t, routerContainer, "http", 9080) @@ -1055,6 +1084,14 @@ func TestHostNetworkPortBinding(t *testing.T) { if !pod2.Spec.HostNetwork { t.Errorf("pod %s is not running on the host's network", pod2.Name) } + + t.Log("verifying that the second ingresscontroller's internal service has the expected ports") + ing2ServiceName := controller.InternalIngressControllerServiceName(ing2) + ing2Service := &corev1.Service{} + if err := kclient.Get(context.TODO(), ing2ServiceName, ing2Service); err != nil { + t.Fatal(err) + } + assert.Equal(t, expectedInternalServicePorts, ing2Service.Spec.Ports) } func assertContainerHasPort(t *testing.T, container corev1.Container, name string, port int32) { @@ -4179,3 +4216,63 @@ u3YLAbyW/lHhOCiZu2iAI8AbmXem9lW6Tr7p/97s0w== return secret, cl.Create(context.TODO(), secret) } + +// TestReconcileInternalService verifies that the operator reverts changes to +// the router-internal service. +func TestReconcileInternalService(t *testing.T) { + t.Parallel() + icName := types.NamespacedName{ + Namespace: operatorNamespace, + Name: "reconcile-internal-service", + } + domain := icName.Name + "." + dnsConfig.Spec.BaseDomain + ic := newPrivateController(icName, domain) + if err := kclient.Create(context.TODO(), ic); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { assertIngressControllerDeleted(t, kclient, ic) }) + + if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, icName, availableConditionsForPrivateIngressController...); err != nil { + t.Fatalf("failed to observe expected conditions: %v", err) + } + + serviceName := controller.InternalIngressControllerServiceName(ic) + service := &corev1.Service{} + if err := kclient.Get(context.TODO(), serviceName, service); err != nil { + t.Fatal(err) + } + + findPort := func(name string, service *corev1.Service) *corev1.ServicePort { + for i := range service.Spec.Ports { + if service.Spec.Ports[i].Name == name { + return &service.Spec.Ports[i] + } + } + return nil + } + + metricsPort := findPort("metrics", service) + if metricsPort == nil { + t.Fatalf(`"metrics" port was not found among the service's ports: %v`, service.Spec.Ports) + } + originalMetricsPortTargetPort := metricsPort.TargetPort + metricsPort.TargetPort = intstr.FromInt(12345) + if err := kclient.Update(context.TODO(), service); err != nil { + t.Fatal(err) + } + + if err := wait.PollImmediate(1*time.Second, time.Minute, func() (bool, error) { + if err := kclient.Get(context.TODO(), serviceName, service); err != nil { + t.Log(err) + return false, nil + } + if metricsPort := findPort("metrics", service); metricsPort == nil { + t.Fatalf(`"metrics" port was removed from the service's ports: %v`, service.Spec.Ports) + } else if metricsPort.TargetPort == originalMetricsPortTargetPort { + return true, nil + } + return false, nil + }); err != nil { + t.Errorf("failed to observe expected conditions: %v", err) + } +}