Initial KEP for immutable fields

[apelisse]

No description provided.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: apelisse
To complete the pull request process, please assign deads2k
You can assign the PR to them by writing /assign @deads2k in a comment when ready.

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

Needs approval from an approver in each of these files:

• keps/sig-api-machinery/OWNERS

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

[apelisse]

/assign @deads2k @liggitt @erictune

[apelisse]

/cc @jennybuckley @kwiesmueller

[sttts]

I would really like to have a way in CRD OpenAPI to specify a normalization behaviour for lists and maps:

x-kubernetes-normalize-null: empty
x-kubernetes-normalize-null: undefined
x-kubernetes-normalize-empty: null
x-kubernetes-normalize-empty: undefined

And then we should be strict here in the mutation test.

Also note that in Proto we have neither null nor empty by default. I still think we need a more broad discussion of null, undefined, empty values.

[apelisse]

I don't disagree, I don't know if this should be blocking?

[sttts]

At least we have to leave it open here and have a plan before marking it implementable. But I would not like to move forward with implementation before being very clear about this topic and where it is heading.

[sttts]

Or in other words: whatever we decide here will drive the API design of the whole CRD ecosystem. We should take the time it needs to understand the topic.

[apelisse]

I think we should make a decision and enforce it rather than offer too many complicated options. We currently consider empty and null to be equal for existing type, and I think that we should just tell people that's how things are going to work.

[sttts]

My main problem with any non-strict behaviour for CRDs is that it feels very odd to have different objects in etcd, but consider them equal.

For native resource with proto encoding this is not the because proto unifies undefined, null and empty all to one value und unmarshal those to null for comparison.

I wonder whether we should talk about normalization for CRDs before talking about equality:

• we have one normalization already: undefined => default value
• we don't have normalization for empty
• we don't have normalization for null

With normalization happening as part of our deserialization stack, we can separate both concerns and use strict equality in immutability checks.

Today we use semantic equality mostly because JSON and Proto have different normalization behaviours (partly driven by omitempty as well).

normalization JSON omitempty
undefined	undefined
null	undefined
empty	undefined

normalization JSON
undefined	null
null	null
empty	empty

normalization Proto omitempty/non-omitempty
undefined	undefined
null	undefined
empty	undefined

normalization CRD omitempty/non-omitempty
undefined	undefined
null	null
empty	empty

normalization CRD default=null
undefined	null
null	null
empty	empty

normalization CRD default=empty
undefined	empty
null	null
empty	empty

[jpbetz]

If I'm understanding all the normalization rules right, then for native types we have something like this?

normalization native (Proto in etcd), client speaking JSON	non-omitempty	omitempty
undefined	empty	undefined
null	empty	undefined
empty	empty	undefined

normalization native omitempty/non-omitempty, client speaking Proto
undefined	undefined
null	undefined
empty	undefined

And we don't have the concept of omitempty empty for CRDs?

Would it make sense to normalize the same way for CRDs as we do for native types (using proto)? I.e. normalize null and empty to undefined. We'd need to normalize all data received from clients and all data read from etcd (since we have non-normalized data at rest). If we did that, we should be able to do strict equality in the api-server code.

I'm guesses that to be consistent we should also normalize native types stored as json like we do for CRDs?

(edit: Adding the normalization would be breaking to any CRD users that expect normalization to not occur.. )

[sttts]

not sure about the first table, what are the values?

And we don't have the concept of omitempty empty for CRDs?

There is no Golang type that could have omitempty in the request handler logic. JSON unmarshalling for CRDs is faithful, i.e. does not swallow or normalize anything.

I am not convinced we want to start putting up fuzzy glasses for CRDs by adding normalization by default. Moreover, we cannot even do that without breaking compatibility, at least not in general (we could do it for equality only).

I don't like the idea of having the normal REST logic with strict empty-null-undefined logic, but immutability to act differently. That's pretty counter-intuitive.

I tend to think about normalization as a property of the fields, i.e. the schema properties. For example we can express nullable fields (which preserve null in all cases, and empty and undefined) using special Golang types (like extra pointers) and at worst custom marshallers. What we don't do today is publishing of this as part of OpenAPI, and in general we never formalized that. But my point is that there are native types which do not normalize prior to persisting values and before comparing, it is just not what we get by default with naive or lazy Golang type definitions and our serializer stack.

In other words, we might want to formalize the normalization of any slice and map in the schemas, for native types and for CRDs. CRD schemas could get a x-kubernetes-normalize-empty: undefined vendor extensions (not necessarily for 1.17, could be added later). Equality should be strict, relative to the defined normalization (which is not the indenty normalization for native types then).

Also for later introduction of protobuf support for a CRDs, we can require a certain normalization that is compatible with proto. I.e. the CRD validation of a CRD instance with proto enabled, would validate the proto numerals are all specified, and it would validate that normalization is compatible.

[sttts]

properties:
  foo:
    x-kubernetes-proto: 6
    x-kubernetes-normalize-empty: empty
    x-kubernetes-normalize-null: empty # if nullable
    x-kubernetes-normalize-undefined: undefined
    nullable: true
    x-kubernetes-immutable: true

validation error: properties.foo.x-kubernetes-normalize-undefined must be empty if x-kubernetes-proto is set.

[jpbetz]

Thanks for explaining. I don't think I appreciated the extend our existing golang type definitions play into the situation. That's good to know.

From a usability perspective, it wish we could provide a sane default normalization rule that users could get by default and not have to specify any normalization settings explicitly, but I agree we can't just start doing that for CRDs since it breaks backward compatibility.

I really like the idea of specifying the normalization we expect then validating our encodings and go types are all aligned with that.

[apelisse]

Actually, this can be done by making the list/map mutable, but the values of the list/map immutable.

[erictune]

Is it a goal that this could, in theory, be used to annotate the Pod type?
If so, it looks like recursive is insufficient.

I think we'd want to do:

type PodSpec struct {
...
// No new items can be added, but mutable fields of items can be modified.
// +immutable=keys 
Containers []Container
...
}

type Container struct {
	// +immutable
	Name string `json:"name" protobuf:"bytes,1,opt,name=name"`

         // Can be updated.  
	Image string `json:"image,omitempty" protobuf:"bytes,2,opt,name=image"`
        ...
}

[erictune]

Perhaps add the following, to make it explicit:

---

Clients implement idempotent creation by retrying creation, and they currently do not get errors if the same creation is tried twice. This KEP will not change that. Specifically, two consecutive POSTs of the same object, which are mutated the same way by mutating admission controller (which is the typical case), will both succeed. If an error were returned the second time, and the client did not see the first success response, the client would be confused.

[erictune]

This is needed to implement an object with the same semantics as Pod.

[jpbetz]

xref #1099 (comment) and #1099 (comment) for more detail on how Pod immutability can be supported

[erictune]

I think we can get by with just one "selection" behavior, if we want to keep it simple. That behavior is:

• +immutable tag on a scalar FieldDecl means that field is immutable.
• +immutable tag on a map of list FieldDecl means that the set of map keys cannot change. Each item is validated separately, comparing same keys of old/new. This tag does not affect item validation.
• +immutable tag on a list FieldDecl means that that the list length cannot change. Each item is validated separately, comparing same keys of old/new. This tag does not affect item validation.

[apelisse]

Agreed, that makes a lot of sense to me.

[erictune]

The above semantics supports the Pod/Containers use case, where you want to allow some fields to be updated on list items, but don't allow adding list items.
It also supports where you want to allow new list items.

The Deployment use case is this: you want to have a PodTemplate where everything is mutable. Today this is handled by having a separate copy of the type definition, which has lacks +immutable specifiers. I think this is a good-enough pattern for other Deployment-like use cases, and so there is no need for special support for this case (like "Recursive with addition and/or deletion").

[logicalhan]

Prior art, same idea but they opted to name the concept "CreateOnly" which I personally found non-confusing.

[michaelgugino]

I'm interested in immutable fields in CRDs as well.

Would it be simpler to do something like:

spec:
  immutableFields:
  otherField
  ...

instead? That seems simpler to implement to me, less to document, anything that's immutable, put in there. Seems like this could be implemented rather uniformly.

[jpbetz]

@apelisse Would it be okay to add the two main open issues (equality, recursive) to this PR and merge it as provisional? We can open followups to resolve those, hopefully this week.

[apelisse]

Sure, I'll update the KEP

[apelisse]

Replaced by #1265