From 5ebd59b41a82180d6e19f8e88eb9c507dac0f9e4 Mon Sep 17 00:00:00 2001 From: Razvan Dobre Date: Tue, 4 Nov 2025 13:43:52 +0200 Subject: [PATCH 1/7] Comply with mutating webhook --- controllers/kafkacluster_controller.go | 21 +-- pkg/k8sutil/patch_options.go | 175 +++++++++++++++++++++++++ pkg/k8sutil/resource.go | 5 +- pkg/resources/kafka/kafka.go | 5 +- 4 files changed, 195 insertions(+), 11 deletions(-) create mode 100644 pkg/k8sutil/patch_options.go diff --git a/controllers/kafkacluster_controller.go b/controllers/kafkacluster_controller.go index 62341e1fa..4eaeccfaa 100644 --- a/controllers/kafkacluster_controller.go +++ b/controllers/kafkacluster_controller.go @@ -385,15 +385,18 @@ func SetupKafkaClusterWithManager(mgr ctrl.Manager) *ctrl.Builder { } return false }, - UpdateFunc: func(e event.UpdateEvent) bool { - switch newObj := e.ObjectNew.(type) { - case *corev1.Pod, *corev1.ConfigMap, *corev1.PersistentVolumeClaim: - patchResult, err := patch.DefaultPatchMaker.Calculate(e.ObjectOld, e.ObjectNew) - if err != nil { - log.Error(err, "could not match objects", "kind", e.ObjectOld.GetObjectKind()) - } else if patchResult.IsEmpty() { - return false - } + UpdateFunc: func(e event.UpdateEvent) bool { + switch newObj := e.ObjectNew.(type) { + case *corev1.Pod, *corev1.ConfigMap, *corev1.PersistentVolumeClaim: + opts := []patch.CalculateOption{ + k8sutil.IgnoreMutationWebhookFields(), + } + patchResult, err := patch.DefaultPatchMaker.Calculate(e.ObjectOld, e.ObjectNew, opts...) + if err != nil { + log.Error(err, "could not match objects", "kind", e.ObjectOld.GetObjectKind()) + } else if patchResult.IsEmpty() { + return false + } case *v1beta1.KafkaCluster: oldObj := e.ObjectOld.(*v1beta1.KafkaCluster) if !reflect.DeepEqual(oldObj.Spec, newObj.Spec) || diff --git a/pkg/k8sutil/patch_options.go b/pkg/k8sutil/patch_options.go new file mode 100644 index 000000000..46366bbe6 --- /dev/null +++ b/pkg/k8sutil/patch_options.go @@ -0,0 +1,175 @@ +// Copyright © 2019 Cisco Systems, Inc. and/or its affiliates +// +// 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 k8sutil + +import ( + "emperror.dev/errors" + json "github.com/json-iterator/go" + corev1 "k8s.io/api/core/v1" + + "github.com/banzaicloud/k8s-objectmatcher/patch" +) + +// IgnoreMutationWebhookFields creates a CalculateOption that ignores fields commonly +// modified by mutation webhooks like Gatekeeper, OPA, and Pod Security Policies +func IgnoreMutationWebhookFields() patch.CalculateOption { + return func(current, modified []byte) ([]byte, []byte, error) { + currentPod := &corev1.Pod{} + if err := json.Unmarshal(current, currentPod); err != nil { + // Not a pod, return unchanged + return current, modified, nil + } + + modifiedPod := &corev1.Pod{} + if err := json.Unmarshal(modified, modifiedPod); err != nil { + return current, modified, nil + } + + // Remove fields that mutation webhooks commonly modify + currentPod = cleanMutationWebhookFields(currentPod) + modifiedPod = cleanMutationWebhookFields(modifiedPod) + + currentBytes, err := json.Marshal(currentPod) + if err != nil { + return []byte{}, []byte{}, errors.Wrap(err, "could not marshal cleaned current pod") + } + + modifiedBytes, err := json.Marshal(modifiedPod) + if err != nil { + return []byte{}, []byte{}, errors.Wrap(err, "could not marshal cleaned modified pod") + } + + return currentBytes, modifiedBytes, nil + } +} + +func cleanMutationWebhookFields(pod *corev1.Pod) *corev1.Pod { + // Create a copy to avoid modifying the original + cleaned := pod.DeepCopy() + + // Remove mutation webhook annotations that should not trigger reconciliation + if cleaned.Annotations != nil { + delete(cleaned.Annotations, "gatekeeper.sh/mutation-id") + delete(cleaned.Annotations, "gatekeeper.sh/mutations") + } + + // Clean security context fields commonly set by PSPs/Gatekeeper + for i := range cleaned.Spec.InitContainers { + cleanSecurityContext(&cleaned.Spec.InitContainers[i]) + } + for i := range cleaned.Spec.Containers { + cleanSecurityContext(&cleaned.Spec.Containers[i]) + } + + return cleaned +} + +func cleanSecurityContext(container *corev1.Container) { + if container.SecurityContext == nil { + return + } + + // Note: We intentionally do NOT clean security context fields here by default + // because those are typically important security controls that should be reconciled. + // If you need to ignore specific security context fields, uncomment the relevant lines below: + + // AllowPrivilegeEscalation is often set by PSPs + // container.SecurityContext.AllowPrivilegeEscalation = nil + + // ReadOnlyRootFilesystem is often set by PSPs + // container.SecurityContext.ReadOnlyRootFilesystem = nil + + // Capabilities are often modified by PSPs + // container.SecurityContext.Capabilities = nil +} + +// IgnorePodResourcesIfAnnotated creates a CalculateOption that ignores pod resource +// requests/limits if the pod has specific annotations indicating it's managed by +// an external system (e.g., ScaleOps, VPA) +func IgnorePodResourcesIfAnnotated() patch.CalculateOption { + return func(current, modified []byte) ([]byte, []byte, error) { + currentMap := map[string]interface{}{} + if err := json.Unmarshal(current, ¤tMap); err != nil { + return current, modified, nil + } + + // Check if this pod should ignore resource diffs (e.g., via annotation) + if shouldIgnoreResources(currentMap) { + // Remove resources from comparison + current = removeResourcesFromPod(current) + modified = removeResourcesFromPod(modified) + } + + return current, modified, nil + } +} + +func shouldIgnoreResources(podMap map[string]interface{}) bool { + metadata, ok := podMap["metadata"].(map[string]interface{}) + if !ok { + return false + } + + annotations, ok := metadata["annotations"].(map[string]interface{}) + if !ok { + return false + } + + // Check for annotations that indicate external resource management + annotationsToCheck := []string{ + "scaleops.sh/pod-owner-grouping", + "vpa.k8s.io/updateMode", + "cluster-autoscaler.kubernetes.io/safe-to-evict-local-volumes", + } + + for _, ann := range annotationsToCheck { + if _, exists := annotations[ann]; exists { + return true + } + } + + return false +} + +func removeResourcesFromPod(podBytes []byte) []byte { + podMap := map[string]interface{}{} + if err := json.Unmarshal(podBytes, &podMap); err != nil { + return podBytes + } + + if spec, ok := podMap["spec"].(map[string]interface{}); ok { + // Remove resources from all containers + if containers, ok := spec["containers"].([]interface{}); ok { + for _, c := range containers { + if container, ok := c.(map[string]interface{}); ok { + delete(container, "resources") + } + } + } + if initContainers, ok := spec["initContainers"].([]interface{}); ok { + for _, c := range initContainers { + if container, ok := c.(map[string]interface{}); ok { + delete(container, "resources") + } + } + } + } + + result, err := json.Marshal(podMap) + if err != nil { + return podBytes + } + return result +} diff --git a/pkg/k8sutil/resource.go b/pkg/k8sutil/resource.go index b50d4228d..9117c7381 100644 --- a/pkg/k8sutil/resource.go +++ b/pkg/k8sutil/resource.go @@ -164,7 +164,10 @@ func Reconcile(log logr.Logger, client runtimeClient.Client, desired runtime.Obj // CheckIfObjectUpdated checks if the given object is updated using K8sObjectMatcher func CheckIfObjectUpdated(log logr.Logger, desiredType reflect.Type, current, desired runtime.Object) bool { - patchResult, err := patch.DefaultPatchMaker.Calculate(current, desired) + opts := []patch.CalculateOption{ + IgnoreMutationWebhookFields(), + } + patchResult, err := patch.DefaultPatchMaker.Calculate(current, desired, opts...) if err != nil { log.Error(err, "could not match objects", "kind", desiredType) return true diff --git a/pkg/resources/kafka/kafka.go b/pkg/resources/kafka/kafka.go index 72c907bcd..aa43e7228 100644 --- a/pkg/resources/kafka/kafka.go +++ b/pkg/resources/kafka/kafka.go @@ -953,7 +953,10 @@ func (r *Reconciler) handleRollingUpgrade(log logr.Logger, desiredPod, currentPo desiredPod.Spec.Tolerations = uniqueTolerations } // Check if the resource actually updated or if labels match TaintedBrokersSelector - patchResult, err := patch.DefaultPatchMaker.Calculate(currentPod, desiredPod) + opts := []patch.CalculateOption{ + k8sutil.IgnoreMutationWebhookFields(), + } + patchResult, err := patch.DefaultPatchMaker.Calculate(currentPod, desiredPod, opts...) switch { case err != nil: log.Error(err, "could not match objects", "kind", desiredType) From f6b0f59c158a69ef68b67b85de4db0009ef1501c Mon Sep 17 00:00:00 2001 From: Razvan Dobre Date: Tue, 4 Nov 2025 13:51:05 +0200 Subject: [PATCH 2/7] lint --- controllers/kafkacluster_controller.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/controllers/kafkacluster_controller.go b/controllers/kafkacluster_controller.go index 4eaeccfaa..41f05a4c2 100644 --- a/controllers/kafkacluster_controller.go +++ b/controllers/kafkacluster_controller.go @@ -385,18 +385,18 @@ func SetupKafkaClusterWithManager(mgr ctrl.Manager) *ctrl.Builder { } return false }, - UpdateFunc: func(e event.UpdateEvent) bool { - switch newObj := e.ObjectNew.(type) { - case *corev1.Pod, *corev1.ConfigMap, *corev1.PersistentVolumeClaim: - opts := []patch.CalculateOption{ - k8sutil.IgnoreMutationWebhookFields(), - } - patchResult, err := patch.DefaultPatchMaker.Calculate(e.ObjectOld, e.ObjectNew, opts...) - if err != nil { - log.Error(err, "could not match objects", "kind", e.ObjectOld.GetObjectKind()) - } else if patchResult.IsEmpty() { - return false - } + UpdateFunc: func(e event.UpdateEvent) bool { + switch newObj := e.ObjectNew.(type) { + case *corev1.Pod, *corev1.ConfigMap, *corev1.PersistentVolumeClaim: + opts := []patch.CalculateOption{ + k8sutil.IgnoreMutationWebhookFields(), + } + patchResult, err := patch.DefaultPatchMaker.Calculate(e.ObjectOld, e.ObjectNew, opts...) + if err != nil { + log.Error(err, "could not match objects", "kind", e.ObjectOld.GetObjectKind()) + } else if patchResult.IsEmpty() { + return false + } case *v1beta1.KafkaCluster: oldObj := e.ObjectOld.(*v1beta1.KafkaCluster) if !reflect.DeepEqual(oldObj.Spec, newObj.Spec) || From aa73ccb0d8b9c64d32cf4d01404eaef4be5dd308 Mon Sep 17 00:00:00 2001 From: Razvan Dobre Date: Tue, 4 Nov 2025 13:52:47 +0200 Subject: [PATCH 3/7] mod tidy --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index e17b83695..92a6a1cf1 100644 --- a/go.mod +++ b/go.mod @@ -105,7 +105,7 @@ require ( github.com/jcmturner/gofork v1.7.6 // indirect github.com/jcmturner/gokrb5/v8 v8.4.4 // indirect github.com/jcmturner/rpc/v2 v2.0.3 // indirect - github.com/json-iterator/go v1.1.12 // indirect + github.com/json-iterator/go v1.1.12 github.com/klauspost/compress v1.18.1 // indirect github.com/mattn/go-colorable v0.1.14 // indirect github.com/mattn/go-isatty v0.0.20 // indirect From aa1fb42717b9083cae59b41cfb97a335ba283a2d Mon Sep 17 00:00:00 2001 From: Razvan Dobre Date: Tue, 4 Nov 2025 14:06:11 +0200 Subject: [PATCH 4/7] Unittest --- pkg/k8sutil/patch_options_test.go | 598 ++++++++++++++++++++++++++++++ 1 file changed, 598 insertions(+) create mode 100644 pkg/k8sutil/patch_options_test.go diff --git a/pkg/k8sutil/patch_options_test.go b/pkg/k8sutil/patch_options_test.go new file mode 100644 index 000000000..0b15e3c01 --- /dev/null +++ b/pkg/k8sutil/patch_options_test.go @@ -0,0 +1,598 @@ +// Copyright © 2019 Cisco Systems, Inc. and/or its affiliates +// +// 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 k8sutil + +import ( + "testing" + + "github.com/banzaicloud/k8s-objectmatcher/patch" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestIgnoreMutationWebhookFields(t *testing.T) { + tests := []struct { + name string + currentPod *corev1.Pod + modifiedPod *corev1.Pod + expectDiff bool + description string + }{ + { + name: "ignore gatekeeper mutation annotations", + currentPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + Annotations: map[string]string{ + "gatekeeper.sh/mutation-id": "abc123", + "gatekeeper.sh/mutations": "Assign//policy1:1", + "other-annotation": "keep-this", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:latest", + }, + }, + }, + }, + modifiedPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + Annotations: map[string]string{ + "other-annotation": "keep-this", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:latest", + }, + }, + }, + }, + expectDiff: false, + description: "Gatekeeper mutation annotations should be ignored", + }, + { + name: "detect actual spec changes", + currentPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:1.0", + }, + }, + }, + }, + modifiedPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:2.0", + }, + }, + }, + }, + expectDiff: true, + description: "Real spec changes should be detected", + }, + { + name: "only gatekeeper annotations differ", + currentPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + Annotations: map[string]string{ + "gatekeeper.sh/mutation-id": "xyz789", + "app": "kafka", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:latest", + }, + }, + }, + }, + modifiedPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + Annotations: map[string]string{ + "app": "kafka", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:latest", + }, + }, + }, + }, + expectDiff: false, + description: "Only gatekeeper annotations differ, should not trigger diff", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Set last applied annotation on current pod + if err := patch.DefaultAnnotator.SetLastAppliedAnnotation(tt.currentPod); err != nil { + t.Fatalf("Failed to set last applied annotation: %v", err) + } + + opts := []patch.CalculateOption{ + IgnoreMutationWebhookFields(), + } + + patchResult, err := patch.DefaultPatchMaker.Calculate(tt.currentPod, tt.modifiedPod, opts...) + if err != nil { + t.Fatalf("Failed to calculate patch: %v", err) + } + + hasDiff := !patchResult.IsEmpty() + if hasDiff != tt.expectDiff { + t.Errorf("%s: expected diff=%v, got diff=%v\nPatch: %s", + tt.description, tt.expectDiff, hasDiff, string(patchResult.Patch)) + } + }) + } +} + +func TestIgnorePodResourcesIfAnnotated(t *testing.T) { + tests := []struct { + name string + currentPod *corev1.Pod + modifiedPod *corev1.Pod + expectDiff bool + description string + }{ + { + name: "ignore resources with scaleops annotation", + currentPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + Annotations: map[string]string{ + "scaleops.sh/pod-owner-grouping": "kafkacluster", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:latest", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("2000m"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("4000m"), + corev1.ResourceMemory: resource.MustParse("8Gi"), + }, + }, + }, + }, + }, + }, + modifiedPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + Annotations: map[string]string{ + "scaleops.sh/pod-owner-grouping": "kafkacluster", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:latest", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("1000m"), + corev1.ResourceMemory: resource.MustParse("2Gi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("2000m"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + }, + }, + }, + }, + }, + expectDiff: false, + description: "Resource differences should be ignored with scaleops annotation", + }, + { + name: "ignore resources with vpa annotation", + currentPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + Annotations: map[string]string{ + "vpa.k8s.io/updateMode": "Auto", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:latest", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("2000m"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + }, + }, + }, + }, + }, + modifiedPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + Annotations: map[string]string{ + "vpa.k8s.io/updateMode": "Auto", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:latest", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("1000m"), + corev1.ResourceMemory: resource.MustParse("2Gi"), + }, + }, + }, + }, + }, + }, + expectDiff: false, + description: "Resource differences should be ignored with VPA annotation", + }, + { + name: "detect resource changes without annotation", + currentPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:latest", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("2000m"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + }, + }, + }, + }, + }, + modifiedPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:latest", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("1000m"), + corev1.ResourceMemory: resource.MustParse("2Gi"), + }, + }, + }, + }, + }, + }, + expectDiff: true, + description: "Resource changes should be detected without external management annotation", + }, + { + name: "detect non-resource changes even with annotation", + currentPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + Annotations: map[string]string{ + "scaleops.sh/pod-owner-grouping": "kafkacluster", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:1.0", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("1000m"), + }, + }, + }, + }, + }, + }, + modifiedPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + Annotations: map[string]string{ + "scaleops.sh/pod-owner-grouping": "kafkacluster", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:2.0", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("2000m"), + }, + }, + }, + }, + }, + }, + expectDiff: true, + description: "Non-resource changes (image) should still be detected", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Set last applied annotation on current pod + if err := patch.DefaultAnnotator.SetLastAppliedAnnotation(tt.currentPod); err != nil { + t.Fatalf("Failed to set last applied annotation: %v", err) + } + + opts := []patch.CalculateOption{ + IgnorePodResourcesIfAnnotated(), + } + + patchResult, err := patch.DefaultPatchMaker.Calculate(tt.currentPod, tt.modifiedPod, opts...) + if err != nil { + t.Fatalf("Failed to calculate patch: %v", err) + } + + hasDiff := !patchResult.IsEmpty() + if hasDiff != tt.expectDiff { + t.Errorf("%s: expected diff=%v, got diff=%v\nPatch: %s", + tt.description, tt.expectDiff, hasDiff, string(patchResult.Patch)) + } + }) + } +} + +func TestCombinedIgnoreOptions(t *testing.T) { + t.Run("combine mutation webhook and resource ignoring", func(t *testing.T) { + currentPod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + Annotations: map[string]string{ + "gatekeeper.sh/mutation-id": "abc123", + "gatekeeper.sh/mutations": "Assign//policy1:1", + "scaleops.sh/pod-owner-grouping": "kafkacluster", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:latest", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("2000m"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + }, + }, + }, + }, + } + + modifiedPod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + Annotations: map[string]string{ + "scaleops.sh/pod-owner-grouping": "kafkacluster", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:latest", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("1000m"), + corev1.ResourceMemory: resource.MustParse("2Gi"), + }, + }, + }, + }, + }, + } + + if err := patch.DefaultAnnotator.SetLastAppliedAnnotation(currentPod); err != nil { + t.Fatalf("Failed to set last applied annotation: %v", err) + } + + opts := []patch.CalculateOption{ + IgnoreMutationWebhookFields(), + IgnorePodResourcesIfAnnotated(), + } + + patchResult, err := patch.DefaultPatchMaker.Calculate(currentPod, modifiedPod, opts...) + if err != nil { + t.Fatalf("Failed to calculate patch: %v", err) + } + + if !patchResult.IsEmpty() { + t.Errorf("Expected no diff when both mutation webhook annotations and resources differ, but got patch: %s", + string(patchResult.Patch)) + } + }) +} + +func TestShouldIgnoreResources(t *testing.T) { + tests := []struct { + name string + annotations map[string]string + expect bool + }{ + { + name: "scaleops annotation present", + annotations: map[string]string{ + "scaleops.sh/pod-owner-grouping": "kafkacluster", + }, + expect: true, + }, + { + name: "vpa annotation present", + annotations: map[string]string{ + "vpa.k8s.io/updateMode": "Auto", + }, + expect: true, + }, + { + name: "cluster-autoscaler annotation present", + annotations: map[string]string{ + "cluster-autoscaler.kubernetes.io/safe-to-evict-local-volumes": "data", + }, + expect: true, + }, + { + name: "no relevant annotations", + annotations: map[string]string{ + "some.other/annotation": "value", + }, + expect: false, + }, + { + name: "no annotations", + annotations: nil, + expect: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + podMap := map[string]interface{}{ + "metadata": map[string]interface{}{ + "name": "test-pod", + "namespace": "default", + }, + } + + if tt.annotations != nil { + metadata := podMap["metadata"].(map[string]interface{}) + annMap := make(map[string]interface{}) + for k, v := range tt.annotations { + annMap[k] = v + } + metadata["annotations"] = annMap + } + + result := shouldIgnoreResources(podMap) + if result != tt.expect { + t.Errorf("Expected shouldIgnoreResources=%v, got %v", tt.expect, result) + } + }) + } +} + +func TestCleanMutationWebhookFields(t *testing.T) { + t.Run("removes gatekeeper annotations", func(t *testing.T) { + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + Annotations: map[string]string{ + "gatekeeper.sh/mutation-id": "abc123", + "gatekeeper.sh/mutations": "Assign//policy1:1", + "keep-this": "value", + }, + }, + } + + cleaned := cleanMutationWebhookFields(pod) + + if _, exists := cleaned.Annotations["gatekeeper.sh/mutation-id"]; exists { + t.Error("gatekeeper.sh/mutation-id should be removed") + } + if _, exists := cleaned.Annotations["gatekeeper.sh/mutations"]; exists { + t.Error("gatekeeper.sh/mutations should be removed") + } + if cleaned.Annotations["keep-this"] != "value" { + t.Error("Other annotations should be preserved") + } + }) + + t.Run("does not modify original pod", func(t *testing.T) { + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + Annotations: map[string]string{ + "gatekeeper.sh/mutation-id": "abc123", + }, + }, + } + + cleanMutationWebhookFields(pod) + + if _, exists := pod.Annotations["gatekeeper.sh/mutation-id"]; !exists { + t.Error("Original pod should not be modified") + } + }) +} From de7798a1bb1a03fcbba96144af296a6e434e500f Mon Sep 17 00:00:00 2001 From: Razvan Dobre Date: Fri, 7 Nov 2025 13:48:09 +0200 Subject: [PATCH 5/7] Patch --- pkg/k8sutil/patch_options.go | 132 ++++++++- pkg/k8sutil/patch_options_test.go | 428 +++++++++++++++++++++++++++++- 2 files changed, 555 insertions(+), 5 deletions(-) diff --git a/pkg/k8sutil/patch_options.go b/pkg/k8sutil/patch_options.go index 46366bbe6..17e5c6859 100644 --- a/pkg/k8sutil/patch_options.go +++ b/pkg/k8sutil/patch_options.go @@ -37,9 +37,12 @@ func IgnoreMutationWebhookFields() patch.CalculateOption { return current, modified, nil } + // Check if ScaleOps is managing resources in EITHER pod + isScaleOpsManaged := isScaleOpsManagedPod(currentPod) || isScaleOpsManagedPod(modifiedPod) + // Remove fields that mutation webhooks commonly modify - currentPod = cleanMutationWebhookFields(currentPod) - modifiedPod = cleanMutationWebhookFields(modifiedPod) + currentPod = cleanMutationWebhookFields(currentPod, isScaleOpsManaged) + modifiedPod = cleanMutationWebhookFields(modifiedPod, isScaleOpsManaged) currentBytes, err := json.Marshal(currentPod) if err != nil { @@ -55,14 +58,137 @@ func IgnoreMutationWebhookFields() patch.CalculateOption { } } -func cleanMutationWebhookFields(pod *corev1.Pod) *corev1.Pod { +// isScaleOpsManagedPod checks if a pod is managed by ScaleOps +func isScaleOpsManagedPod(pod *corev1.Pod) bool { + return pod.Annotations != nil && (pod.Annotations["scaleops.sh/managed-containers"] != "" || + pod.Annotations["scaleops.sh/pod-owner-grouping"] != "") +} + +func cleanMutationWebhookFields(pod *corev1.Pod, isScaleOpsManaged bool) *corev1.Pod { // Create a copy to avoid modifying the original cleaned := pod.DeepCopy() // Remove mutation webhook annotations that should not trigger reconciliation if cleaned.Annotations != nil { + // Gatekeeper annotations delete(cleaned.Annotations, "gatekeeper.sh/mutation-id") delete(cleaned.Annotations, "gatekeeper.sh/mutations") + + // ScaleOps annotations + delete(cleaned.Annotations, "scaleops.sh/admission") + delete(cleaned.Annotations, "scaleops.sh/applied-policy") + delete(cleaned.Annotations, "scaleops.sh/last-applied-resources") + delete(cleaned.Annotations, "scaleops.sh/managed-containers") + delete(cleaned.Annotations, "scaleops.sh/managed-keep-limit-cpu") + delete(cleaned.Annotations, "scaleops.sh/managed-keep-limit-memory") + delete(cleaned.Annotations, "scaleops.sh/origin-resources") + delete(cleaned.Annotations, "scaleops.sh/pod-owner-grouping") + delete(cleaned.Annotations, "scaleops.sh/pod-owner-identifier") + + // Remove the last-applied annotation that may contain ScaleOps fields + // Note: This is regenerated on updates by the k8s-objectmatcher library + delete(cleaned.Annotations, "banzaicloud.com/last-applied") + + // If annotations map is empty, set to nil to normalize comparison + if len(cleaned.Annotations) == 0 { + cleaned.Annotations = nil + } + } + + // Remove ScaleOps labels + if cleaned.Labels != nil { + delete(cleaned.Labels, "scaleops.sh/applied-recommendation") + delete(cleaned.Labels, "scaleops.sh/managed") + delete(cleaned.Labels, "scaleops.sh/managed-unevictable") + delete(cleaned.Labels, "scaleops.sh/pod-owner-grouping") + delete(cleaned.Labels, "scaleops.sh/pod-owner-identifier") + + // If labels map is empty, set to nil to normalize comparison + if len(cleaned.Labels) == 0 { + cleaned.Labels = nil + } + } + + // Remove ScaleOps-added affinity rules (preferred scheduling only) + if cleaned.Spec.Affinity != nil { + if cleaned.Spec.Affinity.NodeAffinity != nil { + // Remove preferred node affinity added by ScaleOps (node-packing) + if cleaned.Spec.Affinity.NodeAffinity.PreferredDuringSchedulingIgnoredDuringExecution != nil { + var filtered []corev1.PreferredSchedulingTerm + for _, term := range cleaned.Spec.Affinity.NodeAffinity.PreferredDuringSchedulingIgnoredDuringExecution { + // Keep only terms that are NOT ScaleOps node-packing preferences + isScaleOpsTerm := false + for _, expr := range term.Preference.MatchExpressions { + if expr.Key == "scaleops.sh/node-packing" { + isScaleOpsTerm = true + break + } + } + if !isScaleOpsTerm { + filtered = append(filtered, term) + } + } + if len(filtered) == 0 { + cleaned.Spec.Affinity.NodeAffinity.PreferredDuringSchedulingIgnoredDuringExecution = nil + } else { + cleaned.Spec.Affinity.NodeAffinity.PreferredDuringSchedulingIgnoredDuringExecution = filtered + } + } + // Clean up empty NodeAffinity + if cleaned.Spec.Affinity.NodeAffinity.PreferredDuringSchedulingIgnoredDuringExecution == nil && + cleaned.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution == nil { + cleaned.Spec.Affinity.NodeAffinity = nil + } + } + + if cleaned.Spec.Affinity.PodAffinity != nil { + // Remove preferred pod affinity added by ScaleOps (managed-unevictable) + if cleaned.Spec.Affinity.PodAffinity.PreferredDuringSchedulingIgnoredDuringExecution != nil { + var filtered []corev1.WeightedPodAffinityTerm + for _, term := range cleaned.Spec.Affinity.PodAffinity.PreferredDuringSchedulingIgnoredDuringExecution { + // Keep only terms that are NOT ScaleOps managed-unevictable preferences + isScaleOpsTerm := false + if term.PodAffinityTerm.LabelSelector != nil { + for _, expr := range term.PodAffinityTerm.LabelSelector.MatchExpressions { + if expr.Key == "scaleops.sh/managed-unevictable" { + isScaleOpsTerm = true + break + } + } + } + if !isScaleOpsTerm { + filtered = append(filtered, term) + } + } + if len(filtered) == 0 { + cleaned.Spec.Affinity.PodAffinity.PreferredDuringSchedulingIgnoredDuringExecution = nil + } else { + cleaned.Spec.Affinity.PodAffinity.PreferredDuringSchedulingIgnoredDuringExecution = filtered + } + } + // Clean up empty PodAffinity + if cleaned.Spec.Affinity.PodAffinity.PreferredDuringSchedulingIgnoredDuringExecution == nil && + cleaned.Spec.Affinity.PodAffinity.RequiredDuringSchedulingIgnoredDuringExecution == nil { + cleaned.Spec.Affinity.PodAffinity = nil + } + } + + // Clean up empty Affinity + if cleaned.Spec.Affinity.NodeAffinity == nil && + cleaned.Spec.Affinity.PodAffinity == nil && + cleaned.Spec.Affinity.PodAntiAffinity == nil { + cleaned.Spec.Affinity = nil + } + } + + // Clean resources if ScaleOps is managing them + if isScaleOpsManaged { + for i := range cleaned.Spec.InitContainers { + cleaned.Spec.InitContainers[i].Resources = corev1.ResourceRequirements{} + } + for i := range cleaned.Spec.Containers { + cleaned.Spec.Containers[i].Resources = corev1.ResourceRequirements{} + } } // Clean security context fields commonly set by PSPs/Gatekeeper diff --git a/pkg/k8sutil/patch_options_test.go b/pkg/k8sutil/patch_options_test.go index 0b15e3c01..66fb562b2 100644 --- a/pkg/k8sutil/patch_options_test.go +++ b/pkg/k8sutil/patch_options_test.go @@ -565,7 +565,7 @@ func TestCleanMutationWebhookFields(t *testing.T) { }, } - cleaned := cleanMutationWebhookFields(pod) + cleaned := cleanMutationWebhookFields(pod, false) if _, exists := cleaned.Annotations["gatekeeper.sh/mutation-id"]; exists { t.Error("gatekeeper.sh/mutation-id should be removed") @@ -589,10 +589,434 @@ func TestCleanMutationWebhookFields(t *testing.T) { }, } - cleanMutationWebhookFields(pod) + cleanMutationWebhookFields(pod, false) if _, exists := pod.Annotations["gatekeeper.sh/mutation-id"]; !exists { t.Error("Original pod should not be modified") } }) } + +func TestIgnoreScaleOpsFields(t *testing.T) { + tests := []struct { + name string + currentPod *corev1.Pod + modifiedPod *corev1.Pod + expectDiff bool + description string + }{ + { + name: "ignore scaleops annotations and labels", + currentPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + Annotations: map[string]string{ + "scaleops.sh/admission": "true", + "scaleops.sh/applied-policy": "high-availability", + "scaleops.sh/last-applied-resources": "{}", + "scaleops.sh/managed-containers": "{}", + "scaleops.sh/managed-keep-limit-cpu": "true", + "scaleops.sh/managed-keep-limit-memory": "true", + "scaleops.sh/origin-resources": "{}", + "scaleops.sh/pod-owner-grouping": "kafkabroker", + "scaleops.sh/pod-owner-identifier": "pipeline-kafka-123", + "app": "kafka", + }, + Labels: map[string]string{ + "scaleops.sh/applied-recommendation": "kafkabroker-pipeline-kafka-123", + "scaleops.sh/managed": "true", + "scaleops.sh/managed-unevictable": "true", + "scaleops.sh/pod-owner-grouping": "kafkabroker", + "scaleops.sh/pod-owner-identifier": "pipeline-kafka-123", + "app": "kafka", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:latest", + }, + }, + }, + }, + modifiedPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + Annotations: map[string]string{ + "app": "kafka", + }, + Labels: map[string]string{ + "app": "kafka", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:latest", + }, + }, + }, + }, + expectDiff: false, + description: "ScaleOps annotations and labels should be ignored", + }, + { + name: "ignore scaleops-modified resources", + currentPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + Annotations: map[string]string{ + "scaleops.sh/managed-containers": "{}", + "scaleops.sh/pod-owner-grouping": "kafkabroker", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:latest", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("697m"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("4"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + }, + }, + }, + }, + }, + modifiedPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + Annotations: map[string]string{ + "scaleops.sh/managed-containers": "{}", + "scaleops.sh/pod-owner-grouping": "kafkabroker", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:latest", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("1"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("4"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + }, + }, + }, + }, + }, + expectDiff: false, + description: "ScaleOps-modified resources should be ignored when annotations present", + }, + { + name: "ignore scaleops-added affinity rules", + currentPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + }, + Spec: corev1.PodSpec{ + Affinity: &corev1.Affinity{ + NodeAffinity: &corev1.NodeAffinity{ + PreferredDuringSchedulingIgnoredDuringExecution: []corev1.PreferredSchedulingTerm{ + { + Weight: 95, + Preference: corev1.NodeSelectorTerm{ + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: "scaleops.sh/node-packing", + Operator: corev1.NodeSelectorOpIn, + Values: []string{"high"}, + }, + }, + }, + }, + { + Weight: 50, + Preference: corev1.NodeSelectorTerm{ + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: "scaleops.sh/node-packing", + Operator: corev1.NodeSelectorOpIn, + Values: []string{"medium"}, + }, + }, + }, + }, + }, + }, + PodAffinity: &corev1.PodAffinity{ + PreferredDuringSchedulingIgnoredDuringExecution: []corev1.WeightedPodAffinityTerm{ + { + Weight: 100, + PodAffinityTerm: corev1.PodAffinityTerm{ + LabelSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "scaleops.sh/managed-unevictable", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"true"}, + }, + }, + }, + TopologyKey: "kubernetes.io/hostname", + }, + }, + }, + }, + }, + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:latest", + }, + }, + }, + }, + modifiedPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:latest", + }, + }, + }, + }, + expectDiff: false, + description: "ScaleOps-added affinity rules should be ignored", + }, + { + name: "detect image changes even with scaleops", + currentPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + Annotations: map[string]string{ + "scaleops.sh/pod-owner-grouping": "kafkabroker", + }, + Labels: map[string]string{ + "scaleops.sh/managed": "true", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:3.6.1", + }, + }, + }, + }, + modifiedPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + Annotations: map[string]string{ + "scaleops.sh/pod-owner-grouping": "kafkabroker", + }, + Labels: map[string]string{ + "scaleops.sh/managed": "true", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:3.9.1", + }, + }, + }, + }, + expectDiff: true, + description: "Image changes should be detected even with ScaleOps annotations", + }, + { + name: "complex scaleops scenario - all mutations ignored", + currentPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pipeline-kafka-101-4s7b2", + Namespace: "default", + Annotations: map[string]string{ + "scaleops.sh/admission": "true", + "scaleops.sh/applied-policy": "high-availability", + "scaleops.sh/managed-containers": "{}", + "scaleops.sh/pod-owner-grouping": "kafkabroker", + "app": "kafka", + }, + Labels: map[string]string{ + "scaleops.sh/managed": "true", + "scaleops.sh/managed-unevictable": "true", + "scaleops.sh/pod-owner-grouping": "kafkabroker", + "app": "kafka", + "brokerId": "101", + }, + }, + Spec: corev1.PodSpec{ + Affinity: &corev1.Affinity{ + NodeAffinity: &corev1.NodeAffinity{ + PreferredDuringSchedulingIgnoredDuringExecution: []corev1.PreferredSchedulingTerm{ + { + Weight: 95, + Preference: corev1.NodeSelectorTerm{ + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: "scaleops.sh/node-packing", + Operator: corev1.NodeSelectorOpIn, + Values: []string{"high"}, + }, + }, + }, + }, + }, + }, + PodAffinity: &corev1.PodAffinity{ + PreferredDuringSchedulingIgnoredDuringExecution: []corev1.WeightedPodAffinityTerm{ + { + Weight: 100, + PodAffinityTerm: corev1.PodAffinityTerm{ + LabelSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "scaleops.sh/managed-unevictable", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"true"}, + }, + }, + }, + TopologyKey: "kubernetes.io/hostname", + }, + }, + }, + }, + }, + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:3.9.1", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("697m"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("4"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + }, + }, + { + Name: "fluent-bit", + Image: "fluent-bit:latest", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("100Mi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("256Mi"), + }, + }, + }, + }, + }, + }, + modifiedPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pipeline-kafka-101-4s7b2", + Namespace: "default", + Annotations: map[string]string{ + "app": "kafka", + }, + Labels: map[string]string{ + "app": "kafka", + "brokerId": "101", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:3.9.1", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("1"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("4"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + }, + }, + { + Name: "fluent-bit", + Image: "fluent-bit:latest", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("256Mi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("256Mi"), + }, + }, + }, + }, + }, + }, + expectDiff: false, + description: "Complex ScaleOps scenario: all ScaleOps mutations should be ignored", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Set last applied annotation on current pod + if err := patch.DefaultAnnotator.SetLastAppliedAnnotation(tt.currentPod); err != nil { + t.Fatalf("Failed to set last applied annotation: %v", err) + } + + opts := []patch.CalculateOption{ + IgnoreMutationWebhookFields(), + } + + patchResult, err := patch.DefaultPatchMaker.Calculate(tt.currentPod, tt.modifiedPod, opts...) + if err != nil { + t.Fatalf("Failed to calculate patch: %v", err) + } + + hasDiff := !patchResult.IsEmpty() + if hasDiff != tt.expectDiff { + t.Errorf("%s: expected diff=%v, got diff=%v\nPatch: %s\nCurrent: %s\nModified: %s", + tt.description, tt.expectDiff, hasDiff, + string(patchResult.Patch), + string(patchResult.Current), + string(patchResult.Modified)) + } + }) + } +} From 2248b585c1df5293e57111a0bc7dcc9cfee8fc44 Mon Sep 17 00:00:00 2001 From: Razvan Dobre Date: Fri, 7 Nov 2025 13:54:00 +0200 Subject: [PATCH 6/7] Remove unused code --- pkg/k8sutil/patch_options.go | 79 -------- pkg/k8sutil/patch_options_test.go | 309 +----------------------------- 2 files changed, 1 insertion(+), 387 deletions(-) diff --git a/pkg/k8sutil/patch_options.go b/pkg/k8sutil/patch_options.go index 17e5c6859..05b39fa7f 100644 --- a/pkg/k8sutil/patch_options.go +++ b/pkg/k8sutil/patch_options.go @@ -220,82 +220,3 @@ func cleanSecurityContext(container *corev1.Container) { // Capabilities are often modified by PSPs // container.SecurityContext.Capabilities = nil } - -// IgnorePodResourcesIfAnnotated creates a CalculateOption that ignores pod resource -// requests/limits if the pod has specific annotations indicating it's managed by -// an external system (e.g., ScaleOps, VPA) -func IgnorePodResourcesIfAnnotated() patch.CalculateOption { - return func(current, modified []byte) ([]byte, []byte, error) { - currentMap := map[string]interface{}{} - if err := json.Unmarshal(current, ¤tMap); err != nil { - return current, modified, nil - } - - // Check if this pod should ignore resource diffs (e.g., via annotation) - if shouldIgnoreResources(currentMap) { - // Remove resources from comparison - current = removeResourcesFromPod(current) - modified = removeResourcesFromPod(modified) - } - - return current, modified, nil - } -} - -func shouldIgnoreResources(podMap map[string]interface{}) bool { - metadata, ok := podMap["metadata"].(map[string]interface{}) - if !ok { - return false - } - - annotations, ok := metadata["annotations"].(map[string]interface{}) - if !ok { - return false - } - - // Check for annotations that indicate external resource management - annotationsToCheck := []string{ - "scaleops.sh/pod-owner-grouping", - "vpa.k8s.io/updateMode", - "cluster-autoscaler.kubernetes.io/safe-to-evict-local-volumes", - } - - for _, ann := range annotationsToCheck { - if _, exists := annotations[ann]; exists { - return true - } - } - - return false -} - -func removeResourcesFromPod(podBytes []byte) []byte { - podMap := map[string]interface{}{} - if err := json.Unmarshal(podBytes, &podMap); err != nil { - return podBytes - } - - if spec, ok := podMap["spec"].(map[string]interface{}); ok { - // Remove resources from all containers - if containers, ok := spec["containers"].([]interface{}); ok { - for _, c := range containers { - if container, ok := c.(map[string]interface{}); ok { - delete(container, "resources") - } - } - } - if initContainers, ok := spec["initContainers"].([]interface{}); ok { - for _, c := range initContainers { - if container, ok := c.(map[string]interface{}); ok { - delete(container, "resources") - } - } - } - } - - result, err := json.Marshal(podMap) - if err != nil { - return podBytes - } - return result -} diff --git a/pkg/k8sutil/patch_options_test.go b/pkg/k8sutil/patch_options_test.go index 66fb562b2..ce6b84ca0 100644 --- a/pkg/k8sutil/patch_options_test.go +++ b/pkg/k8sutil/patch_options_test.go @@ -172,245 +172,6 @@ func TestIgnoreMutationWebhookFields(t *testing.T) { } } -func TestIgnorePodResourcesIfAnnotated(t *testing.T) { - tests := []struct { - name string - currentPod *corev1.Pod - modifiedPod *corev1.Pod - expectDiff bool - description string - }{ - { - name: "ignore resources with scaleops annotation", - currentPod: &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-pod", - Namespace: "default", - Annotations: map[string]string{ - "scaleops.sh/pod-owner-grouping": "kafkacluster", - }, - }, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "kafka", - Image: "kafka:latest", - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("2000m"), - corev1.ResourceMemory: resource.MustParse("4Gi"), - }, - Limits: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("4000m"), - corev1.ResourceMemory: resource.MustParse("8Gi"), - }, - }, - }, - }, - }, - }, - modifiedPod: &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-pod", - Namespace: "default", - Annotations: map[string]string{ - "scaleops.sh/pod-owner-grouping": "kafkacluster", - }, - }, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "kafka", - Image: "kafka:latest", - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("1000m"), - corev1.ResourceMemory: resource.MustParse("2Gi"), - }, - Limits: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("2000m"), - corev1.ResourceMemory: resource.MustParse("4Gi"), - }, - }, - }, - }, - }, - }, - expectDiff: false, - description: "Resource differences should be ignored with scaleops annotation", - }, - { - name: "ignore resources with vpa annotation", - currentPod: &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-pod", - Namespace: "default", - Annotations: map[string]string{ - "vpa.k8s.io/updateMode": "Auto", - }, - }, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "kafka", - Image: "kafka:latest", - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("2000m"), - corev1.ResourceMemory: resource.MustParse("4Gi"), - }, - }, - }, - }, - }, - }, - modifiedPod: &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-pod", - Namespace: "default", - Annotations: map[string]string{ - "vpa.k8s.io/updateMode": "Auto", - }, - }, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "kafka", - Image: "kafka:latest", - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("1000m"), - corev1.ResourceMemory: resource.MustParse("2Gi"), - }, - }, - }, - }, - }, - }, - expectDiff: false, - description: "Resource differences should be ignored with VPA annotation", - }, - { - name: "detect resource changes without annotation", - currentPod: &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-pod", - Namespace: "default", - }, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "kafka", - Image: "kafka:latest", - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("2000m"), - corev1.ResourceMemory: resource.MustParse("4Gi"), - }, - }, - }, - }, - }, - }, - modifiedPod: &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-pod", - Namespace: "default", - }, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "kafka", - Image: "kafka:latest", - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("1000m"), - corev1.ResourceMemory: resource.MustParse("2Gi"), - }, - }, - }, - }, - }, - }, - expectDiff: true, - description: "Resource changes should be detected without external management annotation", - }, - { - name: "detect non-resource changes even with annotation", - currentPod: &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-pod", - Namespace: "default", - Annotations: map[string]string{ - "scaleops.sh/pod-owner-grouping": "kafkacluster", - }, - }, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "kafka", - Image: "kafka:1.0", - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("1000m"), - }, - }, - }, - }, - }, - }, - modifiedPod: &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-pod", - Namespace: "default", - Annotations: map[string]string{ - "scaleops.sh/pod-owner-grouping": "kafkacluster", - }, - }, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "kafka", - Image: "kafka:2.0", - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("2000m"), - }, - }, - }, - }, - }, - }, - expectDiff: true, - description: "Non-resource changes (image) should still be detected", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Set last applied annotation on current pod - if err := patch.DefaultAnnotator.SetLastAppliedAnnotation(tt.currentPod); err != nil { - t.Fatalf("Failed to set last applied annotation: %v", err) - } - - opts := []patch.CalculateOption{ - IgnorePodResourcesIfAnnotated(), - } - - patchResult, err := patch.DefaultPatchMaker.Calculate(tt.currentPod, tt.modifiedPod, opts...) - if err != nil { - t.Fatalf("Failed to calculate patch: %v", err) - } - - hasDiff := !patchResult.IsEmpty() - if hasDiff != tt.expectDiff { - t.Errorf("%s: expected diff=%v, got diff=%v\nPatch: %s", - tt.description, tt.expectDiff, hasDiff, string(patchResult.Patch)) - } - }) - } -} - func TestCombinedIgnoreOptions(t *testing.T) { t.Run("combine mutation webhook and resource ignoring", func(t *testing.T) { currentPod := &corev1.Pod{ @@ -469,7 +230,6 @@ func TestCombinedIgnoreOptions(t *testing.T) { opts := []patch.CalculateOption{ IgnoreMutationWebhookFields(), - IgnorePodResourcesIfAnnotated(), } patchResult, err := patch.DefaultPatchMaker.Calculate(currentPod, modifiedPod, opts...) @@ -478,79 +238,12 @@ func TestCombinedIgnoreOptions(t *testing.T) { } if !patchResult.IsEmpty() { - t.Errorf("Expected no diff when both mutation webhook annotations and resources differ, but got patch: %s", + t.Errorf("Expected no diff when both mutation webhook annotations and resources differ (ScaleOps managed), but got patch: %s", string(patchResult.Patch)) } }) } -func TestShouldIgnoreResources(t *testing.T) { - tests := []struct { - name string - annotations map[string]string - expect bool - }{ - { - name: "scaleops annotation present", - annotations: map[string]string{ - "scaleops.sh/pod-owner-grouping": "kafkacluster", - }, - expect: true, - }, - { - name: "vpa annotation present", - annotations: map[string]string{ - "vpa.k8s.io/updateMode": "Auto", - }, - expect: true, - }, - { - name: "cluster-autoscaler annotation present", - annotations: map[string]string{ - "cluster-autoscaler.kubernetes.io/safe-to-evict-local-volumes": "data", - }, - expect: true, - }, - { - name: "no relevant annotations", - annotations: map[string]string{ - "some.other/annotation": "value", - }, - expect: false, - }, - { - name: "no annotations", - annotations: nil, - expect: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - podMap := map[string]interface{}{ - "metadata": map[string]interface{}{ - "name": "test-pod", - "namespace": "default", - }, - } - - if tt.annotations != nil { - metadata := podMap["metadata"].(map[string]interface{}) - annMap := make(map[string]interface{}) - for k, v := range tt.annotations { - annMap[k] = v - } - metadata["annotations"] = annMap - } - - result := shouldIgnoreResources(podMap) - if result != tt.expect { - t.Errorf("Expected shouldIgnoreResources=%v, got %v", tt.expect, result) - } - }) - } -} - func TestCleanMutationWebhookFields(t *testing.T) { t.Run("removes gatekeeper annotations", func(t *testing.T) { pod := &corev1.Pod{ From 70884fa6fd58d409464fb3a352d9211eef168f3f Mon Sep 17 00:00:00 2001 From: Razvan Dobre Date: Fri, 7 Nov 2025 14:25:26 +0200 Subject: [PATCH 7/7] Fix linting --- pkg/k8sutil/patch_options_test.go | 619 +++++++++++++----------------- 1 file changed, 272 insertions(+), 347 deletions(-) diff --git a/pkg/k8sutil/patch_options_test.go b/pkg/k8sutil/patch_options_test.go index ce6b84ca0..d9cc4a22e 100644 --- a/pkg/k8sutil/patch_options_test.go +++ b/pkg/k8sutil/patch_options_test.go @@ -290,401 +290,326 @@ func TestCleanMutationWebhookFields(t *testing.T) { }) } -func TestIgnoreScaleOpsFields(t *testing.T) { - tests := []struct { - name string - currentPod *corev1.Pod - modifiedPod *corev1.Pod - expectDiff bool - description string - }{ - { - name: "ignore scaleops annotations and labels", - currentPod: &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-pod", - Namespace: "default", - Annotations: map[string]string{ - "scaleops.sh/admission": "true", - "scaleops.sh/applied-policy": "high-availability", - "scaleops.sh/last-applied-resources": "{}", - "scaleops.sh/managed-containers": "{}", - "scaleops.sh/managed-keep-limit-cpu": "true", - "scaleops.sh/managed-keep-limit-memory": "true", - "scaleops.sh/origin-resources": "{}", - "scaleops.sh/pod-owner-grouping": "kafkabroker", - "scaleops.sh/pod-owner-identifier": "pipeline-kafka-123", - "app": "kafka", - }, - Labels: map[string]string{ - "scaleops.sh/applied-recommendation": "kafkabroker-pipeline-kafka-123", - "scaleops.sh/managed": "true", - "scaleops.sh/managed-unevictable": "true", - "scaleops.sh/pod-owner-grouping": "kafkabroker", - "scaleops.sh/pod-owner-identifier": "pipeline-kafka-123", - "app": "kafka", - }, - }, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "kafka", - Image: "kafka:latest", - }, - }, - }, - }, - modifiedPod: &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-pod", - Namespace: "default", - Annotations: map[string]string{ - "app": "kafka", - }, - Labels: map[string]string{ - "app": "kafka", - }, - }, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "kafka", - Image: "kafka:latest", - }, - }, +// Helper functions for creating test pods +func createBasicPod() *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + Annotations: map[string]string{}, + Labels: map[string]string{}, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "kafka", + Image: "kafka:latest", }, }, - expectDiff: false, - description: "ScaleOps annotations and labels should be ignored", }, - { - name: "ignore scaleops-modified resources", - currentPod: &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-pod", - Namespace: "default", - Annotations: map[string]string{ - "scaleops.sh/managed-containers": "{}", - "scaleops.sh/pod-owner-grouping": "kafkabroker", - }, - }, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "kafka", - Image: "kafka:latest", - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("697m"), - corev1.ResourceMemory: resource.MustParse("4Gi"), - }, - Limits: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("4"), - corev1.ResourceMemory: resource.MustParse("4Gi"), - }, + } +} + +func createScaleOpsAffinity() *corev1.Affinity { + return &corev1.Affinity{ + NodeAffinity: &corev1.NodeAffinity{ + PreferredDuringSchedulingIgnoredDuringExecution: []corev1.PreferredSchedulingTerm{ + { + Weight: 95, + Preference: corev1.NodeSelectorTerm{ + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: "scaleops.sh/node-packing", + Operator: corev1.NodeSelectorOpIn, + Values: []string{"high"}, }, }, }, }, - }, - modifiedPod: &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-pod", - Namespace: "default", - Annotations: map[string]string{ - "scaleops.sh/managed-containers": "{}", - "scaleops.sh/pod-owner-grouping": "kafkabroker", - }, - }, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "kafka", - Image: "kafka:latest", - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("1"), - corev1.ResourceMemory: resource.MustParse("4Gi"), - }, - Limits: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("4"), - corev1.ResourceMemory: resource.MustParse("4Gi"), - }, + { + Weight: 50, + Preference: corev1.NodeSelectorTerm{ + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: "scaleops.sh/node-packing", + Operator: corev1.NodeSelectorOpIn, + Values: []string{"medium"}, }, }, }, }, }, - expectDiff: false, - description: "ScaleOps-modified resources should be ignored when annotations present", }, - { - name: "ignore scaleops-added affinity rules", - currentPod: &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-pod", - Namespace: "default", - }, - Spec: corev1.PodSpec{ - Affinity: &corev1.Affinity{ - NodeAffinity: &corev1.NodeAffinity{ - PreferredDuringSchedulingIgnoredDuringExecution: []corev1.PreferredSchedulingTerm{ - { - Weight: 95, - Preference: corev1.NodeSelectorTerm{ - MatchExpressions: []corev1.NodeSelectorRequirement{ - { - Key: "scaleops.sh/node-packing", - Operator: corev1.NodeSelectorOpIn, - Values: []string{"high"}, - }, - }, - }, - }, - { - Weight: 50, - Preference: corev1.NodeSelectorTerm{ - MatchExpressions: []corev1.NodeSelectorRequirement{ - { - Key: "scaleops.sh/node-packing", - Operator: corev1.NodeSelectorOpIn, - Values: []string{"medium"}, - }, - }, - }, - }, - }, - }, - PodAffinity: &corev1.PodAffinity{ - PreferredDuringSchedulingIgnoredDuringExecution: []corev1.WeightedPodAffinityTerm{ + PodAffinity: &corev1.PodAffinity{ + PreferredDuringSchedulingIgnoredDuringExecution: []corev1.WeightedPodAffinityTerm{ + { + Weight: 100, + PodAffinityTerm: corev1.PodAffinityTerm{ + LabelSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ { - Weight: 100, - PodAffinityTerm: corev1.PodAffinityTerm{ - LabelSelector: &metav1.LabelSelector{ - MatchExpressions: []metav1.LabelSelectorRequirement{ - { - Key: "scaleops.sh/managed-unevictable", - Operator: metav1.LabelSelectorOpIn, - Values: []string{"true"}, - }, - }, - }, - TopologyKey: "kubernetes.io/hostname", - }, + Key: "scaleops.sh/managed-unevictable", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"true"}, }, }, }, - }, - Containers: []corev1.Container{ - { - Name: "kafka", - Image: "kafka:latest", - }, - }, - }, - }, - modifiedPod: &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-pod", - Namespace: "default", - }, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "kafka", - Image: "kafka:latest", - }, + TopologyKey: "kubernetes.io/hostname", }, }, }, - expectDiff: false, - description: "ScaleOps-added affinity rules should be ignored", }, - { - name: "detect image changes even with scaleops", - currentPod: &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-pod", - Namespace: "default", - Annotations: map[string]string{ - "scaleops.sh/pod-owner-grouping": "kafkabroker", - }, - Labels: map[string]string{ - "scaleops.sh/managed": "true", - }, - }, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "kafka", - Image: "kafka:3.6.1", - }, - }, - }, + } +} + +func createKafkaContainer(image, cpuRequest string) corev1.Container { + container := corev1.Container{ + Name: "kafka", + Image: image, + } + if cpuRequest != "" { + container.Resources = corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse(cpuRequest), + corev1.ResourceMemory: resource.MustParse("4Gi"), }, - modifiedPod: &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-pod", - Namespace: "default", - Annotations: map[string]string{ - "scaleops.sh/pod-owner-grouping": "kafkabroker", - }, - Labels: map[string]string{ - "scaleops.sh/managed": "true", - }, - }, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "kafka", - Image: "kafka:3.9.1", - }, - }, - }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("4"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + } + } + return container +} + +func createComplexScaleOpsPodBefore() *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pipeline-kafka-101-4s7b2", + Namespace: "default", + Annotations: map[string]string{ + "scaleops.sh/admission": "true", + "scaleops.sh/applied-policy": "high-availability", + "scaleops.sh/managed-containers": "{}", + "scaleops.sh/pod-owner-grouping": "kafkabroker", + "app": "kafka", + }, + Labels: map[string]string{ + "scaleops.sh/managed": "true", + "scaleops.sh/managed-unevictable": "true", + "scaleops.sh/pod-owner-grouping": "kafkabroker", + "app": "kafka", + "brokerId": "101", }, - expectDiff: true, - description: "Image changes should be detected even with ScaleOps annotations", }, - { - name: "complex scaleops scenario - all mutations ignored", - currentPod: &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pipeline-kafka-101-4s7b2", - Namespace: "default", - Annotations: map[string]string{ - "scaleops.sh/admission": "true", - "scaleops.sh/applied-policy": "high-availability", - "scaleops.sh/managed-containers": "{}", - "scaleops.sh/pod-owner-grouping": "kafkabroker", - "app": "kafka", - }, - Labels: map[string]string{ - "scaleops.sh/managed": "true", - "scaleops.sh/managed-unevictable": "true", - "scaleops.sh/pod-owner-grouping": "kafkabroker", - "app": "kafka", - "brokerId": "101", - }, - }, - Spec: corev1.PodSpec{ - Affinity: &corev1.Affinity{ - NodeAffinity: &corev1.NodeAffinity{ - PreferredDuringSchedulingIgnoredDuringExecution: []corev1.PreferredSchedulingTerm{ - { - Weight: 95, - Preference: corev1.NodeSelectorTerm{ - MatchExpressions: []corev1.NodeSelectorRequirement{ - { - Key: "scaleops.sh/node-packing", - Operator: corev1.NodeSelectorOpIn, - Values: []string{"high"}, - }, - }, + Spec: corev1.PodSpec{ + Affinity: &corev1.Affinity{ + NodeAffinity: &corev1.NodeAffinity{ + PreferredDuringSchedulingIgnoredDuringExecution: []corev1.PreferredSchedulingTerm{ + { + Weight: 95, + Preference: corev1.NodeSelectorTerm{ + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: "scaleops.sh/node-packing", + Operator: corev1.NodeSelectorOpIn, + Values: []string{"high"}, }, }, }, }, - PodAffinity: &corev1.PodAffinity{ - PreferredDuringSchedulingIgnoredDuringExecution: []corev1.WeightedPodAffinityTerm{ - { - Weight: 100, - PodAffinityTerm: corev1.PodAffinityTerm{ - LabelSelector: &metav1.LabelSelector{ - MatchExpressions: []metav1.LabelSelectorRequirement{ - { - Key: "scaleops.sh/managed-unevictable", - Operator: metav1.LabelSelectorOpIn, - Values: []string{"true"}, - }, - }, + }, + }, + PodAffinity: &corev1.PodAffinity{ + PreferredDuringSchedulingIgnoredDuringExecution: []corev1.WeightedPodAffinityTerm{ + { + Weight: 100, + PodAffinityTerm: corev1.PodAffinityTerm{ + LabelSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "scaleops.sh/managed-unevictable", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"true"}, }, - TopologyKey: "kubernetes.io/hostname", }, }, + TopologyKey: "kubernetes.io/hostname", }, }, }, - Containers: []corev1.Container{ - { - Name: "kafka", - Image: "kafka:3.9.1", - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("697m"), - corev1.ResourceMemory: resource.MustParse("4Gi"), - }, - Limits: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("4"), - corev1.ResourceMemory: resource.MustParse("4Gi"), - }, - }, + }, + }, + Containers: []corev1.Container{ + createKafkaContainer("kafka:3.9.1", "697m"), + { + Name: "fluent-bit", + Image: "fluent-bit:latest", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("100Mi"), }, - { - Name: "fluent-bit", - Image: "fluent-bit:latest", - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("100m"), - corev1.ResourceMemory: resource.MustParse("100Mi"), - }, - Limits: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("100m"), - corev1.ResourceMemory: resource.MustParse("256Mi"), - }, - }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("256Mi"), }, }, }, }, - modifiedPod: &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pipeline-kafka-101-4s7b2", - Namespace: "default", - Annotations: map[string]string{ - "app": "kafka", - }, - Labels: map[string]string{ - "app": "kafka", - "brokerId": "101", - }, - }, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "kafka", - Image: "kafka:3.9.1", - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("1"), - corev1.ResourceMemory: resource.MustParse("4Gi"), - }, - Limits: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("4"), - corev1.ResourceMemory: resource.MustParse("4Gi"), - }, - }, + }, + } +} + +func createComplexScaleOpsPodAfter() *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pipeline-kafka-101-4s7b2", + Namespace: "default", + Annotations: map[string]string{ + "app": "kafka", + }, + Labels: map[string]string{ + "app": "kafka", + "brokerId": "101", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + createKafkaContainer("kafka:3.9.1", "1"), + { + Name: "fluent-bit", + Image: "fluent-bit:latest", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("256Mi"), }, - { - Name: "fluent-bit", - Image: "fluent-bit:latest", - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("100m"), - corev1.ResourceMemory: resource.MustParse("256Mi"), - }, - Limits: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("100m"), - corev1.ResourceMemory: resource.MustParse("256Mi"), - }, - }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("256Mi"), }, }, }, }, + }, + } +} + +func getScaleOpsTestCases() []struct { + name string + currentPod *corev1.Pod + modifiedPod *corev1.Pod + expectDiff bool + description string +} { + return []struct { + name string + currentPod *corev1.Pod + modifiedPod *corev1.Pod + expectDiff bool + description string + }{ + { + name: "ignore scaleops annotations and labels", + currentPod: func() *corev1.Pod { + pod := createBasicPod() + pod.Annotations = map[string]string{ + "scaleops.sh/admission": "true", + "scaleops.sh/applied-policy": "high-availability", + "scaleops.sh/last-applied-resources": "{}", + "scaleops.sh/managed-containers": "{}", + "scaleops.sh/managed-keep-limit-cpu": "true", + "scaleops.sh/managed-keep-limit-memory": "true", + "scaleops.sh/origin-resources": "{}", + "scaleops.sh/pod-owner-grouping": "kafkabroker", + "scaleops.sh/pod-owner-identifier": "pipeline-kafka-123", + "app": "kafka", + } + pod.Labels = map[string]string{ + "scaleops.sh/applied-recommendation": "kafkabroker-pipeline-kafka-123", + "scaleops.sh/managed": "true", + "scaleops.sh/managed-unevictable": "true", + "scaleops.sh/pod-owner-grouping": "kafkabroker", + "scaleops.sh/pod-owner-identifier": "pipeline-kafka-123", + "app": "kafka", + } + return pod + }(), + modifiedPod: func() *corev1.Pod { + pod := createBasicPod() + pod.Annotations["app"] = "kafka" + pod.Labels["app"] = "kafka" + return pod + }(), + expectDiff: false, + description: "ScaleOps annotations and labels should be ignored", + }, + { + name: "ignore scaleops-modified resources", + currentPod: func() *corev1.Pod { + pod := createBasicPod() + pod.Annotations = map[string]string{ + "scaleops.sh/managed-containers": "{}", + "scaleops.sh/pod-owner-grouping": "kafkabroker", + } + pod.Spec.Containers[0] = createKafkaContainer("kafka:latest", "697m") + return pod + }(), + modifiedPod: func() *corev1.Pod { + pod := createBasicPod() + pod.Annotations = map[string]string{ + "scaleops.sh/managed-containers": "{}", + "scaleops.sh/pod-owner-grouping": "kafkabroker", + } + pod.Spec.Containers[0] = createKafkaContainer("kafka:latest", "1") + return pod + }(), + expectDiff: false, + description: "ScaleOps-modified resources should be ignored when annotations present", + }, + { + name: "ignore scaleops-added affinity rules", + currentPod: func() *corev1.Pod { + pod := createBasicPod() + pod.Spec.Affinity = createScaleOpsAffinity() + return pod + }(), + modifiedPod: createBasicPod(), + expectDiff: false, + description: "ScaleOps-added affinity rules should be ignored", + }, + { + name: "detect image changes even with scaleops", + currentPod: func() *corev1.Pod { + pod := createBasicPod() + pod.Annotations["scaleops.sh/pod-owner-grouping"] = "kafkabroker" + pod.Labels["scaleops.sh/managed"] = "true" + pod.Spec.Containers[0].Image = "kafka:3.6.1" + return pod + }(), + modifiedPod: func() *corev1.Pod { + pod := createBasicPod() + pod.Annotations["scaleops.sh/pod-owner-grouping"] = "kafkabroker" + pod.Labels["scaleops.sh/managed"] = "true" + pod.Spec.Containers[0].Image = "kafka:3.9.1" + return pod + }(), + expectDiff: true, + description: "Image changes should be detected even with ScaleOps annotations", + }, + { + name: "complex scaleops scenario - all mutations ignored", + currentPod: createComplexScaleOpsPodBefore(), + modifiedPod: createComplexScaleOpsPodAfter(), expectDiff: false, description: "Complex ScaleOps scenario: all ScaleOps mutations should be ignored", }, } +} + +func TestIgnoreScaleOpsFields(t *testing.T) { + tests := getScaleOpsTestCases() for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {