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

Generator remote write update handling of X-Scope-OrgID #4021

Merged
merged 4 commits into from
Aug 28, 2024

Conversation

mdisibio
Copy link
Contributor

@mdisibio mdisibio commented Aug 28, 2024

What this PR does:
This PR changes the header precedence in Generator remote write, so that statically configured X-Scope-OrgID is honored. This change is necessary to unlock a use case that is currently not possible. Will do my best to explain. The desired behavior is to have a setup like this:

  • 2 remote write targets with different headers and auth:
    • remote write target A:
      • The usual setup:
      • dynamically set X-Scope-OrgID header == tenant ID
    • remote write target B:
      • statically configured X-Scope-OrgID (we want to target a fixed metrics tenant)
      • Dynamically injected metrics label with the tenant ID like {__id__="<tenantid>"}

Target B doesn't work currently because the logic gives highest precedence to dynamically set X-Scope-OrgID. The statically configured header is deleted and a warning logged.

This logic was inherited and designed to protect against broken authentication, but after review it doesn't seem a compelling use case. Remote write headers can be set in two locations:

  1. In the global config:
metrics_generator:
  storage:
    remote_write:
      - url: ...
        headers:
          X-Scope-OrgID: ...
  1. In runtime overrides per-tenant:
overrides:
  '*':
    metrics_generator_remote_write_headers:
      X-Scope-OrgID: ...

Both locations are statically configured by a Tempo operator, and it is more likely they are intentional than not.

Which issue(s) this PR fixes:
Fixes n/a

Checklist

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

Copy link
Member

@yvrhdn yvrhdn left a comment

Choose a reason for hiding this comment

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

LGTM, PR is clear 👍

@mdisibio mdisibio marked this pull request as ready for review August 28, 2024 16:49
@mdisibio mdisibio enabled auto-merge (squash) August 28, 2024 16:57
@mdisibio mdisibio merged commit 9b72235 into grafana:main Aug 28, 2024
16 checks passed
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.

2 participants