diff --git a/staging/src/k8s.io/apimachinery/pkg/conversion/deep_equal.go b/staging/src/k8s.io/apimachinery/pkg/conversion/deep_equal.go index f21abe1e53a..25b2923f223 100644 --- a/staging/src/k8s.io/apimachinery/pkg/conversion/deep_equal.go +++ b/staging/src/k8s.io/apimachinery/pkg/conversion/deep_equal.go @@ -34,3 +34,14 @@ func EqualitiesOrDie(funcs ...interface{}) Equalities { } return e } + +// Performs a shallow copy of the equalities map +func (e Equalities) Copy() Equalities { + result := Equalities{reflect.Equalities{}} + + for key, value := range e.Equalities { + result.Equalities[key] = value + } + + return result +} diff --git a/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal.go b/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal.go index fb6b054792b..e25247100e7 100644 --- a/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal.go +++ b/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal.go @@ -100,7 +100,8 @@ func makeUsefulPanic(v reflect.Value) { // Tests for deep equality using reflected types. The map argument tracks // comparisons that have already been seen, which allows short circuiting on // recursive types. -func (e Equalities) deepValueEqual(v1, v2 reflect.Value, visited map[visit]bool, depth int) bool { +// equateNilAndEmpty controls whether empty maps/slices are equivalent to nil +func (e Equalities) deepValueEqual(v1, v2 reflect.Value, visited map[visit]bool, equateNilAndEmpty bool, depth int) bool { defer makeUsefulPanic(v1) if !v1.IsValid() || !v2.IsValid() { @@ -150,17 +151,24 @@ func (e Equalities) deepValueEqual(v1, v2 reflect.Value, visited map[visit]bool, // We don't need to check length here because length is part of // an array's type, which has already been filtered for. for i := 0; i < v1.Len(); i++ { - if !e.deepValueEqual(v1.Index(i), v2.Index(i), visited, depth+1) { + if !e.deepValueEqual(v1.Index(i), v2.Index(i), visited, equateNilAndEmpty, depth+1) { return false } } return true case reflect.Slice: - if (v1.IsNil() || v1.Len() == 0) != (v2.IsNil() || v2.Len() == 0) { - return false - } - if v1.IsNil() || v1.Len() == 0 { - return true + if equateNilAndEmpty { + if (v1.IsNil() || v1.Len() == 0) != (v2.IsNil() || v2.Len() == 0) { + return false + } + + if v1.IsNil() || v1.Len() == 0 { + return true + } + } else { + if v1.IsNil() != v2.IsNil() { + return false + } } if v1.Len() != v2.Len() { return false @@ -169,7 +177,7 @@ func (e Equalities) deepValueEqual(v1, v2 reflect.Value, visited map[visit]bool, return true } for i := 0; i < v1.Len(); i++ { - if !e.deepValueEqual(v1.Index(i), v2.Index(i), visited, depth+1) { + if !e.deepValueEqual(v1.Index(i), v2.Index(i), visited, equateNilAndEmpty, depth+1) { return false } } @@ -178,22 +186,28 @@ func (e Equalities) deepValueEqual(v1, v2 reflect.Value, visited map[visit]bool, if v1.IsNil() || v2.IsNil() { return v1.IsNil() == v2.IsNil() } - return e.deepValueEqual(v1.Elem(), v2.Elem(), visited, depth+1) - case reflect.Pointer: - return e.deepValueEqual(v1.Elem(), v2.Elem(), visited, depth+1) + return e.deepValueEqual(v1.Elem(), v2.Elem(), visited, equateNilAndEmpty, depth+1) + case reflect.Ptr: + return e.deepValueEqual(v1.Elem(), v2.Elem(), visited, equateNilAndEmpty, depth+1) case reflect.Struct: for i, n := 0, v1.NumField(); i < n; i++ { - if !e.deepValueEqual(v1.Field(i), v2.Field(i), visited, depth+1) { + if !e.deepValueEqual(v1.Field(i), v2.Field(i), visited, equateNilAndEmpty, depth+1) { return false } } return true case reflect.Map: - if (v1.IsNil() || v1.Len() == 0) != (v2.IsNil() || v2.Len() == 0) { - return false - } - if v1.IsNil() || v1.Len() == 0 { - return true + if equateNilAndEmpty { + if (v1.IsNil() || v1.Len() == 0) != (v2.IsNil() || v2.Len() == 0) { + return false + } + if v1.IsNil() || v1.Len() == 0 { + return true + } + } else { + if v1.IsNil() != v2.IsNil() { + return false + } } if v1.Len() != v2.Len() { return false @@ -202,7 +216,7 @@ func (e Equalities) deepValueEqual(v1, v2 reflect.Value, visited map[visit]bool, return true } for _, k := range v1.MapKeys() { - if !e.deepValueEqual(v1.MapIndex(k), v2.MapIndex(k), visited, depth+1) { + if !e.deepValueEqual(v1.MapIndex(k), v2.MapIndex(k), visited, equateNilAndEmpty, depth+1) { return false } } @@ -232,6 +246,14 @@ func (e Equalities) deepValueEqual(v1, v2 reflect.Value, visited map[visit]bool, // Unexported field members cannot be compared and will cause an informative panic; you must add an Equality // function for these types. func (e Equalities) DeepEqual(a1, a2 interface{}) bool { + return e.deepEqual(a1, a2, true) +} + +func (e Equalities) DeepEqualWithNilDifferentFromEmpty(a1, a2 interface{}) bool { + return e.deepEqual(a1, a2, false) +} + +func (e Equalities) deepEqual(a1, a2 interface{}, equateNilAndEmpty bool) bool { if a1 == nil || a2 == nil { return a1 == a2 } @@ -240,7 +262,7 @@ func (e Equalities) DeepEqual(a1, a2 interface{}) bool { if v1.Type() != v2.Type() { return false } - return e.deepValueEqual(v1, v2, make(map[visit]bool), 0) + return e.deepValueEqual(v1, v2, make(map[visit]bool), equateNilAndEmpty, 0) } func (e Equalities) deepValueDerive(v1, v2 reflect.Value, visited map[visit]bool, depth int) bool { diff --git a/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal_test.go b/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal_test.go index bcb08b27eae..6222817615c 100644 --- a/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal_test.go +++ b/staging/src/k8s.io/apimachinery/third_party/forked/golang/reflect/deep_equal_test.go @@ -16,6 +16,10 @@ func TestEqualities(t *testing.T) { type Baz struct { Y Bar } + type Zap struct { + A []int + B map[string][]int + } err := e.AddFuncs( func(a, b int) bool { return a+1 == b @@ -32,10 +36,12 @@ func TestEqualities(t *testing.T) { X int } - table := []struct { + type Case struct { a, b interface{} equal bool - }{ + } + + table := []Case{ {1, 2, true}, {2, 1, false}, {"foo", "fo", false}, @@ -70,6 +76,26 @@ func TestEqualities(t *testing.T) { t.Errorf("Expected (%+v == %+v) == %v, but got %v", item.a, item.b, e, a) } } + + // Cases which hinge upon implicit nil/empty map/slice equality + implicitTable := []Case{ + {map[string][]int{}, map[string][]int(nil), true}, + {[]int{}, []int(nil), true}, + {map[string][]int{"foo": nil}, map[string][]int{"foo": {}}, true}, + {Zap{A: nil, B: map[string][]int{"foo": nil}}, Zap{A: []int{}, B: map[string][]int{"foo": {}}}, true}, + } + + for _, item := range implicitTable { + if e, a := item.equal, e.DeepEqual(item.a, item.b); e != a { + t.Errorf("Expected (%+v == %+v) == %v, but got %v", item.a, item.b, e, a) + } + } + + for _, item := range implicitTable { + if e, a := !item.equal, e.DeepEqualWithNilDifferentFromEmpty(item.a, item.b); e != a { + t.Errorf("Expected (%+v == %+v) == %v, but got %v", item.a, item.b, e, a) + } + } } func TestDerivatives(t *testing.T) { 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 new file mode 100644 index 00000000000..74cc27fd149 --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality.go @@ -0,0 +1,109 @@ +/* +Copyright 2021 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 ( + "context" + "fmt" + "reflect" + + "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/conversion" + "k8s.io/apimachinery/pkg/runtime" +) + +var ignoreTimestampEqualities = func() conversion.Equalities { + var eqs = equality.Semantic.Copy() + + err := eqs.AddFunc( + func(a, b metav1.ManagedFieldsEntry) bool { + // Two objects' managed fields are equivalent if, ignoring timestamp, + // the objects are deeply equal. + a.Time = nil + b.Time = nil + return reflect.DeepEqual(a, b) + }, + ) + + if err != nil { + panic(err) + } + + return eqs +}() + +// IgnoreManagedFieldsTimestampsTransformer reverts timestamp updates +// if the non-managed parts of the object are equivalent +func IgnoreManagedFieldsTimestampsTransformer( + _ context.Context, + newObj runtime.Object, + oldObj runtime.Object, +) (runtime.Object, error) { + // If managedFields modulo timestamps are unchanged + // and + // rest of object is unchanged + // then + // revert any changes to timestamps in managed fields + // (to prevent spurious ResourceVersion bump) + // + // Procecure: + // Do a quicker check to see if just managed fields modulo timestamps are + // unchanged. If so, then do the full, slower check. + // + // In most cases which actually update the object, the managed fields modulo + // timestamp check will fail, and we will be able to return early. + // + // In other cases, the managed fields may be exactly the same, + // except for timestamp, but the objects are the different. This is the + // slow path which checks the full object. + oldAccessor, err := meta.Accessor(oldObj) + if err != nil { + return nil, fmt.Errorf("failed to acquire accessor for oldObj: %v", err) + } + + accessor, err := meta.Accessor(newObj) + if err != nil { + return nil, fmt.Errorf("failed to acquire accessor for newObj: %v", err) + } + + oldManagedFields := oldAccessor.GetManagedFields() + newManagedFields := accessor.GetManagedFields() + + // This condition ensures the managed fields are always compared first. If + // this check fails, the if statement will short circuit. If the check + // succeeds the slow path is taken which compares entire objects. + if ignoreTimestampEqualities.DeepEqualWithNilDifferentFromEmpty(oldManagedFields, newManagedFields) && + ignoreTimestampEqualities.DeepEqualWithNilDifferentFromEmpty(newObj, oldObj) { + + // Remove any changed timestamps, so that timestamp is not the only + // change seen by etcd. + // + // newManagedFields is known to be exactly pairwise equal to + // oldManagedFields except for timestamps. + // + // Simply replace possibly changed new timestamps with their old values. + for idx := 0; idx < len(oldManagedFields); idx++ { + newManagedFields[idx].Time = oldManagedFields[idx].Time + } + + accessor.SetManagedFields(newManagedFields) + return newObj, nil + } + return newObj, nil +} diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go index 6ee4904bf4f..50f6005ab83 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go @@ -660,7 +660,7 @@ func (p *patcher) patchResource(ctx context.Context, scope *RequestScope) (runti } wasCreated := false - p.updatedObjectInfo = rest.DefaultUpdatedObjectInfo(nil, p.applyPatch, p.applyAdmission, dedupOwnerReferencesTransformer) + p.updatedObjectInfo = rest.DefaultUpdatedObjectInfo(nil, p.applyPatch, p.applyAdmission, dedupOwnerReferencesTransformer, fieldmanager.IgnoreManagedFieldsTimestampsTransformer) requestFunc := func() (runtime.Object, error) { // Pass in UpdateOptions to override UpdateStrategy.AllowUpdateOnCreate options := patchToUpdateOptions(p.options) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go index 1c8f140175d..cb0ba5d7de8 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go @@ -191,6 +191,15 @@ func UpdateResource(r rest.Updater, scope *RequestScope, admit admission.Interfa }) } + // Ignore changes that only affect managed fields + // timestamps. FieldManager can't know about changes + // like normalized fields, defaulted fields and other + // mutations. + // Only makes sense when SSA field manager is being used + if scope.FieldManager != nil { + transformers = append(transformers, fieldmanager.IgnoreManagedFieldsTimestampsTransformer) + } + createAuthorizerAttributes := authorizer.AttributesRecord{ User: userInfo, ResourceRequest: true, diff --git a/test/integration/apiserver/apply/apply_test.go b/test/integration/apiserver/apply/apply_test.go index a59585a8c53..b206fc4c5cf 100644 --- a/test/integration/apiserver/apply/apply_test.go +++ b/test/integration/apiserver/apply/apply_test.go @@ -249,6 +249,185 @@ func TestNoOpUpdateSameResourceVersion(t *testing.T) { } } +func getRV(obj runtime.Object) (string, error) { + acc, err := meta.Accessor(obj) + if err != nil { + return "", err + } + return acc.GetResourceVersion(), nil +} + +// TestNoSemanticUpdateAppleSameResourceVersion makes sure that APPLY requests which makes no semantic changes +// will not change the resource version (no write to etcd is done) +// +// Some of the non-semantic changes are: +// - Applying an atomic struct that removes a default +// - Changing Quantity or other fields that are normalized +func TestNoSemanticUpdateApplySameResourceVersion(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, genericfeatures.ServerSideApply, true)() + + client, closeFn := setup(t) + defer closeFn() + + ssBytes := []byte(`{ + "apiVersion": "apps/v1", + "kind": "StatefulSet", + "metadata": { + "name": "nginx", + "labels": {"app": "nginx"} + }, + "spec": { + "serviceName": "nginx", + "selector": { "matchLabels": {"app": "nginx"}}, + "template": { + "metadata": { + "labels": {"app": "nginx"} + }, + "spec": { + "containers": [{ + "name": "nginx", + "image": "nginx", + "resources": { + "limits": {"memory": "2048Mi"} + } + }] + } + }, + "volumeClaimTemplates": [{ + "metadata": {"name": "nginx"}, + "spec": { + "accessModes": ["ReadWriteOnce"], + "resources": {"requests": {"storage": "1Gi"}} + } + }] + } + }`) + + obj, err := client.AppsV1().RESTClient().Patch(types.ApplyPatchType). + Namespace("default"). + Param("fieldManager", "apply_test"). + Resource("statefulsets"). + Name("nginx"). + Body(ssBytes). + Do(context.TODO()). + Get() + if err != nil { + t.Fatalf("Failed to create object: %v", err) + } + + rvCreated, err := getRV(obj) + if err != nil { + t.Fatalf("Failed to get RV: %v", err) + } + + // Sleep for one second to make sure that the times of each update operation is different. + time.Sleep(1200 * time.Millisecond) + + obj, err = client.AppsV1().RESTClient().Patch(types.ApplyPatchType). + Namespace("default"). + Param("fieldManager", "apply_test"). + Resource("statefulsets"). + Name("nginx"). + Body(ssBytes). + Do(context.TODO()). + Get() + if err != nil { + t.Fatalf("Failed to create object: %v", err) + } + rvApplied, err := getRV(obj) + if err != nil { + t.Fatalf("Failed to get RV: %v", err) + } + if rvApplied != rvCreated { + t.Fatal("ResourceVersion changed after apply") + } +} + +// TestNoSemanticUpdateAppleSameResourceVersion makes sure that PUT requests which makes no semantic changes +// will not change the resource version (no write to etcd is done) +// +// Some of the non-semantic changes are: +// - Applying an atomic struct that removes a default +// - Changing Quantity or other fields that are normalized +func TestNoSemanticUpdatePutSameResourceVersion(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, genericfeatures.ServerSideApply, true)() + + client, closeFn := setup(t) + defer closeFn() + + ssBytes := []byte(`{ + "apiVersion": "apps/v1", + "kind": "StatefulSet", + "metadata": { + "name": "nginx", + "labels": {"app": "nginx"} + }, + "spec": { + "serviceName": "nginx", + "selector": { "matchLabels": {"app": "nginx"}}, + "template": { + "metadata": { + "labels": {"app": "nginx"} + }, + "spec": { + "containers": [{ + "name": "nginx", + "image": "nginx", + "resources": { + "limits": {"memory": "2048Mi"} + } + }] + } + }, + "volumeClaimTemplates": [{ + "metadata": {"name": "nginx"}, + "spec": { + "accessModes": ["ReadWriteOnce"], + "resources": { "requests": { "storage": "1Gi"}} + } + }] + } + }`) + + obj, err := client.AppsV1().RESTClient().Post(). + Namespace("default"). + Param("fieldManager", "apply_test"). + Resource("statefulsets"). + Body(ssBytes). + Do(context.TODO()). + Get() + if err != nil { + t.Fatalf("Failed to create object: %v", err) + } + + rvCreated, err := getRV(obj) + if err != nil { + t.Fatalf("Failed to get RV: %v", err) + } + + // Sleep for one second to make sure that the times of each update operation is different. + time.Sleep(1200 * time.Millisecond) + + obj, err = client.AppsV1().RESTClient().Put(). + Namespace("default"). + Param("fieldManager", "apply_test"). + Resource("statefulsets"). + Name("nginx"). + Body(ssBytes). + Do(context.TODO()). + Get() + if err != nil { + t.Fatalf("Failed to create object: %v", err) + } + rvApplied, err := getRV(obj) + if err != nil { + t.Fatalf("Failed to get RV: %v", err) + } + if rvApplied != rvCreated { + t.Fatal("ResourceVersion changed after similar PUT") + } +} + // TestCreateOnApplyFailsWithUID makes sure that PATCH requests with the apply content type // will not create the object if it doesn't already exist and it specifies a UID func TestCreateOnApplyFailsWithUID(t *testing.T) {