KEP-4222: Decode CBOR to UnstructuredList as UnstructuredJSONScheme does. #124775
Conversation
|
Skipping CI for Draft Pull Request. |
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: benluddy The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
|
xref #123783 |
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
| @@ -156,6 +156,33 @@ func TestEncode(t *testing.T) { | |||
| } | |||
| }, | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
This was possible even before this PR, correct? So it's just additional data related to the new code IIUC
There was a problem hiding this comment.
Yes, although the behavior of the "list element missing kind", "list element missing version", and item GVK heuristic cases would have had different expectations since the original implementation had been calling SetUnstructuredContent.
staging/src/k8s.io/apimachinery/pkg/runtime/serializer/cbor/cbor_test.go
Show resolved
Hide resolved
Yes! I'm wrapping up a PR that does roundtrip comparisons vs. JSON for all of the built-in GVKs to Unstructured and back, and I have a checklist entry to add a similar test that will operate on fuzzed Unstructured/UnstructuredList. The object you get with JSON actually varies between strict and non-strict decodes (#123783). I don't think that was intentional, so I'm not carrying the behavior over to this decoder. |
| var cast bool | ||
| items, cast = uncast.([]interface{}) | ||
| if !cast { | ||
| strict, lax = nil, fmt.Errorf("items field of UnstructuredList must be encoded as an array or null if present") |
There was a problem hiding this comment.
why is strict nil and lax an error? should it be the other way around?
| items, cast = uncast.([]interface{}) | ||
| if !cast { | ||
| strict, lax = nil, fmt.Errorf("items field of UnstructuredList must be encoded as an array or null if present") | ||
| return |
There was a problem hiding this comment.
if someone is using lax, they'll get no error and also no content. What would someone do with that return? is that a useful return?
Given this is a new serializer down an error case, do we have any viable use-case for not failing and also not returning a useable list? Is it better to always fail?
There was a problem hiding this comment.
Seems like it's better to fail both lax and strict. We can't make sense of how to decode in these cases.
There was a problem hiding this comment.
I will add an explanation about these errors in the comments. It does fail in both cases. The last return value is meant to be interpreted according to the usual Go convention, and indicates that the unmarshal operation failed entirely.
The signature is roughly based on UnmarshalStrict from sigs.k8s.io/json.
| object, cast := items[i].(map[string]interface{}) | ||
| if !cast { | ||
| strict, lax = nil, fmt.Errorf("elements of the items field of UnstructuredList must be encoded as a map") | ||
| return |
There was a problem hiding this comment.
similar to the above question.
Decoding to map[string]interface{} and passing the result to UnstructuredList's
SetUnstructuredContent method does not produce objects that are identical to those produced by
UnstructuredJSONScheme's decode method. UnstructuredJSONScheme's decode:
1. removes the "items" key from the map in its Object field
2. sets "apiVersion" and "kind" (determined heuristically from the list's GVK) on elements of its
Items slice that were not serialized with a nonempty string "apiVersion" and "kind"
3. returns a missing kind error if any element is missing "kind"
benluddy
force-pushed
the
cbor-unstructuredlist
branch
from
1dc46eb
to
7e6b866
Compare
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
Decoding to map[string]interface{} and passing the result to UnstructuredList's SetUnstructuredContent method does not produce objects that are identical to those produced by UnstructuredJSONScheme's decode method. UnstructuredJSONScheme's decode:
removes the "items" key from the map in its Object field
sets "apiVersion" and "kind" (determined heuristically from the list's GVK) on elements of its Items slice that were not serialized with a nonempty string "apiVersion" and "kind"
returns a missing kind error if any element is missing "kind"
What type of PR is this?
/kind cleanup
/sig api-machinery
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: