Skip to content

metrics-generator disable x-scope-orgid header append - #2974

Merged
joe-elliott merged 6 commits into
grafana:mainfrom
vineetjp:metrics-generator-disable-x-scope-orgid
Oct 6, 2023
Merged

metrics-generator disable x-scope-orgid header append#2974
joe-elliott merged 6 commits into
grafana:mainfrom
vineetjp:metrics-generator-disable-x-scope-orgid

Conversation

@vineetjp

@vineetjp vineetjp commented Oct 1, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

What this PR does:
Adds option to disable addition of X-Scope-OrgID header in metrics generator with remote writing
Which issue(s) this PR fixes:
Fixes #1554

Checklist

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

CLAassistant commented Oct 1, 2023 •
edited
Loading

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.
@joe-elliott

joe-elliott commented Oct 2, 2023

Copy link
Copy Markdown
Collaborator

Fixes #2973
joe-elliott
joe-elliott reviewed Oct 2, 2023
View reviewed changes

@joe-elliott joe-elliott left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

thanks for this improvement!

A couple of small comments, but overall it's looking good.
Comment thread modules/generator/storage/config_util.go Outdated
Comment thread docs/sources/tempo/configuration/_index.md Outdated
@joe-elliott joe-elliott mentioned this pull request Oct 2, 2023
Closed
@vineetjp
Updated as per review comments
2ac2622 
1. Changed removeOrgIdHeader to addOrgIdHeader
2. addOrgIdHeader default value will be true
joe-elliott
joe-elliott reviewed Oct 3, 2023
View reviewed changes
Comment thread CHANGELOG.md Outdated
Comment thread modules/generator/storage/config.go Outdated
vineetjp and others added 2 commits October 3, 2023 23:32
yvrhdn
yvrhdn reviewed Oct 4, 2023
View reviewed changes

@yvrhdn yvrhdn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changes look good overall, just left a comment about removing pre-existing X-Scope-OrgID headers
Comment thread modules/generator/storage/config_util.go Outdated
}

cloneCfg.Headers[user.OrgIDHeaderName] = tenant
if addOrgIDHeader {

@yvrhdn yvrhdn Oct 4, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we should move this if-case higher and merge it with if tenant != util.FakeTenantID at line 24.
Make it:

if tenant != util.FakeTenantID && addOrgIDHeader {

Reasoning: we only need to remove any pre-existing X-Scope-OrgID headers if we want to inject our own. If we are not injecting any, there won't be a conflict and we don't need to remove them either.

We have the following possible scenarios:

  • single-tenant: we don't remove X-Scope-OrgID
  • multi-tenant with addOrgIDHeader = true: we remove X-Scope-OrgID headers and inject our own
  • multi-tenant with addOrgIDHeader = false: we do not inject our own X-Scope-OrgID header so I don't think we need to remove them as well

@vineetjp vineetjp Oct 4, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@kvrhdn I agree with your point of view. addOrgIDHeader = false make metrics generator more lenient on remote write authentication then it should also allow X-Scope-OrgID that a user wants to pass using remote write config. The suggested changes has been committed 364bdc3
yvrhdn
yvrhdn approved these changes Oct 5, 2023
View reviewed changes

@yvrhdn yvrhdn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! I'll give Joe a chance to check in again as well 🙂
joe-elliott
joe-elliott approved these changes Oct 6, 2023
View reviewed changes

@joe-elliott joe-elliott left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice addition. Thanks!
@joe-elliott
joe-elliott merged commit 39f52ac into grafana:main Oct 6, 2023
@vineetjp
vineetjp deleted the metrics-generator-disable-x-scope-orgid branch October 6, 2023 13:31
@jburnham jburnham mentioned this pull request Jan 29, 2024
Merged
8 tasks
mattdurham pushed a commit to mattdurham/tempo that referenced this pull request Jun 18, 2026
@vineetjp @joe-elliott
metrics-generator disable x-scope-orgid header append (grafana#2974)
e9da898 
* metrics-generator disable x-scope-orgid header append

* Update CHANGELOG.md

* Updated as per review comments

1. Changed removeOrgIdHeader to addOrgIdHeader
2. addOrgIdHeader default value will be true

* Update CHANGELOG.md

Co-authored-by: Joe Elliott <joe.elliott@grafana.com>

* changed addorgidheader config from pointer to normal variable

* Moved addOrgIDHeader check

---------

Co-authored-by: Joe Elliott <joe.elliott@grafana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants

@vineetjp @CLAassistant @joe-elliott @yvrhdn