api-machinery: KEP for immutable fields

[sttts]

Continuous #1099

[deads2k]

allowing a delete and a recreate with different values defies my expectations. This is consistent with how it's treated for objects in some way? Seems weird to allow removing entire stanzas to remove values. Is this to make it work nicely with unions?

[munnerz]

I think this is what @deads2k is already alluding to, but how is a delete and insert identified differently from an update of an entry in-place? It seems like this sort of thing would only be possible if using a list that is keyed on, e.g. a name field. Even then, I agree it seems like an odd thing to permit as from a users perspective, they now just have to perform two API operations instead of one in order to update certain fields.

[sttts]

I get your point. tl/dr: you question the sense of pure removal/addition without identifier. I don't object.

What about string slices? Is the identifier implicitly the value and we would allow a deletion and an insertion in a special mode, but no order changes?

Note, we are very flexible what to allow. The big question is what we really want to offer in Kubernetes-like APIs defined through CRDs.

[sttts]

Not sure your two question are actually the same. I fixed what I think @deads2k pointed out. My example was wrong. Specifying x-kubernetes-immutable applies to the field itself, not only the sub-fields in case of an object.

[jpbetz]

Looks really good. Added a couple suggestions and questions.

[jpbetz]

@erictune mentioned a requirement that this proposal supports with but might be worth calling in the goals here: create requests that set immutable fields must be idempotent. It highlights the importance of handling equality correctly.

[sttts]

do you have an example?

[jpbetz]

Here's the original comment: #1099 (comment)

It's got an example in it.

[apelisse]

I'm not sure I understand what this means

[sttts]

We can move these up to the items level. Both are equivalent.

[sttts]

actually, there is a slight difference between both:

type: object
nullable: true
properties:
  x:
    type: string
    x-kubernetes-immutable: true

allows a change from {x: something} to null or undefined (when used as a field value), but

type: object
x-kubernetes-immutable: true
nullable: true
properties:
  x:
    type: string

does not allow that.

Without nullable and as array items, both coincide.

[erictune]

Suggestion: Recommend a marker syntax for maps and map-slices in Go.
Possible syntax:

// Okay to change any way
A map[string]string

// Can't touch this.
// +immutable
B map[string]string 

// Allowed addition and deletion.
// +immutable-map-values  <--- not sure how to express this in Go.
C map[string]string 

// Allowed addition and deletion.
// +listType=map
// +listMapKey=name
// +immutable-map-values
D []SomeType

// Allowed existing value modification.
// +immutable-keys
E map[string]string 

// Allowed existing value modification.
// +listType=map
// +listMapKey=name
// +immutable-keys
F []SomeType

[sttts]

cc @DirectXMan12

[sttts]

@erictune there is a discussion (for quite some time) about how to express properties of items and map values. Am not sure this is so different here, at least it is overlapping.

I'd like to have general +kubebuilder:validation:items:<some-marker> and +kubebuilder:validation:values:<some-marker> syntax.

[erictune]

There is an alternative.

We could generate proto files and proto descriptors from the types.go. A map[string]string could turn into a map<StringOrEmptyOrNull, StringOrEmptyOrNull>.

[sttts]

How unusual would such an encoding be (outside of kube-proto) ?

We could derive the right proto schema from the specified normalization behaviour. But then we come to the question of compatibility if you change normalization at a later point.

We should start early to flesh out proto targetting 1.18. There seem to be so many details to get right.

[erictune]

Proto's standard libraries define something like this which is used for proto's JSON support:
https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/struct.proto

We'd basically be defining something similar.

[sttts]

Ic, just another sum type (oneof). The question will be whether we then model something in Proto that was just an accident by our use of JSON initially. With JSON we have no choice to get null and undefined as separate values, with proto we could fix our accident.

[apelisse]

With JSON we have no choice to get null and undefined as separate values, with proto we could fix our accident.

We need to find a good balance between the "API semantics make sense", "APIs have too many knobs" and "consistency between CRD and built-in types" trade-off.

[sttts]

Some further thoughts for proto and normalization:

• We already have nullable to explicitly allow nulls.
• With native types we have omitempty which maps null and empty to undefined.

1. In the case of non-nullable, we only have two values. Without normalization we get:

   undefined -> undefined
   empty -> empty
   

   By using a default, we can get:

   undefined -> empty
   empty -> empty
   

   Omitempty would give us:

   undefined -> undefined
   empty -> undefined
   

   Hence, if we had x-kubernetes-omitempty: true we would have covered all variants we know
   today with native types.

2. With nullable and no normalization we get

   undefined -> undefined
   null -> null
   empty -> empty
   

   With defaults, we are able to express:

   undefined -> null
   null -> null
   empty -> empty
   

   With omitempty, we would get:

   undefined -> undefined
   null -> undefined
   empty -> undefined
   

   What we miss is:

   undefined -> empty
   null -> empty
   empty -> empty
   

   I question that we really need this at all. If we identity empty in proto with undefined in JSON, x-kubernetes-omitempty: true would be enough.

In other words, we maybe only need x-kubernetes-omitempty: true and cover all interesting cases.

[erictune]

/lgtm

[deads2k]

I got my comments from slack and in the PR addressed.

/approve
/hold

holding until monday PM for @lavalamp to have a pass if he wishes.

[deads2k]

/lgtm
/hold cancel

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, erictune, 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

[liggitt]

I have a vague memory of an immutable-once-set semantic we wanted at one point (would enable an async allocator to set a field once via the API, after which it would be locked). Was that discarded or deferred? Not having that means any immutable field must be set synchronously on create by the object originator or a webhook.

[sttts]

Am not aware of that old discussion, but it definitly makes sense to flesh that out.

It sounds like you want a way to declare a struct not completely immutable, but stop it from being deleted, and certain fields to be set-once-only:

• This is more than marking fields immutable, because you could still set them to undefined.
• It is less than marking the whole struct's keys immutable because not all fields might be set only.
  In other words, this is some weaker form of x-kubernetes-immutable-keys on the hosting struct, let's call it x-kubernetes-set-once-keys:

Example:

apiVersion: setonce/v1
kind: Once
spec:
   once: true
   name: foo

with

type: object
x-kubernetes-immutable-keys: true
properties:
  spec:
    type: object
    x-kubernetes-set-once-keys: ["once","name"]
    properties:
      once:
        type: boolean
      name:
        type: string
        x-kubernetes-immutable: true

Here, name can only be set to a fixed value once, and never changed anymore. once can be set once (i.e. from undefined to some value), but the actual value can be changed any time.

[apelisse]

@liggitt Did you have an actual use-case in mind? I can't remember talking about that either, and I'd really like to keep the number of knobs low if we can.

[liggitt]

Did you have an actual use-case in mind?

We use this capability in built-in resources today:

• scheduler setting spec.nodeName (and it being immutable after that point)
• populating clusterIP in services
• append-only lists (like ephemeralContainers)
• remove-only lists (like finalizers that may only shrink after deletion)

Talked through a lot of scenarios with @sttts, and I think the API guideline to think twice about boolean use probably applies here. Modeling restrictions on key/value immutability as an enum rather than a boolean allows more use cases to avoid requiring a validating webhook. Something like:

x-kubernetes-mutability: Immutable | AddOnly | RemoveOnly
x-kubernetes-key-mutability: Immutable | AddOnly | RemoveOnly

[lavalamp]

scheduler setting spec.nodeName (and it being immutable after that point)

This is write-once, I agree (but not sure we need a general mechanism for this).

populating clusterIP in services

Is this actually write-once?

append-only lists (like ephemeralContainers)

append-only isn't quite the same as write-once.

remove-only lists (like finalizers that may only shrink after deletion)

IMO the "after deletion" qualifier definitely means that there's not going to be a general mechanism for this.

[liggitt]

scheduler setting spec.nodeName (and it being immutable after that point)

This is write-once, I agree (but not sure we need a general mechanism for this).

populating clusterIP in services

Is this actually write-once?

We set clusterIPs in-process in the service registry implementation, and do not allow it to change on update. To replicate these interactions without write-once capabilities, a custom resource would either have to:

• use a mutating webhook on create (which would require allocation to be really fast) and built-in immutability on the CR schema
• use a controller to allocate asynchronously and a validating webhook to prevent changing/unsetting a value

If we can allow custom resources to express invariants without hooking up dynamic logic, that seems valuable.

append-only isn't quite the same as write-once

You can accomplish append-only with a combination of key-mutability: AddOnly on the container, and mutability: Immutable on the individual items.

IMO the "after deletion" qualifier definitely means that there's not going to be a general mechanism for this.

I agree the AddOnly case is stronger than RemoveOnly.

[lavalamp]

If we can allow custom resources to express invariants without hooking up dynamic logic, that seems valuable.

Yes, the question isn't if it is valuable, the question is how valuable, to how many users, and at what cost, where cost is initial implementation cost (amortized) + ongoing maintenance cost + ongoing API complexity cost to users and us

[liggitt]

One question about immutable-once-set, and one comment about the direct-to-beta proposal. LGTM otherwise.

[lavalamp]

Sorry these comments are late.

• I recommend removing the future-looking sections (proto normalization, add/delete).
• I would like to see more justification for the immutable keys rather than not supporting this feature or supporting it by permitting nested types to turn off immutablity.

[lavalamp]

and our built in tooling? Does openapi-gen cover that?

[sttts]

yes, openapi-gen will produce the immutable tags as vendor extensions in the go-openapi definition we compile in. SSA will get them through the schema passed into the handlers.

[lavalamp]

why?

[lavalamp]

Now that I have read the bottom, I suggest you revise this to consider unset, empty, and 0-length items to be the same.

[lavalamp]

This sentence contradicts itself. If you can already express it, the system can mechanically convert one expression to the other, the semantics must already be the same.

[lavalamp]

I think this would be much less surprising for humans making specs.