From b7fedd4561cf22384f9b695d59a62eb11abb11f7 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 7 Feb 2020 15:46:26 -0600 Subject: [PATCH 1/3] Pass specific feature flags to the Ruby server This will pass the value for specific allowed flags to the Ruby server via metadata. --- .../unreleased/rs-ruby-feature-flags.yml | 5 +++++ .../metadata/featureflag/feature_flags.go | 4 ++++ internal/rubyserver/proxy.go | 8 ++++++++ internal/rubyserver/proxy_test.go | 19 +++++++++++++++++++ ruby/lib/gitaly_server.rb | 7 +++++++ 5 files changed, 43 insertions(+) create mode 100644 changelogs/unreleased/rs-ruby-feature-flags.yml diff --git a/changelogs/unreleased/rs-ruby-feature-flags.yml b/changelogs/unreleased/rs-ruby-feature-flags.yml new file mode 100644 index 0000000000..9afff08171 --- /dev/null +++ b/changelogs/unreleased/rs-ruby-feature-flags.yml @@ -0,0 +1,5 @@ +--- +title: Pass specific feature flags to the Ruby server +merge_request: 1818 +author: +type: added diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go index 35aa7984bd..dc85077b4c 100644 --- a/internal/metadata/featureflag/feature_flags.go +++ b/internal/metadata/featureflag/feature_flags.go @@ -1,5 +1,9 @@ package featureflag +// RubyFeatureFlags allows specific flags to be passed to the Ruby server +// via metadata headers. All features should start with `ruby_` for clarity. +var RubyFeatureFlags = []string{"ruby_push_mirror_retries"} + const ( // UploadPackFilter enables partial clones by sending uploadpack.allowFilter and uploadpack.allowAnySHA1InWant // to upload-pack diff --git a/internal/rubyserver/proxy.go b/internal/rubyserver/proxy.go index 7535ab681b..5b07318384 100644 --- a/internal/rubyserver/proxy.go +++ b/internal/rubyserver/proxy.go @@ -4,9 +4,11 @@ import ( "context" "io" "os" + "strconv" "strings" "gitlab.com/gitlab-org/gitaly/internal/helper" + "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/metadata" ) @@ -62,6 +64,12 @@ func setHeaders(ctx context.Context, repo *gitalypb.Repository, mustExist bool) repoAltDirsHeader, repoAltDirsCombined, ) + // Feature flags for the Ruby server, passed via metadata headers + for _, rubyFf := range featureflag.RubyFeatureFlags { + headerName := featureflag.HeaderKey(rubyFf) + md = metadata.Join(md, metadata.Pairs(headerName, strconv.FormatBool(featureflag.IsEnabled(ctx, rubyFf)))) + } + if inMD, ok := metadata.FromIncomingContext(ctx); ok { for _, header := range ProxyHeaderWhitelist { for _, v := range inMD[header] { diff --git a/internal/rubyserver/proxy_test.go b/internal/rubyserver/proxy_test.go index 5b804c061d..e168501eeb 100644 --- a/internal/rubyserver/proxy_test.go +++ b/internal/rubyserver/proxy_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "google.golang.org/grpc/metadata" ) @@ -43,3 +44,21 @@ func TestSetHeadersPreservesWhitelistedMetadata(t *testing.T) { require.Equal(t, []string{value}, outMd[key], "outgoing MD should contain whitelisted key") } + +func TestRubyHeaders(t *testing.T) { + ctx, cancel := testhelper.Context() + defer cancel() + + key := featureflag.RubyFeatureFlags[0] + value := "false" + inCtx := metadata.NewIncomingContext(ctx, metadata.Pairs(key, value)) + + outCtx, err := SetHeaders(inCtx, testRepo) + require.NoError(t, err) + + outMd, ok := metadata.FromOutgoingContext(outCtx) + require.True(t, ok, "outgoing context should have metadata") + + headerName := featureflag.HeaderKey(key) + require.Equal(t, []string{value}, outMd[headerName], "outgoing MD should contain whitelisted feature key") +} diff --git a/ruby/lib/gitaly_server.rb b/ruby/lib/gitaly_server.rb index a57f4dc245..64b3b06e3f 100644 --- a/ruby/lib/gitaly_server.rb +++ b/ruby/lib/gitaly_server.rb @@ -20,6 +20,7 @@ module GitalyServer GL_REPOSITORY_HEADER = 'gitaly-gl-repository'.freeze REPO_ALT_DIRS_HEADER = 'gitaly-repo-alt-dirs'.freeze GITALY_SERVERS_HEADER = 'gitaly-servers'.freeze + RUBY_FEATURE_HEADER = 'gitaly-feature-ruby-'.freeze def self.storage_path(call) call.metadata.fetch(STORAGE_PATH_HEADER) @@ -37,6 +38,12 @@ module GitalyServer call.metadata.fetch(REPO_ALT_DIRS_HEADER) end + def self.feature_flags(call) + call.metadata.select do |key, _| + key.start_with?(RUBY_FEATURE_HEADER) + end + end + def self.client(call) Client.new(call.metadata[GITALY_SERVERS_HEADER]) end -- GitLab From 1e9d8c3095e4fcb2b921f1785a4290f97cf146bb Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Mon, 10 Feb 2020 16:41:07 -0600 Subject: [PATCH 2/3] Proxy automatically whitelists Ruby feature headers --- internal/metadata/featureflag/feature_flags.go | 4 ---- internal/rubyserver/proxy.go | 18 ++++++++++-------- internal/rubyserver/proxy_test.go | 10 ++++------ 3 files changed, 14 insertions(+), 18 deletions(-) diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go index dc85077b4c..35aa7984bd 100644 --- a/internal/metadata/featureflag/feature_flags.go +++ b/internal/metadata/featureflag/feature_flags.go @@ -1,9 +1,5 @@ package featureflag -// RubyFeatureFlags allows specific flags to be passed to the Ruby server -// via metadata headers. All features should start with `ruby_` for clarity. -var RubyFeatureFlags = []string{"ruby_push_mirror_retries"} - const ( // UploadPackFilter enables partial clones by sending uploadpack.allowFilter and uploadpack.allowAnySHA1InWant // to upload-pack diff --git a/internal/rubyserver/proxy.go b/internal/rubyserver/proxy.go index 5b07318384..cbe0947ecc 100644 --- a/internal/rubyserver/proxy.go +++ b/internal/rubyserver/proxy.go @@ -4,11 +4,9 @@ import ( "context" "io" "os" - "strconv" "strings" "gitlab.com/gitlab-org/gitaly/internal/helper" - "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/metadata" ) @@ -17,6 +15,9 @@ import ( // forwarded as-is to gitaly-ruby. var ProxyHeaderWhitelist = []string{"gitaly-servers"} +// Headers prefixed with this string get whitelisted automatically +const rubyFeaturePrefix = "gitaly-feature-ruby-" + const ( storagePathHeader = "gitaly-storage-path" repoPathHeader = "gitaly-repo-path" @@ -64,13 +65,14 @@ func setHeaders(ctx context.Context, repo *gitalypb.Repository, mustExist bool) repoAltDirsHeader, repoAltDirsCombined, ) - // Feature flags for the Ruby server, passed via metadata headers - for _, rubyFf := range featureflag.RubyFeatureFlags { - headerName := featureflag.HeaderKey(rubyFf) - md = metadata.Join(md, metadata.Pairs(headerName, strconv.FormatBool(featureflag.IsEnabled(ctx, rubyFf)))) - } - if inMD, ok := metadata.FromIncomingContext(ctx); ok { + // Automatically whitelist any Ruby-specific feature flag + for header := range inMD { + if strings.HasPrefix(header, rubyFeaturePrefix) { + ProxyHeaderWhitelist = append(ProxyHeaderWhitelist, header) + } + } + for _, header := range ProxyHeaderWhitelist { for _, v := range inMD[header] { md = metadata.Join(md, metadata.Pairs(header, v)) diff --git a/internal/rubyserver/proxy_test.go b/internal/rubyserver/proxy_test.go index e168501eeb..03936f41c6 100644 --- a/internal/rubyserver/proxy_test.go +++ b/internal/rubyserver/proxy_test.go @@ -4,7 +4,6 @@ import ( "testing" "github.com/stretchr/testify/require" - "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "google.golang.org/grpc/metadata" ) @@ -45,12 +44,12 @@ func TestSetHeadersPreservesWhitelistedMetadata(t *testing.T) { require.Equal(t, []string{value}, outMd[key], "outgoing MD should contain whitelisted key") } -func TestRubyHeaders(t *testing.T) { +func TestRubyFeatureHeaders(t *testing.T) { ctx, cancel := testhelper.Context() defer cancel() - key := featureflag.RubyFeatureFlags[0] - value := "false" + key := "gitaly-feature-ruby-test-feature" + value := "true" inCtx := metadata.NewIncomingContext(ctx, metadata.Pairs(key, value)) outCtx, err := SetHeaders(inCtx, testRepo) @@ -59,6 +58,5 @@ func TestRubyHeaders(t *testing.T) { outMd, ok := metadata.FromOutgoingContext(outCtx) require.True(t, ok, "outgoing context should have metadata") - headerName := featureflag.HeaderKey(key) - require.Equal(t, []string{value}, outMd[headerName], "outgoing MD should contain whitelisted feature key") + require.Equal(t, []string{value}, outMd[key], "outgoing MD should contain whitelisted feature key") } -- GitLab From 1cd28e7a614710f967bce8e71158c4ade4749d75 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Mon, 10 Feb 2020 19:44:57 -0600 Subject: [PATCH 3/3] Add GitalyServer::FeatureFlags interface to metadata flags --- .../unreleased/rs-ruby-feature-flags.yml | 2 +- ruby/lib/gitaly_server.rb | 6 +- ruby/lib/gitaly_server/feature_flags.rb | 59 +++++++++++++++++++ .../lib/gitaly_server/feature_flags_spec.rb | 45 ++++++++++++++ 4 files changed, 107 insertions(+), 5 deletions(-) create mode 100644 ruby/lib/gitaly_server/feature_flags.rb create mode 100644 ruby/spec/lib/gitaly_server/feature_flags_spec.rb diff --git a/changelogs/unreleased/rs-ruby-feature-flags.yml b/changelogs/unreleased/rs-ruby-feature-flags.yml index 9afff08171..c044dcb53e 100644 --- a/changelogs/unreleased/rs-ruby-feature-flags.yml +++ b/changelogs/unreleased/rs-ruby-feature-flags.yml @@ -1,5 +1,5 @@ --- -title: Pass specific feature flags to the Ruby server +title: Pass Ruby-specific feature flags to the Ruby server merge_request: 1818 author: type: added diff --git a/ruby/lib/gitaly_server.rb b/ruby/lib/gitaly_server.rb index 64b3b06e3f..acbfd2f965 100644 --- a/ruby/lib/gitaly_server.rb +++ b/ruby/lib/gitaly_server.rb @@ -13,6 +13,7 @@ require_relative 'gitaly_server/wiki_service.rb' require_relative 'gitaly_server/conflicts_service.rb' require_relative 'gitaly_server/remote_service.rb' require_relative 'gitaly_server/health_service.rb' +require_relative 'gitaly_server/feature_flags.rb' module GitalyServer STORAGE_PATH_HEADER = 'gitaly-storage-path'.freeze @@ -20,7 +21,6 @@ module GitalyServer GL_REPOSITORY_HEADER = 'gitaly-gl-repository'.freeze REPO_ALT_DIRS_HEADER = 'gitaly-repo-alt-dirs'.freeze GITALY_SERVERS_HEADER = 'gitaly-servers'.freeze - RUBY_FEATURE_HEADER = 'gitaly-feature-ruby-'.freeze def self.storage_path(call) call.metadata.fetch(STORAGE_PATH_HEADER) @@ -39,9 +39,7 @@ module GitalyServer end def self.feature_flags(call) - call.metadata.select do |key, _| - key.start_with?(RUBY_FEATURE_HEADER) - end + FeatureFlags.new(call.metadata) end def self.client(call) diff --git a/ruby/lib/gitaly_server/feature_flags.rb b/ruby/lib/gitaly_server/feature_flags.rb new file mode 100644 index 0000000000..fcfa994f8c --- /dev/null +++ b/ruby/lib/gitaly_server/feature_flags.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +module GitalyServer + # Interface to Ruby-specific feature flags passed to the Gitaly Ruby server + # via headers. + class FeatureFlags + # Only headers prefixed with this String will be made available + HEADER_PREFIX = 'gitaly-feature-ruby-' + + def initialize(metadata) + @flags = metadata.select do |key, _| + key.start_with?(HEADER_PREFIX) + end + end + + # Check if a given flag is enabled + # + # The `gitaly-feature-ruby-` prefix is optional, and underscores are + # translated to hyphens automatically. + # + # Examples + # + # enabled?('gitaly-feature-ruby-my-flag') + # => true + # + # enabled?(:my_flag) + # => true + # + # enabled?('my-flag') + # => true + # + # enabled?(:unknown_flag) + # => false + def enabled?(flag) + flag = normalize_flag(flag) + + @flags.fetch(flag, false) == 'true' + end + + def disabled?(flag) + !enabled?(flag) + end + + def inspect + pairs = @flags.map { |name, value| "#{name}=#{value}" } + pairs.unshift(self.class.name) + + "#<#{pairs.join(' ')}>" + end + + private + + def normalize_flag(flag) + flag = flag.to_s.delete_prefix(HEADER_PREFIX).tr('_', '-') + + "#{HEADER_PREFIX}#{flag}" + end + end +end diff --git a/ruby/spec/lib/gitaly_server/feature_flags_spec.rb b/ruby/spec/lib/gitaly_server/feature_flags_spec.rb new file mode 100644 index 0000000000..56a911bcb6 --- /dev/null +++ b/ruby/spec/lib/gitaly_server/feature_flags_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe GitalyServer::FeatureFlags do + describe '#enabled?' do + let(:metadata) do + { + "#{described_class::HEADER_PREFIX}some-feature" => 'true', + 'gitaly-storage-path' => 'foo', + 'gitaly-repo-path' => 'bar' + } + end + + subject { described_class.new(metadata) } + + it 'returns true for an enabled flag' do + expect(subject.enabled?(:some_feature)).to eq(true) + end + + it 'returns false for an unknown flag' do + expect(subject.enabled?(:missing_feature)).to eq(false) + end + + it 'removes the prefix if provided' do + expect(subject.enabled?(metadata.keys.first)).to eq(true) + end + + it 'translates underscores' do + expect(subject.enabled?('some-feature')).to eq(true) + end + end + + describe '#disabled?' do + it 'is the inverse of `enabled?`' do + instance = described_class.new({}) + + expect(instance).to receive(:enabled?) + .with(:some_feature) + .and_return(false) + + expect(instance.disabled?(:some_feature)).to eq(true) + end + end +end -- GitLab