-
Notifications
You must be signed in to change notification settings - Fork 818
Introduce a regex tenant resolver #6713
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Introduce a regex tenant resolver #6713
Conversation
1b8963e
to
e6a4222
Compare
@CharlieTLe |
e6a4222
to
63f97d2
Compare
pkg/cortex/modules.go
Outdated
// because if the # of matched tenantIDs is only one, `X-Scope-OrgID` header is | ||
// set to input regex. | ||
byPassForSingleQuerier = false | ||
tenant.WithDefaultResolver(tenantfederation.NewRegexResolver(prometheus.DefaultRegisterer, t.Cfg.TenantFederation.UserSyncInterval, util_log.Logger, t.Distributor.AllUserStats)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there is a better way to gather all users. Calling t.Distributor.AllUserStats
seems a bit expensive just to get user IDs.
And it cannot cover users that don't ingest anymore but maybe their data still present on long term storage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And it cannot cover users that don't ingest anymore but maybe their data still present on long term storage.
Thanks for catching.
How about utilizing the userScanner
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling t.Distributor.AllUserStats seems a bit expensive just to get user IDs.
Do you have any good ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks awesome!
63f97d2
to
605f91a
Compare
605f91a
to
d1b80e0
Compare
@CharlieTLe |
b7ef458
to
7321e4b
Compare
eaeb19e
to
1135b06
Compare
CHANGELOG.md
Outdated
@@ -11,6 +11,9 @@ | |||
* [FEATURE] Ingester: Support out-of-order native histogram ingestion. It automatically enabled when `-ingester.out-of-order-time-window > 0` and `-blocks-storage.tsdb.enable-native-histograms=true`. #6626 #6663 | |||
* [FEATURE] Ruler: Add support for percentage based sharding for rulers. #6680 | |||
* [FEATURE] Ruler: Add support for group labels. #6665 | |||
* [FEATURE] Query federation: Introduce a regex tenant resolver to allow regex in `X-Scope-OrgID` value. #6713 | |||
- Add a `tenant-federation.regex-matcher-enabled` flag. If it enabled, user can input regex to `X-Scope-OrgId`, the matched tenantIDs are automatically involved. | |||
- Add a `tenant-federation.user-sync-interval` flag, it specifies how frequently to scan users. The scanned users are used to calculate matched tenantIDs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should document it as experimental feature in the doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should document that the user discovery is based on scanning block storage so new users are only available every 2h (assuming blocks are only uploaded every 2h).
return nil, errors.Wrap(err, "failed to create the bucket client") | ||
} | ||
|
||
userScanner, err := users.NewScanner(cfg, bucketClient, logger, reg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably need to wrap the registry with the component name? I think you will get duplicate metrics registration issue as we initialize multiple user scanner in different components. This can fail if running in single binary mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's safe, but it's better to add the component name.
Help: "Number of discovered users.", | ||
}) | ||
|
||
go r.updateUsersLoop(userSyncInterval, userScanTimeout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we maintain this using a service rather than a simple goroutine?
defer cancel() | ||
|
||
// only active users are considered | ||
activeUsers, _, _, err := r.userScanner.ScanUsers(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should include deleting users as well. Store Gateway still load blocks from deleting users and it should be expected to query those users within the clean up delay.
userScanTimeout = time.Second * 30 | ||
) | ||
|
||
type RegexResolver struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add some comments of what RegexResolver
and RegexValidator
does? I am not too familiar with this code path so unsure their difference
1135b06
to
1a3d2f1
Compare
Signed-off-by: SungJin1212 <[email protected]>
1a3d2f1
to
8737cf6
Compare
This PR introduces a regex tenant resolver to allow regex in the
X-Scope-OrgID
value when the user usestenant-federation
feature.It introduces two flags,
tenant-federation.regex-matcher-enabled
andtenant-federation.user-sync-interval
.tenant-federation.regex-matcher-enabled
enables the regex resolver, which allows regex to theX-Scope-OrgID
value.tenant-federation.user-sync-interval
specifies how frequently to scan users. The scanned users are used to calculate matched tenantIDs.The regex matching rule follows the Prometheus regex matcher (
=~
), See here.For example, if there are 3 tenants, whose IDs are
user-1
,user-2
, anduser-3
. We can setX-Scope-OrgID
touser-.+
to query whole tenants.Also, we can use an existing way like setting
user-1|user-2|user-3
toX-Scope-OrgID
.It reuses
userScanner
to find considered tenant IDs. So, only tenants who uploaded blocks are subject to regex resolution.Which issue(s) this PR fixes:
Fixes #6588
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]