Merge pull request #125263 from jpbetz/fix-nop-apply

Fix check to ignore non-semantic changes to objects to handle unstructured

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality.go

@@ -52,7 +52,7 @@ func getAvoidTimestampEqualities() conversion.Equalities {
 		}
 
 		var eqs = equality.Semantic.Copy()
-		err := eqs.AddFunc(
+		err := eqs.AddFuncs(
 			func(a, b metav1.ManagedFieldsEntry) bool {
 				// Two objects' managed fields are equivalent if, ignoring timestamp,
 				//	the objects are deeply equal.
@@ -60,6 +60,14 @@ func getAvoidTimestampEqualities() conversion.Equalities {
 				b.Time = nil
 				return reflect.DeepEqual(a, b)
 			},
+			func(a, b unstructured.Unstructured) bool {
+				// Check if the managed fields are equal by converting to structured types and leveraging the above
+				// function, then, ignoring the managed fields, equality check the rest of the unstructured data.
+				if !avoidTimestampEqualities.DeepEqual(a.GetManagedFields(), b.GetManagedFields()) {
+					return false
+				}
+				return equalIgnoringValueAtPath(a.Object, b.Object, []string{"metadata", "managedFields"})
+			},
 		)
 
 		if err != nil {
@@ -71,6 +79,36 @@ func getAvoidTimestampEqualities() conversion.Equalities {
 	return avoidTimestampEqualities
 }
 
+func equalIgnoringValueAtPath(a, b any, path []string) bool {
+	if len(path) == 0 { // found the value to ignore
+		return true
+	}
+	aMap, aOk := a.(map[string]any)
+	bMap, bOk := b.(map[string]any)
+	if !aOk || !bOk {
+		// Can't traverse into non-maps, ignore
+		return true
+	}
+	if len(aMap) != len(bMap) {
+		return false
+	}
+	pathHead := path[0]
+	for k, aVal := range aMap {
+		bVal, ok := bMap[k]
+		if !ok {
+			return false
+		}
+		if k == pathHead {
+			if !equalIgnoringValueAtPath(aVal, bVal, path[1:]) {
+				return false
+			}
+		} else if !avoidTimestampEqualities.DeepEqual(aVal, bVal) {
+			return false
+		}
+	}
+	return true
+}
+
 // IgnoreManagedFieldsTimestampsTransformer reverts timestamp updates
 // if the non-managed parts of the object are equivalent
 func IgnoreManagedFieldsTimestampsTransformer(

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality_test.go

@@ -0,0 +1,198 @@
+/*
+Copyright 2024 The Kubernetes Authors.
+
+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 fieldmanager
+
+import "testing"
+
+func TestEqualIgnoringFieldValueAtPath(t *testing.T) {
+	cases := []struct {
+		name string
+		a, b map[string]any
+		want bool
+	}{
+		{
+			name: "identical objects",
+			a: map[string]any{
+				"metadata": map[string]any{
+					"labels":        map[string]any{"env": "dev"},
+					"managedFields": []any{},
+				},
+				"spec": map[string]any{
+					"field": "value",
+				},
+			},
+			b: map[string]any{
+				"metadata": map[string]any{
+					"labels":        map[string]any{"env": "dev"},
+					"managedFields": []any{},
+				},
+				"spec": map[string]any{
+					"field": "value",
+				},
+			},
+			want: true,
+		},
+		{
+			name: "different metadata label value",
+			a: map[string]any{
+				"metadata": map[string]any{
+					"labels":        map[string]any{"env": "dev"},
+					"managedFields": []any{},
+				},
+				"spec": map[string]any{
+					"field": "value",
+				},
+			},
+			b: map[string]any{
+				"metadata": map[string]any{
+					"labels":        map[string]any{"env": "prod"},
+					"managedFields": []any{},
+				},
+				"spec": map[string]any{
+					"field": "value",
+				},
+			},
+			want: false,
+		},
+		{
+			name: "different spec value",
+			a: map[string]any{
+				"metadata": map[string]any{
+					"labels":        map[string]any{"env": "dev"},
+					"managedFields": []any{},
+				},
+				"spec": map[string]any{
+					"field": "value1",
+				},
+			},
+			b: map[string]any{
+				"metadata": map[string]any{
+					"labels":        map[string]any{"env": "dev"},
+					"managedFields": []any{},
+				},
+				"spec": map[string]any{
+					"field": "value2",
+				},
+			},
+			want: false,
+		},
+		{
+			name: "extra spec field",
+			a: map[string]any{
+				"metadata": map[string]any{
+					"labels":        map[string]any{"env": "dev"},
+					"managedFields": []any{},
+				},
+				"spec": map[string]any{
+					"field":      "value1",
+					"otherField": "other",
+				},
+			},
+			b: map[string]any{
+				"metadata": map[string]any{
+					"labels":        map[string]any{"env": "dev"},
+					"managedFields": []any{},
+				},
+				"spec": map[string]any{
+					"field": "value1",
+				},
+			},
+			want: false,
+		},
+		{
+			name: "different spec field",
+			a: map[string]any{
+				"metadata": map[string]any{
+					"labels":        map[string]any{"env": "dev"},
+					"managedFields": []any{},
+				},
+				"spec": map[string]any{
+					"field": "value1",
+				},
+			},
+			b: map[string]any{
+				"metadata": map[string]any{
+					"labels":        map[string]any{"env": "dev"},
+					"managedFields": []any{},
+				},
+				"spec": map[string]any{
+					"field":      "value1",
+					"otherField": "other",
+				},
+			},
+			want: false,
+		},
+		{
+			name: "different managed fields should be ignored",
+			a: map[string]any{
+				"metadata": map[string]any{
+					"labels":        map[string]any{"env": "dev"},
+					"managedFields": []any{map[string]any{"manager": "client1"}},
+				},
+				"spec": map[string]any{
+					"field": "value",
+				},
+			},
+			b: map[string]any{
+				"metadata": map[string]any{
+					"labels":        map[string]any{"env": "dev"},
+					"managedFields": []any{map[string]any{"manager": "client2"}},
+				},
+				"spec": map[string]any{
+					"field": "value",
+				},
+			},
+			want: true,
+		},
+		{
+			name: "wrong type for metadata result in differences being ignored",
+			a: map[string]any{
+				"metadata": []any{
+					"something",
+				},
+				"spec": map[string]any{
+					"field": "value",
+				},
+			},
+			b: map[string]any{
+				"metadata": []any{
+					"something else",
+				},
+				"spec": map[string]any{
+					"field": "value",
+				},
+			},
+			want: true,
+		},
+	}
+
+	path := []string{"metadata", "managedFields"}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			actual := equalIgnoringValueAtPath(c.a, c.b, path)
+			if actual != c.want {
+				t.Error("Expected equality check to return ", c.want, ", but got ", actual)
+			}
+			// Check that equality is commutative
+			actual = equalIgnoringValueAtPath(c.b, c.a, path)
+			if actual != c.want {
+				t.Error("Expected equality check to return ", c.want, ", but got ", actual)
+			}
+		})
+	}
+}

test/integration/apiserver/apply/apply_crd_test.go

@@ -25,6 +25,8 @@ import (
 	"testing"
 	"time"
 
+	"k8s.io/apimachinery/pkg/api/meta"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/wait"
 
 	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
@@ -733,3 +735,109 @@ spec:
 		t.Fatalf("Expecting to get one conflict when a different applier updates existing list item, got: %v", status.Status().Details.Causes)
 	}
 }
+
+func TestNoOpApplyWithDefaultsSameResourceVersionCRD(t *testing.T) {
+	// https://github.com/kubernetes/kubernetes/issues/124605
+	server, err := apiservertesting.StartTestServer(t, apiservertesting.NewDefaultTestServerOptions(), nil, framework.SharedEtcd())
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer server.TearDownFn()
+	config := server.ClientConfig
+
+	apiExtensionClient, err := clientset.NewForConfig(config)
+	if err != nil {
+		t.Fatal(err)
+	}
+	dynamicClient, err := dynamic.NewForConfig(config)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	noxuDefinition := fixtures.NewNoxuV1CustomResourceDefinition(apiextensionsv1.ClusterScoped)
+	err = json.Unmarshal([]byte(`{
+		"openAPIV3Schema": {
+			"type": "object",
+			"properties": {
+				"spec": {
+					"type": "object",
+					"properties": {
+						"infrastructureRef": {
+							"type": "object",
+							"properties": {
+								"name": {
+									"type": "string"
+								},
+								"namespace": {
+									"type": "string",
+									"default": "default-namespace"
+								}
+							},
+							"x-kubernetes-map-type": "atomic"
+						}
+					}
+				}
+			}
+		}
+	}`), &noxuDefinition.Spec.Versions[0].Schema)
+	if err != nil {
+		t.Fatal(err)
+	}
+	noxuDefinition, err = fixtures.CreateNewV1CustomResourceDefinition(noxuDefinition, apiExtensionClient, dynamicClient)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	group := noxuDefinition.Spec.Group
+	kind := noxuDefinition.Spec.Names.Kind
+	resource := noxuDefinition.Spec.Names.Plural
+	version := noxuDefinition.Spec.Versions[0].Name
+	apiVersion := noxuDefinition.Spec.Group + "/" + version
+	gvr := schema.GroupVersionResource{Group: group, Version: version, Resource: resource}
+	name := "mytest"
+
+	// fieldWithDefault will be defaulted
+	applyConfiguration := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"apiVersion": apiVersion,
+			"kind":       kind,
+			"metadata": map[string]interface{}{
+				"name": name,
+			},
+			"spec": map[string]interface{}{"infrastructureRef": map[string]interface{}{"name": "infrastructure-machine-1\n"}},
+		},
+	}
+
+	created, err := dynamicClient.Resource(gvr).Apply(context.TODO(), name, applyConfiguration, metav1.ApplyOptions{FieldManager: "apply_test"})
+
+	if err != nil {
+		t.Fatalf("failed to create custom resource with apply: %v:\n%v", err, created)
+	}
+
+	createdAccessor, err := meta.Accessor(created)
+	if err != nil {
+		t.Fatalf("Failed to get meta accessor for updated object: %v", err)
+	}
+
+	// Sleep for one second to make sure that the times of each update operation is different.
+	time.Sleep(1 * time.Second)
+
+	updated, err := dynamicClient.Resource(gvr).Apply(context.TODO(), name, applyConfiguration, metav1.ApplyOptions{FieldManager: "apply_test"})
+	if err != nil {
+		t.Fatalf("failed to update custom resource with apply: %v:\n%v", err, updated)
+	}
+
+	updatedAccessor, err := meta.Accessor(updated)
+	if err != nil {
+		t.Fatalf("Failed to get meta accessor for updated object: %v", err)
+	}
+
+	if createdAccessor.GetResourceVersion() != updatedAccessor.GetResourceVersion() {
+		t.Fatalf("Expected same resource version to be %v but got: %v\nold object:\n%v\nnew object:\n%v",
+			createdAccessor.GetResourceVersion(),
+			updatedAccessor.GetResourceVersion(),
+			created,
+			updated,
+		)
+	}
+}