Closed cassandracomar closed 3 years ago
That's unfortunate. It's the kind of thing I was worried about when I was making that change.
/cc @clux @MikailBag
Oof. That's not good :(
(after some double checking: command
serialization differences in 0.12 and 0.11)
So we were emitting nulls before because of option wrapping, and now we are only emitting vectors if the vector is non-empty.
Not sure if there are any feasible fixes here except reverting option unpacking? I assume we cannot simply just emit the empty vector in the omitempty
cases, because we don't have access to that annotation, and it sounds possibly scarier to emit it in the blanket case.
Will the inverse work? Always emit empty maps as {}
and empty arrays as []
?
fwiw, I tested this and uniformly emitting the empty vec/map seems to work.
On Fri, Jun 25, 2021, 5:59 PM Arnav Singh @.***> wrote:
Will the inverse work? Always emit empty maps as {} and empty arrays as [] ?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Arnavion/k8s-openapi/issues/103#issuecomment-868854047, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACOKBB4ACW5URKFD3OI7NTTUT32TANCNFSM47KKYBSQ .
Actually, won't it cause a problem for this kind of usage?
struct Foo {
bar: Vec<Bar>,
baz: String,
}
// Goal: set foo.baz without affecting foo.bar
let foo = { baz: "newbaz", ..Foo::default() };
replace_namespaced_foo(&foo);
This would've worked before #95, and still works after #95, because in both cases the request body doesn't contain the "bar"
field. But if we emit the default empty Vec
as []
, then foo
in the cluster will end up having an empty bar
shouldn't that usage properly be a PATCH
?
On Fri, Jun 25, 2021, 6:43 PM Arnav Singh @.***> wrote:
Actually, won't it cause a problem for this kind of usage?
struct Foo { bar: Vec
, baz: String, } // Goal: set foo.baz without affecting foo.barlet foo = { baz: "newbaz", ..Foo::default() };replace_namespaced_foo(&foo); This would've worked before #95 https://github.com/Arnavion/k8s-openapi/pull/95, and still works after
95 https://github.com/Arnavion/k8s-openapi/pull/95, because in both
cases the request body doesn't contain the "bar" field. But if we emit the default empty Vec as [], then foo in the cluster will end up having an empty bar
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Arnavion/k8s-openapi/issues/103#issuecomment-868868194, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACOKBECUAW3XZJPFKEXIODTUUBABANCNFSM47KKYBSQ .
Yeah, that is a PATCH
use case, but you wouldn't be able to run a patch by using Foo
's own serialization if the struct requires bar
. You'd have to use serde_json::json!{"baz": "newbaz"}
as the patch body, and rely on the fact that bar
is actually optional on the server side.
Not sure that's a problem though. If you have the full struct Foo
and you are doing a full replace
, then you should have all the values, and patch with the full struct should probably be interpreted as: change all values.
A lot of the "change parts of the spec" type patch operations need to use json!
or json_patch
already.
Okay, I thought replace_
(PUT) allows you to specify a subset of the fields, but yes I was misremembering.
Unfortunately, always emitting []
/ {}
breaks at least the CRD test, specifically creating a CRD spec with a CustomResourceValidation
:
... because it ends up serializing the JSONSchemaProps::dependencies
map as {}
, which the API server rejects:
only [Description Type Format Title Maximum ExclusiveMaximum Minimum ExclusiveMinimum MaxLength MinLength Pattern MaxItems MinItems UniqueItems MultipleOf Required Items Properties ExternalDocs Example XPreserveUnknownFields] fields are allowed at the root of the schema if the status subresource is enabled, spec.validation.openAPIV3Schema.dependencies: Forbidden: dependencies is not supported, spec.validation.openAPIV3Schema.properties[spec].dependencies: Forbidden: dependencies is not supported, spec.validation.openAPIV3Schema.properties[spec].properties[prop3].dependencies: Forbidden: dependencies is not supported, spec.validation.openAPIV3Schema.properties[spec].properties[prop1].dependencies: Forbidden: dependencies is not supported, spec.validation.openAPIV3Schema.properties[spec].properties[prop2].dependencies: Forbidden: dependencies is not supported, spec.validation.openAPIV3Schema.properties[spec].properties[prop2].items.dependencies: Forbidden: dependencies is not supported
Indeed, the API server always rejects the spec if this field is set: https://github.com/kubernetes/apiextensions-apiserver/blob/92e6c85f057fe3cc1c2d6ce0ef0b43c9b6b0c142/pkg/apis/apiextensions/validation/validation.go#L1037
part of the difficulty is that PUT in fact does ignore skipped/elided fields, which is what produces the behavior described in this issue. so people may be relying on this behavior despite the fact that it doesn't match the documentation for update/replace operations.
On Sat, Jun 26, 2021 at 2:28 PM Arnav Singh @.***> wrote:
Okay, I thought replace_ (PUT) allows you to specify a subset of the fields, but yes I was misremembering.
Unfortunately, always emitting [] / {} breaks at least the CRD test, specifically creating a CRD spec with a CustomResourceValidation:
... because it ends up serializing the JSONSchemaProps::dependencies map as {}, which the API server rejects:
only [Description Type Format Title Maximum ExclusiveMaximum Minimum ExclusiveMinimum MaxLength MinLength Pattern MaxItems MinItems UniqueItems MultipleOf Required Items Properties ExternalDocs Example XPreserveUnknownFields] fields are allowed at the root of the schema if the status subresource is enabled, spec.validation.openAPIV3Schema.dependencies: Forbidden: dependencies is not supported, spec.validation.openAPIV3Schema.properties[spec].dependencies: Forbidden: dependencies is not supported, spec.validation.openAPIV3Schema.properties[spec].properties[prop3].dependencies: Forbidden: dependencies is not supported, spec.validation.openAPIV3Schema.properties[spec].properties[prop1].dependencies: Forbidden: dependencies is not supported, spec.validation.openAPIV3Schema.properties[spec].properties[prop2].dependencies: Forbidden: dependencies is not supported, spec.validation.openAPIV3Schema.properties[spec].properties[prop2].items.dependencies: Forbidden: dependencies is not supported
Indeed, the API server always rejects the spec if this field is set: https://github.com/kubernetes/apiextensions-apiserver/blob/92e6c85f057fe3cc1c2d6ce0ef0b43c9b6b0c142/pkg/apis/apiextensions/validation/validation.go#L1037
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Arnavion/k8s-openapi/issues/103#issuecomment-869043291, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACOKBGDD6NHP7QKZKPG7CDTUYL35ANCNFSM47KKYBSQ .
So there's no way other than reverting to the pre-#95 behavior?
I can't think of any way to do it. The annotations that would let us do this safely (for a subset of vecs and btreemaps) are in the api repo and not in the schema. Unless we did some kind of first-pass of the api repo to create a file of specific whitelist paths or something crazy.
I think it's probably smarter to revert it for now until/if we can come up with a way to partially enable it.
noticed this with
Container::command
in an integration test that setscommand: ["false"]
to force aPod
down then attempts to restorecommand
back to the default by unsetting it. as serialization skips the updatedcommand
, thePUT
operation does not updatecommand
back to the empty list.I believe this affects all fields updated by #95.
in order to allow fields to be restored to default, we either need to emit the empty list or emit an explicit
null
. foromitempty
container types, these should be equivalent. I've asked on the k8s slack about this to determine if this is expected behavior from upstream.