diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality.go index 2f0bb4e4841..6065607d5a0 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality.go +++ b/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( diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality_test.go new file mode 100644 index 00000000000..68b786f85b6 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/test/integration/apiserver/apply/apply_crd_test.go b/test/integration/apiserver/apply/apply_crd_test.go index 71a5e2fad46..4ac048e7974 100644 --- a/test/integration/apiserver/apply/apply_crd_test.go +++ b/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, + ) + } +}