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/docs/experimental-features.md b/docs/experimental-features.md index fc62d9b3315..fed35f8486d 100644 --- a/docs/experimental-features.md +++ b/docs/experimental-features.md @@ -53,7 +53,13 @@ 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: + +* 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: @@ -62,11 +68,12 @@ variable of their knative setup: apiVersion: v1 kind: ConfigMap metadata: - name: config-experimental-features + name: config-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 @@ -81,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: diff --git a/pkg/apis/experimental/api_validation.go b/pkg/apis/feature/api_validation.go similarity index 99% rename from pkg/apis/experimental/api_validation.go rename to pkg/apis/feature/api_validation.go index 46e36455310..2a5f9226ab7 100644 --- a/pkg/apis/experimental/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/experimental/api_validation_test.go b/pkg/apis/feature/api_validation_test.go similarity index 91% rename from pkg/apis/experimental/api_validation_test.go rename to pkg/apis/feature/api_validation_test.go index 1e2320a8653..86e7958ada4 100644 --- a/pkg/apis/experimental/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" @@ -43,8 +45,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 +54,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 +69,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 +88,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 +108,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 +124,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 +161,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 +176,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 61% rename from pkg/apis/experimental/features.go rename to pkg/apis/feature/features.go index b10ce3e1990..b1235588644 100644 --- a/pkg/apis/experimental/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" @@ -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 73% rename from pkg/apis/experimental/features_test.go rename to pkg/apis/feature/features_test.go index 738c8809ac0..00bd13e8752 100644 --- a/pkg/apis/experimental/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) { @@ -34,10 +36,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 +48,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/feature/flag_names.go b/pkg/apis/feature/flag_names.go new file mode 100644 index 00000000000..5d7b554ea45 --- /dev/null +++ b/pkg/apis/feature/flag_names.go @@ -0,0 +1,17 @@ +/* +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/experimental/store.go b/pkg/apis/feature/store.go similarity index 91% rename from pkg/apis/experimental/store.go rename to pkg/apis/feature/store.go index 20aacf7d10c..cfa78baf9fb 100644 --- a/pkg/apis/experimental/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" @@ -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 96% rename from pkg/apis/experimental/store_test.go rename to pkg/apis/feature/store_test.go index 53456314ee2..dad957a2458 100644 --- a/pkg/apis/experimental/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" ) 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..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/experimental" + 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).