From 95325484419369a0de84b6e257d76a48b210c05a Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Fri, 20 May 2022 10:43:36 -0400 Subject: [PATCH] Refactor to remove custom multitenancy options type; and make it optional to supply a static list of tenants Signed-off-by: Ed Snible --- cmd/collector/app/builder_flags.go | 8 ++-- cmd/collector/app/handler/grpc_handler.go | 20 +++++----- cmd/collector/app/span_handler_builder.go | 2 +- pkg/config/tenancy/flags.go | 15 ++------ pkg/config/tenancy/flags_test.go | 45 ++++++----------------- pkg/config/tenancy/tenancy.go | 31 ++++++++++------ 6 files changed, 49 insertions(+), 72 deletions(-) diff --git a/cmd/collector/app/builder_flags.go b/cmd/collector/app/builder_flags.go index 64ea7500e54..7de078ba2bd 100644 --- a/cmd/collector/app/builder_flags.go +++ b/cmd/collector/app/builder_flags.go @@ -55,8 +55,6 @@ var tlsZipkinFlagsConfig = tlscfg.ServerFlagsConfig{ Prefix: "collector.zipkin", } -var tenancyFlagsConfig = tenancy.TenancyFlagsConfig{} - // CollectorOptions holds configuration for collector type CollectorOptions struct { // DynQueueSizeMemory determines how much memory to use for the queue @@ -91,8 +89,8 @@ type CollectorOptions struct { // CollectorGRPCMaxConnectionAgeGrace is an additive period after MaxConnectionAge after which the connection will be forcibly closed. // See gRPC's keepalive.ServerParameters#MaxConnectionAgeGrace. CollectorGRPCMaxConnectionAgeGrace time.Duration - // TenancyGRPC configures tenancy for gRPC endpoint to collect spans - TenancyGRPC tenancy.Options + // Tenancy configures tenancy for endpoints that collect spans + Tenancy tenancy.Options } // AddFlags adds flags for CollectorOptions @@ -114,7 +112,7 @@ func AddFlags(flags *flag.FlagSet) { tlsHTTPFlagsConfig.AddFlags(flags) tlsZipkinFlagsConfig.AddFlags(flags) - tenancyFlagsConfig.AddFlags(flags) + tenancy.AddFlags(flags) } // InitFromViper initializes CollectorOptions with properties from viper diff --git a/cmd/collector/app/handler/grpc_handler.go b/cmd/collector/app/handler/grpc_handler.go index a23e6253fa2..0fe218b8f29 100644 --- a/cmd/collector/app/handler/grpc_handler.go +++ b/cmd/collector/app/handler/grpc_handler.go @@ -46,14 +46,10 @@ func NewGRPCHandler(logger *zap.Logger, spanProcessor processor.SpanProcessor, t // PostSpans implements gRPC CollectorService. func (g *GRPCHandler) PostSpans(ctx context.Context, r *api_v2.PostSpansRequest) (*api_v2.PostSpansResponse, error) { - var tenant string - if g.tenancyConfig.Enabled { - var err error - tenant, err = g.validateTenant(ctx) - if err != nil { - g.logger.Error("rejecting spans (tenancy)", zap.Error(err)) - return nil, err - } + tenant, err := g.validateTenant(ctx) + if err != nil { + g.logger.Error("rejecting spans (tenancy)", zap.Error(err)) + return nil, err } for _, span := range r.GetBatch().Spans { @@ -61,7 +57,7 @@ func (g *GRPCHandler) PostSpans(ctx context.Context, r *api_v2.PostSpansRequest) span.Process = r.Batch.Process } } - _, err := g.spanProcessor.ProcessSpans(r.GetBatch().Spans, processor.SpansOptions{ + _, err = g.spanProcessor.ProcessSpans(r.GetBatch().Spans, processor.SpansOptions{ InboundTransport: processor.GRPCTransport, SpanFormat: processor.ProtoSpanFormat, Tenant: tenant, @@ -77,6 +73,10 @@ func (g *GRPCHandler) PostSpans(ctx context.Context, r *api_v2.PostSpansRequest) } func (g *GRPCHandler) validateTenant(ctx context.Context) (string, error) { + if !g.tenancyConfig.Enabled { + return "", nil + } + md, ok := metadata.FromIncomingContext(ctx) if !ok { return "", status.Errorf(codes.PermissionDenied, "missing tenant header") @@ -89,7 +89,7 @@ func (g *GRPCHandler) validateTenant(ctx context.Context) (string, error) { return "", status.Errorf(codes.PermissionDenied, "extra tenant header") } - if !g.tenancyConfig.Valid.Valid(tenants[0]) { + if !g.tenancyConfig.Valid(tenants[0]) { return "", status.Errorf(codes.PermissionDenied, "unknown tenant") } diff --git a/cmd/collector/app/span_handler_builder.go b/cmd/collector/app/span_handler_builder.go index 4477bf0101a..f3dbf078f9d 100644 --- a/cmd/collector/app/span_handler_builder.go +++ b/cmd/collector/app/span_handler_builder.go @@ -74,7 +74,7 @@ func (b *SpanHandlerBuilder) BuildHandlers(spanProcessor processor.SpanProcessor zs.NewChainedSanitizer(zs.NewStandardSanitizers()...), ), handler.NewJaegerSpanHandler(b.Logger, spanProcessor), - handler.NewGRPCHandler(b.Logger, spanProcessor, tenancy.NewTenancyConfig(&b.CollectorOpts.TenancyGRPC)), + handler.NewGRPCHandler(b.Logger, spanProcessor, tenancy.NewTenancyConfig(&b.CollectorOpts.Tenancy)), } } diff --git a/pkg/config/tenancy/flags.go b/pkg/config/tenancy/flags.go index f028ac56544..307e5a8f301 100644 --- a/pkg/config/tenancy/flags.go +++ b/pkg/config/tenancy/flags.go @@ -16,7 +16,6 @@ package tenancy import ( "flag" - "fmt" "strings" "github.com/spf13/viper" @@ -28,27 +27,19 @@ const ( validTenants = "multi_tenancy.tenants" ) -// TenancyFlagsConfig describes which CLI flags for TLS client should be generated. -type TenancyFlagsConfig struct { -} - // AddFlags adds flags for tenancy to the FlagSet. -func (c TenancyFlagsConfig) AddFlags(flags *flag.FlagSet) { +func AddFlags(flags *flag.FlagSet) { flags.Bool(tenancyEnabled, false, "Enable tenancy header when receiving or querying") flags.String(tenancyHeader, "x-tenant", "HTTP header carrying tenant") flags.String(validTenants, "", "Acceptable tenants") } -// InitFromViper creates tls.Config populated with values retrieved from Viper. -func (c TenancyFlagsConfig) InitFromViper(v *viper.Viper) (Options, error) { +// InitFromViper creates tenancy.Options populated with values retrieved from Viper. +func InitFromViper(v *viper.Viper) (Options, error) { var p Options p.Enabled = v.GetBool(tenancyEnabled) p.Header = v.GetString(tenancyHeader) p.Tenants = strings.Split(v.GetString(validTenants), ",") - if p.Enabled && len(p.Tenants) == 0 { - return p, fmt.Errorf("%s must have at least one tenant when tenancy is enabled", validTenants) - } - return p, nil } diff --git a/pkg/config/tenancy/flags_test.go b/pkg/config/tenancy/flags_test.go index a938557834e..f65653e49d9 100644 --- a/pkg/config/tenancy/flags_test.go +++ b/pkg/config/tenancy/flags_test.go @@ -67,38 +67,17 @@ func TestTenancyFlags(t *testing.T) { Tenants: []string{"acme"}, }, }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - v := viper.New() - command := cobra.Command{} - flagSet := &flag.FlagSet{} - flagCfg := TenancyFlagsConfig{} - flagCfg.AddFlags(flagSet) - command.PersistentFlags().AddGoFlagSet(flagSet) - v.BindPFlags(command.PersistentFlags()) - - err := command.ParseFlags(test.cmd) - require.NoError(t, err) - tenancyCfg, err := flagCfg.InitFromViper(v) - require.NoError(t, err) - assert.Equal(t, test.expected, tenancyCfg) - }) - } -} - -// TestFailedTLSFlags verifies that TLS options cannot be used when tls.enabled=false -func TestInvalidTenancyFlags(t *testing.T) { - tests := []struct { - name string - cmd []string - }{ { - name: "no tenants", + // Not supplying a list of tenants will mean + // "tenant header required, but any value will pass" + name: "no_tenants", cmd: []string{ "--multi_tenancy.enabled=true", - "--multi_tenancy.tenants=", + }, + expected: Options{ + Enabled: true, + Header: "x-tenant", + Tenants: []string{""}, }, }, } @@ -108,15 +87,15 @@ func TestInvalidTenancyFlags(t *testing.T) { v := viper.New() command := cobra.Command{} flagSet := &flag.FlagSet{} - flagCfg := TenancyFlagsConfig{} - flagCfg.AddFlags(flagSet) + AddFlags(flagSet) command.PersistentFlags().AddGoFlagSet(flagSet) v.BindPFlags(command.PersistentFlags()) err := command.ParseFlags(test.cmd) require.NoError(t, err) - _, err = flagCfg.InitFromViper(v) - require.Error(t, err) + tenancyCfg, err := InitFromViper(v) + require.NoError(t, err) + assert.Equal(t, test.expected, tenancyCfg) }) } } diff --git a/pkg/config/tenancy/tenancy.go b/pkg/config/tenancy/tenancy.go index 772051a8455..1b4ddb8e6cb 100644 --- a/pkg/config/tenancy/tenancy.go +++ b/pkg/config/tenancy/tenancy.go @@ -18,11 +18,11 @@ package tenancy type TenancyConfig struct { Enabled bool Header string - Valid Guard + guard guard } // Guard verifies a valid tenant when tenancy is enabled -type Guard interface { +type guard interface { Valid(candidate string) bool } @@ -38,13 +38,17 @@ func NewTenancyConfig(options *Options) *TenancyConfig { return &TenancyConfig{ Enabled: options.Enabled, Header: options.Header, - Valid: tenancyGuardFactory(options), + guard: tenancyGuardFactory(options), } } -type tenantUnaware bool +func (tc *TenancyConfig) Valid(tenant string) bool { + return tc.guard.Valid(tenant) +} + +type tenantDontCare bool -func (tenantUnaware) Valid(candidate string) bool { +func (tenantDontCare) Valid(candidate string) bool { return true } @@ -52,25 +56,30 @@ type tenantList struct { tenants map[string]bool } -func (tl tenantList) Valid(candidate string) bool { +func (tl *tenantList) Valid(candidate string) bool { _, ok := tl.tenants[candidate] return ok } -func newTenantList(tenants []string) tenantList { +func newTenantList(tenants []string) *tenantList { tenantMap := make(map[string]bool) for _, tenant := range tenants { tenantMap[tenant] = true } - return tenantList{ + return &tenantList{ tenants: tenantMap, } } -func tenancyGuardFactory(options *Options) Guard { - if !options.Enabled { - return tenantUnaware(true) +func tenancyGuardFactory(options *Options) guard { + // Three cases + // - no tenancy + // - tenancy, but no guarding by tenant + // - tenancy, with guarding by a list + + if !options.Enabled || len(options.Tenants) == 0 { + return tenantDontCare(true) } return newTenantList(options.Tenants)