Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce config map watcher for feature flags #2637

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 90 additions & 0 deletions pkg/apis/config/feature_flags.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
/*
Copyright 2020 The Tekton 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 config

import (
"fmt"
"os"
"strconv"

corev1 "k8s.io/api/core/v1"
)

const (
disableHomeEnvOverwriteKey = "disable-home-env-overwrite"
disableWorkingDirOverwriteKey = "disable-working-directory-overwrite"
disableAffinityAssistantKey = "disable-affinity-assistant"
runningInEnvWithInjectedSidecarsKey = "running-in-environment-with-injected-sidecars"
DefaultDisableHomeEnvOverwrite = false
DefaultDisableWorkingDirOverwrite = false
DefaultDisableAffinityAssistant = false
DefaultRunningInEnvWithInjectedSidecars = true
)

// FeatureFlags holds the features configurations
// +k8s:deepcopy-gen=true
type FeatureFlags struct {
DisableHomeEnvOverwrite bool
DisableWorkingDirOverwrite bool
DisableAffinityAssistant bool
RunningInEnvWithInjectedSidecars bool
}

// GetFeatureFlagsConfigName returns the name of the configmap containing all
// feature flags.
func GetFeatureFlagsConfigName() string {
if e := os.Getenv("CONFIG_FEATURE_FLAGS_NAME"); e != "" {
return e
}
return "feature-flags"
}

// 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 {
if cfg, ok := cfgMap[key]; ok {
value, err := strconv.ParseBool(cfg)
if err != nil {
return fmt.Errorf("failed parsing feature flags config %q: %v", cfg, err)
}
*feature = value
return nil
}
*feature = defaultValue
return nil
}

tc := FeatureFlags{}
if err := setFeature(disableHomeEnvOverwriteKey, DefaultDisableHomeEnvOverwrite, &tc.DisableHomeEnvOverwrite); err != nil {
return nil, err
}
if err := setFeature(disableWorkingDirOverwriteKey, DefaultDisableWorkingDirOverwrite, &tc.DisableWorkingDirOverwrite); err != nil {
return nil, err
}
if err := setFeature(disableAffinityAssistantKey, DefaultDisableAffinityAssistant, &tc.DisableAffinityAssistant); err != nil {
return nil, err
}
if err := setFeature(runningInEnvWithInjectedSidecarsKey, DefaultRunningInEnvWithInjectedSidecars, &tc.RunningInEnvWithInjectedSidecars); err != nil {
return nil, err
}
return &tc, nil
}

// NewFeatureFlagsFromConfigMap returns a Config for the given configmap
func NewFeatureFlagsFromConfigMap(config *corev1.ConfigMap) (*FeatureFlags, error) {
return NewFeatureFlagsFromMap(config.Data)
}
105 changes: 105 additions & 0 deletions pkg/apis/config/feature_flags_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
/*
Copyright 2020 The Tekton 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 config

import (
"os"
"testing"

"github.com/google/go-cmp/cmp"
test "github.com/tektoncd/pipeline/pkg/reconciler/testing"
"github.com/tektoncd/pipeline/test/diff"
)

func TestNewFeatureFlagsFromConfigMap(t *testing.T) {
type testCase struct {
expectedConfig *FeatureFlags
fileName string
}

testCases := []testCase{
{
expectedConfig: &FeatureFlags{
RunningInEnvWithInjectedSidecars: DefaultRunningInEnvWithInjectedSidecars,
},
fileName: GetFeatureFlagsConfigName(),
},
{
expectedConfig: &FeatureFlags{
DisableHomeEnvOverwrite: true,
DisableWorkingDirOverwrite: true,
DisableAffinityAssistant: true,
RunningInEnvWithInjectedSidecars: false,
},
fileName: "feature-flags-all-flags-set",
},
}

for _, tc := range testCases {
verifyConfigFileWithExpectedFeatureFlagsConfig(t, tc.fileName, tc.expectedConfig)
}
}

func TestNewFeatureFlagsFromEmptyConfigMap(t *testing.T) {
FeatureFlagsConfigEmptyName := "feature-flags-empty"
expectedConfig := &FeatureFlags{
RunningInEnvWithInjectedSidecars: true,
}
verifyConfigFileWithExpectedFeatureFlagsConfig(t, FeatureFlagsConfigEmptyName, expectedConfig)
}

func TestGetFeatureFlagsConfigName(t *testing.T) {
for _, tc := range []struct {
description string
featureFlagEnvValue string
expected string
}{{
description: "Feature flags config value not set",
featureFlagEnvValue: "",
expected: "feature-flags",
}, {
description: "Feature flags config value set",
featureFlagEnvValue: "feature-flags-test",
expected: "feature-flags-test",
}} {
t.Run(tc.description, func(t *testing.T) {
original := os.Getenv("CONFIG_FEATURE_FLAGS_NAME")
defer t.Cleanup(func() {
os.Setenv("CONFIG_FEATURE_FLAGS_NAME", original)
})
if tc.featureFlagEnvValue != "" {
os.Setenv("CONFIG_FEATURE_FLAGS_NAME", tc.featureFlagEnvValue)
}
got := GetFeatureFlagsConfigName()
want := tc.expected
if got != want {
t.Errorf("GetFeatureFlagsConfigName() = %s, want %s", got, want)
}
})
}
}

func verifyConfigFileWithExpectedFeatureFlagsConfig(t *testing.T, fileName string, expectedConfig *FeatureFlags) {
cm := test.ConfigMapFromTestFile(t, fileName)
if flags, err := NewFeatureFlagsFromConfigMap(cm); err == nil {
if d := cmp.Diff(flags, expectedConfig); d != "" {
t.Errorf("Diff:\n%s", diff.PrintWantGot(d))
}
} else {
t.Errorf("NewFeatureFlagsFromConfigMap(actual) = %v", err)
}
}
24 changes: 19 additions & 5 deletions pkg/apis/config/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ type cfgKey struct{}
// Config holds the collection of configurations that we attach to contexts.
// +k8s:deepcopy-gen=false
type Config struct {
Defaults *Defaults
Defaults *Defaults
FeatureFlags *FeatureFlags
}

// FromContext extracts a Config from the provided context.
Expand All @@ -46,8 +47,10 @@ func FromContextOrDefaults(ctx context.Context) *Config {
return cfg
}
defaults, _ := NewDefaultsFromMap(map[string]string{})
featureFlags, _ := NewFeatureFlagsFromMap(map[string]string{})
return &Config{
Defaults: defaults,
Defaults: defaults,
FeatureFlags: featureFlags,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you clarify why we need FeatureFlags to be separated from Defaults?
AFAIU feature flags are a special type of defaults, that take a boolean value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for working on this, I've been wondering why we used the configStore for pipelineruns and not for taskruns too.
My main questions on this is whether we really need the extra module, or if we could just use the existing one and add extra flags to it?

Thank you for the detailed review. Please let me know if the following sounds correct. I'd be happy to update the PR if needed to merge the two sets of options.

I have implemented the draft this way mainly because the feature-flags and config-defaults are different config maps. Modules using the config store are only minimally affected by this: they use config store the same way, i.e. by passing the context, just specify the flags under FeatureFlags "option group" as Defaults and FeatureFlags are both structs under the same config module.

If combining the two sets is preferred, please let me know and I will update the PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to like having it seperate 🤔
/cc @sbwsg

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, feature-flags is already a separate ConfigMap. If we wanted to merge it with config-defaults then I'd prefer that be a discussion/PR unto itself rather than mix it in with a behavioral change such as this. @afrittoli if you prefer to go that route do you want to pursue it at an upcoming WG?

}
}

Expand All @@ -67,10 +70,11 @@ type Store struct {
func NewStore(logger configmap.Logger, onAfterStore ...func(name string, value interface{})) *Store {
store := &Store{
UntypedStore: configmap.NewUntypedStore(
"defaults",
"defaults/features",
logger,
configmap.Constructors{
GetDefaultsConfigName(): NewDefaultsFromConfigMap,
GetDefaultsConfigName(): NewDefaultsFromConfigMap,
GetFeatureFlagsConfigName(): NewFeatureFlagsFromConfigMap,
},
onAfterStore...,
),
Expand All @@ -86,7 +90,17 @@ func (s *Store) ToContext(ctx context.Context) context.Context {

// Load creates a Config from the current config state of the Store.
func (s *Store) Load() *Config {
defaults := s.UntypedLoad(GetDefaultsConfigName())
if defaults == nil {
defaults, _ = NewDefaultsFromMap(map[string]string{})
}
featureFlags := s.UntypedLoad(GetFeatureFlagsConfigName())
if featureFlags == nil {
featureFlags, _ = NewFeatureFlagsFromMap(map[string]string{})
}

return &Config{
Defaults: s.UntypedLoad(GetDefaultsConfigName()).(*Defaults).DeepCopy(),
Defaults: defaults.(*Defaults).DeepCopy(),
FeatureFlags: featureFlags.(*FeatureFlags).DeepCopy(),
}
}
18 changes: 14 additions & 4 deletions pkg/apis/config/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,24 @@ import (
)

func TestStoreLoadWithContext(t *testing.T) {
store := NewStore(logtesting.TestLogger(t))
defaultConfig := test.ConfigMapFromTestFile(t, "config-defaults")
featuresConfig := test.ConfigMapFromTestFile(t, "feature-flags-all-flags-set")

expectedDefaults, _ := NewDefaultsFromConfigMap(defaultConfig)
expectedFeatures, _ := NewFeatureFlagsFromConfigMap(featuresConfig)

expected := &Config{
Defaults: expectedDefaults,
FeatureFlags: expectedFeatures,
}

store := NewStore(logtesting.TestLogger(t))
store.OnConfigChanged(defaultConfig)
store.OnConfigChanged(featuresConfig)

config := FromContext(store.ToContext(context.Background()))

expected, _ := NewDefaultsFromConfigMap(defaultConfig)
if d := cmp.Diff(config.Defaults, expected); d != "" {
t.Errorf("Unexpected default config %s", diff.PrintWantGot(d))
if d := cmp.Diff(config, expected); d != "" {
t.Errorf("Unexpected config %s", diff.PrintWantGot(d))
}
}
24 changes: 24 additions & 0 deletions pkg/apis/config/testdata/feature-flags-all-flags-set.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Copyright 2020 The Tekton 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
#
# https://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.

apiVersion: v1
kind: ConfigMap
metadata:
name: feature-flags
namespace: tekton-pipelines
data:
disable-home-env-overwrite: "true"
disable-working-directory-overwrite: "true"
disable-affinity-assistant: "true"
running-in-environment-with-injected-sidecars: "false"
26 changes: 26 additions & 0 deletions pkg/apis/config/testdata/feature-flags-empty.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# Copyright 2020 The Tekton 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
#
# https://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.

apiVersion: v1
kind: ConfigMap
metadata:
name: feature-flags
namespace: tekton-pipelines
data:
_example: |
################################
# #
# EXAMPLE CONFIGURATION #
# #
################################
24 changes: 24 additions & 0 deletions pkg/apis/config/testdata/feature-flags.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Copyright 2020 The Tekton 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
#
# https://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.

apiVersion: v1
kind: ConfigMap
metadata:
name: feature-flags
namespace: tekton-pipelines
data:
disable-home-env-overwrite: "false"
disable-working-directory-overwrite: "false"
disable-affinity-assistant: "false"
running-in-environment-with-injected-sidecars: "true"
16 changes: 16 additions & 0 deletions pkg/apis/config/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading