diff --git a/managed/cmd/pmm-managed/main.go b/managed/cmd/pmm-managed/main.go index 90f6eb5fcc..b940bb3f2a 100644 --- a/managed/cmd/pmm-managed/main.go +++ b/managed/cmd/pmm-managed/main.go @@ -729,6 +729,7 @@ func main() { //nolint:gocognit,maintidx,cyclop clickhouseAddrF := kingpin.Flag("clickhouse-addr", "Clickhouse database address").Default("127.0.0.1:9000").Envar("PMM_CLICKHOUSE_ADDR").String() clickhouseUsernameF := kingpin.Flag("clickhouse-username", "Clickhouse database user").Default("default").Envar("PMM_CLICKHOUSE_USER").String() clickhousePasswordF := kingpin.Flag("clickhouse-password", "Clickhouse database user password").Default("clickhouse").Envar("PMM_CLICKHOUSE_PASSWORD").String() + clickhouseBuiltinDisabledF := kingpin.Flag("clickhouse-disable-builtin", "Disable the built-in ClickHouse").Envar("PMM_DISABLE_BUILTIN_CLICKHOUSE").Bool() watchtowerHostF := kingpin.Flag("watchtower-host", "Watchtower host").Default("http://watchtower:8080").Envar("PMM_WATCHTOWER_HOST").URL() @@ -824,15 +825,19 @@ func main() { //nolint:gocognit,maintidx,cyclop grafanadb.DSN.DB = "grafana" grafanadb.DSN.Params = q.Encode() - chURI := url.URL{ - Scheme: "tcp", - User: url.UserPassword(*clickhouseUsernameF, *clickhousePasswordF), - Host: *clickhouseAddrF, - Path: *clickHouseDatabaseF, + chParams, err := models.NewClickHouseParams( + *clickhouseAddrF, + *clickHouseDatabaseF, + *clickhouseUsernameF, + *clickhousePasswordF, + *clickhouseBuiltinDisabledF, + ) + if err != nil { + l.Panicf("cannot load clickhouse params: %+v", err) } qanDB := ds.QanDBSelect - qanDB.DSN = chURI.String() + qanDB.DSN = chParams.URL().String() ds.VM.Address = *victoriaMetricsURLF @@ -882,7 +887,7 @@ func main() { //nolint:gocognit,maintidx,cyclop cleaner := clean.New(db) externalRules := vmalert.NewExternalRules() - vmdb, err := victoriametrics.NewVictoriaMetrics(*victoriaMetricsConfigF, db, vmParams, haService) + vmdb, err := victoriametrics.NewVictoriaMetrics(*victoriaMetricsConfigF, db, vmParams, chParams, haService) if err != nil { l.Panicf("VictoriaMetrics service problem: %+v", err) } @@ -1022,7 +1027,7 @@ func main() { //nolint:gocognit,maintidx,cyclop versionCache := versioncache.New(db, versioner) dumpService := dump.New(db, &dump.URLs{ - ClickhouseURL: chURI.String(), + ClickhouseURL: chParams.URL().String(), VMURL: *victoriaMetricsURLF, }) diff --git a/managed/models/clickhouse_params.go b/managed/models/clickhouse_params.go new file mode 100644 index 0000000000..3b920f3373 --- /dev/null +++ b/managed/models/clickhouse_params.go @@ -0,0 +1,74 @@ +// Copyright (C) 2023 Percona LLC +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package models + +import ( + "fmt" + "net" + "net/url" + "strconv" +) + +// ClickHouseParams represents ClickHouse server params. +type ClickHouseParams struct { + BuiltinDisabled bool + url *url.URL +} + +// ExternalClickHouse returns true if ClickHouse is configured externally. +func (p *ClickHouseParams) ExternalClickHouse() bool { + return p.url.Hostname() != "127.0.0.1" +} + +// URL returns the ClickHouse URL. +func (p *ClickHouseParams) URL() *url.URL { + u := *p.url + return &u +} + +// NewClickHouseParams returns validated ClickHouse configuration params, +// or an error if any required field is missing or malformed. +func NewClickHouseParams(addr, dbName, dbUsername, dbPassword string, builtinDisabled bool) (*ClickHouseParams, error) { + if addr == "" { + return nil, fmt.Errorf("addr is required") + } + host, port, err := net.SplitHostPort(addr) + if err != nil { + return nil, fmt.Errorf("invalid addr %q: %w", addr, err) + } + if host == "" { + return nil, fmt.Errorf("invalid addr %q: empty host", addr) + } + if _, err := strconv.ParseUint(port, 10, 16); err != nil { + return nil, fmt.Errorf("invalid port in addr %q: %w", addr, err) + } + if dbName == "" { + return nil, fmt.Errorf("database name is required") + } + if dbUsername == "" { + return nil, fmt.Errorf("username is required") + } + + return &ClickHouseParams{ + BuiltinDisabled: builtinDisabled, + url: &url.URL{ + Scheme: "tcp", + User: url.UserPassword(dbUsername, dbPassword), + Host: addr, + Path: dbName, + }, + }, nil +} diff --git a/managed/models/clickhouse_params_test.go b/managed/models/clickhouse_params_test.go new file mode 100644 index 0000000000..cabbc4ee71 --- /dev/null +++ b/managed/models/clickhouse_params_test.go @@ -0,0 +1,101 @@ +// Copyright (C) 2023 Percona LLC +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package models + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNewClickHouseParams(t *testing.T) { + t.Run("valid", func(t *testing.T) { + p, err := NewClickHouseParams("127.0.0.1:9000", "pmm", "default", "clickhouse", false) + require.NoError(t, err) + assert.Equal(t, "tcp://default:clickhouse@127.0.0.1:9000/pmm", p.URL().String()) + assert.False(t, p.BuiltinDisabled) + }) + + t.Run("valid builtin disabled", func(t *testing.T) { + p, err := NewClickHouseParams("127.0.0.1:9000", "pmm", "default", "clickhouse", true) + require.NoError(t, err) + assert.True(t, p.BuiltinDisabled) + }) + + t.Run("valid empty password", func(t *testing.T) { + _, err := NewClickHouseParams("127.0.0.1:9000", "pmm", "default", "", false) + require.NoError(t, err) + }) + + errCases := []struct { + name string + addr string + dbName string + dbUsername string + dbPassword string + wantErrSub string + }{ + {"empty addr", "", "pmm", "default", "clickhouse", "addr is required"}, + {"missing port", "127.0.0.1", "pmm", "default", "clickhouse", "invalid addr"}, + {"empty host", ":9000", "pmm", "default", "clickhouse", "empty host"}, + {"non numeric port", "localhost:abc", "pmm", "default", "clickhouse", "invalid port"}, + {"port out of range", "localhost:99999", "pmm", "default", "clickhouse", "invalid port"}, + {"empty db name", "127.0.0.1:9000", "", "default", "clickhouse", "database name is required"}, + {"empty username", "127.0.0.1:9000", "pmm", "", "clickhouse", "username is required"}, + } + for _, tc := range errCases { + t.Run(tc.name, func(t *testing.T) { + _, err := NewClickHouseParams(tc.addr, tc.dbName, tc.dbUsername, tc.dbPassword, false) + require.Error(t, err) + assert.ErrorContains(t, err, tc.wantErrSub) + }) + } +} + +func TestCHParamsExternalClickHouse(t *testing.T) { + cases := []struct { + name string + addr string + want bool + }{ + {"loopback", "127.0.0.1:9000", false}, + {"localhost", "localhost:9000", true}, + {"external host", "ch-01.test.net:9000", true}, + {"wildcard", "0.0.0.0:9000", true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + p, err := NewClickHouseParams(tc.addr, "pmm", "default", "clickhouse", false) + require.NoError(t, err) + assert.Equal(t, tc.want, p.ExternalClickHouse()) + }) + } +} + +func TestCHParamsURL(t *testing.T) { + t.Run("simple", func(t *testing.T) { + p, err := NewClickHouseParams("127.0.0.1:9000", "pmm", "default", "clickhouse", false) + require.NoError(t, err) + assert.Equal(t, "tcp://default:clickhouse@127.0.0.1:9000/pmm", p.URL().String()) + }) + + t.Run("password with special chars", func(t *testing.T) { + p, err := NewClickHouseParams("127.0.0.1:9000", "pmm", "default", "p@ss/word", false) + require.NoError(t, err) + assert.Equal(t, "tcp://default:p%40ss%2Fword@127.0.0.1:9000/pmm", p.URL().String()) + }) +} diff --git a/managed/services/victoriametrics/prometheus.go b/managed/services/victoriametrics/prometheus.go index 8026645881..1aeb44f249 100644 --- a/managed/services/victoriametrics/prometheus.go +++ b/managed/services/victoriametrics/prometheus.go @@ -87,7 +87,7 @@ func AddScrapeConfigs(l *logrus.Entry, cfg *config.Config, q *reform.Querier, // } switch { case pushMetrics: - paramsHost = "127.0.0.1" + paramsHost = localhost case agent.PMMAgentID != nil: pmmAgentNode = &models.Node{NodeID: pointer.GetString(pmmAgent.RunsOnNodeID)} if err = q.Reload(pmmAgentNode); err != nil { @@ -273,6 +273,20 @@ func addInternalServicesToScrape(s models.MetricsResolutions, svc *Service, pmmS } if svc.params.ExternalVM() { + svc.l.Infof("Skip scrape config for ClickHouse, VictoriaMetrics is configured to run externally.") + return cfg + } + + if svc.chParams.BuiltinDisabled { + svc.l.Infof("Skip scrape config for ClickHouse, scraping has been disabled for builtin clickhouse.") + return cfg + } + + if svc.chParams.ExternalClickHouse() { + svc.l.Warnf( + "External ClickHouse detected (%s); skipping built-in ClickHouse scrape (127.0.0.1:9363). Set PMM_DISABLE_BUILTIN_CLICKHOUSE=true to make this explicit.", + svc.chParams.URL().Redacted(), + ) return cfg } diff --git a/managed/services/victoriametrics/scrape_configs.go b/managed/services/victoriametrics/scrape_configs.go index 32ff757067..5887620d5c 100644 --- a/managed/services/victoriametrics/scrape_configs.go +++ b/managed/services/victoriametrics/scrape_configs.go @@ -55,7 +55,7 @@ func scrapeConfigForClickhouse(mr time.Duration, pmmServerNodeName string) *conf MetricsPath: "/metrics", ServiceDiscoveryConfig: config.ServiceDiscoveryConfig{ StaticConfigs: []*config.Group{{ - Targets: []string{"127.0.0.1:9363"}, + Targets: []string{localhost + ":9363"}, Labels: map[string]string{"instance": pmmServerNodeName}, }}, }, @@ -70,7 +70,7 @@ func scrapeConfigForGrafana(interval time.Duration, pmmServerNodeName string) *c MetricsPath: "/graph/metrics", ServiceDiscoveryConfig: config.ServiceDiscoveryConfig{ StaticConfigs: []*config.Group{{ - Targets: []string{"127.0.0.1:3000"}, + Targets: []string{localhost + ":3000"}, Labels: map[string]string{"instance": pmmServerNodeName}, }}, }, @@ -85,7 +85,7 @@ func scrapeConfigForPMMManaged(interval time.Duration, pmmServerNodeName string) MetricsPath: "/debug/metrics", ServiceDiscoveryConfig: config.ServiceDiscoveryConfig{ StaticConfigs: []*config.Group{{ - Targets: []string{"127.0.0.1:7773"}, + Targets: []string{localhost + ":7773"}, Labels: map[string]string{"instance": pmmServerNodeName}, }}, }, @@ -100,7 +100,7 @@ func scrapeConfigForQANAPI2(interval time.Duration, pmmServerNodeName string) *c MetricsPath: "/debug/metrics", ServiceDiscoveryConfig: config.ServiceDiscoveryConfig{ StaticConfigs: []*config.Group{{ - Targets: []string{"127.0.0.1:9933"}, + Targets: []string{localhost + ":9933"}, Labels: map[string]string{"instance": pmmServerNodeName}, }}, }, @@ -116,7 +116,7 @@ func scrapeConfigForNomadServer(resolution time.Duration, pmmServerNodeName stri Scheme: "https", ServiceDiscoveryConfig: config.ServiceDiscoveryConfig{ StaticConfigs: []*config.Group{{ - Targets: []string{"127.0.0.1:4646"}, + Targets: []string{localhost + ":4646"}, Labels: map[string]string{"instance": pmmServerNodeName}, }}, }, diff --git a/managed/services/victoriametrics/victoriametrics.go b/managed/services/victoriametrics/victoriametrics.go index ce4277027d..b28902a5b3 100644 --- a/managed/services/victoriametrics/victoriametrics.go +++ b/managed/services/victoriametrics/victoriametrics.go @@ -49,6 +49,8 @@ const ( victoriametricsDir = "/srv/victoriametrics" victoriametricsDataDir = "/srv/victoriametrics/data" dirPerm = os.FileMode(0o775) + + localhost = "127.0.0.1" ) var checkFailedRE = regexp.MustCompile(`(?s)cannot unmarshal data: (.+)`) @@ -60,7 +62,8 @@ type Service struct { baseURL *url.URL client *http.Client - params *models.VictoriaMetricsParams + params *models.VictoriaMetricsParams + chParams *models.ClickHouseParams l *logrus.Entry reloadCh chan struct{} @@ -68,11 +71,20 @@ type Service struct { } // NewVictoriaMetrics creates new VictoriaMetrics service. -func NewVictoriaMetrics(scrapeConfigPath string, db *reform.DB, params *models.VictoriaMetricsParams, haService haService) (*Service, error) { +func NewVictoriaMetrics( + scrapeConfigPath string, + db *reform.DB, + params *models.VictoriaMetricsParams, + chParams *models.ClickHouseParams, + haService haService, +) (*Service, error) { u, err := url.Parse(params.URL()) if err != nil { return nil, err } + if chParams == nil { + return nil, fmt.Errorf("ClickHouseParams is required") + } return &Service{ scrapeConfigPath: scrapeConfigPath, @@ -80,6 +92,7 @@ func NewVictoriaMetrics(scrapeConfigPath string, db *reform.DB, params *models.V baseURL: u, client: &http.Client{}, // TODO instrument with utils/irt; see vmalert package https://jira.percona.com/browse/PMM-7229 params: params, + chParams: chParams, l: logrus.WithField("component", "victoriametrics"), reloadCh: make(chan struct{}, 1), haService: haService, @@ -435,7 +448,7 @@ func scrapeConfigForVMAlert(interval time.Duration, pmmServerNodeName string) *c ServiceDiscoveryConfig: config.ServiceDiscoveryConfig{ StaticConfigs: []*config.Group{ { - Targets: []string{"127.0.0.1:8880"}, + Targets: []string{localhost + ":8880"}, Labels: map[string]string{"instance": pmmServerNodeName}, }, }, diff --git a/managed/services/victoriametrics/victoriametrics_test.go b/managed/services/victoriametrics/victoriametrics_test.go index f21ccdf75f..449a113a07 100644 --- a/managed/services/victoriametrics/victoriametrics_test.go +++ b/managed/services/victoriametrics/victoriametrics_test.go @@ -45,10 +45,13 @@ func setup(t *testing.T) (*reform.DB, *Service, []byte) { vmParams, err := models.NewVictoriaMetricsParams(models.BasePrometheusConfigPath, models.VMBaseURL) check.NoError(err) + chParams, err := models.NewClickHouseParams("127.0.0.1:9000", "pmm", "default", "clickhouse", false) + check.NoError(err) + mockHaService := newMockHaService(t) mockHaService.On("Params").Return(&models.HAParams{Enabled: false, NodeID: "pmm-ha-service-0"}).Maybe() mockHaService.On("IsLeader").Return(true).Maybe() - svc, err := NewVictoriaMetrics(configPath, db, vmParams, mockHaService) + svc, err := NewVictoriaMetrics(configPath, db, vmParams, chParams, mockHaService) check.NoError(err) original, err := os.ReadFile(configPath) @@ -991,3 +994,68 @@ scrape_configs: assert.NoError(t, err) assert.Equal(t, expected, string(newcfg), "actual:\n%s", newcfg) } + +func TestVMConfig_OmitsClickhouseScrape(t *testing.T) { + newSvc := func(t *testing.T, chParams *models.ClickHouseParams, vmURL string) (*reform.DB, *Service) { + t.Helper() + sqlDB := testdb.Open(t, models.SkipFixtures, nil) + db := reform.NewDB(sqlDB, postgresql.Dialect, reform.NewPrintfLogger(t.Logf)) + vmParams, err := models.NewVictoriaMetricsParams(models.BasePrometheusConfigPath, vmURL) + require.NoError(t, err) + + mockHaService := newMockHaService(t) + mockHaService.On("Params").Return(&models.HAParams{Enabled: false, NodeID: "pmm-ha-service-0"}).Maybe() + mockHaService.On("IsLeader").Return(true).Maybe() + + svc, err := NewVictoriaMetrics(configPath, db, vmParams, chParams, mockHaService) + require.NoError(t, err) + require.NoError(t, svc.IsReady(t.Context())) + t.Cleanup(func() { _ = db.DBInterface().(*sql.DB).Close() }) + return db, svc + } + + newCHParams := func(t *testing.T, addr string, builtinDisabled bool) *models.ClickHouseParams { + t.Helper() + chp, err := models.NewClickHouseParams(addr, "pmm", "default", "clickhouse", builtinDisabled) + require.NoError(t, err) + return chp + } + + cases := []struct { + name string + addr string + builtinDisabled bool + vmURL string + wantClickhouse bool + }{ + {"internal enabled positive control", "127.0.0.1:9000", false, models.VMBaseURL, true}, + {"external addr, builtin not disabled (auto-skip)", "ch.external:9000", false, models.VMBaseURL, false}, + {"builtin disabled", "127.0.0.1:9000", true, models.VMBaseURL, false}, + {"external addr, builtin disabled", "ch.external:9000", true, models.VMBaseURL, false}, + {"external VM short-circuits CH scrape", "127.0.0.1:9000", false, "http://vm.external:9090/prometheus/", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, svc := newSvc(t, newCHParams(t, tc.addr, tc.builtinDisabled), tc.vmURL) + cfg, err := svc.marshalConfig(svc.loadBaseConfig()) + require.NoError(t, err) + if tc.wantClickhouse { + assert.Contains(t, string(cfg), "127.0.0.1:9363") + assert.Contains(t, string(cfg), "job_name: clickhouse") + } else { + assert.NotContains(t, string(cfg), "127.0.0.1:9363") + assert.NotContains(t, string(cfg), "job_name: clickhouse") + } + }) + } +} + +func TestNewVictoriaMetrics_NilClickHouseParams(t *testing.T) { + vmParams, err := models.NewVictoriaMetricsParams(models.BasePrometheusConfigPath, models.VMBaseURL) + require.NoError(t, err) + + svc, err := NewVictoriaMetrics(configPath, nil, vmParams, nil, newMockHaService(t)) + require.Error(t, err) + assert.Nil(t, svc) + assert.Contains(t, err.Error(), "ClickHouseParams is required") +}