Update documentation for use of kubebuilder Enum tags

[youngnick]

The Kubernetes API conventions strongly recommend against using OpenAPI Enum validation, because there is a strong implication that the set of enumerated values is immutable; implementations can reasonably expect that the entire set of an Enum is known. Because of this, for the core APIs, adding a value to or removing a value from an Enum is a breaking API change and requires an API version bump.

Before we go to beta, I'd like us to discuss our use of kubebuilder Enum tags, and whether it will make version management easier for these types to:

• provide an aliased string type (we already do this in most places I can see)
• provide constants for the valid/Enum values
• document that the constants are supported.

I can see an argument that because we have the experimental channel, the rules apply less, but I'm pretty reluctant to disregard lessons learned in the core APIs without a very good reason.

/kind design
I think?

[youngnick]

Here's some relevant quotes from the core docs:
(https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#primitive-types):

Do not use enums. Use aliases for string instead (e.g., NodeConditionType).

(https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#new-enum-value-in-existing-field)

If an API drives behavior that is implemented by external clients (like Ingress or NetworkPolicy), the enum field must explicitly indicate that additional values may be allowed in the future, and define how unrecognized values must be handled by clients. If this was not done in the first release containing the enum field, it is not safe to add new values that can break existing clients.

I'm not wedded to removing all enums, just want us to have talked about it before we go to v1beta1.

[jpeach]

IIUC "do not use enums" refers to code like this:

const (
      Foo = iota
      Bar
      Baz
)

define how unrecognized values must be handled by clients

It sounds like this would apply if the API uses any enum strings in status, which is context-dependent. The other subtley in my reading of this is "If an API drives behavior that is implemented by external clients", which implies that a one controller sets the value and some other controller observes and responds to that change.

[mikemorris]

adding a value to or removing a value from an Enum is a breaking API change

Removing a value (rather than just deprecating in place) makes sense as a breaking change, but I'm a little unclear on how adding a value (to the end, not reordering), would be much worse than adding a new value for an aliased string type?

In both cases I'd expect a client would need to handle unexpected values safely or risk unexpected behavior/panics - is it just that there's a greater expectation for enums that matching is always exhaustive and a catch-all won't ever be needed if all currently-known states are handled?

I think it might be helpful to see a side-by-side example of a current enum field versus what the proposed alternative might look like. (My understanding is that enums are fine for internal handling of values, just discouraged as enums in a public API?)

[robscott]

Thanks for catching this @youngnick! I agree that even adding an allowed value to an enum could be considered a breaking change if we hadn't prepared users/implementers for that possibility. Your quote from the conventions seems very relevant here:

If an API drives behavior that is implemented by external clients (like Ingress or NetworkPolicy), the enum field must explicitly indicate that additional values may be allowed in the future, and define how unrecognized values must be handled by clients. If this was not done in the first release containing the enum field, it is not safe to add new values that can break existing clients.

It seems very straightforward to add text along with each of our enums that indicates that additional values may be allowed in the future. The second part is less clear. If, for example, an implementation runs into a match type that it doesn't recognize, what should it do? Should it drop/ignore the whole rule or route? Is it sufficient to just drop that specific match and add a condition to status? In any case, we'll need to work on adding conditions to status that can be used to represent unknown values for any enum field.

I don't think removing enum validation itself really solves the issue for us here, the core issue seems to be that we need implementations to be able to handle unknown/unrecognized future values. The API conventions around enums do seem rather inconsistent/confusing. I know enum type validation is widespread in k/k APIs, and I don't think there are any great alternatives available. To contrast with some of the suggestions not to use enums, this is also part of API conventions:

Some fields will have a list of allowed values (enumerations). These values will be strings, and they will be in CamelCase, with an initial uppercase letter. Examples: ClusterFirst, Pending, ClientIP. When an acronym or initialism each letter in the acronym should be uppercase, such as with ClientIP or TCPDelay. When a proper name or the name of a command-line executable is used as a constant the proper name should be represented in consistent casing - examples: systemd, iptables, IPVS, cgroupfs, Docker (as a generic concept), docker (as the command-line executable). If a proper name is used which has mixed capitalization like eBPF that should be preserved in a longer constant such as eBPFDelegation.

https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#constants

[bowei]

If want to make controllers handle this properly, we need to put a test case with "UnknownEnumValue" into the conformance tests for each of the potential places where enums are going to be used.

[youngnick]

Thanks everyone for the great discussion. I kind of pushed a little hard here because I wanted to make sure we do something to clear this up.

It sounds to me like the steps we need to take here are instead:

• Clarify that, where we use kubebuilder Enum tags, the enumerations should be considered open to future values.
• Add expectations for what controllers should do with unknown values for each enum. This will involve adding some more detail about the flow for, and some Conditions to describe this state.
• Add conformance tests to validate that controllers behave correctly with unknown values for the enums.

I've updated the title of the issue accordingly.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[youngnick]

/remove-lifecycle stale

/assign

I made this mess, I better clean it up. 😉