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 6eef86ca8cb..42d66abedcd 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 59d3b7a7a5c..5789079d850 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 9ec54bc4609..a1c368a42ef 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 58e0ed988dd..98d20440650 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 869042d58aa..c59f4765987 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 e37ffb1a888..adfbe4c4e16 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 e5eda50b292..ae373209eb5 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 ff20dc99ab4..a500dea7f43 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 f34f0524414..69eb78f6e90 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 0267699c33c..c24c4d10d6c 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 7ae8dac2f7c..235b319c115 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 2dd506a2e10..b52b7f8c35a 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 318a46f3483..f725afd638b 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 0bcbb10e2bb..5d5f6a5a62e 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 00000000000..bc7ebdf37ad --- /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 00000000000..1cdd48045af --- /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 00000000000..a9620fd3077 --- /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 6bc91aaf382..9129882ee58 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 b9c385ae473..b3f035ad373 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 adfbe4c4e16..f562672332b 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 ae373209eb5..fb7ff3ca8c0 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 5d5f6a5a62e..974b2e705a3 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 1cdd48045af..d0edee2758c 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 a9620fd3077..3089c242204 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) }) }