From c27d510a9a9a920af3605f87a46a6c5cfd98bccb Mon Sep 17 00:00:00 2001 From: John Cai Date: Sun, 5 Jun 2022 23:44:51 -0400 Subject: git2go: Pass feature flags into gitaly-git2go binary Currently when we call gitaly-git2go, we lose feature flags since they are in the context inside the request that comes from Rails. In order to allow gitaly-git2go to benefit from feature flags, we can pass it in through an environment variable much like we do for gitaly-hooks. In order to test this, add a new subcommand for gitaly-git2go that simply returns which feature flags are set in the context. Since we only gets built for tests, build this in only with the `test` build tag. Changelog: changed --- Makefile | 1 + cmd/gitaly-git2go-v15/featureflags.go | 35 ++++++++++++++++++++ cmd/gitaly-git2go-v15/main.go | 52 +++++++++++++++++++++++++++-- internal/git2go/executor.go | 17 ++++++++++ internal/git2go/featureflags.go | 12 +++++++ internal/git2go/featureflags_test.go | 62 +++++++++++++++++++++++++++++++++++ internal/testhelper/testcfg/build.go | 2 +- 7 files changed, 178 insertions(+), 3 deletions(-) create mode 100644 cmd/gitaly-git2go-v15/featureflags.go create mode 100644 internal/git2go/featureflags.go create mode 100644 internal/git2go/featureflags_test.go diff --git a/Makefile b/Makefile index a97ea5138..7da3bd8e4 100644 --- a/Makefile +++ b/Makefile @@ -345,6 +345,7 @@ test-ruby: rspec .PHONY: test-go ## Run Go tests. test-go: prepare-tests + ${Q}GIT2GO_BUILD_TAGS="${GIT2GO_BUILD_TAGS},test" ${Q}$(call run_go_tests) .PHONY: test diff --git a/cmd/gitaly-git2go-v15/featureflags.go b/cmd/gitaly-git2go-v15/featureflags.go new file mode 100644 index 000000000..476454c94 --- /dev/null +++ b/cmd/gitaly-git2go-v15/featureflags.go @@ -0,0 +1,35 @@ +//go:build static && system_libgit2 && gitaly_test +// +build static,system_libgit2,gitaly_test + +package main + +import ( + "context" + "encoding/gob" + "flag" + + "gitlab.com/gitlab-org/gitaly/v15/internal/git2go" + "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" +) + +// This subcommand is only called in tests, so we don't want to register it like +// the other subcommands but instead will do it in an init block. The test build +// flag will guarantee that this is not built and registered in the +// gitaly-git2go binary +func init() { + subcommands["feature-flags"] = &featureFlagsSubcommand{} +} + +type featureFlagsSubcommand struct{} + +func (featureFlagsSubcommand) Flags() *flag.FlagSet { + return flag.NewFlagSet("feature-flags", flag.ExitOnError) +} + +func (featureFlagsSubcommand) Run(ctx context.Context, decoder *gob.Decoder, encoder *gob.Encoder) error { + rawFlags := featureflag.RawFromContext(ctx) + + return encoder.Encode(git2go.FeatureFlags{ + Raw: rawFlags, + }) +} diff --git a/cmd/gitaly-git2go-v15/main.go b/cmd/gitaly-git2go-v15/main.go index e4a51354e..060cdb033 100644 --- a/cmd/gitaly-git2go-v15/main.go +++ b/cmd/gitaly-git2go-v15/main.go @@ -9,12 +9,16 @@ import ( "flag" "fmt" "os" + "strconv" + "strings" "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" git "github.com/libgit2/git2go/v33" "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" ) @@ -61,11 +65,22 @@ func main() { encoder := gob.NewEncoder(os.Stdout) var logFormat, logLevel, correlationID string + var enabledFeatureFlags, disabledFeatureFlags featureFlagArg flags := flag.NewFlagSet(git2go.BinaryName, flag.PanicOnError) flags.StringVar(&logFormat, "log-format", "", "logging format") flags.StringVar(&logLevel, "log-level", "", "logging level") flags.StringVar(&correlationID, "correlation-id", "", "correlation ID used for request tracing") + flags.Var( + &enabledFeatureFlags, + "enabled-feature-flags", + "comma separated list of explicitly enabled feature flags", + ) + flags.Var( + &disabledFeatureFlags, + "disabled-feature-flags", + "comma separated list of explicitly disabled feature flags", + ) _ = flags.Parse(os.Args[1:]) if correlationID == "" { @@ -76,8 +91,10 @@ func main() { ctx := correlation.ContextWithCorrelation(context.Background(), correlationID) logger := glog.Default().WithFields(logrus.Fields{ - "command.name": git2go.BinaryName, - "correlation_id": correlationID, + "command.name": git2go.BinaryName, + "correlation_id": correlationID, + "enabled_feature_flags": enabledFeatureFlags, + "disabled_feature_flags": disabledFeatureFlags, }) if flags.NArg() < 1 { @@ -116,6 +133,13 @@ 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)) + if err := subcmd.Run(ctx, decoder, encoder); err != nil { subcmdLogger.WithError(err).Errorf("%s command failed", subcmdFlags.Name()) fatalf(logger, encoder, "%s: %s", subcmdFlags.Name(), err) @@ -123,3 +147,27 @@ func main() { subcmdLogger.Infof("%s command finished", subcmdFlags.Name()) } + +type featureFlagArg []string + +func (v *featureFlagArg) String() string { + return strings.Join(*v, ",") +} + +func (v *featureFlagArg) Set(s string) error { + if s == "" { + return nil + } + + for _, enabledFF := range strings.Split(s, ",") { + *v = append(*v, enabledFF) + } + + return nil +} + +func (v featureFlagArg) SetRaw(raw featureflag.Raw, enabled bool) { + for _, ff := range v { + raw[ff] = strconv.FormatBool(enabled) + } +} diff --git a/internal/git2go/executor.go b/internal/git2go/executor.go index 899c7e64e..75d451fe0 100644 --- a/internal/git2go/executor.go +++ b/internal/git2go/executor.go @@ -9,6 +9,7 @@ import ( "io" "os/exec" "path/filepath" + "strings" "gitlab.com/gitlab-org/gitaly/v15/internal/command" "gitlab.com/gitlab-org/gitaly/v15/internal/git" @@ -17,6 +18,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/storage" glog "gitlab.com/gitlab-org/gitaly/v15/internal/log" + "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v15/internal/version" "gitlab.com/gitlab-org/labkit/correlation" ) @@ -55,6 +57,19 @@ func (b *Executor) run(ctx context.Context, repo repository.GitRepo, stdin io.Re return nil, fmt.Errorf("gitaly-git2go: %w", err) } + var enabledFeatureFlags, disabledFeatureFlags []string + + for ff, value := range featureflag.RawFromContext(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) + } + } + env := alternates.Env(repoPath, repo.GetGitObjectDirectory(), repo.GetGitAlternateObjectDirectories()) env = append(env, b.gitCmdFactory.GetExecutionEnvironment(ctx).EnvironmentVariables...) @@ -67,6 +82,8 @@ func (b *Executor) run(ctx context.Context, repo repository.GitRepo, stdin io.Re "-log-format", b.logFormat, "-log-level", b.logLevel, "-correlation-id", correlation.ExtractFromContext(ctx), + "-enabled-feature-flags", strings.Join(enabledFeatureFlags, ","), + "-disabled-feature-flags", strings.Join(disabledFeatureFlags, ","), subcmd, }, args...) diff --git a/internal/git2go/featureflags.go b/internal/git2go/featureflags.go new file mode 100644 index 000000000..cabc60e6f --- /dev/null +++ b/internal/git2go/featureflags.go @@ -0,0 +1,12 @@ +package git2go + +// 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 + // 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 new file mode 100644 index 000000000..1ea54df52 --- /dev/null +++ b/internal/git2go/featureflags_test.go @@ -0,0 +1,62 @@ +package git2go + +import ( + "context" + "encoding/gob" + "strconv" + "testing" + + "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest" + "gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo" + "gitlab.com/gitlab-org/gitaly/v15/internal/git/repository" + "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config" + "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" + "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" + "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg" +) + +func (b *Executor) FeatureFlags(ctx context.Context, repo repository.GitRepo) (featureflag.Raw, error) { + output, err := b.run(ctx, repo, nil, "feature-flags") + if err != nil { + return nil, err + } + + var result FeatureFlags + if err := gob.NewDecoder(output).Decode(&result); err != nil { + return nil, err + } + + if result.Err != nil { + return nil, result.Err + } + + return result.Raw, err +} + +var ( + 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 testExecutorFeatureFlags(t *testing.T, ctx context.Context) { + cfg := testcfg.Build(t) + testcfg.BuildGitalyGit2Go(t, cfg) + + repoProto, _ := gittest.InitRepo(t, cfg, cfg.Storages[0]) + + repo := localrepo.NewTestRepo(t, cfg, repoProto) + + executor := NewExecutor(cfg, gittest.NewCommandFactory(t, cfg), config.NewLocator(cfg)) + + raw, 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"]) +} diff --git a/internal/testhelper/testcfg/build.go b/internal/testhelper/testcfg/build.go index 59ab9ee7a..262bc0058 100644 --- a/internal/testhelper/testcfg/build.go +++ b/internal/testhelper/testcfg/build.go @@ -119,7 +119,7 @@ func BuildBinary(t testing.TB, targetDir, sourcePath string) string { cmd := exec.Command( "go", "build", - "-tags", "static,system_libgit2", + "-tags", "static,system_libgit2,gitaly_test", "-o", sharedBinaryPath, sourcePath, ) -- cgit v1.2.3