Never try to update read-only, system-populated metadata fields. #94
Conversation
controller/common/manage_children.go
Outdated
| if found { | ||
| unstructured.SetNestedField(newObj.UnstructuredContent(), val, "metadata", fieldName) | ||
| } else { | ||
| unstructured.RemoveNestedField(newObj.UnstructuredContent(), "metadata", fieldName) |
There was a problem hiding this comment.
To verify, we have to omit a field that cannot be found via NestedFieldNoCopy because user has added the read-only field, but we don't want to pick up the change. Am I right?
There was a problem hiding this comment.
Right. The goal is for the state of these system fields (set vs unset, and value if set) to exactly match the "before" object, so they cannot trigger a diff during DeepEqual. I'll add some more comments to record this.
controller/common/manage_children.go
Outdated
| return err | ||
| } | ||
| if found { | ||
| unstructured.SetNestedField(newObj.UnstructuredContent(), val, "metadata", fieldName) |
There was a problem hiding this comment.
To verify, SetNestedField covers both cases of field value change and adding back removed field. Am I right?
There was a problem hiding this comment.
Yes. I'll add a comment to record that expectation. The unit test confirms it with the case "uid": "should bring back removed value".
| dynamicapply.SetLastApplied(newObj, update.UnstructuredContent()) | ||
| return newObj, nil | ||
| } | ||
|
|
||
| // objectMetaSystemFields is a list of JSON field names within ObjectMeta that |
There was a problem hiding this comment.
Should we put this to a global file?
There was a problem hiding this comment.
What do you mean by a global file? I put it here for now because I like to keep things close to where they're being used if they're only used once. We can always move it later.
I looked around in Kubernetes itself to see if there was any existing list or code to revert ObjectMeta system fields, but I didn't find anything reusable. It looks like they just hard-coded the steps to set the fields in various places.
There was a problem hiding this comment.
Oh, i mean a file that records all hard-coded attributes/structs. +1 on keeping things close if they are only used once.
|
In the long run, we may still have to fix the |
If the user asks us to update them, we will silently ignore the changes. The API server would ignore the changes anyway, but we need to account for that in advance because otherwise we'll either send needless no-op updates (for InPlace strategies) or needlessly delete and recreate objects (for Recreate strategies).
enisoc
force-pushed
the
recreate-hotloop
branch
from
d6cc4fe
to
8f84386
Compare
|
/lgtm |
If the user asks us to update them, we will silently ignore the changes. The API server would ignore the changes anyway, but we need to account for that in advance because otherwise we'll either send needless no-op updates (for InPlace strategies) or needlessly delete and recreate objects (for Recreate strategies).
Fixes #76