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

* Generator remote write change header precedence so that statically configured X-Scope-OrgID is honored * restore trimspace * Update modules/generator/storage/config_util.go Co-authored-by: Yuna Verheyden <[email protected]> * changelog --------- Co-authored-by: Yuna Verheyden <[email protected]>
grafana · Aug 28, 2024 · 9b72235 · 9b72235
1 parent 7ee2119
commit 9b72235
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 24 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,6 @@
 ## main / unreleased
 
+* [CHANGE] **BREAKING CHANGE** The dynamic injection of X-Scope-OrgID header for metrics generator remote-writes is changed. If the header is aleady set in per-tenant overrides or global tempo configuration, then it is honored and not overwritten. [#4021](https://github.com/grafana/tempo/pull/4021) (@mdisibio)
 * [FEATURE] Discarded span logging `log_discarded_spans` [#3957](https://github.com/grafana/tempo/issues/3957) (@dastrobu)
 * [ENHANCEMENT] TraceQL: Attribute iterators collect matched array values [#3867](https://github.com/grafana/tempo/pull/3867) (@electron0zero, @stoewer)
 * [ENHANCEMENT] Allow returning partial traces that exceed the MaxBytes limit for V2 [#3941](https://github.com/grafana/tempo/pull/3941) (@javiermolinar)

diff --git a/modules/generator/storage/config_util.go b/modules/generator/storage/config_util.go
@@ -13,43 +13,51 @@ import (
 
 // generateTenantRemoteWriteConfigs creates a copy of the remote write configurations with the
 // X-Scope-OrgID header present for the given tenant, unless Tempo is run in single tenant mode or instructed not to add X-Scope-OrgID header.
-func generateTenantRemoteWriteConfigs(originalCfgs []prometheus_config.RemoteWriteConfig, tenant string, headers map[string]string, addOrgIDHeader bool, logger log.Logger, sendNativeHistograms bool) []*prometheus_config.RemoteWriteConfig {
-	var cloneCfgs []*prometheus_config.RemoteWriteConfig
+func generateTenantRemoteWriteConfigs(inputs []prometheus_config.RemoteWriteConfig, tenant string, headers map[string]string, addOrgIDHeader bool, logger log.Logger, sendNativeHistograms bool) []*prometheus_config.RemoteWriteConfig {
+	var outputs []*prometheus_config.RemoteWriteConfig
 
-	for _, originalCfg := range originalCfgs {
-		cloneCfg := &prometheus_config.RemoteWriteConfig{}
-		*cloneCfg = originalCfg
+	for _, input := range inputs {
+		output := &prometheus_config.RemoteWriteConfig{}
+		*output = input
 
-		// Inject/overwrite X-Scope-OrgID header in multi-tenant setups
-		if tenant != util.FakeTenantID && addOrgIDHeader {
-			// Copy headers so we can modify them
-			cloneCfg.Headers = copyMap(cloneCfg.Headers)
+		// Copy headers so we can modify them
+		output.Headers = copyMap(output.Headers)
 
-			// Ensure that no variation of the X-Scope-OrgId header can be added, which might trick authentication
-			for k, v := range cloneCfg.Headers {
+		// Inject/overwrite custom headers from runtime overrides
+		for k, v := range headers {
+			output.Headers[k] = v
+		}
+
+		// Inject X-Scope-OrgID header in multi-tenant setups if not set already
+		if tenant != util.FakeTenantID && addOrgIDHeader {
+			existing := ""
+			for k, v := range output.Headers {
 				if strings.EqualFold(user.OrgIDHeaderName, strings.TrimSpace(k)) {
-					level.Warn(logger).Log("msg", "discarding X-Scope-OrgId header", "key", k, "value", v)
-					delete(cloneCfg.Headers, k)
+					existing = v
+					break
 				}
 			}
 
-			cloneCfg.Headers[user.OrgIDHeaderName] = tenant
-		}
-
-		// Inject/overwrite custom headers
-		// Caution! This can overwrite the X-Scope-OrgID header
-		for k, v := range headers {
-			cloneCfg.Headers[k] = v
+			if existing == "" {
+				output.Headers[user.OrgIDHeaderName] = tenant
+			} else {
+				// Remote write config already contains the header, so we don't overwrite it.
+				level.Warn(logger).Log("msg", "underlying remote write already contains X-Scope-OrgId header, not applying new value",
+					"remoteWriteName", input.Name,
+					"remoteWriteURL", input.URL,
+					"existing", existing,
+					"new", tenant)
+			}
 		}
 
-		cloneCfg.SendNativeHistograms = sendNativeHistograms
+		output.SendNativeHistograms = sendNativeHistograms
 		// TODO: enable exemplars
 		// cloneCfg.SendExemplars = sendExemplars
 
-		cloneCfgs = append(cloneCfgs, cloneCfg)
+		outputs = append(outputs, output)
 	}
 
-	return cloneCfgs
+	return outputs
 }
 
 // copyMap creates a new map containing all values from the given map.

diff --git a/modules/generator/storage/config_util_test.go b/modules/generator/storage/config_util_test.go
@@ -34,13 +34,16 @@ func Test_generateTenantRemoteWriteConfigs(t *testing.T) {
 
 	result := generateTenantRemoteWriteConfigs(original, "my-tenant", nil, addOrgIDHeader, logger, false)
 
+	// First case doesn't have a header, gets set.
 	assert.Equal(t, original[0].URL, result[0].URL)
 	assert.Equal(t, map[string]string{}, original[0].Headers, "Original headers have been modified")
 	assert.Equal(t, map[string]string{"X-Scope-OrgID": "my-tenant"}, result[0].Headers)
 
+	// Second case already contains header, not overwritten
+	// Also checks case-insensitivity
 	assert.Equal(t, original[1].URL, result[1].URL)
 	assert.Equal(t, map[string]string{"foo": "bar", "x-scope-orgid": "fake-tenant"}, original[1].Headers, "Original headers have been modified")
-	assert.Equal(t, map[string]string{"foo": "bar", "X-Scope-OrgID": "my-tenant"}, result[1].Headers)
+	assert.Equal(t, map[string]string{"foo": "bar", "x-scope-orgid": "fake-tenant"}, result[1].Headers, "Existing header was incorrectly overwritten")
 }
 
 func Test_generateTenantRemoteWriteConfigs_singleTenant(t *testing.T) {