Skip to content
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

continuoustest: Set both tenant-id and authentication #7619

Conversation

Itaykal
Copy link
Contributor

@Itaykal Itaykal commented Mar 13, 2024

What this PR does

Allows setting tenant-id when writing data using basic-auth/bearer-token in order to test metrics for specific tenants

Which issue(s) this PR fixes or relates to

Fixes #6091

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

@Itaykal Itaykal requested a review from a team as a code owner March 13, 2024 13:00
@Itaykal
Copy link
Contributor Author

Itaykal commented Mar 13, 2024

A few notes I'm not so sure about:

  1. Which test to run, I'm unable to find any documentation about countinoustest in the contribution guides.
  2. Should I update the CHANGELOG? if I understand this correctly it's not a part of the Mimir runtime

If there's anything specific that I need to run I'll gladly do so.

Copy link
Contributor

@56quarters 56quarters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why you'd want to use both tenant ID and auth at the same time? This doesn't make sense to me.

In Mimir, you'd only set the tenant ID via header.

With Grafana Enterprise Metrics you'd set basic auth and the authentication layer would resolve that to a tenant ID.

Adding the ability to set both of them seems like it will introduce confusion.

@Itaykal
Copy link
Contributor Author

Itaykal commented Mar 13, 2024

Can you explain why you'd want to use both tenant ID and auth at the same time? This doesn't make sense to me.

The official Helm chart supports configuring a gateway that respects this behavior (basic-auth + custom tenant-id), it would be quite frustrating if other tools in the ecosystem such as continuous test won't support this.

And even if you don't use the helm chart specified here, It's not a stretch to assume some sort of authentication is required to reach the cluster.

In Mimir, you'd only set the tenant ID via header.

In some environments this can't be the only security measure and basic auth or any other authentication solution could be used (without coupling them to tenants as done in GEM).

Adding the ability to set both of them seems like it will introduce confusion.

I can see why you'd say that, but I think re-creating the same behavior on non GEM instances will be a more complicated solution prone to breaking changes on some environments.

I understand your point of this not being a common use-case but I've encountered it, as well as the issue's creator, so I guess this is a thing people do

Copy link
Contributor

@56quarters 56quarters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for blocking this, your explanation makes sense. Can you add an entry to the changelog that mentions that tenant ID and basic/bearer auth is now allowed to be combined?

@Itaykal
Copy link
Contributor Author

Itaykal commented Apr 18, 2024

Sorry for blocking this, your explanation makes sense. Can you add an entry to the changelog that mentions that tenant ID and basic/bearer auth is now allowed to be combined?

Done

CHANGELOG.md Outdated Show resolved Hide resolved
@56quarters 56quarters merged commit 547df5f into grafana:main Apr 18, 2024
29 checks passed
@kressnick25
Copy link
Contributor

Continuous test client appears to still only set basic-auth OR tenant ID if both are defined - per
https://github.com/grafana/mimir/blob/main/pkg/continuoustest/client.go#L268-L274

if rt.bearerToken != "" {
    req.Header.Set("Authorization", "Bearer "+rt.bearerToken)
} else if rt.basicAuthUser != "" && rt.basicAuthPassword != "" {
    req.SetBasicAuth(rt.basicAuthUser, rt.basicAuthPassword)
} else {
    req.Header.Set("X-Scope-OrgID", rt.tenantID)
}

When passing both username/password AND tenantId to the latest pre-release binary, I am receiving
status_code=401 err="server returned HTTP status 401 Unauthorized and body \"no org id\\n\" response in the mimir-continous-test output

@Itaykal
Copy link
Contributor Author

Itaykal commented Jun 28, 2024

@kressnick25 Could you create an issue about this? I'll try to find time to work on it in the following days but I'm not sure I could

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Continuous Test: set both basic-auth and tenant_id
3 participants