From c0202733ecbe7b64dc9a0ca02534e9f67ef261c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Gouteroux?= Date: Wed, 31 Jul 2024 10:47:37 +0200 Subject: [PATCH 1/3] fix: validate scrape_config job name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: François Gouteroux --- clients/pkg/promtail/promtail.go | 9 ++- .../pkg/promtail/scrapeconfig/scrapeconfig.go | 21 ++++++ .../scrapeconfig/scrapeconfig_test.go | 65 +++++++++++++++++++ 3 files changed, 94 insertions(+), 1 deletion(-) diff --git a/clients/pkg/promtail/promtail.go b/clients/pkg/promtail/promtail.go index 73e52f21703e1..65b543c05e84e 100644 --- a/clients/pkg/promtail/promtail.go +++ b/clients/pkg/promtail/promtail.go @@ -18,6 +18,7 @@ import ( "github.com/grafana/loki/v3/clients/pkg/promtail/api" "github.com/grafana/loki/v3/clients/pkg/promtail/client" "github.com/grafana/loki/v3/clients/pkg/promtail/config" + "github.com/grafana/loki/v3/clients/pkg/promtail/scrapeconfig" "github.com/grafana/loki/v3/clients/pkg/promtail/server" "github.com/grafana/loki/v3/clients/pkg/promtail/targets" "github.com/grafana/loki/v3/clients/pkg/promtail/targets/target" @@ -139,10 +140,16 @@ func (p *Promtail) reloadConfig(cfg *config.Config) error { } cfg.Setup(p.logger) + var err error + err = scrapeconfig.ValidateJobName(cfg.ScrapeConfig) + if err != nil { + return err + } + if cfg.LimitsConfig.ReadlineRateEnabled { stages.SetReadLineRateLimiter(cfg.LimitsConfig.ReadlineRate, cfg.LimitsConfig.ReadlineBurst, cfg.LimitsConfig.ReadlineRateDrop) } - var err error + // entryHandlers contains all sinks were scraped log entries should get to var entryHandlers = []api.EntryHandler{} diff --git a/clients/pkg/promtail/scrapeconfig/scrapeconfig.go b/clients/pkg/promtail/scrapeconfig/scrapeconfig.go index b2466b83791e1..67b4f0898034d 100644 --- a/clients/pkg/promtail/scrapeconfig/scrapeconfig.go +++ b/clients/pkg/promtail/scrapeconfig/scrapeconfig.go @@ -1,6 +1,7 @@ package scrapeconfig import ( + "errors" "fmt" "reflect" "time" @@ -26,6 +27,7 @@ import ( "github.com/prometheus/prometheus/discovery/triton" "github.com/prometheus/prometheus/discovery/zookeeper" "github.com/prometheus/prometheus/model/relabel" + "github.com/prometheus/prometheus/util/strutil" "github.com/grafana/loki/v3/clients/pkg/logentry/stages" "github.com/grafana/loki/v3/clients/pkg/promtail/discovery/consulagent" @@ -484,3 +486,22 @@ func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error { return nil } + +func ValidateJobName(scrapeConfigs []Config) error { + jobNames := map[string]struct{}{} + for i, cfg := range scrapeConfigs { + if cfg.JobName == "" { + return errors.New("`job_name` must be defined for the scrape_config with a " + + "unique name, " + + "at least one scrape_config has no `job_name` defined") + } + if _, ok := jobNames[cfg.JobName]; ok { + return fmt.Errorf("`job_name` must be unique for each scrape_config, "+ + "a duplicate `job_name` of %s was found", cfg.JobName) + } + jobNames[cfg.JobName] = struct{}{} + + scrapeConfigs[i].JobName = strutil.SanitizeLabelName(cfg.JobName) + } + return nil +} diff --git a/clients/pkg/promtail/scrapeconfig/scrapeconfig_test.go b/clients/pkg/promtail/scrapeconfig/scrapeconfig_test.go index f8898d86aa591..f098aae9a6392 100644 --- a/clients/pkg/promtail/scrapeconfig/scrapeconfig_test.go +++ b/clients/pkg/promtail/scrapeconfig/scrapeconfig_test.go @@ -142,3 +142,68 @@ func TestLoadConfig(t *testing.T) { require.NotZero(t, len(config.PipelineStages)) } + +func Test_validateJobName(t *testing.T) { + tests := []struct { + name string + configs []Config + // Only validated against the first job in the provided scrape configs + expectedJob string + wantErr bool + }{ + { + name: "valid with spaces removed", + configs: []Config{ + { + JobName: "jobby job job", + }, + }, + wantErr: false, + expectedJob: "jobby_job_job", + }, + { + name: "missing job", + configs: []Config{ + {}, + }, + wantErr: true, + }, + { + name: "duplicate job", + configs: []Config{ + { + JobName: "job1", + }, + { + JobName: "job1", + }, + }, + wantErr: true, + }, + { + name: "validate with special characters", + configs: []Config{ + { + JobName: "job$1-2!3@4*job", + }, + }, + wantErr: false, + expectedJob: "job_1_2_3_4_job", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateJobName(tt.configs) + if (err != nil) != tt.wantErr { + t.Errorf("validateJobName() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !tt.wantErr { + if tt.configs[0].JobName != tt.expectedJob { + t.Errorf("Expected to find a job with name %v but did not find it", tt.expectedJob) + return + } + } + }) + } +} From 44c04e49681a95f8d36837018e7f5ecf15cc9ef6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Gouteroux?= Date: Tue, 27 Aug 2024 16:04:24 +0200 Subject: [PATCH 2/3] Revert "fix: validate scrape_config job name" This reverts commit c0202733ecbe7b64dc9a0ca02534e9f67ef261c7. --- clients/pkg/promtail/promtail.go | 9 +-- .../pkg/promtail/scrapeconfig/scrapeconfig.go | 21 ------ .../scrapeconfig/scrapeconfig_test.go | 65 ------------------- 3 files changed, 1 insertion(+), 94 deletions(-) diff --git a/clients/pkg/promtail/promtail.go b/clients/pkg/promtail/promtail.go index 65b543c05e84e..73e52f21703e1 100644 --- a/clients/pkg/promtail/promtail.go +++ b/clients/pkg/promtail/promtail.go @@ -18,7 +18,6 @@ import ( "github.com/grafana/loki/v3/clients/pkg/promtail/api" "github.com/grafana/loki/v3/clients/pkg/promtail/client" "github.com/grafana/loki/v3/clients/pkg/promtail/config" - "github.com/grafana/loki/v3/clients/pkg/promtail/scrapeconfig" "github.com/grafana/loki/v3/clients/pkg/promtail/server" "github.com/grafana/loki/v3/clients/pkg/promtail/targets" "github.com/grafana/loki/v3/clients/pkg/promtail/targets/target" @@ -140,16 +139,10 @@ func (p *Promtail) reloadConfig(cfg *config.Config) error { } cfg.Setup(p.logger) - var err error - err = scrapeconfig.ValidateJobName(cfg.ScrapeConfig) - if err != nil { - return err - } - if cfg.LimitsConfig.ReadlineRateEnabled { stages.SetReadLineRateLimiter(cfg.LimitsConfig.ReadlineRate, cfg.LimitsConfig.ReadlineBurst, cfg.LimitsConfig.ReadlineRateDrop) } - + var err error // entryHandlers contains all sinks were scraped log entries should get to var entryHandlers = []api.EntryHandler{} diff --git a/clients/pkg/promtail/scrapeconfig/scrapeconfig.go b/clients/pkg/promtail/scrapeconfig/scrapeconfig.go index 67b4f0898034d..b2466b83791e1 100644 --- a/clients/pkg/promtail/scrapeconfig/scrapeconfig.go +++ b/clients/pkg/promtail/scrapeconfig/scrapeconfig.go @@ -1,7 +1,6 @@ package scrapeconfig import ( - "errors" "fmt" "reflect" "time" @@ -27,7 +26,6 @@ import ( "github.com/prometheus/prometheus/discovery/triton" "github.com/prometheus/prometheus/discovery/zookeeper" "github.com/prometheus/prometheus/model/relabel" - "github.com/prometheus/prometheus/util/strutil" "github.com/grafana/loki/v3/clients/pkg/logentry/stages" "github.com/grafana/loki/v3/clients/pkg/promtail/discovery/consulagent" @@ -486,22 +484,3 @@ func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error { return nil } - -func ValidateJobName(scrapeConfigs []Config) error { - jobNames := map[string]struct{}{} - for i, cfg := range scrapeConfigs { - if cfg.JobName == "" { - return errors.New("`job_name` must be defined for the scrape_config with a " + - "unique name, " + - "at least one scrape_config has no `job_name` defined") - } - if _, ok := jobNames[cfg.JobName]; ok { - return fmt.Errorf("`job_name` must be unique for each scrape_config, "+ - "a duplicate `job_name` of %s was found", cfg.JobName) - } - jobNames[cfg.JobName] = struct{}{} - - scrapeConfigs[i].JobName = strutil.SanitizeLabelName(cfg.JobName) - } - return nil -} diff --git a/clients/pkg/promtail/scrapeconfig/scrapeconfig_test.go b/clients/pkg/promtail/scrapeconfig/scrapeconfig_test.go index f098aae9a6392..f8898d86aa591 100644 --- a/clients/pkg/promtail/scrapeconfig/scrapeconfig_test.go +++ b/clients/pkg/promtail/scrapeconfig/scrapeconfig_test.go @@ -142,68 +142,3 @@ func TestLoadConfig(t *testing.T) { require.NotZero(t, len(config.PipelineStages)) } - -func Test_validateJobName(t *testing.T) { - tests := []struct { - name string - configs []Config - // Only validated against the first job in the provided scrape configs - expectedJob string - wantErr bool - }{ - { - name: "valid with spaces removed", - configs: []Config{ - { - JobName: "jobby job job", - }, - }, - wantErr: false, - expectedJob: "jobby_job_job", - }, - { - name: "missing job", - configs: []Config{ - {}, - }, - wantErr: true, - }, - { - name: "duplicate job", - configs: []Config{ - { - JobName: "job1", - }, - { - JobName: "job1", - }, - }, - wantErr: true, - }, - { - name: "validate with special characters", - configs: []Config{ - { - JobName: "job$1-2!3@4*job", - }, - }, - wantErr: false, - expectedJob: "job_1_2_3_4_job", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := ValidateJobName(tt.configs) - if (err != nil) != tt.wantErr { - t.Errorf("validateJobName() error = %v, wantErr %v", err, tt.wantErr) - return - } - if !tt.wantErr { - if tt.configs[0].JobName != tt.expectedJob { - t.Errorf("Expected to find a job with name %v but did not find it", tt.expectedJob) - return - } - } - }) - } -} From 99f6af6706bd5c272e783689deee5a15d1c3d1cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Gouteroux?= Date: Tue, 27 Aug 2024 16:15:44 +0200 Subject: [PATCH 3/3] fix: validate unique name in unmarshal function --- clients/pkg/promtail/config/config.go | 22 ++++++++++++++ clients/pkg/promtail/config/config_test.go | 35 ++++++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/clients/pkg/promtail/config/config.go b/clients/pkg/promtail/config/config.go index 615b8e9abaad5..0454a8facf494 100644 --- a/clients/pkg/promtail/config/config.go +++ b/clients/pkg/promtail/config/config.go @@ -40,6 +40,28 @@ type Config struct { WAL wal.Config `yaml:"wal"` } +// UnmarshalYAML implements the yaml.Unmarshaler interface. +func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error { + *c = Config{} + // We want to set c to the defaults and then overwrite it with the input. + // To make unmarshal fill the plain data struct rather than calling UnmarshalYAML + // again, we have to hide it using a type indirection. + type plain Config + if err := unmarshal((*plain)(c)); err != nil { + return err + } + + // Validate unique names. + jobNames := map[string]struct{}{} + for _, j := range c.ScrapeConfig { + if _, ok := jobNames[j.JobName]; ok { + return fmt.Errorf("found multiple scrape configs with job name %q", j.JobName) + } + jobNames[j.JobName] = struct{}{} + } + return nil +} + // RegisterFlags with prefix registers flags where every name is prefixed by // prefix. If prefix is a non-empty string, prefix should end with a period. func (c *Config) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) { diff --git a/clients/pkg/promtail/config/config_test.go b/clients/pkg/promtail/config/config_test.go index 32bab70501e39..a812dd984abc7 100644 --- a/clients/pkg/promtail/config/config_test.go +++ b/clients/pkg/promtail/config/config_test.go @@ -47,12 +47,47 @@ clients: name: value ` +const testDuplicateJobsName = ` +clients: + - external_labels: + cluster: dev1 + url: https://1:shh@example.com/loki/api/v1/push + - external_labels: + cluster: prod1 + url: https://1:shh@example.com/loki/api/v1/push +scrape_configs: + - job_name: kubernetes-pods-name + kubernetes_sd_configs: + - role: pod + - job_name: system + static_configs: + - targets: + - localhost + labels: + job: varlogs + - job_name: system + static_configs: + - targets: + - localhost + labels: + job: varlogs2 +limits_config: + readline_rate: 100 + readline_burst: 200 +` + func Test_Load(t *testing.T) { var dst Config err := yaml.Unmarshal([]byte(testFile), &dst) require.Nil(t, err) } +func Test_Load_DuplicateJobsName(t *testing.T) { + var dst Config + err := yaml.Unmarshal([]byte(testDuplicateJobsName), &dst) + require.ErrorContains(t, err, `found multiple scrape configs with job name "system"`) +} + func TestHeadersConfigLoad(t *testing.T) { var dst Config err := yaml.Unmarshal([]byte(headersTestFile), &dst)