Repository: grafana/dskit (go)

Issue:
Tenant package relies on mutable global state
The `tenant` package is used for (among other things) extracting a tenant ID from a `context.Context`. 

There are two separate implementations of this logic:
* Logic that understands tenant IDs separated by a `|` and so can return multiple tenant IDs from a context.
* Logic that does not understand tenant IDs separate by a `|` and so returns an error in this case.

There is a `WithDefaultResolver` that changes which logic is used, [via mutating a global](https://github.com/grafana/dskit/blob/4cfd2dc0d14c7e79e6482f231c8c58ae6ceea6d7/tenant/resolver.go#L14-L17). [Mimir](https://github.com/grafana/mimir/blob/e974d3d816e42b8c182b7acbc0e205ce5251da44/pkg/mimir/mimir.go#L720-L727) and [Loki](https://github.com/grafana/loki/blob/bd505f8e2d37172ff35a89f4ac42efec9566a263/pkg/loki/modules.go#L332-L337) do this in response to some form of multi-tenant queries being enabled or disabled by configuration.

This is a problem because the behavior of huge portions of each database can change by changes made far away from the code in question. It also makes it more difficult to unit test the code in question since adjusting the global state for a single test affects the behavior of every other test.

I propose:

* Remove the `WithDefaultResolver` to prevent mutation of global state
* Remove all global state since it's unnecessary. The logic of `MultiResolver` would move to the global `tenant.TenantID()` and `tenant.TenantIDs()` methods. 
* This would also make `SingleResolver`, `MultiResolver`, and `Resolver` unnecessary

This would require changes in Mimir and Loki to reject multi-tenant queries when they aren't enabled. However, this would result in a better design anyway since the databases are checking for and enforcing exactly what they intend to enforce.

Pull request:
Remove global state from tenant ID parsing package
**What this PR does**:

Remove the ability to set a global "default" parser for tenant IDs since this makes uses of this package fragile and bug prone. This also removes the `SingleResolver` struct which does not support multiple tenant IDs and was previously the default logic.

All code for parsing tenant IDs is now aware of multiple tenant IDs separated by a `|` character. Consumers that don't want to support multiple tenant IDs at all should use the `TenantID()` method which returns an error if there are multiple tenant IDs. Consumers that wish to optionally support multiple tenant IDs should validate incoming input to ensure only a single ID is present.

This changes the default behavior of `TenantID()` and `TenantIDs()` in two
ways:
    
* `SingleResolver` did not previously enforce a limit on the length of a tenant
   ID. A limit of 150 characters is now enforced. This has always been the
   documented behavior as far back as Cortex, where this code originated. Not
   enforcing it was an oversight.
* `SingleResolver` previously allowed tenant IDs to contain the `|` character.
   This is no longer allowed as part of a tenant ID and instead will be treated
   as a divider between multiple tenant IDs. This has always been the documented
   behavior as far back as Cortex, where this code originated. Not enforcing it
   was an oversight.

**Which issue(s) this PR fixes**:

Fixes #443

**Checklist**
- [X] Tests updated
- [X] `CHANGELOG.md` updated - the order of entries should be `[CHANGE]`, `[FEATURE]`, `[ENHANCEMENT]`, `[BUGFIX]`


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index b3564a1..4023b08 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -60,6 +60,7 @@
 * [CHANGE] server: fix incorrect spelling of "gRPC" in `server.Config` fields. #422
 * [CHANGE] memberlist: re-resolve `JoinMembers` during full joins to the cluster (either at startup or periodic). The re-resolution happens on every 100 attempted nodes. This helps speed up joins and respect context cancelation #411
 * [CHANGE] ring: `ring.DoBatch()` was deprecated in favor of `DoBatchWithOptions()`. #431
+* [CHANGE] tenant: Remove `tenant.WithDefaultResolver()` and `SingleResolver` in favor of global functions `tenant.TenantID()`, `tenant.TenantIDs()`,  or `MultiResolver`. #445
 * [FEATURE] Cache: Add support for configuring a Redis cache backend. #268 #271 #276
 * [FEATURE] Add support for waiting on the rate limiter using the new `WaitN` method. #279
 * [FEATURE] Add `log.BufferedLogger` type. #338
diff --git a/tenant/resolver.go b/tenant/resolver.go
index aa19d75..35e95b1 100644
--- a/tenant/resolver.go
+++ b/tenant/resolver.go
@@ -2,20 +2,11 @@ package tenant
 
 import (
 	"context"
-	"errors"
-	"net/http"
 	"strings"
 
 	"github.com/grafana/dskit/user"
 )
 
-var defaultResolver Resolver = NewSingleResolver()
-
-// WithDefaultResolver updates the resolver used for the package methods.
-func WithDefaultResolver(r Resolver) {
-	defaultResolver = r
-}
-
 // TenantID returns exactly a single tenant ID from the context. It should be
 // used when a certain endpoint should only support exactly a single
 // tenant ID. It returns an error user.ErrNoOrgID if there is no tenant ID
@@ -25,7 +16,16 @@ func WithDefaultResolver(r Resolver) {
 //
 //nolint:revive
 func TenantID(ctx context.Context) (string, error) {
-	return defaultResolver.TenantID(ctx)
+	orgIDs, err := TenantIDs(ctx)
+	if err != nil {
+		return "", err
+	}
+
+	if len(orgIDs) > 1 {
+		return "", user.ErrTooManyOrgIDs
+	}
+
+	return orgIDs[0], nil
 }
 
 // TenantIDs returns all tenant IDs from the context. It should return
@@ -36,7 +36,20 @@ func TenantID(ctx context.Context) (string, error) {
 //
 //nolint:revive
 func TenantIDs(ctx context.Context) ([]string, error) {
-	return defaultResolver.TenantIDs(ctx)
+	//lint:ignore faillint wrapper around upstream method
+	orgID, err := user.ExtractOrgID(ctx)
+	if err != nil {
+		return nil, err
+	}
+
+	orgIDs := strings.Split(orgID, tenantIDsSeparator)
+	for _, id := range orgIDs {
+		if err := ValidTenantID(id); err != nil {
+			return nil, err
+		}
+	}
+
+	return NormalizeTenantIDs(orgIDs), nil
 }
 
 type Resolver interface {
@@ -52,109 +65,20 @@ type Resolver interface {
 	TenantIDs(context.Context) ([]string, error)
 }
 
-// NewSingleResolver creates a tenant resolver, which restricts all requests to
-// be using a single tenant only. This allows a wider set of characters to be
-// used within the tenant ID and should not impose a breaking change.
-func NewSingleResolver() *SingleResolver {
-	return &SingleResolver{}
-}
-
-type SingleResolver struct {
-}
-
-// containsUnsafePathSegments will return true if the string is a directory
-// reference like `.` and `..` or if any path separator character like `/` and
-// `\` can be found.
-func containsUnsafePathSegments(id string) bool {
-	// handle the relative reference to current and parent path.
-	if id == "." || id == ".." {
-		return true
-	}
-
-	return strings.ContainsAny(id, "\\/")
-}
-
-var errInvalidTenantID = errors.New("invalid tenant ID")
-
-func (t *SingleResolver) TenantID(ctx context.Context) (string, error) {
-	//lint:ignore faillint wrapper around upstream method
-	id, err := user.ExtractOrgID(ctx)
-	if err != nil {
-		return "", err
-	}
-
-	if containsUnsafePathSegments(id) {
-		return "", errInvalidTenantID
-	}
-
-	return id, nil
-}
-
-func (t *SingleResolver) TenantIDs(ctx context.Context) ([]string, error) {
-	orgID, err := t.TenantID(ctx)
-	if err != nil {
-		return nil, err
-	}
-	return []string{orgID}, err
-}
-
-type MultiResolver struct {
-}
+type MultiResolver struct{}
 
 // NewMultiResolver creates a tenant resolver, which allows request to have
 // multiple tenant ids submitted separated by a '|' character. This enforces
 // further limits on the character set allowed within tenants as detailed here:
-// https://cortexmetrics.io/docs/guides/limitations/#tenant-id-naming)
+// https://grafana.com/docs/mimir/latest/configure/about-tenant-ids/
 func NewMultiResolver() *MultiResolver {
 	return &MultiResolver{}
 }
 
 func (t *MultiResolver) TenantID(ctx context.Context) (string, error) {
-	orgIDs, err := t.TenantIDs(ctx)
-	if err != nil {
-		return "", err
-	}
-
-	if len(orgIDs) > 1 {
-		return "", user.ErrTooManyOrgIDs
-	}
-
-	return orgIDs[0], nil
+	return TenantID(ctx)
 }
 
 func (t *MultiResolver) TenantIDs(ctx context.Context) ([]string, error) {
-	//lint:ignore faillint wrapper around upstream method
-	orgID, err := user.ExtractOrgID(ctx)
-	if err != nil {
-		return nil, err
-	}
-
-	orgIDs := strings.Split(orgID, tenantIDsLabelSeparator)
-	for _, orgID := range orgIDs {
-		if err := ValidTenantID(orgID); err != nil {
-			return nil, err
-		}
-		if containsUnsafePathSegments(orgID) {
-			return nil, errInvalidTenantID
-		}
-	}
-
-	return NormalizeTenantIDs(orgIDs), nil
-}
-
-// ExtractTenantIDFromHTTPRequest extracts a single TenantID through a given
-// resolver directly from a HTTP request.
-func ExtractTenantIDFromHTTPRequest(req *http.Request) (string, context.Context, error) {
-	//lint:ignore faillint wrapper around upstream method
-	_, ctx, err := user.ExtractOrgIDFromHTTPRequest(req)
-	if err != nil {
-		return "", nil, err
-	}
-
-	tenantID, err := defaultResolver.TenantID(ctx)
-	if err != nil {
-		return "", nil, err
-	}
-
-	return tenantID, ctx, nil
+	return TenantIDs(ctx)
 }
diff --git a/tenant/tenant.go b/tenant/tenant.go
index a580750..4a89b57 100644
--- a/tenant/tenant.go
+++ b/tenant/tenant.go
@@ -4,14 +4,23 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"net/http"
 	"sort"
 	"strings"
 
 	"github.com/grafana/dskit/user"
 )
 
+const (
+	// MaxTenantIDLength is the max length of single tenant ID in bytes
+	MaxTenantIDLength = 150
+
+	tenantIDsSeparator = "|"
+)
+
 var (
-	errTenantIDTooLong = errors.New("tenant ID is too long: max 150 characters")
+	errTenantIDTooLong = fmt.Errorf("tenant ID is too long: max %d characters", MaxTenantIDLength)
+	errUnsafeTenantID  = errors.New("tenant ID is '.' or '..'")
 )
 
 type errTenantIDUnsupportedCharacter struct {
@@ -27,9 +36,7 @@ func (e *errTenantIDUnsupportedCharacter) Error() string {
 	)
 }
 
-const tenantIDsLabelSeparator = "|"
-
-// NormalizeTenantIDs is creating a normalized form by sortiing and de-duplicating the list of tenantIDs
+// NormalizeTenantIDs creates a normalized form by sorting and de-duplicating the list of tenantIDs
 func NormalizeTenantIDs(tenantIDs []string) []string {
 	sort.Strings(tenantIDs)
 
@@ -49,7 +56,7 @@ func NormalizeTenantIDs(tenantIDs []string) []string {
 	return tenantIDs[0:posOut]
 }
 
-// ValidTenantID
+// ValidTenantID returns an error if the single tenant ID is invalid, nil otherwise
 func ValidTenantID(s string) error {
 	// check if it contains invalid runes
 	for pos, r := range s {
@@ -61,19 +68,49 @@ func ValidTenantID(s string) error {
 		}
 	}
 
-	if len(s) > 150 {
+	if len(s) > MaxTenantIDLength {
 		return errTenantIDTooLong
 	}
 
+	if containsUnsafePathSegments(s) {
+		return errUnsafeTenantID
+	}
+
 	return nil
 }
 
+// JoinTenantIDs returns all tenant IDs concatenated with the separator character `|`
 func JoinTenantIDs(tenantIDs []string) string {
-	return strings.Join(tenantIDs, tenantIDsLabelSeparator)
+	return strings.Join(tenantIDs, tenantIDsSeparator)
+}
+
+// ExtractTenantIDFromHTTPRequest extracts a single tenant ID directly from a HTTP request.
+func ExtractTenantIDFromHTTPRequest(req *http.Request) (string, context.Context, error) {
+	//lint:ignore faillint wrapper around upstream method
+	_, ctx, err := user.ExtractOrgIDFromHTTPRequest(req)
+	if err != nil {
+		return "", nil, err
+	}
+
+	tenantID, err := TenantID(ctx)
+	if err != nil {
+		return "", nil, err
+	}
+
+	return tenantID, ctx, nil
+}
+
+// TenantIDsFromOrgID extracts different tenants from an orgID string value
+//
+// ignore stutter warning
+//
+//nolint:revive
+func TenantIDsFromOrgID(orgID string) ([]string, error) {
+	return TenantIDs(user.InjectOrgID(context.TODO(), orgID))
 }
 
 // this checks if a rune is supported in tenant IDs (according to
-// https://cortexmetrics.io/docs/guides/limitations/#tenant-id-naming)
+// https://grafana.com/docs/mimir/latest/configure/about-tenant-ids/
 func isSupported(c rune) bool {
 	// characters
 	if ('a' <= c && c <= 'z') || ('A' <= c && c <= 'Z') {
@@ -96,11 +133,8 @@ func isSupported(c rune) bool {
 		c == ')'
 }
 
-// TenantIDsFromOrgID extracts different tenants from an orgID string value
-//
-// ignore stutter warning
-//
-//nolint:revive
-func TenantIDsFromOrgID(orgID string) ([]string, error) {
-	return TenantIDs(user.InjectOrgID(context.TODO(), orgID))
+// containsUnsafePathSegments will return true if the string is a directory
+// reference like `.` and `..`
+func containsUnsafePathSegments(id string) bool {
+	return id == "." || id == ".."
 }


Test patch:
diff --git a/tenant/resolver_test.go b/tenant/resolver_test.go
index c71c013..03c0431 100644
--- a/tenant/resolver_test.go
+++ b/tenant/resolver_test.go
@@ -2,6 +2,7 @@ package tenant
 
 import (
 	"context"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -22,92 +23,36 @@ type resolverTestCase struct {
 	tenantIDs    []string
 }
 
-func (tc *resolverTestCase) test(r Resolver) func(t *testing.T) {
-	return func(t *testing.T) {
-
-		ctx := context.Background()
-		if tc.headerValue != nil {
-			ctx = user.InjectOrgID(ctx, *tc.headerValue)
-		}
-
-		tenantID, err := r.TenantID(ctx)
-		if tc.errTenantID != nil {
-			assert.Equal(t, tc.errTenantID, err)
-		} else {
-			assert.NoError(t, err)
-			assert.Equal(t, tc.tenantID, tenantID)
-		}
-
-		tenantIDs, err := r.TenantIDs(ctx)
-		if tc.errTenantIDs != nil {
-			assert.Equal(t, tc.errTenantIDs, err)
-		} else {
-			assert.NoError(t, err)
-			assert.Equal(t, tc.tenantIDs, tenantIDs)
-		}
-	}
-}
-
-var commonResolverTestCases = []resolverTestCase{
-	{
-		name:         "no-header",
-		errTenantID:  user.ErrNoOrgID,
-		errTenantIDs: user.ErrNoOrgID,
-	},
-	{
-		name:        "empty",
-		headerValue: strptr(""),
-		tenantIDs:   []string{""},
-	},
-	{
-		name:        "single-tenant",
-		headerValue: strptr("tenant-a"),
-		tenantID:    "tenant-a",
-		tenantIDs:   []string{"tenant-a"},
-	},
-	{
-		name:         "parent-dir",
-		headerValue:  strptr(".."),
-		errTenantID:  errInvalidTenantID,
-		errTenantIDs: errInvalidTenantID,
-	},
-	{
-		name:         "current-dir",
-		headerValue:  strptr("."),
-		errTenantID:  errInvalidTenantID,
-		errTenantIDs: errInvalidTenantID,
-	},
-}
-
-func TestSingleResolver(t *testing.T) {
-	r := NewSingleResolver()
-	for _, tc := range append(commonResolverTestCases, []resolverTestCase{
+func TestTenantIDs(t *testing.T) {
+	for _, tc := range []resolverTestCase{
 		{
-			name:        "multi-tenant",
-			headerValue: strptr("tenant-a|tenant-b"),
-			tenantID:    "tenant-a|tenant-b",
-			tenantIDs:   []string{"tenant-a|tenant-b"},
+			name:         "no-header",
+			errTenantID:  user.ErrNoOrgID,
+			errTenantIDs: user.ErrNoOrgID,
 		},
 		{
-			name:         "containing-forward-slash",
-			headerValue:  strptr("forward/slash"),
-			errTenantID:  errInvalidTenantID,
-			errTenantIDs: errInvalidTenantID,
+			name:        "empty",
+			headerValue: strptr(""),
+			tenantIDs:   []string{""},
 		},
 		{
-			name:         "containing-backward-slash",
-			headerValue:  strptr(`backward\slash`),
-			errTenantID:  errInvalidTenantID,
-			errTenantIDs: errInvalidTenantID,
+			name:        "single-tenant",
+			headerValue: strptr("tenant-a"),
+			tenantID:    "tenant-a",
+			tenantIDs:   []string{"tenant-a"},
+		},
+		{
+			name:         "parent-dir",
+			headerValue:  strptr(".."),
+			errTenantID:  errUnsafeTenantID,
+			errTenantIDs: errUnsafeTenantID,
+		},
+		{
+			name:         "current-dir",
+			headerValue:  strptr("."),
+			errTenantID:  errUnsafeTenantID,
+			errTenantIDs: errUnsafeTenantID,
 		},
-	}...) {
-		t.Run(tc.name, tc.test(r))
-	}
-}
-
-func TestMultiResolver(t *testing.T) {
-	r := NewMultiResolver()
-	for _, tc := range append(commonResolverTestCases, []resolverTestCase{
 		{
 			name:        "multi-tenant",
 			headerValue: strptr("tenant-a|tenant-b"),
@@ -129,8 +74,8 @@ func TestMultiResolver(t *testing.T) {
 		{
 			name:         "multi-tenant-with-relative-path",
 			headerValue:  strptr("tenant-a|tenant-b|.."),
-			errTenantID:  errInvalidTenantID,
-			errTenantIDs: errInvalidTenantID,
+			errTenantID:  errUnsafeTenantID,
+			errTenantIDs: errUnsafeTenantID,
 		},
 		{
 			name:         "containing-forward-slash",
@@ -144,7 +89,34 @@ func TestMultiResolver(t *testing.T) {
 			errTenantID:  &errTenantIDUnsupportedCharacter{pos: 8, tenantID: "backward\\slash"},
 			errTenantIDs: &errTenantIDUnsupportedCharacter{pos: 8, tenantID: "backward\\slash"},
 		},
-	}...) {
-		t.Run(tc.name, tc.test(r))
+		{
+			name:         "too-long",
+			headerValue:  strptr(strings.Repeat("123", MaxTenantIDLength)),
+			errTenantID:  errTenantIDTooLong,
+			errTenantIDs: errTenantIDTooLong,
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			ctx := context.Background()
+			if tc.headerValue != nil {
+				ctx = user.InjectOrgID(ctx, *tc.headerValue)
+			}
+
+			tenantID, err := TenantID(ctx)
+			if tc.errTenantID != nil {
+				assert.Equal(t, tc.errTenantID, err)
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, tc.tenantID, tenantID)
+			}
+
+			tenantIDs, err := TenantIDs(ctx)
+			if tc.errTenantIDs != nil {
+				assert.Equal(t, tc.errTenantIDs, err)
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, tc.tenantIDs, tenantIDs)
+			}
+		})
 	}
 }
diff --git a/tenant/tenant_test.go b/tenant/tenant_test.go
index b242fd7..bc3a60b 100644
--- a/tenant/tenant_test.go
+++ b/tenant/tenant_test.go
@@ -29,6 +29,14 @@ func TestValidTenantIDs(t *testing.T) {
 			name: strings.Repeat("a", 151),
 			err:  strptr("tenant ID is too long: max 150 characters"),
 		},
+		{
+			name: ".",
+			err:  strptr("tenant ID is '.' or '..'"),
+		},
+		{
+			name: "..",
+			err:  strptr("tenant ID is '.' or '..'"),
+		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			err := ValidTenantID(tc.name)