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

StatefulSetSpec::default() already has a selector and serviceName when serializing #112

Open
nightkr opened this issue Nov 18, 2021 · 3 comments

Comments

@nightkr
Copy link
Contributor

nightkr commented Nov 18, 2021

Repro

use k8s_openapi::api::apps::v1::StatefulSetSpec;
println!("{}", serde_yaml::to_string(&StatefulSetSpec::default()).unwrap());

Expected

---
template: {}

Produced

---
selector: {}
serviceName: ""
template: {}

Why is this a problem?

LabelSelector and string are both atomic (see in curl $K8S_API/openapi/v2 | jq '.definitions."io.k8s.apimachinery.pkg.apis.meta.v1.LabelSelector"' that "x-kubernetes-map-type": "atomic"), which means that server-side apply will replace them outright rather than trying to merge them. This prevents server-side apply from updating StatefulSet objects safely.

Versions

[[package]]
name = "k8s-openapi"
version = "0.13.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "4f8de9873b904e74b3533f77493731ee26742418077503683db44e1b3c54aa5c"
dependencies = [
 "base64",
 "bytes",
 "chrono",
 "serde",
 "serde-value",
 "serde_json",
]
@nightkr nightkr changed the title StatefulSetSpec::default() already has a selector and serviceName when serializing StatefulSetSpec::default() already has a selector and serviceName when serializing Nov 18, 2021
@nightkr
Copy link
Contributor Author

nightkr commented Nov 18, 2021

I think this comes down to a larger issue about when things are Optional, and how we handle patches. Currently, (AFAIK) the policy is that all properties that are not required (in the OpenAPI) are wrapped in Option, which makes some sense for standard CRUD operations.

However, this makes less sense for patches, where almost no field is statically required since a missing value just means that it falls through to the old value.

On the other hand, non-atomic values (objects that are not explicitly tagged as atomic, as well as list-maps) don't have much of a point in being Optional either, since a missing value is essentially equivalent to an empty value ({} or [], respectively).


Maybe we will end up needing separate types to handle CRUD vs patch properly (either completely separate like Go handles it, or parametrized with some type mode mechanism). That seems unfortunate (both in terms of compile time and teachability), but I'm not sure I see a way around it... :/

@clux
Copy link

clux commented Nov 18, 2021

Don't know if you've seen, but this is something they had to tackle on the go side as well with Apply* variants of all the structs. Basically an entirely parallel world of structs where everything is optional to get around this problem for server side apply.

Stuff for it is under KEP 2155 and some of my notes under kube-rs/kube#649

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants