KEP-5051: Add Server Side Apply: Unsetting Fields KEP

[jpbetz]

KEP for #5051

This would fix a gap in Server Side Apply functionality that we have been aware of since Server Side Apply became GA.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• keps/prod-readiness/OWNERS [jpbetz]
• keps/sig-api-machinery/OWNERS [jpbetz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jpbetz]

/assign @sttts

This is ready for early feedback. I'll have a draft implementation out next week.

cc @deads2k

[sftim]

/retitle KEP-5051: Add Server Side Apply: Unsetting Fields KEP

[sftim]

Maybe {k8s_io__value: unset} is only exposed on the wire.
Do we want to accept {k8s_io__value: unset} in manifest files, or should tools such as kubectl balk on encountering it?

[yongruilin]

Though it is stated above that MutatingAdmissionPolicy won't graduate to Beta before UnsettingField, would it still introduce some risk that falsely allowing unset field markers when UnsettingField feature is disabled?

And, OOC, is it normal to handle relation between features like this(have a graduate dependency)?

[jpbetz]

Though it is stated above that MutatingAdmissionPolicy won't graduate to Beta before UnsettingField, would it still introduce some risk that falsely allowing unset field markers when UnsettingField feature is disabled?

We intend to unconditionally allow MutatingAdmissionPolicy to use unsetting fields markers when the MutatingAdmissionPolicy feature gate is enabled. The graduation restriction is purely to ensure that the unsetting fields is not being by a feature that claims a stability level above the stability that we claim unsetting fields is at.

While we could configure MutatingAdmissionPolicy to not support unsetting fields unless both the MutatingAdmissionPolicy and UnsettingField feature gates are enabled, we don't really need to. What we're effectively doing is saying "MutatingAdmissionPolicy has unsetting field support. We making it an intrinsic part of the feature in alpha. Can we guard against any risk this might cause by restricting MutatingAdmissionPolicy promotion to not progress beyond UnsettingField"

And, OOC, is it normal to handle relation between features like this(have a graduate dependency)?

Yes, we've go graduation dependencies between other KEPs. It's not uncommon.

[jpbetz]

Maybe {k8s_io__value: unset} is only exposed on the wire. Do we want to accept {k8s_io__value: unset} in manifest files, or should tools such as kubectl balk on encountering it?

For manifests, I'm not 100% decided. I'm leaning toward not allowing {k8s_io__value: unset}, but I'd like to mull it over a bit more. OK if I add a line item to the beta graduation like "We decide if the marker values should be allowed in CREATE/UPDATE manifests, and stripped out, or if we will not allow marker values in manifests." for now and discuss it more in 1.34?

For apply configurations we do want to allow users to put {k8s_io__value: unset} in yaml files and have kubectl allow it. I'll need to do some research on all the ways kubectl validates on the client today to make sure this work as expected. I'll add a beta criteria line item for this as well.

[jpbetz]

Thanks @sftim @aaron-prindle @yongruilin! Feedback applied

[sftim]

OK if I add a line item to the beta graduation like "We decide if the marker values should be allowed in CREATE/UPDATE manifests, and stripped out, or if we will not allow marker values in manifests." for now and discuss it more in 1.34?

I think that's fine, but maybe we want to feature-gate kubectl or client-go so you have to really opt in to it working.

[sftim]

Some nits

[sftim]

The big question in my mind is still:
If I have a manifest and kubectl and I want to create an object that looks like my manifest, what command can I use to do that?

Because apparently manifests can't have field unset markers in but an apply configuration supplied to kubectl can, and I think we need to do more planning about this aspect. Enough that we don't regret the lack of it.

[jpbetz]

The big question in my mind is still: If I have a manifest and kubectl and I want to create an object that looks like my manifest, what command can I use to do that?

Because apparently manifests can't have field unset markers in but an apply configuration supplied to kubectl can, and I think we need to do more planning about this aspect. Enough that we don't regret the lack of it.

I don't think it's as bad as it might look at first glance. Using a fully populated apply configuration (that could be also used as a manifest) that could be used to create a resource, together with {k8s_io__value: unset} is not something we expect users to need to do because they are the owner of the spec and so if they do not want a field set, it's sufficient to just omit it from the manifest. Controllers force apply, so owning the unset field doesn't really benefit such users. {k8s_io__value: unset} is almost exclusively useful for control loops that are reconciling resources and in CEL admission to represent mutations that clear fields based on policy.

But if we do end up needing something. I don't think we're completely painted into a corner. We could teach kubectl to remove the markers or we could even pre-process them out server side.

[jpbetz]

OK if I add a line item to the beta graduation like "We decide if the marker values should be allowed in CREATE/UPDATE manifests, and stripped out, or if we will not allow marker values in manifests." for now and discuss it more in 1.34?

I think that's fine, but maybe we want to feature-gate kubectl or client-go so you have to really opt in to it working.

I agree. I'll track this in the beta graduation criteria section.

[jpbetz]

Thanks @sttts @sftim feedback applied

[deads2k]

Some questions. One potentially significant. I think the rest are easy.

[deads2k]

when encoded in JSON or CBOR, this will be a string containing this? Actually what format is this? it's not quite JSON.

[sftim]

This looks like YAML to me. I think the JSON would be something like:

  "spec": {
    "field": {
      "k8s_io__value": "unset"
    },

[jpbetz]

That's right. I could switch this example to be more idiomatic YAML if that helps:

spec:
  field:
    k8s_io__value: unset

[deads2k]

This looks like YAML to me. I think the JSON would be something like:

Oh, my eye didn't read it that way. That means that it is logically impossible to escape the value for a piece of say integer configuration exposed from one resource to set a value in another. For instance, a controller of deployments would be unable to express "must be empty" in its API.

That seems really problematic.

[deads2k]

An API that sets a value in another API seems like a natural thing to want. This happens when stacking configuration APIs for instance

1. configuration API for operator/A has field/X
2. operator/A is wrapped by operator/B along with others to act as a unit
3. operator/B must expose a way to configure the value of field/X and does so in field/Y
4. For a user to express "affirmatively unset field/X", they need to set field/Y to the string value "{k8s_io__value: unset}"

Perhaps there's another way to accomplish this?

[jpbetz]

Unescaping __k8s_io__value to k8s_io__value might not be that bad in the current implementation. Okay if I make this a beta graduation criteria? (It seems valuable for production grade but not essential for proving the idea at the alpha level?)

[deads2k]

Unescaping __k8s_io__value to k8s_io__value might not be that bad in the current implementation. Okay if I make this a beta graduation criteria? (It seems valuable for production grade but not essential for proving the idea at the alpha level?)

Must be present before going beta, but yes I think we could try alpha without it.

[jpbetz]

👍 I've almost got it working, I'll target this for the 1st alpha, possibly as a separate PR.

[deads2k]

PRR looks good, concept seems ok, but I'm struggling with whether the escaping can work for scalar values like string or int when embedding one schema in another. I think it might not be possible and I'm torn as to whether that makes the feature inconsistent enough to avoid doing it.

Taking a more concrete example, maybe something like a crd/MultiDeploymentManager that has slice .spec.deploymentTemplates DeploymentSpec. A controller reads the .spec.deploymentTemplates and applies the deployments. For some reason, the author wants to express, "multiDeploymentManager/foo's .spec.deploymentTemplates[0].replicas must be not set". I don't see how we could store that value in the custom resource multiDeploymentManager/foo.

[jpbetz]

PRR looks good, concept seems ok, but I'm struggling with whether the escaping can work for scalar values like string or int when embedding one schema in another. I think it might not be possible and I'm torn as to whether that makes the feature inconsistent enough to avoid doing it.

Taking a more concrete example, maybe something like a crd/MultiDeploymentManager that has slice .spec.deploymentTemplates DeploymentSpec. A controller reads the .spec.deploymentTemplates and applies the deployments. For some reason, the author wants to express, "multiDeploymentManager/foo's .spec.deploymentTemplates[0].replicas must be not set". I don't see how we could store that value in the custom resource multiDeploymentManager/foo.

EDIT: See d2acb44 (#5052) Is this sufficient?

[jpbetz]

An alternative to using a marker as a substitute for values would be to use a field name prefix. So if the fields is "replicas", then setting "(marker-prefix)_replicas" would unset the field. For list maps, a field insider the entry would need to be used.

This would be slightly less painful to use in CRDs because all fields have a single, well defined type.

If this is more acceptable I could go that way. Problem is that I'm unwell at the moment so I don't know if I can get a KEP update by Thursday.

EDIT: d2acb44 (#5052) proposes this.

cc @sttts

[deads2k]

We're going to hold on this. Mutating Admission Policy can proceed without unsetting in the SSA section (we will continue to have the jsonpatch capability). Since we must be compatible with existing manifests, adding this capability afterwards will mean we keep cruft (the jsonpatch deletion), but we can actually add it.

For future-us, the embeddability discussion is where this got stuck. Whether it's possible and whether it's necessary