From 5c70213f9f1d2eb5c326e22f1945c92e7f73fa69 Mon Sep 17 00:00:00 2001
From: Paul Gier <pgier@redhat.com>
Date: Wed, 13 Jun 2018 10:34:59 -0500
Subject: [PATCH] config: set target group source index during unmarshalling
 (#4245)

* config: set target group source index during unmarshalling

Fixes issue #4214 where the scrape pool is unnecessarily reloaded for a
config reload where the config hasn't changed.  Previously, the discovery
manager changed the static config after loading which caused the in-memory
config to differ from a freshly reloaded config.

Signed-off-by: Paul Gier <pgier@redhat.com>

* [issue #4214] Test that static targets are not modified by discovery manager

Signed-off-by: Paul Gier <pgier@redhat.com>
---
 config/config.go          | 14 ++++++++++++++
 config/config_test.go     |  4 ++++
 discovery/manager.go      | 11 +----------
 discovery/manager_test.go | 39 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 58 insertions(+), 10 deletions(-)

diff --git a/config/config.go b/config/config.go
index 746aae3bf..b7a566764 100644
--- a/config/config.go
+++ b/config/config.go
@@ -370,6 +370,13 @@ func (c *ScrapeConfig) UnmarshalYAML(unmarshal func(interface{}) error) error {
 			}
 		}
 	}
+
+	// Add index to the static config target groups for unique identification
+	// within scrape pool.
+	for i, tg := range c.ServiceDiscoveryConfig.StaticConfigs {
+		tg.Source = fmt.Sprintf("%d", i)
+	}
+
 	return nil
 }
 
@@ -432,6 +439,13 @@ func (c *AlertmanagerConfig) UnmarshalYAML(unmarshal func(interface{}) error) er
 			}
 		}
 	}
+
+	// Add index to the static config target groups for unique identification
+	// within scrape pool.
+	for i, tg := range c.ServiceDiscoveryConfig.StaticConfigs {
+		tg.Source = fmt.Sprintf("%d", i)
+	}
+
 	return nil
 }
 
diff --git a/config/config_test.go b/config/config_test.go
index 8caaebd55..549a4f909 100644
--- a/config/config_test.go
+++ b/config/config_test.go
@@ -128,6 +128,7 @@ var expectedConf = &Config{
 							"my":   "label",
 							"your": "label",
 						},
+						Source: "0",
 					},
 				},
 
@@ -484,6 +485,7 @@ var expectedConf = &Config{
 						Targets: []model.LabelSet{
 							{model.AddressLabel: "localhost:9090"},
 						},
+						Source: "0",
 					},
 				},
 			},
@@ -503,6 +505,7 @@ var expectedConf = &Config{
 						Targets: []model.LabelSet{
 							{model.AddressLabel: "localhost:9090"},
 						},
+						Source: "0",
 					},
 				},
 			},
@@ -548,6 +551,7 @@ var expectedConf = &Config{
 								{model.AddressLabel: "1.2.3.5:9093"},
 								{model.AddressLabel: "1.2.3.6:9093"},
 							},
+							Source: "0",
 						},
 					},
 				},
diff --git a/discovery/manager.go b/discovery/manager.go
index 669a91dc5..97468a549 100644
--- a/discovery/manager.go
+++ b/discovery/manager.go
@@ -285,7 +285,7 @@ func (m *Manager) providersFromConfig(cfg sd_config.ServiceDiscoveryConfig) map[
 		app("triton", i, t)
 	}
 	if len(cfg.StaticConfigs) > 0 {
-		app("static", 0, NewStaticProvider(cfg.StaticConfigs))
+		app("static", 0, &StaticProvider{cfg.StaticConfigs})
 	}
 
 	return providers
@@ -296,15 +296,6 @@ type StaticProvider struct {
 	TargetGroups []*targetgroup.Group
 }
 
-// NewStaticProvider returns a StaticProvider configured with the given
-// target groups.
-func NewStaticProvider(groups []*targetgroup.Group) *StaticProvider {
-	for i, tg := range groups {
-		tg.Source = fmt.Sprintf("%d", i)
-	}
-	return &StaticProvider{groups}
-}
-
 // Run implements the Worker interface.
 func (sd *StaticProvider) Run(ctx context.Context, ch chan<- []*targetgroup.Group) {
 	// We still have to consider that the consumer exits right away in which case
diff --git a/discovery/manager_test.go b/discovery/manager_test.go
index c57351270..19efb7d62 100644
--- a/discovery/manager_test.go
+++ b/discovery/manager_test.go
@@ -774,6 +774,45 @@ scrape_configs:
 	verifyPresence(discoveryManager.targets, poolKey{setName: "prometheus", provider: "static/0"}, "{__address__=\"bar:9090\"}", false)
 }
 
+func TestApplyConfigDoesNotModifyStaticProviderTargets(t *testing.T) {
+	cfgText := `
+scrape_configs:
+ - job_name: 'prometheus'
+   static_configs:
+   - targets: ["foo:9090"]
+   - targets: ["bar:9090"]
+   - targets: ["baz:9090"]
+`
+	originalConfig := &config.Config{}
+	if err := yaml.UnmarshalStrict([]byte(cfgText), originalConfig); err != nil {
+		t.Fatalf("Unable to load YAML config cfgYaml: %s", err)
+	}
+	origScrpCfg := originalConfig.ScrapeConfigs[0]
+
+	processedConfig := &config.Config{}
+	if err := yaml.UnmarshalStrict([]byte(cfgText), processedConfig); err != nil {
+		t.Fatalf("Unable to load YAML config cfgYaml: %s", err)
+	}
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	discoveryManager := NewManager(ctx, nil)
+	go discoveryManager.Run()
+
+	c := make(map[string]sd_config.ServiceDiscoveryConfig)
+	for _, v := range processedConfig.ScrapeConfigs {
+		c[v.JobName] = v.ServiceDiscoveryConfig
+	}
+	discoveryManager.ApplyConfig(c)
+	<-discoveryManager.SyncCh()
+
+	for _, sdcfg := range c {
+		if !reflect.DeepEqual(origScrpCfg.ServiceDiscoveryConfig.StaticConfigs, sdcfg.StaticConfigs) {
+			t.Fatalf("discovery manager modified static config \n  expected: %v\n  got: %v\n",
+				origScrpCfg.ServiceDiscoveryConfig.StaticConfigs, sdcfg.StaticConfigs)
+		}
+	}
+}
+
 type update struct {
 	targetGroups []targetgroup.Group
 	interval     time.Duration