Support Kubernetes style feature-gates for new API fields

[skaegi]

Some of the Tekton objects are now "beta" and hopeful "stable" in the not too distant future. When doing work in the API WG we should not be immediately graduating any new fields added to "beta" or whatever degree of stability the CRD currently is. Instead we should provide a mechanism for them to go through some sort of Alpha state until we're sure they work well.

The alternative is that we'll be forced to retain backwards compatibility with mistakes we might be able to catch with real world use before the feature is promoted to beta or stable.

--
What I'm hoping for is a little pre-discussion before I create a concrete TEP. What I'm looking at is the use of techniques from Kubernetes sig-architure like -- https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#adding-unstable-features-to-stable-versions . The idea being that (when possible) you add new feature fields as optional fields that use feature-gates to prevent their use based on the associated feature flag setting.

This issue is specifically targeting the addition of new fields as that currently is the critical problem, however we should also consider applying feature-flags in Tekton to new CRDs.

[vdemeester]

Interesting extract of the doc

An API change is considered compatible if it:

• adds new functionality that is not required for correct behavior (e.g.,
  does not add a new required field)
• does not change existing semantics, including:
  • the semantic meaning of default values and behavior
  • interpretation of existing API types, fields, and values
  • which fields are required and which are not
  • mutable fields do not become immutable
  • valid values do not become invalid
  • explicitly invalid values do not become valid

Put another way:

1. Any API call (e.g. a structure POSTed to a REST endpoint) that succeeded
   before your change must succeed after your change.
2. Any API call that does not use your change must behave the same as it did
   before your change.
3. Any API call that uses your change must not cause problems (e.g. crash or
   degrade behavior) when issued against an API servers that do not include your
   change.
4. It must be possible to round-trip your change (convert to different API
   versions and back) with no loss of information.
5. Existing clients need not be aware of your change in order for them to
   continue to function as they did previously, even when your change is in use.
6. It must be possible to rollback to a previous version of API server that
   does not include your change and have no impact on API objects which do not use
   your change. API objects that use your change will be impacted in case of a
   rollback.

---

• Any field with a default value in one API version must have a non-nil default
  value in all API versions. This can be split into 2 cases:
  • Adding a new API version with a default value for an existing non-defaulted
    field: it is required to add a default value semantically equivalent to
    being unset in all previous API versions, to preserve the semantic meaning
    of the value being unset.
  • Adding a new field with a default value: the default values must be
    semantically equivalent in all currently supported API versions.

[vdemeester]

About "Adding Unstable Features to Stable Versions", there is 2 options described in the document

Alpha field in existing API version

The preferred approach adds an alpha field to the existing object, and ensures it is disabled by default:

It has to be +optional and omitempty.

Before persisting the object to storage, clear disabled alpha fields on create, and on update if the existing object does not already have a value in the field. This prevents new usage of the feature while it is disabled, while ensuring existing data is preserved. Ensuring existing data is preserved is needed so that when the feature is enabled by default in a future version n and data is unconditionally allowed to be persisted in the field, an n-1 API server (with the feature still disabled by default) will not drop the data on update.

• if the feature progresses to beta or stable status, the feature gate can be removed or be enabled by default.
• if the schema of the alpha field must change in an incompatible way, a new field name must be used.
• if the feature is abandoned, or the field name is changed, the field should be removed from the go struct, with a tombstone comment ensuring the field name and protobuf tag are not reused: // +k8s:deprecated=width,protobuf=3.

New alpha API version

The latter requires that all objects in the same API group as Frobber to be
replicated in the new version, v7alpha1. This also requires user to use a new
client which uses the other version. Therefore, this is not a preferred option.

---

I feel this definitely make sense and we could adopt something similar/close to that. We already have the "feature gate" notion (and configmap), we would just need to formalize it better, in a document and in code.

This is not that far from what we are doing right now, except that, for any new field added, we would require a feature-flag to be disabled by default and that could be enable later in time. This would be something to do for Custom Tasks, future of PipelineResource (if any), and would have been useful to do for finally task, when experssion…

I definitely like this approach and I feel we need this. It make adding a new feature (field, type, …) a little bit cumbersome, but with proper documentation and code helpers, it shouldn't be that bad — and it's at the benefit of the users. So 👍 on this.

Note: This should be something that applies to any tektoncd component that defines CRD, be pipeline, triggers, …

/cc @tektoncd/core-maintainers

[vdemeester]

/assign @afrittoli

[bobcatfish]

Related proposal in kubebuilder to add support for feature gates as part of their controller framework: kubernetes-sigs/kubebuilder#849

[tekton-robot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
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 with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

[bobcatfish]

We want this for v1 and there's a TEP in progress at tektoncd/community#280

/lifecycle frozen

[lbernick]

@afrittoli @skaegi Looks like this work is complete; should this be closed?

[ghost]

I'm going to close this as TEP-0033 Tekton Feature Gates has been implemented.

/close

[tekton-robot]

@sbwsg: Closing this issue.

In response to this:

I'm going to close this as TEP-0033 Tekton Feature Gates has been 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.