sig-api-machinery KEP: Defaulting for Custom Resources

[sttts]

This topic is unblocked by #1002.

[sttts]

/assign @deads2k @lavalamp @liggitt @mbohlool @apelisse

[apelisse]

Thanks for writing this Stefan. I'd just really like native types and CRD to look and feel very similar. You're not mentioning how kubebuilder would integrate with this feature (I think that could be useful).

[sttts]

@apelisse added a sentence about kubebuilder. It just needs another tag to define the default. Should be super straight forward. /cc @DirectXMan12

[DirectXMan12]

couple of comments inline, otherwise big 👍 from me

[sttts]

@DirectXMan12 addressed your comment.

[liggitt]

would it be practicable to validate the default value at CRD create/update time? If possible, I'd like to avoid persisting fundamentally flawed CRDs

[sttts]

With structural schemas, yes. We can validate them in advance.

[sttts]

More precisely we can verify the types, not value validation. So there will always be a chance that the default does not fullfil the later. But as both validation and defaults are under control of the same party, we should be fine.

[sttts]

added

[liggitt]

if we anticipate this field being able to be populated by default in 1.16, we must not fail validation if we encounter this in existing objects. we would need to follow a process similar to https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#alpha-field-in-existing-api-version

Since CRD validation has already been rejecting this field, the only tweak would be "Before persisting the object to storage, reject the disabled alpha field on create, and on update if the existing object does not already have a value in the field."

The important thing is that we allow/preserve data in the Default field on update if the existing object had data in that field, even if the alpha feature gate is disabled

[sttts]

sounds good

[sttts]

added ratcheting validaton

[liggitt]

I went looking for the footnote...
...
it's been a long day

[liggitt]

I hard a hard time telling from the diagram, what about after reading the response from a conversion webhook?

[sttts]

Like for native types: no defaulting after conversion.

[liggitt]

what is persisted into etcd in this scenario?

• crd has two versions (v1, v2)
• v1 defaults field a to 1
• v2 defaults field b to 2
• v2 is the storage version
• user submits v1 object with a and b unset

Clearly, a defaults to 1 as part of deserialize->default->validate of the user's request

What is less clear to me is if v2 defaulting (setting b to 2) is applied after conversion, before storing in etcd.

[liggitt]

oops, crossed wires. that answered my question, thanks.

[liggitt]

probably worth calling out explicitly (maybe even with that scenario). the v2 defaulting would get applied on the way out of storage, so from an API user's perspective, I think they would see the newly created object returned with both a:1, b:2 set, but only a:1 would be in etcd, right?

[sttts]

made this explicit:

Like for native resources, we do defaulting

* during request payload deserialization
* after mutating webhook admission
* during read from storage.

Note: like for native resources, we do not default after webhook conversions. Hence, webhook conversions must be complete in the sense that they return defaulted objects. Technically we could do defaulting, but to match native resources, we do not.

[liggitt]

in case of lists

clarify how we determine this.

• type: "array" in the schema?
• _, ok := default.([]interface{})?
• _, ok := value.([]interface{})?

all of the above?

and maps

clarify how we determine this.

• type: "object" in the schema?
• _, ok := default.(map[string]interface{})?
• _, ok := value.(map[string]interface{})?

all of the above?

I mostly want to make sure defaulting doesn't replace an empty array or object with a default value of the correct type prior to validation and mask what should be reported to the user as a schema validation error

[sttts]

Made the cases explicit as you wrote down above.

Note: we could do type validation during pruning, i.e. as part of the deserialization process, to match native types.

[liggitt]

is it possible to ensure the default value does not contain a field that would get dropped via pruning? this would help prevent typos. I'm willing to do more expensive things in CRD create/update validation to improve the user experience, as long as it falls out in a relatively straightforward way in the code

[liggitt]

for a given JSONSchemaProps object, I was envisioning checks like this:

if props.Default != null {
  reflect.DeepEqual(
    props.Default,
    prune(
      props.Default,
      makeStructuralSchema(props),
      isPreservingUnknownFields, /* from parent schemas or CRD field */
    ),
  )

  validate(props.Default, ConvertJSONSchemaProps(props))
}

[sttts]

yeah, we can do that.

[sttts]

added

[liggitt]

if I'm reading this correctly, we'd want a nullable field to skip all the checks after the first one, right? something like this?

* `if v, ok := obj[fld]; !ok` => default
* `else if nullable` => no default
* `else if v == nil` => default
...

same for object

[apelisse]

@liggitt I read this as being very similar to what Stefan wrote, but harder to read?

[liggitt]

if the schema specifies the field is nullable, and the current value is [], my example would not default, and stefan's would

I think we want to avoid treating nil and []/{} values differently for defaulting purposes

[apelisse]

Oh I see what happened here.

[apelisse]

If it's nil and non-nullable, shouldn't it be an error anyway?

[apelisse]

Let's think in terms of UX behavior:

{
  defaulted-list: [] # Do I want this defaulted? Not sure, I'm specifying a value after all ...
}
---
{
  defaulted-list: null # Do I want this defaulted? Not sure, I'm specificying a value after all ...
}
---
{
  # defaulted-list should certainly be defaulted
}

One of the principle we used in apply was that if users specified something, then we assume that it's what they want, and setting something to nil is different from not setting it at all.

[liggitt]

If it's nil and non-nullable, shouldn't it be an error anyway?

That's a good point. We could default only if the property was completely absent.

Things I like about only defaulting unspecified properties:

• It's simpler to explain
• It's simpler to implement
• It doesn't mess with user data in any way
• It avoids masking schema errors if they explicitly send null for a property that is not nullable, even if it has a default

Opposing considerations:

• The distinction between "null" and "absent" is not easily maintained in all serialization formats (notably protobuf), so if that got lost in a round-trip, we could end up applying defaulting anyway (this isn't a huge concern, given we currently maintain the distinction for custom resources, so I think we have to figure out a way to continue doing so if we ever switch to persist in other formats like proto)
• We would not be able to replicate defaulting rules for built-in resources using this standard in a hypothetical world where built-in types get converted to CRDs (this also isn't a huge concern, given there are tons of defaulting rules for built-in types we couldn't replicate using openapi at all)
• Sending explicit null values in some patch formats today removes fields, so there's some prior art for null == unset in kube. For example, kubectl patch ... --type=merge -p '{"spec":{"key":null}}' removes an existing key property, it does not persist a literal null value. I'm not sure what server-side apply does here.

Overall, I think the simpler and more intuitive behavior is probably what we want.

[liggitt]

/lgtm

would like a follow-up clarifying #1006 (comment)

[liggitt]

cc @lavalamp @deads2k
for approval

[liggitt]

/lgtm

[deads2k]

Spoken with the stakeholders, we think this is ready.

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, liggitt, sttts

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/sig-api-machinery/OWNERS [deads2k]

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