Refactor to remove custom multitenancy options type; and make it opti…

…onal to supply a static list of tenants

Signed-off-by: Ed Snible <[email protected]>

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

cmd/collector/app/handler/grpc_handler.go

@@ -46,22 +46,18 @@ 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 {
 		if span.GetProcess() == nil {
 			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")
 	}
 

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)),
 	}
 }
 

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
 }

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)
 		})
 	}
 }

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,39 +38,48 @@ 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
 }
 
 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)