skip_serializing_if on omitempty container types makes PUT operations not update set fields back to empty

[cassandracomar]

noticed this with Container::command in an integration test that sets command: ["false"] to force a Pod down then attempts to restore command back to the default by unsetting it. as serialization skips the updated command, the PUT operation does not update command 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. for omitempty container types, these should be equivalent. I've asked on the k8s slack about this to determine if this is expected behavior from upstream.

[Arnavion]

That's unfortunate. It's the kind of thing I was worried about when I was making that change.

/cc @clux @MikailBag

[clux]

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.

[Arnavion]

Will the inverse work? Always emit empty maps as {} and empty arrays as [] ?

[cassandracomar]

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 <#103 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACOKBB4ACW5URKFD3OI7NTTUT32TANCNFSM47KKYBSQ> .

[Arnavion]

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

[cassandracomar]

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<Bar>, 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 <#95>, and still works after #95 <#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 <#103 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACOKBECUAW3XZJPFKEXIODTUUBABANCNFSM47KKYBSQ> .

[clux]

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.

[Arnavion]

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:

k8s-openapi/k8s-openapi-tests/src/custom_resource_definition.rs

Lines 201 to 204 in 445e89e

	let (request, response_body) =
	apiextensions::CustomResourceDefinition::create_custom_resource_definition(&custom_resource_definition, Default::default())
	.expect("couldn't create custom resource definition");
	let response = client.execute(request);

... 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

[cassandracomar]

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: https://github.com/Arnavion/k8s-openapi/blob/445e89ec444ebb1c68e61361e64eec4c4a3f4785/k8s-openapi-tests/src/custom_resource_definition.rs#L201-L204 ... 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 <#103 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACOKBGDD6NHP7QKZKPG7CDTUYL35ANCNFSM47KKYBSQ> .

[Arnavion]

So there's no way other than reverting to the pre-#95 behavior?

[clux]

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.