Implement support for Enum values

[yannh]

Initially opened here kubernetes/kubernetes#97503 but I believe this improvement suggestion might be better here? This is about supporting parameters that support a list of values:

The "Protocol" parameter of a ContainerPort needs to be one of 'TCP', 'UDP', or 'SCTP' - as documented here

kubernetes/staging/src/k8s.io/api/core/v1/types.go

Line 1837 in ddf3eb5
// Protocol for port. Must be UDP, TCP, or SCTP.

However this restriction is not explicit in the generated OpenAPI file: https://raw.githubusercontent.com/kubernetes/kubernetes/ddf3eb5a1877338da806c2be15b53f456b6342c9/api/openapi-spec/swagger.json

There are a number of similar parameters which only accept a list of values - that restriction not being part of the generated OpenAPI file - "RestartPolicy" being another one.

This restricts the amount of validation that schema validators (such as Kubeval/Kubeconform) can do - and confuses users who would expect the tool to catch this kind of error.

OpenAPI does support enums though - is this a restriction of the tooling? It would be great to have these as part of the OpenAPI schema! pray

[yannh]

I've started toying with adding a new tag which I'm currently calling "enum":

// A string with a list of accepted values
// +enum=["TCP","UDP","SCTP"]
StringEnum string

Here is the branch I am currently working on: https://github.com/kubernetes/kube-openapi/compare/master...yannh:enum-support?expand=1

I would love early feedback on this. Currently this only works for "simple" properties.

[sttts]

Are these enums "closed" in the sense that they will never be extended in the future?

In general, I am not against extending the schema. Please compare with the recent defaulting KEP by @apelisse. It is the same direction. It has some thoughts about syntax.

Also keep in mind that new tags must work with kubebuilder cc @DirectXMan12

[apelisse]

Thanks Stefan, I completely agree with the sentiment.

An alternative syntax (I don't know if it's better), since "enums" always have to be lists, could be to repeat the tag:

// +enum="TCP"
// +enum="UDP"
// +enum="..."

That definitely uses more space.

Also, something else that is important to note. We already have mechanisms for "enums" by defining a string type, and then giving this type values. We could possibly use that mechanism somehow to specify the values of an enum. Other related problem, we don't have a good mechanism to document the possible values of an enum, i.e. "TCP means Transmission Control Protocol, bla bla bla" (today, the comments we define on the constants are silently dropped, there is an issue somewhere about this, @thockin)

[DirectXMan12]

I'd prefer if folks synced with roughly what we use in kubebuilder -- see https://book.kubebuilder.io/reference/markers/crd-validation.html for the marker itself, and https://book.kubebuilder.io/reference/markers.html#marker-syntax for the syntax.

[yannh]

Hi @DirectXMan12 ! That would mean, support for both "Surrounding them with curly braces and separating with commas" and "Separated by semicolons" - for a single field called "+Enum"? I'm fine with that - I would probably re-use your parsing code, if that's ok.

@apelisse : do we need to specify the type of an enum, or should it be inferred from the parameter type?
On the documentation - I'm open to suggestions! Thanks everyone for the feedback.

[apelisse]

I'd prefer if folks synced with roughly what we use in kubebuilder -- see https://book.kubebuilder.io/reference/markers/crd-validation.html for the marker itself, and https://book.kubebuilder.io/reference/markers.html#marker-syntax for the syntax.

That seems very different from what we do for built-in types? I'd rather avoid having to do:

// Protocol defines network protocols supported for things like container ports.
type Protocol string

const (
	// ProtocolTCP is the TCP protocol.
	ProtocolTCP Protocol = "TCP"
	// ProtocolUDP is the UDP protocol.
	ProtocolUDP Protocol = "UDP"
	// ProtocolSCTP is the SCTP protocol.
	ProtocolSCTP Protocol = "SCTP"
)

type ContainerPort struct {
	// ... omitted fields ...

	// Required: Supports "TCP", "UDP" and "SCTP"
	// +optional
	// +enum=["TCP", "UDP", "SCTP"]
	Protocol Protocol
}

[thockin]

Agree. I should be able to say something like:

// Protocol defines network protocols supported for things like container ports.
// +enum=open
type Protocol string

const (
	ProtocolTCP Protocol = "TCP"
	ProtocolUDP Protocol = "UDP"
	ProtocolSCTP Protocol = "SCTP"
)

type ContainerPort struct {
	// ... omitted fields ...

	// The IP protocol for this port.
	// +optional
	Protocol Protocol
}

and get docs that say:

field: Protocol
type: core.v1.Protocol (enum string, open)
desc: The IP protocol for this port.  Supported values are "TCP", "UDP", "SCTP". 

That's a trivial case. What about a case where the enum values actually need their own comments?

// LimitType defines a type of object that is limited
// +enum=open
type LimitType string

const (
    // applies to all pods in a namespace
    LimitTypePod LimitType = "Pod"
    // applies to all containers in a namespace
    LimitTypeContainer LimitType = "Container"
    // applies to all persistent volume claims in a namespace
    LimitTypePersistentVolumeClaim LimitType = "PersistentVolumeClaim"
)

// LimitRangeItem defines a min/max usage limit for any resource that matches on kind
type LimitRangeItem struct {
    // The type of resource that this limit applies to
    // +optional
    Type LimitType

with the resulting docs:

field: Type
type: core.v1.LimitType (enum string, open)
desc: The type of resource that this limit applies to.  Supported values are "Pod" (applies to all pods in a namespace), "Container" (applies to all containers in a namespace), "PersistentVolumeClaim" (applies to all persistent volume claims in a namespace). 

Now, I have intentionally violated godoc here (another reason for an IDL), but we could try to follow it better with some conventions. This is still kind of a trivial example. If the per-value comments were longer or multi-sentence, this would be a poor fit.

// ProcMountType defines the type of proc mount
// +enum=open
type ProcMountType string

const (
    // DefaultProcMount uses the container runtime defaults for readonly and masked
    // paths for /proc.  Most container runtimes mask certain paths in /proc to avoid
    // accidental security exposure of special devices or information.
    DefaultProcMount ProcMountType = "Default"

    // UnmaskedProcMount bypasses the default masking behavior of the container
    // runtime and ensures the newly created /proc the container stays intact with
    // no modifications.
    UnmaskedProcMount ProcMountType = "Unmasked"
)

type SecurityContext struct {
    // ... omitted fields ...

    // the type of proc mount to use for the containers.
    // The default is "Default".
    // +optional
    ProcMount *ProcMountType
}

producing something like

field: ProcMount
type: core.v1.ProcMountType (enum string, open)
desc: The type of proc mount to use for the containers.  Supported values are "Pod", "Container", "PersistentVolumeClaim".  "Default" uses the container runtime defaults for readonly and masked paths for /proc.  Most container runtimes mask certain paths in /proc to avoid accidental security exposure of special devices or information.  "Unmasked" bypasses the default masking behavior of the container runtime and ensures the newly created /proc the container stays intact with no modifications.

or even:

field: ProcMount
type: core.v1.ProcMountType (enum string, open)
desc: The type of proc mount to use for the containers.  Supported values are "Pod", "Container", "PersistentVolumeClaim".
values:
 - "Default" uses the container runtime defaults for readonly and masked paths for /proc.  Most container runtimes mask certain paths in /proc to avoid accidental security exposure of special devices or information.
 - "Unmasked" bypasses the default masking behavior of the container runtime and ensures the newly created /proc the container stays intact with no modifications.

The TL;DR of what I am blathering on about is that we should think about how people consume the results and define our conventions and processing steps to optimize for that. I don't think users want to see "this field is a ProcMountType" and then have to click-thru to the definition of that type to see the valid values. I think they want to see the valid values right alongside the field. I could be convinced that click-thru to get the MEANINGS of the fields is acceptable, if openAPI can support that. E.g.

field: ProcMount
type: core.v1.ProcMountType (enum string, open)
desc: The type of proc mount to use for the containers.  Supported values are "Pod", "Container", "PersistentVolumeClaim".

// somewhere else...

typedef: core.v1.ProcMountType
type: enum string
open: true
desc: The policy which determines how runtimes mount /proc into a container.
values:
 - "Default" uses the container runtime defaults for readonly and masked paths for /proc.  Most container runtimes mask certain paths in /proc to avoid accidental security exposure of special devices or information.
 - "Unmasked" bypasses the default masking behavior of the container runtime and ensures the newly created /proc the container stays intact with no modifications.

[yannh]

Thanks for the comments! From my point of view - I am only interested in automated schema validation - any solution presented here would work for me. It however seems to go a bit beyond my understanding of this codebase - I'm not sure I would be able to implement these suggestions.

[apelisse]

@yannh, Thanks for your help! We have to think of this problem holistically, I'd love to help you if you want to give it a try :-)

[galdor]

Is there a reason not to generate definitions for each enum type and use them in various places (other definitions, parameters, etc.) ? In the current situation, it is impossible to generate validating code, and it forces developers to hardcode enumeration values everywhere.

[jpbetz]

For posterity-- I had attempted to solve this problem in late 2019: #176. I decided to wait until a openapi v3 because it would be a breaking change to v2, and it looks like the bots closed the PR for me after I ignored it for too long.

[k8s-triage-robot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

[jiahuif]

/remove-lifecycle stale
Reopening because I am starting to work on this feature.

[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

[k8s-triage-robot]

The Kubernetes project currently lacks enough active 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 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 rotten

[apelisse]

Not sure if we need to close this, the feature is only alpha at the moment. Let's probably keep it open until beta at least.
/remove-lifecycle rotten

[apelisse]

For reference, this is the KEP and this is the enhancement.

[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

[k8s-triage-robot]

The Kubernetes project currently lacks enough active 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 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 rotten

[apelisse]

This is beta now, maybe we can close it?

[jiahuif]

Yes! We can consider this implemented.
/close

[k8s-ci-robot]

@jiahuif: Closing this issue.

In response to this:

Yes! We can consider this implemented.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.