From 0bc8a3308487202d48b1f386fa1d0ab0da5adf95 Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Mon, 7 Jun 2021 10:54:33 -0500 Subject: [PATCH 1/2] consul: probe consul namespace feature before using namespace api This PR changes Nomad's wrapper around the Consul NamespaceAPI so that it will detect if the Consul Namespaces feature is enabled before making a request to the Namespaces API. Namespaces are not enabled in Consul OSS, and require a suitable license to be used with Consul ENT. Previously Nomad would check for a 404 status code when makeing a request to the Namespaces API to "detect" if Consul OSS was being used. This does not work for Consul ENT with Namespaces disabled, which returns a 500. Now we avoid requesting the namespace API altogether if Consul is detected to be the OSS sku, or if the Namespaces feature is not licensed. Since Consul can be upgraded from OSS to ENT, or a new license applied, we cache the value for 1 minute, refreshing on demand if expired. Fixes https://github.com/hashicorp/nomad-enterprise/issues/575 Note that the ticket originally describes using attributes from https://github.com/hashicorp/nomad/issues/10688. This turns out not to be possible due to a chicken-egg situation between bootstrapping the agent and setting up the consul client. Also fun: the Consul fingerprinter creates its own Consul client, because there is no [currently] no way to pass the agent's client through the fingerprint factory. --- CHANGELOG.md | 1 + client/allocrunner/groupservice_hook_test.go | 2 +- .../taskrunner/connect_native_hook_test.go | 8 +- .../taskrunner/envoy_bootstrap_hook_test.go | 6 +- .../taskrunner/task_runner_test.go | 14 ++- client/fingerprint/consul.go | 74 +++-------- client/fingerprint/consul_test.go | 71 +++++------ command/agent/agent.go | 2 +- command/agent/consul/catalog_testing.go | 51 +++++++- command/agent/consul/check_watcher_test.go | 4 +- command/agent/consul/connect_proxies_test.go | 3 +- command/agent/consul/group_test.go | 2 +- command/agent/consul/int_test.go | 2 +- command/agent/consul/namespaces_client.go | 71 +++++++++-- .../agent/consul/namespaces_client_test.go | 117 ++++++++++++++++++ command/agent/consul/self.go | 56 +++++++++ command/agent/consul/self_test.go | 98 +++++++++++++++ command/agent/consul/service_client_test.go | 4 +- command/agent/consul/unit_test.go | 16 +-- 19 files changed, 468 insertions(+), 134 deletions(-) create mode 100644 command/agent/consul/namespaces_client_test.go create mode 100644 command/agent/consul/self.go create mode 100644 command/agent/consul/self_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 6eef86ca8cb2..42d66abedcdf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ BUG FIXES: * api: Fixed event stream connection initialization when there are no events to send [[GH-10637](https://github.com/hashicorp/nomad/issues/10637)] * cli: Fixed a bug where `quota status` and `namespace status` commands may panic if the CLI targets a pre-1.1.0 cluster [[GH-10620](https://github.com/hashicorp/nomad/pull/10620)] * cli: Fixed a bug where `alloc exec` may fail with "unexpected EOF" without returning the exit code after a command [[GH-10657](https://github.com/hashicorp/nomad/issues/10657)] +* consul: Fixed a bug where consul namespace API would be queried even when consul namespaces were not enabled [[GH-10715](https://github.com/hashicorp/nomad/pull/10715)] * csi: Fixed a bug where `mount_options` were not passed to CSI controller plugins for validation during volume creation and mounting. [[GH-10643](https://github.com/hashicorp/nomad/issues/10643)] * csi: Fixed a bug where `capability` blocks were not passed to CSI controller plugins for validation for `nomad volume register` commands. [[GH-10703](https://github.com/hashicorp/nomad/issues/10703)] * drivers/exec: Fixed a bug where `exec` and `java` tasks inherit the Nomad agent's `oom_score_adj` value [[GH-10698](https://github.com/hashicorp/nomad/issues/10698)] diff --git a/client/allocrunner/groupservice_hook_test.go b/client/allocrunner/groupservice_hook_test.go index 59d3b7a7a5ca..5789079d850a 100644 --- a/client/allocrunner/groupservice_hook_test.go +++ b/client/allocrunner/groupservice_hook_test.go @@ -238,7 +238,7 @@ func TestGroupServiceHook_Update08Alloc(t *testing.T) { consulConfig.Address = testconsul.HTTPAddr consulClient, err := consulapi.NewClient(consulConfig) require.NoError(t, err) - namespacesClient := agentconsul.NewNamespacesClient(consulClient.Namespaces()) + namespacesClient := agentconsul.NewNamespacesClient(consulClient.Namespaces(), consulClient.Agent()) serviceClient := agentconsul.NewServiceClient(consulClient.Agent(), namespacesClient, testlog.HCLogger(t), true) diff --git a/client/allocrunner/taskrunner/connect_native_hook_test.go b/client/allocrunner/taskrunner/connect_native_hook_test.go index 9ec54bc46092..a1c368a42efc 100644 --- a/client/allocrunner/taskrunner/connect_native_hook_test.go +++ b/client/allocrunner/taskrunner/connect_native_hook_test.go @@ -312,7 +312,7 @@ func TestTaskRunner_ConnectNativeHook_Ok(t *testing.T) { consulConfig.Address = testConsul.HTTPAddr consulAPIClient, err := consulapi.NewClient(consulConfig) require.NoError(t, err) - namespacesClient := agentconsul.NewNamespacesClient(consulAPIClient.Namespaces()) + namespacesClient := agentconsul.NewNamespacesClient(consulAPIClient.Namespaces(), consulAPIClient.Agent()) consulClient := agentconsul.NewServiceClient(consulAPIClient.Agent(), namespacesClient, logger, true) go consulClient.Run() @@ -377,7 +377,7 @@ func TestTaskRunner_ConnectNativeHook_with_SI_token(t *testing.T) { consulConfig.Address = testConsul.HTTPAddr consulAPIClient, err := consulapi.NewClient(consulConfig) require.NoError(t, err) - namespacesClient := agentconsul.NewNamespacesClient(consulAPIClient.Namespaces()) + namespacesClient := agentconsul.NewNamespacesClient(consulAPIClient.Namespaces(), consulAPIClient.Agent()) consulClient := agentconsul.NewServiceClient(consulAPIClient.Agent(), namespacesClient, logger, true) go consulClient.Run() @@ -454,7 +454,7 @@ func TestTaskRunner_ConnectNativeHook_shareTLS(t *testing.T) { consulConfig.Address = testConsul.HTTPAddr consulAPIClient, err := consulapi.NewClient(consulConfig) require.NoError(t, err) - namespacesClient := agentconsul.NewNamespacesClient(consulAPIClient.Namespaces()) + namespacesClient := agentconsul.NewNamespacesClient(consulAPIClient.Namespaces(), consulAPIClient.Agent()) consulClient := agentconsul.NewServiceClient(consulAPIClient.Agent(), namespacesClient, logger, true) go consulClient.Run() @@ -574,7 +574,7 @@ func TestTaskRunner_ConnectNativeHook_shareTLS_override(t *testing.T) { consulConfig.Address = testConsul.HTTPAddr consulAPIClient, err := consulapi.NewClient(consulConfig) require.NoError(t, err) - namespacesClient := agentconsul.NewNamespacesClient(consulAPIClient.Namespaces()) + namespacesClient := agentconsul.NewNamespacesClient(consulAPIClient.Namespaces(), consulAPIClient.Agent()) consulClient := agentconsul.NewServiceClient(consulAPIClient.Agent(), namespacesClient, logger, true) go consulClient.Run() diff --git a/client/allocrunner/taskrunner/envoy_bootstrap_hook_test.go b/client/allocrunner/taskrunner/envoy_bootstrap_hook_test.go index 58e0ed988dd5..98d20440650e 100644 --- a/client/allocrunner/taskrunner/envoy_bootstrap_hook_test.go +++ b/client/allocrunner/taskrunner/envoy_bootstrap_hook_test.go @@ -325,7 +325,7 @@ func TestEnvoyBootstrapHook_with_SI_token(t *testing.T) { consulConfig.Address = testConsul.HTTPAddr consulAPIClient, err := consulapi.NewClient(consulConfig) require.NoError(t, err) - namespacesClient := agentconsul.NewNamespacesClient(consulAPIClient.Namespaces()) + namespacesClient := agentconsul.NewNamespacesClient(consulAPIClient.Namespaces(), consulAPIClient.Agent()) consulClient := agentconsul.NewServiceClient(consulAPIClient.Agent(), namespacesClient, logger, true) go consulClient.Run() @@ -426,7 +426,7 @@ func TestTaskRunner_EnvoyBootstrapHook_sidecar_ok(t *testing.T) { consulConfig.Address = testConsul.HTTPAddr consulAPIClient, err := consulapi.NewClient(consulConfig) require.NoError(t, err) - namespacesClient := agentconsul.NewNamespacesClient(consulAPIClient.Namespaces()) + namespacesClient := agentconsul.NewNamespacesClient(consulAPIClient.Namespaces(), consulAPIClient.Agent()) consulClient := agentconsul.NewServiceClient(consulAPIClient.Agent(), namespacesClient, logger, true) go consulClient.Run() @@ -491,7 +491,7 @@ func TestTaskRunner_EnvoyBootstrapHook_gateway_ok(t *testing.T) { consulConfig.Address = testConsul.HTTPAddr consulAPIClient, err := consulapi.NewClient(consulConfig) require.NoError(t, err) - namespacesClient := agentconsul.NewNamespacesClient(consulAPIClient.Namespaces()) + namespacesClient := agentconsul.NewNamespacesClient(consulAPIClient.Namespaces(), consulAPIClient.Agent()) // Register Group Services serviceClient := agentconsul.NewServiceClient(consulAPIClient.Agent(), namespacesClient, logger, true) diff --git a/client/allocrunner/taskrunner/task_runner_test.go b/client/allocrunner/taskrunner/task_runner_test.go index 869042d58aab..c59f47659873 100644 --- a/client/allocrunner/taskrunner/task_runner_test.go +++ b/client/allocrunner/taskrunner/task_runner_test.go @@ -1155,9 +1155,12 @@ func TestTaskRunner_CheckWatcher_Restart(t *testing.T) { // Replace mock Consul ServiceClient, with the real ServiceClient // backed by a mock consul whose checks are always unhealthy. - consulAgent := agentconsul.NewMockAgent() + consulAgent := agentconsul.NewMockAgent(agentconsul.Features{ + Enterprise: false, + Namespaces: false, + }) consulAgent.SetStatus("critical") - namespacesClient := agentconsul.NewNamespacesClient(agentconsul.NewMockNamespaces(nil)) + namespacesClient := agentconsul.NewNamespacesClient(agentconsul.NewMockNamespaces(nil), consulAgent) consulClient := agentconsul.NewServiceClient(consulAgent, namespacesClient, conf.Logger, true) go consulClient.Run() defer consulClient.Shutdown() @@ -1835,8 +1838,11 @@ func TestTaskRunner_DriverNetwork(t *testing.T) { defer cleanup() // Use a mock agent to test for services - consulAgent := agentconsul.NewMockAgent() - namespacesClient := agentconsul.NewNamespacesClient(agentconsul.NewMockNamespaces(nil)) + consulAgent := agentconsul.NewMockAgent(agentconsul.Features{ + Enterprise: false, + Namespaces: false, + }) + namespacesClient := agentconsul.NewNamespacesClient(agentconsul.NewMockNamespaces(nil), consulAgent) consulClient := agentconsul.NewServiceClient(consulAgent, namespacesClient, conf.Logger, true) defer consulClient.Shutdown() go consulClient.Run() diff --git a/client/fingerprint/consul.go b/client/fingerprint/consul.go index e37ffb1a888f..adfbe4c4e162 100644 --- a/client/fingerprint/consul.go +++ b/client/fingerprint/consul.go @@ -3,12 +3,11 @@ package fingerprint import ( "fmt" "strconv" - "strings" "time" - consul "github.com/hashicorp/consul/api" + consulapi "github.com/hashicorp/consul/api" log "github.com/hashicorp/go-hclog" - "github.com/hashicorp/go-version" + agentconsul "github.com/hashicorp/nomad/command/agent/consul" ) const ( @@ -19,17 +18,14 @@ const ( // ConsulFingerprint is used to fingerprint for Consul type ConsulFingerprint struct { logger log.Logger - client *consul.Client + client *consulapi.Client lastState string extractors map[string]consulExtractor } -// consulInfo aliases the type returned from the Consul agent self endpoint. -type consulInfo = map[string]map[string]interface{} - // consulExtractor is used to parse out one attribute from consulInfo. Returns // the value of the attribute, and whether the attribute exists. -type consulExtractor func(consulInfo) (string, bool) +type consulExtractor func(agentconsul.Self) (string, bool) // NewConsulFingerprint is used to create a Consul fingerprint func NewConsulFingerprint(logger log.Logger) Fingerprint { @@ -95,7 +91,7 @@ func (f *ConsulFingerprint) initialize(req *FingerprintRequest) error { return fmt.Errorf("failed to initialize Consul client config: %v", err) } - f.client, err = consul.NewClient(consulConfig) + f.client, err = consulapi.NewClient(consulConfig) if err != nil { return fmt.Errorf("failed to initialize Consul client: %s", err) } @@ -117,7 +113,7 @@ func (f *ConsulFingerprint) initialize(req *FingerprintRequest) error { return nil } -func (f *ConsulFingerprint) query(resp *FingerprintResponse) consulInfo { +func (f *ConsulFingerprint) query(resp *FingerprintResponse) agentconsul.Self { // We'll try to detect consul by making a query to to the agent's self API. // If we can't hit this URL consul is probably not running on this machine. info, err := f.client.Agent().Self() @@ -144,48 +140,36 @@ func (f *ConsulFingerprint) link(resp *FingerprintResponse) { } } -func (f *ConsulFingerprint) server(info consulInfo) (string, bool) { +func (f *ConsulFingerprint) server(info agentconsul.Self) (string, bool) { s, ok := info["Config"]["Server"].(bool) return strconv.FormatBool(s), ok } -func (f *ConsulFingerprint) version(info consulInfo) (string, bool) { +func (f *ConsulFingerprint) version(info agentconsul.Self) (string, bool) { v, ok := info["Config"]["Version"].(string) return v, ok } -func (f *ConsulFingerprint) sku(info consulInfo) (string, bool) { - v, ok := info["Config"]["Version"].(string) - if !ok { - return "", ok - } - - ver, vErr := version.NewVersion(v) - if vErr != nil { - return "", false - } - if strings.Contains(ver.Metadata(), "ent") { - return "ent", true - } - return "oss", true +func (f *ConsulFingerprint) sku(info agentconsul.Self) (string, bool) { + return agentconsul.SKU(info) } -func (f *ConsulFingerprint) revision(info consulInfo) (string, bool) { +func (f *ConsulFingerprint) revision(info agentconsul.Self) (string, bool) { r, ok := info["Config"]["Revision"].(string) return r, ok } -func (f *ConsulFingerprint) name(info consulInfo) (string, bool) { +func (f *ConsulFingerprint) name(info agentconsul.Self) (string, bool) { n, ok := info["Config"]["NodeName"].(string) return n, ok } -func (f *ConsulFingerprint) dc(info consulInfo) (string, bool) { +func (f *ConsulFingerprint) dc(info agentconsul.Self) (string, bool) { d, ok := info["Config"]["Datacenter"].(string) return d, ok } -func (f *ConsulFingerprint) segment(info consulInfo) (string, bool) { +func (f *ConsulFingerprint) segment(info agentconsul.Self) (string, bool) { tags, tagsOK := info["Member"]["Tags"].(map[string]interface{}) if !tagsOK { return "", false @@ -194,38 +178,16 @@ func (f *ConsulFingerprint) segment(info consulInfo) (string, bool) { return s, ok } -func (f *ConsulFingerprint) connect(info consulInfo) (string, bool) { +func (f *ConsulFingerprint) connect(info agentconsul.Self) (string, bool) { c, ok := info["DebugConfig"]["ConnectEnabled"].(bool) return strconv.FormatBool(c), ok } -func (f *ConsulFingerprint) grpc(info consulInfo) (string, bool) { +func (f *ConsulFingerprint) grpc(info agentconsul.Self) (string, bool) { p, ok := info["DebugConfig"]["GRPCPort"].(float64) return fmt.Sprintf("%d", int(p)), ok } -func (f *ConsulFingerprint) namespaces(info consulInfo) (string, bool) { - return f.feature("Namespaces", info) -} - -// possible values as of v1.9.5+ent: -// Automated Backups, Automated Upgrades, Enhanced Read Scalability, -// Network Segments, Redundancy Zone, Advanced Network Federation, -// Namespaces, SSO, Audit Logging -func (f *ConsulFingerprint) feature(name string, info consulInfo) (string, bool) { - lic, licOK := info["Stats"]["license"].(map[string]interface{}) - if !licOK { - return "", false - } - - features, exists := lic["features"].(string) - if !exists { - return "", false - } - - if !strings.Contains(features, name) { - return "", false - } - - return "true", true +func (f *ConsulFingerprint) namespaces(info agentconsul.Self) (string, bool) { + return agentconsul.Namespaces(info) } diff --git a/client/fingerprint/consul_test.go b/client/fingerprint/consul_test.go index e5eda50b292f..ae373209eb5d 100644 --- a/client/fingerprint/consul_test.go +++ b/client/fingerprint/consul_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/hashicorp/nomad/client/config" + agentconsul "github.com/hashicorp/nomad/command/agent/consul" "github.com/hashicorp/nomad/helper/testlog" "github.com/hashicorp/nomad/nomad/structs" "github.com/stretchr/testify/require" @@ -51,7 +52,7 @@ func TestConsulFingerprint_server(t *testing.T) { fp := newConsulFingerPrint(t) t.Run("is server", func(t *testing.T) { - s, ok := fp.server(consulInfo{ + s, ok := fp.server(agentconsul.Self{ "Config": {"Server": true}, }) require.True(t, ok) @@ -59,7 +60,7 @@ func TestConsulFingerprint_server(t *testing.T) { }) t.Run("is not server", func(t *testing.T) { - s, ok := fp.server(consulInfo{ + s, ok := fp.server(agentconsul.Self{ "Config": {"Server": false}, }) require.True(t, ok) @@ -67,14 +68,14 @@ func TestConsulFingerprint_server(t *testing.T) { }) t.Run("missing", func(t *testing.T) { - _, ok := fp.server(consulInfo{ + _, ok := fp.server(agentconsul.Self{ "Config": {}, }) require.False(t, ok) }) t.Run("malformed", func(t *testing.T) { - _, ok := fp.server(consulInfo{ + _, ok := fp.server(agentconsul.Self{ "Config": {"Server": 9000}, }) require.False(t, ok) @@ -87,7 +88,7 @@ func TestConsulFingerprint_version(t *testing.T) { fp := newConsulFingerPrint(t) t.Run("oss", func(t *testing.T) { - v, ok := fp.version(consulInfo{ + v, ok := fp.version(agentconsul.Self{ "Config": {"Version": "v1.9.5"}, }) require.True(t, ok) @@ -95,7 +96,7 @@ func TestConsulFingerprint_version(t *testing.T) { }) t.Run("ent", func(t *testing.T) { - v, ok := fp.version(consulInfo{ + v, ok := fp.version(agentconsul.Self{ "Config": {"Version": "v1.9.5+ent"}, }) require.True(t, ok) @@ -103,14 +104,14 @@ func TestConsulFingerprint_version(t *testing.T) { }) t.Run("missing", func(t *testing.T) { - _, ok := fp.version(consulInfo{ + _, ok := fp.version(agentconsul.Self{ "Config": {}, }) require.False(t, ok) }) t.Run("malformed", func(t *testing.T) { - _, ok := fp.version(consulInfo{ + _, ok := fp.version(agentconsul.Self{ "Config": {"Version": 9000}, }) require.False(t, ok) @@ -123,7 +124,7 @@ func TestConsulFingerprint_sku(t *testing.T) { fp := newConsulFingerPrint(t) t.Run("oss", func(t *testing.T) { - s, ok := fp.sku(consulInfo{ + s, ok := fp.sku(agentconsul.Self{ "Config": {"Version": "v1.9.5"}, }) require.True(t, ok) @@ -131,7 +132,7 @@ func TestConsulFingerprint_sku(t *testing.T) { }) t.Run("oss dev", func(t *testing.T) { - s, ok := fp.sku(consulInfo{ + s, ok := fp.sku(agentconsul.Self{ "Config": {"Version": "v1.9.5-dev"}, }) require.True(t, ok) @@ -139,7 +140,7 @@ func TestConsulFingerprint_sku(t *testing.T) { }) t.Run("ent", func(t *testing.T) { - s, ok := fp.sku(consulInfo{ + s, ok := fp.sku(agentconsul.Self{ "Config": {"Version": "v1.9.5+ent"}, }) require.True(t, ok) @@ -147,7 +148,7 @@ func TestConsulFingerprint_sku(t *testing.T) { }) t.Run("ent dev", func(t *testing.T) { - s, ok := fp.sku(consulInfo{ + s, ok := fp.sku(agentconsul.Self{ "Config": {"Version": "v1.9.5+ent-dev"}, }) require.True(t, ok) @@ -155,14 +156,14 @@ func TestConsulFingerprint_sku(t *testing.T) { }) t.Run("missing", func(t *testing.T) { - _, ok := fp.sku(consulInfo{ + _, ok := fp.sku(agentconsul.Self{ "Config": {}, }) require.False(t, ok) }) t.Run("malformed", func(t *testing.T) { - _, ok := fp.sku(consulInfo{ + _, ok := fp.sku(agentconsul.Self{ "Config": {"Version": "***"}, }) require.False(t, ok) @@ -175,7 +176,7 @@ func TestConsulFingerprint_revision(t *testing.T) { fp := newConsulFingerPrint(t) t.Run("ok", func(t *testing.T) { - r, ok := fp.revision(consulInfo{ + r, ok := fp.revision(agentconsul.Self{ "Config": {"Revision": "3c1c22679"}, }) require.True(t, ok) @@ -183,14 +184,14 @@ func TestConsulFingerprint_revision(t *testing.T) { }) t.Run("malformed", func(t *testing.T) { - _, ok := fp.revision(consulInfo{ + _, ok := fp.revision(agentconsul.Self{ "Config": {"Revision": 9000}, }) require.False(t, ok) }) t.Run("missing", func(t *testing.T) { - _, ok := fp.revision(consulInfo{ + _, ok := fp.revision(agentconsul.Self{ "Config": {}, }) require.False(t, ok) @@ -203,7 +204,7 @@ func TestConsulFingerprint_dc(t *testing.T) { fp := newConsulFingerPrint(t) t.Run("ok", func(t *testing.T) { - dc, ok := fp.dc(consulInfo{ + dc, ok := fp.dc(agentconsul.Self{ "Config": {"Datacenter": "dc1"}, }) require.True(t, ok) @@ -211,14 +212,14 @@ func TestConsulFingerprint_dc(t *testing.T) { }) t.Run("malformed", func(t *testing.T) { - _, ok := fp.dc(consulInfo{ + _, ok := fp.dc(agentconsul.Self{ "Config": {"Datacenter": 9000}, }) require.False(t, ok) }) t.Run("missing", func(t *testing.T) { - _, ok := fp.dc(consulInfo{ + _, ok := fp.dc(agentconsul.Self{ "Config": {}, }) require.False(t, ok) @@ -231,7 +232,7 @@ func TestConsulFingerprint_segment(t *testing.T) { fp := newConsulFingerPrint(t) t.Run("ok", func(t *testing.T) { - s, ok := fp.segment(consulInfo{ + s, ok := fp.segment(agentconsul.Self{ "Member": {"Tags": map[string]interface{}{"segment": "seg1"}}, }) require.True(t, ok) @@ -239,21 +240,21 @@ func TestConsulFingerprint_segment(t *testing.T) { }) t.Run("segment missing", func(t *testing.T) { - _, ok := fp.segment(consulInfo{ + _, ok := fp.segment(agentconsul.Self{ "Member": {"Tags": map[string]interface{}{}}, }) require.False(t, ok) }) t.Run("tags missing", func(t *testing.T) { - _, ok := fp.segment(consulInfo{ + _, ok := fp.segment(agentconsul.Self{ "Member": {}, }) require.False(t, ok) }) t.Run("malformed", func(t *testing.T) { - _, ok := fp.segment(consulInfo{ + _, ok := fp.segment(agentconsul.Self{ "Member": {"Tags": map[string]interface{}{"segment": 9000}}, }) require.False(t, ok) @@ -266,7 +267,7 @@ func TestConsulFingerprint_connect(t *testing.T) { fp := newConsulFingerPrint(t) t.Run("connect enabled", func(t *testing.T) { - s, ok := fp.connect(consulInfo{ + s, ok := fp.connect(agentconsul.Self{ "DebugConfig": {"ConnectEnabled": true}, }) require.True(t, ok) @@ -274,7 +275,7 @@ func TestConsulFingerprint_connect(t *testing.T) { }) t.Run("connect not enabled", func(t *testing.T) { - s, ok := fp.connect(consulInfo{ + s, ok := fp.connect(agentconsul.Self{ "DebugConfig": {"ConnectEnabled": false}, }) require.True(t, ok) @@ -282,7 +283,7 @@ func TestConsulFingerprint_connect(t *testing.T) { }) t.Run("connect missing", func(t *testing.T) { - _, ok := fp.connect(consulInfo{ + _, ok := fp.connect(agentconsul.Self{ "DebugConfig": {}, }) require.False(t, ok) @@ -295,7 +296,7 @@ func TestConsulFingerprint_grpc(t *testing.T) { fp := newConsulFingerPrint(t) t.Run("grpc set", func(t *testing.T) { - s, ok := fp.grpc(consulInfo{ + s, ok := fp.grpc(agentconsul.Self{ "DebugConfig": {"GRPCPort": 8502.0}, // JSON numbers are floats }) require.True(t, ok) @@ -303,7 +304,7 @@ func TestConsulFingerprint_grpc(t *testing.T) { }) t.Run("grpc disabled", func(t *testing.T) { - s, ok := fp.grpc(consulInfo{ + s, ok := fp.grpc(agentconsul.Self{ "DebugConfig": {"GRPCPort": -1.0}, // JSON numbers are floats }) require.True(t, ok) @@ -311,7 +312,7 @@ func TestConsulFingerprint_grpc(t *testing.T) { }) t.Run("grpc missing", func(t *testing.T) { - _, ok := fp.grpc(consulInfo{ + _, ok := fp.grpc(agentconsul.Self{ "DebugConfig": {}, }) require.False(t, ok) @@ -325,7 +326,7 @@ func TestConsulFingerprint_namespaces(t *testing.T) { fp := newConsulFingerPrint(t) t.Run("supports namespaces", func(t *testing.T) { - s, ok := fp.namespaces(consulInfo{ + s, ok := fp.namespaces(agentconsul.Self{ "Stats": {"license": map[string]interface{}{"features": "Automated Backups, Automated Upgrades, Enhanced Read Scalability, Network Segments, Redundancy Zone, Advanced Network Federation, Namespaces, SSO, Audit Logging"}}, }) require.True(t, ok) @@ -333,24 +334,24 @@ func TestConsulFingerprint_namespaces(t *testing.T) { }) t.Run("no namespaces", func(t *testing.T) { - _, ok := fp.namespaces(consulInfo{ + _, ok := fp.namespaces(agentconsul.Self{ "Stats": {"license": map[string]interface{}{"features": "Automated Backups, Automated Upgrades, Enhanced Read Scalability, Network Segments, Redundancy Zone, Advanced Network Federation, SSO, Audit Logging"}}, }) require.False(t, ok) }) t.Run("stats missing", func(t *testing.T) { - _, ok := fp.namespaces(consulInfo{}) + _, ok := fp.namespaces(agentconsul.Self{}) require.False(t, ok) }) t.Run("license missing", func(t *testing.T) { - _, ok := fp.namespaces(consulInfo{"Stats": {}}) + _, ok := fp.namespaces(agentconsul.Self{"Stats": {}}) require.False(t, ok) }) t.Run("features missing", func(t *testing.T) { - _, ok := fp.namespaces(consulInfo{"Stats": {"license": map[string]interface{}{}}}) + _, ok := fp.namespaces(agentconsul.Self{"Stats": {"license": map[string]interface{}{}}}) require.False(t, ok) }) } diff --git a/command/agent/agent.go b/command/agent/agent.go index ff20dc99ab44..a500dea7f431 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -1197,7 +1197,7 @@ func (a *Agent) setupConsul(consulConfig *config.ConsulConfig) error { } // Create Consul Agent client for looking info about the agent. consulAgentClient := consulClient.Agent() - namespacesClient := consul.NewNamespacesClient(consulClient.Namespaces()) + namespacesClient := consul.NewNamespacesClient(consulClient.Namespaces(), consulAgentClient) a.consulService = consul.NewServiceClient(consulAgentClient, namespacesClient, a.logger, isClient) a.consulProxies = consul.NewConnectProxiesClient(consulAgentClient) diff --git a/command/agent/consul/catalog_testing.go b/command/agent/consul/catalog_testing.go index f34f05244143..69eb78f6e90e 100644 --- a/command/agent/consul/catalog_testing.go +++ b/command/agent/consul/catalog_testing.go @@ -78,6 +78,13 @@ type MockAgent struct { // hits is the total number of times agent methods have been called hits int + // ent indicates whether the agent is mocking an enterprise consul + ent bool + + // namespaces indicates whether the agent is mocking consul with namespaces + // feature enabled + namespaces bool + // mu guards above fields mu sync.Mutex @@ -90,13 +97,21 @@ type MockAgent struct { var _ AgentAPI = (*MockAgent)(nil) +type Features struct { + Enterprise bool + Namespaces bool +} + // NewMockAgent that returns all checks as passing. -func NewMockAgent() *MockAgent { +func NewMockAgent(f Features) *MockAgent { return &MockAgent{ services: make(map[string]map[string]*api.AgentServiceRegistration), checks: make(map[string]map[string]*api.AgentCheckRegistration), checkTTLs: make(map[string]map[string]int), checkStatus: api.HealthPassing, + + ent: f.Enterprise, + namespaces: f.Namespaces, } } @@ -121,7 +136,34 @@ func (c *MockAgent) Self() (map[string]map[string]interface{}, error) { defer c.mu.Unlock() c.hits++ - s := map[string]map[string]interface{}{ + version := "1.9.5" + build := "1.9.5:22ce6c6a" + if c.ent { + version = "1.9.5+ent" + build = "1.9.5+ent:22ce6c6a" + } + + stats := make(map[string]interface{}) + if c.ent { + if c.namespaces { + stats = map[string]interface{}{ + "license": map[string]interface{}{ + "features": "Namespaces,", + }, + } + } + } + + return map[string]map[string]interface{}{ + "Config": { + "Datacenter": "dc1", + "NodeName": "x52", + "NodeID": "9e7bf42e-a0b4-61b7-24f9-66dead411f0f", + "Revision": "22ce6c6ad", + "Server": true, + "Version": version, + }, + "Stats": stats, "Member": { "Addr": "127.0.0.1", "DelegateCur": 4, @@ -134,7 +176,7 @@ func (c *MockAgent) Self() (map[string]map[string]interface{}, error) { "ProtocolMin": 1, "Status": 1, "Tags": map[string]interface{}{ - "build": "0.8.1:'e9ca44d", + "build": build, }, }, "xDS": { @@ -147,8 +189,7 @@ func (c *MockAgent) Self() (map[string]map[string]interface{}, error) { }, }, }, - } - return s, nil + }, nil } func getNamespace(q *api.QueryOptions) string { diff --git a/command/agent/consul/check_watcher_test.go b/command/agent/consul/check_watcher_test.go index 0267699c33c1..c24c4d10d6ce 100644 --- a/command/agent/consul/check_watcher_test.go +++ b/command/agent/consul/check_watcher_test.go @@ -151,7 +151,7 @@ func (c *fakeChecksAPI) ChecksWithFilterOpts(filter string, opts *api.QueryOptio func testWatcherSetup(t *testing.T) (*fakeChecksAPI, *checkWatcher) { logger := testlog.HCLogger(t) checksAPI := newFakeChecksAPI() - namespacesClient := NewNamespacesClient(NewMockNamespaces(nil)) + namespacesClient := NewNamespacesClient(NewMockNamespaces(nil), NewMockAgent(ossFeatures)) cw := newCheckWatcher(logger, checksAPI, namespacesClient) cw.pollFreq = 10 * time.Millisecond return checksAPI, cw @@ -180,7 +180,7 @@ func TestCheckWatcher_Skip(t *testing.T) { logger := testlog.HCLogger(t) checksAPI := newFakeChecksAPI() - namespacesClient := NewNamespacesClient(NewMockNamespaces(nil)) + namespacesClient := NewNamespacesClient(NewMockNamespaces(nil), NewMockAgent(ossFeatures)) cw := newCheckWatcher(logger, checksAPI, namespacesClient) restarter1 := newFakeCheckRestarter(cw, "testalloc1", "testtask1", "testcheck1", check) diff --git a/command/agent/consul/connect_proxies_test.go b/command/agent/consul/connect_proxies_test.go index 7ae8dac2f7ca..235b319c1157 100644 --- a/command/agent/consul/connect_proxies_test.go +++ b/command/agent/consul/connect_proxies_test.go @@ -7,8 +7,7 @@ import ( ) func TestConnectProxies_Proxies(t *testing.T) { - agentAPI := NewMockAgent() - pc := NewConnectProxiesClient(agentAPI) + pc := NewConnectProxiesClient(NewMockAgent(ossFeatures)) proxies, err := pc.Proxies() require.NoError(t, err) diff --git a/command/agent/consul/group_test.go b/command/agent/consul/group_test.go index 2dd506a2e108..b52b7f8c35a0 100644 --- a/command/agent/consul/group_test.go +++ b/command/agent/consul/group_test.go @@ -32,7 +32,7 @@ func TestConsul_Connect(t *testing.T) { consulConfig.Address = testconsul.HTTPAddr consulClient, err := consulapi.NewClient(consulConfig) require.NoError(t, err) - namespacesClient := NewNamespacesClient(consulClient.Namespaces()) + namespacesClient := NewNamespacesClient(consulClient.Namespaces(), consulClient.Agent()) serviceClient := NewServiceClient(consulClient.Agent(), namespacesClient, testlog.HCLogger(t), true) // Lower periodicInterval to ensure periodic syncing doesn't improperly diff --git a/command/agent/consul/int_test.go b/command/agent/consul/int_test.go index 318a46f3483a..f725afd638b2 100644 --- a/command/agent/consul/int_test.go +++ b/command/agent/consul/int_test.go @@ -135,7 +135,7 @@ func TestConsul_Integration(t *testing.T) { consulClient, err := consulapi.NewClient(consulConfig) r.Nil(err) - namespacesClient := consul.NewNamespacesClient(consulClient.Namespaces()) + namespacesClient := consul.NewNamespacesClient(consulClient.Namespaces(), consulClient.Agent()) serviceClient := consul.NewServiceClient(consulClient.Agent(), namespacesClient, testlog.HCLogger(t), true) defer serviceClient.Shutdown() // just-in-case cleanup consulRan := make(chan struct{}) diff --git a/command/agent/consul/namespaces_client.go b/command/agent/consul/namespaces_client.go index 0bcbb10e2bb4..5d5f6a5a62eb 100644 --- a/command/agent/consul/namespaces_client.go +++ b/command/agent/consul/namespaces_client.go @@ -2,34 +2,87 @@ package consul import ( "sort" - "strings" + "sync" + "time" +) + +const ( + // namespaceEnabledCacheTTL is how long to cache the response from Consul + // /v1/agent/self API, which is used to determine whether namespaces are + // available. + namespaceEnabledCacheTTL = 1 * time.Minute ) // NamespacesClient is a wrapper for the Consul NamespacesAPI, that is used to // deal with Consul OSS vs Consul Enterprise behavior in listing namespaces. type NamespacesClient struct { namespacesAPI NamespaceAPI + agentAPI AgentAPI + + lock sync.Mutex + enabled bool // namespaces requires Ent + Namespaces feature + updated time.Time // memoize response for a while } // NewNamespacesClient returns a NamespacesClient backed by a NamespaceAPI. -func NewNamespacesClient(namespacesAPI NamespaceAPI) *NamespacesClient { +func NewNamespacesClient(namespacesAPI NamespaceAPI, agentAPI AgentAPI) *NamespacesClient { return &NamespacesClient{ namespacesAPI: namespacesAPI, + agentAPI: agentAPI, } } +func stale(updated, now time.Time) bool { + return now.After(updated.Add(namespaceEnabledCacheTTL)) +} + +func (ns *NamespacesClient) allowable(now time.Time) bool { + ns.lock.Lock() + defer ns.lock.Unlock() + + if !stale(ns.updated, now) { + return ns.enabled + } + + self, err := ns.agentAPI.Self() + if err != nil { + return ns.enabled + } + + sku, ok := SKU(self) + if !ok { + return ns.enabled + } + + if sku != "ent" { + ns.enabled = false + ns.updated = now + return ns.enabled + } + + enabledStr, ok := Namespaces(self) + if !ok { + return ns.enabled + } + + ns.enabled = enabledStr == "true" + ns.updated = now + return ns.enabled +} + // List returns a list of Consul Namespaces. // -// If using Consul OSS, the list is a single element with the "default" namespace, -// even though the response from Consul OSS is an error. +// TODO(shoenig): return empty string instead of "default" when namespaces are not +// enabled. (Coming in followup PR). func (ns *NamespacesClient) List() ([]string, error) { + if !ns.allowable(time.Now()) { + // TODO(shoenig): lets return the empty string instead, that way we do not + // need to normalize at call sites later on + return []string{"default"}, nil + } + namespaces, _, err := ns.namespacesAPI.List(nil) if err != nil { - // check if the error was a 404, indicating Consul is the OSS version - // which does not have the /v1/namespace handler - if strings.Contains(err.Error(), "response code: 404") { - return []string{"default"}, nil - } return nil, err } diff --git a/command/agent/consul/namespaces_client_test.go b/command/agent/consul/namespaces_client_test.go new file mode 100644 index 000000000000..bc7ebdf37ad2 --- /dev/null +++ b/command/agent/consul/namespaces_client_test.go @@ -0,0 +1,117 @@ +package consul + +import ( + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestNamespacesClient_List(t *testing.T) { + t.Parallel() + + t.Run("oss", func(t *testing.T) { + c := NewNamespacesClient(NewMockNamespaces(nil), NewMockAgent(Features{ + Enterprise: false, + Namespaces: false, + })) + list, err := c.List() + require.NoError(t, err) + require.Equal(t, []string{"default"}, list) // todo(shoenig): change in followup PR + }) + + t.Run("ent without namespaces", func(t *testing.T) { + c := NewNamespacesClient(NewMockNamespaces(nil), NewMockAgent(Features{ + Enterprise: true, + Namespaces: false, + })) + list, err := c.List() + require.NoError(t, err) + require.Equal(t, []string{"default"}, list) // todo(shoenig): change in followup PR + }) + + t.Run("ent with namespaces", func(t *testing.T) { + c := NewNamespacesClient(NewMockNamespaces([]string{"banana", "apple", "cherry"}), NewMockAgent(Features{ + Enterprise: true, + Namespaces: true, + })) + list, err := c.List() + require.NoError(t, err) + + // remember default always exists... if enterprise and namespaces are enabled + require.Equal(t, []string{"apple", "banana", "cherry", "default"}, list) + }) +} + +func TestNewNamespacesClient_stale(t *testing.T) { + t.Parallel() + + t.Run("ok", func(t *testing.T) { + now := time.Now() + updated := now.Add(-59 * time.Second) + result := stale(updated, now) + require.False(t, result) + }) + + t.Run("stale", func(t *testing.T) { + now := time.Now() + updated := now.Add(-61 * time.Second) + result := stale(updated, now) + require.True(t, result) + }) +} + +func TestNewNamespacesClient_allowable(t *testing.T) { + t.Parallel() + + try := func(ent, feature, enabled, exp bool, updated, now time.Time) { + expired := now.After(updated.Add(namespaceEnabledCacheTTL)) + name := fmt.Sprintf("ent:%t_feature:%t_enabled:%t_exp:%t_expired:%t", ent, feature, enabled, exp, expired) + t.Run(name, func(t *testing.T) { + c := NewNamespacesClient(NewMockNamespaces([]string{"a", "b"}), NewMockAgent(Features{ + Enterprise: ent, + Namespaces: feature, + })) + + // put the client into the state we want + c.enabled = enabled + c.updated = updated + + result := c.allowable(now) + require.Equal(t, exp, result) + require.Equal(t, exp, c.enabled) // cached value should match result + }) + } + + previous := time.Now() + over := previous.Add(namespaceEnabledCacheTTL + 1) + under := previous.Add(namespaceEnabledCacheTTL - 1) + + // oss, no refresh, no state change + try(false, false, false, false, previous, under) + + // oss, refresh, no state change + try(false, false, false, false, previous, over) + + // ent->oss, refresh, state change + try(false, false, true, false, previous, over) + + // ent, disabled, no refresh, no state change + try(true, false, false, false, previous, under) + + // ent, disabled, refresh, no state change + try(true, false, false, false, previous, over) + + // ent, enabled, no refresh, no state change + try(true, true, true, true, previous, under) + + // ent, enabled, refresh, no state change + try(true, true, true, true, previous, over) + + // ent, disabled, refresh, state change (i.e. new license with namespaces) + try(true, true, false, true, previous, over) // ??? + + // ent, disabled, refresh, no state change yet (i.e. new license with namespaces, still cached without) + try(true, true, false, false, previous, under) +} diff --git a/command/agent/consul/self.go b/command/agent/consul/self.go new file mode 100644 index 000000000000..1cdd48045af9 --- /dev/null +++ b/command/agent/consul/self.go @@ -0,0 +1,56 @@ +package consul + +import ( + "strings" + + "github.com/hashicorp/go-version" +) + +// Self represents the response body from Consul /v1/agent/self API endpoint. +// Care must always be taken to do type checks when casting, as structure could +// potentially change over time. +type Self = map[string]map[string]interface{} + +func SKU(info Self) (string, bool) { + v, ok := info["Config"]["Version"].(string) + if !ok { + return "", ok + } + + ver, vErr := version.NewVersion(v) + if vErr != nil { + return "", false + } + if strings.Contains(ver.Metadata(), "ent") { + return "ent", true + } + return "oss", true +} + +func Namespaces(info Self) (string, bool) { + return feature("Namespaces", info) +} + +// Feature returns whether the indicated feature is enabled by Consul and the +// associated License. +// possible values as of v1.9.5+ent: +// Automated Backups, Automated Upgrades, Enhanced Read Scalability, +// Network Segments, Redundancy Zone, Advanced Network Federation, +// Namespaces, SSO, Audit Logging +func feature(name string, info Self) (string, bool) { + lic, licOK := info["Stats"]["license"].(map[string]interface{}) + if !licOK { + return "", false + } + + features, exists := lic["features"].(string) + if !exists { + return "", false + } + + if !strings.Contains(features, name) { + return "", false + } + + return "true", true +} diff --git a/command/agent/consul/self_test.go b/command/agent/consul/self_test.go new file mode 100644 index 000000000000..a9620fd3077f --- /dev/null +++ b/command/agent/consul/self_test.go @@ -0,0 +1,98 @@ +package consul + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +var ( + ossFeatures = Features{ + Enterprise: false, + Namespaces: false, + } +) + +func TestSelf_SKU(t *testing.T) { + t.Parallel() + + t.Run("oss", func(t *testing.T) { + s, ok := SKU(Self{ + "Config": {"Version": "v1.9.5"}, + }) + require.True(t, ok) + require.Equal(t, "oss", s) + }) + + t.Run("oss dev", func(t *testing.T) { + s, ok := SKU(Self{ + "Config": {"Version": "v1.9.5-dev"}, + }) + require.True(t, ok) + require.Equal(t, "oss", s) + }) + + t.Run("ent", func(t *testing.T) { + s, ok := SKU(Self{ + "Config": {"Version": "v1.9.5+ent"}, + }) + require.True(t, ok) + require.Equal(t, "ent", s) + }) + + t.Run("ent dev", func(t *testing.T) { + s, ok := SKU(Self{ + "Config": {"Version": "v1.9.5+ent-dev"}, + }) + require.True(t, ok) + require.Equal(t, "ent", s) + }) + + t.Run("missing", func(t *testing.T) { + _, ok := SKU(Self{ + "Config": {}, + }) + require.False(t, ok) + }) + + t.Run("malformed", func(t *testing.T) { + _, ok := SKU(Self{ + "Config": {"Version": "***"}, + }) + require.False(t, ok) + }) +} + +func TestSelf_Namespaces(t *testing.T) { + t.Parallel() + + t.Run("supports namespaces", func(t *testing.T) { + s, ok := Namespaces(Self{ + "Stats": {"license": map[string]interface{}{"features": "Automated Backups, Automated Upgrades, Enhanced Read Scalability, Network Segments, Redundancy Zone, Advanced Network Federation, Namespaces, SSO, Audit Logging"}}, + }) + require.True(t, ok) + require.Equal(t, "true", s) + }) + + t.Run("no namespaces", func(t *testing.T) { + _, ok := Namespaces(Self{ + "Stats": {"license": map[string]interface{}{"features": "Automated Backups, Automated Upgrades, Enhanced Read Scalability, Network Segments, Redundancy Zone, Advanced Network Federation, SSO, Audit Logging"}}, + }) + require.False(t, ok) + }) + + t.Run("stats missing", func(t *testing.T) { + _, ok := Namespaces(Self{}) + require.False(t, ok) + }) + + t.Run("license missing", func(t *testing.T) { + _, ok := Namespaces(Self{"Stats": {}}) + require.False(t, ok) + }) + + t.Run("features missing", func(t *testing.T) { + _, ok := Namespaces(Self{"Stats": {"license": map[string]interface{}{}}}) + require.False(t, ok) + }) +} diff --git a/command/agent/consul/service_client_test.go b/command/agent/consul/service_client_test.go index 6bc91aaf3820..9129882ee589 100644 --- a/command/agent/consul/service_client_test.go +++ b/command/agent/consul/service_client_test.go @@ -336,8 +336,8 @@ func TestSyncLogic_maybeTweakTags_emptySC(t *testing.T) { func TestServiceRegistration_CheckOnUpdate(t *testing.T) { t.Parallel() - mockAgent := NewMockAgent() - namespacesClient := NewNamespacesClient(NewMockNamespaces(nil)) + mockAgent := NewMockAgent(ossFeatures) + namespacesClient := NewNamespacesClient(NewMockNamespaces(nil), mockAgent) logger := testlog.HCLogger(t) sc := NewServiceClient(mockAgent, namespacesClient, logger, true) diff --git a/command/agent/consul/unit_test.go b/command/agent/consul/unit_test.go index b9c385ae4731..b3f035ad373e 100644 --- a/command/agent/consul/unit_test.go +++ b/command/agent/consul/unit_test.go @@ -105,18 +105,18 @@ func (t *testFakeCtx) syncOnce(reason syncReason) error { // setupFake creates a testFakeCtx with a ServiceClient backed by a fakeConsul. // A test Workload is also provided. func setupFake(t *testing.T) *testFakeCtx { - fc := NewMockAgent() - nsc := NewNamespacesClient(NewMockNamespaces(nil)) - tw := testWorkload() + agentClient := NewMockAgent(ossFeatures) + nsClient := NewNamespacesClient(NewMockNamespaces(nil), agentClient) + workload := testWorkload() // by default start fake client being out of probation - sc := NewServiceClient(fc, nsc, testlog.HCLogger(t), true) - sc.deregisterProbationExpiry = time.Now().Add(-1 * time.Minute) + serviceClient := NewServiceClient(agentClient, nsClient, testlog.HCLogger(t), true) + serviceClient.deregisterProbationExpiry = time.Now().Add(-1 * time.Minute) return &testFakeCtx{ - ServiceClient: sc, - FakeConsul: fc, - Workload: tw, + ServiceClient: serviceClient, + FakeConsul: agentClient, + Workload: workload, } } From 4b3ed53511ea3d3e866566c57c78570489336b0b Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Mon, 7 Jun 2021 15:22:26 -0500 Subject: [PATCH 2/2] consul: pr cleanup namespace probe function signatures --- client/fingerprint/consul.go | 2 +- client/fingerprint/consul_test.go | 63 +++++++++++++---------- command/agent/consul/namespaces_client.go | 7 +-- command/agent/consul/self.go | 19 ++++--- command/agent/consul/self_test.go | 21 ++++---- 5 files changed, 56 insertions(+), 56 deletions(-) diff --git a/client/fingerprint/consul.go b/client/fingerprint/consul.go index adfbe4c4e162..f562672332b9 100644 --- a/client/fingerprint/consul.go +++ b/client/fingerprint/consul.go @@ -189,5 +189,5 @@ func (f *ConsulFingerprint) grpc(info agentconsul.Self) (string, bool) { } func (f *ConsulFingerprint) namespaces(info agentconsul.Self) (string, bool) { - return agentconsul.Namespaces(info) + return strconv.FormatBool(agentconsul.Namespaces(info)), true } diff --git a/client/fingerprint/consul_test.go b/client/fingerprint/consul_test.go index ae373209eb5d..fb7ff3ca8c0c 100644 --- a/client/fingerprint/consul_test.go +++ b/client/fingerprint/consul_test.go @@ -326,33 +326,38 @@ func TestConsulFingerprint_namespaces(t *testing.T) { fp := newConsulFingerPrint(t) t.Run("supports namespaces", func(t *testing.T) { - s, ok := fp.namespaces(agentconsul.Self{ + value, ok := fp.namespaces(agentconsul.Self{ "Stats": {"license": map[string]interface{}{"features": "Automated Backups, Automated Upgrades, Enhanced Read Scalability, Network Segments, Redundancy Zone, Advanced Network Federation, Namespaces, SSO, Audit Logging"}}, }) require.True(t, ok) - require.Equal(t, "true", s) + require.Equal(t, "true", value) }) t.Run("no namespaces", func(t *testing.T) { - _, ok := fp.namespaces(agentconsul.Self{ + value, ok := fp.namespaces(agentconsul.Self{ "Stats": {"license": map[string]interface{}{"features": "Automated Backups, Automated Upgrades, Enhanced Read Scalability, Network Segments, Redundancy Zone, Advanced Network Federation, SSO, Audit Logging"}}, }) - require.False(t, ok) + require.True(t, ok) + require.Equal(t, "false", value) + }) t.Run("stats missing", func(t *testing.T) { - _, ok := fp.namespaces(agentconsul.Self{}) - require.False(t, ok) + value, ok := fp.namespaces(agentconsul.Self{}) + require.True(t, ok) + require.Equal(t, "false", value) }) t.Run("license missing", func(t *testing.T) { - _, ok := fp.namespaces(agentconsul.Self{"Stats": {}}) - require.False(t, ok) + value, ok := fp.namespaces(agentconsul.Self{"Stats": {}}) + require.True(t, ok) + require.Equal(t, "false", value) }) t.Run("features missing", func(t *testing.T) { - _, ok := fp.namespaces(agentconsul.Self{"Stats": {"license": map[string]interface{}{}}}) - require.False(t, ok) + value, ok := fp.namespaces(agentconsul.Self{"Stats": {"license": map[string]interface{}{}}}) + require.True(t, ok) + require.Equal(t, "false", value) }) } @@ -372,15 +377,16 @@ func TestConsulFingerprint_Fingerprint_oss(t *testing.T) { err := cf.Fingerprint(&FingerprintRequest{Config: cfg, Node: node}, &resp) require.NoError(t, err) require.Equal(t, map[string]string{ - "consul.datacenter": "dc1", - "consul.revision": "3c1c22679", - "consul.segment": "seg1", - "consul.server": "true", - "consul.sku": "oss", - "consul.version": "1.9.5", - "consul.connect": "true", - "consul.grpc": "8502", - "unique.consul.name": "HAL9000", + "consul.datacenter": "dc1", + "consul.revision": "3c1c22679", + "consul.segment": "seg1", + "consul.server": "true", + "consul.sku": "oss", + "consul.version": "1.9.5", + "consul.connect": "true", + "consul.grpc": "8502", + "consul.ft.namespaces": "false", + "unique.consul.name": "HAL9000", }, resp.Attributes) require.True(t, resp.Detected) @@ -425,15 +431,16 @@ func TestConsulFingerprint_Fingerprint_oss(t *testing.T) { err3 := cf.Fingerprint(&FingerprintRequest{Config: cfg, Node: node}, &resp3) require.NoError(t, err3) require.Equal(t, map[string]string{ - "consul.datacenter": "dc1", - "consul.revision": "3c1c22679", - "consul.segment": "seg1", - "consul.server": "true", - "consul.sku": "oss", - "consul.version": "1.9.5", - "consul.connect": "true", - "consul.grpc": "8502", - "unique.consul.name": "HAL9000", + "consul.datacenter": "dc1", + "consul.revision": "3c1c22679", + "consul.segment": "seg1", + "consul.server": "true", + "consul.sku": "oss", + "consul.version": "1.9.5", + "consul.connect": "true", + "consul.grpc": "8502", + "consul.ft.namespaces": "false", + "unique.consul.name": "HAL9000", }, resp3.Attributes) // consul now available again diff --git a/command/agent/consul/namespaces_client.go b/command/agent/consul/namespaces_client.go index 5d5f6a5a62eb..974b2e705a3b 100644 --- a/command/agent/consul/namespaces_client.go +++ b/command/agent/consul/namespaces_client.go @@ -60,12 +60,7 @@ func (ns *NamespacesClient) allowable(now time.Time) bool { return ns.enabled } - enabledStr, ok := Namespaces(self) - if !ok { - return ns.enabled - } - - ns.enabled = enabledStr == "true" + ns.enabled = Namespaces(self) ns.updated = now return ns.enabled } diff --git a/command/agent/consul/self.go b/command/agent/consul/self.go index 1cdd48045af9..d0edee2758cd 100644 --- a/command/agent/consul/self.go +++ b/command/agent/consul/self.go @@ -27,30 +27,29 @@ func SKU(info Self) (string, bool) { return "oss", true } -func Namespaces(info Self) (string, bool) { +// Namespaces returns true if the "Namespaces" feature is enabled in Consul, and +// false otherwise. Consul OSS will always return false, and Consul ENT will return +// false if the license file does not contain the necessary feature. +func Namespaces(info Self) bool { return feature("Namespaces", info) } -// Feature returns whether the indicated feature is enabled by Consul and the +// feature returns whether the indicated feature is enabled by Consul and the // associated License. // possible values as of v1.9.5+ent: // Automated Backups, Automated Upgrades, Enhanced Read Scalability, // Network Segments, Redundancy Zone, Advanced Network Federation, // Namespaces, SSO, Audit Logging -func feature(name string, info Self) (string, bool) { +func feature(name string, info Self) bool { lic, licOK := info["Stats"]["license"].(map[string]interface{}) if !licOK { - return "", false + return false } features, exists := lic["features"].(string) if !exists { - return "", false - } - - if !strings.Contains(features, name) { - return "", false + return false } - return "true", true + return strings.Contains(features, name) } diff --git a/command/agent/consul/self_test.go b/command/agent/consul/self_test.go index a9620fd3077f..3089c2422049 100644 --- a/command/agent/consul/self_test.go +++ b/command/agent/consul/self_test.go @@ -67,32 +67,31 @@ func TestSelf_Namespaces(t *testing.T) { t.Parallel() t.Run("supports namespaces", func(t *testing.T) { - s, ok := Namespaces(Self{ + enabled := Namespaces(Self{ "Stats": {"license": map[string]interface{}{"features": "Automated Backups, Automated Upgrades, Enhanced Read Scalability, Network Segments, Redundancy Zone, Advanced Network Federation, Namespaces, SSO, Audit Logging"}}, }) - require.True(t, ok) - require.Equal(t, "true", s) + require.True(t, enabled) }) t.Run("no namespaces", func(t *testing.T) { - _, ok := Namespaces(Self{ + enabled := Namespaces(Self{ "Stats": {"license": map[string]interface{}{"features": "Automated Backups, Automated Upgrades, Enhanced Read Scalability, Network Segments, Redundancy Zone, Advanced Network Federation, SSO, Audit Logging"}}, }) - require.False(t, ok) + require.False(t, enabled) }) t.Run("stats missing", func(t *testing.T) { - _, ok := Namespaces(Self{}) - require.False(t, ok) + enabled := Namespaces(Self{}) + require.False(t, enabled) }) t.Run("license missing", func(t *testing.T) { - _, ok := Namespaces(Self{"Stats": {}}) - require.False(t, ok) + enabled := Namespaces(Self{"Stats": {}}) + require.False(t, enabled) }) t.Run("features missing", func(t *testing.T) { - _, ok := Namespaces(Self{"Stats": {"license": map[string]interface{}{}}}) - require.False(t, ok) + enabled := Namespaces(Self{"Stats": {"license": map[string]interface{}{}}}) + require.False(t, enabled) }) }