Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle preserve-unknown-fields #31

Closed
clux opened this issue Feb 8, 2022 · 4 comments · Fixed by #77
Closed

handle preserve-unknown-fields #31

clux opened this issue Feb 8, 2022 · 4 comments · Fixed by #77
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@clux
Copy link
Member

clux commented Feb 8, 2022

via https://twitter.com/matteojoliveau/status/1490747666707496961

A CRD that sets x-kubernetes-preserve-unknown-fields can effectively put whatever they want into the schema without specifying it in the schema.

Currently, kopium does not generate anything for this CRD, and this property is not handled at all.

We should handle the property, and where it is seen, it should include an arbitrary json value that can be flattened into the struct as a member:

struct FooSpecWithPreserveProperty {
    #[serde(flatten)]
    pub unknowns: serde_json::Value,
}

This is probably not that bad to for a new contributor to try out as it's mostly just work in the analyzer.
In particular, it requires more logic in the initial type examination to handle the extra property, but probably a bit subtlety since we are not actually examining a member to get the this member and type: we get it from the object instead (if the object has the property, we need this extra overflow type that always looks like the above).

It also needs a test crd, maybe the image.kpack.io thing from the tweet if we can find it, all i can find is a doc spec for it but not the crd.

@alex-hunt-materialize
Copy link
Contributor

This is especially relevant for things that expect arbitrary labels, like the very common matchLabels property.

                  matchLabels:
                    type: object
                    x-kubernetes-preserve-unknown-fields: true

This currently creates an empty struct.

Complete CRD is available at https://gist.github.com/alex-hunt-materialize/2743b1e2e58a49c4df0a11ecb39f46ab (generated by the Linkerd 2.11 helm chart).

@alex-hunt-materialize
Copy link
Contributor

alex-hunt-materialize commented Apr 1, 2022

Would it potentially make sense to have separate handling for "simple" cases like the above, where there are no named properties of the object?

In this case, it would make sense to have it be a BTreeMap<String, serde_json::Value> rather than a separate struct (really a BTreeMap<String, String>, but nothing in the CRD actually tells us that).

I suspect that the majority of x-kubernetes-preserve-unknown-fields: true use cases are "simple" like that, and this would avoid the need to mess with serde(flatten) or to have additional struct nesting for the client.

#42

@clux
Copy link
Member Author

clux commented Apr 2, 2022

Yeah, I think you are right. The common map case is the more important one here, and we can leave the degenerate case for another issue (because it's a bit hairy, we can't have the top level spec type be a BTreeMap because we can't impl on that)

Incidentally, we had a similar question on how to proceed with the basic case kube-rs/kube#844 (comment) (for now we only did the map case).

@clux
Copy link
Member Author

clux commented Apr 17, 2022

Another case that needs to be handled: status objects. The istio destinationrule crd (now included) defines this status object:

          status:
            type: object
            x-kubernetes-preserve-unknown-fields: true

which fails because it sets #[kube(status = "DestinationRuleStatus")] so it also needs some trivial wrapper struct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants