Revert "app/vmbackup: introduce new flag type URL (#6152)"

This reverts commit 029060af60.

app/vmbackup/main.go

@@ -3,6 +3,7 @@ package main
 import (
 	"flag"
 	"fmt"
+	"net/url"
 	"os"
 	"path/filepath"
 	"strings"
@@ -26,9 +27,9 @@ var (
 	httpListenAddr    = flag.String("httpListenAddr", ":8420", "TCP address for exporting metrics at /metrics page")
 	storageDataPath   = flag.String("storageDataPath", "victoria-metrics-data", "Path to VictoriaMetrics data. Must match -storageDataPath from VictoriaMetrics or vmstorage")
 	snapshotName      = flag.String("snapshotName", "", "Name for the snapshot to backup. See https://docs.victoriametrics.com/single-server-victoriametrics/#how-to-work-with-snapshots. There is no need in setting -snapshotName if -snapshot.createURL is set")
-	snapshotCreateURL = flagutil.NewURL("snapshot.createURL", "VictoriaMetrics create snapshot url. When this is given a snapshot will automatically be created during backup. "+
+	snapshotCreateURL = flag.String("snapshot.createURL", "", "VictoriaMetrics create snapshot url. When this is given a snapshot will automatically be created during backup. "+
 		"Example: http://victoriametrics:8428/snapshot/create . There is no need in setting -snapshotName if -snapshot.createURL is set")
-	snapshotDeleteURL = flagutil.NewURL("snapshot.deleteURL", "VictoriaMetrics delete snapshot url. Optional. Will be generated from -snapshot.createURL if not provided. "+
+	snapshotDeleteURL = flag.String("snapshot.deleteURL", "", "VictoriaMetrics delete snapshot url. Optional. Will be generated from -snapshot.createURL if not provided. "+
 		"All created snapshots will be automatically deleted. Example: http://victoriametrics:8428/snapshot/delete")
 	dst = flag.String("dst", "", "Where to put the backup on the remote storage. "+
 		"Example: gs://bucket/path/to/backup, s3://bucket/path/to/backup, azblob://container/path/to/backup or fs:///path/to/local/backup/dir\n"+
@@ -54,23 +55,31 @@ func main() {
 	// See https://github.com/VictoriaMetrics/VictoriaMetrics/issues/2055
 	deleteSnapshot := func() {}
 
-	if len(snapshotCreateURL.String()) > 0 {
+	if len(*snapshotCreateURL) > 0 {
+		// create net/url object
+		createURL, err := url.Parse(*snapshotCreateURL)
+		if err != nil {
+			logger.Fatalf("cannot parse snapshotCreateURL: %s", err)
+		}
 		if len(*snapshotName) > 0 {
 			logger.Fatalf("-snapshotName shouldn't be set if -snapshot.createURL is set, since snapshots are created automatically in this case")
 		}
-
+		logger.Infof("Snapshot create url %s", createURL.Redacted())
-		logger.Infof("Snapshot create url %s", snapshotCreateURL.String())
+		if len(*snapshotDeleteURL) <= 0 {
-		if len(snapshotDeleteURL.String()) <= 0 {
+			err := flag.Set("snapshot.deleteURL", strings.Replace(*snapshotCreateURL, "/create", "/delete", 1))
-			err := flag.Set("snapshot.deleteURL", strings.Replace(snapshotCreateURL.Get(), "/create", "/delete", 1))
 			if err != nil {
 				logger.Fatalf("Failed to set snapshot.deleteURL flag: %v", err)
 			}
 		}
-
+		deleteURL, err := url.Parse(*snapshotDeleteURL)
-		logger.Infof("Snapshot delete url %s", snapshotDeleteURL)
-		name, err := snapshot.Create(snapshotCreateURL.Get())
 		if err != nil {
-			logger.Fatalf("cannot create snapshot via %q: %s", snapshotCreateURL, err)
+			logger.Fatalf("cannot parse snapshotDeleteURL: %s", err)
+		}
+		logger.Infof("Snapshot delete url %s", deleteURL.Redacted())
+
+		name, err := snapshot.Create(createURL.String())
+		if err != nil {
+			logger.Fatalf("cannot create snapshot: %s", err)
 		}
 		err = flag.Set("snapshotName", name)
 		if err != nil {
@@ -78,9 +87,9 @@ func main() {
 		}
 
 		deleteSnapshot = func() {
-			err := snapshot.Delete(snapshotDeleteURL.Get(), name)
+			err := snapshot.Delete(deleteURL.String(), name)
 			if err != nil {
-				logger.Fatalf("cannot delete snapshot via %q: %s", snapshotDeleteURL, err)
+				logger.Fatalf("cannot delete snapshot: %s", err)
 			}
 		}
 	}

docs/CHANGELOG.md

@@ -41,8 +41,6 @@ See also [LTS releases](https://docs.victoriametrics.com/lts-releases/).
 * FEATURE: [vmauth](https://docs.victoriametrics.com/vmauth/): support regex matching when routing incoming requests based on HTTP [query args](https://en.wikipedia.org/wiki/Query_string) via `src_query_args` option at `url_map`. See [these docs](https://docs.victoriametrics.com/vmauth/#generic-http-proxy-for-different-backends) and [this feature request](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6070).
 * FEATURE: [vmui](https://docs.victoriametrics.com/#vmui): optimize auto-suggestion performance for metric names when the database contains big number of unique time series.
 * FEATURE: [vmui](https://docs.victoriametrics.com/#vmui): in the Select component, user-entered values are now preserved on blur if they match options in the list.
-* FEATURE: [vmbackup](https://docs.victoriametrics.com/vmbackup): support loading values for flags `-snapshot.createURL` and `-snapshot.deleteURL` from files for security reasons. To load value from file use the following format `-snapshot.createURL=file:///abs/path/to/file` or `-snapshot.deleteURL=file://./relative/path/to/file`. See related [issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/5973).
-* FEATURE: [vmbackup](https://docs.victoriametrics.com/vmbackup): hide sensitive information for  flags `-snapshot.createURL` and `-snapshot.deleteURL` when printing logs. The change masks HTTP basic authentication user and password values, as well as GET params matching `auth`, `pass`, `key`, `secret`, `token` words. See related [issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/5973). Thanks to @wasim-nihal for [initial implementation](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/6060).
 
 * BUGFIX: [vmalert](https://docs.victoriametrics.com/vmalert/): supported any status codes from the range 200-299 from alertmanager. Previously, only 200 status code considered a successful action. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6110).
 * BUGFIX: [vmalert](https://docs.victoriametrics.com/vmalert/): avoid blocking `/api/v1/rules`, `/api/v1/alerts`, `/metrics` APIs when alerting rule uses template functions `query`, which could takes a while to execute. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6079).

lib/flagutil/url.go

@@ -1,159 +0,0 @@
-package flagutil
-
-import (
-	"flag"
-	"fmt"
-	"log"
-	"net/url"
-	"os"
-	"regexp"
-	"strings"
-	"sync/atomic"
-	"unicode"
-
-	"github.com/VictoriaMetrics/VictoriaMetrics/lib/fasttime"
-)
-
-// NewURL returns new `url` flag with the given name and description.
-//
-// The url value is redacted when calling URL.String() in the following way:
-// 1. Basic Auth username and password are replaced with "xxxxx"
-// 2. Values of GET params matching `secretWordsRe` expression are replaced with "xxxxx".
-//
-// Call URL.Get() for obtaining original URL address.
-func NewURL(name, description string) *URL {
-	description += fmt.Sprintf("\nFlag value can be read from the given file when using -%s=file:///abs/path/to/file or -%s=file://./relative/path/to/file . ", name, name)
-	u := &URL{
-		flagname: name,
-	}
-	ru := &redactedURL{
-		URL:      &url.URL{},
-		redacted: "",
-	}
-	u.value.Store(&ru)
-	flag.Var(u, name, description)
-	return u
-}
-
-// URL is a flag holding URL address
-//
-// If the flag value is file:///path/to/file,
-// then its contents is automatically re-read from the given file on disk.
-type URL struct {
-	nextRefreshTimestamp atomic.Uint64
-
-	value atomic.Pointer[*redactedURL]
-
-	// flagname is the name of the flag
-	flagname string
-
-	// sourcePath contains either url or path to file with the url
-	sourcePath string
-}
-
-type redactedURL struct {
-	*url.URL
-	redacted string
-}
-
-// Get returns the current u address.
-//
-// It re-reads u value from the file:///path/to/file
-// if they were passed to URL.Set.
-func (u *URL) Get() string {
-	u.maybeRereadURL()
-	ru := *u.value.Load()
-	return ru.URL.String()
-}
-
-// Get returns the current u redacted address.
-//
-// It re-reads u value from the file:///path/to/file
-// if they were passed to URL.Set.
-func (u *URL) String() string {
-	u.maybeRereadURL()
-	ru := *u.value.Load()
-	return ru.redacted
-}
-
-func (u *URL) maybeRereadURL() {
-	if u.sourcePath == "" {
-		// Fast path - nothing to re-read
-		return
-	}
-	tsCurr := fasttime.UnixTimestamp()
-	tsNext := u.nextRefreshTimestamp.Load()
-	if tsCurr < tsNext {
-		// Fast path - nothing to re-read
-		return
-	}
-
-	// Re-read value from s.sourcePath
-	u.nextRefreshTimestamp.Store(tsCurr + 2)
-	data, err := os.ReadFile(u.sourcePath)
-	if err != nil {
-		// cannot use lib/logger, since it can be uninitialized yet
-		log.Printf("flagutil: fall back to the previous url for -%s, since failed to re-read it from %q: cannot read %q: %s\n", u.flagname, u.sourcePath, u.sourcePath, err.Error())
-	} else {
-		addr := strings.TrimRightFunc(string(data), unicode.IsSpace)
-		res, err := newRedactedURL(addr)
-		if err != nil {
-			log.Printf("flagutil: cannot parse %q: %s\n", u.flagname, err.Error())
-			return
-		}
-		u.value.Store(&res)
-	}
-}
-
-// Set implements flag.Value interface.
-func (u *URL) Set(value string) error {
-	u.nextRefreshTimestamp.Store(0)
-	var s string
-	switch {
-	case strings.HasPrefix(value, "file://"):
-		u.sourcePath = strings.TrimPrefix(value, "file://")
-		data, err := os.ReadFile(u.sourcePath)
-		if err != nil {
-			// cannot use lib/logger, since it can be uninitialized yet
-			return fmt.Errorf("cannot read %q: %w", u.sourcePath, err)
-		}
-		s = strings.TrimRightFunc(string(data), unicode.IsSpace)
-	default:
-		u.sourcePath = ""
-		s = value
-	}
-
-	res, err := newRedactedURL(s)
-	if err != nil {
-		return fmt.Errorf("cannot parse %q: %s", u.flagname, err)
-	}
-	u.value.Store(&res)
-	return nil
-}
-
-var secretWordsRe = regexp.MustCompile("auth|pass|key|secret|token")
-
-func newRedactedURL(s string) (*redactedURL, error) {
-	u, err := url.Parse(s)
-	if err != nil {
-		return nil, fmt.Errorf("cannot parse URL: %s", err)
-	}
-	ru := &redactedURL{URL: u}
-
-	// copy URL before mutating query params
-	u2 := *u
-	values := u2.Query()
-	for k, vs := range values {
-		if secretWordsRe.MatchString(k) {
-			for i := range vs {
-				vs[i] = "xxxxx"
-			}
-		}
-	}
-	u2.RawQuery = values.Encode()
-	if _, has := u2.User.Password(); has {
-		u2.User = url.UserPassword("xxxxx", "xxxxx")
-	}
-	ru.redacted = u2.String()
-	return ru, nil
-}

lib/flagutil/url_test.go

@@ -1,45 +0,0 @@
-package flagutil
-
-import (
-	"os"
-	"testing"
-)
-
-func TestNewURL(t *testing.T) {
-	u := &URL{}
-	f := func(s, exp string) {
-		t.Helper()
-		if err := u.Set(s); err != nil {
-			t.Fatalf("failed to set %q value: %s", s, err)
-		}
-		if u.String() != exp {
-			t.Fatalf("expected to get %q; got %q instead", exp, u.String())
-		}
-	}
-
-	f("", "")
-	f("http://foo:8428", "http://foo:8428")
-	f("http://username:password@foo:8428", "http://xxxxx:xxxxx@foo:8428")
-	f("http://foo:8428?authToken=bar", "http://foo:8428?authToken=xxxxx")
-	f("http://username:password@foo:8428?authToken=bar", "http://xxxxx:xxxxx@foo:8428?authToken=xxxxx")
-
-	file, err := os.CreateTemp("", "")
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer func() { _ = os.Remove(file.Name()) }()
-
-	writeToFile(t, file.Name(), "http://foo:8428")
-	f("file://"+file.Name(), "http://foo:8428")
-
-	writeToFile(t, file.Name(), "http://xxxxx:password@foo:8428?authToken=bar")
-	f("file://"+file.Name(), "http://xxxxx:xxxxx@foo:8428?authToken=xxxxx")
-}
-
-func writeToFile(t *testing.T, file, b string) {
-	t.Helper()
-	err := os.WriteFile(file, []byte(b), 0644)
-	if err != nil {
-		t.Fatal(err)
-	}
-}

lib/snapshot/snapshot.go

@@ -51,13 +51,13 @@ func Create(createSnapshotURL string) (string, error) {
 		return "", err
 	}
 	if resp.StatusCode != http.StatusOK {
-		return "", fmt.Errorf("unexpected status code: %d; expecting %d; response body: %q", resp.StatusCode, http.StatusOK, body)
+		return "", fmt.Errorf("unexpected status code returned from %q: %d; expecting %d; response body: %q", u.Redacted(), resp.StatusCode, http.StatusOK, body)
 	}
 
 	snap := snapshot{}
 	err = json.Unmarshal(body, &snap)
 	if err != nil {
-		return "", fmt.Errorf("cannot parse JSON response: %w; response body: %q", err, body)
+		return "", fmt.Errorf("cannot parse JSON response from %q: %w; response body: %q", u.Redacted(), err, body)
 	}
 
 	if snap.Status == "ok" {
@@ -67,7 +67,7 @@ func Create(createSnapshotURL string) (string, error) {
 	if snap.Status == "error" {
 		return "", errors.New(snap.Msg)
 	}
-	return "", fmt.Errorf("unknown status: %v", snap.Status)
+	return "", fmt.Errorf("Unkown status: %v", snap.Status)
 }
 
 // Delete deletes a snapshot via the provided api endpoint
@@ -95,13 +95,13 @@ func Delete(deleteSnapshotURL string, snapshotName string) error {
 		return err
 	}
 	if resp.StatusCode != http.StatusOK {
-		return fmt.Errorf("unexpected status code: %d; expecting %d; response body: %q", resp.StatusCode, http.StatusOK, body)
+		return fmt.Errorf("unexpected status code returned from %q: %d; expecting %d; response body: %q", u.Redacted(), resp.StatusCode, http.StatusOK, body)
 	}
 
 	snap := snapshot{}
 	err = json.Unmarshal(body, &snap)
 	if err != nil {
-		return fmt.Errorf("cannot parse JSON response: %w; response body: %q", err, body)
+		return fmt.Errorf("cannot parse JSON response from %q: %w; response body: %q", u.Redacted(), err, body)
 	}
 
 	if snap.Status == "ok" {
@@ -111,5 +111,5 @@ func Delete(deleteSnapshotURL string, snapshotName string) error {
 	if snap.Status == "error" {
 		return errors.New(snap.Msg)
 	}
-	return fmt.Errorf("unknown status: %v", snap.Status)
+	return fmt.Errorf("Unkown status: %v", snap.Status)
 }