diff --git a/go.mod b/go.mod index 57b9f5cde08a..7e7ad69dc5d0 100644 --- a/go.mod +++ b/go.mod @@ -152,7 +152,6 @@ require ( github.com/shopspring/decimal v1.2.0 // indirect github.com/spf13/cobra v1.1.3 github.com/spf13/pflag v1.0.5 - github.com/spf13/viper v1.7.1 github.com/stretchr/testify v1.6.1 github.com/twpayne/go-geom v1.3.7-0.20210228220813-9d9885b50d3e github.com/wadey/gocovmerge v0.0.0-20160331181800-b5bfa59ec0ad diff --git a/pkg/ccl/sqlproxyccl/denylist/BUILD.bazel b/pkg/ccl/sqlproxyccl/denylist/BUILD.bazel index 7d86105730f0..5fe40d0b5c83 100644 --- a/pkg/ccl/sqlproxyccl/denylist/BUILD.bazel +++ b/pkg/ccl/sqlproxyccl/denylist/BUILD.bazel @@ -1,4 +1,3 @@ -load("@bazel_gomock//:gomock.bzl", "gomock") load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") # gazelle:exclude service.go @@ -12,32 +11,22 @@ go_library( go_library( name = "denylist", - srcs = [ - "file.go", - "local_file.go", - ":mocks_denylist", # keep - ], + srcs = ["file.go"], embed = [":service"], # keep importpath = "github.com/cockroachdb/cockroach/pkg/ccl/sqlproxyccl/denylist", visibility = ["//visibility:public"], deps = [ - "//pkg/util", "//pkg/util/log", "//pkg/util/syncutil", "//pkg/util/timeutil", - "@com_github_cockroachdb_errors//:errors", "@com_github_golang_mock//gomock", # keep - "@com_github_spf13_viper//:viper", "@in_gopkg_yaml_v2//:yaml_v2", ], ) go_test( name = "denylist_test", - srcs = [ - "file_test.go", - "local_file_test.go", - ], + srcs = ["file_test.go"], embed = [":denylist"], deps = [ "//pkg/util/leaktest", @@ -46,12 +35,3 @@ go_test( "@in_gopkg_yaml_v2//:yaml_v2", ], ) - -gomock( - name = "mocks_denylist", - out = "mocks_generated.go", - interfaces = ["Service"], - library = ":service", - package = "denylist", - self_package = "github.com/cockroachdb/cockroach/pkg/ccl/sqlproxyccl/denylist", -) diff --git a/pkg/ccl/sqlproxyccl/denylist/local_file.go b/pkg/ccl/sqlproxyccl/denylist/local_file.go deleted file mode 100644 index 34d56e04c07a..000000000000 --- a/pkg/ccl/sqlproxyccl/denylist/local_file.go +++ /dev/null @@ -1,104 +0,0 @@ -// Copyright 2021 The Cockroach Authors. -// -// Licensed as a CockroachDB Enterprise file under the Cockroach Community -// License (the "License"); you may not use this file except in compliance with -// the License. You may obtain a copy of the License at -// -// https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt - -package denylist - -import ( - "context" - "time" - - "github.com/cockroachdb/cockroach/pkg/util" - "github.com/cockroachdb/cockroach/pkg/util/log" - "github.com/cockroachdb/cockroach/pkg/util/syncutil" - "github.com/cockroachdb/cockroach/pkg/util/timeutil" - "github.com/cockroachdb/errors" - "github.com/spf13/viper" -) - -type viperDenyList struct { - pollInterval time.Duration - mu struct { - syncutil.Mutex - viperCfg *viper.Viper - } -} - -// NewViperDenyListFromFile returns a new denylist Service backed by a local -// file using https://github.com/spf13/viper. -func NewViperDenyListFromFile( - ctx context.Context, file string, pollConfigInterval time.Duration, -) (Service, error) { - d := &viperDenyList{pollInterval: pollConfigInterval} - if file != "" { - vprCfg, err := newViperCfgFromFile(file) - if err != nil { - return nil, err - } - log.Infof(ctx, "current denied keys: %+v", vprCfg.AllKeys()) - d.mu.viperCfg = vprCfg - d.watchForUpdates(ctx) - } - - return d, nil -} - -// newViperCfgFromFile creates a new viper instance from a local file. -func newViperCfgFromFile(cfgFileName string) (*viper.Viper, error) { - v := viper.New() - v.SetConfigFile(cfgFileName) - if err := v.ReadInConfig(); err != nil { - return nil, errors.Wrap(err, "could not read denylist file") - } - log.Infof(context.Background(), "successfully loaded denylist file %s", cfgFileName) - return v, nil -} - -// Denied implements the Service interface using viper to query the deny list. -func (d *viperDenyList) Denied(entity DenyEntity) (*Entry, error) { - if d.mu.viperCfg == nil { - return nil, nil - } - d.mu.Lock() - defer d.mu.Unlock() - return d.deniedLocked(entity.Item) -} - -func (d *viperDenyList) deniedLocked(item string) (*Entry, error) { - if msg := d.mu.viperCfg.Get(item); msg != nil { - return &Entry{Reason: msg.(string)}, nil - } - return nil, nil -} - -// WatchForUpdates periodically reloads the denylist file. The daemon is -// canceled on ctx cancellation. -// TODO(spaskob): use notification via SIGHUP instead or use the viper API -// WatchConfig. -func (d *viperDenyList) watchForUpdates(ctx context.Context) { - go func() { - t := timeutil.NewTimer() - defer t.Stop() - for { - t.Reset(util.Jitter(d.pollInterval, 0.15 /* fraction */)) - select { - case <-ctx.Done(): - log.Errorf(ctx, "WatchList daemon stopped: %v", ctx.Err()) - return - case <-t.C: - t.Read = true - d.mu.Lock() - if err := d.mu.viperCfg.ReadInConfig(); err != nil { - // Only log the error since it may happen that the file is - // being written to or replaced at the same time. - log.Errorf(ctx, "could not read denylist %s: %v", d.mu.viperCfg.ConfigFileUsed(), err) - } - d.mu.Unlock() - } - } - }() -} diff --git a/pkg/ccl/sqlproxyccl/denylist/local_file_test.go b/pkg/ccl/sqlproxyccl/denylist/local_file_test.go deleted file mode 100644 index 749176147c91..000000000000 --- a/pkg/ccl/sqlproxyccl/denylist/local_file_test.go +++ /dev/null @@ -1,90 +0,0 @@ -// Copyright 2021 The Cockroach Authors. -// -// Licensed as a CockroachDB Enterprise file under the Cockroach Community -// License (the "License"); you may not use this file except in compliance with -// the License. You may obtain a copy of the License at -// -// https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt - -package denylist - -import ( - "context" - "io/ioutil" - "os" - "testing" - "time" - - "github.com/cockroachdb/cockroach/pkg/util/leaktest" - "github.com/cockroachdb/cockroach/pkg/util/timeutil" - "github.com/stretchr/testify/require" -) - -// TestViperForwardCompatibility makes sure that the new config file format -// will not return error when ingested by the old binary, and will not -// cause sqlproxy fail to start. -func TestViperForwardCompatibility(t *testing.T) { - // Make sure that new config file format will not - // cause errors for the old binary. - defer leaktest.AfterTest(t)() - - files := []File{ - { - Seq: 3, - Denylist: []*DenyEntry{ - {DenyEntity{"1.1.1.1", IPAddrType}, timeutil.Now().Add(time.Hour), "some reason"}, - {DenyEntity{"63", ClusterType}, timeutil.Now().Add(2 * time.Hour), "another reason"}, - }, - }, - { - // empty file - }, - { - Seq: 7, - // empty list - }, - } - for _, file := range files { - cfgFile, err := ioutil.TempFile("", "*_denylist.yml") - require.NoError(t, err) - defer func() { _ = os.Remove(cfgFile.Name()) }() - - raw, err := file.Serialize() - require.NoError(t, err) - err = ioutil.WriteFile(cfgFile.Name(), raw, 0777) - require.NoError(t, err) - - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - // make sure old parser won't break on new config format - _, err = NewViperDenyListFromFile(ctx, cfgFile.Name(), - time.Minute) - require.NoError(t, err) - } -} - -func TestViperDenyList(t *testing.T) { - defer leaktest.AfterTest(t)() - - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - cfgFile, err := ioutil.TempFile("", "*_denylist.yml") - require.NoError(t, err) - defer func() { _ = os.Remove(cfgFile.Name()) }() - - dl, err := NewViperDenyListFromFile(ctx, cfgFile.Name(), time.Millisecond) - require.NoError(t, err) - - e, err := dl.Denied(DenyEntity{"123", ClusterType}) - require.NoError(t, err) - require.True(t, e == nil) - - _, err = cfgFile.Write([]byte("456: denied")) - require.NoError(t, err) - time.Sleep(50 * time.Millisecond) - e, err = dl.Denied(DenyEntity{"456", ClusterType}) - require.NoError(t, err) - require.Equal(t, &Entry{Reason: "denied"}, e) -} diff --git a/pkg/server/BUILD.bazel b/pkg/server/BUILD.bazel index 2bac3419978d..7b85d416c144 100644 --- a/pkg/server/BUILD.bazel +++ b/pkg/server/BUILD.bazel @@ -325,6 +325,7 @@ go_test( "//pkg/storage", "//pkg/storage/enginepb", "//pkg/testutils", + "//pkg/testutils/diagutils", "//pkg/testutils/serverutils", "//pkg/testutils/skip", "//pkg/testutils/sqlutils", diff --git a/pkg/server/stats_test.go b/pkg/server/stats_test.go index 51d3f7f01631..c77369d90425 100644 --- a/pkg/server/stats_test.go +++ b/pkg/server/stats_test.go @@ -26,8 +26,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sqlstats" "github.com/cockroachdb/cockroach/pkg/sql/tests" "github.com/cockroachdb/cockroach/pkg/testutils" + "github.com/cockroachdb/cockroach/pkg/testutils/diagutils" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" - "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/timeutil" @@ -39,13 +39,19 @@ func TestTelemetrySQLStatsIndependence(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - // This test fails if the central reporting server from CRL - // breaks. It should be improved to customize the reporting URL and - // mock the registration collector with an in-memory server. - skip.WithIssue(t, 63851) - ctx := context.Background() params, _ := tests.CreateTestServerParams() + + r := diagutils.NewServer() + defer r.Close() + + url := r.URL() + params.Knobs.Server = &TestingKnobs{ + DiagnosticsTestingKnobs: diagnostics.TestingKnobs{ + OverrideReportingURL: &url, + }, + } + s, sqlDB, _ := serverutils.StartServer(t, params) defer s.Stopper().Stop(ctx) diff --git a/vendor b/vendor index 116c0f7262fe..1d937e9e79b4 160000 --- a/vendor +++ b/vendor @@ -1 +1 @@ -Subproject commit 116c0f7262fe491db09e1547117cd91f75a33b99 +Subproject commit 1d937e9e79b40199f143f7b07450931e9cc95d02