Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history 
- `--enable-tenancy` -> `--enforce-tenancy`
- Create `RewritePromQL` and `RewriteLabelMatchers` to clean up code in
  query api. Also move getLabelMatchers to tenancy pkg.
- Use prom-label-proxys `EnforceMatchers` to rewrite labels on non-query
  APIs instead of own solution
- Don't specifically handle `illegalLabelMatcherError`

Signed-off-by: Jacob Baungard Hansen <[email protected]>
  • Loading branch information
@jacobbaungard
jacobbaungard committed Oct 30, 2023
1 parent 689948e commit b2043ad
Show file tree
Hide file tree
Showing 5 changed files with 87 additions and 105 deletions.
2 changes: 1 addition & 1 deletion cmd/thanos/query.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ func registerQuery(app *extkingpin.App) {
tenantHeader := cmd.Flag("query.tenant-header", "HTTP header to determine tenant.").Default(tenancy.DefaultTenantHeader).String()
defaultTenant := cmd.Flag("query.default-tenant-id", "Default tenant ID to use if tenant header is not present").Default(tenancy.DefaultTenant).String()
tenantCertField := cmd.Flag("query.tenant-certificate-field", "Use TLS client's certificate field to determine tenant for write requests. Must be one of "+tenancy.CertificateFieldOrganization+", "+tenancy.CertificateFieldOrganizationalUnit+" or "+tenancy.CertificateFieldCommonName+". This setting will cause the query.tenant-header flag value to be ignored.").Default("").Enum("", tenancy.CertificateFieldOrganization, tenancy.CertificateFieldOrganizationalUnit, tenancy.CertificateFieldCommonName)
enforceTenancy := cmd.Flag("query.enable-tenancy", "Enable tenancy. Only responses where the value of the configured tenant-label-name and value of the tenant header matches are returned.").Default("false").Bool()
enforceTenancy := cmd.Flag("query.enforce-tenancy", "Enforce tenancy on Query APIs. Only responses where the value of the configured tenant-label-name and value of the tenant header matches are returned.").Default("false").Bool()
tenantLabel := cmd.Flag("query.tenant-label-name", "Label name to use when enforce tenancy when -querier.tenancy is enabled").Default(tenancy.DefaultTenantLabel).String()

var storeRateLimits store.SeriesSelectLimits
Expand Down
7 changes: 4 additions & 3 deletions docs/components/query.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -363,9 +363,10 @@ Flags:
--query.default-tenant-id="default-tenant"
Default tenant ID to use if tenant header is
not present
--query.enable-tenancy Enable tenancy. Only responses where the value
of the configured tenant-label-name and value
of the tenant header matches are returned.
--query.enforce-tenancy Enforce tenancy on Query APIs. Only
responses where the value of the configured
tenant-label-name and value of the tenant
header matches are returned.
--query.lookback-delta=QUERY.LOOKBACK-DELTA
The maximum lookback duration for retrieving
metrics during expression evaluations.
Expand Down
102 changes: 8 additions & 94 deletions pkg/api/query/v1.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -651,19 +651,9 @@ func (qapi *QueryAPI) query(r *http.Request) (interface{}, []error, *api.ApiErro
lookbackDelta = lookbackDeltaFromReq
}

tenant, err := tenancy.GetTenantFromHTTP(r, qapi.tenantHeader, qapi.defaultTenant, qapi.tenantCertField)
queryStr, tenant, ctx, err := tenancy.RewritePromQL(ctx, r, qapi.tenantHeader, qapi.defaultTenant, qapi.tenantCertField, qapi.enforceTenancy, qapi.tenantLabel, r.FormValue("query"))
if err != nil {
apiErr = &api.ApiError{Typ: api.ErrorBadData, Err: err}
return nil, nil, apiErr, func() {}
}
ctx = context.WithValue(ctx, tenancy.TenantKey, tenant)

queryStr := r.FormValue("query")
if qapi.enforceTenancy {
queryStr, err = tenancy.EnforceQueryTenancy(qapi.tenantLabel, tenant, queryStr)
if err != nil {
return nil, nil, &api.ApiError{Typ: api.ErrorBadData, Err: err}, func() {}
}
return nil, nil, &api.ApiError{Typ: api.ErrorBadData, Err: err}, func() {}
}

// We are starting promQL tracing span here, because we have no control over promQL code.
Expand Down Expand Up @@ -958,24 +948,14 @@ func (qapi *QueryAPI) queryRange(r *http.Request) (interface{}, []error, *api.Ap
lookbackDelta = lookbackDeltaFromReq
}

tenant, err := tenancy.GetTenantFromHTTP(r, qapi.tenantHeader, qapi.defaultTenant, qapi.tenantCertField)
queryStr, tenant, ctx, err := tenancy.RewritePromQL(ctx, r, qapi.tenantHeader, qapi.defaultTenant, qapi.tenantCertField, qapi.enforceTenancy, qapi.tenantLabel, r.FormValue("query"))
if err != nil {
apiErr = &api.ApiError{Typ: api.ErrorBadData, Err: err}
return nil, nil, apiErr, func() {}
return nil, nil, &api.ApiError{Typ: api.ErrorBadData, Err: err}, func() {}
}
ctx = context.WithValue(ctx, tenancy.TenantKey, tenant)

// Record the query range requested.
qapi.queryRangeHist.Observe(end.Sub(start).Seconds())

queryStr := r.FormValue("query")
if qapi.enforceTenancy {
queryStr, err = tenancy.EnforceQueryTenancy(qapi.tenantLabel, tenant, queryStr)
if err != nil {
return nil, nil, &api.ApiError{Typ: api.ErrorBadData, Err: err}, func() {}
}
}

// We are starting promQL tracing span here, because we have no control over promQL code.
span, ctx := tracing.StartSpan(ctx, "promql_range_query")
defer span.Finish()
Expand Down Expand Up @@ -1071,17 +1051,11 @@ func (qapi *QueryAPI) labelValues(r *http.Request) (interface{}, []error, *api.A
return nil, nil, apiErr, func() {}
}

tenant, err := tenancy.GetTenantFromHTTP(r, qapi.tenantHeader, qapi.defaultTenant, qapi.tenantCertField)
matcherSets, ctx, err := tenancy.RewriteLabelMatchers(ctx, r, qapi.tenantHeader, qapi.defaultTenant, qapi.tenantCertField, qapi.enforceTenancy, qapi.tenantLabel, r.Form[MatcherParam])
if err != nil {
apiErr = &api.ApiError{Typ: api.ErrorBadData, Err: err}
return nil, nil, apiErr, func() {}
}
ctx = context.WithValue(ctx, tenancy.TenantKey, tenant)

matcherSets, apiErr := qapi.getLabelMatchers(r.Form[MatcherParam], tenant)
if apiErr != nil {
return nil, nil, apiErr, func() {}
}

q, err := qapi.queryableCreate(
true,
Expand Down Expand Up @@ -1150,17 +1124,11 @@ func (qapi *QueryAPI) series(r *http.Request) (interface{}, []error, *api.ApiErr
return nil, nil, &api.ApiError{Typ: api.ErrorBadData, Err: err}, func() {}
}

tenant, err := tenancy.GetTenantFromHTTP(r, qapi.tenantHeader, qapi.defaultTenant, "")
matcherSets, ctx, err := tenancy.RewriteLabelMatchers(r.Context(), r, qapi.tenantHeader, qapi.defaultTenant, qapi.tenantCertField, qapi.enforceTenancy, qapi.tenantLabel, r.Form[MatcherParam])
if err != nil {
apiErr := &api.ApiError{Typ: api.ErrorBadData, Err: err}
return nil, nil, apiErr, func() {}
}
ctx := context.WithValue(r.Context(), tenancy.TenantKey, tenant)

matcherSets, apiErr := qapi.getLabelMatchers(r.Form[MatcherParam], tenant)
if apiErr != nil {
return nil, nil, apiErr, func() {}
}

enableDedup, apiErr := qapi.parseEnableDedupParam(r)
if apiErr != nil {
Expand Down Expand Up @@ -1233,15 +1201,9 @@ func (qapi *QueryAPI) labelNames(r *http.Request) (interface{}, []error, *api.Ap
return nil, nil, apiErr, func() {}
}

tenant, err := tenancy.GetTenantFromHTTP(r, qapi.tenantHeader, qapi.defaultTenant, "")
matcherSets, ctx, err := tenancy.RewriteLabelMatchers(r.Context(), r, qapi.tenantHeader, qapi.defaultTenant, qapi.tenantCertField, qapi.enforceTenancy, qapi.tenantLabel, r.Form[MatcherParam])
if err != nil {
apiErr = &api.ApiError{Typ: api.ErrorBadData, Err: err}
return nil, nil, apiErr, func() {}
}
ctx := context.WithValue(r.Context(), tenancy.TenantKey, tenant)

matcherSets, apiErr := qapi.getLabelMatchers(r.Form[MatcherParam], tenant)
if apiErr != nil {
apiErr := &api.ApiError{Typ: api.ErrorBadData, Err: err}
return nil, nil, apiErr, func() {}
}

Expand Down Expand Up @@ -1311,54 +1273,6 @@ func (qapi *QueryAPI) stores(_ *http.Request) (interface{}, []error, *api.ApiErr
return statuses, nil, nil, func() {}
}

func (qapi *QueryAPI) getLabelMatchers(matchers []string, tenant string) ([][]*labels.Matcher, *api.ApiError) {
tenantLabelMatcher := &labels.Matcher{
Name: qapi.tenantLabel,
Type: labels.MatchEqual,
Value: tenant,
}

matcherSets := make([][]*labels.Matcher, 0, len(matchers))

// If tenancy is enforced, but there are no matchers at all, add the tenant matcher
if len(matchers) == 0 && qapi.enforceTenancy {
var matcher []*labels.Matcher
matcher = append(matcher, tenantLabelMatcher)
matcherSets = append(matcherSets, matcher)
return matcherSets, nil
}

for _, s := range matchers {
matchers, err := parser.ParseMetricSelector(s)
if err != nil {
return nil, &api.ApiError{Typ: api.ErrorBadData, Err: err}
}
if qapi.enforceTenancy {
// first check if there's a tenant matcher already, in which case we overwrite it
// if there are multiple tenant matchers, we remove the subsequent ones
found := false
for idx, matchValue := range matchers {
if matchValue.Name == qapi.tenantLabel {
if found {
// remove any additional tenant matchers.
matchers = append(matchers[:idx], matchers[idx+1:]...)
} else {
matchers[idx] = tenantLabelMatcher
found = true
}
}
}
// if there are no pre-existing tenant matchers, add it.
if !found {
matchers = append(matchers, tenantLabelMatcher)
}
}
matcherSets = append(matcherSets, matchers)
}

return matcherSets, nil
}

// NewTargetsHandler created handler compatible with HTTP /api/v1/targets https://prometheus.io/docs/prometheus/latest/querying/api/#targets
// which uses gRPC Unary Targets API.
func NewTargetsHandler(client targets.UnaryClient, enablePartialResponse bool) func(*http.Request) (interface{}, []error, *api.ApiError, func()) {
Expand Down
79 changes: 73 additions & 6 deletions pkg/tenancy/tenancy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,14 +154,81 @@ func EnforceQueryTenancy(tenantLabel string, tenant string, query string) (strin
}

if err := e.EnforceNode(expr); err != nil {
var illegalLabelMatcherError *injectproxy.IllegalLabelMatcherError
if errors.As(err, *illegalLabelMatcherError) {
return "", illegalLabelMatcherError
}
return "", errors.Wrap(err, "error enforcing label")
}

queryStr := expr.String()
return expr.String(), nil
}

func getLabelMatchers(formMatchers []string, tenant string, enforceTenancy bool, tenantLabel string) ([][]*labels.Matcher, error) {
tenantLabelMatcher := &labels.Matcher{
Name: tenantLabel,
Type: labels.MatchEqual,
Value: tenant,
}

matcherSets := make([][]*labels.Matcher, 0, len(formMatchers))

// If tenancy is enforced, but there are no matchers at all, add the tenant matcher
if len(formMatchers) == 0 && enforceTenancy {
var matcher []*labels.Matcher
matcher = append(matcher, tenantLabelMatcher)
matcherSets = append(matcherSets, matcher)
return matcherSets, nil
}

for _, s := range formMatchers {
matchers, err := parser.ParseMetricSelector(s)
if err != nil {
return nil, err
}

if enforceTenancy {
e := injectproxy.NewEnforcer(false, tenantLabelMatcher)
matchers, err = e.EnforceMatchers(matchers)
if err != nil {
return nil, err
}
}

matcherSets = append(matcherSets, matchers)
}

return matcherSets, nil
}

// This function will:
// - Get tenant from HTTP header and add it to context.
// - if tenancy is enforce, add a tenant matcher.
func RewritePromQL(ctx context.Context, r *http.Request, tenantHeader string, defaultTenantID string, certTenantField string, enforceTenancy bool, tenantLabel string, queryStr string) (string, string, context.Context, error) {
tenant, err := GetTenantFromHTTP(r, tenantHeader, defaultTenantID, certTenantField)
if err != nil {
return "", "", ctx, err
}
ctx = context.WithValue(ctx, TenantKey, tenant)

if enforceTenancy {
queryStr, err = EnforceQueryTenancy(tenantLabel, tenant, queryStr)
return queryStr, tenant, ctx, err
}
return queryStr, tenant, ctx, nil
}

// This function will:
// - Get tenant from HTTP header and add it to context.
// - Parse all labels matchers provided.
// - If tenancy is enforced, make sure a tenant matcher is present.
func RewriteLabelMatchers(ctx context.Context, r *http.Request, tenantHeader string, defaultTenantID string, certTenantField string, enforceTenancy bool, tenantLabel string, formMatchers []string) ([][]*labels.Matcher, context.Context, error) {
tenant, err := GetTenantFromHTTP(r, tenantHeader, defaultTenantID, certTenantField)
if err != nil {
return nil, ctx, err
}
ctx = context.WithValue(ctx, TenantKey, tenant)

matcherSets, err := getLabelMatchers(formMatchers, tenant, enforceTenancy, tenantLabel)
if err != nil {
return nil, ctx, err
}

return queryStr, nil
return matcherSets, ctx, nil
}
2 changes: 1 addition & 1 deletion test/e2e/e2ethanos/services.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,7 @@ func (q *QuerierBuilder) collectArgs() ([]string, error) {
args = append(args, "--query.telemetry.request-series-seconds-quantiles="+strconv.FormatFloat(bucket, 'f', -1, 64))
}
if q.enforceTenancy {
args = append(args, "--query.enable-tenancy")
args = append(args, "--query.enforce-tenancy")
}
return args, nil
}
Expand Down

0 comments on commit b2043ad

Please sign in to comment.