-
Notifications
You must be signed in to change notification settings - Fork 512
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
Rename "fake" tenant that is used with no auth to "anonymous", and make it configurable #1063
Conversation
Maybe we should consider making this configurable, to make migration less painful? |
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 love the idea of renaming fake but we should make it in a backward compatible way. I disagree on renaming it straight away. Making the default tenant configurable may be an option.
Making it configurable, and change the default sounds like the best way forward to me. WDYT? |
Makes sense to me. |
@@ -65,7 +65,7 @@ where `default_value` is the value to use if the environment variable is undefin | |||
# CLI flag: -auth.enabled | |||
[auth_enabled: <boolean> | default = true] | |||
|
|||
# Tenant name to use when auth is disabled. | |||
# [advanced] Tenant name to use when auth is disabled. |
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.
Does "auth" mean authentication, authorization, or both?
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 never quite spell it out anywhere :) I always read is as authentication (ie. "who"), because we can't disable authorization in Mimir (each tenant can only access their own data, nothing else). But perhaps my understanding is wrong.
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 guess the query federation could be considered an authorization mechanism, no? Because it authorizes tenants to access data of other tenants... But I don't think we call it that anywhere
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 guess the query federation could be considered an authorization mechanism, no? Because it authorizes tenants to access data of other tenants...
Yes, I think you're right. I believe @osg-grafana's question was about "auth" in -auth.enabled
and -auth.no-auth-tenant
names and descriptions though.
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.
"auth" is not a word, so I will have to think about what it means every time I see it.
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.
Approving despite "auth" being ambiguous.
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.
LGTM. This should be mentioned in the migration efforts.
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.
LGTM (modulo a couple of nits). There are few more tests using "fake"
: could you update them too, just to be consistent with new default, please?
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
…efault. Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
Co-authored-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
829f124
to
68197af
Compare
What this PR does: This PR renames
fake
tenant that is used when authentication is disabled toanonymous
. This is now also configurable using-auth.no-auth-tenant
option.Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]