ZEP-0035: Feature Flags

[mkcp]

• One-line PR description:
  This ZEP proposes the need for, API, docs, and implementation of a Feature Flagging library in Zarf.

• Issue link:
  Feature Flags #35

• Other comments:
  Currently Revision 2, ready for pre-alpha review and potential merge.

[AustinAbro321]

One important note with Zarf is that anything in the schema will span to both create time and deploy time. If a feature is enabled and used at create time, do we assume that it's enabled for the deployer. I'd lean towards yes, but curious on others thoughts.

[mkcp]

That's a good point, yeah. This is maybe a hacky solution, but what if we retrieved the set ofAll() enabled features when creating a package and serialized their names to a build or metadata field?

[AustinAbro321]

I could see that being useful in the event we are changing how a specific feature in the schema works (though maybe if we're doing that it's a sign we should be using a different key).

I was thinking of an even simpler case where we have a gated feature in the schema and someone opts into it, would that feature also be gated on deploy?

[soltysh]

I believe this will depend on whether a particular functionality alters the package or its deployment or not. But storing that information in package metadata is definitely good idea.

[AustinAbro321]

I'd prefer global. From what I can tell it looks like Kubernetes declares their feature flags globally on init() https://github.com/kubernetes/kubernetes/blob/c4beea784c4ba2ab4cdf98d13dfe094fcb746288/pkg/features/kube_features.go#L1945

[soltysh]

I'm supportive.

[mkcp]

Considering changing this to Feature to make it a bit more clear what it's describing.

[mkcp]

Migrating Flag type to Feature in latest

[mkcp]

Same as the type name below, considering changing the pkg name to feature to make it a bit more clear what it's describing.

[mkcp]

MIgrating package flag -> feature in latest.

[brandtkeller]

Processing my understanding - appreciative of the huge scope this is appropriately engaging.

[brandtkeller]

Agree with intent - curious if there is an overlay of alternative designs that uses our already-available utilization of cobra/viper/pflag . Are there constraints present here with the lifecycle exposed by those and our current use? maybe.

pflag has a lot in the way of lifecycle management and the integration with cobra/viper already answers questions for how we consume from config / generate documentation and manage the lifecycle of active/hidden/deprecated flags.

Just want to ensure we weigh that optionality before understanding if we require a bespoke implementation.

[soltysh]

I believe the plag will only to handle the flag parsing part of things. This document focuses primarily on the managing the feature gates, enabled through flags. For example, given the following 4 features at various stages:

1. featureA - stable, enabled by default, can't be disabled.
2. featureB - beta, enabled by default, can be disabled.
3. featureC - beta, disabled by default, can be enabled.
4. featureD - alpha, disabled by default, can be enabled.

A zarf user, by default, has access to featureA and featureB (on by default). If a user wants to enable featureC, one needs to run zarf --feature-gate=featureC=true, if one wants both featureC and featureD - zarf --feature-gate=featureC=true,featureD=true. Alternatively if user wants to disable a feature that is on by default - zarf --feature-gate=featureB=false.

From a developer perspective you have 2 helpful utilities:

1. A registry of feature gates, ideally with information about its state (alpha, beta, stable), whether it's on or off by default, and lastly if it can be turned off.
2. A helper method which allows you to check if featureA is on or off, that you then use in your code base.

An example from k8s:

1. Feature metadata: https://github.com/kubernetes/kubernetes/blob/2a03dd1d5ecc66ca6a21fc9f526d4eedc9fef179/pkg/features/kube_features.go#L405-L410 with a pointer to KEP, and history https://github.com/kubernetes/kubernetes/blob/2a03dd1d5ecc66ca6a21fc9f526d4eedc9fef179/pkg/features/kube_features.go#L1315-L1319
2. Sample usage: https://github.com/kubernetes/kubernetes/blob/2a03dd1d5ecc66ca6a21fc9f526d4eedc9fef179/pkg/apis/batch/v1/defaults.go#L62

[soltysh]

On the other hand, kubectl uses env vars to achieve this:

1. Feature metadata: https://github.com/kubernetes/kubernetes/blob/2a03dd1d5ecc66ca6a21fc9f526d4eedc9fef179/staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go#L434-L438
2. Sample usage: https://github.com/kubernetes/kubernetes/blob/2a03dd1d5ecc66ca6a21fc9f526d4eedc9fef179/staging/src/k8s.io/kubectl/pkg/cmd/cmd.go#L362-L364

Although, I'm planning to simplify the interface for kubectl a bit, since it's a bit clunky atm. The goal is to make it closer to what --feature-gates do.

[soltysh]

Also, we could just re-use the k8s library for feature gates, rather than rolling yet another one https://github.com/kubernetes/component-base/blob/master/featuregate/feature_gate.go. Or use some other library that I'm pretty exist 😅

[mkcp]

Seconding what @soltysh is saying here where pflag works great as a kind of CLI frontend for sure, but doesn't provide a flexible enough model or API for features. Especially when it comes to users needing to query or change feature state within code.

Regarding borrowing or reusing the K8s feature-gates - I definitely took a lot of inspiration from the implementation! Reading through the type was my starting point. K8s runtime mutable features which are compelling, as well as multiple features per gate. Maybe more than we need pre-1.0 though?

If are effective with the API and configuration layer I do think we can come back to the implementation and add functionality in or swap out for more if we need it in the future. The config layer (CLI flags, env vars, zarf-config.{yaml,toml}) already does this well imo. Where we'll need to be careful are the abstractions we offer to users for the SDK.

[brandtkeller]

There are no backwards compatibility concerns for previous versions of Zarf as feature flags deal with the code itself.

Does this consider the impact of the package created and potential drift between the zarf version at creation and the version used during deploy?

[soltysh]

Discussed in https://github.com/zarf-dev/proposals/pull/41/files#r2210582550

[brandtkeller]

++ It is a great reason for us to have a process that tracks current state and what we require for graduation?

[brandtkeller]

appreciate this - how can we make the propagation throughout the system more observable.

[mkcp]

I think that w/ the addition of separate sets for default and user features that sourcing these gets even easier.

[soltysh]

I believe the plag will only to handle the flag parsing part of things. This document focuses primarily on the managing the feature gates, enabled through flags. For example, given the following 4 features at various stages:

1. featureA - stable, enabled by default, can't be disabled.
2. featureB - beta, enabled by default, can be disabled.
3. featureC - beta, disabled by default, can be enabled.
4. featureD - alpha, disabled by default, can be enabled.

A zarf user, by default, has access to featureA and featureB (on by default). If a user wants to enable featureC, one needs to run zarf --feature-gate=featureC=true, if one wants both featureC and featureD - zarf --feature-gate=featureC=true,featureD=true. Alternatively if user wants to disable a feature that is on by default - zarf --feature-gate=featureB=false.

From a developer perspective you have 2 helpful utilities:

1. A registry of feature gates, ideally with information about its state (alpha, beta, stable), whether it's on or off by default, and lastly if it can be turned off.
2. A helper method which allows you to check if featureA is on or off, that you then use in your code base.

An example from k8s:

1. Feature metadata: https://github.com/kubernetes/kubernetes/blob/2a03dd1d5ecc66ca6a21fc9f526d4eedc9fef179/pkg/features/kube_features.go#L405-L410 with a pointer to KEP, and history https://github.com/kubernetes/kubernetes/blob/2a03dd1d5ecc66ca6a21fc9f526d4eedc9fef179/pkg/features/kube_features.go#L1315-L1319
2. Sample usage: https://github.com/kubernetes/kubernetes/blob/2a03dd1d5ecc66ca6a21fc9f526d4eedc9fef179/pkg/apis/batch/v1/defaults.go#L62

[soltysh]

On the other hand, kubectl uses env vars to achieve this:

1. Feature metadata: https://github.com/kubernetes/kubernetes/blob/2a03dd1d5ecc66ca6a21fc9f526d4eedc9fef179/staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go#L434-L438
2. Sample usage: https://github.com/kubernetes/kubernetes/blob/2a03dd1d5ecc66ca6a21fc9f526d4eedc9fef179/staging/src/k8s.io/kubectl/pkg/cmd/cmd.go#L362-L364

Although, I'm planning to simplify the interface for kubectl a bit, since it's a bit clunky atm. The goal is to make it closer to what --feature-gates do.

[soltysh]

I'm inclined to move this to goals, to ensure we don't have forever features. Ideally you want to enforce some kind of timeline for every feature, but if you see something is stuck for too long you might want to consider to drop it, which is also easier depending on the stage the features is at. Alpha features can be dropped almost at a whim, whereas beta (especially those enabled by default) or stable require a proper deprecation period to allow users to stop using those.

[mkcp]

Yeah I see what you mean, having features while models all of this explicitly does help with the problem. And the problem exists whether it's modeled or not.

[soltysh]

I believe this will depend on whether a particular functionality alters the package or its deployment or not. But storing that information in package metadata is definitely good idea.

[soltysh]

I'd suggest not to store Enabled next to Default, but rather keep this structure minimal such that it will only hold information about feature gate, such as: name, description, version, enabled, stage. Separately, you'll want to keep a registry of currently enabled flags (those with enabled=true out of the box and those enabled by users explicitly through flags), and devs should always use the registry helper to know if it's on or off.

This separation ensures we don't accidentally enable something that should not be enabled.

[mkcp]

That's a good point, and the default vs. enabled states were intended to allow for this behavior in the same set of features. Thinking through this more though, there's another complication if a feature is disabled by default then enabled by users (default=true, enabled=false) -> (default=true, enabled=true) - that effectively overwrites the flag and there's no way for us to distinguish during runtime that it was changed.

I see the advantages of removing the "default" field and instead having two different sets of features. Having a simpler API starts to outweigh the downsides of some more impl. complexity. Namely, it allows us to store multiple copies of flags and see if the state has been changed by users. e.g. psuedocode:

default={Foo=false, bar=...}
enabled={Foo=true}

This could be accomplished with another field on the struct, e.g. modified and constructors (e.g. feature.NewDefault, feature.NewEnabled, etc.) but I'd also lean towards keeping the struct minimal to keep the API simple and "just" use sets instead of boolean logic.

I'll update the API some so it can filter for default or not rather than passing the fields back and having the user do so.

[soltysh]

Potentially, you could also have a better way to inform users about deprecated features, if they are using those.

[mkcp]

Definitely a benefit, going to add it to goals.

[soltysh]

I'm supportive.

[soltysh]

Discussed in https://github.com/zarf-dev/proposals/pull/41/files#r2210582550

[mkcp]

Revision 2 of the proposal is done which instead proposes globally stored features and updates the API accordingly. I've provided some examples of how this is intended to work, but if there's any gaps please let me know!

I believe this is ready to merge given final draft review and alpha impl is ready to begin. There's still some TODOs left in that I'd like to come back to after implementing alpha. Nitpicks absolutely welcome!

[AustinAbro321]

Nice work so far. Clarifying question on CLI UX

[AustinAbro321]

Curious on how this would look for something like --registry-proxy in #37. I think that'd be a good candidate for feature flag.

I would expect that if users are required to run zarf init --feature-enable=registry-proxy --registry-proxy, just running zarf init --feature-enable=registry-proxy wouldn't actually do anything in that case. Also if someone runs zarf init -h before registry-proxy is enabled should they see the --registry-proxy flag in help text?

[mkcp]

The first form that you mentioned is also what I'd expect, that way the feature itself can be enabled but not actually run. Users could also enable the feature through ZARF_FEATURE_ENABLE="registry-proxy" zarf init --registry-proxy or through on-disk config so the main idea is keeping parity. Given that registry-proxy is an option, flagging it enables that option.

As for help text, it's a good callout. In a quick experiment, I wasn't able to get the boolean state of one rootCmd pflag to mark another as hidden (e.g. if IsColorDisabled { ...MarkHidden("--log-level") }. There may be another path though.

That said, the least clever path would be showing the flag text every time and documenting its release stage and which feature flag to use.

[AustinAbro321]

Yeah worth experimenting more on that in the future, but marking it as disabled in the flag text as a backup is reasonable for now.

[AustinAbro321]

A few todos still, but this is certainly in a state where we can start iterating on the implementation. Nice work.