Repository: VictoriaMetrics/VictoriaMetrics
Pull request #6820 (2024-08-15 20:21:33)

lib/httputils: parse URL before creating HTTP transport
https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6740

Haleygo: Also need to fix the address examples [here](https://github.com/search?q=repo%3AVictoriaMetrics%2FVictoriaMetrics%20(e.g.%2C%20127.0.0.&type=code). 
hagen1778: Thanks! Updated

Patch:
diff --git a/app/vmalert/datasource/init.go b/app/vmalert/datasource/init.go
index 74fb77935..a99f130e8 100644
--- a/app/vmalert/datasource/init.go
+++ b/app/vmalert/datasource/init.go
@@ -17,7 +17,7 @@ import (
 
 var (
 	addr = flag.String("datasource.url", "", "Datasource compatible with Prometheus HTTP API. It can be single node VictoriaMetrics or vmselect endpoint. Required parameter. "+
-		"Supports address in the form of IP address with a port (e.g., 127.0.0.1:8428) or DNS SRV record. "+
+		"Supports address in the form of IP address with a port (e.g., http://127.0.0.1:8428) or DNS SRV record. "+
 		"See also -remoteRead.disablePathAppend and -datasource.showURL")
 	appendTypePrefix  = flag.Bool("datasource.appendTypePrefix", false, "Whether to add type prefix to -datasource.url based on the query type. Set to true if sending different query types to the vmselect URL.")
 	showDatasourceURL = flag.Bool("datasource.showURL", false, "Whether to avoid stripping sensitive information such as auth headers or passwords from URLs in log messages or UI and exported metrics. "+
@@ -99,7 +99,7 @@ func Init(extraParams url.Values) (QuerierBuilder, error) {
 
 	tr, err := httputils.Transport(*addr, *tlsCertFile, *tlsKeyFile, *tlsCAFile, *tlsServerName, *tlsInsecureSkipVerify)
 	if err != nil {
-		return nil, fmt.Errorf("failed to create transport: %w", err)
+		return nil, fmt.Errorf("failed to create transport for -datasource.url=%q: %w", *addr, err)
 	}
 	tr.DialContext = netutil.NewStatDialFunc("vmalert_datasource")
 	tr.DisableKeepAlives = *disableKeepAlive
diff --git a/app/vmalert/notifier/alertmanager.go b/app/vmalert/notifier/alertmanager.go
index ff268dd99..8930256ef 100644
--- a/app/vmalert/notifier/alertmanager.go
+++ b/app/vmalert/notifier/alertmanager.go
@@ -132,7 +132,8 @@ func NewAlertManager(alertManagerURL string, fn AlertURLGenerator, authCfg proma
 	}
 	tr, err := httputils.Transport(alertManagerURL, tls.CertFile, tls.KeyFile, tls.CAFile, tls.ServerName, tls.InsecureSkipVerify)
 	if err != nil {
-		return nil, fmt.Errorf("failed to create transport: %w", err)
+		return nil, fmt.Errorf("failed to create transport for alertmanager URL=%q: %w", alertManagerURL, err)
+
 	}
 
 	ba := new(promauth.BasicAuthConfig)
diff --git a/app/vmalert/remoteread/init.go b/app/vmalert/remoteread/init.go
index c7ce0ea1d..612db0fad 100644
--- a/app/vmalert/remoteread/init.go
+++ b/app/vmalert/remoteread/init.go
@@ -17,7 +17,7 @@ var (
 	addr = flag.String("remoteRead.url", "", "Optional URL to datasource compatible with Prometheus HTTP API. It can be single node VictoriaMetrics or vmselect."+
 		"Remote read is used to restore alerts state."+
 		"This configuration makes sense only if `vmalert` was configured with `remoteWrite.url` before and has been successfully persisted its state. "+
-		"Supports address in the form of IP address with a port (e.g., 127.0.0.1:8428) or DNS SRV record. "+
+		"Supports address in the form of IP address with a port (e.g., http://127.0.0.1:8428) or DNS SRV record. "+
 		"See also '-remoteRead.disablePathAppend', '-remoteRead.showURL'.")
 
 	showRemoteReadURL = flag.Bool("remoteRead.showURL", false, "Whether to show -remoteRead.url in the exported metrics. "+
@@ -68,7 +68,7 @@ func Init() (datasource.QuerierBuilder, error) {
 	}
 	tr, err := httputils.Transport(*addr, *tlsCertFile, *tlsKeyFile, *tlsCAFile, *tlsServerName, *tlsInsecureSkipVerify)
 	if err != nil {
-		return nil, fmt.Errorf("failed to create transport: %w", err)
+		return nil, fmt.Errorf("failed to create transport for -remoteRead.url=%q: %w", *addr, err)
 	}
 	tr.IdleConnTimeout = *idleConnectionTimeout
 	tr.DialContext = netutil.NewStatDialFunc("vmalert_remoteread")
diff --git a/app/vmalert/remotewrite/debug_client.go b/app/vmalert/remotewrite/debug_client.go
index 17443c731..7a549907c 100644
--- a/app/vmalert/remotewrite/debug_client.go
+++ b/app/vmalert/remotewrite/debug_client.go
@@ -32,7 +32,7 @@ func NewDebugClient() (*DebugClient, error) {
 
 	t, err := httputils.Transport(*addr, *tlsCertFile, *tlsKeyFile, *tlsCAFile, *tlsServerName, *tlsInsecureSkipVerify)
 	if err != nil {
-		return nil, fmt.Errorf("failed to create transport: %w", err)
+		return nil, fmt.Errorf("failed to create transport for -remoteWrite.url=%q: %w", *addr, err)
 	}
 	c := &DebugClient{
 		c: &http.Client{
diff --git a/app/vmalert/remotewrite/init.go b/app/vmalert/remotewrite/init.go
index bd8352d91..412ce9e43 100644
--- a/app/vmalert/remotewrite/init.go
+++ b/app/vmalert/remotewrite/init.go
@@ -15,7 +15,7 @@ import (
 var (
 	addr = flag.String("remoteWrite.url", "", "Optional URL to VictoriaMetrics or vminsert where to persist alerts state "+
 		"and recording rules results in form of timeseries. "+
-		"Supports address in the form of IP address with a port (e.g., 127.0.0.1:8428) or DNS SRV record. "+
+		"Supports address in the form of IP address with a port (e.g., http://127.0.0.1:8428) or DNS SRV record. "+
 		"For example, if -remoteWrite.url=http://127.0.0.1:8428 is specified, "+
 		"then the alerts state will be written to http://127.0.0.1:8428/api/v1/write . See also -remoteWrite.disablePathAppend, '-remoteWrite.showURL'.")
 	showRemoteWriteURL = flag.Bool("remoteWrite.showURL", false, "Whether to show -remoteWrite.url in the exported metrics. "+
@@ -72,7 +72,7 @@ func Init(ctx context.Context) (*Client, error) {
 
 	t, err := httputils.Transport(*addr, *tlsCertFile, *tlsKeyFile, *tlsCAFile, *tlsServerName, *tlsInsecureSkipVerify)
 	if err != nil {
-		return nil, fmt.Errorf("failed to create transport: %w", err)
+		return nil, fmt.Errorf("failed to create transport for -remoteWrite.url=%q: %w", *addr, err)
 	}
 	t.IdleConnTimeout = *idleConnectionTimeout
 	t.DialContext = netutil.NewStatDialFunc("vmalert_remotewrite")
diff --git a/app/vmctl/main.go b/app/vmctl/main.go
index 3a07a6247..366444925 100644
--- a/app/vmctl/main.go
+++ b/app/vmctl/main.go
@@ -68,7 +68,7 @@ func main() {
 
 					tr, err := httputils.Transport(addr, certFile, keyFile, caFile, serverName, insecureSkipVerify)
 					if err != nil {
-						return fmt.Errorf("failed to create Transport: %s", err)
+						return fmt.Errorf("failed to create transport for -%s=%q: %s", otsdbAddr, addr, err)
 					}
 					oCfg := opentsdb.Config{
 						Addr:       addr,
@@ -180,7 +180,7 @@ func main() {
 
 					tr, err := httputils.Transport(addr, certFile, keyFile, caFile, serverName, insecureSkipVerify)
 					if err != nil {
-						return fmt.Errorf("failed to create transport: %s", err)
+						return fmt.Errorf("failed to create transport for -%s=%q: %s", remoteReadSrcAddr, addr, err)
 					}
 
 					rr, err := remoteread.NewClient(remoteread.Config{
@@ -438,7 +438,7 @@ func initConfigVM(c *cli.Context) (vm.Config, error) {
 
 	tr, err := httputils.Transport(addr, certFile, keyFile, caFile, serverName, insecureSkipVerify)
 	if err != nil {
-		return vm.Config{}, fmt.Errorf("failed to create Transport: %s", err)
+		return vm.Config{}, fmt.Errorf("failed to create transport for -%s=%q: %s", vmAddr, addr, err)
 	}
 
 	bfRetries := c.Int(vmBackoffRetries)
diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md
index 7b169e2b0..c986ce360 100644
--- a/docs/CHANGELOG.md
+++ b/docs/CHANGELOG.md
@@ -44,6 +44,7 @@ See also [LTS releases](https://docs.victoriametrics.com/lts-releases/).
 * BUGFIX: [vmagent](https://docs.victoriametrics.com/vmagent/): account for `-usePromCompatibleNaming` cmd-line flag during when pushing data to remote storages. Thanks to @12345XXX  for the [pull request](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/6776).
 * BUGFIX: `vminsert` in [VictoriaMetrics cluster](https://docs.victoriametrics.com/cluster-victoriametrics/): reduce CPU usage by limiting the number of concurrently running inserts. The issue was introduced in [this commit](https://github.com/VictoriaMetrics/VictoriaMetrics/commit/498fe1cfa523be5bfecaa372293c3cded85e75ab) starting from v1.101.0. See [this](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6733) issue for details.
 * BUGFIX: [MetricsQL](https://docs.victoriametrics.com/metricsql/): fix calculation [histogram_quantile](https://docs.victoriametrics.com/metricsql/#histogram_quantile) over Prometheus buckets with inconsistent values. It was producing incorrect results in case lower buckets. The issue was introduced in [v1.102.0](https://docs.victoriametrics.com/changelog/#v11020) release, see [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6714) for the details.
+* BUGFIX: [vmalert](https://docs.victoriametrics.com/vmalert/), [vmctl](https://docs.victoriametrics.com/vmctl/) and snapshot API: verify correctness of URLs provided via cmd-line flags before executing HTTP requests. See [this](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6740) issue for details.
 
 ## [v1.102.1](https://github.com/VictoriaMetrics/VictoriaMetrics/releases/tag/v1.102.1)
 
diff --git a/docs/vmalert.md b/docs/vmalert.md
index 11ded4093..e2be95c61 100644
--- a/docs/vmalert.md
+++ b/docs/vmalert.md
@@ -1075,7 +1075,7 @@ The shortlist of configuration flags is the following:
   -datasource.tlsServerName string
      Optional TLS server name to use for connections to -datasource.url. By default, the server name from -datasource.url is used
   -datasource.url string
-     Datasource compatible with Prometheus HTTP API. It can be single node VictoriaMetrics or vmselect URL. Required parameter. Supports address in the form of IP address with a port (e.g., 127.0.0.1:8428) or DNS SRV record. See also -remoteRead.disablePathAppend and -datasource.showURL
+     Datasource compatible with Prometheus HTTP API. It can be single node VictoriaMetrics or vmselect URL. Required parameter. Supports address in the form of IP address with a port (e.g., http://127.0.0.1:8428) or DNS SRV record. See also -remoteRead.disablePathAppend and -datasource.showURL
   -defaultTenant.graphite string
      Default tenant for Graphite alerting groups. See https://docs.victoriametrics.com/vmalert/#multitenancy .This flag is available only in Enterprise binaries. See https://docs.victoriametrics.com/enterprise/
   -defaultTenant.prometheus string
@@ -1341,7 +1341,7 @@ The shortlist of configuration flags is the following:
   -remoteRead.tlsServerName string
      Optional TLS server name to use for connections to -remoteRead.url. By default, the server name from -remoteRead.url is used
   -remoteRead.url vmalert
-     Optional URL to datasource compatible with Prometheus HTTP API. It can be single node VictoriaMetrics or vmselect.Remote read is used to restore alerts state.This configuration makes sense only if vmalert was configured with `remoteWrite.url` before and has been successfully persisted its state. Supports address in the form of IP address with a port (e.g., 127.0.0.1:8428) or DNS SRV record. See also '-remoteRead.disablePathAppend', '-remoteRead.showURL'.
+     Optional URL to datasource compatible with Prometheus HTTP API. It can be single node VictoriaMetrics or vmselect.Remote read is used to restore alerts state.This configuration makes sense only if vmalert was configured with `remoteWrite.url` before and has been successfully persisted its state. Supports address in the form of IP address with a port (e.g., http://127.0.0.1:8428) or DNS SRV record. See also '-remoteRead.disablePathAppend', '-remoteRead.showURL'.
   -remoteWrite.basicAuth.password string
      Optional basic auth password for -remoteWrite.url
   -remoteWrite.basicAuth.passwordFile string
@@ -1397,7 +1397,7 @@ The shortlist of configuration flags is the following:
   -remoteWrite.tlsServerName string
      Optional TLS server name to use for connections to -remoteWrite.url. By default, the server name from -remoteWrite.url is used
   -remoteWrite.url string
-     Optional URL to VictoriaMetrics or vminsert where to persist alerts state and recording rules results in form of timeseries. Supports address in the form of IP address with a port (e.g., 127.0.0.1:8428) or DNS SRV record. For example, if -remoteWrite.url=http://127.0.0.1:8428 is specified, then the alerts state will be written to http://127.0.0.1:8428/api/v1/write . See also -remoteWrite.disablePathAppend, '-remoteWrite.showURL'.
+     Optional URL to VictoriaMetrics or vminsert where to persist alerts state and recording rules results in form of timeseries. Supports address in the form of IP address with a port (e.g., http://127.0.0.1:8428) or DNS SRV record. For example, if -remoteWrite.url=http://127.0.0.1:8428 is specified, then the alerts state will be written to http://127.0.0.1:8428/api/v1/write . See also -remoteWrite.disablePathAppend, '-remoteWrite.showURL'.
   -replay.disableProgressBar
      Whether to disable rendering progress bars during the replay. Progress bar rendering might be verbose or break the logs parsing, so it is recommended to be disabled when not used in interactive mode.
   -replay.maxDatapointsPerQuery /query_range
diff --git a/lib/httputils/tls.go b/lib/httputils/tls.go
index 35f32b89c..6b7f652a0 100644
--- a/lib/httputils/tls.go
+++ b/lib/httputils/tls.go
@@ -5,6 +5,7 @@ import (
 	"crypto/x509"
 	"fmt"
 	"net/http"
+	"net/url"
 	"os"
 	"strings"
 )
@@ -12,6 +13,10 @@ import (
 // Transport creates http.Transport object based on provided URL.
 // Returns Transport with TLS configuration if URL contains `https` prefix
 func Transport(URL, certFile, keyFile, caFile, serverName string, insecureSkipVerify bool) (*http.Transport, error) {
+	_, err := url.Parse(URL)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse URL: %w", err)
+	}
 	t := http.DefaultTransport.(*http.Transport).Clone()
 	if !strings.HasPrefix(URL, "https") {
 		return t, nil
diff --git a/lib/snapshot/snapshot.go b/lib/snapshot/snapshot.go
index 87bf8d09f..e8925f042 100644
--- a/lib/snapshot/snapshot.go
+++ b/lib/snapshot/snapshot.go
@@ -38,7 +38,7 @@ func Create(createSnapshotURL string) (string, error) {
 	// create Transport
 	tr, err := httputils.Transport(createSnapshotURL, *tlsCertFile, *tlsKeyFile, *tlsCAFile, *tlsServerName, *tlsInsecureSkipVerify)
 	if err != nil {
-		return "", err
+		return "", fmt.Errorf("failed to create transport for createSnapshotURL=%q: %s", createSnapshotURL, err)
 	}
 	hc := &http.Client{Transport: tr}
 
@@ -83,7 +83,8 @@ func Delete(deleteSnapshotURL string, snapshotName string) error {
 	// create Transport
 	tr, err := httputils.Transport(deleteSnapshotURL, *tlsCertFile, *tlsKeyFile, *tlsCAFile, *tlsServerName, *tlsInsecureSkipVerify)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to create transport for deleteSnapshotURL=%q: %s", deleteSnapshotURL, err)
+
 	}
 	hc := &http.Client{Transport: tr}
 	resp, err := hc.PostForm(u.String(), formData)


Test patch:
diff --git a/lib/httputils/tls_test.go b/lib/httputils/tls_test.go
index a27669e0b..96b6e2a5e 100644
--- a/lib/httputils/tls_test.go
+++ b/lib/httputils/tls_test.go
@@ -49,4 +49,10 @@ func TestTransport(t *testing.T) {
 	if tr.TLSClientConfig == nil {
 		t.Fatalf("expected TLSClientConfig to be set, got nil")
 	}
+
+	noSchemaURL := "127.0.0.1:8880"
+	_, err = Transport(noSchemaURL, certFile, keyFile, CAFile, serverName, insecureSkipVerify)
+	if err == nil {
+		t.Fatalf("expected to have parse error for URL without specified schema; got nil instead")
+	}
 }