From dd39d693cd5612efe916132c4c550d46df7bdafa Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Tue, 5 Nov 2024 16:31:47 -0600 Subject: [PATCH 1/3] Update feature gate logging to include default on After the 5.7 release, when AutoUserSchemaCreate was graduated to default on/true, we discovered that our current system (and the underlying featuregate implementation) treats features explicitly turned on by the user differently than features turned on by default. This PR updates that logging to make clear that the features are those specifically requested by the user (kept as a string to help debugging) and revises the 'ShowGates' function to include those set through defaults. Issues: [PGO-1824] --- cmd/postgres-operator/main.go | 3 ++- internal/feature/features.go | 22 +++++++++++++++++++--- internal/feature/features_test.go | 16 ++++++++++++---- internal/upgradecheck/http_test.go | 2 +- 4 files changed, 34 insertions(+), 9 deletions(-) diff --git a/cmd/postgres-operator/main.go b/cmd/postgres-operator/main.go index b2f8ae49b..841cf9bb6 100644 --- a/cmd/postgres-operator/main.go +++ b/cmd/postgres-operator/main.go @@ -134,7 +134,8 @@ func main() { features := feature.NewGate() assertNoError(features.Set(os.Getenv("PGO_FEATURE_GATES"))) - log.Info("feature gates enabled", "PGO_FEATURE_GATES", features.String()) + // This logs just the feature gates as set by the user + log.Info("feature gates enabled during deployment", "PGO_FEATURE_GATES", features.String()) cfg, err := runtime.GetConfig() assertNoError(err) diff --git a/internal/feature/features.go b/internal/feature/features.go index db424ead4..f8335c695 100644 --- a/internal/feature/features.go +++ b/internal/feature/features.go @@ -42,6 +42,9 @@ package feature import ( "context" + "fmt" + "sort" + "strings" "k8s.io/component-base/featuregate" ) @@ -52,6 +55,7 @@ type Feature = featuregate.Feature type Gate interface { Enabled(Feature) bool String() string + GetAll() map[Feature]featuregate.FeatureSpec } // MutableGate contains features that can be enabled or disabled. @@ -122,11 +126,23 @@ func NewContext(ctx context.Context, gate Gate) context.Context { return context.WithValue(ctx, contextKey{}, gate) } +// ShowGates returns all the gates that are on by default +// (if not turned off by the user) or were explicitly set +// on by the user. func ShowGates(ctx context.Context) string { - featuresEnabled := "" + featuresEnabled := []string{} gate, ok := ctx.Value(contextKey{}).(Gate) if ok { - featuresEnabled = gate.String() + specs := gate.GetAll() + for feature := range specs { + // `gate.Enabled` first checks if the feature is enabled; + // then (if not explicitly set by the user), + // it checks if the feature is on/true by default + if gate.Enabled(feature) { + featuresEnabled = append(featuresEnabled, fmt.Sprintf("%s=true", feature)) + } + } } - return featuresEnabled + sort.Strings(featuresEnabled) + return strings.Join(featuresEnabled, ",") } diff --git a/internal/feature/features_test.go b/internal/feature/features_test.go index f76dd216e..042d93fc9 100644 --- a/internal/feature/features_test.go +++ b/internal/feature/features_test.go @@ -53,13 +53,21 @@ func TestContext(t *testing.T) { t.Parallel() gate := NewGate() ctx := NewContext(context.Background(), gate) - assert.Equal(t, ShowGates(ctx), "") + + // ShowGates returns all fields that are turned on, whether explicitly + // by the user or implicitly due to feature default. + // Currently, the only feature defaulting to true is `AutoCreateUserSchema`. + assert.Equal(t, ShowGates(ctx), "AutoCreateUserSchema=true") assert.NilError(t, gate.Set("TablespaceVolumes=true")) assert.Assert(t, true == Enabled(ctx, TablespaceVolumes)) - assert.Equal(t, ShowGates(ctx), "TablespaceVolumes=true") + assert.Equal(t, ShowGates(ctx), "AutoCreateUserSchema=true,TablespaceVolumes=true") - assert.NilError(t, gate.SetFromMap(map[string]bool{TablespaceVolumes: false})) + assert.NilError(t, gate.SetFromMap(map[string]bool{ + TablespaceVolumes: false, + AutoCreateUserSchema: false, + })) assert.Assert(t, false == Enabled(ctx, TablespaceVolumes)) - assert.Equal(t, ShowGates(ctx), "TablespaceVolumes=false") + assert.Assert(t, false == Enabled(ctx, AutoCreateUserSchema)) + assert.Equal(t, ShowGates(ctx), "") } diff --git a/internal/upgradecheck/http_test.go b/internal/upgradecheck/http_test.go index 9535f942e..3d2c6cba9 100644 --- a/internal/upgradecheck/http_test.go +++ b/internal/upgradecheck/http_test.go @@ -69,7 +69,7 @@ func TestCheckForUpgrades(t *testing.T) { assert.Equal(t, data.RegistrationToken, "speakFriend") assert.Equal(t, data.BridgeClustersTotal, 2) assert.Equal(t, data.PGOClustersTotal, 2) - assert.Equal(t, data.FeatureGatesEnabled, "TablespaceVolumes=true") + assert.Equal(t, data.FeatureGatesEnabled, "AutoCreateUserSchema=true,TablespaceVolumes=true") } t.Run("success", func(t *testing.T) { From 90eff1fda7ae3e06b4ec4c9b4ff1f214be2c0818 Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Tue, 5 Nov 2024 22:15:37 -0600 Subject: [PATCH 2/3] PR feedback --- cmd/postgres-operator/main.go | 4 +++- internal/feature/features.go | 31 +++++++++++++++++--------- internal/feature/features_test.go | 33 +++++++++++++++------------- internal/upgradecheck/header.go | 2 +- internal/upgradecheck/header_test.go | 5 ++++- 5 files changed, 47 insertions(+), 28 deletions(-) diff --git a/cmd/postgres-operator/main.go b/cmd/postgres-operator/main.go index 841cf9bb6..81ce51e52 100644 --- a/cmd/postgres-operator/main.go +++ b/cmd/postgres-operator/main.go @@ -134,8 +134,10 @@ func main() { features := feature.NewGate() assertNoError(features.Set(os.Getenv("PGO_FEATURE_GATES"))) + + ctx = feature.NewContext(ctx, features) // This logs just the feature gates as set by the user - log.Info("feature gates enabled during deployment", "PGO_FEATURE_GATES", features.String()) + log.Info("feature gates enabled during deployment", "PGO_FEATURE_GATES", feature.ShowAssigned(ctx)) cfg, err := runtime.GetConfig() assertNoError(err) diff --git a/internal/feature/features.go b/internal/feature/features.go index f8335c695..f16d84b73 100644 --- a/internal/feature/features.go +++ b/internal/feature/features.go @@ -43,7 +43,7 @@ package feature import ( "context" "fmt" - "sort" + "slices" "strings" "k8s.io/component-base/featuregate" @@ -54,8 +54,6 @@ type Feature = featuregate.Feature // Gate indicates what features exist and which are enabled. type Gate interface { Enabled(Feature) bool - String() string - GetAll() map[Feature]featuregate.FeatureSpec } // MutableGate contains features that can be enabled or disabled. @@ -126,13 +124,13 @@ func NewContext(ctx context.Context, gate Gate) context.Context { return context.WithValue(ctx, contextKey{}, gate) } -// ShowGates returns all the gates that are on by default -// (if not turned off by the user) or were explicitly set -// on by the user. -func ShowGates(ctx context.Context) string { +// ShowEnabled returns all the features enabled in the Gate contained in ctx. +func ShowEnabled(ctx context.Context) string { featuresEnabled := []string{} - gate, ok := ctx.Value(contextKey{}).(Gate) - if ok { + if gate, ok := ctx.Value(contextKey{}).(interface { + Gate + GetAll() map[Feature]featuregate.FeatureSpec + }); ok { specs := gate.GetAll() for feature := range specs { // `gate.Enabled` first checks if the feature is enabled; @@ -143,6 +141,19 @@ func ShowGates(ctx context.Context) string { } } } - sort.Strings(featuresEnabled) + slices.Sort(featuresEnabled) return strings.Join(featuresEnabled, ",") } + +// ShowAssigned returns the features enabled or disabled by Set and SetFromMap +// in the Gate contained in ctx. +func ShowAssigned(ctx context.Context) string { + featuresAssigned := "" + if gate, ok := ctx.Value(contextKey{}).(interface { + Gate + String() string + }); ok { + featuresAssigned = gate.String() + } + return featuresAssigned +} diff --git a/internal/feature/features_test.go b/internal/feature/features_test.go index 042d93fc9..6f7eb78ab 100644 --- a/internal/feature/features_test.go +++ b/internal/feature/features_test.go @@ -6,6 +6,7 @@ package feature import ( "context" + "strings" "testing" "gotest.tools/v3/assert" @@ -14,6 +15,7 @@ import ( func TestDefaults(t *testing.T) { t.Parallel() gate := NewGate() + ctx := NewContext(context.Background(), gate) assert.Assert(t, false == gate.Enabled(AppendCustomQueries)) assert.Assert(t, true == gate.Enabled(AutoCreateUserSchema)) @@ -24,16 +26,17 @@ func TestDefaults(t *testing.T) { assert.Assert(t, false == gate.Enabled(TablespaceVolumes)) assert.Assert(t, false == gate.Enabled(VolumeSnapshots)) - assert.Equal(t, gate.String(), "") + assert.Equal(t, ShowAssigned(ctx), "") } func TestStringFormat(t *testing.T) { t.Parallel() gate := NewGate() + ctx := NewContext(context.Background(), gate) assert.NilError(t, gate.Set("")) assert.NilError(t, gate.Set("TablespaceVolumes=true")) - assert.Equal(t, gate.String(), "TablespaceVolumes=true") + assert.Equal(t, ShowAssigned(ctx), "TablespaceVolumes=true") assert.Assert(t, true == gate.Enabled(TablespaceVolumes)) err := gate.Set("NotAGate=true") @@ -54,20 +57,20 @@ func TestContext(t *testing.T) { gate := NewGate() ctx := NewContext(context.Background(), gate) - // ShowGates returns all fields that are turned on, whether explicitly - // by the user or implicitly due to feature default. - // Currently, the only feature defaulting to true is `AutoCreateUserSchema`. - assert.Equal(t, ShowGates(ctx), "AutoCreateUserSchema=true") + assert.Equal(t, ShowAssigned(ctx), "") + assert.Assert(t, ShowEnabled(ctx) != "") // This assumes some feature is enabled by default. assert.NilError(t, gate.Set("TablespaceVolumes=true")) - assert.Assert(t, true == Enabled(ctx, TablespaceVolumes)) - assert.Equal(t, ShowGates(ctx), "AutoCreateUserSchema=true,TablespaceVolumes=true") + assert.Assert(t, Enabled(ctx, TablespaceVolumes)) + assert.Equal(t, ShowAssigned(ctx), "TablespaceVolumes=true") + assert.Assert(t, + strings.Contains(ShowEnabled(ctx), "TablespaceVolumes=true"), + "got: %v", ShowEnabled(ctx)) - assert.NilError(t, gate.SetFromMap(map[string]bool{ - TablespaceVolumes: false, - AutoCreateUserSchema: false, - })) - assert.Assert(t, false == Enabled(ctx, TablespaceVolumes)) - assert.Assert(t, false == Enabled(ctx, AutoCreateUserSchema)) - assert.Equal(t, ShowGates(ctx), "") + assert.NilError(t, gate.SetFromMap(map[string]bool{TablespaceVolumes: false})) + assert.Assert(t, !Enabled(ctx, TablespaceVolumes)) + assert.Equal(t, ShowAssigned(ctx), "TablespaceVolumes=false") + assert.Assert(t, + !strings.Contains(ShowEnabled(ctx), "TablespaceVolumes"), + "got: %v", ShowEnabled(ctx)) } diff --git a/internal/upgradecheck/header.go b/internal/upgradecheck/header.go index 766de8dd0..2deffe248 100644 --- a/internal/upgradecheck/header.go +++ b/internal/upgradecheck/header.go @@ -57,7 +57,7 @@ func generateHeader(ctx context.Context, cfg *rest.Config, crClient crclient.Cli BridgeClustersTotal: getBridgeClusters(ctx, crClient), BuildSource: os.Getenv("BUILD_SOURCE"), DeploymentID: ensureDeploymentID(ctx, crClient), - FeatureGatesEnabled: feature.ShowGates(ctx), + FeatureGatesEnabled: feature.ShowEnabled(ctx), IsOpenShift: isOpenShift, KubernetesEnv: getServerVersion(ctx, cfg), PGOClustersTotal: getManagedClusters(ctx, crClient), diff --git a/internal/upgradecheck/header_test.go b/internal/upgradecheck/header_test.go index c144e7629..1271a381e 100644 --- a/internal/upgradecheck/header_test.go +++ b/internal/upgradecheck/header_test.go @@ -136,7 +136,10 @@ func TestGenerateHeader(t *testing.T) { assert.Equal(t, len(pgoList.Items), res.PGOClustersTotal) assert.Equal(t, "1.2.3", res.PGOVersion) assert.Equal(t, server.String(), res.KubernetesEnv) - assert.Equal(t, "TablespaceVolumes=true", res.FeatureGatesEnabled) + assert.Check(t, strings.Contains( + res.FeatureGatesEnabled, + "TablespaceVolumes=true", + )) assert.Equal(t, "test", res.PGOInstaller) assert.Equal(t, "test-origin", res.PGOInstallerOrigin) assert.Equal(t, "developer", res.BuildSource) From 8dfe6ad4a798a677d352842923b1eb843ea9a303 Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Wed, 6 Nov 2024 10:20:50 -0600 Subject: [PATCH 3/3] PR feedback, v2 --- internal/feature/features_test.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/internal/feature/features_test.go b/internal/feature/features_test.go index 6f7eb78ab..63a76e509 100644 --- a/internal/feature/features_test.go +++ b/internal/feature/features_test.go @@ -15,7 +15,6 @@ import ( func TestDefaults(t *testing.T) { t.Parallel() gate := NewGate() - ctx := NewContext(context.Background(), gate) assert.Assert(t, false == gate.Enabled(AppendCustomQueries)) assert.Assert(t, true == gate.Enabled(AutoCreateUserSchema)) @@ -25,18 +24,14 @@ func TestDefaults(t *testing.T) { assert.Assert(t, false == gate.Enabled(PGBouncerSidecars)) assert.Assert(t, false == gate.Enabled(TablespaceVolumes)) assert.Assert(t, false == gate.Enabled(VolumeSnapshots)) - - assert.Equal(t, ShowAssigned(ctx), "") } func TestStringFormat(t *testing.T) { t.Parallel() gate := NewGate() - ctx := NewContext(context.Background(), gate) assert.NilError(t, gate.Set("")) assert.NilError(t, gate.Set("TablespaceVolumes=true")) - assert.Equal(t, ShowAssigned(ctx), "TablespaceVolumes=true") assert.Assert(t, true == gate.Enabled(TablespaceVolumes)) err := gate.Set("NotAGate=true")