Drop unrecognized fields before update

[cmurphy]

Add a nested store to the proxy store to strip non-Kubernetes fields from the object being updated.

The steve formatter and proxy store adds fields to objects when it outputs them to the client, for usability by the UI. It adds the object's fields[1], relationships to other objects[2], a summary of the object's state[3], and additional information in the conditions[4]. These fields are not native to Kubernetes, so when a client submits the object back as an update, Kubernetes reports a warning that they are unrecognized. This change ensures the extra fields are removed before submitting the update.

The store drops the entire status field before updating. Steve stores only support client-go's Update and not UpdateStatus[5], so updating the status has no effect anyway.
[1]

steve/pkg/stores/proxy/proxy_store.go

Line 189 in bf2e965

data.PutValue(object, cells, "metadata", "fields")

[2]

steve/pkg/resources/common/formatter.go

Line 106 in bf2e965

data.PutValue(unstr.Object, rel, "metadata", "relationships")

[3]

steve/pkg/resources/common/formatter.go

Line 100 in bf2e965

data.PutValue(unstr.Object, map[string]interface{}{

[4]

steve/pkg/resources/common/formatter.go

Line 108 in bf2e965

summary.NormalizeConditions(unstr)

[5] https://pkg.go.dev/k8s.io/client-go/dynamic#ResourceInterface

rancher/rancher#41772

[KevinJoiner]

I was under the impression that the Update call from ResourceInterface includes the Status since it is a part of the object. And the UpdateStatus call is used to update the status only.

If this is not true and Update calls to K8s will not update the status even if it differs from the stored object then this should be dropped.

[cmurphy]

I don't think that's the case. Here's what the docs say:

PUT/POST/PATCH requests to the custom resource ignore changes to the status stanza.

Here's a brief reproducer showing this: https://gist.github.com/cmurphy/1512fa9482f8eba5cfed352f5d809894

Even though the status isn't updated, the API server still validates its fields, which is why we see the warnings about .status.conditions in the response in steve. So the status object still needs to be valid even though it's not being used.

[KevinJoiner]

Thanks for the example! It was easy to run and worked exactly as you said. I didn't know about that behavior before this. Though I did notice that the behavior depends on status being defined as a subresource in the CRD definition. If I removed the status subresource line in crds.yaml, then changes to that status field persisted which concerns me because some of our resources such as v3.User do not have status defined as a subresource`

apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  creationTimestamp: "2023-06-16T14:15:37Z"
  generation: 1
  name: users.management.cattle.io
  resourceVersion: "743"
  uid: 06aa895c-bfbf-4f71-8a4f-43e31cd2bfcd
spec:
  conversion:
    strategy: None
  group: management.cattle.io
  names:
    kind: User
    listKind: UserList
    plural: users
    singular: user
  scope: Cluster
  versions:
  - name: v3
    schema:
      openAPIV3Schema:
        type: object
        x-kubernetes-preserve-unknown-fields: true
    served: true
    storage: true

[cmurphy]

Thanks, that's unfortunate that our CRDs don't conform to k8s standards. I'll rework this to drop the individual condition fields in the condition slice.

[KevinJoiner]

We may be able to fix most of these with our crd generation changes.

[maxsokolovsky]

It's always good to start a test with t.Parallel() and also run subtests in parallel, unless there is global state.

for _, test := range tests {
	test := test
	t.Run(test.name, func(t *testing.T) {
		t.Parallel()

[cmurphy]

done

[maxsokolovsky]

Nit: the test itself can also be run in parallel:

Suggested change

	tests := []struct {
	func TestUnformat(t *testing.T) {
	t.Parallel()

[maxsokolovsky]

Would it be better if this tested the exported method Update on the type instead?
unformat is just an implementation detail and might change, while we want to be certain that the public interface works as expected.

[cmurphy]

In this case I don't find a lot of value in it. I would have to implement a mock store to hold the underlying types.Store, so it would be adding a lot of boilerplate to test the entire .Update() flow, when all that really matters is this one function.

[maxsokolovsky]

Makes sense. I just thought there might already be a simple mock store for testing...