From f82cbf61bcfddcf65046115d8bf61e2e0afc6569 Mon Sep 17 00:00:00 2001 From: Luca Miccini Date: Fri, 3 Apr 2026 09:01:56 +0200 Subject: [PATCH] Fix reconcile loops caused by server-defaulted fields in CreateOrPatch Service, Deployment, DaemonSet, and StatefulSet CreateOrPatch functions were overwriting entire specs/templates, stripping Kubernetes server-defaulted fields (e.g. TargetPort, ImagePullPolicy, SessionAffinity). This caused controllerutil.CreateOrPatch to detect a diff on every reconcile, creating infinite reconcile loops. - Add shared pod.MergeContainersByName to preserve container-level server defaults (ImagePullPolicy, TerminationMessagePath, TerminationMessagePolicy) across Deployment, DaemonSet, StatefulSet - Add service.MergeServicePorts to preserve port-level server defaults (TargetPort, Protocol) - Change Service CreateOrPatch to copy only operator-controlled fields, keeping the existing spec as base so server-defaulted fields are preserved implicitly - Deprecate statefulset.MergeContainersByName in favor of the shared pod.MergeContainersByName Co-Authored-By: Claude Opus 4.6 --- modules/common/daemonset/daemonset.go | 21 ++ modules/common/deployment/deployment.go | 21 ++ modules/common/pod/merge.go | 62 +++++ modules/common/pod/merge_test.go | 337 ++++++++++++++++++++++++ modules/common/service/merge.go | 58 ++++ modules/common/service/merge_test.go | 270 +++++++++++++++++++ modules/common/service/service.go | 23 +- modules/common/statefulset/container.go | 34 +-- 8 files changed, 796 insertions(+), 30 deletions(-) create mode 100644 modules/common/pod/merge.go create mode 100644 modules/common/pod/merge_test.go create mode 100644 modules/common/service/merge.go create mode 100644 modules/common/service/merge_test.go diff --git a/modules/common/daemonset/daemonset.go b/modules/common/daemonset/daemonset.go index 8b5421b2..42ecefcc 100644 --- a/modules/common/daemonset/daemonset.go +++ b/modules/common/daemonset/daemonset.go @@ -23,6 +23,7 @@ import ( "time" "github.com/openstack-k8s-operators/lib-common/modules/common/helper" + "github.com/openstack-k8s-operators/lib-common/modules/common/pod" "github.com/openstack-k8s-operators/lib-common/modules/common/util" appsv1 "k8s.io/api/apps/v1" k8s_errors "k8s.io/apimachinery/pkg/api/errors" @@ -63,9 +64,29 @@ func (d *DaemonSet) CreateOrPatch( } daemonset.Annotations = util.MergeStringMaps(daemonset.Annotations, d.daemonset.Annotations) daemonset.Labels = util.MergeStringMaps(daemonset.Labels, d.daemonset.Labels) + + // Save existing containers before overwriting the Template so we + // can merge them below to preserve server-defaulted fields. + existingContainers := daemonset.Spec.Template.Spec.Containers + existingInitContainers := daemonset.Spec.Template.Spec.InitContainers + daemonset.Spec.Template = d.daemonset.Spec.Template daemonset.Spec.UpdateStrategy = d.daemonset.Spec.UpdateStrategy + // Merge containers by name to preserve server-defaulted fields + // (e.g. TerminationMessagePath, ImagePullPolicy) and avoid + // unnecessary reconcile loops. + daemonset.Spec.Template.Spec.Containers = existingContainers + pod.MergeContainersByName( + &daemonset.Spec.Template.Spec.Containers, + d.daemonset.Spec.Template.Spec.Containers, + ) + daemonset.Spec.Template.Spec.InitContainers = existingInitContainers + pod.MergeContainersByName( + &daemonset.Spec.Template.Spec.InitContainers, + d.daemonset.Spec.Template.Spec.InitContainers, + ) + err := controllerutil.SetControllerReference(h.GetBeforeObject(), daemonset, h.GetScheme()) if err != nil { return err diff --git a/modules/common/deployment/deployment.go b/modules/common/deployment/deployment.go index 5fa3baa8..0d53d41d 100644 --- a/modules/common/deployment/deployment.go +++ b/modules/common/deployment/deployment.go @@ -23,6 +23,7 @@ import ( "time" "github.com/openstack-k8s-operators/lib-common/modules/common/helper" + "github.com/openstack-k8s-operators/lib-common/modules/common/pod" "github.com/openstack-k8s-operators/lib-common/modules/common/util" appsv1 "k8s.io/api/apps/v1" k8s_errors "k8s.io/apimachinery/pkg/api/errors" @@ -63,10 +64,30 @@ func (d *Deployment) CreateOrPatch( } deployment.Annotations = util.MergeStringMaps(deployment.Annotations, d.deployment.Annotations) deployment.Labels = util.MergeStringMaps(deployment.Labels, d.deployment.Labels) + + // Save existing containers before overwriting the Template so we + // can merge them below to preserve server-defaulted fields. + existingContainers := deployment.Spec.Template.Spec.Containers + existingInitContainers := deployment.Spec.Template.Spec.InitContainers + deployment.Spec.Template = d.deployment.Spec.Template deployment.Spec.Replicas = d.deployment.Spec.Replicas deployment.Spec.Strategy = d.deployment.Spec.Strategy + // Merge containers by name to preserve server-defaulted fields + // (e.g. TerminationMessagePath, ImagePullPolicy) and avoid + // unnecessary reconcile loops. + deployment.Spec.Template.Spec.Containers = existingContainers + pod.MergeContainersByName( + &deployment.Spec.Template.Spec.Containers, + d.deployment.Spec.Template.Spec.Containers, + ) + deployment.Spec.Template.Spec.InitContainers = existingInitContainers + pod.MergeContainersByName( + &deployment.Spec.Template.Spec.InitContainers, + d.deployment.Spec.Template.Spec.InitContainers, + ) + err := controllerutil.SetControllerReference(h.GetBeforeObject(), deployment, h.GetScheme()) if err != nil { return err diff --git a/modules/common/pod/merge.go b/modules/common/pod/merge.go new file mode 100644 index 00000000..17f2b99e --- /dev/null +++ b/modules/common/pod/merge.go @@ -0,0 +1,62 @@ +/* +Copyright 2026 Red Hat + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package pod + +import ( + corev1 "k8s.io/api/core/v1" +) + +// MergeContainersByName merges desired container specs into existing containers +// matched by name. It starts from the desired container and preserves only the +// server-defaulted fields (TerminationMessagePath, TerminationMessagePolicy, +// ImagePullPolicy) from the existing container. All other fields come from the +// desired spec, which ensures that new fields added in future Kubernetes +// versions are not silently dropped. +// +// When container counts differ or a desired container name is not found in +// existing, the existing slice is replaced with the desired containers. +func MergeContainersByName(existing *[]corev1.Container, desired []corev1.Container) { + if len(*existing) != len(desired) { + *existing = desired + return + } + + existingByName := make(map[string]int, len(*existing)) + for i := range *existing { + existingByName[(*existing)[i].Name] = i + } + + for _, d := range desired { + idx, ok := existingByName[d.Name] + if !ok { + *existing = desired + return + } + // Preserve server-defaulted fields from the existing container + // only when the desired spec doesn't explicitly set them. + if d.ImagePullPolicy == "" { + d.ImagePullPolicy = (*existing)[idx].ImagePullPolicy + } + if d.TerminationMessagePath == "" { + d.TerminationMessagePath = (*existing)[idx].TerminationMessagePath + } + if d.TerminationMessagePolicy == "" { + d.TerminationMessagePolicy = (*existing)[idx].TerminationMessagePolicy + } + (*existing)[idx] = d + } +} diff --git a/modules/common/pod/merge_test.go b/modules/common/pod/merge_test.go new file mode 100644 index 00000000..021d8eef --- /dev/null +++ b/modules/common/pod/merge_test.go @@ -0,0 +1,337 @@ +/* +Copyright 2026 Red Hat + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package pod + +import ( + "testing" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" +) + +func TestMergeContainersByName(t *testing.T) { + tests := []struct { + name string + existing []corev1.Container + desired []corev1.Container + verify func(t *testing.T, containers []corev1.Container) + }{ + { + name: "successful merge preserves server defaults", + existing: []corev1.Container{ + { + Name: "app", + Image: "old-image:v1", + TerminationMessagePath: "/dev/termination-log", + TerminationMessagePolicy: corev1.TerminationMessageReadFile, + ImagePullPolicy: corev1.PullIfNotPresent, + }, + }, + desired: []corev1.Container{ + { + Name: "app", + Image: "new-image:v2", + Env: []corev1.EnvVar{ + {Name: "FOO", Value: "bar"}, + }, + }, + }, + verify: func(t *testing.T, containers []corev1.Container) { + c := containers[0] + if c.Image != "new-image:v2" { + t.Errorf("Image = %q, want %q", c.Image, "new-image:v2") + } + if len(c.Env) != 1 || c.Env[0].Name != "FOO" { + t.Errorf("Env not merged correctly") + } + // Server defaults should be preserved + if c.TerminationMessagePath != "/dev/termination-log" { + t.Errorf("TerminationMessagePath lost: %q", c.TerminationMessagePath) + } + if c.TerminationMessagePolicy != corev1.TerminationMessageReadFile { + t.Errorf("TerminationMessagePolicy lost: %v", c.TerminationMessagePolicy) + } + if c.ImagePullPolicy != corev1.PullIfNotPresent { + t.Errorf("ImagePullPolicy lost: %v", c.ImagePullPolicy) + } + }, + }, + { + name: "multi-container merge by name not order", + existing: []corev1.Container{ + {Name: "sidecar", Image: "sidecar:v1", ImagePullPolicy: corev1.PullAlways}, + {Name: "main", Image: "main:v1", ImagePullPolicy: corev1.PullIfNotPresent}, + }, + desired: []corev1.Container{ + {Name: "main", Image: "main:v2"}, + {Name: "sidecar", Image: "sidecar:v2"}, + }, + verify: func(t *testing.T, containers []corev1.Container) { + // Order should be preserved (existing order) + if containers[0].Name != "sidecar" || containers[0].Image != "sidecar:v2" { + t.Errorf("sidecar not merged: %+v", containers[0]) + } + if containers[1].Name != "main" || containers[1].Image != "main:v2" { + t.Errorf("main not merged: %+v", containers[1]) + } + // ImagePullPolicy preserved + if containers[0].ImagePullPolicy != corev1.PullAlways { + t.Errorf("sidecar ImagePullPolicy lost") + } + if containers[1].ImagePullPolicy != corev1.PullIfNotPresent { + t.Errorf("main ImagePullPolicy lost") + } + }, + }, + { + name: "merges all operator-controlled fields", + existing: []corev1.Container{ + { + Name: "app", + Image: "old:v1", + ImagePullPolicy: corev1.PullAlways, + }, + }, + desired: []corev1.Container{ + { + Name: "app", + Image: "new:v2", + Command: []string{"/bin/sh"}, + Args: []string{"-c", "echo"}, + Env: []corev1.EnvVar{{Name: "K", Value: "V"}}, + Ports: []corev1.ContainerPort{{ContainerPort: 8080}}, + VolumeMounts: []corev1.VolumeMount{ + {Name: "data", MountPath: "/data"}, + }, + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + }, + }, + LivenessProbe: &corev1.Probe{InitialDelaySeconds: 5}, + ReadinessProbe: &corev1.Probe{InitialDelaySeconds: 10}, + Lifecycle: &corev1.Lifecycle{}, + SecurityContext: &corev1.SecurityContext{}, + }, + }, + verify: func(t *testing.T, containers []corev1.Container) { + c := containers[0] + if c.Image != "new:v2" { + t.Errorf("Image not merged") + } + if len(c.Command) != 1 || c.Command[0] != "/bin/sh" { + t.Errorf("Command not merged") + } + if len(c.Args) != 2 { + t.Errorf("Args not merged") + } + if len(c.Env) != 1 { + t.Errorf("Env not merged") + } + if len(c.Ports) != 1 { + t.Errorf("Ports not merged") + } + if len(c.VolumeMounts) != 1 { + t.Errorf("VolumeMounts not merged") + } + if c.Resources.Limits == nil { + t.Errorf("Resources not merged") + } + if c.LivenessProbe == nil { + t.Errorf("LivenessProbe not merged") + } + if c.ReadinessProbe == nil { + t.Errorf("ReadinessProbe not merged") + } + if c.Lifecycle == nil { + t.Errorf("Lifecycle not merged") + } + if c.SecurityContext == nil { + t.Errorf("SecurityContext not merged") + } + // Server default preserved + if c.ImagePullPolicy != corev1.PullAlways { + t.Errorf("ImagePullPolicy lost") + } + }, + }, + { + name: "desired fields override existing for StartupProbe, WorkingDir, EnvFrom", + existing: []corev1.Container{ + { + Name: "app", + Image: "old:v1", + ImagePullPolicy: corev1.PullAlways, + StartupProbe: &corev1.Probe{InitialDelaySeconds: 15}, + WorkingDir: "/old/dir", + EnvFrom: []corev1.EnvFromSource{ + {Prefix: "OLD_"}, + }, + }, + }, + desired: []corev1.Container{ + { + Name: "app", + Image: "new:v2", + StartupProbe: &corev1.Probe{InitialDelaySeconds: 30}, + WorkingDir: "/new/dir", + EnvFrom: []corev1.EnvFromSource{ + {Prefix: "NEW_"}, + }, + }, + }, + verify: func(t *testing.T, containers []corev1.Container) { + c := containers[0] + if c.Image != "new:v2" { + t.Errorf("Image not merged") + } + if c.StartupProbe == nil || c.StartupProbe.InitialDelaySeconds != 30 { + t.Errorf("StartupProbe should come from desired, got %v", c.StartupProbe) + } + if c.WorkingDir != "/new/dir" { + t.Errorf("WorkingDir should come from desired, got %q", c.WorkingDir) + } + if len(c.EnvFrom) != 1 || c.EnvFrom[0].Prefix != "NEW_" { + t.Errorf("EnvFrom should come from desired, got %v", c.EnvFrom) + } + if c.ImagePullPolicy != corev1.PullAlways { + t.Errorf("ImagePullPolicy should be preserved from existing") + } + }, + }, + { + name: "desired without optional fields clears them from existing", + existing: []corev1.Container{ + { + Name: "app", + Image: "old:v1", + ImagePullPolicy: corev1.PullAlways, + StartupProbe: &corev1.Probe{InitialDelaySeconds: 15}, + WorkingDir: "/old/dir", + EnvFrom: []corev1.EnvFromSource{{Prefix: "OLD_"}}, + VolumeDevices: []corev1.VolumeDevice{{Name: "dev", DevicePath: "/dev/xvda"}}, + }, + }, + desired: []corev1.Container{ + { + Name: "app", + Image: "new:v2", + }, + }, + verify: func(t *testing.T, containers []corev1.Container) { + c := containers[0] + if c.StartupProbe != nil { + t.Errorf("StartupProbe should be nil when not in desired") + } + if c.WorkingDir != "" { + t.Errorf("WorkingDir should be empty when not in desired") + } + if c.EnvFrom != nil { + t.Errorf("EnvFrom should be nil when not in desired") + } + if c.VolumeDevices != nil { + t.Errorf("VolumeDevices should be nil when not in desired") + } + if c.ImagePullPolicy != corev1.PullAlways { + t.Errorf("ImagePullPolicy should be preserved from existing") + } + }, + }, + { + name: "count mismatch falls back to replacement", + existing: []corev1.Container{ + {Name: "app", Image: "old:v1"}, + }, + desired: []corev1.Container{ + {Name: "app", Image: "new:v2"}, + {Name: "sidecar", Image: "sidecar:v1"}, + }, + verify: func(t *testing.T, containers []corev1.Container) { + if len(containers) != 2 { + t.Errorf("expected 2 containers after replacement, got %d", len(containers)) + } + if containers[0].Name != "app" || containers[0].Image != "new:v2" { + t.Errorf("first container not replaced correctly: %+v", containers[0]) + } + if containers[1].Name != "sidecar" || containers[1].Image != "sidecar:v1" { + t.Errorf("second container not replaced correctly: %+v", containers[1]) + } + }, + }, + { + name: "name mismatch falls back to replacement", + existing: []corev1.Container{ + {Name: "app", Image: "old:v1"}, + }, + desired: []corev1.Container{ + {Name: "different", Image: "new:v2"}, + }, + verify: func(t *testing.T, containers []corev1.Container) { + if len(containers) != 1 || containers[0].Name != "different" { + t.Errorf("expected replacement with desired, got %+v", containers) + } + }, + }, + { + name: "desired explicit server-default fields are honored", + existing: []corev1.Container{ + { + Name: "app", + Image: "old:v1", + ImagePullPolicy: corev1.PullIfNotPresent, + TerminationMessagePath: "/dev/termination-log", + TerminationMessagePolicy: corev1.TerminationMessageReadFile, + }, + }, + desired: []corev1.Container{ + { + Name: "app", + Image: "new:v2", + ImagePullPolicy: corev1.PullAlways, + TerminationMessagePath: "/custom/path", + TerminationMessagePolicy: corev1.TerminationMessageFallbackToLogsOnError, + }, + }, + verify: func(t *testing.T, containers []corev1.Container) { + c := containers[0] + if c.ImagePullPolicy != corev1.PullAlways { + t.Errorf("ImagePullPolicy = %v, want PullAlways (from desired)", c.ImagePullPolicy) + } + if c.TerminationMessagePath != "/custom/path" { + t.Errorf("TerminationMessagePath = %q, want /custom/path (from desired)", c.TerminationMessagePath) + } + if c.TerminationMessagePolicy != corev1.TerminationMessageFallbackToLogsOnError { + t.Errorf("TerminationMessagePolicy = %v, want FallbackToLogsOnError (from desired)", c.TerminationMessagePolicy) + } + }, + }, + { + name: "empty slices succeed", + existing: []corev1.Container{}, + desired: []corev1.Container{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + MergeContainersByName(&tt.existing, tt.desired) + if tt.verify != nil { + tt.verify(t, tt.existing) + } + }) + } +} diff --git a/modules/common/service/merge.go b/modules/common/service/merge.go new file mode 100644 index 00000000..d6ead639 --- /dev/null +++ b/modules/common/service/merge.go @@ -0,0 +1,58 @@ +/* +Copyright 2026 Red Hat + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package service + +import ( + corev1 "k8s.io/api/core/v1" +) + +// MergeServicePorts merges desired service port specs into existing ports +// matched by name. It starts from the desired port and preserves only the +// server-defaulted fields (Protocol, TargetPort) from the existing port when +// the desired spec doesn't explicitly set them. All other fields come from the +// desired spec. +// +// When port counts differ or a desired port name is not found in existing, the +// existing slice is replaced with the desired ports. +func MergeServicePorts(existing *[]corev1.ServicePort, desired []corev1.ServicePort) { + if len(*existing) != len(desired) { + *existing = desired + return + } + + existingByName := make(map[string]int, len(*existing)) + for i := range *existing { + existingByName[(*existing)[i].Name] = i + } + + for _, d := range desired { + idx, ok := existingByName[d.Name] + if !ok { + *existing = desired + return + } + // Preserve server-defaulted fields from the existing port + // only when the desired spec doesn't explicitly set them. + if d.Protocol == "" { + d.Protocol = (*existing)[idx].Protocol + } + if d.TargetPort.IntValue() == 0 && d.TargetPort.StrVal == "" { + d.TargetPort = (*existing)[idx].TargetPort + } + (*existing)[idx] = d + } +} diff --git a/modules/common/service/merge_test.go b/modules/common/service/merge_test.go new file mode 100644 index 00000000..5f314a08 --- /dev/null +++ b/modules/common/service/merge_test.go @@ -0,0 +1,270 @@ +/* +Copyright 2026 Red Hat + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package service + +import ( + "testing" + + . "github.com/onsi/gomega" // nolint:revive + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/intstr" +) + +func TestMergeServicePorts(t *testing.T) { + tests := []struct { + name string + existing []corev1.ServicePort + desired []corev1.ServicePort + want []corev1.ServicePort + }{ + { + name: "Preserves TargetPort from existing when desired omits it", + existing: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + Protocol: corev1.ProtocolTCP, + TargetPort: intstr.FromInt(80), + }, + }, + desired: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + }, + }, + want: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + Protocol: corev1.ProtocolTCP, + TargetPort: intstr.FromInt(80), + }, + }, + }, + { + name: "Preserves Protocol from existing when desired omits it", + existing: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + Protocol: corev1.ProtocolTCP, + TargetPort: intstr.FromInt(80), + }, + }, + desired: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + TargetPort: intstr.FromInt(8080), + }, + }, + want: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + Protocol: corev1.ProtocolTCP, + TargetPort: intstr.FromInt(8080), + }, + }, + }, + { + name: "Desired values take precedence when explicitly set", + existing: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + Protocol: corev1.ProtocolTCP, + TargetPort: intstr.FromInt(80), + }, + }, + desired: []corev1.ServicePort{ + { + Name: "http", + Port: 8080, + Protocol: corev1.ProtocolUDP, + TargetPort: intstr.FromInt(9090), + }, + }, + want: []corev1.ServicePort{ + { + Name: "http", + Port: 8080, + Protocol: corev1.ProtocolUDP, + TargetPort: intstr.FromInt(9090), + }, + }, + }, + { + name: "Multiple ports matched by name", + existing: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + Protocol: corev1.ProtocolTCP, + TargetPort: intstr.FromInt(80), + }, + { + Name: "https", + Port: 443, + Protocol: corev1.ProtocolTCP, + TargetPort: intstr.FromInt(443), + }, + }, + desired: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + Protocol: corev1.ProtocolTCP, + }, + { + Name: "https", + Port: 443, + Protocol: corev1.ProtocolTCP, + }, + }, + want: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + Protocol: corev1.ProtocolTCP, + TargetPort: intstr.FromInt(80), + }, + { + Name: "https", + Port: 443, + Protocol: corev1.ProtocolTCP, + TargetPort: intstr.FromInt(443), + }, + }, + }, + { + name: "Different port counts replaces entirely", + existing: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + Protocol: corev1.ProtocolTCP, + TargetPort: intstr.FromInt(80), + }, + }, + desired: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + Protocol: corev1.ProtocolTCP, + }, + { + Name: "https", + Port: 443, + Protocol: corev1.ProtocolTCP, + }, + }, + want: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + Protocol: corev1.ProtocolTCP, + }, + { + Name: "https", + Port: 443, + Protocol: corev1.ProtocolTCP, + }, + }, + }, + { + name: "Unmatched port name replaces entirely", + existing: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + Protocol: corev1.ProtocolTCP, + TargetPort: intstr.FromInt(80), + }, + }, + desired: []corev1.ServicePort{ + { + Name: "web", + Port: 80, + Protocol: corev1.ProtocolTCP, + }, + }, + want: []corev1.ServicePort{ + { + Name: "web", + Port: 80, + Protocol: corev1.ProtocolTCP, + }, + }, + }, + { + name: "TargetPort with string value is preserved from desired", + existing: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + Protocol: corev1.ProtocolTCP, + TargetPort: intstr.FromInt(80), + }, + }, + desired: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + TargetPort: intstr.FromString("http"), + }, + }, + want: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + Protocol: corev1.ProtocolTCP, + TargetPort: intstr.FromString("http"), + }, + }, + }, + { + name: "Empty existing replaced by desired", + existing: []corev1.ServicePort{}, + desired: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + Protocol: corev1.ProtocolTCP, + }, + }, + want: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + Protocol: corev1.ProtocolTCP, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + existing := make([]corev1.ServicePort, len(tt.existing)) + copy(existing, tt.existing) + MergeServicePorts(&existing, tt.desired) + g.Expect(existing).To(Equal(tt.want)) + }) + } +} diff --git a/modules/common/service/service.go b/modules/common/service/service.go index d6d4541b..88c78d5a 100644 --- a/modules/common/service/service.go +++ b/modules/common/service/service.go @@ -310,7 +310,28 @@ func (s *Service) CreateOrPatch( op, err := controllerutil.CreateOrPatch(ctx, h.GetClient(), service, func() error { service.Labels = util.MergeStringMaps(s.service.Labels, service.Labels) service.Annotations = util.MergeStringMaps(s.service.Annotations, service.Annotations) - service.Spec = s.service.Spec + + // Copy only operator-controlled fields from the desired spec. + // The existing spec is kept as the base so that + // server-defaulted fields (e.g. SessionAffinity, + // IPFamilyPolicy, InternalTrafficPolicy, ClusterIPs) are + // preserved automatically, avoiding unnecessary reconcile + // loops. If Kubernetes adds new defaulted fields in the + // future, they are preserved without code changes here. + service.Spec.Selector = s.service.Spec.Selector + service.Spec.Type = s.service.Spec.Type + service.Spec.ClusterIP = s.service.Spec.ClusterIP + service.Spec.PublishNotReadyAddresses = s.service.Spec.PublishNotReadyAddresses + service.Spec.ExternalTrafficPolicy = s.service.Spec.ExternalTrafficPolicy + service.Spec.LoadBalancerClass = s.service.Spec.LoadBalancerClass + service.Spec.LoadBalancerSourceRanges = s.service.Spec.LoadBalancerSourceRanges + service.Spec.AllocateLoadBalancerNodePorts = s.service.Spec.AllocateLoadBalancerNodePorts + + // Merge ports by name to preserve server-defaulted fields + // (e.g. TargetPort, Protocol) and avoid unnecessary reconcile + // loops. Falls back to full replacement if port sets don't + // match by name. + MergeServicePorts(&service.Spec.Ports, s.service.Spec.Ports) err := controllerutil.SetControllerReference(h.GetBeforeObject(), service, h.GetScheme()) if err != nil { diff --git a/modules/common/statefulset/container.go b/modules/common/statefulset/container.go index 86db4418..bc06affe 100644 --- a/modules/common/statefulset/container.go +++ b/modules/common/statefulset/container.go @@ -17,6 +17,7 @@ limitations under the License. package statefulset import ( + "github.com/openstack-k8s-operators/lib-common/modules/common/pod" corev1 "k8s.io/api/core/v1" ) @@ -29,34 +30,9 @@ import ( // // When container counts differ or a desired container name is not found in // existing, the existing slice is replaced with the desired containers. +// +// Deprecated: Use pod.MergeContainersByName instead. This function is kept +// for backward compatibility. func MergeContainersByName(existing *[]corev1.Container, desired []corev1.Container) { - if len(*existing) != len(desired) { - *existing = desired - return - } - - existingByName := make(map[string]int, len(*existing)) - for i := range *existing { - existingByName[(*existing)[i].Name] = i - } - - for _, d := range desired { - idx, ok := existingByName[d.Name] - if !ok { - *existing = desired - return - } - // Preserve server-defaulted fields from the existing container - // only when the desired spec doesn't explicitly set them. - if d.ImagePullPolicy == "" { - d.ImagePullPolicy = (*existing)[idx].ImagePullPolicy - } - if d.TerminationMessagePath == "" { - d.TerminationMessagePath = (*existing)[idx].TerminationMessagePath - } - if d.TerminationMessagePolicy == "" { - d.TerminationMessagePolicy = (*existing)[idx].TerminationMessagePolicy - } - (*existing)[idx] = d - } + pod.MergeContainersByName(existing, desired) }