From 97d189318d34810858f1dc371bef1f58886b35b5 Mon Sep 17 00:00:00 2001 From: JeromeJu Date: Mon, 5 Feb 2024 16:02:49 +0000 Subject: [PATCH] TEP-0138 New features to use Per-feature flag struct This commit uses the Per-feature flag struct for features added in between the window that the changes per TEP0138 was made. All new features should conform to the new Per-feature flag struct. /kind misc fixes: #7285 --- pkg/apis/config/feature_flags.go | 61 +++++++++++++++++++++------ pkg/apis/config/feature_flags_test.go | 30 +++++++++++-- 2 files changed, 73 insertions(+), 18 deletions(-) diff --git a/pkg/apis/config/feature_flags.go b/pkg/apis/config/feature_flags.go index 03e67b57cb8..86757f0d55b 100644 --- a/pkg/apis/config/feature_flags.go +++ b/pkg/apis/config/feature_flags.go @@ -96,20 +96,13 @@ const ( DefaultCoschedule = CoscheduleWorkspaces // KeepPodOnCancel is the flag used to enable cancelling a pod using the entrypoint, and keep pod on cancel KeepPodOnCancel = "keep-pod-on-cancel" - // DefaultEnableKeepPodOnCancel is the default value for "keep-pod-on-cancel" - DefaultEnableKeepPodOnCancel = false // EnableCELInWhenExpression is the flag to enabled CEL in WhenExpression EnableCELInWhenExpression = "enable-cel-in-whenexpression" - // DefaultEnableCELInWhenExpression is the default value for EnableCELInWhenExpression - DefaultEnableCELInWhenExpression = false // EnableStepActions is the flag to enable the use of StepActions in Steps EnableStepActions = "enable-step-actions" // DefaultEnableStepActions is the default value for EnableStepActions - DefaultEnableStepActions = false // EnableParamEnum is the flag to enabled enum in params EnableParamEnum = "enable-param-enum" - // DefaultEnableParamEnum is the default value for EnableParamEnum - DefaultEnableParamEnum = false disableAffinityAssistantKey = "disable-affinity-assistant" disableCredsInitKey = "disable-creds-init" @@ -129,7 +122,36 @@ const ( ) // DefaultFeatureFlags holds all the default configurations for the feature flags configmap. -var DefaultFeatureFlags, _ = NewFeatureFlagsFromMap(map[string]string{}) +var ( + DefaultFeatureFlags, _ = NewFeatureFlagsFromMap(map[string]string{}) + + // DefaultEnableKeepPodOnCancel is the default PerFeatureFlag value for "keep-pod-on-cancel" + DefaultEnableKeepPodOnCancel = PerFeatureFlag{ + Name: KeepPodOnCancel, + Stability: AlphaAPIFields, + Enabled: DefaultAlphaFeatureEnabled, + } + + // DefaultEnableCELInWhenExpression is the default PerFeatureFlag value for EnableCELInWhenExpression + DefaultEnableCELInWhenExpression = PerFeatureFlag{ + Name: EnableCELInWhenExpression, + Stability: AlphaAPIFields, + Enabled: DefaultAlphaFeatureEnabled, + } + + // DefaultEnableStepActions is the default PerFeatureFlag value for EnableStepActions + DefaultEnableStepActions = PerFeatureFlag{ + Name: EnableStepActions, + Stability: AlphaAPIFields, + Enabled: DefaultAlphaFeatureEnabled} + + // DefaultEnableParamEnum is the default PerFeatureFlag value for EnableParamEnum + DefaultEnableParamEnum = PerFeatureFlag{ + Name: EnableParamEnum, + Stability: AlphaAPIFields, + Enabled: DefaultAlphaFeatureEnabled, + } +) // FeatureFlags holds the features configurations // +k8s:deepcopy-gen=true @@ -174,6 +196,19 @@ func GetFeatureFlagsConfigName() string { // NewFeatureFlagsFromMap returns a Config given a map corresponding to a ConfigMap func NewFeatureFlagsFromMap(cfgMap map[string]string) (*FeatureFlags, error) { + setPerFeatureFlag := func(key string, defaultValue PerFeatureFlag, feature *bool) error { + if cfg, ok := cfgMap[key]; ok { + value, err := strconv.ParseBool(cfg) + if err != nil { + return fmt.Errorf("failed parsing feature flags config %q: %w for feature %s", cfg, err, key) + } + *feature = value + return nil + } + *feature = defaultValue.Enabled + return nil + } + setFeature := func(key string, defaultValue bool, feature *bool) error { if cfg, ok := cfgMap[key]; ok { value, err := strconv.ParseBool(cfg) @@ -221,7 +256,7 @@ func NewFeatureFlagsFromMap(cfgMap map[string]string) (*FeatureFlags, error) { if err := setMaxResultSize(cfgMap, DefaultMaxResultSize, &tc.MaxResultSize); err != nil { return nil, err } - if err := setFeature(KeepPodOnCancel, DefaultEnableKeepPodOnCancel, &tc.EnableKeepPodOnCancel); err != nil { + if err := setPerFeatureFlag(KeepPodOnCancel, DefaultEnableKeepPodOnCancel, &tc.EnableKeepPodOnCancel); err != nil { return nil, err } if err := setEnforceNonFalsifiability(cfgMap, &tc.EnforceNonfalsifiability); err != nil { @@ -233,13 +268,13 @@ func NewFeatureFlagsFromMap(cfgMap map[string]string) (*FeatureFlags, error) { if err := setCoschedule(cfgMap, DefaultCoschedule, tc.DisableAffinityAssistant, &tc.Coschedule); err != nil { return nil, err } - if err := setFeature(EnableCELInWhenExpression, DefaultEnableCELInWhenExpression, &tc.EnableCELInWhenExpression); err != nil { + if err := setPerFeatureFlag(EnableCELInWhenExpression, DefaultEnableCELInWhenExpression, &tc.EnableCELInWhenExpression); err != nil { return nil, err } - if err := setFeature(EnableStepActions, DefaultEnableStepActions, &tc.EnableStepActions); err != nil { + if err := setPerFeatureFlag(EnableStepActions, DefaultEnableStepActions, &tc.EnableStepActions); err != nil { return nil, err } - if err := setFeature(EnableParamEnum, DefaultEnableParamEnum, &tc.EnableParamEnum); err != nil { + if err := setPerFeatureFlag(EnableParamEnum, DefaultEnableParamEnum, &tc.EnableParamEnum); err != nil { return nil, err } // Given that they are alpha features, Tekton Bundles and Custom Tasks should be switched on if @@ -381,8 +416,6 @@ func IsSpireEnabled(ctx context.Context) bool { return FromContextOrDefaults(ctx).FeatureFlags.EnforceNonfalsifiability == EnforceNonfalsifiabilityWithSpire } -// TODO(#7285): Patch the default values of new features that were added after -// `enable-api-fields` was no longer used. type PerFeatureFlag struct { // Name of the feature flag Name string diff --git a/pkg/apis/config/feature_flags_test.go b/pkg/apis/config/feature_flags_test.go index cb0f145ad1e..a5386741746 100644 --- a/pkg/apis/config/feature_flags_test.go +++ b/pkg/apis/config/feature_flags_test.go @@ -53,6 +53,10 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) { SetSecurityContext: config.DefaultSetSecurityContext, Coschedule: config.DefaultCoschedule, EnforceNonfalsifiability: config.DefaultEnforceNonfalsifiability, + EnableKeepPodOnCancel: config.DefaultEnableKeepPodOnCancel.Enabled, + EnableCELInWhenExpression: config.DefaultEnableCELInWhenExpression.Enabled, + EnableStepActions: config.DefaultEnableStepActions.Enabled, + EnableParamEnum: config.DefaultEnableParamEnum.Enabled, }, fileName: config.GetFeatureFlagsConfigName(), }, @@ -98,6 +102,10 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) { MaxResultSize: config.DefaultMaxResultSize, SetSecurityContext: config.DefaultSetSecurityContext, Coschedule: config.DefaultCoschedule, + EnableKeepPodOnCancel: config.DefaultEnableKeepPodOnCancel.Enabled, + EnableCELInWhenExpression: config.DefaultEnableCELInWhenExpression.Enabled, + EnableStepActions: config.DefaultEnableStepActions.Enabled, + EnableParamEnum: config.DefaultEnableParamEnum.Enabled, }, fileName: "feature-flags-enable-api-fields-overrides-bundles-and-custom-tasks", }, @@ -118,6 +126,7 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) { MaxResultSize: config.DefaultMaxResultSize, SetSecurityContext: config.DefaultSetSecurityContext, Coschedule: config.DefaultCoschedule, + EnableParamEnum: config.DefaultEnableParamEnum.Enabled, }, fileName: "feature-flags-bundles-and-custom-tasks", }, @@ -138,6 +147,7 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) { MaxResultSize: config.DefaultMaxResultSize, SetSecurityContext: config.DefaultSetSecurityContext, Coschedule: config.DefaultCoschedule, + EnableParamEnum: config.DefaultEnableParamEnum.Enabled, }, fileName: "feature-flags-beta-api-fields", }, @@ -154,6 +164,10 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) { MaxResultSize: config.DefaultMaxResultSize, SetSecurityContext: config.DefaultSetSecurityContext, Coschedule: config.DefaultCoschedule, + EnableKeepPodOnCancel: config.DefaultEnableKeepPodOnCancel.Enabled, + EnableCELInWhenExpression: config.DefaultEnableCELInWhenExpression.Enabled, + EnableStepActions: config.DefaultEnableStepActions.Enabled, + EnableParamEnum: config.DefaultEnableParamEnum.Enabled, }, fileName: "feature-flags-enforce-nonfalsifiability-spire", }, @@ -169,6 +183,10 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) { MaxResultSize: 8192, SetSecurityContext: config.DefaultSetSecurityContext, Coschedule: config.DefaultCoschedule, + EnableKeepPodOnCancel: config.DefaultEnableKeepPodOnCancel.Enabled, + EnableCELInWhenExpression: config.DefaultEnableCELInWhenExpression.Enabled, + EnableStepActions: config.DefaultEnableStepActions.Enabled, + EnableParamEnum: config.DefaultEnableParamEnum.Enabled, }, fileName: "feature-flags-results-via-sidecar-logs", }, @@ -201,6 +219,10 @@ func TestNewFeatureFlagsFromEmptyConfigMap(t *testing.T) { MaxResultSize: config.DefaultMaxResultSize, SetSecurityContext: config.DefaultSetSecurityContext, Coschedule: config.DefaultCoschedule, + EnableKeepPodOnCancel: config.DefaultEnableKeepPodOnCancel.Enabled, + EnableCELInWhenExpression: config.DefaultEnableCELInWhenExpression.Enabled, + EnableStepActions: config.DefaultEnableStepActions.Enabled, + EnableParamEnum: config.DefaultEnableParamEnum.Enabled, } verifyConfigFileWithExpectedFeatureFlagsConfig(t, FeatureFlagsConfigEmptyName, expectedConfig) } @@ -265,7 +287,7 @@ func TestNewFeatureFlagsConfigMapErrors(t *testing.T) { want: `invalid value for feature flag "coschedule": "invalid"`, }, { fileName: "feature-flags-invalid-keep-pod-on-cancel", - want: `failed parsing feature flags config "invalid": strconv.ParseBool: parsing "invalid": invalid syntax`, + want: `failed parsing feature flags config "invalid": strconv.ParseBool: parsing "invalid": invalid syntax for feature keep-pod-on-cancel`, }, { fileName: "feature-flags-invalid-running-in-environment-with-injected-sidecars", want: `failed parsing feature flags config "invalid-boolean": strconv.ParseBool: parsing "invalid-boolean": invalid syntax`, @@ -274,13 +296,13 @@ func TestNewFeatureFlagsConfigMapErrors(t *testing.T) { want: `failed parsing feature flags config "truee": strconv.ParseBool: parsing "truee": invalid syntax`, }, { fileName: "feature-flags-invalid-enable-cel-in-whenexpression", - want: `failed parsing feature flags config "invalid": strconv.ParseBool: parsing "invalid": invalid syntax`, + want: `failed parsing feature flags config "invalid": strconv.ParseBool: parsing "invalid": invalid syntax for feature enable-cel-in-whenexpression`, }, { fileName: "feature-flags-invalid-enable-step-actions", - want: `failed parsing feature flags config "invalid": strconv.ParseBool: parsing "invalid": invalid syntax`, + want: `failed parsing feature flags config "invalid": strconv.ParseBool: parsing "invalid": invalid syntax for feature enable-step-actions`, }, { fileName: "feature-flags-invalid-enable-param-enum", - want: `failed parsing feature flags config "invalid": strconv.ParseBool: parsing "invalid": invalid syntax`, + want: `failed parsing feature flags config "invalid": strconv.ParseBool: parsing "invalid": invalid syntax for feature enable-param-enum`, }} { t.Run(tc.fileName, func(t *testing.T) { cm := test.ConfigMapFromTestFile(t, tc.fileName)