From f01ee2e9669cf8e48a037fc93bb4d4da699d8112 Mon Sep 17 00:00:00 2001 From: Jerop Date: Wed, 12 Apr 2023 18:02:45 -0400 Subject: [PATCH] TEP-0089: Refactor setting of "enforce-nonfalsifiability" feature flag In this change, we refactor the code for setting "enforce-nonfalsifiability" feature flag to make it easier to understand, and consistent with the rest of the code. This refactor will make it easier to promote the feature to beta, and beyond. This change also documents that `enable-api-fields` has to be set to `"alpha"` for non-falsifiability to work. This commit also includes a change to `TestNewFeatureFlagsConfigMapErrors` to check the exact error, instead of just checking that there was an error. This is useful in validating that both errors that can be triggered while setting the "enforce-nonfalsifiability" feature flag are tested. --- docs/spire.md | 2 +- pkg/apis/config/feature_flags.go | 61 +++++++++++++++------------ pkg/apis/config/feature_flags_test.go | 14 +++++- 3 files changed, 46 insertions(+), 31 deletions(-) diff --git a/docs/spire.md b/docs/spire.md index 31553910683..620928deb98 100644 --- a/docs/spire.md +++ b/docs/spire.md @@ -64,7 +64,7 @@ When a TaskRun is created: ## Enabling TaskRun result attestations To enable TaskRun attestations: -1. Make sure `enforce-nonfalsifiability` is set to `"spire"` in the `feature-flags` configmap, see [`install.md`](./install.md#customizing-the-pipelines-controller-behavior) for details +1. Make sure `enforce-nonfalsifiability` is set to `"spire"` and `enable-api-fields` is set to `"alpha"` in the `feature-flags` configmap, see [`install.md`](./install.md#customizing-the-pipelines-controller-behavior) for details 1. Create a SPIRE deployment containing a SPIRE server, SPIRE agents and the SPIRE CSI driver, for convenience, [this sample single cluster deployment](https://github.com/spiffe/spiffe-csi/tree/main/example/config) can be used. 1. Register the SPIRE workload entry for Tekton with the "Admin" flag, which will allow the Tekton controller to communicate with the SPIRE server to manage the TaskRun identities dynamically. ``` diff --git a/pkg/apis/config/feature_flags.go b/pkg/apis/config/feature_flags.go index aa532fafdf9..51041d336bc 100644 --- a/pkg/apis/config/feature_flags.go +++ b/pkg/apis/config/feature_flags.go @@ -125,22 +125,6 @@ func GetFeatureFlagsConfigName() string { return "feature-flags" } -func getEnforceNonfalsifiabilityFeature(cfgMap map[string]string) (string, error) { - var mapValue struct{} - var acceptedValues = map[string]struct{}{ - EnforceNonfalsifiabilityNone: mapValue, - EnforceNonfalsifiabilityWithSpire: mapValue, - } - var value = DefaultEnforceNonfalsifiability - if cfg, ok := cfgMap[enforceNonfalsifiability]; ok { - value = strings.ToLower(cfg) - } - if _, ok := acceptedValues[value]; !ok { - return DefaultEnforceNonfalsifiability, fmt.Errorf("invalid value for feature flag %q: %q", enforceNonfalsifiability, value) - } - return value, nil -} - // NewFeatureFlagsFromMap returns a Config given a map corresponding to a ConfigMap func NewFeatureFlagsFromMap(cfgMap map[string]string) (*FeatureFlags, error) { setFeature := func(key string, defaultValue bool, feature *bool) error { @@ -190,6 +174,9 @@ func NewFeatureFlagsFromMap(cfgMap map[string]string) (*FeatureFlags, error) { if err := setMaxResultSize(cfgMap, DefaultMaxResultSize, &tc.MaxResultSize); err != nil { return nil, err } + if err := setEnforceNonFalsifiability(cfgMap, tc.EnableAPIFields, &tc.EnforceNonfalsifiability); err != nil { + return nil, err + } // Given that they are alpha features, Tekton Bundles and Custom Tasks should be switched on if // enable-api-fields is "alpha". If enable-api-fields is not "alpha" then fall back to the value of @@ -199,21 +186,10 @@ func NewFeatureFlagsFromMap(cfgMap map[string]string) (*FeatureFlags, error) { // defeat the purpose of having a single shared gate for all alpha features. if tc.EnableAPIFields == AlphaAPIFields { tc.EnableTektonOCIBundles = true - // Only consider SPIRE if alpha is on. - enforceNonfalsifiabilityValue, err := getEnforceNonfalsifiabilityFeature(cfgMap) - if err != nil { - return nil, err - } - tc.EnforceNonfalsifiability = enforceNonfalsifiabilityValue } else { if err := setFeature(enableTektonOCIBundles, DefaultEnableTektonOciBundles, &tc.EnableTektonOCIBundles); err != nil { return nil, err } - // Do not enable any form of non-falsifiability enforcement in non-alpha mode. - tc.EnforceNonfalsifiability = EnforceNonfalsifiabilityNone - if enforceNonfalsifiabilityValue, err := getEnforceNonfalsifiabilityFeature(cfgMap); err != nil || enforceNonfalsifiabilityValue != DefaultEnforceNonfalsifiability { - return nil, fmt.Errorf("%q can be set to non-default values (%q) only in alpha", enforceNonfalsifiability, enforceNonfalsifiabilityValue) - } } return &tc, nil } @@ -234,6 +210,35 @@ func setEnabledAPIFields(cfgMap map[string]string, defaultValue string, feature return nil } +// setEnforceNonFalsifiability sets the "enforce-nonfalsifiability" flag based on the content of a given map. +// If the feature gate is invalid, then an error is returned. +func setEnforceNonFalsifiability(cfgMap map[string]string, enableAPIFields string, feature *string) error { + var value = DefaultEnforceNonfalsifiability + if cfg, ok := cfgMap[enforceNonfalsifiability]; ok { + value = strings.ToLower(cfg) + } + + // validate that "enforce-nonfalsifiability" is set to a valid value + switch value { + case EnforceNonfalsifiabilityNone, EnforceNonfalsifiabilityWithSpire: + break + default: + return fmt.Errorf("invalid value for feature flag %q: %q", enforceNonfalsifiability, value) + } + + // validate that "enforce-nonfalsifiability" is set to allowed values for stability level + switch enableAPIFields { + case AlphaAPIFields: + *feature = value + default: + // Do not consider any form of non-falsifiability enforcement in non-alpha mode + if value != DefaultEnforceNonfalsifiability { + return fmt.Errorf("%q can be set to non-default values (%q) only in alpha", enforceNonfalsifiability, value) + } + } + return nil +} + // setResultExtractionMethod sets the "results-from" flag based on the content of a given map. // If the feature gate is invalid or missing then an error is returned. func setResultExtractionMethod(cfgMap map[string]string, defaultValue string, feature *string) error { @@ -263,7 +268,7 @@ func setMaxResultSize(cfgMap map[string]string, defaultValue int, feature *int) } // if max limit is > 1.5 MB (CRD limit). if value >= 1572864 { - return fmt.Errorf("invalid value for feature flag %q: %q. This is exceeding the CRD limit", resultExtractionMethod, value) + return fmt.Errorf("invalid value for feature flag %q: %q. This is exceeding the CRD limit", resultExtractionMethod, fmt.Sprint(value)) } *feature = value return nil diff --git a/pkg/apis/config/feature_flags_test.go b/pkg/apis/config/feature_flags_test.go index c63d7d8a922..d9041383e65 100644 --- a/pkg/apis/config/feature_flags_test.go +++ b/pkg/apis/config/feature_flags_test.go @@ -209,27 +209,37 @@ func TestGetFeatureFlagsConfigName(t *testing.T) { func TestNewFeatureFlagsConfigMapErrors(t *testing.T) { for _, tc := range []struct { fileName string + want string }{{ fileName: "feature-flags-invalid-boolean", + want: `failed parsing feature flags config "im-really-not-a-valid-boolean": strconv.ParseBool: parsing "im-really-not-a-valid-boolean": invalid syntax`, }, { fileName: "feature-flags-invalid-enable-api-fields", + want: `invalid value for feature flag "enable-api-fields": "im-not-a-valid-feature-gate"`, }, { fileName: "feature-flags-invalid-trusted-resources-verification-no-match-policy", + want: `invalid value for feature flag "trusted-resources-verification-no-match-policy": "wrong value"`, }, { fileName: "feature-flags-invalid-results-from", + want: `invalid value for feature flag "results-from": "im-not-a-valid-results-from"`, }, { fileName: "feature-flags-invalid-max-result-size-too-large", + want: `invalid value for feature flag "results-from": "10000000000000". This is exceeding the CRD limit`, }, { fileName: "feature-flags-invalid-max-result-size-bad-value", + want: `strconv.Atoi: parsing "foo": invalid syntax`, }, { fileName: "feature-flags-enforce-nonfalsifiability-bad-flag", + want: `invalid value for feature flag "enforce-nonfalsifiability": "bad-value"`, }, { fileName: "feature-flags-spire-with-stable", + want: `"enforce-nonfalsifiability" can be set to non-default values ("spire") only in alpha`, }} { t.Run(tc.fileName, func(t *testing.T) { cm := test.ConfigMapFromTestFile(t, tc.fileName) - if _, err := config.NewFeatureFlagsFromConfigMap(cm); err == nil { - t.Error("expected error but received nil") + _, err := config.NewFeatureFlagsFromConfigMap(cm) + if d := cmp.Diff(tc.want, err.Error()); d != "" { + t.Errorf("failed to get expected error; diff:\n%s", diff.PrintWantGot(d)) } }) }