Repository: kubernetes/kubernetes (language: go)

Issue:
Server-side apply bumps resourceVersion when re-applying the same object
### What happened?

When continuously re-applying the same object via a controller or kubectl with SSA the resourceVersion increases every time.

### What did you expect to happen?

I expected the resourceVersion to stay the same if the client always sends the same object via SSA, especially if the  resulting object doesn't change (apart from resourceVersion & managedFields, but these are managed by the apiserver).

### How can we reproduce it (as minimally and precisely as possible)?

Apply this CRD: 

```bash
kubectl apply -f ./crd_machines.yaml
```

<details>
<summary>crd_machines.yaml</summary>

```yaml
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  name: machines.cluster.x-k8s.io
spec:
  group: cluster.x-k8s.io
  names:
    kind: Machine
    listKind: MachineList
    plural: machines
    singular: machine
  scope: Namespaced
  versions:
  - name: v1beta1
    schema:
      openAPIV3Schema:
        properties:
          apiVersion:
            type: string
          kind:
            type: string
          metadata:
            type: object
          spec:
            properties:
             infrastructureRef:
               properties:
                 name:
                   type: string
                 namespace:
                   type: string
                   default: default-namespace
               type: object
               x-kubernetes-map-type: atomic
            type: object
        type: object
    served: true
    storage: true
```

</details>

Apply this CR:

```bash
kubectl apply -f ./cr_machine.yaml --server-side=true --field-manager=test-manager
```

<details>
<summary>cr_machine.yaml</summary>

```yaml
apiVersion: cluster.x-k8s.io/v1beta1
kind: Machine
metadata:
  name: machine-1
  namespace: default
spec:
  infrastructureRef:
    name: infrastructure-machine-1
```

</details>

Get state of the CR after first apply
```bash
k get machine -o yaml --show-managed-fields > /tmp/machine-1.yaml
```

Apply the CR again
```bash
kubectl apply -f ./cr_machine.yaml --server-side=true --field-manager=test-manager
```

Get state of the CR after second apply
```bash
k get machine -o yaml --show-managed-fields > /tmp/machine-2.yaml
```

Diff
```bash
diff /tmp/machine-1.yaml /tmp/machine-2.yaml
```

```bash
16c16
<       time: "2024-04-29T09:39:07Z"
---
>       time: "2024-04-29T09:39:40Z"
19c19
<     resourceVersion: "1167"
---
>     resourceVersion: "1209"
```

=> The resourceVersion and the "time" field of the managedField entry of the "test-manager" increases.




### Anything else we need to know?

The use case we have is a lot more complex than this simple scenario. The tl;dr is that we want to implement caching for our SSA calls, so that we don't have SSA calls on every reconcile of our controller if nothing actually changes. This is relatively hard to do if the resourceVersion increases after every apply.


I took a closer look at the APIserver and ~ the following happens:

1. the "incoming" SSA patch is applied over the live object from etcd
    1. this leads to a change because `infrastructureRef` has `x-kubernetes-map-type: atomic` which means the `namespace` field will be dropped
    2. Because of that change the "time" field in the managedField entry gets set to "time.Now()" [here](https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/util/managedfields/internal/managedfieldsupdater.go#L76)
    3. later during admission the namespace field gets defaulted again (I also tried this with a mutating webhook, same result)

On a high-level:
* client always applies the same object
* defaulting always results in the same object
* resourceVersion and managedFields[].time continuously go up

I think there are many similar cases that would lead to this behavior. Basically whenever the admission chain mutates a field previously set via SSA.

Notes:
* This only happens if there is >1s between each apply, otherwise the managedField[].time is identical and then the resourceVersion stays the same (because we then don't write to etcd)

### Kubernetes version

<details>

```console
$  kubectl version
Client Version: v1.29.0
Kustomize Version: v5.0.4-0.20230601165947-6ce0bf390ce3
Server Version: v1.29.2
```

I also tried this with client-go v0.30 and kube-apiserver v1.30. Same result.

</details>


### OS version

<details>

Running this natively on Apple Silicon M2. But can't imagine this makes a difference. Also got the same results in Docker Desktop on Mac.

</details>



Discussion / hints:
(cc @fabriziopandini @chrischdi @alvaroaleman @vincepri, might be interesting for you)
/sig api-machinery
It looks less a bug but more of a intended behavior to me.
/cc @jpbetz 
/triage accepted
Bypassing the writes triggered by noop applies would break storage version migration (https://github.com/kubernetes/kubernetes/pull/123344), so we're committed to supporting this behavior as a feature.
@cici37  @jpbetz I am confused. Are you saying this is intended behavior for a NOP SSA apply to increment resourceVersion?

This seems to not actually happen in all cases, only for certain objects/edge cases (see https://github.com/kubernetes/kubernetes/issues/121404).

For example, try this:

```
for i in {0..1000}; do
cat <<EOF | kubectl apply -f - --server-side
apiVersion: v1
kind: ConfigMap
metadata:
  name: game-demo
data:
  player_initial_lives: "3"
  ui_properties_file_name: "user-interface.properties"
EOF
sleep .5
done
```

No resourceVersion is incremented above).

Surprisingly I couldn't find any clear statement that SSA doesn't increment the resource version, but https://kubernetes.io/blog/2022/10/20/advanced-server-side-apply/ does say "a no-op apply is not very much more work for the API server than an extra GET" which seems impossible if resourceVersion is incremented
https://github.com/kubernetes/kubernetes/pull/123344/files#r1506112409 is probably only updating it because either the fieldManager changes or in the patch body the apiVersion is different (or both, perhaps)?
> https://github.com/kubernetes/kubernetes/pull/123344/files#r1506112409 is probably only updating it because either the fieldManager changes or in the patch body the apiVersion is different (or both, perhaps)?

We need this in storage version migration to make sure we correctly handles the CRD schema changes. See Joe's comments here - https://github.com/kubernetes/kubernetes/pull/123344/files#r1508023986

If it helps to understand this better, we have tests to validate this. https://github.com/kubernetes/kubernetes/blob/master/test/integration/storageversionmigrator/storageversionmigrator_test.go
> I am confused. Are you saying this is intended behavior for a NOP SSA apply to increment resourceVersion?

I may have misspoke.  When storage versions changes, a NOP SSA apply will trigger a resourceVersion bump.  If the storage version is unchanged and an update would be a NOP for etcd, I believe we short-circuit the write ([here I think](https://github.com/kubernetes/kubernetes/blob/9f8273a5c5d6d087c0839145616c828ab9a62f60/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go#L499)?).  This might explain the configMap case.

For the reported issue:

We've seen cases like this before where the time gets bumped causing a NOP to become a write. This could be improved.  Note that this isn't technically a bug because the API server is allowed to bump the resourceVersion for any write (the discussion about storage version changes causing this is a good example of where this can happen in a way that the caller wouldn't expect).  But I'd like to see the etcd write optimized away for cases like this.
Sounds similar to https://github.com/kubernetes/kubernetes/issues/121404 perhaps which had some investigation. 

FWIW as a controller author my expectation is that repeatedly applying does not repeatedly trigger resourceVersion increments. It's fine if it happens rarely (storage version change, etc) but if it happens consistently that means our own writes re-trigger reconciliation and we have an infinite loop of writes. 

AFAIK all the marketing around writing SSA controllers has been that you just apply what you want and don't try to do a get+diff+apply. Even if we did want to do that, it doesn't really work well currently (https://github.com/kubernetes/kubernetes/issues/115563). If we aren't able to rely on NOP writes (usually) being NOPs, I don't see how we are supposed to use SSA to write a controller at all? 
> but if it happens consistently that means our own writes re-trigger reconciliation and we have an infinite loop of writes.

This is a really good point. I agree.
https://github.com/kubernetes/kubernetes/pull/106388 looks like it was intended fix this sort of problem. I'm looking into why it doesn't work for this case.
https://github.com/kubernetes/kubernetes/blob/a9a1cdbd1531df06e27029686669a8fea99d44e8/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/equality.go#L55 doesn't work for CRs since the managed fields are inside of a unstructured object and are not typed as `ManagedFieldsEntry` (for the slow path equality check in IgnoreManagedFieldsTimestampsTransformer).



Fix patch:
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 a1f27f1d106e4..e68e9e02ac18b 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
@@ -28,6 +28,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/conversion"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apiserver/pkg/endpoints/metrics"
@@ -51,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.
@@ -59,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 {
@@ -70,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(


Test patch:
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 0000000000000..68b786f85b6e0
--- /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 71a5e2fad4641..4ac048e7974dd 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,
+		)
+	}
+}