diff options
36 files changed, 452 insertions, 235 deletions
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 340777a2a..151cc3cff 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -245,6 +245,8 @@ test:nightly: TEST_TARGET: [ test, test-with-proxies, test-with-praefect ] rules: - if: '$CI_PIPELINE_SOURCE == "schedule"' + - when: manual + allow_failure: true test:praefect_smoke: <<: *test_definition diff --git a/cmd/gitaly-git2go-v15/featureflags.go b/cmd/gitaly-git2go-v15/featureflags.go index b5b4126a1..669fe0445 100644 --- a/cmd/gitaly-git2go-v15/featureflags.go +++ b/cmd/gitaly-git2go-v15/featureflags.go @@ -27,9 +27,16 @@ func (featureFlagsSubcommand) Flags() *flag.FlagSet { } func (featureFlagsSubcommand) Run(ctx context.Context, decoder *gob.Decoder, encoder *gob.Encoder) error { - rawFlags := featureflag.RawFromContext(ctx) + var flags []git2go.FeatureFlag + for flag, value := range featureflag.FromContext(ctx) { + flags = append(flags, git2go.FeatureFlag{ + Name: flag.Name, + MetadataKey: flag.MetadataKey(), + Value: value, + }) + } return encoder.Encode(git2go.FeatureFlags{ - Raw: rawFlags, + Flags: flags, }) } diff --git a/cmd/gitaly-git2go-v15/main.go b/cmd/gitaly-git2go-v15/main.go index 060cdb033..2ce2796ae 100644 --- a/cmd/gitaly-git2go-v15/main.go +++ b/cmd/gitaly-git2go-v15/main.go @@ -9,7 +9,6 @@ import ( "flag" "fmt" "os" - "strconv" "strings" "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" @@ -17,7 +16,6 @@ import ( "github.com/sirupsen/logrus" "gitlab.com/gitlab-org/gitaly/v15/internal/git2go" glog "gitlab.com/gitlab-org/gitaly/v15/internal/log" - "gitlab.com/gitlab-org/gitaly/v15/internal/metadata" "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" "gitlab.com/gitlab-org/labkit/correlation" ) @@ -133,12 +131,8 @@ func main() { subcmdLogger.Infof("starting %s command", subcmdFlags.Name()) ctx = ctxlogrus.ToContext(ctx, subcmdLogger) - - featureFlags := make(featureflag.Raw) - enabledFeatureFlags.SetRaw(featureFlags, true) - disabledFeatureFlags.SetRaw(featureFlags, false) - - ctx = metadata.OutgoingToIncoming(featureflag.OutgoingWithRaw(ctx, featureFlags)) + ctx = enabledFeatureFlags.ToContext(ctx, true) + ctx = disabledFeatureFlags.ToContext(ctx, false) if err := subcmd.Run(ctx, decoder, encoder); err != nil { subcmdLogger.WithError(err).Errorf("%s command failed", subcmdFlags.Name()) @@ -148,10 +142,14 @@ func main() { subcmdLogger.Infof("%s command finished", subcmdFlags.Name()) } -type featureFlagArg []string +type featureFlagArg []featureflag.FeatureFlag func (v *featureFlagArg) String() string { - return strings.Join(*v, ",") + metadataKeys := make([]string, 0, len(*v)) + for _, flag := range *v { + metadataKeys = append(metadataKeys, flag.MetadataKey()) + } + return strings.Join(metadataKeys, ",") } func (v *featureFlagArg) Set(s string) error { @@ -159,15 +157,22 @@ func (v *featureFlagArg) Set(s string) error { return nil } - for _, enabledFF := range strings.Split(s, ",") { - *v = append(*v, enabledFF) + for _, metadataKey := range strings.Split(s, ",") { + flag, err := featureflag.FromMetadataKey(metadataKey) + if err != nil { + return err + } + + *v = append(*v, flag) } return nil } -func (v featureFlagArg) SetRaw(raw featureflag.Raw, enabled bool) { - for _, ff := range v { - raw[ff] = strconv.FormatBool(enabled) +func (v featureFlagArg) ToContext(ctx context.Context, enabled bool) context.Context { + for _, flag := range v { + ctx = featureflag.IncomingCtxWithFeatureFlag(ctx, flag, enabled) } + + return ctx } diff --git a/cmd/gitaly-hooks/hooks.go b/cmd/gitaly-hooks/hooks.go index 35248e143..95bca501e 100644 --- a/cmd/gitaly-hooks/hooks.go +++ b/cmd/gitaly-hooks/hooks.go @@ -161,7 +161,10 @@ func executeHook(cmd hookCommand, args []string) error { hookClient := gitalypb.NewHookServiceClient(conn) - ctx = featureflag.OutgoingWithRaw(ctx, payload.FeatureFlags) + for _, flag := range payload.FeatureFlagsWithValue { + ctx = featureflag.OutgoingCtxWithFeatureFlag(ctx, flag.Flag, flag.Enabled) + } + if err := cmd.exec(ctx, payload, hookClient, args); err != nil { return err } diff --git a/cmd/gitaly-hooks/hooks_test.go b/cmd/gitaly-hooks/hooks_test.go index 9db2ea23f..7e3f6d8ed 100644 --- a/cmd/gitaly-hooks/hooks_test.go +++ b/cmd/gitaly-hooks/hooks_test.go @@ -50,10 +50,10 @@ var ( disabledFeatureFlag = featureflag.FeatureFlag{Name: "disabled-feature-flag", OnByDefault: true} ) -func rawFeatureFlags(ctx context.Context) featureflag.Raw { +func featureFlags(ctx context.Context) map[featureflag.FeatureFlag]bool { ctx = featureflag.IncomingCtxWithFeatureFlag(ctx, enabledFeatureFlag, true) ctx = featureflag.IncomingCtxWithFeatureFlag(ctx, disabledFeatureFlag, false) - return featureflag.RawFromContext(ctx) + return featureflag.FromContext(ctx) } // envForHooks generates a set of environment variables for gitaly hooks @@ -62,7 +62,7 @@ func envForHooks(t testing.TB, ctx context.Context, cfg config.Cfg, repo *gitaly UserID: glHookValues.GLID, Username: glHookValues.GLUsername, Protocol: glHookValues.GLProtocol, - }, git.AllHooks, rawFeatureFlags(ctx)).Env() + }, git.AllHooks, featureFlags(ctx)).Env() require.NoError(t, err) env := append(command.AllowedEnvironment(os.Environ()), []string{ @@ -413,7 +413,7 @@ func TestHooksPostReceiveFailed(t *testing.T) { Protocol: glProtocol, }, git.PostReceiveHook, - rawFeatureFlags(ctx), + featureFlags(ctx), ).Env() require.NoError(t, err) diff --git a/internal/cache/keyer.go b/internal/cache/keyer.go index 72e696adb..b89b240db 100644 --- a/internal/cache/keyer.go +++ b/internal/cache/keyer.go @@ -274,15 +274,18 @@ func compositeKeyHashHex(ctx context.Context, genID string, req proto.Message) ( h := sha256.New() - ffs := featureflag.AllFlags(ctx) - sort.Strings(ffs) + var flagsWithValue []string + for flag, enabled := range featureflag.FromContext(ctx) { + flagsWithValue = append(flagsWithValue, flag.FormatWithValue(enabled)) + } + sort.Strings(flagsWithValue) for _, i := range []string{ version.GetVersion(), method, genID, string(reqSum), - strings.Join(ffs, " "), + strings.Join(flagsWithValue, " "), } { _, err := h.Write(prefixLen(i)) if err != nil { diff --git a/internal/git/command_options.go b/internal/git/command_options.go index 044ae754d..89e24eabc 100644 --- a/internal/git/command_options.go +++ b/internal/git/command_options.go @@ -285,14 +285,17 @@ func withInternalFetch(req repoScopedRequest, withSidechannel bool) func(ctx con return helper.ErrInvalidArgumentf("empty Gitaly address") } - featureFlagPairs := featureflag.AllFlags(ctx) + var flagsWithValue []string + for flag, value := range featureflag.FromContext(ctx) { + flagsWithValue = append(flagsWithValue, flag.FormatWithValue(value)) + } c.env = append(c.env, fmt.Sprintf("GITALY_PAYLOAD=%s", payload), fmt.Sprintf("GIT_SSH_COMMAND=%s %s", filepath.Join(cfg.BinDir, "gitaly-ssh"), "upload-pack"), fmt.Sprintf("GITALY_ADDRESS=%s", storageInfo.Address), fmt.Sprintf("GITALY_TOKEN=%s", storageInfo.Token), - fmt.Sprintf("GITALY_FEATUREFLAGS=%s", strings.Join(featureFlagPairs, ",")), + fmt.Sprintf("GITALY_FEATUREFLAGS=%s", strings.Join(flagsWithValue, ",")), fmt.Sprintf("CORRELATION_ID=%s", correlation.ExtractFromContextOrGenerate(ctx)), // please see https://github.com/git/git/commit/0da0e49ba12225684b75e86a4c9344ad121652cb for mote details "GIT_SSH_VARIANT=simple", diff --git a/internal/git/hooks_options.go b/internal/git/hooks_options.go index 5a24f8a9d..9a77a2ce8 100644 --- a/internal/git/hooks_options.go +++ b/internal/git/hooks_options.go @@ -115,7 +115,7 @@ func (cc *cmdCfg) configureHooks( transaction, userDetails, requestedHooks, - featureflag.RawFromContext(ctx)).Env() + featureflag.FromContext(ctx)).Env() if err != nil { return err } diff --git a/internal/git/hooks_payload.go b/internal/git/hooks_payload.go index f0185d495..918164f79 100644 --- a/internal/git/hooks_payload.go +++ b/internal/git/hooks_payload.go @@ -47,14 +47,23 @@ const ( ReceivePackHooks = ReferenceTransactionHook | UpdateHook | PreReceiveHook | PostReceiveHook ) +// FeatureFlagWithValue is used as part of the HooksPayload to pass on feature flags with their +// values to gitaly-hooks. +type FeatureFlagWithValue struct { + // Flag is the feature flag. + Flag featureflag.FeatureFlag `json:"flag"` + // Enabled indicates whether the flag is enabled or not. + Enabled bool `json:"enabled"` +} + // HooksPayload holds parameters required for all hooks. type HooksPayload struct { // RequestedHooks is a bitfield of requested Hooks. Hooks which // were not requested will not get executed. RequestedHooks Hook `json:"requested_hooks"` - // FeatureFlags contains feature flags with their values. They are set - // into the outgoing context when calling HookService. - FeatureFlags featureflag.Raw `json:"feature_flags,omitempty"` + // FeatureFlagsWithValue contains feature flags with their values. They are set into the + // outgoing context when calling HookService. + FeatureFlagsWithValue []FeatureFlagWithValue `json:"feature_flags_with_value,omitempty"` // Repo is the repository in which the hook is running. Repo *gitalypb.Repository `json:"-"` @@ -110,17 +119,25 @@ func NewHooksPayload( tx *txinfo.Transaction, userDetails *UserDetails, requestedHooks Hook, - featureFlags featureflag.Raw, + featureFlagsWithValue map[featureflag.FeatureFlag]bool, ) HooksPayload { + flags := make([]FeatureFlagWithValue, 0, len(featureFlagsWithValue)) + for flag, enabled := range featureFlagsWithValue { + flags = append(flags, FeatureFlagWithValue{ + Flag: flag, + Enabled: enabled, + }) + } + return HooksPayload{ - Repo: repo, - RuntimeDir: cfg.RuntimeDir, - InternalSocket: cfg.InternalSocketPath(), - InternalSocketToken: cfg.Auth.Token, - Transaction: tx, - UserDetails: userDetails, - RequestedHooks: requestedHooks, - FeatureFlags: featureFlags, + Repo: repo, + RuntimeDir: cfg.RuntimeDir, + InternalSocket: cfg.InternalSocketPath(), + InternalSocketToken: cfg.Auth.Token, + Transaction: tx, + UserDetails: userDetails, + RequestedHooks: requestedHooks, + FeatureFlagsWithValue: flags, } } diff --git a/internal/git/hooks_payload_test.go b/internal/git/hooks_payload_test.go index b6220d656..361e68a7f 100644 --- a/internal/git/hooks_payload_test.go +++ b/internal/git/hooks_payload_test.go @@ -27,7 +27,9 @@ func TestHooksPayload(t *testing.T) { }) t.Run("roundtrip succeeds", func(t *testing.T) { - env, err := git.NewHooksPayload(cfg, repo, nil, nil, git.PreReceiveHook, featureflag.Raw{"flag-key": "flag-value"}).Env() + env, err := git.NewHooksPayload(cfg, repo, nil, nil, git.PreReceiveHook, map[featureflag.FeatureFlag]bool{ + {Name: "flag_key"}: true, + }).Env() require.NoError(t, err) payload, err := git.HooksPayloadFromEnv([]string{ @@ -43,7 +45,12 @@ func TestHooksPayload(t *testing.T) { RuntimeDir: cfg.RuntimeDir, InternalSocket: cfg.InternalSocketPath(), RequestedHooks: git.PreReceiveHook, - FeatureFlags: featureflag.Raw{"flag-key": "flag-value"}, + FeatureFlagsWithValue: []git.FeatureFlagWithValue{ + { + Flag: featureflag.FeatureFlag{Name: "flag_key"}, + Enabled: true, + }, + }, }, payload) }) diff --git a/internal/git/updateref/update_with_hooks.go b/internal/git/updateref/update_with_hooks.go index de4c7cb92..fbd615be4 100644 --- a/internal/git/updateref/update_with_hooks.go +++ b/internal/git/updateref/update_with_hooks.go @@ -188,7 +188,7 @@ func (u *UpdaterWithHooks) UpdateReference( quarantinedRepo = quarantineDir.QuarantinedRepo() } - hooksPayload, err := git.NewHooksPayload(u.cfg, quarantinedRepo, transaction, &receiveHooksPayload, git.ReceivePackHooks, featureflag.RawFromContext(ctx)).Env() + hooksPayload, err := git.NewHooksPayload(u.cfg, quarantinedRepo, transaction, &receiveHooksPayload, git.ReceivePackHooks, featureflag.FromContext(ctx)).Env() if err != nil { return err } @@ -210,7 +210,7 @@ func (u *UpdaterWithHooks) UpdateReference( // We only need to update the hooks payload to the unquarantined repo in case we // had a quarantine environment. Otherwise, the initial hooks payload is for the // real repository anyway. - hooksPayload, err = git.NewHooksPayload(u.cfg, repo, transaction, &receiveHooksPayload, git.ReceivePackHooks, featureflag.RawFromContext(ctx)).Env() + hooksPayload, err = git.NewHooksPayload(u.cfg, repo, transaction, &receiveHooksPayload, git.ReceivePackHooks, featureflag.FromContext(ctx)).Env() if err != nil { return err } diff --git a/internal/git/updateref/update_with_hooks_test.go b/internal/git/updateref/update_with_hooks_test.go index fa02c5069..cf2464a58 100644 --- a/internal/git/updateref/update_with_hooks_test.go +++ b/internal/git/updateref/update_with_hooks_test.go @@ -110,15 +110,24 @@ func TestUpdaterWithHooks_UpdateReference(t *testing.T) { oldRev := "1e292f8fedd741b75372e19097c76d327140c312" - payload, err := git.NewHooksPayload(cfg, repo, nil, &git.UserDetails{ - UserID: "1234", - Username: "Username", - Protocol: "web", - }, git.ReceivePackHooks, featureflag.RawFromContext(ctx)).Env() - require.NoError(t, err) + requirePayload := func(t *testing.T, env []string) { + require.Len(t, env, 1) + + expectedPayload := git.NewHooksPayload(cfg, repo, nil, &git.UserDetails{ + UserID: "1234", + Username: "Username", + Protocol: "web", + }, git.ReceivePackHooks, featureflag.FromContext(ctx)) + + actualPayload, err := git.HooksPayloadFromEnv(env) + require.NoError(t, err) + + // Flags aren't sorted, so we just verify they contain the same elements. + require.ElementsMatch(t, expectedPayload.FeatureFlagsWithValue, actualPayload.FeatureFlagsWithValue) + expectedPayload.FeatureFlagsWithValue = nil + actualPayload.FeatureFlagsWithValue = nil - expectedEnv := []string{ - payload, + require.Equal(t, expectedPayload, actualPayload) } referenceTransactionCalls := 0 @@ -138,21 +147,21 @@ func TestUpdaterWithHooks_UpdateReference(t *testing.T) { require.NoError(t, err) require.Equal(t, fmt.Sprintf("%s %s refs/heads/master\n", oldRev, git.ZeroOID.String()), string(changes)) require.Empty(t, pushOptions) - require.Equal(t, env, expectedEnv) + requirePayload(t, env) return nil }, update: func(t *testing.T, ctx context.Context, repo *gitalypb.Repository, ref, oldValue, newValue string, env []string, stdout, stderr io.Writer) error { require.Equal(t, "refs/heads/master", ref) require.Equal(t, oldRev, oldValue) require.Equal(t, newValue, git.ZeroOID.String()) - require.Equal(t, env, expectedEnv) + requirePayload(t, env) return nil }, postReceive: func(t *testing.T, ctx context.Context, repo *gitalypb.Repository, pushOptions, env []string, stdin io.Reader, stdout, stderr io.Writer) error { changes, err := io.ReadAll(stdin) require.NoError(t, err) require.Equal(t, fmt.Sprintf("%s %s refs/heads/master\n", oldRev, git.ZeroOID.String()), string(changes)) - require.Equal(t, env, expectedEnv) + requirePayload(t, env) require.Empty(t, pushOptions) return nil }, @@ -169,7 +178,7 @@ func TestUpdaterWithHooks_UpdateReference(t *testing.T) { } referenceTransactionCalls++ - require.Equal(t, env, expectedEnv) + requirePayload(t, env) return nil }, expectedRefDeletion: true, diff --git a/internal/git2go/executor.go b/internal/git2go/executor.go index c1e220f43..5007b6c6f 100644 --- a/internal/git2go/executor.go +++ b/internal/git2go/executor.go @@ -59,14 +59,12 @@ func (b *Executor) run(ctx context.Context, repo repository.GitRepo, stdin io.Re var enabledFeatureFlags, disabledFeatureFlags []string - for ff, value := range featureflag.RawFromContext(ctx) { + for flag, value := range featureflag.FromContext(ctx) { switch value { - case "true": - enabledFeatureFlags = append(enabledFeatureFlags, ff) - case "false": - disabledFeatureFlags = append(disabledFeatureFlags, ff) - default: - return nil, fmt.Errorf("invalid value for feature flag %q: %q", ff, value) + case true: + enabledFeatureFlags = append(enabledFeatureFlags, flag.MetadataKey()) + case false: + disabledFeatureFlags = append(disabledFeatureFlags, flag.MetadataKey()) } } diff --git a/internal/git2go/featureflags.go b/internal/git2go/featureflags.go index cabc60e6f..bfde6867b 100644 --- a/internal/git2go/featureflags.go +++ b/internal/git2go/featureflags.go @@ -1,11 +1,21 @@ package git2go +// FeatureFlag is a feature flag state as seen by the `gitaly-git2go featureflag` test subcommand. +type FeatureFlag struct { + // MetadataKey is the metadata key of the feature flag. + MetadataKey string + // Name is the name of the feature flag. + Name string + // Value is the value of the feature flag. + Value bool +} + // FeatureFlags is a struct only used by tests to confirm that feature flags are // being properly propagated from the git2go Executor to the gitaly-git2go // binary type FeatureFlags struct { - // Raw is a map of feature flags and their corresponding values - Raw map[string]string + // Flags is the set of feature flags observed by the command. + Flags []FeatureFlag // Err is set if an error occurred. Err must exist on all gob serialized // results so that any error can be returned. Err error diff --git a/internal/git2go/featureflags_test.go b/internal/git2go/featureflags_test.go index 06741fb05..61ebd6d31 100644 --- a/internal/git2go/featureflags_test.go +++ b/internal/git2go/featureflags_test.go @@ -3,7 +3,6 @@ package git2go import ( "context" "encoding/gob" - "strconv" "testing" "github.com/stretchr/testify/require" @@ -16,7 +15,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg" ) -func (b *Executor) FeatureFlags(ctx context.Context, repo repository.GitRepo) (featureflag.Raw, error) { +func (b *Executor) FeatureFlags(ctx context.Context, repo repository.GitRepo) ([]FeatureFlag, error) { output, err := b.run(ctx, repo, nil, "feature-flags") if err != nil { return nil, err @@ -31,17 +30,16 @@ func (b *Executor) FeatureFlags(ctx context.Context, repo repository.GitRepo) (f return nil, result.Err } - return result.Raw, err + return result.Flags, err } var ( - featureA = featureflag.NewFeatureFlag("feature-a", "", "", false) - featureB = featureflag.NewFeatureFlag("feature-b", "", "", true) + featureA = featureflag.NewFeatureFlag("feature_a", "", "", false) + featureB = featureflag.NewFeatureFlag("feature_b", "", "", true) ) -func TestFeatureFlagsExecutor_FeatureFlags(t *testing.T) { - testhelper.NewFeatureSets(featureA, featureB). - Run(t, testExecutorFeatureFlags) +func TestExecutor_explicitFeatureFlags(t *testing.T) { + testhelper.NewFeatureSets(featureA, featureB).Run(t, testExecutorFeatureFlags) } func testExecutorFeatureFlags(t *testing.T, ctx context.Context) { @@ -54,9 +52,19 @@ func testExecutorFeatureFlags(t *testing.T, ctx context.Context) { executor := NewExecutor(cfg, gittest.NewCommandFactory(t, cfg), config.NewLocator(cfg)) - raw, err := executor.FeatureFlags(ctx, repo) + flags, err := executor.FeatureFlags(ctx, repo) require.NoError(t, err) - require.Equal(t, strconv.FormatBool(featureA.IsEnabled(ctx)), raw["gitaly-feature-feature-a"]) - require.Equal(t, strconv.FormatBool(featureB.IsEnabled(ctx)), raw["gitaly-feature-feature-b"]) + require.Subset(t, flags, []FeatureFlag{ + { + Name: "feature_a", + MetadataKey: "gitaly-feature-feature-a", + Value: featureA.IsEnabled(ctx), + }, + { + Name: "feature_b", + MetadataKey: "gitaly-feature-feature-b", + Value: featureB.IsEnabled(ctx), + }, + }, flags) } diff --git a/internal/gitaly/hook/postreceive_test.go b/internal/gitaly/hook/postreceive_test.go index d8afe3ff5..f78265d9f 100644 --- a/internal/gitaly/hook/postreceive_test.go +++ b/internal/gitaly/hook/postreceive_test.go @@ -83,7 +83,7 @@ func TestPostReceive_customHook(t *testing.T) { ctx := testhelper.Context(t) - payload, err := git.NewHooksPayload(cfg, repo, nil, receiveHooksPayload, git.PostReceiveHook, featureflag.RawFromContext(ctx)).Env() + payload, err := git.NewHooksPayload(cfg, repo, nil, receiveHooksPayload, git.PostReceiveHook, featureflag.FromContext(ctx)).Env() require.NoError(t, err) primaryPayload, err := git.NewHooksPayload( @@ -94,7 +94,7 @@ func TestPostReceive_customHook(t *testing.T) { }, receiveHooksPayload, git.PostReceiveHook, - featureflag.RawFromContext(ctx), + featureflag.FromContext(ctx), ).Env() require.NoError(t, err) @@ -106,7 +106,7 @@ func TestPostReceive_customHook(t *testing.T) { }, receiveHooksPayload, git.PostReceiveHook, - featureflag.RawFromContext(ctx), + featureflag.FromContext(ctx), ).Env() require.NoError(t, err) @@ -387,7 +387,7 @@ func TestPostReceive_quarantine(t *testing.T) { Protocol: "web", }, git.PreReceiveHook, - featureflag.RawFromContext(ctx), + featureflag.FromContext(ctx), ).Env() require.NoError(t, err) diff --git a/internal/gitaly/hook/prereceive_test.go b/internal/gitaly/hook/prereceive_test.go index 12dc504bd..7dfc05cee 100644 --- a/internal/gitaly/hook/prereceive_test.go +++ b/internal/gitaly/hook/prereceive_test.go @@ -42,7 +42,7 @@ func TestPrereceive_customHooks(t *testing.T) { ctx := testhelper.Context(t) - payload, err := git.NewHooksPayload(cfg, repo, nil, receiveHooksPayload, git.PreReceiveHook, featureflag.RawFromContext(ctx)).Env() + payload, err := git.NewHooksPayload(cfg, repo, nil, receiveHooksPayload, git.PreReceiveHook, featureflag.FromContext(ctx)).Env() require.NoError(t, err) primaryPayload, err := git.NewHooksPayload( @@ -53,7 +53,7 @@ func TestPrereceive_customHooks(t *testing.T) { }, receiveHooksPayload, git.PreReceiveHook, - featureflag.RawFromContext(ctx), + featureflag.FromContext(ctx), ).Env() require.NoError(t, err) @@ -65,7 +65,7 @@ func TestPrereceive_customHooks(t *testing.T) { }, receiveHooksPayload, git.PreReceiveHook, - featureflag.RawFromContext(ctx), + featureflag.FromContext(ctx), ).Env() require.NoError(t, err) @@ -206,7 +206,7 @@ func TestPrereceive_quarantine(t *testing.T) { Protocol: "web", }, git.PreReceiveHook, - featureflag.RawFromContext(ctx), + featureflag.FromContext(ctx), ).Env() require.NoError(t, err) diff --git a/internal/gitaly/hook/transactions_test.go b/internal/gitaly/hook/transactions_test.go index 5c485983f..727485fd9 100644 --- a/internal/gitaly/hook/transactions_test.go +++ b/internal/gitaly/hook/transactions_test.go @@ -45,7 +45,7 @@ func TestHookManager_stopCalled(t *testing.T) { Protocol: "web", }, git.ReferenceTransactionHook, - featureflag.RawFromContext(ctx), + featureflag.FromContext(ctx), ).Env() require.NoError(t, err) diff --git a/internal/gitaly/hook/update_test.go b/internal/gitaly/hook/update_test.go index 464340861..326f2dca1 100644 --- a/internal/gitaly/hook/update_test.go +++ b/internal/gitaly/hook/update_test.go @@ -39,7 +39,7 @@ func TestUpdate_customHooks(t *testing.T) { ctx := testhelper.Context(t) - payload, err := git.NewHooksPayload(cfg, repo, nil, receiveHooksPayload, git.UpdateHook, featureflag.RawFromContext(ctx)).Env() + payload, err := git.NewHooksPayload(cfg, repo, nil, receiveHooksPayload, git.UpdateHook, featureflag.FromContext(ctx)).Env() require.NoError(t, err) primaryPayload, err := git.NewHooksPayload( @@ -50,7 +50,7 @@ func TestUpdate_customHooks(t *testing.T) { }, receiveHooksPayload, git.UpdateHook, - featureflag.RawFromContext(ctx), + featureflag.FromContext(ctx), ).Env() require.NoError(t, err) @@ -62,7 +62,7 @@ func TestUpdate_customHooks(t *testing.T) { }, receiveHooksPayload, git.UpdateHook, - featureflag.RawFromContext(ctx), + featureflag.FromContext(ctx), ).Env() require.NoError(t, err) @@ -237,7 +237,7 @@ func TestUpdate_quarantine(t *testing.T) { Protocol: "web", }, git.PreReceiveHook, - featureflag.RawFromContext(ctx), + featureflag.FromContext(ctx), ).Env() require.NoError(t, err) diff --git a/internal/gitaly/service/hook/post_receive_test.go b/internal/gitaly/service/hook/post_receive_test.go index 39b0aeb27..96b3d221f 100644 --- a/internal/gitaly/service/hook/post_receive_test.go +++ b/internal/gitaly/service/hook/post_receive_test.go @@ -109,7 +109,7 @@ func TestHooksMissingStdin(t *testing.T) { Protocol: "protocol", }, git.PostReceiveHook, - featureflag.RawFromContext(ctx), + featureflag.FromContext(ctx), ).Env() require.NoError(t, err) @@ -237,7 +237,7 @@ To create a merge request for okay, visit: Protocol: "protocol", }, git.PostReceiveHook, - featureflag.RawFromContext(ctx), + featureflag.FromContext(ctx), ).Env() require.NoError(t, err) diff --git a/internal/gitaly/service/hook/pre_receive_test.go b/internal/gitaly/service/hook/pre_receive_test.go index 34175bb46..12fed62be 100644 --- a/internal/gitaly/service/hook/pre_receive_test.go +++ b/internal/gitaly/service/hook/pre_receive_test.go @@ -149,7 +149,7 @@ func TestPreReceiveHook_GitlabAPIAccess(t *testing.T) { Protocol: protocol, }, git.PreReceiveHook, - featureflag.RawFromContext(ctx), + featureflag.FromContext(ctx), ).Env() require.NoError(t, err) @@ -271,7 +271,7 @@ func TestPreReceive_APIErrors(t *testing.T) { Protocol: "web", }, git.PreReceiveHook, - featureflag.RawFromContext(ctx), + featureflag.FromContext(ctx), ).Env() require.NoError(t, err) @@ -345,7 +345,7 @@ exit %d Protocol: "web", }, git.PreReceiveHook, - featureflag.RawFromContext(ctx), + featureflag.FromContext(ctx), ).Env() require.NoError(t, err) @@ -478,7 +478,7 @@ func TestPreReceiveHook_Primary(t *testing.T) { Protocol: "web", }, git.PreReceiveHook, - featureflag.RawFromContext(ctx), + featureflag.FromContext(ctx), ).Env() require.NoError(t, err) diff --git a/internal/gitaly/service/hook/reference_transaction_test.go b/internal/gitaly/service/hook/reference_transaction_test.go index b466cee33..5a1c2c9eb 100644 --- a/internal/gitaly/service/hook/reference_transaction_test.go +++ b/internal/gitaly/service/hook/reference_transaction_test.go @@ -155,7 +155,7 @@ func TestReferenceTransactionHook(t *testing.T) { }, nil, git.ReferenceTransactionHook, - featureflag.RawFromContext(ctx), + featureflag.FromContext(ctx), ).Env() require.NoError(t, err) diff --git a/internal/gitaly/service/hook/update_test.go b/internal/gitaly/service/hook/update_test.go index 98e30678c..233745f6c 100644 --- a/internal/gitaly/service/hook/update_test.go +++ b/internal/gitaly/service/hook/update_test.go @@ -47,7 +47,7 @@ func TestUpdate_CustomHooks(t *testing.T) { Protocol: "web", }, git.UpdateHook, - featureflag.RawFromContext(ctx), + featureflag.FromContext(ctx), ).Env() require.NoError(t, err) diff --git a/internal/gitaly/service/smarthttp/inforefs_test.go b/internal/gitaly/service/smarthttp/inforefs_test.go index 3aa10036f..ca0e6e4cc 100644 --- a/internal/gitaly/service/smarthttp/inforefs_test.go +++ b/internal/gitaly/service/smarthttp/inforefs_test.go @@ -360,7 +360,7 @@ func TestCacheInfoRefsUploadPack(t *testing.T) { // Praefect explicitly injects all unset feature flags, the key is thus differend depending // on whether Praefect is in use or not. We thus manually inject all feature flags here such // that they're forced to the same state. - for _, ff := range featureflag.All { + for _, ff := range featureflag.DefinedFlags() { ctx = featureflag.OutgoingCtxWithFeatureFlag(ctx, ff, true) ctx = featureflag.IncomingCtxWithFeatureFlag(ctx, ff, true) } diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go index 981e658e5..7f9a46ff1 100644 --- a/internal/gitaly/service/smarthttp/receive_pack_test.go +++ b/internal/gitaly/service/smarthttp/receive_pack_test.go @@ -68,7 +68,7 @@ func TestPostReceivePack_successful(t *testing.T) { // the context's feature flags we see here and the context's metadata as it would // arrive on the proxied Gitaly. To fix this, we thus inject all feature flags // explicitly here. - for _, ff := range featureflag.All { + for _, ff := range featureflag.DefinedFlags() { ctx = featureflag.OutgoingCtxWithFeatureFlag(ctx, ff, true) ctx = featureflag.IncomingCtxWithFeatureFlag(ctx, ff, true) } @@ -105,6 +105,18 @@ func TestPostReceivePack_successful(t *testing.T) { // figuring out their actual contents. So let's just remove it, too. payload.Transaction = nil + var expectedFeatureFlags []git.FeatureFlagWithValue + for feature, enabled := range featureflag.FromContext(ctx) { + expectedFeatureFlags = append(expectedFeatureFlags, git.FeatureFlagWithValue{ + Flag: feature, Enabled: enabled, + }) + } + + // Compare here without paying attention to the order given that flags aren't sorted and + // unset the struct member afterwards. + require.ElementsMatch(t, expectedFeatureFlags, payload.FeatureFlagsWithValue) + payload.FeatureFlagsWithValue = nil + require.Equal(t, git.HooksPayload{ RuntimeDir: cfg.RuntimeDir, InternalSocket: cfg.InternalSocketPath(), @@ -115,7 +127,6 @@ func TestPostReceivePack_successful(t *testing.T) { Protocol: "http", }, RequestedHooks: git.ReceivePackHooks, - FeatureFlags: featureflag.RawFromContext(ctx), }, payload) } diff --git a/internal/gitaly/service/ssh/receive_pack_test.go b/internal/gitaly/service/ssh/receive_pack_test.go index 66fc1e596..a65c17ef5 100644 --- a/internal/gitaly/service/ssh/receive_pack_test.go +++ b/internal/gitaly/service/ssh/receive_pack_test.go @@ -120,7 +120,7 @@ func TestReceivePackPushSuccess(t *testing.T) { // when deserializing the HooksPayload. By setting all flags to `true` explicitly, we both // verify that gitaly-ssh picks up feature flags correctly and fix the test to behave the // same with and without Praefect. - for _, featureFlag := range featureflag.All { + for _, featureFlag := range featureflag.DefinedFlags() { ctx = featureflag.ContextWithFeatureFlag(ctx, featureFlag, true) } @@ -152,11 +152,18 @@ func TestReceivePackPushSuccess(t *testing.T) { // figuring out their actual contents. So let's just remove it, too. payload.Transaction = nil - expectedFeatureFlags := featureflag.Raw{} - for _, feature := range featureflag.All { - expectedFeatureFlags[feature.MetadataKey()] = "true" + var expectedFeatureFlags []git.FeatureFlagWithValue + for _, feature := range featureflag.DefinedFlags() { + expectedFeatureFlags = append(expectedFeatureFlags, git.FeatureFlagWithValue{ + Flag: feature, Enabled: true, + }) } + // Compare here without paying attention to the order given that flags aren't sorted and + // unset the struct member afterwards. + require.ElementsMatch(t, expectedFeatureFlags, payload.FeatureFlagsWithValue) + payload.FeatureFlagsWithValue = nil + require.Equal(t, git.HooksPayload{ RuntimeDir: cfg.RuntimeDir, InternalSocket: cfg.InternalSocketPath(), @@ -167,7 +174,6 @@ func TestReceivePackPushSuccess(t *testing.T) { Protocol: "ssh", }, RequestedHooks: git.ReceivePackHooks, - FeatureFlags: expectedFeatureFlags, }, payload) } @@ -668,11 +674,16 @@ func sshPushCommand(ctx context.Context, t *testing.T, cfg config.Cfg, cloneDeta }) require.NoError(t, err) + var flagsWithValues []string + for flag, value := range featureflag.FromContext(ctx) { + flagsWithValues = append(flagsWithValues, flag.FormatWithValue(value)) + } + cmd := gittest.NewCommand(t, cfg, "-C", cloneDetails.LocalRepoPath, "push", "-v", "git@localhost:test/test.git", "master") cmd.Env = []string{ fmt.Sprintf("GITALY_PAYLOAD=%s", payload), fmt.Sprintf("GITALY_ADDRESS=%s", serverSocketPath), - fmt.Sprintf("GITALY_FEATUREFLAGS=%s", strings.Join(featureflag.AllFlags(ctx), ",")), + fmt.Sprintf("GITALY_FEATUREFLAGS=%s", strings.Join(flagsWithValues, ",")), fmt.Sprintf("GIT_SSH_COMMAND=%s receive-pack", filepath.Join(cfg.BinDir, "gitaly-ssh")), } diff --git a/internal/gitaly/service/ssh/upload_pack_test.go b/internal/gitaly/service/ssh/upload_pack_test.go index 113cd816a..a9d010699 100644 --- a/internal/gitaly/service/ssh/upload_pack_test.go +++ b/internal/gitaly/service/ssh/upload_pack_test.go @@ -56,10 +56,15 @@ func runClone( payload, err := protojson.Marshal(request) require.NoError(t, err) + var flagsWithValues []string + for flag, value := range featureflag.FromContext(ctx) { + flagsWithValues = append(flagsWithValues, flag.FormatWithValue(value)) + } + env := []string{ fmt.Sprintf("GITALY_ADDRESS=%s", cfg.SocketPath), fmt.Sprintf("GITALY_PAYLOAD=%s", payload), - fmt.Sprintf("GITALY_FEATUREFLAGS=%s", strings.Join(featureflag.AllFlags(ctx), ",")), + fmt.Sprintf("GITALY_FEATUREFLAGS=%s", strings.Join(flagsWithValues, ",")), fmt.Sprintf(`GIT_SSH_COMMAND=%s upload-pack`, filepath.Join(cfg.BinDir, "gitaly-ssh")), } if withSidechannel { diff --git a/internal/metadata/featureflag/context.go b/internal/metadata/featureflag/context.go index 9483abf25..950e81c70 100644 --- a/internal/metadata/featureflag/context.go +++ b/internal/metadata/featureflag/context.go @@ -10,13 +10,19 @@ import ( ) const ( - // Delim is a delimiter used between a feature flag name and its value. - Delim = ":" - - // ffPrefix is the prefix used for Gitaly-scoped feature flags. - ffPrefix = "gitaly-feature-" + // explicitFeatureFlagKey is used by ContextWithExplicitFeatureFlags to mark a context as + // requiring all feature flags to have been explicitly defined. + explicitFeatureFlagKey = "require_explicit_feature_flag_checks" ) +// ContextWithExplicitFeatureFlags marks the context such that all feature flags which are checked +// must have been explicitly set in that context. If a feature flag wasn't set to an explicit value, +// then checking this feature flag will panic. This is not for use in production systems, but is +// intended for tests to verify that we test each feature flag properly. +func ContextWithExplicitFeatureFlags(ctx context.Context) context.Context { + return injectIntoIncomingAndOutgoingContext(ctx, explicitFeatureFlagKey, true) +} + // ContextWithFeatureFlag sets the feature flag in both the incoming and outgoing context. func ContextWithFeatureFlag(ctx context.Context, flag FeatureFlag, enabled bool) context.Context { return injectIntoIncomingAndOutgoingContext(ctx, flag.MetadataKey(), enabled) @@ -68,56 +74,44 @@ func incomingCtxWithFeatureFlag(ctx context.Context, key string, enabled bool) c return metadata.NewIncomingContext(ctx, md) } -// AllFlags returns all feature flags with their value that use the Gitaly metadata -// prefix. Note: results will not be sorted. -func AllFlags(ctx context.Context) []string { - featureFlagMap := RawFromContext(ctx) - featureFlagSlice := make([]string, 0, len(featureFlagMap)) - for key, value := range featureFlagMap { - featureFlagSlice = append(featureFlagSlice, - fmt.Sprintf("%s%s%s", strings.TrimPrefix(key, ffPrefix), Delim, value), - ) +func injectIntoIncomingAndOutgoingContext(ctx context.Context, key string, enabled bool) context.Context { + incomingMD, ok := metadata.FromIncomingContext(ctx) + if !ok { + incomingMD = metadata.New(map[string]string{}) } - return featureFlagSlice + incomingMD.Set(key, strconv.FormatBool(enabled)) + + ctx = metadata.NewIncomingContext(ctx, incomingMD) + + return metadata.AppendToOutgoingContext(ctx, key, strconv.FormatBool(enabled)) } -// Raw contains feature flags and their values in their raw form with header prefix in place -// and values unparsed. -type Raw map[string]string - -// RawFromContext returns a map that contains all feature flags with their values. The feature -// flags are in their raw format with the header prefix in place. If multiple values are set a -// flag, the first occurrence is used. -// -// This is mostly intended for propagating the feature flags by other means than the metadata, -// for example into the hooks through the environment. -func RawFromContext(ctx context.Context) Raw { +// FromContext returns the set of all feature flags defined in the context. This returns both +// feature flags that are currently defined by Gitaly, but may also return some that aren't defined +// by us in case they match the feature flag prefix but don't have a definition. This function also +// returns the state of the feature flag *as defined in the context*. This value may be overridden. +func FromContext(ctx context.Context) map[FeatureFlag]bool { md, ok := metadata.FromIncomingContext(ctx) if !ok { - return nil + return map[FeatureFlag]bool{} } - featureFlags := map[string]string{} - for key, values := range md { - if !strings.HasPrefix(key, ffPrefix) || len(values) == 0 { + flags := map[FeatureFlag]bool{} + for metadataName, values := range md { + if len(values) == 0 { continue } - featureFlags[key] = values[0] - } - - return featureFlags -} + flag, err := FromMetadataKey(metadataName) + if err != nil { + continue + } -// OutgoingWithRaw returns a new context with raw flags appended into the outgoing -// metadata. -func OutgoingWithRaw(ctx context.Context, flags Raw) context.Context { - for key, value := range flags { - ctx = metadata.AppendToOutgoingContext(ctx, key, value) + flags[flag] = values[0] == "true" } - return ctx + return flags } func rubyHeaderKey(flag string) string { diff --git a/internal/metadata/featureflag/context_test.go b/internal/metadata/featureflag/context_test.go index 35e85a9c8..8e4e222da 100644 --- a/internal/metadata/featureflag/context_test.go +++ b/internal/metadata/featureflag/context_test.go @@ -119,41 +119,61 @@ func TestGRPCMetadataFeatureFlag(t *testing.T) { } } -func TestAllEnabledFlags(t *testing.T) { - flags := map[string]string{ - ffPrefix + "meow": "true", - ffPrefix + "foo": "true", - ffPrefix + "woof": "false", // not enabled - ffPrefix + "bar": "TRUE", // not enabled +func TestFromContext(t *testing.T) { + defer func(old map[string]FeatureFlag) { + flagsByName = old + }(flagsByName) + + defaultDisabledFlag := FeatureFlag{ + Name: "default_disabled", + OnByDefault: false, } - ctx := metadata.NewIncomingContext(createContext(), metadata.New(flags)) - require.ElementsMatch(t, AllFlags(ctx), []string{"meow:true", "foo:true", "woof:false", "bar:TRUE"}) -} - -func TestRaw(t *testing.T) { - enabledFlag := FeatureFlag{Name: "enabled-flag"} - disabledFlag := FeatureFlag{Name: "disabled-flag"} + defaultEnabledFlag := FeatureFlag{ + Name: "default_enabled", + OnByDefault: true, + } - raw := Raw{ - ffPrefix + enabledFlag.Name: "true", - ffPrefix + disabledFlag.Name: "false", + flagsByName = map[string]FeatureFlag{ + "default_disabled": defaultDisabledFlag, + "default_enabled": defaultEnabledFlag, } - t.Run("RawFromContext", func(t *testing.T) { - ctx := createContext() - ctx = IncomingCtxWithFeatureFlag(ctx, enabledFlag, true) - ctx = IncomingCtxWithFeatureFlag(ctx, disabledFlag, false) + t.Run("with no defined flags", func(t *testing.T) { + require.Empty(t, FromContext(createContext())) + }) + + t.Run("with single defined flag", func(t *testing.T) { + ctx := ContextWithFeatureFlag(createContext(), defaultDisabledFlag, false) + ctx = ContextWithFeatureFlag(ctx, defaultEnabledFlag, true) - require.Equal(t, raw, RawFromContext(ctx)) + require.Equal(t, map[FeatureFlag]bool{ + defaultDisabledFlag: false, + defaultEnabledFlag: true, + }, FromContext(ctx)) }) - t.Run("OutgoingWithRaw", func(t *testing.T) { - outgoingMD, ok := metadata.FromOutgoingContext(OutgoingWithRaw(createContext(), raw)) - require.True(t, ok) - require.Equal(t, metadata.MD{ - ffPrefix + enabledFlag.Name: {"true"}, - ffPrefix + disabledFlag.Name: {"false"}, - }, outgoingMD) + t.Run("with defined flags and non-default values", func(t *testing.T) { + ctx := ContextWithFeatureFlag(createContext(), defaultDisabledFlag, true) + ctx = ContextWithFeatureFlag(ctx, defaultEnabledFlag, false) + + require.Equal(t, map[FeatureFlag]bool{ + defaultDisabledFlag: true, + defaultEnabledFlag: false, + }, FromContext(ctx)) + }) + + t.Run("with defined and undefined flag", func(t *testing.T) { + undefinedFlag := FeatureFlag{ + Name: "undefined_flag", + } + + ctx := ContextWithFeatureFlag(createContext(), defaultDisabledFlag, false) + ctx = ContextWithFeatureFlag(ctx, undefinedFlag, false) + + require.Equal(t, map[FeatureFlag]bool{ + defaultDisabledFlag: false, + undefinedFlag: false, + }, FromContext(ctx)) }) } diff --git a/internal/metadata/featureflag/featureflag.go b/internal/metadata/featureflag/featureflag.go index 70f90d2d9..e2c91568f 100644 --- a/internal/metadata/featureflag/featureflag.go +++ b/internal/metadata/featureflag/featureflag.go @@ -32,31 +32,22 @@ var ( []string{"flag", "enabled"}, ) - // All is the list of all registered feature flags. - All = []FeatureFlag{} + // flagsByName is the set of defined feature flags mapped by their respective name. + flagsByName = map[string]FeatureFlag{} ) -const explicitFeatureFlagKey = "require_explicit_feature_flag_checks" +const ( + // ffPrefix is the prefix used for Gitaly-scoped feature flags. + ffPrefix = "gitaly-feature-" +) -func injectIntoIncomingAndOutgoingContext(ctx context.Context, key string, enabled bool) context.Context { - incomingMD, ok := metadata.FromIncomingContext(ctx) - if !ok { - incomingMD = metadata.New(map[string]string{}) +// DefinedFlags returns the set of feature flags that have been explicitly defined. +func DefinedFlags() []FeatureFlag { + flags := make([]FeatureFlag, 0, len(flagsByName)) + for _, flag := range flagsByName { + flags = append(flags, flag) } - - incomingMD.Set(key, strconv.FormatBool(enabled)) - - ctx = metadata.NewIncomingContext(ctx, incomingMD) - - return metadata.AppendToOutgoingContext(ctx, key, strconv.FormatBool(enabled)) -} - -// ContextWithExplicitFeatureFlags marks the context such that all feature flags which are checked -// must have been explicitly set in that context. If a feature flag wasn't set to an explicit value, -// then checking this feature flag will panic. This is not for use in production systems, but is -// intended for tests to verify that we test each feature flag properly. -func ContextWithExplicitFeatureFlags(ctx context.Context) context.Context { - return injectIntoIncomingAndOutgoingContext(ctx, explicitFeatureFlagKey, true) + return flags } // FeatureFlag gates the implementation of new or changed functionality. @@ -73,14 +64,56 @@ type FeatureFlag struct { // input that are not used for anything but only for the sake of linking to the feature flag rollout // issue in the Gitaly project. func NewFeatureFlag(name, version, rolloutIssueURL string, onByDefault bool) FeatureFlag { + if strings.ContainsAny(name, "-:") { + // It is critical that feature flags don't contain either a dash nor a colon: + // + // - We convert dashes to underscores when converting a feature flag's name to the + // metadata key, and vice versa. The name wouldn't round-trip in case it had + // underscores and must thus use dashes instead. + // - We use colons to separate feature flags and their values. + panic("Feature flags must not contain dashes or colons.") + } + featureFlag := FeatureFlag{ Name: name, OnByDefault: onByDefault, } - All = append(All, featureFlag) + + flagsByName[name] = featureFlag + return featureFlag } +// FromMetadataKey parses the given gRPC metadata key into a Gitaly feature flag and performs the +// necessary conversions. Returns an error in case the metadata does not refer to a feature flag. +// +// This function tries to look up the default value via our set of flag definitions. In case the +// flag definition is unknown to Gitaly it assumes a default value of `false`. +func FromMetadataKey(metadataKey string) (FeatureFlag, error) { + if !strings.HasPrefix(metadataKey, ffPrefix) { + return FeatureFlag{}, fmt.Errorf("not a feature flag: %q", metadataKey) + } + + flagName := strings.TrimPrefix(metadataKey, ffPrefix) + flagName = strings.ReplaceAll(flagName, "-", "_") + + flag, ok := flagsByName[flagName] + if !ok { + flag = FeatureFlag{ + Name: flagName, + OnByDefault: false, + } + } + + return flag, nil +} + +// FormatWithValue converts the feature flag into a string with the given state. Note that this +// function uses the feature flag name and not the raw metadata key as used in gRPC metadata. +func (ff FeatureFlag) FormatWithValue(enabled bool) string { + return fmt.Sprintf("%s:%v", ff.Name, enabled) +} + // IsEnabled checks if the feature flag is enabled for the passed context. // Only returns true if the metadata for the feature flag is set to "true" func (ff FeatureFlag) IsEnabled(ctx context.Context) bool { diff --git a/internal/metadata/featureflag/featureflag_test.go b/internal/metadata/featureflag/featureflag_test.go index 8944e21fd..7a5d5915b 100644 --- a/internal/metadata/featureflag/featureflag_test.go +++ b/internal/metadata/featureflag/featureflag_test.go @@ -1,12 +1,74 @@ package featureflag import ( + "fmt" "testing" "github.com/stretchr/testify/require" "google.golang.org/grpc/metadata" ) +func TestFeatureFlag_FromMetadataKey(t *testing.T) { + defer func(old map[string]FeatureFlag) { + flagsByName = old + }(flagsByName) + + defaultEnabled := FeatureFlag{ + Name: "default_enabled", + OnByDefault: true, + } + defaultDisabled := FeatureFlag{ + Name: "default_disabled", + OnByDefault: false, + } + + flagsByName = map[string]FeatureFlag{ + defaultEnabled.Name: defaultEnabled, + defaultDisabled.Name: defaultDisabled, + } + + for _, tc := range []struct { + desc string + metadataKey string + expectedErr error + expectedFlag FeatureFlag + }{ + { + desc: "empty key", + metadataKey: "", + expectedErr: fmt.Errorf("not a feature flag: \"\""), + }, + { + desc: "invalid prefix", + metadataKey: "invalid-prefix", + expectedErr: fmt.Errorf("not a feature flag: \"invalid-prefix\""), + }, + { + desc: "default enabled flag", + metadataKey: defaultEnabled.MetadataKey(), + expectedFlag: defaultEnabled, + }, + { + desc: "default disabled flag", + metadataKey: defaultDisabled.MetadataKey(), + expectedFlag: defaultDisabled, + }, + { + desc: "undefined flag", + metadataKey: "gitaly-feature-not-defined", + expectedFlag: FeatureFlag{ + Name: "not_defined", + }, + }, + } { + t.Run(tc.desc, func(t *testing.T) { + flag, err := FromMetadataKey(tc.metadataKey) + require.Equal(t, tc.expectedErr, err) + require.Equal(t, tc.expectedFlag, flag) + }) + } +} + func TestFeatureFlag_enabled(t *testing.T) { for _, tc := range []struct { desc string diff --git a/internal/middleware/featureflag/featureflag_handler.go b/internal/middleware/featureflag/featureflag_handler.go index 33c3c046d..36e64edc9 100644 --- a/internal/middleware/featureflag/featureflag_handler.go +++ b/internal/middleware/featureflag/featureflag_handler.go @@ -26,11 +26,13 @@ func StreamInterceptor(srv interface{}, stream grpc.ServerStream, _ *grpc.Stream // track adds the list of the feature flags into the logging context. // The list is sorted by feature flag name to produce consistent output. func track(ctx context.Context) { - flags := featureflag.AllFlags(ctx) - if len(flags) != 0 { - sort.Slice(flags, func(i, j int) bool { - return flags[i][:strings.Index(flags[i], featureflag.Delim)] < flags[j][:strings.Index(flags[j], featureflag.Delim)] - }) - ctxlogrus.AddFields(ctx, logrus.Fields{"feature_flags": strings.Join(flags, " ")}) + var flagsWithValue []string + for flag, value := range featureflag.FromContext(ctx) { + flagsWithValue = append(flagsWithValue, flag.FormatWithValue(value)) + } + sort.Strings(flagsWithValue) + + if len(flagsWithValue) != 0 { + ctxlogrus.AddFields(ctx, logrus.Fields{"feature_flags": strings.Join(flagsWithValue, " ")}) } } diff --git a/internal/praefect/coordinator.go b/internal/praefect/coordinator.go index 38e61c0dc..6f6d439b6 100644 --- a/internal/praefect/coordinator.go +++ b/internal/praefect/coordinator.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "strconv" "sync" "time" @@ -589,8 +588,6 @@ func (c *Coordinator) maintenanceStreamParameters(ctx context.Context, call grpc // streamParametersContexts converts the contexts with incoming metadata into a context that is // usable by peer Gitaly nodes. func streamParametersContext(ctx context.Context) context.Context { - outgoingCtx := metadata.IncomingToOutgoing(ctx) - // When upgrading Gitaly nodes where the upgrade contains feature flag default changes, then // there will be a window where a subset of Gitaly nodes has a different understanding of // the current default value. If the feature flag wasn't set explicitly on upgrade by @@ -614,17 +611,20 @@ func streamParametersContext(ctx context.Context) context.Context { // Praefect. But given that feature flags should be introduced with a default value of // `false` to account for zero-dodwntime upgrades, the view would also be consistent in that // case. - rawFeatureFlags := featureflag.RawFromContext(ctx) - if rawFeatureFlags == nil { - rawFeatureFlags = map[string]string{} + + explicitlySetFlags := map[string]bool{} + for flag := range featureflag.FromContext(ctx) { + explicitlySetFlags[flag.Name] = true } - for _, ff := range featureflag.All { - if _, ok := rawFeatureFlags[ff.MetadataKey()]; !ok { - rawFeatureFlags[ff.MetadataKey()] = strconv.FormatBool(ff.OnByDefault) + outgoingCtx := metadata.IncomingToOutgoing(ctx) + for _, flag := range featureflag.DefinedFlags() { + if !explicitlySetFlags[flag.Name] { + outgoingCtx = featureflag.OutgoingCtxWithFeatureFlag( + outgoingCtx, flag, flag.OnByDefault, + ) } } - outgoingCtx = featureflag.OutgoingWithRaw(outgoingCtx, rawFeatureFlags) return outgoingCtx } diff --git a/internal/praefect/coordinator_test.go b/internal/praefect/coordinator_test.go index 0b8201f33..38e26a9f0 100644 --- a/internal/praefect/coordinator_test.go +++ b/internal/praefect/coordinator_test.go @@ -2689,8 +2689,8 @@ func TestNewRequestFinalizer_contextIsDisjointedFromTheRPC(t *testing.T) { func TestStreamParametersContext(t *testing.T) { // Because we're using NewFeatureFlag, they'll end up in the All array. - enabledFF := featureflag.NewFeatureFlag("default-enabled", "", "", true) - disabledFF := featureflag.NewFeatureFlag("default-disabled", "", "", false) + enabledFF := featureflag.NewFeatureFlag("default_enabled", "", "", true) + disabledFF := featureflag.NewFeatureFlag("default_disabled", "", "", false) type expectedFlag struct { flag featureflag.FeatureFlag @@ -2699,7 +2699,7 @@ func TestStreamParametersContext(t *testing.T) { expectedFlags := func(overrides ...expectedFlag) []expectedFlag { flagValues := map[featureflag.FeatureFlag]bool{} - for _, flag := range featureflag.All { + for _, flag := range featureflag.DefinedFlags() { flagValues[flag] = flag.OnByDefault } for _, override := range overrides { @@ -2803,10 +2803,6 @@ func TestStreamParametersContext(t *testing.T) { ), expectedOutgoingMD: metadata.Join( metadataForFlags(expectedFlags()), - metadata.Pairs( - enabledFF.MetadataKey(), "true", - disabledFF.MetadataKey(), "false", - ), ), expectedFlags: expectedFlags(), }, @@ -2827,10 +2823,6 @@ func TestStreamParametersContext(t *testing.T) { expectedFlag{flag: enabledFF, enabled: false}, expectedFlag{flag: disabledFF, enabled: true}, )), - metadata.Pairs( - enabledFF.MetadataKey(), "false", - disabledFF.MetadataKey(), "true", - ), ), expectedFlags: expectedFlags( expectedFlag{flag: enabledFF, enabled: false}, @@ -2856,7 +2848,6 @@ func TestStreamParametersContext(t *testing.T) { expectedFlag{flag: disabledFF, enabled: true}, )), metadata.Pairs( - disabledFF.MetadataKey(), "true", "incoming", "value", ), ), diff --git a/internal/testhelper/featureset_test.go b/internal/testhelper/featureset_test.go index ddbb8652c..f3311b7d2 100644 --- a/internal/testhelper/featureset_test.go +++ b/internal/testhelper/featureset_test.go @@ -171,15 +171,12 @@ func TestNewFeatureSetsWithRubyFlags(t *testing.T) { } func TestFeatureSets_Run(t *testing.T) { - // This test depends on feature flags being default-enabled in the test - // context, which requires those flags to exist in the ff.All slice. So - // let's just append them here so we do not need to use a "real" - // feature flag, as that would require constant change when we remove - // old feature flags. - defer func(old []ff.FeatureFlag) { - ff.All = old - }(ff.All) - ff.All = append(ff.All, featureFlagA, featureFlagB) + // Define two default-enabled feature flags. Note that with `NewFeatureFlag()`, we + // automatically add them to the list of defined feature flags. While this is stateful and + // would theoretically also impact other tests, we don't really need to mind that given + // that we use test-specific names for the flags here. + featureFlagA := ff.NewFeatureFlag("global_feature_flag_a", "", "", true) + featureFlagB := ff.NewFeatureFlag("global_feature_flag_b", "", "", true) var featureFlags [][2]bool NewFeatureSets(featureFlagB, featureFlagA).Run(t, func(t *testing.T, ctx context.Context) { diff --git a/internal/testhelper/testcfg/build.go b/internal/testhelper/testcfg/build.go index f4e6be180..6b5653931 100644 --- a/internal/testhelper/testcfg/build.go +++ b/internal/testhelper/testcfg/build.go @@ -6,6 +6,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strings" "sync" "testing" @@ -116,6 +117,24 @@ func BuildBinary(t testing.TB, targetDir, sourcePath string) string { "PATH=%s:%s", filepath.Dir(gitExecEnv.BinaryPath), os.Getenv("PATH"), )) + // Go 1.18 has started to extract VCS information so that it can be embedded into + // the resulting binary and will thus execute Git in the Gitaly repository. In CI, + // the Gitaly repository is owned by a different user than the one that is executing + // tests though, which means that Git will refuse to open the repository because of + // CVE-2022-24765. + // + // Let's override this mechanism by labelling the Git repository as safe. While this + // does in theory make us vulnerable to this exploit, it is clear that any adversary + // would already have arbitrary code execution because we are executing code right + // now that would be controlled by the very same adversary. + _, currentFile, _, ok := runtime.Caller(0) + require.True(t, ok) + gitEnvironment = append(gitEnvironment, + "GIT_CONFIG_COUNT=1", + "GIT_CONFIG_KEY_0=safe.directory", + "GIT_CONFIG_VALUE_0="+filepath.Join(filepath.Dir(currentFile), "..", "..", ".."), + ) + buildTags := []string{ "static", "system_libgit2", "gitaly_test", } |