From 292102d429d147647a7a9765dbc0f6167f350bcf Mon Sep 17 00:00:00 2001 From: slinkydeveloper Date: Tue, 1 Jun 2021 09:16:59 +0200 Subject: [PATCH 1/6] Added tristate flags Renamed experimental features config map Signed-off-by: Francesco Guardiani --- config/400-config-experimental-features.yaml | 1 - config/400-config-features.yaml | 1 + ...perimental-features.yaml => features.yaml} | 2 +- .../api_validation.go | 0 .../api_validation_test.go | 32 +++++++++---------- .../{experimental => feature}/features.go | 32 +++++++++++++++---- .../features_test.go | 14 +++++--- pkg/apis/{experimental => feature}/store.go | 9 ++++-- .../{experimental => feature}/store_test.go | 0 .../testdata/config-features.yaml} | 7 ++-- test/experimental/main_test.go | 2 +- 11 files changed, 65 insertions(+), 35 deletions(-) delete mode 120000 config/400-config-experimental-features.yaml create mode 120000 config/400-config-features.yaml rename config/core/configmaps/{experimental-features.yaml => features.yaml} (95%) rename pkg/apis/{experimental => feature}/api_validation.go (100%) rename pkg/apis/{experimental => feature}/api_validation_test.go (92%) rename pkg/apis/{experimental => feature}/features.go (62%) rename pkg/apis/{experimental => feature}/features_test.go (77%) rename pkg/apis/{experimental => feature}/store.go (92%) rename pkg/apis/{experimental => feature}/store_test.go (100%) rename pkg/apis/{experimental/testdata/config-experimental-features.yaml => feature/testdata/config-features.yaml} (87%) diff --git a/config/400-config-experimental-features.yaml b/config/400-config-experimental-features.yaml deleted file mode 120000 index 512caf4da6a..00000000000 --- a/config/400-config-experimental-features.yaml +++ /dev/null @@ -1 +0,0 @@ -core/configmaps/experimental-features.yaml \ No newline at end of file diff --git a/config/400-config-features.yaml b/config/400-config-features.yaml new file mode 120000 index 00000000000..864ceb03167 --- /dev/null +++ b/config/400-config-features.yaml @@ -0,0 +1 @@ +core/configmaps/features.yaml \ No newline at end of file diff --git a/config/core/configmaps/experimental-features.yaml b/config/core/configmaps/features.yaml similarity index 95% rename from config/core/configmaps/experimental-features.yaml rename to config/core/configmaps/features.yaml index cd44c522877..36fc042ffd7 100644 --- a/config/core/configmaps/experimental-features.yaml +++ b/config/core/configmaps/features.yaml @@ -15,7 +15,7 @@ apiVersion: v1 kind: ConfigMap metadata: - name: config-experimental-features + name: config-features namespace: knative-eventing labels: eventing.knative.dev/release: devel diff --git a/pkg/apis/experimental/api_validation.go b/pkg/apis/feature/api_validation.go similarity index 100% rename from pkg/apis/experimental/api_validation.go rename to pkg/apis/feature/api_validation.go diff --git a/pkg/apis/experimental/api_validation_test.go b/pkg/apis/feature/api_validation_test.go similarity index 92% rename from pkg/apis/experimental/api_validation_test.go rename to pkg/apis/feature/api_validation_test.go index 1e2320a8653..f52095e0a02 100644 --- a/pkg/apis/experimental/api_validation_test.go +++ b/pkg/apis/feature/api_validation_test.go @@ -43,8 +43,8 @@ func TestValidateAPIFields(t *testing.T) { { name: "invalid input", featureName: flagName, - flags: map[string]bool{ - flagName: true, + flags: map[string]Flag{ + flagName: Enabled, }, object: []string{}, experimentalFields: []string{"Filter"}, @@ -52,8 +52,8 @@ func TestValidateAPIFields(t *testing.T) { { name: "enabled flag", featureName: flagName, - flags: map[string]bool{ - flagName: true, + flags: map[string]Flag{ + flagName: Enabled, }, object: eventingv1.TriggerSpec{ Broker: "blabla", @@ -67,8 +67,8 @@ func TestValidateAPIFields(t *testing.T) { { name: "disabled pointer flag", featureName: flagName, - flags: map[string]bool{ - flagName: false, + flags: map[string]Flag{ + flagName: Disabled, }, object: eventingv1.TriggerSpec{ Broker: "blabla", @@ -86,8 +86,8 @@ func TestValidateAPIFields(t *testing.T) { { name: "disabled nested string flag", featureName: flagName, - flags: map[string]bool{ - flagName: false, + flags: map[string]Flag{ + flagName: Disabled, }, object: eventingv1.TriggerSpec{ Broker: "blabla", @@ -106,8 +106,8 @@ func TestValidateAPIFields(t *testing.T) { { name: "enabled nested string flag", featureName: flagName, - flags: map[string]bool{ - flagName: true, + flags: map[string]Flag{ + flagName: Enabled, }, object: eventingv1.TriggerSpec{ Broker: "blabla", @@ -122,8 +122,8 @@ func TestValidateAPIFields(t *testing.T) { { name: "disabled map flag", featureName: flagName, - flags: map[string]bool{ - flagName: false, + flags: map[string]Flag{ + flagName: Disabled, }, object: &eventingv1.TriggerFilter{ Attributes: map[string]string{}, @@ -159,8 +159,8 @@ func TestValidateAnnotations(t *testing.T) { }{{ name: "enabled flag", featureName: flagName, - flags: map[string]bool{ - flagName: true, + flags: map[string]Flag{ + flagName: Enabled, }, object: &eventingv1.Broker{ ObjectMeta: metav1.ObjectMeta{ @@ -174,8 +174,8 @@ func TestValidateAnnotations(t *testing.T) { { name: "disabled flag", featureName: flagName, - flags: map[string]bool{ - flagName: false, + flags: map[string]Flag{ + flagName: Disabled, }, object: &eventingv1.Broker{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/apis/experimental/features.go b/pkg/apis/feature/features.go similarity index 62% rename from pkg/apis/experimental/features.go rename to pkg/apis/feature/features.go index b10ce3e1990..b599e5b3623 100644 --- a/pkg/apis/experimental/features.go +++ b/pkg/apis/feature/features.go @@ -23,13 +23,31 @@ import ( corev1 "k8s.io/api/core/v1" ) +// Flag is a string value which can be either Enabled, Disabled, or Allowed. +type Flag string + +const ( + // Enabled turns on an optional behavior. + Enabled Flag = "Enabled" + // Disabled turns off an optional behavior. + Disabled Flag = "Disabled" + // Allowed neither explicitly disables or enables a behavior. + // eg. allow a client to control behavior with an annotation or allow a new value through validation. + Allowed Flag = "Allowed" +) + // Flags is a map containing all the enabled/disabled flags for the experimental features. // Missing entry in the map means feature is equal to feature not enabled. -type Flags map[string]bool +type Flags map[string]Flag // IsEnabled returns true if the feature is enabled func (e Flags) IsEnabled(featureName string) bool { - return e != nil && e[featureName] + return e != nil && e[featureName] == Enabled +} + +// IsAllowed returns true if the feature is enabled or allowed +func (e Flags) IsAllowed(featureName string) bool { + return e.IsEnabled(featureName) || (e != nil && e[featureName] == Allowed) } // NewFlagsConfigFromMap creates a Flags from the supplied Map @@ -42,10 +60,12 @@ func NewFlagsConfigFromMap(data map[string]string) (Flags, error) { continue } sanitizedKey := strings.TrimSpace(k) - if strings.EqualFold(v, "true") { - flags[sanitizedKey] = true - } else if strings.EqualFold(v, "false") { - flags[sanitizedKey] = false + if strings.EqualFold(v, string(Allowed)) { + flags[sanitizedKey] = Allowed + } else if strings.EqualFold(v, string(Disabled)) { + flags[sanitizedKey] = Disabled + } else if strings.EqualFold(v, string(Enabled)) { + flags[sanitizedKey] = Enabled } else { return Flags{}, fmt.Errorf("cannot parse the boolean flag '%s' = '%s'. Allowed values: [true, false]", k, v) } diff --git a/pkg/apis/experimental/features_test.go b/pkg/apis/feature/features_test.go similarity index 77% rename from pkg/apis/experimental/features_test.go rename to pkg/apis/feature/features_test.go index 738c8809ac0..572982b1b84 100644 --- a/pkg/apis/experimental/features_test.go +++ b/pkg/apis/feature/features_test.go @@ -34,10 +34,10 @@ func TestFlags_IsEnabled_EmptyMap(t *testing.T) { func TestFlags_IsEnabled_ContainingFlag(t *testing.T) { require.True(t, Flags{ - "myflag": true, + "myflag": Enabled, }.IsEnabled("myflag")) require.False(t, Flags{ - "myflag": false, + "myflag": Disabled, }.IsEnabled("myflag")) } @@ -46,7 +46,11 @@ func TestGetFlags(t *testing.T) { flags, err := NewFlagsConfigFromConfigMap(example) require.NoError(t, err) - require.True(t, flags.IsEnabled("my-true-flag")) - require.False(t, flags.IsEnabled("my-false-flag")) - require.False(t, flags.IsEnabled("non-existing-flag")) + require.True(t, flags.IsEnabled("my-enabled-flag")) + require.False(t, flags.IsEnabled("my-allowed-flag")) + require.False(t, flags.IsEnabled("non-disabled-flag")) + + require.True(t, flags.IsAllowed("my-enabled-flag")) + require.True(t, flags.IsAllowed("my-allowed-flag")) + require.False(t, flags.IsAllowed("non-disabled-flag")) } diff --git a/pkg/apis/experimental/store.go b/pkg/apis/feature/store.go similarity index 92% rename from pkg/apis/experimental/store.go rename to pkg/apis/feature/store.go index 20aacf7d10c..8814517ff68 100644 --- a/pkg/apis/experimental/store.go +++ b/pkg/apis/feature/store.go @@ -24,7 +24,7 @@ import ( const ( // FlagsConfigName is the name of config map containing the experimental features flags - FlagsConfigName = "config-experimental-features" + FlagsConfigName = "config-features" ) type cfgKey struct{} @@ -63,7 +63,7 @@ type Store struct { func NewStore(logger configmap.Logger, onAfterStore ...func(name string, value interface{})) *Store { store := &Store{ UntypedStore: configmap.NewUntypedStore( - "experimental-flags", + "feature-flags", logger, configmap.Constructors{ FlagsConfigName: NewFlagsConfigFromConfigMap, @@ -85,6 +85,11 @@ func (s *Store) IsEnabled(featureName string) bool { return s.Load().IsEnabled(featureName) } +// IsAllowed is a shortcut for Load().IsAllowed(featureName) +func (s *Store) IsAllowed(featureName string) bool { + return s.Load().IsAllowed(featureName) +} + // Load creates a Config from the current config state of the Store. func (s *Store) Load() Flags { loaded := s.UntypedLoad(FlagsConfigName) diff --git a/pkg/apis/experimental/store_test.go b/pkg/apis/feature/store_test.go similarity index 100% rename from pkg/apis/experimental/store_test.go rename to pkg/apis/feature/store_test.go diff --git a/pkg/apis/experimental/testdata/config-experimental-features.yaml b/pkg/apis/feature/testdata/config-features.yaml similarity index 87% rename from pkg/apis/experimental/testdata/config-experimental-features.yaml rename to pkg/apis/feature/testdata/config-features.yaml index d6392db2817..2681fb6c04b 100644 --- a/pkg/apis/experimental/testdata/config-experimental-features.yaml +++ b/pkg/apis/feature/testdata/config-features.yaml @@ -15,7 +15,7 @@ apiVersion: v1 kind: ConfigMap metadata: - name: config-experimental-features + name: config-features namespace: knative-eventing labels: eventing.knative.dev/release: devel @@ -23,5 +23,6 @@ metadata: knative.dev/config-category: eventing data: _example: | - my-true-flag: "true" - my-false-flag: "false" + my-enabled-flag: "enabled" + my-disabled-flag: "disabled" + my-allowed-flag: "allowed" diff --git a/test/experimental/main_test.go b/test/experimental/main_test.go index 9177375fc94..0356202305a 100644 --- a/test/experimental/main_test.go +++ b/test/experimental/main_test.go @@ -24,7 +24,7 @@ import ( "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "knative.dev/eventing/pkg/apis/experimental" + "knative.dev/eventing/pkg/apis/feature" "knative.dev/pkg/system" // Uncomment the following line to load the gcp plugin (only required to authenticate against GKE clusters). From fafdf09b37fdcf7a44a8207cd0e9ceff3c3ac9b0 Mon Sep 17 00:00:00 2001 From: slinkydeveloper Date: Tue, 1 Jun 2021 09:26:07 +0200 Subject: [PATCH 2/6] Update doc Signed-off-by: Francesco Guardiani --- docs/experimental-features.md | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/docs/experimental-features.md b/docs/experimental-features.md index fc62d9b3315..4c95a7c0bcd 100644 --- a/docs/experimental-features.md +++ b/docs/experimental-features.md @@ -55,6 +55,12 @@ and When the feature is implemented, we provide a Golang API to check if the feature is enabled or not: [`experimental.FromContext(ctx).IsEnabled(featureName)`](../pkg/apis/experimental). +There are 3 feature states: + +* Enabled: the feature is enabled. +* Allowed: the feature may be enabled (e.g. using an annotation or looser validation). +* Disabled: the feature cannot be enabled. + The user can easily enable the features modifying a config map/environment variable of their knative setup: @@ -65,8 +71,9 @@ metadata: name: config-experimental-features namespace: knative-eventing data: - my-fancy-feature: false - another-cool-feature: true + my-fancy-feature: enabled + another-cool-feature: disabled + yet-another-feature: allowed ``` ### Strategies to add new APIs for experimental features From 908729d123e947c79aa53fd53bc4f1b8e1c970b2 Mon Sep 17 00:00:00 2001 From: slinkydeveloper Date: Tue, 1 Jun 2021 09:27:31 +0200 Subject: [PATCH 3/6] goimports Signed-off-by: Francesco Guardiani --- test/experimental/main_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/experimental/main_test.go b/test/experimental/main_test.go index 0356202305a..af45258aaf2 100644 --- a/test/experimental/main_test.go +++ b/test/experimental/main_test.go @@ -24,7 +24,7 @@ import ( "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "knative.dev/eventing/pkg/apis/feature" + experimental "knative.dev/eventing/pkg/apis/feature" "knative.dev/pkg/system" // Uncomment the following line to load the gcp plugin (only required to authenticate against GKE clusters). From 36a744d95ddc0af250cc8ab0082452d079bf944e Mon Sep 17 00:00:00 2001 From: slinkydeveloper Date: Tue, 1 Jun 2021 09:40:41 +0200 Subject: [PATCH 4/6] Added some nits from #5440 Signed-off-by: Francesco Guardiani --- pkg/apis/feature/api_validation.go | 2 +- pkg/apis/feature/api_validation_test.go | 4 +++- pkg/apis/feature/features.go | 2 +- pkg/apis/feature/features_test.go | 6 ++++-- pkg/apis/feature/flag_names.go | 14 ++++++++++++++ pkg/apis/feature/store.go | 2 +- pkg/apis/feature/store_test.go | 3 ++- 7 files changed, 26 insertions(+), 7 deletions(-) create mode 100644 pkg/apis/feature/flag_names.go diff --git a/pkg/apis/feature/api_validation.go b/pkg/apis/feature/api_validation.go index 46e36455310..2a5f9226ab7 100644 --- a/pkg/apis/feature/api_validation.go +++ b/pkg/apis/feature/api_validation.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package experimental +package feature import ( "context" diff --git a/pkg/apis/feature/api_validation_test.go b/pkg/apis/feature/api_validation_test.go index f52095e0a02..86e7958ada4 100644 --- a/pkg/apis/feature/api_validation_test.go +++ b/pkg/apis/feature/api_validation_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package experimental +package feature_test import ( "context" @@ -27,6 +27,8 @@ import ( duckv1 "knative.dev/pkg/apis/duck/v1" eventingv1 "knative.dev/eventing/pkg/apis/eventing/v1" + + . "knative.dev/eventing/pkg/apis/feature" ) const flagName = "my-flag" diff --git a/pkg/apis/feature/features.go b/pkg/apis/feature/features.go index b599e5b3623..b1235588644 100644 --- a/pkg/apis/feature/features.go +++ b/pkg/apis/feature/features.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package experimental +package feature import ( "fmt" diff --git a/pkg/apis/feature/features_test.go b/pkg/apis/feature/features_test.go index 572982b1b84..00bd13e8752 100644 --- a/pkg/apis/feature/features_test.go +++ b/pkg/apis/feature/features_test.go @@ -14,14 +14,16 @@ See the License for the specific language governing permissions and limitations under the License. */ -package experimental +package feature_test import ( "testing" "github.com/stretchr/testify/require" - . "knative.dev/pkg/configmap/testing" _ "knative.dev/pkg/system/testing" + + . "knative.dev/eventing/pkg/apis/feature" + . "knative.dev/pkg/configmap/testing" ) func TestFlags_IsEnabled_NilMap(t *testing.T) { diff --git a/pkg/apis/feature/flag_names.go b/pkg/apis/feature/flag_names.go new file mode 100644 index 00000000000..18b8968a721 --- /dev/null +++ b/pkg/apis/feature/flag_names.go @@ -0,0 +1,14 @@ +/* +Copyright 2021 The Knative Authors +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package feature diff --git a/pkg/apis/feature/store.go b/pkg/apis/feature/store.go index 8814517ff68..cfa78baf9fb 100644 --- a/pkg/apis/feature/store.go +++ b/pkg/apis/feature/store.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package experimental +package feature import ( "context" diff --git a/pkg/apis/feature/store_test.go b/pkg/apis/feature/store_test.go index 53456314ee2..dad957a2458 100644 --- a/pkg/apis/feature/store_test.go +++ b/pkg/apis/feature/store_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package experimental +package feature_test import ( "context" @@ -23,6 +23,7 @@ import ( "github.com/stretchr/testify/require" logtesting "knative.dev/pkg/logging/testing" + . "knative.dev/eventing/pkg/apis/feature" . "knative.dev/pkg/configmap/testing" ) From ed0439880815fb7af7013ea2a580c1d7b099211a Mon Sep 17 00:00:00 2001 From: slinkydeveloper Date: Tue, 1 Jun 2021 09:42:08 +0200 Subject: [PATCH 5/6] Boilerplate Signed-off-by: Francesco Guardiani --- pkg/apis/feature/flag_names.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/apis/feature/flag_names.go b/pkg/apis/feature/flag_names.go index 18b8968a721..5d7b554ea45 100644 --- a/pkg/apis/feature/flag_names.go +++ b/pkg/apis/feature/flag_names.go @@ -1,9 +1,12 @@ /* Copyright 2021 The Knative Authors + Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 + Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. From 3787ec86a03769c8c964940e3a8777dd8319dab2 Mon Sep 17 00:00:00 2001 From: slinkydeveloper Date: Tue, 1 Jun 2021 10:00:05 +0200 Subject: [PATCH 6/6] Fixed bad links in the doc and bad name Signed-off-by: Francesco Guardiani --- docs/experimental-features.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/experimental-features.md b/docs/experimental-features.md index 4c95a7c0bcd..fed35f8486d 100644 --- a/docs/experimental-features.md +++ b/docs/experimental-features.md @@ -53,7 +53,7 @@ similar to and [Knative Serving feature flags](https://github.com/knative/serving/blob/master/pkg/apis/config/features.go). When the feature is implemented, we provide a Golang API to check if the feature -is enabled or not: [`experimental.FromContext(ctx).IsEnabled(featureName)`](../pkg/apis/experimental). +is enabled or not: [`feature.FromContext(ctx).IsEnabled(featureName)`](../pkg/apis/feature). There are 3 feature states: @@ -68,7 +68,7 @@ variable of their knative setup: apiVersion: v1 kind: ConfigMap metadata: - name: config-experimental-features + name: config-features namespace: knative-eventing data: my-fancy-feature: enabled @@ -88,13 +88,13 @@ features: should not be specified, and `x-kubernetes-preserve-unknown-fields` should be used instead. Then, in the webhook, you can reject resources with fields related to experimental features when validating the input CR using - [`experimental.ValidateAPIFields()`](../pkg/apis/experimental/api_validation.go) + [`feature.ValidateAPIFields()`](../pkg/apis/feature/api_validation.go) - Alternative to the above approach, if the feature affects a whole resource, and the API is a single value, that is not an object or an array of values, use an annotation. Then, in the webhook, you can reject resources with annotations related to experimental features when validating the input CR using - [`experimental.ValidateAnnotations()`](../pkg/apis/experimental/api_validation.go). + [`feature.ValidateAnnotations()`](../pkg/apis/feature/api_validation.go). This approach is not suggested in case adding an API field is doable as described above, but there might be some situations when this is not possible, where an annotation is more suited as the feature API. For example: