From cc94164b884d04bc21114c8e904314d1021f90ea Mon Sep 17 00:00:00 2001 From: Kevin Minehart Date: Tue, 26 Jan 2021 11:47:33 -0600 Subject: [PATCH 01/11] Move some utility functions out of `util` and into their own packages (#3734) * separate out some util packages Signed-off-by: Kevin Minehart * use goimports with -local Signed-off-by: Kevin Minehart * refactor: reduce the number of transitive imports when using math and logging functions Signed-off-by: Kevin Minehart * add deprecation warning to pkg/log/log.go Signed-off-by: Kevin Minehart --- pkg/util/log/experimental.go | 21 ++++++++++++++ pkg/util/log/log.go | 30 ++++++++++++++++++++ pkg/util/log/wrappers.go | 53 ++++++++++++++++++++++++++++++++++++ 3 files changed, 104 insertions(+) create mode 100644 pkg/util/log/experimental.go create mode 100644 pkg/util/log/log.go create mode 100644 pkg/util/log/wrappers.go diff --git a/pkg/util/log/experimental.go b/pkg/util/log/experimental.go new file mode 100644 index 000000000..3241af692 --- /dev/null +++ b/pkg/util/log/experimental.go @@ -0,0 +1,21 @@ +package log + +import ( + "github.com/go-kit/kit/log/level" + "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/client_golang/prometheus/promauto" +) + +var experimentalFeaturesInUse = promauto.NewCounter( + prometheus.CounterOpts{ + Namespace: "cortex", + Name: "experimental_features_in_use_total", + Help: "The number of experimental features in use.", + }, +) + +// WarnExperimentalUse logs a warning and increments the experimental features metric. +func WarnExperimentalUse(feature string) { + level.Warn(Logger).Log("msg", "experimental feature in use", "feature", feature) + experimentalFeaturesInUse.Inc() +} diff --git a/pkg/util/log/log.go b/pkg/util/log/log.go new file mode 100644 index 000000000..2f146db27 --- /dev/null +++ b/pkg/util/log/log.go @@ -0,0 +1,30 @@ +package log + +import ( + "fmt" + "os" + + "github.com/go-kit/kit/log" + kitlog "github.com/go-kit/kit/log" + "github.com/go-kit/kit/log/level" +) + +var ( + // Logger is a shared go-kit logger. + // TODO: Change all components to take a non-global logger via their constructors. + // Prefer accepting a non-global logger as an argument. + Logger = kitlog.NewNopLogger() +) + +// CheckFatal prints an error and exits with error code 1 if err is non-nil +func CheckFatal(location string, err error) { + if err != nil { + logger := level.Error(Logger) + if location != "" { + logger = log.With(logger, "msg", "error "+location) + } + // %+v gets the stack trace from errors using github.com/pkg/errors + logger.Log("err", fmt.Sprintf("%+v", err)) + os.Exit(1) + } +} diff --git a/pkg/util/log/wrappers.go b/pkg/util/log/wrappers.go new file mode 100644 index 000000000..9c3747272 --- /dev/null +++ b/pkg/util/log/wrappers.go @@ -0,0 +1,53 @@ +package log + +import ( + "context" + + "github.com/go-kit/kit/log" + kitlog "github.com/go-kit/kit/log" + "github.com/weaveworks/common/middleware" + + "github.com/cortexproject/cortex/pkg/tenant" +) + +// WithUserID returns a Logger that has information about the current user in +// its details. +func WithUserID(userID string, l kitlog.Logger) kitlog.Logger { + // See note in WithContext. + return kitlog.With(l, "org_id", userID) +} + +// WithTraceID returns a Logger that has information about the traceID in +// its details. +func WithTraceID(traceID string, l kitlog.Logger) kitlog.Logger { + // See note in WithContext. + return kitlog.With(l, "traceID", traceID) +} + +// WithContext returns a Logger that has information about the current user in +// its details. +// +// e.g. +// log := util.WithContext(ctx) +// log.Errorf("Could not chunk chunks: %v", err) +func WithContext(ctx context.Context, l kitlog.Logger) kitlog.Logger { + // Weaveworks uses "orgs" and "orgID" to represent Cortex users, + // even though the code-base generally uses `userID` to refer to the same thing. + userID, err := tenant.TenantID(ctx) + if err == nil { + l = WithUserID(userID, l) + } + + traceID, ok := middleware.ExtractTraceID(ctx) + if !ok { + return l + } + + return WithTraceID(traceID, l) +} + +// WithSourceIPs returns a Logger that has information about the source IPs in +// its details. +func WithSourceIPs(sourceIPs string, l log.Logger) log.Logger { + return log.With(l, "sourceIPs", sourceIPs) +} From 4795265e42785ce86a82c74a42690848cd0092b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20=C5=A0tibran=C3=BD?= Date: Wed, 3 Feb 2021 17:53:29 +0100 Subject: [PATCH 02/11] Remove util.Logger and move util.InitLogger to util/log package. (#3781) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Remove util.Logger and move util.InitLogger to util/log package. Signed-off-by: Peter Štibraný * Review feedback. Signed-off-by: Peter Štibraný --- pkg/util/log/log.go | 79 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/pkg/util/log/log.go b/pkg/util/log/log.go index 2f146db27..92ea3f697 100644 --- a/pkg/util/log/log.go +++ b/pkg/util/log/log.go @@ -7,6 +7,9 @@ import ( "github.com/go-kit/kit/log" kitlog "github.com/go-kit/kit/log" "github.com/go-kit/kit/log/level" + "github.com/prometheus/client_golang/prometheus" + "github.com/weaveworks/common/logging" + "github.com/weaveworks/common/server" ) var ( @@ -14,8 +17,84 @@ var ( // TODO: Change all components to take a non-global logger via their constructors. // Prefer accepting a non-global logger as an argument. Logger = kitlog.NewNopLogger() + + logMessages = prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "log_messages_total", + Help: "Total number of log messages.", + }, []string{"level"}) + + supportedLevels = []level.Value{ + level.DebugValue(), + level.InfoValue(), + level.WarnValue(), + level.ErrorValue(), + } ) +func init() { + prometheus.MustRegister(logMessages) +} + +// InitLogger initialises the global gokit logger (util_log.Logger) and overrides the +// default logger for the server. +func InitLogger(cfg *server.Config) { + l, err := NewPrometheusLogger(cfg.LogLevel, cfg.LogFormat) + if err != nil { + panic(err) + } + + // when use util_log.Logger, skip 3 stack frames. + Logger = log.With(l, "caller", log.Caller(3)) + + // cfg.Log wraps log function, skip 4 stack frames to get caller information. + // this works in go 1.12, but doesn't work in versions earlier. + // it will always shows the wrapper function generated by compiler + // marked in old versions. + cfg.Log = logging.GoKit(log.With(l, "caller", log.Caller(4))) +} + +// PrometheusLogger exposes Prometheus counters for each of go-kit's log levels. +type PrometheusLogger struct { + logger log.Logger +} + +// NewPrometheusLogger creates a new instance of PrometheusLogger which exposes +// Prometheus counters for various log levels. +func NewPrometheusLogger(l logging.Level, format logging.Format) (log.Logger, error) { + logger := log.NewLogfmtLogger(log.NewSyncWriter(os.Stderr)) + if format.String() == "json" { + logger = log.NewJSONLogger(log.NewSyncWriter(os.Stderr)) + } + logger = level.NewFilter(logger, l.Gokit) + + // Initialise counters for all supported levels: + for _, level := range supportedLevels { + logMessages.WithLabelValues(level.String()) + } + + logger = &PrometheusLogger{ + logger: logger, + } + + // return a Logger without caller information, shouldn't use directly + logger = log.With(logger, "ts", log.DefaultTimestampUTC) + return logger, nil +} + +// Log increments the appropriate Prometheus counter depending on the log level. +func (pl *PrometheusLogger) Log(kv ...interface{}) error { + pl.logger.Log(kv...) + l := "unknown" + for i := 1; i < len(kv); i += 2 { + if v, ok := kv[i].(level.Value); ok { + l = v.String() + break + } + } + logMessages.WithLabelValues(l).Inc() + return nil +} + // CheckFatal prints an error and exits with error code 1 if err is non-nil func CheckFatal(location string, err error) { if err != nil { From 7a30a895264f0bea6ac95f2f22b4ee035342b246 Mon Sep 17 00:00:00 2001 From: Arve Knudsen Date: Mon, 6 Sep 2021 17:25:52 +0200 Subject: [PATCH 03/11] Chore: Upgrade weaveworks/common (#4462) * Chore: Upgrade weaveworks/common Signed-off-by: Arve Knudsen * Add changelog entries Signed-off-by: Arve Knudsen * Use instrument.NewHistogramCollector Signed-off-by: Arve Knudsen * Use tracing.ExtractSampledTraceID Signed-off-by: Arve Knudsen --- pkg/util/log/wrappers.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/util/log/wrappers.go b/pkg/util/log/wrappers.go index 9c3747272..a76d87e32 100644 --- a/pkg/util/log/wrappers.go +++ b/pkg/util/log/wrappers.go @@ -5,7 +5,7 @@ import ( "github.com/go-kit/kit/log" kitlog "github.com/go-kit/kit/log" - "github.com/weaveworks/common/middleware" + "github.com/weaveworks/common/tracing" "github.com/cortexproject/cortex/pkg/tenant" ) @@ -38,7 +38,7 @@ func WithContext(ctx context.Context, l kitlog.Logger) kitlog.Logger { l = WithUserID(userID, l) } - traceID, ok := middleware.ExtractTraceID(ctx) + traceID, ok := tracing.ExtractSampledTraceID(ctx) if !ok { return l } From 3120328551090fe52696be98aefb2ef9c1735833 Mon Sep 17 00:00:00 2001 From: Christian Simon Date: Thu, 26 Nov 2020 13:57:16 +0000 Subject: [PATCH 04/11] Add tenant resolver (#3486) * Add tenant resolver package This implements the multi tenant resolver as described by the [proposal] for multi tenant query-federation. By default it behaves like before, but it's implementation can be swapped out. [proposal]: https://github.com/cortexproject/cortex/pull/3364 Signed-off-by: Christian Simon * Replace usages of `ExtractOrgID` Use TenantID or UserID depending on which of the methods are meant to be used. Signed-off-by: Christian Simon * Replace usages of `ExtractOrgIDFromHTTPRequest` This is replaced by ExtractTenantIDFromHTTPRequest, which makes sure that exactly one tenant ID is set. Signed-off-by: Christian Simon * Add methods to `tenant` package to use resolver directly Signed-off-by: Christian Simon * Remove UserID method from Resolver interface We need a better definition for what we are trying to achieve with UserID before we can add it to the interface Signed-off-by: Christian Simon * Update comment on the TenantID/TenantIDs Signed-off-by: Christian Simon * Improve performance of NormalizeTenantIDs - reduce allocations by reusing the input slice during de-duplication Signed-off-by: Christian Simon --- pkg/tenant/resolver.go | 132 ++++++++++++++++++++++++++++++++++++ pkg/tenant/resolver_test.go | 107 +++++++++++++++++++++++++++++ pkg/tenant/tenant.go | 89 ++++++++++++++++++++++++ pkg/tenant/tenant_test.go | 42 ++++++++++++ 4 files changed, 370 insertions(+) create mode 100644 pkg/tenant/resolver.go create mode 100644 pkg/tenant/resolver_test.go create mode 100644 pkg/tenant/tenant.go create mode 100644 pkg/tenant/tenant_test.go diff --git a/pkg/tenant/resolver.go b/pkg/tenant/resolver.go new file mode 100644 index 000000000..e5fbea252 --- /dev/null +++ b/pkg/tenant/resolver.go @@ -0,0 +1,132 @@ +package tenant + +import ( + "context" + "net/http" + "strings" + + "github.com/weaveworks/common/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 +// supplied or user.ErrTooManyOrgIDs if there are multiple tenant IDs present. +// +// ignore stutter warning +//nolint:golint +func TenantID(ctx context.Context) (string, error) { + return defaultResolver.TenantID(ctx) +} + +// TenantIDs returns all tenant IDs from the context. It should return +// normalized list of ordered and distinct tenant IDs (as produced by +// NormalizeTenantIDs). +// +// ignore stutter warning +//nolint:golint +func TenantIDs(ctx context.Context) ([]string, error) { + return defaultResolver.TenantIDs(ctx) +} + +type Resolver interface { + // 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 + // supplied or user.ErrTooManyOrgIDs if there are multiple tenant IDs present. + TenantID(context.Context) (string, error) + + // TenantIDs returns all tenant IDs from the context. It should return + // normalized list of ordered and distinct tenant IDs (as produced by + // NormalizeTenantIDs). + 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 { +} + +func (t *SingleResolver) TenantID(ctx context.Context) (string, error) { + //lint:ignore faillint wrapper around upstream method + return user.ExtractOrgID(ctx) +} + +func (t *SingleResolver) TenantIDs(ctx context.Context) ([]string, error) { + //lint:ignore faillint wrapper around upstream method + orgID, err := user.ExtractOrgID(ctx) + if err != nil { + return nil, err + } + return []string{orgID}, err +} + +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) +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 +} + +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 + } + } + + 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 +} diff --git a/pkg/tenant/resolver_test.go b/pkg/tenant/resolver_test.go new file mode 100644 index 000000000..69559263b --- /dev/null +++ b/pkg/tenant/resolver_test.go @@ -0,0 +1,107 @@ +package tenant + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/weaveworks/common/user" +) + +func strptr(s string) *string { + return &s +} + +type resolverTestCase struct { + name string + headerValue *string + errTenantID error + errTenantIDs error + tenantID string + 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"}, + }, +} + +func TestSingleResolver(t *testing.T) { + r := NewSingleResolver() + for _, tc := range append(commonResolverTestCases, []resolverTestCase{ + { + name: "multi-tenant", + headerValue: strptr("tenant-a|tenant-b"), + tenantID: "tenant-a|tenant-b", + tenantIDs: []string{"tenant-a|tenant-b"}, + }, + }...) { + 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"), + errTenantID: user.ErrTooManyOrgIDs, + tenantIDs: []string{"tenant-a", "tenant-b"}, + }, + { + name: "multi-tenant-wrong-order", + headerValue: strptr("tenant-b|tenant-a"), + errTenantID: user.ErrTooManyOrgIDs, + tenantIDs: []string{"tenant-a", "tenant-b"}, + }, + { + name: "multi-tenant-duplicate-order", + headerValue: strptr("tenant-b|tenant-b|tenant-a"), + errTenantID: user.ErrTooManyOrgIDs, + tenantIDs: []string{"tenant-a", "tenant-b"}, + }, + }...) { + t.Run(tc.name, tc.test(r)) + } +} diff --git a/pkg/tenant/tenant.go b/pkg/tenant/tenant.go new file mode 100644 index 000000000..102091c78 --- /dev/null +++ b/pkg/tenant/tenant.go @@ -0,0 +1,89 @@ +package tenant + +import ( + "errors" + "fmt" + "sort" +) + +var ( + errTenantIDTooLong = errors.New("tenant ID is too long: max 150 characters") +) + +type errTenantIDUnsupportedCharacter struct { + pos int + tenantID string +} + +func (e *errTenantIDUnsupportedCharacter) Error() string { + return fmt.Sprintf( + "tenant ID '%s' contains unsupported character '%c'", + e.tenantID, + e.tenantID[e.pos], + ) +} + +const tenantIDsLabelSeparator = "|" + +// NormalizeTenantIDs is creating a normalized form by sortiing and de-duplicating the list of tenantIDs +func NormalizeTenantIDs(tenantIDs []string) []string { + sort.Strings(tenantIDs) + + count := len(tenantIDs) + if count <= 1 { + return tenantIDs + } + + posOut := 1 + for posIn := 1; posIn < count; posIn++ { + if tenantIDs[posIn] != tenantIDs[posIn-1] { + tenantIDs[posOut] = tenantIDs[posIn] + posOut++ + } + } + + return tenantIDs[0:posOut] +} + +// ValidTenantID +func ValidTenantID(s string) error { + // check if it contains invalid runes + for pos, r := range s { + if !isSupported(r) { + return &errTenantIDUnsupportedCharacter{ + tenantID: s, + pos: pos, + } + } + } + + if len(s) > 150 { + return errTenantIDTooLong + } + + return nil +} + +// this checks if a rune is supported in tenant IDs (according to +// https://cortexmetrics.io/docs/guides/limitations/#tenant-id-naming) +func isSupported(c rune) bool { + // characters + if ('a' <= c && c <= 'z') || ('A' <= c && c <= 'Z') { + return true + } + + // digits + if '0' <= c && c <= '9' { + return true + } + + // special + return c == '!' || + c == '-' || + c == '_' || + c == '.' || + c == '*' || + c == '\'' || + c == '(' || + c == ')' +} diff --git a/pkg/tenant/tenant_test.go b/pkg/tenant/tenant_test.go new file mode 100644 index 000000000..b242fd77f --- /dev/null +++ b/pkg/tenant/tenant_test.go @@ -0,0 +1,42 @@ +package tenant + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestValidTenantIDs(t *testing.T) { + for _, tc := range []struct { + name string + err *string + }{ + { + name: "tenant-a", + }, + { + name: "ABCDEFGHIJKLMNOPQRSTUVWXYZ-abcdefghijklmnopqrstuvwxyz_0987654321!.*'()", + }, + { + name: "invalid|", + err: strptr("tenant ID 'invalid|' contains unsupported character '|'"), + }, + { + name: strings.Repeat("a", 150), + }, + { + name: strings.Repeat("a", 151), + err: strptr("tenant ID is too long: max 150 characters"), + }, + } { + t.Run(tc.name, func(t *testing.T) { + err := ValidTenantID(tc.name) + if tc.err == nil { + assert.Nil(t, err) + } else { + assert.EqualError(t, err, *tc.err) + } + }) + } +} From 615a344cec067e22b350a6084849fd6f7204a912 Mon Sep 17 00:00:00 2001 From: Christian Simon Date: Wed, 23 Dec 2020 15:33:36 +0000 Subject: [PATCH 05/11] Add multi tenant query federation (#3250) * Add tenant query federation This experimental feature allows queries to cover data from more than a single Cortex tenant and can be activated by supplying `-tenant-federation.enabled` to all cortex instances. To query multiple tenants a `|` separated tenant list can be specified in the `X-Scope-OrgID` header. The source tenant of a metric will be exposed in the label `__tenant_id__`. Signed-off-by: Christian Simon * Aggregate the limit of maxQueriers correctly This ensures the limit is aggregated correctly of the setting of each individual tenant. It also implements the logic for the v2 query frontend. Signed-off-by: Christian Simon * Fix tenant labels and make LabelNames more efficient Signed-off-by: Christian Simon * Use tsdb_errors for error handling Signed-off-by: Christian Simon * Fix uninitialized label matcher Regexp matcher need to be initialized, this adds a test for regexp matcher and fixes the underlying issue. Signed-off-by: Christian Simon * Use map for filterValuesByMatchers to reduce allocations Signed-off-by: Christian Simon * Address review suggestions Signed-off-by: Christian Simon * Add validation.SmallestPositiveNonZeroIntPerTenant to avoid code duplication Signed-off-by: Christian Simon * Add limitations and experimental status to docs Signed-off-by: Christian Simon * Remove unnecessary cast of defaultTenantLabel Signed-off-by: Christian Simon * Handle query-range limits for multi tenant queries Signed-off-by: Christian Simon * Skip results cache, if multi tenants query Signed-off-by: Christian Simon * Add failint to ensure query path supports multiple tenants To avoid any future regressions in the multi tenant support within the query path, a faillint command tests if TenantID is used and if it finds one, it suggestest using TenantIDs instead> Signed-off-by: Christian Simon * Align CHANGELOG line with the flag description Signed-off-by: Christian Simon * Add a limitation about missing results cache Signed-off-by: Christian Simon --- pkg/tenant/tenant.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/pkg/tenant/tenant.go b/pkg/tenant/tenant.go index 102091c78..fa8089890 100644 --- a/pkg/tenant/tenant.go +++ b/pkg/tenant/tenant.go @@ -1,9 +1,13 @@ package tenant import ( + "context" "errors" "fmt" "sort" + "strings" + + "github.com/weaveworks/common/user" ) var ( @@ -64,6 +68,10 @@ func ValidTenantID(s string) error { return nil } +func JoinTenantIDs(tenantIDs []string) string { + return strings.Join(tenantIDs, tenantIDsLabelSeparator) +} + // this checks if a rune is supported in tenant IDs (according to // https://cortexmetrics.io/docs/guides/limitations/#tenant-id-naming) func isSupported(c rune) bool { @@ -87,3 +95,11 @@ func isSupported(c rune) bool { c == '(' || c == ')' } + +// TenantIDsFromOrgID extracts different tenants from an orgID string value +// +// ignore stutter warning +//nolint:golint +func TenantIDsFromOrgID(orgID string) ([]string, error) { + return TenantIDs(user.InjectOrgID(context.TODO(), orgID)) +} From d1ef33a3d65efcc78c1a6940e3c70871e34ad774 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Wed, 21 Jul 2021 15:13:02 +0100 Subject: [PATCH 06/11] Restrict path segments in TenantIDs (#4375) (#4376) This prevents paths generated from TenantIDs to become vulnerable to path traversal attacks. CVE-2021-36157 Signed-off-by: Christian Simon --- pkg/tenant/resolver.go | 32 +++++++++++++++++++++++++--- pkg/tenant/resolver_test.go | 42 +++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 3 deletions(-) diff --git a/pkg/tenant/resolver.go b/pkg/tenant/resolver.go index e5fbea252..72517b082 100644 --- a/pkg/tenant/resolver.go +++ b/pkg/tenant/resolver.go @@ -2,6 +2,7 @@ package tenant import ( "context" + "errors" "net/http" "strings" @@ -59,14 +60,36 @@ func NewSingleResolver() *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 - return user.ExtractOrgID(ctx) + 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) { - //lint:ignore faillint wrapper around upstream method - orgID, err := user.ExtractOrgID(ctx) + orgID, err := t.TenantID(ctx) if err != nil { return nil, err } @@ -109,6 +132,9 @@ func (t *MultiResolver) TenantIDs(ctx context.Context) ([]string, error) { if err := ValidTenantID(orgID); err != nil { return nil, err } + if containsUnsafePathSegments(orgID) { + return nil, errInvalidTenantID + } } return NormalizeTenantIDs(orgIDs), nil diff --git a/pkg/tenant/resolver_test.go b/pkg/tenant/resolver_test.go index 69559263b..4d2da2416 100644 --- a/pkg/tenant/resolver_test.go +++ b/pkg/tenant/resolver_test.go @@ -64,6 +64,18 @@ var commonResolverTestCases = []resolverTestCase{ 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) { @@ -75,6 +87,18 @@ func TestSingleResolver(t *testing.T) { tenantID: "tenant-a|tenant-b", tenantIDs: []string{"tenant-a|tenant-b"}, }, + { + name: "containing-forward-slash", + headerValue: strptr("forward/slash"), + errTenantID: errInvalidTenantID, + errTenantIDs: errInvalidTenantID, + }, + { + name: "containing-backward-slash", + headerValue: strptr(`backward\slash`), + errTenantID: errInvalidTenantID, + errTenantIDs: errInvalidTenantID, + }, }...) { t.Run(tc.name, tc.test(r)) } @@ -101,6 +125,24 @@ func TestMultiResolver(t *testing.T) { errTenantID: user.ErrTooManyOrgIDs, tenantIDs: []string{"tenant-a", "tenant-b"}, }, + { + name: "multi-tenant-with-relative-path", + headerValue: strptr("tenant-a|tenant-b|.."), + errTenantID: errInvalidTenantID, + errTenantIDs: errInvalidTenantID, + }, + { + name: "containing-forward-slash", + headerValue: strptr("forward/slash"), + errTenantID: &errTenantIDUnsupportedCharacter{pos: 7, tenantID: "forward/slash"}, + errTenantIDs: &errTenantIDUnsupportedCharacter{pos: 7, tenantID: "forward/slash"}, + }, + { + name: "containing-backward-slash", + headerValue: strptr(`backward\slash`), + errTenantID: &errTenantIDUnsupportedCharacter{pos: 8, tenantID: "backward\\slash"}, + errTenantIDs: &errTenantIDUnsupportedCharacter{pos: 8, tenantID: "backward\\slash"}, + }, }...) { t.Run(tc.name, tc.test(r)) } From dfe7611e8654b0b09e90fa81e09beab21955c6a2 Mon Sep 17 00:00:00 2001 From: Arve Knudsen Date: Thu, 9 Sep 2021 17:09:45 +0200 Subject: [PATCH 07/11] Move log and tenant packages to root Signed-off-by: Arve Knudsen --- {pkg/util/log => dslog}/experimental.go | 0 {pkg/util/log => dslog}/log.go | 0 {pkg/util/log => dslog}/wrappers.go | 0 {pkg/tenant => tenant}/resolver.go | 0 {pkg/tenant => tenant}/resolver_test.go | 0 {pkg/tenant => tenant}/tenant.go | 0 {pkg/tenant => tenant}/tenant_test.go | 0 7 files changed, 0 insertions(+), 0 deletions(-) rename {pkg/util/log => dslog}/experimental.go (100%) rename {pkg/util/log => dslog}/log.go (100%) rename {pkg/util/log => dslog}/wrappers.go (100%) rename {pkg/tenant => tenant}/resolver.go (100%) rename {pkg/tenant => tenant}/resolver_test.go (100%) rename {pkg/tenant => tenant}/tenant.go (100%) rename {pkg/tenant => tenant}/tenant_test.go (100%) diff --git a/pkg/util/log/experimental.go b/dslog/experimental.go similarity index 100% rename from pkg/util/log/experimental.go rename to dslog/experimental.go diff --git a/pkg/util/log/log.go b/dslog/log.go similarity index 100% rename from pkg/util/log/log.go rename to dslog/log.go diff --git a/pkg/util/log/wrappers.go b/dslog/wrappers.go similarity index 100% rename from pkg/util/log/wrappers.go rename to dslog/wrappers.go diff --git a/pkg/tenant/resolver.go b/tenant/resolver.go similarity index 100% rename from pkg/tenant/resolver.go rename to tenant/resolver.go diff --git a/pkg/tenant/resolver_test.go b/tenant/resolver_test.go similarity index 100% rename from pkg/tenant/resolver_test.go rename to tenant/resolver_test.go diff --git a/pkg/tenant/tenant.go b/tenant/tenant.go similarity index 100% rename from pkg/tenant/tenant.go rename to tenant/tenant.go diff --git a/pkg/tenant/tenant_test.go b/tenant/tenant_test.go similarity index 100% rename from pkg/tenant/tenant_test.go rename to tenant/tenant_test.go From b71e41c8d2361962d3111d346b086f1a6ac79206 Mon Sep 17 00:00:00 2001 From: Arve Knudsen Date: Thu, 9 Sep 2021 17:35:16 +0200 Subject: [PATCH 08/11] Remove globals Signed-off-by: Arve Knudsen --- dslog/experimental.go | 21 ---------- dslog/log.go | 90 ++++++++++++++++++------------------------- dslog/wrappers.go | 17 ++++---- go.mod | 2 +- go.sum | 4 +- tenant/resolver.go | 14 ++----- tenant/tenant.go | 9 ++--- 7 files changed, 56 insertions(+), 101 deletions(-) delete mode 100644 dslog/experimental.go diff --git a/dslog/experimental.go b/dslog/experimental.go deleted file mode 100644 index 3241af692..000000000 --- a/dslog/experimental.go +++ /dev/null @@ -1,21 +0,0 @@ -package log - -import ( - "github.com/go-kit/kit/log/level" - "github.com/prometheus/client_golang/prometheus" - "github.com/prometheus/client_golang/prometheus/promauto" -) - -var experimentalFeaturesInUse = promauto.NewCounter( - prometheus.CounterOpts{ - Namespace: "cortex", - Name: "experimental_features_in_use_total", - Help: "The number of experimental features in use.", - }, -) - -// WarnExperimentalUse logs a warning and increments the experimental features metric. -func WarnExperimentalUse(feature string) { - level.Warn(Logger).Log("msg", "experimental feature in use", "feature", feature) - experimentalFeaturesInUse.Inc() -} diff --git a/dslog/log.go b/dslog/log.go index 92ea3f697..b16ce4de9 100644 --- a/dslog/log.go +++ b/dslog/log.go @@ -1,84 +1,62 @@ -package log +package dslog import ( "fmt" "os" "github.com/go-kit/kit/log" - kitlog "github.com/go-kit/kit/log" "github.com/go-kit/kit/log/level" "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/client_golang/prometheus/promauto" "github.com/weaveworks/common/logging" - "github.com/weaveworks/common/server" ) -var ( - // Logger is a shared go-kit logger. - // TODO: Change all components to take a non-global logger via their constructors. - // Prefer accepting a non-global logger as an argument. - Logger = kitlog.NewNopLogger() - - logMessages = prometheus.NewCounterVec(prometheus.CounterOpts{ - Name: "log_messages_total", - Help: "Total number of log messages.", - }, []string{"level"}) - - supportedLevels = []level.Value{ - level.DebugValue(), - level.InfoValue(), - level.WarnValue(), - level.ErrorValue(), - } -) - -func init() { - prometheus.MustRegister(logMessages) -} - -// InitLogger initialises the global gokit logger (util_log.Logger) and overrides the -// default logger for the server. -func InitLogger(cfg *server.Config) { - l, err := NewPrometheusLogger(cfg.LogLevel, cfg.LogFormat) - if err != nil { - panic(err) - } - - // when use util_log.Logger, skip 3 stack frames. - Logger = log.With(l, "caller", log.Caller(3)) - - // cfg.Log wraps log function, skip 4 stack frames to get caller information. - // this works in go 1.12, but doesn't work in versions earlier. - // it will always shows the wrapper function generated by compiler - // marked in old versions. - cfg.Log = logging.GoKit(log.With(l, "caller", log.Caller(4))) -} - // PrometheusLogger exposes Prometheus counters for each of go-kit's log levels. type PrometheusLogger struct { - logger log.Logger + logger log.Logger + logMessages *prometheus.CounterVec + experimentalFeaturesInUse prometheus.Counter } // NewPrometheusLogger creates a new instance of PrometheusLogger which exposes // Prometheus counters for various log levels. -func NewPrometheusLogger(l logging.Level, format logging.Format) (log.Logger, error) { +func NewPrometheusLogger(l logging.Level, format logging.Format, reg prometheus.Registerer, metricsNamespace string) log.Logger { logger := log.NewLogfmtLogger(log.NewSyncWriter(os.Stderr)) if format.String() == "json" { logger = log.NewJSONLogger(log.NewSyncWriter(os.Stderr)) } logger = level.NewFilter(logger, l.Gokit) + logMessages := promauto.With(reg).NewCounterVec(prometheus.CounterOpts{ + Name: "log_messages_total", + Help: "Total number of log messages.", + }, []string{"level"}) // Initialise counters for all supported levels: + supportedLevels := []level.Value{ + level.DebugValue(), + level.InfoValue(), + level.WarnValue(), + level.ErrorValue(), + } for _, level := range supportedLevels { logMessages.WithLabelValues(level.String()) } logger = &PrometheusLogger{ - logger: logger, + logger: logger, + logMessages: logMessages, + experimentalFeaturesInUse: promauto.With(reg).NewCounter( + prometheus.CounterOpts{ + Namespace: metricsNamespace, + Name: "experimental_features_in_use_total", + Help: "The number of experimental features in use.", + }, + ), } // return a Logger without caller information, shouldn't use directly logger = log.With(logger, "ts", log.DefaultTimestampUTC) - return logger, nil + return logger } // Log increments the appropriate Prometheus counter depending on the log level. @@ -91,14 +69,14 @@ func (pl *PrometheusLogger) Log(kv ...interface{}) error { break } } - logMessages.WithLabelValues(l).Inc() + pl.logMessages.WithLabelValues(l).Inc() return nil } -// CheckFatal prints an error and exits with error code 1 if err is non-nil -func CheckFatal(location string, err error) { +// CheckFatal prints an error and exits with error code 1 if err is non-nil. +func CheckFatal(location string, err error, logger log.Logger) { if err != nil { - logger := level.Error(Logger) + logger := level.Error(logger) if location != "" { logger = log.With(logger, "msg", "error "+location) } @@ -107,3 +85,11 @@ func CheckFatal(location string, err error) { os.Exit(1) } } + +// WarnExperimentalUse logs a warning and increments the experimental features metric. +func WarnExperimentalUse(feature string, logger log.Logger) { + level.Warn(logger).Log("msg", "experimental feature in use", "feature", feature) + if pl, ok := logger.(*PrometheusLogger); ok { + pl.experimentalFeaturesInUse.Inc() + } +} diff --git a/dslog/wrappers.go b/dslog/wrappers.go index a76d87e32..ce4db69fd 100644 --- a/dslog/wrappers.go +++ b/dslog/wrappers.go @@ -1,27 +1,26 @@ -package log +package dslog import ( "context" "github.com/go-kit/kit/log" - kitlog "github.com/go-kit/kit/log" "github.com/weaveworks/common/tracing" - "github.com/cortexproject/cortex/pkg/tenant" + "github.com/grafana/dskit/tenant" ) // WithUserID returns a Logger that has information about the current user in // its details. -func WithUserID(userID string, l kitlog.Logger) kitlog.Logger { +func WithUserID(userID string, l log.Logger) log.Logger { // See note in WithContext. - return kitlog.With(l, "org_id", userID) + return log.With(l, "org_id", userID) } // WithTraceID returns a Logger that has information about the traceID in // its details. -func WithTraceID(traceID string, l kitlog.Logger) kitlog.Logger { +func WithTraceID(traceID string, l log.Logger) log.Logger { // See note in WithContext. - return kitlog.With(l, "traceID", traceID) + return log.With(l, "traceID", traceID) } // WithContext returns a Logger that has information about the current user in @@ -30,10 +29,10 @@ func WithTraceID(traceID string, l kitlog.Logger) kitlog.Logger { // e.g. // log := util.WithContext(ctx) // log.Errorf("Could not chunk chunks: %v", err) -func WithContext(ctx context.Context, l kitlog.Logger) kitlog.Logger { +func WithContext(ctx context.Context, l log.Logger) log.Logger { // Weaveworks uses "orgs" and "orgID" to represent Cortex users, // even though the code-base generally uses `userID` to refer to the same thing. - userID, err := tenant.TenantID(ctx) + userID, err := tenant.ID(ctx) if err == nil { l = WithUserID(userID, l) } diff --git a/go.mod b/go.mod index 2ad4f5e18..3998bd2c0 100644 --- a/go.mod +++ b/go.mod @@ -22,7 +22,7 @@ require ( github.com/prometheus/common v0.26.0 github.com/sercand/kuberesolver v2.4.0+incompatible // indirect github.com/stretchr/testify v1.7.0 - github.com/weaveworks/common v0.0.0-20210722103813-e649eff5ab4a + github.com/weaveworks/common v0.0.0-20210901124008-1fa3f9fa874c go.etcd.io/etcd v3.3.25+incompatible go.etcd.io/etcd/client/v3 v3.5.0 go.etcd.io/etcd/server/v3 v3.5.0 diff --git a/go.sum b/go.sum index 6f0689440..15799db82 100644 --- a/go.sum +++ b/go.sum @@ -608,8 +608,8 @@ github.com/uber/jaeger-lib v2.2.0+incompatible h1:MxZXOiR2JuoANZ3J6DE/U0kSFv/eJ/ github.com/uber/jaeger-lib v2.2.0+incompatible/go.mod h1:ComeNDZlWwrWnDv8aPp0Ba6+uUTzImX/AauajbLI56U= github.com/urfave/cli v1.20.0/go.mod h1:70zkFmudgCuE/ngEzBv17Jvp/497gISqfk5gWijbERA= github.com/urfave/cli v1.22.1/go.mod h1:Gos4lmkARVdJ6EkW0WaNv/tZAAMe9V7XWyB60NtXRu0= -github.com/weaveworks/common v0.0.0-20210722103813-e649eff5ab4a h1:ALomSnvy/NPeVoc4a1o7keaHHgLS76r9ZYIlwWWF+KA= -github.com/weaveworks/common v0.0.0-20210722103813-e649eff5ab4a/go.mod h1:YU9FvnS7kUnRt6HY10G+2qHkwzP3n3Vb1XsXDsJTSp8= +github.com/weaveworks/common v0.0.0-20210901124008-1fa3f9fa874c h1:+yzwVr4/12cUgsdjbEHq6MsKB7jWBZpZccAP6xvqTzQ= +github.com/weaveworks/common v0.0.0-20210901124008-1fa3f9fa874c/go.mod h1:YU9FvnS7kUnRt6HY10G+2qHkwzP3n3Vb1XsXDsJTSp8= github.com/weaveworks/promrus v1.2.0 h1:jOLf6pe6/vss4qGHjXmGz4oDJQA+AOCqEL3FvvZGz7M= github.com/weaveworks/promrus v1.2.0/go.mod h1:SaE82+OJ91yqjrE1rsvBWVzNZKcHYFtMUyS1+Ogs/KA= github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2 h1:eY9dn8+vbi4tKz5Qo6v2eYzo7kUS51QINcR5jNpbZS8= diff --git a/tenant/resolver.go b/tenant/resolver.go index 72517b082..824427a6f 100644 --- a/tenant/resolver.go +++ b/tenant/resolver.go @@ -16,24 +16,18 @@ func WithDefaultResolver(r Resolver) { defaultResolver = r } -// TenantID returns exactly a single tenant ID from the context. It should be +// ID 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 // supplied or user.ErrTooManyOrgIDs if there are multiple tenant IDs present. -// -// ignore stutter warning -//nolint:golint -func TenantID(ctx context.Context) (string, error) { +func ID(ctx context.Context) (string, error) { return defaultResolver.TenantID(ctx) } -// TenantIDs returns all tenant IDs from the context. It should return +// IDs returns all tenant IDs from the context. It should return // normalized list of ordered and distinct tenant IDs (as produced by // NormalizeTenantIDs). -// -// ignore stutter warning -//nolint:golint -func TenantIDs(ctx context.Context) ([]string, error) { +func IDs(ctx context.Context) ([]string, error) { return defaultResolver.TenantIDs(ctx) } diff --git a/tenant/tenant.go b/tenant/tenant.go index fa8089890..39fee6b3d 100644 --- a/tenant/tenant.go +++ b/tenant/tenant.go @@ -96,10 +96,7 @@ func isSupported(c rune) bool { c == ')' } -// TenantIDsFromOrgID extracts different tenants from an orgID string value -// -// ignore stutter warning -//nolint:golint -func TenantIDsFromOrgID(orgID string) ([]string, error) { - return TenantIDs(user.InjectOrgID(context.TODO(), orgID)) +// IDsFromOrgID extracts different tenants from an orgID string value +func IDsFromOrgID(orgID string) ([]string, error) { + return IDs(user.InjectOrgID(context.TODO(), orgID)) } From c351ce33f3300ec79b5d59874ff756a719abd6aa Mon Sep 17 00:00:00 2001 From: Arve Knudsen Date: Thu, 9 Sep 2021 17:37:46 +0200 Subject: [PATCH 09/11] Update changelog Signed-off-by: Arve Knudsen --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c0b0a4ca3..2d40130c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,4 +2,6 @@ * [CHANGE] Removed global metrics for KV package. Making a KV object will now require a prometheus registerer that will be used to register all relevant KV class metrics. #22 -* [CHANGE] Added CHANGELOG.md and Pull Request template to reference the changelog \ No newline at end of file +* [CHANGE] Added CHANGELOG.md and Pull Request template to reference the changelog +* [ENHANCEMENT] Add `dslog` package # +* [ENHANCEMENT] Add `tenant` package # From a4de4002a6b3787302f502c6a03bc5978d1a8a60 Mon Sep 17 00:00:00 2001 From: Arve Knudsen Date: Fri, 10 Sep 2021 08:29:22 +0200 Subject: [PATCH 10/11] Fix changelog Signed-off-by: Arve Knudsen --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d40130c5..301d4c150 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,5 +3,5 @@ * [CHANGE] Removed global metrics for KV package. Making a KV object will now require a prometheus registerer that will be used to register all relevant KV class metrics. #22 * [CHANGE] Added CHANGELOG.md and Pull Request template to reference the changelog -* [ENHANCEMENT] Add `dslog` package # -* [ENHANCEMENT] Add `tenant` package # +* [ENHANCEMENT] Add `dslog` package #33 +* [ENHANCEMENT] Add `tenant` package #33 From 893e0f00c6b0a1b225889a2f05171ecf88dc0613 Mon Sep 17 00:00:00 2001 From: Arve Knudsen Date: Wed, 15 Sep 2021 10:34:20 +0200 Subject: [PATCH 11/11] Reduce tenant API Signed-off-by: Arve Knudsen --- tenant/resolver.go | 114 ++++++++++++++++++---------------------- tenant/resolver_test.go | 55 ++++++++----------- tenant/tenant.go | 102 ----------------------------------- tenant/tenant_test.go | 42 --------------- 4 files changed, 73 insertions(+), 240 deletions(-) delete mode 100644 tenant/tenant.go delete mode 100644 tenant/tenant_test.go diff --git a/tenant/resolver.go b/tenant/resolver.go index 824427a6f..e2e9016c6 100644 --- a/tenant/resolver.go +++ b/tenant/resolver.go @@ -3,18 +3,17 @@ package tenant import ( "context" "errors" - "net/http" + "fmt" "strings" "github.com/weaveworks/common/user" ) -var defaultResolver Resolver = NewSingleResolver() +var ( + errTenantIDTooLong = errors.New("tenant ID is too long: max 150 characters") +) -// WithDefaultResolver updates the resolver used for the package methods. -func WithDefaultResolver(r Resolver) { - defaultResolver = r -} +var defaultResolver Resolver = &singleResolver{} // ID returns exactly a single tenant ID from the context. It should be // used when a certain endpoint should only support exactly a single @@ -24,9 +23,9 @@ func ID(ctx context.Context) (string, error) { return defaultResolver.TenantID(ctx) } -// IDs returns all tenant IDs from the context. It should return +// IDs returns all tenant IDs from the context. It should return a // normalized list of ordered and distinct tenant IDs (as produced by -// NormalizeTenantIDs). +// normalizeTenantIDs). func IDs(ctx context.Context) ([]string, error) { return defaultResolver.TenantIDs(ctx) } @@ -40,18 +39,11 @@ type Resolver interface { // TenantIDs returns all tenant IDs from the context. It should return // normalized list of ordered and distinct tenant IDs (as produced by - // NormalizeTenantIDs). + // normalizeTenantIDs). 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 { +type singleResolver struct { } // containsUnsafePathSegments will return true if the string is a directory @@ -68,7 +60,7 @@ func containsUnsafePathSegments(id string) bool { var errInvalidTenantID = errors.New("invalid tenant ID") -func (t *SingleResolver) TenantID(ctx context.Context) (string, error) { +func (t *singleResolver) TenantID(ctx context.Context) (string, error) { //lint:ignore faillint wrapper around upstream method id, err := user.ExtractOrgID(ctx) if err != nil { @@ -82,7 +74,7 @@ func (t *SingleResolver) TenantID(ctx context.Context) (string, error) { return id, nil } -func (t *SingleResolver) TenantIDs(ctx context.Context) ([]string, error) { +func (t *singleResolver) TenantIDs(ctx context.Context) ([]string, error) { orgID, err := t.TenantID(ctx) if err != nil { return nil, err @@ -90,63 +82,57 @@ func (t *SingleResolver) TenantIDs(ctx context.Context) ([]string, error) { return []string{orgID}, err } -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) -func NewMultiResolver() *MultiResolver { - return &MultiResolver{} +type errTenantIDUnsupportedCharacter struct { + pos int + tenantID string } -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 +func (e *errTenantIDUnsupportedCharacter) Error() string { + return fmt.Sprintf( + "tenant ID '%s' contains unsupported character '%c'", + e.tenantID, + e.tenantID[e.pos], + ) } -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 +func validTenantID(s string) error { + // check if it contains invalid runes + for pos, r := range s { + if !isSupported(r) { + return &errTenantIDUnsupportedCharacter{ + tenantID: s, + pos: pos, + } + } } - orgIDs := strings.Split(orgID, tenantIDsLabelSeparator) - for _, orgID := range orgIDs { - if err := ValidTenantID(orgID); err != nil { - return nil, err - } - if containsUnsafePathSegments(orgID) { - return nil, errInvalidTenantID - } + if len(s) > 150 { + return errTenantIDTooLong } - return NormalizeTenantIDs(orgIDs), nil + return 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 +// this checks if a rune is supported in tenant IDs (according to +// https://cortexmetrics.io/docs/guides/limitations/#tenant-id-naming) +func isSupported(c rune) bool { + // characters + if ('a' <= c && c <= 'z') || ('A' <= c && c <= 'Z') { + return true } - tenantID, err := defaultResolver.TenantID(ctx) - if err != nil { - return "", nil, err + // digits + if '0' <= c && c <= '9' { + return true } - return tenantID, ctx, nil + // special + return c == '!' || + c == '-' || + c == '_' || + c == '.' || + c == '*' || + c == '\'' || + c == '(' || + c == ')' } diff --git a/tenant/resolver_test.go b/tenant/resolver_test.go index 4d2da2416..8870c94b8 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" @@ -79,7 +80,7 @@ var commonResolverTestCases = []resolverTestCase{ } func TestSingleResolver(t *testing.T) { - r := NewSingleResolver() + r := &singleResolver{} for _, tc := range append(commonResolverTestCases, []resolverTestCase{ { name: "multi-tenant", @@ -104,46 +105,36 @@ func TestSingleResolver(t *testing.T) { } } -func TestMultiResolver(t *testing.T) { - r := NewMultiResolver() - for _, tc := range append(commonResolverTestCases, []resolverTestCase{ +func TestValidTenantID(t *testing.T) { + for _, tc := range []struct { + name string + err *string + }{ { - name: "multi-tenant", - headerValue: strptr("tenant-a|tenant-b"), - errTenantID: user.ErrTooManyOrgIDs, - tenantIDs: []string{"tenant-a", "tenant-b"}, + name: "tenant-a", }, { - name: "multi-tenant-wrong-order", - headerValue: strptr("tenant-b|tenant-a"), - errTenantID: user.ErrTooManyOrgIDs, - tenantIDs: []string{"tenant-a", "tenant-b"}, + name: "ABCDEFGHIJKLMNOPQRSTUVWXYZ-abcdefghijklmnopqrstuvwxyz_0987654321!.*'()", }, { - name: "multi-tenant-duplicate-order", - headerValue: strptr("tenant-b|tenant-b|tenant-a"), - errTenantID: user.ErrTooManyOrgIDs, - tenantIDs: []string{"tenant-a", "tenant-b"}, + name: "invalid|", + err: strptr("tenant ID 'invalid|' contains unsupported character '|'"), }, { - name: "multi-tenant-with-relative-path", - headerValue: strptr("tenant-a|tenant-b|.."), - errTenantID: errInvalidTenantID, - errTenantIDs: errInvalidTenantID, + name: strings.Repeat("a", 150), }, { - name: "containing-forward-slash", - headerValue: strptr("forward/slash"), - errTenantID: &errTenantIDUnsupportedCharacter{pos: 7, tenantID: "forward/slash"}, - errTenantIDs: &errTenantIDUnsupportedCharacter{pos: 7, tenantID: "forward/slash"}, + name: strings.Repeat("a", 151), + err: strptr("tenant ID is too long: max 150 characters"), }, - { - name: "containing-backward-slash", - headerValue: strptr(`backward\slash`), - errTenantID: &errTenantIDUnsupportedCharacter{pos: 8, tenantID: "backward\\slash"}, - errTenantIDs: &errTenantIDUnsupportedCharacter{pos: 8, tenantID: "backward\\slash"}, - }, - }...) { - t.Run(tc.name, tc.test(r)) + } { + t.Run(tc.name, func(t *testing.T) { + err := validTenantID(tc.name) + if tc.err == nil { + assert.Nil(t, err) + } else { + assert.EqualError(t, err, *tc.err) + } + }) } } diff --git a/tenant/tenant.go b/tenant/tenant.go deleted file mode 100644 index 39fee6b3d..000000000 --- a/tenant/tenant.go +++ /dev/null @@ -1,102 +0,0 @@ -package tenant - -import ( - "context" - "errors" - "fmt" - "sort" - "strings" - - "github.com/weaveworks/common/user" -) - -var ( - errTenantIDTooLong = errors.New("tenant ID is too long: max 150 characters") -) - -type errTenantIDUnsupportedCharacter struct { - pos int - tenantID string -} - -func (e *errTenantIDUnsupportedCharacter) Error() string { - return fmt.Sprintf( - "tenant ID '%s' contains unsupported character '%c'", - e.tenantID, - e.tenantID[e.pos], - ) -} - -const tenantIDsLabelSeparator = "|" - -// NormalizeTenantIDs is creating a normalized form by sortiing and de-duplicating the list of tenantIDs -func NormalizeTenantIDs(tenantIDs []string) []string { - sort.Strings(tenantIDs) - - count := len(tenantIDs) - if count <= 1 { - return tenantIDs - } - - posOut := 1 - for posIn := 1; posIn < count; posIn++ { - if tenantIDs[posIn] != tenantIDs[posIn-1] { - tenantIDs[posOut] = tenantIDs[posIn] - posOut++ - } - } - - return tenantIDs[0:posOut] -} - -// ValidTenantID -func ValidTenantID(s string) error { - // check if it contains invalid runes - for pos, r := range s { - if !isSupported(r) { - return &errTenantIDUnsupportedCharacter{ - tenantID: s, - pos: pos, - } - } - } - - if len(s) > 150 { - return errTenantIDTooLong - } - - return nil -} - -func JoinTenantIDs(tenantIDs []string) string { - return strings.Join(tenantIDs, tenantIDsLabelSeparator) -} - -// this checks if a rune is supported in tenant IDs (according to -// https://cortexmetrics.io/docs/guides/limitations/#tenant-id-naming) -func isSupported(c rune) bool { - // characters - if ('a' <= c && c <= 'z') || ('A' <= c && c <= 'Z') { - return true - } - - // digits - if '0' <= c && c <= '9' { - return true - } - - // special - return c == '!' || - c == '-' || - c == '_' || - c == '.' || - c == '*' || - c == '\'' || - c == '(' || - c == ')' -} - -// IDsFromOrgID extracts different tenants from an orgID string value -func IDsFromOrgID(orgID string) ([]string, error) { - return IDs(user.InjectOrgID(context.TODO(), orgID)) -} diff --git a/tenant/tenant_test.go b/tenant/tenant_test.go deleted file mode 100644 index b242fd77f..000000000 --- a/tenant/tenant_test.go +++ /dev/null @@ -1,42 +0,0 @@ -package tenant - -import ( - "strings" - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestValidTenantIDs(t *testing.T) { - for _, tc := range []struct { - name string - err *string - }{ - { - name: "tenant-a", - }, - { - name: "ABCDEFGHIJKLMNOPQRSTUVWXYZ-abcdefghijklmnopqrstuvwxyz_0987654321!.*'()", - }, - { - name: "invalid|", - err: strptr("tenant ID 'invalid|' contains unsupported character '|'"), - }, - { - name: strings.Repeat("a", 150), - }, - { - name: strings.Repeat("a", 151), - err: strptr("tenant ID is too long: max 150 characters"), - }, - } { - t.Run(tc.name, func(t *testing.T) { - err := ValidTenantID(tc.name) - if tc.err == nil { - assert.Nil(t, err) - } else { - assert.EqualError(t, err, *tc.err) - } - }) - } -}