Skip to content

Conversation

vineetjp
Copy link
Contributor

@vineetjp vineetjp commented Oct 1, 2023

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
Copy link

CLAassistant commented Oct 1, 2023

CLA assistant check
All committers have signed the CLA.

@joe-elliott
Copy link
Collaborator

Fixes #2973

Copy link
Collaborator

@joe-elliott joe-elliott left a comment

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.

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

@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.

Changes look good overall, just left a comment about removing pre-existing X-Scope-OrgID headers

@@ -33,7 +33,9 @@ func generateTenantRemoteWriteConfigs(originalCfgs []prometheus_config.RemoteWri
}
}

cloneCfg.Headers[user.OrgIDHeaderName] = tenant
if addOrgIDHeader {
Copy link
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

Copy link
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

Copy link
Contributor

@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! I'll give Joe a chance to check in again as well 🙂

Copy link
Collaborator

@joe-elliott joe-elliott left a comment

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
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.

4 participants