From d0ed31d612f38b5ea69576449ff8cf01ec6f2427 Mon Sep 17 00:00:00 2001 From: Carolyn Van Slyck Date: Wed, 19 Apr 2023 17:19:30 -0500 Subject: [PATCH] Fix binding configuration file settings to command flags When we were binding viper values to the cobra flags, it was occuring before the config file was loaded, so if you configured a flag in the config file, it wasn't persisting the correct value to the variable bound to the flag. When the flag was not bound to the global porter Config.Data, such as porter build --no-lint, the configuration file values were not applied. This is the regression describe in #2735 which was caused by apply cobra values to viper too soon (before the config file was loaded). I have split our viper configuration into a three step process: 1. Configure the viper instance, for example binding custom open telemetry environment variables. 2. Bind viper to the cobra flags, AFTER the config file has been loaded. 3. Apply the consolidated configuration in viper (which now contains both cobra flags, env vars and the config file) back to the global config in Config.Data This ensures that viper has all relevant configuration loaded before binding its value back to the original command flags (i.e. our variables in code). Fixes #2735 Signed-off-by: Carolyn Van Slyck --- cmd/porter/main_test.go | 270 +++++++++++++++++- pkg/cli/config.go | 9 +- pkg/config/config_test.go | 11 +- pkg/config/loader.go | 46 +-- tests/integration/build_test.go | 36 +++ .../bundle-with-lint-error/porter.yaml | 32 +++ 6 files changed, 375 insertions(+), 29 deletions(-) create mode 100644 tests/integration/testdata/bundles/bundle-with-lint-error/porter.yaml diff --git a/cmd/porter/main_test.go b/cmd/porter/main_test.go index 88611d717b..e441ce4a56 100644 --- a/cmd/porter/main_test.go +++ b/cmd/porter/main_test.go @@ -6,6 +6,8 @@ import ( "strings" "testing" + "get.porter.sh/porter/pkg" + "get.porter.sh/porter/pkg/config" "get.porter.sh/porter/pkg/experimental" "get.porter.sh/porter/pkg/porter" "github.com/stretchr/testify/assert" @@ -80,12 +82,16 @@ func TestHelp(t *testing.T) { }) } +// Validate that porter is correctly binding experimental which is a flag on some commands AND is persisted on config.Data +// This is a regression test to ensure that we are applying our configuration from viper and cobra in the proper order +// such that flags defined on config.Data are persisted. +// I'm testing both experimental and verbosity because honestly, I've seen both break enough that I'd rather have excessive test than see it break again. func TestExperimentalFlags(t *testing.T) { // do not run in parallel expEnvVar := "PORTER_EXPERIMENTAL" os.Unsetenv(expEnvVar) - t.Run("flag unset, env unset", func(t *testing.T) { + t.Run("default", func(t *testing.T) { p := porter.NewTestPorter(t) defer p.Close() @@ -106,7 +112,7 @@ func TestExperimentalFlags(t *testing.T) { assert.True(t, p.Config.IsFeatureEnabled(experimental.FlagNoopFeature)) }) - t.Run("flag unset, env set", func(t *testing.T) { + t.Run("env set", func(t *testing.T) { os.Setenv(expEnvVar, experimental.NoopFeature) defer os.Unsetenv(expEnvVar) @@ -119,4 +125,264 @@ func TestExperimentalFlags(t *testing.T) { assert.True(t, p.Config.IsFeatureEnabled(experimental.FlagNoopFeature)) }) + + t.Run("cfg set", func(t *testing.T) { + p := porter.NewTestPorter(t) + defer p.Close() + + cfg := []byte(`experimental: [no-op]`) + require.NoError(t, p.FileSystem.WriteFile("/home/myuser/.porter/config.yaml", cfg, pkg.FileModeWritable)) + cmd := buildRootCommandFrom(p.Porter) + cmd.SetArgs([]string{"install"}) + cmd.Execute() + + assert.True(t, p.Config.IsFeatureEnabled(experimental.FlagNoopFeature)) + }) + + t.Run("flag set, cfg set", func(t *testing.T) { + p := porter.NewTestPorter(t) + defer p.Close() + + cfg := []byte(`experimental: []`) + require.NoError(t, p.FileSystem.WriteFile("/home/myuser/.porter/config.yaml", cfg, pkg.FileModeWritable)) + cmd := buildRootCommandFrom(p.Porter) + cmd.SetArgs([]string{"install", "--experimental", "no-op"}) + cmd.Execute() + + assert.True(t, p.Config.IsFeatureEnabled(experimental.FlagNoopFeature)) + }) + + t.Run("flag set, env set", func(t *testing.T) { + os.Setenv(expEnvVar, "") + defer os.Unsetenv(expEnvVar) + + p := porter.NewTestPorter(t) + defer p.Close() + + cmd := buildRootCommandFrom(p.Porter) + cmd.SetArgs([]string{"install", "--experimental", "no-op"}) + cmd.Execute() + + assert.True(t, p.Config.IsFeatureEnabled(experimental.FlagNoopFeature)) + }) + + t.Run("env set, cfg set", func(t *testing.T) { + os.Setenv(expEnvVar, "") + defer os.Unsetenv(expEnvVar) + + p := porter.NewTestPorter(t) + defer p.Close() + + cfg := []byte(`experimental: [no-op]`) + require.NoError(t, p.FileSystem.WriteFile("/home/myuser/.porter/config.yaml", cfg, pkg.FileModeWritable)) + cmd := buildRootCommandFrom(p.Porter) + cmd.SetArgs([]string{"install"}) + cmd.Execute() + + assert.False(t, p.Config.IsFeatureEnabled(experimental.FlagNoopFeature)) + }) +} + +// Validate that porter is correctly binding verbosity which is a flag on all commands AND is persisted on config.Data +// This is a regression test to ensure that we are applying our configuration from viper and cobra in the proper order +// such that flags defined on config.Data are persisted. +// I'm testing both experimental and verbosity because honestly, I've seen both break enough that I'd rather have excessive test than see it break again. +func TestVerbosity(t *testing.T) { + // do not run in parallel + envVar := "PORTER_VERBOSITY" + os.Unsetenv(envVar) + + t.Run("default", func(t *testing.T) { + p := porter.NewTestPorter(t) + defer p.Close() + + cmd := buildRootCommandFrom(p.Porter) + cmd.SetArgs([]string{"install"}) + cmd.Execute() + assert.Equal(t, config.LogLevelInfo, p.Config.GetVerbosity()) + }) + + t.Run("flag set", func(t *testing.T) { + p := porter.NewTestPorter(t) + defer p.Close() + + cmd := buildRootCommandFrom(p.Porter) + cmd.SetArgs([]string{"install", "--verbosity=debug"}) + cmd.Execute() + + assert.Equal(t, config.LogLevelDebug, p.Config.GetVerbosity()) + }) + + t.Run("env set", func(t *testing.T) { + os.Setenv(envVar, "error") + defer os.Unsetenv(envVar) + + p := porter.NewTestPorter(t) + defer p.Close() + + cmd := buildRootCommandFrom(p.Porter) + cmd.SetArgs([]string{"install"}) + cmd.Execute() + + assert.Equal(t, config.LogLevelError, p.Config.GetVerbosity()) + }) + + t.Run("cfg set", func(t *testing.T) { + p := porter.NewTestPorter(t) + defer p.Close() + + cfg := []byte(`verbosity: warning`) + require.NoError(t, p.FileSystem.WriteFile("/home/myuser/.porter/config.yaml", cfg, pkg.FileModeWritable)) + cmd := buildRootCommandFrom(p.Porter) + cmd.SetArgs([]string{"install"}) + cmd.Execute() + + assert.Equal(t, config.LogLevelWarn, p.Config.GetVerbosity()) + }) + + t.Run("flag set, cfg set", func(t *testing.T) { + p := porter.NewTestPorter(t) + defer p.Close() + + cfg := []byte(`verbosity: debug`) + require.NoError(t, p.FileSystem.WriteFile("/home/myuser/.porter/config.yaml", cfg, pkg.FileModeWritable)) + cmd := buildRootCommandFrom(p.Porter) + cmd.SetArgs([]string{"install", "--verbosity", "warn"}) + cmd.Execute() + + assert.Equal(t, config.LogLevelWarn, p.Config.GetVerbosity()) + }) + + t.Run("flag set, env set", func(t *testing.T) { + os.Setenv(envVar, "warn") + defer os.Unsetenv(envVar) + + p := porter.NewTestPorter(t) + defer p.Close() + + cmd := buildRootCommandFrom(p.Porter) + cmd.SetArgs([]string{"install", "--verbosity=debug"}) + cmd.Execute() + + assert.Equal(t, config.LogLevelDebug, p.Config.GetVerbosity()) + }) + + t.Run("env set, cfg set", func(t *testing.T) { + os.Setenv(envVar, "warn") + defer os.Unsetenv(envVar) + + p := porter.NewTestPorter(t) + defer p.Close() + + cfg := []byte(`verbosity: debug`) + require.NoError(t, p.FileSystem.WriteFile("/home/myuser/.porter/config.yaml", cfg, pkg.FileModeWritable)) + cmd := buildRootCommandFrom(p.Porter) + cmd.SetArgs([]string{"install"}) + cmd.Execute() + + assert.Equal(t, config.LogLevelWarn, p.Config.GetVerbosity()) + }) +} + +// Validate that porter is correctly binding porter explain --output which is a flag that is NOT bound to config.Data +// This is a regression test to ensure that we are applying our configuration from viper and cobra in the proper order +// such that flags defined on a separate data structure from config.Data are persisted. +func TestExplainOutput(t *testing.T) { + // do not run in parallel + envVar := "PORTER_OUTPUT" + os.Unsetenv(envVar) + + const ref = "ghcr.io/getporter/examples/porter-hello:v0.2.0" + + t.Run("default", func(t *testing.T) { + p := porter.NewTestPorter(t) + defer p.Close() + + cmd := buildRootCommandFrom(p.Porter) + cmd.SetArgs([]string{"explain", ref}) + require.NoError(t, cmd.Execute(), "explain failed") + + assert.Contains(t, "Name: examples/porter-hello", p.TestConfig.TestContext.GetOutput(), "explain should have output plaintext") + }) + + t.Run("flag set", func(t *testing.T) { + p := porter.NewTestPorter(t) + defer p.Close() + + cmd := buildRootCommandFrom(p.Porter) + cmd.SetArgs([]string{"explain", ref, "--output=json"}) + require.NoError(t, cmd.Execute(), "explain failed") + + assert.Contains(t, `"name": "examples/porter-hello",`, p.TestConfig.TestContext.GetOutput(), "explain should have output json") + }) + + t.Run("env set", func(t *testing.T) { + os.Setenv(envVar, "json") + defer os.Unsetenv(envVar) + + p := porter.NewTestPorter(t) + defer p.Close() + + cmd := buildRootCommandFrom(p.Porter) + cmd.SetArgs([]string{"explain", ref}) + require.NoError(t, cmd.Execute(), "explain failed") + + assert.Contains(t, `"name": "examples/porter-hello",`, p.TestConfig.TestContext.GetOutput(), "explain should have output json") + }) + + t.Run("cfg set", func(t *testing.T) { + p := porter.NewTestPorter(t) + defer p.Close() + + cfg := []byte(`output: json`) + require.NoError(t, p.FileSystem.WriteFile("/home/myuser/.porter/config.yaml", cfg, pkg.FileModeWritable)) + cmd := buildRootCommandFrom(p.Porter) + cmd.SetArgs([]string{"explain", ref}) + require.NoError(t, cmd.Execute(), "explain failed") + + assert.Contains(t, `"name": "examples/porter-hello",`, p.TestConfig.TestContext.GetOutput(), "explain should have output json") + }) + + t.Run("flag set, cfg set", func(t *testing.T) { + p := porter.NewTestPorter(t) + defer p.Close() + + cfg := []byte(`output: json`) + require.NoError(t, p.FileSystem.WriteFile("/home/myuser/.porter/config.yaml", cfg, pkg.FileModeWritable)) + cmd := buildRootCommandFrom(p.Porter) + cmd.SetArgs([]string{"explain", ref, "--output=yaml"}) + require.NoError(t, cmd.Execute(), "explain failed") + + assert.Contains(t, `- name: name`, p.TestConfig.TestContext.GetOutput(), "explain should have output json") + }) + + t.Run("flag set, env set", func(t *testing.T) { + os.Setenv(envVar, "json") + defer os.Unsetenv(envVar) + + p := porter.NewTestPorter(t) + defer p.Close() + + cmd := buildRootCommandFrom(p.Porter) + cmd.SetArgs([]string{"explain", ref, "--output=yaml"}) + require.NoError(t, cmd.Execute(), "explain failed") + + assert.Contains(t, `- name: name`, p.TestConfig.TestContext.GetOutput(), "explain should have output json") + }) + + t.Run("env set, cfg set", func(t *testing.T) { + os.Setenv(envVar, "yaml") + defer os.Unsetenv(envVar) + + p := porter.NewTestPorter(t) + defer p.Close() + + cfg := []byte(`output: json`) + require.NoError(t, p.FileSystem.WriteFile("/home/myuser/.porter/config.yaml", cfg, pkg.FileModeWritable)) + cmd := buildRootCommandFrom(p.Porter) + cmd.SetArgs([]string{"explain", ref}) + require.NoError(t, cmd.Execute(), "explain failed") + + assert.Contains(t, `- name: name`, p.TestConfig.TestContext.GetOutput(), "explain should have output json") + }) } diff --git a/pkg/cli/config.go b/pkg/cli/config.go index dda1954a26..ad98f053ac 100644 --- a/pkg/cli/config.go +++ b/pkg/cli/config.go @@ -16,11 +16,7 @@ import ( // * Config file // * Flag default (lowest) func LoadHierarchicalConfig(cmd *cobra.Command) config.DataStoreLoaderFunc { - return config.LoadFromViper(func(v *viper.Viper) { - v.AutomaticEnv() - v.SetEnvPrefix("PORTER") - v.SetEnvKeyReplacer(strings.NewReplacer("-", "_")) - + bindCobraFlagsToViper := func(v *viper.Viper) { // Apply the configuration file value to the flag when the flag is not set flags := cmd.Flags() flags.VisitAll(func(f *pflag.Flag) { @@ -42,7 +38,8 @@ func LoadHierarchicalConfig(cmd *cobra.Command) config.DataStoreLoaderFunc { flags.Set(f.Name, val) } }) - }) + } + return config.LoadFromViper(config.BindViperToEnvironmentVariables, bindCobraFlagsToViper) } func getFlagValue(v *viper.Viper, key string) string { diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 669b5598a5..bda5383e1a 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -60,7 +60,9 @@ func TestConfigExperimentalFlags(t *testing.T) { // Do not run in parallel since we are using os.Setenv t.Run("default off", func(t *testing.T) { - c := NewTestConfig(t) + c := New() + defer c.Close() + assert.False(t, c.IsFeatureEnabled(experimental.FlagNoopFeature)) }) @@ -69,13 +71,18 @@ func TestConfigExperimentalFlags(t *testing.T) { defer os.Unsetenv("PORTER_EXPERIMENTAL") c := New() + defer c.Close() + c.DataLoader = LoadFromEnvironment() + _, err := c.Load(context.Background(), nil) require.NoError(t, err, "Load failed") assert.True(t, c.IsFeatureEnabled(experimental.FlagNoopFeature)) }) t.Run("programmatically enabled", func(t *testing.T) { - c := NewTestConfig(t) + c := New() + defer c.Close() + c.Data.ExperimentalFlags = nil c.SetExperimentalFlags(experimental.FlagNoopFeature) assert.True(t, c.IsFeatureEnabled(experimental.FlagNoopFeature)) diff --git a/pkg/config/loader.go b/pkg/config/loader.go index 249df8e138..190bdc1417 100644 --- a/pkg/config/loader.go +++ b/pkg/config/loader.go @@ -26,21 +26,23 @@ func NoopDataLoader(_ context.Context, _ *Config, _ map[string]interface{}) erro // * Config file // * Flag default (lowest) func LoadFromEnvironment() DataStoreLoaderFunc { - return LoadFromViper(func(v *viper.Viper) { - v.SetEnvPrefix("PORTER") - v.SetEnvKeyReplacer(strings.NewReplacer("-", "_", ".", "_")) - v.AutomaticEnv() - - // Bind open telemetry environment variables - // See https://github.com/open-telemetry/opentelemetry-go/tree/main/exporters/otlp/otlptrace - v.BindEnv("telemetry.endpoint", "OTEL_EXPORTER_OTLP_ENDPOINT", "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT") - v.BindEnv("telemetry.protocol", "OTEL_EXPORTER_OTLP_PROTOCOL", "OTEL_EXPORTER_OTLP_TRACES_PROTOCOL") - v.BindEnv("telemetry.insecure", "OTEL_EXPORTER_OTLP_INSECURE", "OTEL_EXPORTER_OTLP_TRACES_INSECURE") - v.BindEnv("telemetry.certificate", "OTEL_EXPORTER_OTLP_CERTIFICATE", "OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE") - v.BindEnv("telemetry.headers", "OTEL_EXPORTER_OTLP_HEADERS", "OTEL_EXPORTER_OTLP_TRACES_HEADERS") - v.BindEnv("telemetry.compression", "OTEL_EXPORTER_OTLP_COMPRESSION", "OTEL_EXPORTER_OTLP_TRACES_COMPRESSION") - v.BindEnv("telemetry.timeout", "OTEL_EXPORTER_OTLP_TIMEOUT", "OTEL_EXPORTER_OTLP_TRACES_TIMEOUT") - }) + return LoadFromViper(BindViperToEnvironmentVariables, nil) +} + +func BindViperToEnvironmentVariables(v *viper.Viper) { + v.SetEnvPrefix("PORTER") + v.SetEnvKeyReplacer(strings.NewReplacer("-", "_", ".", "_")) + v.AutomaticEnv() + + // Bind open telemetry environment variables + // See https://github.com/open-telemetry/opentelemetry-go/tree/main/exporters/otlp/otlptrace + v.BindEnv("telemetry.endpoint", "OTEL_EXPORTER_OTLP_ENDPOINT", "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT") + v.BindEnv("telemetry.protocol", "OTEL_EXPORTER_OTLP_PROTOCOL", "OTEL_EXPORTER_OTLP_TRACES_PROTOCOL") + v.BindEnv("telemetry.insecure", "OTEL_EXPORTER_OTLP_INSECURE", "OTEL_EXPORTER_OTLP_TRACES_INSECURE") + v.BindEnv("telemetry.certificate", "OTEL_EXPORTER_OTLP_CERTIFICATE", "OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE") + v.BindEnv("telemetry.headers", "OTEL_EXPORTER_OTLP_HEADERS", "OTEL_EXPORTER_OTLP_TRACES_HEADERS") + v.BindEnv("telemetry.compression", "OTEL_EXPORTER_OTLP_COMPRESSION", "OTEL_EXPORTER_OTLP_TRACES_COMPRESSION") + v.BindEnv("telemetry.timeout", "OTEL_EXPORTER_OTLP_TIMEOUT", "OTEL_EXPORTER_OTLP_TRACES_TIMEOUT") } // LoadFromFilesystem loads data with the following precedence: @@ -48,11 +50,11 @@ func LoadFromEnvironment() DataStoreLoaderFunc { // * Flag default (lowest) // This is used for testing only. func LoadFromFilesystem() DataStoreLoaderFunc { - return LoadFromViper(func(v *viper.Viper) {}) + return LoadFromViper(nil, nil) } // LoadFromViper loads data from a configurable viper instance. -func LoadFromViper(viperCfg func(v *viper.Viper)) DataStoreLoaderFunc { +func LoadFromViper(viperCfg func(v *viper.Viper), cobraCfg func(v *viper.Viper)) DataStoreLoaderFunc { return func(ctx context.Context, cfg *Config, templateData map[string]interface{}) error { home, _ := cfg.GetHomeDir() @@ -125,8 +127,14 @@ func LoadFromViper(viperCfg func(v *viper.Viper)) DataStoreLoaderFunc { } } - if err = v.Unmarshal(&cfg.Data); err != nil { - log.Error(fmt.Errorf("error unmarshaling viper config as porter config: %w", err)) + // Porter can be used through the CLI, in which case give it a chance to hook up cobra command flags to viper + if cobraCfg != nil { + cobraCfg(v) + } + + // Bind viper back to the configuration data only after all viper and cobra setup is completed + if err := v.Unmarshal(&cfg.Data); err != nil { + return fmt.Errorf("error unmarshaling viper config as porter config: %w", err) } cfg.viper = v diff --git a/tests/integration/build_test.go b/tests/integration/build_test.go index 47fbceac87..aa74f771d8 100644 --- a/tests/integration/build_test.go +++ b/tests/integration/build_test.go @@ -4,9 +4,11 @@ package integration import ( "encoding/json" + "os" "path/filepath" "testing" + "get.porter.sh/porter/pkg" "get.porter.sh/porter/pkg/build" "get.porter.sh/porter/pkg/cnab" "get.porter.sh/porter/pkg/yaml" @@ -39,6 +41,40 @@ func TestBuild(t *testing.T) { } +// This test uses build and the --no-lint flag, which is not a global flag defined on config.DataStore, +// to validate that when a flag value is set via the configuration file, environment variable or directly with the flag +// that the value binds properly to the variable. +// It is a regression test for our cobra+viper configuration setup and was created in response to this regression +// https://github.com/getporter/porter/issues/2735 +func TestBuild_ConfigureNoLintThreeWays(t *testing.T) { + test, err := tester.NewTest(t) + defer test.Close() + require.NoError(t, err, "test setup failed") + + // Use a bundle that will trigger a lint error, it can only be successfully built when --no-lint is set + require.NoError(t, shx.Copy("testdata/bundles/bundle-with-lint-error/porter.yaml", test.TestDir)) + test.Chdir(test.TestDir) + + // Attempt to build the bundle, it should fail with a lint error + _, _, err = test.RunPorter("build") + require.ErrorContains(t, err, "lint errors were detected") + + // Build the bundle and disable the linter with --no-lint + test.RequirePorter("build", "--no-lint") + + // Build the bundle and disable the linter with PORTER_NO_LINT + _, _, err = test.RunPorterWith(func(cmd *shx.PreparedCommand) { + cmd.Args("build").Env("PORTER_NO_LINT=true") + }) + require.NoError(t, err, "expected the build to pass when PORTER_NO_LINT=true is specified") + + // Build the bundle and disable the linter with the configuration file + disableAutoBuildCfg := []byte(`no-lint: true`) + err = os.WriteFile(filepath.Join(test.PorterHomeDir, "config.yaml"), disableAutoBuildCfg, pkg.FileModeWritable) + require.NoError(t, err, "Failed to write out the porter configuration file") + test.RequirePorter("build") +} + func TestRebuild(t *testing.T) { test, err := tester.NewTest(t) defer test.Close() diff --git a/tests/integration/testdata/bundles/bundle-with-lint-error/porter.yaml b/tests/integration/testdata/bundles/bundle-with-lint-error/porter.yaml new file mode 100644 index 0000000000..4f99c0c91b --- /dev/null +++ b/tests/integration/testdata/bundles/bundle-with-lint-error/porter.yaml @@ -0,0 +1,32 @@ +# This bundle is designed to cause the porter lint/build commands to fail +schemaType: Bundle +schemaVersion: 1.0.1 +name: exec-mixin-lint-error +version: 0.1.0 +description: "This bundle is designed to cause the porter lint/build commands to fail, use --no-lint to use it anyway" +registry: "localhost:5000" + +mixins: + - exec + +install: + - exec: + description: trigger a lint error + command: bash + flags: + # This won't work because it's quoted improperly https://getporter.org/best-practices/exec-mixin/ + c: echo "Hello World" + +upgrade: + - exec: + description: "World 2.0" + command: echo + arguments: + - upgrade + +uninstall: + - exec: + description: "Uninstall Hello World" + command: echo + arguments: + - uninstall