From 59b3f21708434653923a0a4265b08f8908c37f79 Mon Sep 17 00:00:00 2001 From: hagen1778 <roman@victoriametrics.com> Date: Wed, 24 Apr 2024 13:47:57 +0200 Subject: [PATCH] Revert "app/vmbackup: introduce new flag type URL (#6152)" This reverts commit 029060af60774bb8f3638d4ed09f88d29e7ca919. (cherry picked from commit 679844feafe0cc6c0187c0acb39ab5ad9f7d6eb5) --- app/vmbackup/main.go | 35 +++++---- docs/CHANGELOG.md | 2 - lib/flagutil/url.go | 159 --------------------------------------- lib/flagutil/url_test.go | 45 ----------- lib/snapshot/snapshot.go | 12 +-- 5 files changed, 28 insertions(+), 225 deletions(-) delete mode 100644 lib/flagutil/url.go delete mode 100644 lib/flagutil/url_test.go diff --git a/app/vmbackup/main.go b/app/vmbackup/main.go index 9f18cb41bc..3603f21050 100644 --- a/app/vmbackup/main.go +++ b/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", snapshotCreateURL.String()) - if len(snapshotDeleteURL.String()) <= 0 { - err := flag.Set("snapshot.deleteURL", strings.Replace(snapshotCreateURL.Get(), "/create", "/delete", 1)) + logger.Infof("Snapshot create url %s", createURL.Redacted()) + if len(*snapshotDeleteURL) <= 0 { + err := flag.Set("snapshot.deleteURL", strings.Replace(*snapshotCreateURL, "/create", "/delete", 1)) if err != nil { logger.Fatalf("Failed to set snapshot.deleteURL flag: %v", err) } } - - logger.Infof("Snapshot delete url %s", snapshotDeleteURL) - name, err := snapshot.Create(snapshotCreateURL.Get()) + deleteURL, err := url.Parse(*snapshotDeleteURL) 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) } } } diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 6450cdd4ef..99f4b4b0cc 100644 --- a/docs/CHANGELOG.md +++ b/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). diff --git a/lib/flagutil/url.go b/lib/flagutil/url.go deleted file mode 100644 index f8dcf8f65d..0000000000 --- a/lib/flagutil/url.go +++ /dev/null @@ -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 -} diff --git a/lib/flagutil/url_test.go b/lib/flagutil/url_test.go deleted file mode 100644 index 4b36f54862..0000000000 --- a/lib/flagutil/url_test.go +++ /dev/null @@ -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) - } -} diff --git a/lib/snapshot/snapshot.go b/lib/snapshot/snapshot.go index c4324ea81d..87bf8d09f6 100644 --- a/lib/snapshot/snapshot.go +++ b/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) }