Merge pull request #10988 from hashicorp/dnephin/acl-legacy-remove-config

acl: isolate deprecated config and warn when they are used
2025-01-27 14:05:45 +00:00 · 2021-09-29 11:40:14 -04:00 · 2021-09-29 11:40:14 -04:00 · 8d1378cc1d
commit 8d1378cc1d
parent 6b33e3bfd7 e8ac5fd90b
7 changed files with 221 additions and 45 deletions
--- a/agent/config/builder.go
+++ b/agent/config/builder.go
@ -745,13 +745,6 @@ func (b *builder) build() (rt RuntimeConfig, err error) {
 		primaryDatacenter = datacenter
 	}

-	enableTokenReplication := false
-	if c.ACLReplicationToken != nil {
-		enableTokenReplication = true
-	}
-
-	boolValWithDefault(c.ACL.TokenReplication, boolValWithDefault(c.EnableACLReplication, enableTokenReplication))
-
 	enableRemoteScriptChecks := boolVal(c.EnableScriptChecks)
 	enableLocalScriptChecks := boolValWithDefault(c.EnableLocalScriptChecks, enableRemoteScriptChecks)

@ -863,24 +856,24 @@ func (b *builder) build() (rt RuntimeConfig, err error) {
 			Datacenter:       datacenter,
 			NodeName:         b.nodeName(c.NodeName),
 			ACLPolicyTTL:     b.durationVal("acl.policy_ttl", c.ACL.PolicyTTL),
-			ACLTokenTTL:      b.durationValWithDefault("acl.token_ttl", c.ACL.TokenTTL, b.durationVal("acl_ttl", c.ACLTTL)),
+			ACLTokenTTL:      b.durationVal("acl.token_ttl", c.ACL.TokenTTL),
 			ACLRoleTTL:       b.durationVal("acl.role_ttl", c.ACL.RoleTTL),
-			ACLDownPolicy:    stringValWithDefault(c.ACL.DownPolicy, stringVal(c.ACLDownPolicy)),
-			ACLDefaultPolicy: stringValWithDefault(c.ACL.DefaultPolicy, stringVal(c.ACLDefaultPolicy)),
+			ACLDownPolicy:    stringVal(c.ACL.DownPolicy),
+			ACLDefaultPolicy: stringVal(c.ACL.DefaultPolicy),
 		},

-		ACLEnableKeyListPolicy: boolValWithDefault(c.ACL.EnableKeyListPolicy, boolVal(c.ACLEnableKeyListPolicy)),
-		ACLMasterToken:         stringValWithDefault(c.ACL.Tokens.Master, stringVal(c.ACLMasterToken)),
+		ACLEnableKeyListPolicy: boolVal(c.ACL.EnableKeyListPolicy),
+		ACLMasterToken:         stringVal(c.ACL.Tokens.Master),

-		ACLTokenReplication: boolValWithDefault(c.ACL.TokenReplication, boolValWithDefault(c.EnableACLReplication, enableTokenReplication)),
+		ACLTokenReplication: boolVal(c.ACL.TokenReplication),

 		ACLTokens: token.Config{
 			DataDir:             dataDir,
 			EnablePersistence:   boolValWithDefault(c.ACL.EnableTokenPersistence, false),
-			ACLDefaultToken:     stringValWithDefault(c.ACL.Tokens.Default, stringVal(c.ACLToken)),
-			ACLAgentToken:       stringValWithDefault(c.ACL.Tokens.Agent, stringVal(c.ACLAgentToken)),
+			ACLDefaultToken:     stringVal(c.ACL.Tokens.Default),
+			ACLAgentToken:       stringVal(c.ACL.Tokens.Agent),
 			ACLAgentMasterToken: stringVal(c.ACL.Tokens.AgentMaster),
-			ACLReplicationToken: stringValWithDefault(c.ACL.Tokens.Replication, stringVal(c.ACLReplicationToken)),
+			ACLReplicationToken: stringVal(c.ACL.Tokens.Replication),
 		},

 		// Autopilot
--- a/agent/config/config.go
+++ b/agent/config/config.go
@ -130,22 +130,6 @@ type Cache struct {
 // configuration it should be treated as an external API which cannot be
 // changed and refactored at will since this will break existing setups.
 type Config struct {
-	// DEPRECATED (ACL-Legacy-Compat) - moved into the "acl.tokens" stanza
-	ACLAgentToken *string `mapstructure:"acl_agent_token"`
-	// DEPRECATED (ACL-Legacy-Compat) - moved into the "acl" stanza
-	ACLDefaultPolicy *string `mapstructure:"acl_default_policy"`
-	// DEPRECATED (ACL-Legacy-Compat) - moved into the "acl" stanza
-	ACLDownPolicy *string `mapstructure:"acl_down_policy"`
-	// DEPRECATED (ACL-Legacy-Compat) - moved into the "acl" stanza
-	ACLEnableKeyListPolicy *bool `mapstructure:"acl_enable_key_list_policy"`
-	// DEPRECATED (ACL-Legacy-Compat) - moved into the "acl" stanza
-	ACLMasterToken *string `mapstructure:"acl_master_token"`
-	// DEPRECATED (ACL-Legacy-Compat) - moved into the "acl.tokens" stanza
-	ACLReplicationToken *string `mapstructure:"acl_replication_token"`
-	// DEPRECATED (ACL-Legacy-Compat) - moved into the "acl.tokens" stanza
-	ACLTTL *string `mapstructure:"acl_ttl"`
-	// DEPRECATED (ACL-Legacy-Compat) - moved into the "acl.tokens" stanza
-	ACLToken                         *string             `mapstructure:"acl_token"`
 	ACL                              ACL                 `mapstructure:"acl"`
 	Addresses                        Addresses           `mapstructure:"addresses"`
 	AdvertiseAddrLAN                 *string             `mapstructure:"advertise_addr"`
@ -188,7 +172,6 @@ type Config struct {
 	DisableUpdateCheck               *bool               `mapstructure:"disable_update_check"`
 	DiscardCheckOutput               *bool               `mapstructure:"discard_check_output"`
 	DiscoveryMaxStale                *string             `mapstructure:"discovery_max_stale"`
-	EnableACLReplication             *bool               `mapstructure:"enable_acl_replication"`
 	EnableAgentTLSForChecks          *bool               `mapstructure:"enable_agent_tls_for_checks"`
 	EnableCentralServiceConfig       *bool               `mapstructure:"enable_central_service_config"`
 	EnableDebug                      *bool               `mapstructure:"enable_debug"`
--- a/agent/config/default.go
+++ b/agent/config/default.go
@ -27,11 +27,11 @@ func DefaultSource() Source {
 		Name:   "default",
 		Format: "hcl",
 		Data: `
-		acl_default_policy = "allow"
-		acl_down_policy = "extend-cache"
-		acl_ttl = "30s"
 		acl = {
+			token_ttl = "30s"
 			policy_ttl = "30s"
+			default_policy = "allow"
+			down_policy = "extend-cache"
 		}
 		bind_addr = "0.0.0.0"
 		bootstrap = false
--- a/agent/config/deprecated.go
+++ b/agent/config/deprecated.go
@ -5,8 +5,29 @@ import "fmt"
 type DeprecatedConfig struct {
 	// DEPRECATED (ACL-Legacy-Compat) - moved into the "acl.tokens" stanza
 	ACLAgentMasterToken *string `mapstructure:"acl_agent_master_token"`
+	// DEPRECATED (ACL-Legacy-Compat) - moved into the "acl.tokens" stanza
+	ACLAgentToken *string `mapstructure:"acl_agent_token"`
+	// DEPRECATED (ACL-Legacy-Compat) - moved into the "acl.tokens" stanza
+	ACLToken *string `mapstructure:"acl_token"`
+	// DEPRECATED (ACL-Legacy-Compat) - moved to "acl.enable_key_list_policy"
+	ACLEnableKeyListPolicy *bool `mapstructure:"acl_enable_key_list_policy"`
+
+	// DEPRECATED (ACL-Legacy-Compat) - moved into the "acl" stanza
+	ACLMasterToken *string `mapstructure:"acl_master_token"`
+	// DEPRECATED (ACL-Legacy-Compat) - moved into the "acl.tokens" stanza
+	ACLReplicationToken *string `mapstructure:"acl_replication_token"`
+	// DEPRECATED (ACL-Legacy-Compat) - moved to "acl.enable_token_replication"
+	EnableACLReplication *bool `mapstructure:"enable_acl_replication"`
+
 	// DEPRECATED (ACL-Legacy-Compat) - moved to "primary_datacenter"
 	ACLDatacenter *string `mapstructure:"acl_datacenter"`
+
+	// DEPRECATED (ACL-Legacy-Compat) - moved to "acl.default_policy"
+	ACLDefaultPolicy *string `mapstructure:"acl_default_policy"`
+	// DEPRECATED (ACL-Legacy-Compat) - moved to "acl.down_policy"
+	ACLDownPolicy *string `mapstructure:"acl_down_policy"`
+	// DEPRECATED (ACL-Legacy-Compat) - moved to "acl.token_ttl"
+	ACLTTL *string `mapstructure:"acl_ttl"`
 }

 func applyDeprecatedConfig(d *decodeTarget) (Config, []string) {
@ -20,6 +41,42 @@ func applyDeprecatedConfig(d *decodeTarget) (Config, []string) {
 		warns = append(warns, deprecationWarning("acl_agent_master_token", "acl.tokens.agent_master"))
 	}

+	if dep.ACLAgentToken != nil {
+		if d.Config.ACL.Tokens.Agent == nil {
+			d.Config.ACL.Tokens.Agent = dep.ACLAgentToken
+		}
+		warns = append(warns, deprecationWarning("acl_agent_token", "acl.tokens.agent"))
+	}
+
+	if dep.ACLToken != nil {
+		if d.Config.ACL.Tokens.Default == nil {
+			d.Config.ACL.Tokens.Default = dep.ACLToken
+		}
+		warns = append(warns, deprecationWarning("acl_token", "acl.tokens.default"))
+	}
+
+	if dep.ACLMasterToken != nil {
+		if d.Config.ACL.Tokens.Master == nil {
+			d.Config.ACL.Tokens.Master = dep.ACLMasterToken
+		}
+		warns = append(warns, deprecationWarning("acl_master_token", "acl.tokens.master"))
+	}
+
+	if dep.ACLReplicationToken != nil {
+		if d.Config.ACL.Tokens.Replication == nil {
+			d.Config.ACL.Tokens.Replication = dep.ACLReplicationToken
+		}
+		d.Config.ACL.TokenReplication = pBool(true)
+		warns = append(warns, deprecationWarning("acl_replication_token", "acl.tokens.replication"))
+	}
+
+	if dep.EnableACLReplication != nil {
+		if d.Config.ACL.TokenReplication == nil {
+			d.Config.ACL.TokenReplication = dep.EnableACLReplication
+		}
+		warns = append(warns, deprecationWarning("enable_acl_replication", "acl.enable_token_replication"))
+	}
+
 	if dep.ACLDatacenter != nil {
 		if d.Config.PrimaryDatacenter == nil {
 			d.Config.PrimaryDatacenter = dep.ACLDatacenter
@ -30,6 +87,34 @@ func applyDeprecatedConfig(d *decodeTarget) (Config, []string) {
 		warns = append(warns, deprecationWarning("acl_datacenter", "primary_datacenter"))
 	}

+	if dep.ACLDefaultPolicy != nil {
+		if d.Config.ACL.DefaultPolicy == nil {
+			d.Config.ACL.DefaultPolicy = dep.ACLDefaultPolicy
+		}
+		warns = append(warns, deprecationWarning("acl_default_policy", "acl.default_policy"))
+	}
+
+	if dep.ACLDownPolicy != nil {
+		if d.Config.ACL.DownPolicy == nil {
+			d.Config.ACL.DownPolicy = dep.ACLDownPolicy
+		}
+		warns = append(warns, deprecationWarning("acl_down_policy", "acl.down_policy"))
+	}
+
+	if dep.ACLTTL != nil {
+		if d.Config.ACL.TokenTTL == nil {
+			d.Config.ACL.TokenTTL = dep.ACLTTL
+		}
+		warns = append(warns, deprecationWarning("acl_ttl", "acl.token_ttl"))
+	}
+
+	if dep.ACLEnableKeyListPolicy != nil {
+		if d.Config.ACL.EnableKeyListPolicy == nil {
+			d.Config.ACL.EnableKeyListPolicy = dep.ACLEnableKeyListPolicy
+		}
+		warns = append(warns, deprecationWarning("acl_enable_key_list_policy", "acl.enable_key_list_policy"))
+	}
+
 	return d.Config, warns
 }

--- a/agent/config/deprecated_test.go
+++ b/agent/config/deprecated_test.go
@ -0,0 +1,93 @@
+package config
+
+import (
+	"sort"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestLoad_DeprecatedConfig(t *testing.T) {
+	opts := LoadOpts{
+		HCL: []string{`
+data_dir = "/foo"
+
+acl_datacenter = "dcone"
+
+acl_agent_master_token = "token1"
+acl_agent_token = "token2"
+acl_token = "token3"
+
+acl_master_token = "token4"
+acl_replication_token = "token5"
+
+acl_default_policy = "deny"
+acl_down_policy = "async-cache"
+
+acl_ttl = "3h"
+acl_enable_key_list_policy = true
+
+`},
+	}
+	patchLoadOptsShims(&opts)
+	result, err := Load(opts)
+	require.NoError(t, err)
+
+	expectWarns := []string{
+		deprecationWarning("acl_agent_master_token", "acl.tokens.agent_master"),
+		deprecationWarning("acl_agent_token", "acl.tokens.agent"),
+		deprecationWarning("acl_datacenter", "primary_datacenter"),
+		deprecationWarning("acl_default_policy", "acl.default_policy"),
+		deprecationWarning("acl_down_policy", "acl.down_policy"),
+		deprecationWarning("acl_enable_key_list_policy", "acl.enable_key_list_policy"),
+		deprecationWarning("acl_master_token", "acl.tokens.master"),
+		deprecationWarning("acl_replication_token", "acl.tokens.replication"),
+		deprecationWarning("acl_token", "acl.tokens.default"),
+		deprecationWarning("acl_ttl", "acl.token_ttl"),
+	}
+	sort.Strings(result.Warnings)
+	require.Equal(t, expectWarns, result.Warnings)
+	// Ideally this would compare against the entire result.RuntimeConfig, but
+	// we have so many non-zero defaults in that response that the noise of those
+	// defaults makes this test difficult to read. So as a workaround, compare
+	// specific values.
+	rt := result.RuntimeConfig
+	require.Equal(t, true, rt.ACLsEnabled)
+	require.Equal(t, "dcone", rt.PrimaryDatacenter)
+	require.Equal(t, "token1", rt.ACLTokens.ACLAgentMasterToken)
+	require.Equal(t, "token2", rt.ACLTokens.ACLAgentToken)
+	require.Equal(t, "token3", rt.ACLTokens.ACLDefaultToken)
+	require.Equal(t, "token4", rt.ACLMasterToken)
+	require.Equal(t, "token5", rt.ACLTokens.ACLReplicationToken)
+	require.Equal(t, "deny", rt.ACLResolverSettings.ACLDefaultPolicy)
+	require.Equal(t, "async-cache", rt.ACLResolverSettings.ACLDownPolicy)
+	require.Equal(t, 3*time.Hour, rt.ACLResolverSettings.ACLTokenTTL)
+	require.Equal(t, true, rt.ACLEnableKeyListPolicy)
+}
+
+func TestLoad_DeprecatedConfig_ACLReplication(t *testing.T) {
+	opts := LoadOpts{
+		HCL: []string{`
+data_dir = "/foo"
+
+enable_acl_replication = true
+
+`},
+	}
+	patchLoadOptsShims(&opts)
+	result, err := Load(opts)
+	require.NoError(t, err)
+
+	expectWarns := []string{
+		deprecationWarning("enable_acl_replication", "acl.enable_token_replication"),
+	}
+	sort.Strings(result.Warnings)
+	require.Equal(t, expectWarns, result.Warnings)
+	// Ideally this would compare against the entire result.RuntimeConfig, but
+	// we have so many non-zero defaults in that response that the noise of those
+	// defaults makes this test difficult to read. So as a workaround, compare
+	// specific values.
+	rt := result.RuntimeConfig
+	require.Equal(t, true, rt.ACLTokenReplication)
+}
--- a/agent/config/runtime_test.go
+++ b/agent/config/runtime_test.go
@ -1633,16 +1633,28 @@ func TestLoad_IntegrationWithFlags(t *testing.T) {
 		expectedWarnings: []string{`The 'acl_datacenter' field is deprecated. Use the 'primary_datacenter' field instead.`},
 	})
 	run(t, testCase{
-		desc: "acl_replication_token enables acl replication",
-		args: []string{`-data-dir=` + dataDir},
-		json: []string{`{ "acl_replication_token": "a" }`},
-		hcl:  []string{`acl_replication_token = "a"`},
+		desc:             "acl_replication_token enables acl replication",
+		args:             []string{`-data-dir=` + dataDir},
+		json:             []string{`{ "acl_replication_token": "a" }`},
+		hcl:              []string{`acl_replication_token = "a"`},
+		expectedWarnings: []string{deprecationWarning("acl_replication_token", "acl.tokens.replication")},
 		expected: func(rt *RuntimeConfig) {
 			rt.ACLTokens.ACLReplicationToken = "a"
 			rt.ACLTokenReplication = true
 			rt.DataDir = dataDir
 		},
 	})
+	run(t, testCase{
+		desc: "acl.tokens.replace does not enable acl replication",
+		args: []string{`-data-dir=` + dataDir},
+		json: []string{`{ "acl": { "tokens": { "replication": "a" }}}`},
+		hcl:  []string{`acl { tokens { replication = "a"}}`},
+		expected: func(rt *RuntimeConfig) {
+			rt.ACLTokens.ACLReplicationToken = "a"
+			rt.ACLTokenReplication = false
+			rt.DataDir = dataDir
+		},
+	})
 	run(t, testCase{
 		desc: "acl_enforce_version_8 is deprecated",
 		args: []string{`-data-dir=` + dataDir},
@ -5902,8 +5914,17 @@ func TestLoad_FullConfig(t *testing.T) {
 	entFullRuntimeConfig(expected)

 	expectedWarns := []string{
-		`The 'acl_datacenter' field is deprecated. Use the 'primary_datacenter' field instead.`,
-		`The 'acl_agent_master_token' field is deprecated. Use the 'acl.tokens.agent_master' field instead.`,
+		deprecationWarning("acl_datacenter", "primary_datacenter"),
+		deprecationWarning("acl_agent_master_token", "acl.tokens.agent_master"),
+		deprecationWarning("acl_agent_token", "acl.tokens.agent"),
+		deprecationWarning("acl_token", "acl.tokens.default"),
+		deprecationWarning("acl_master_token", "acl.tokens.master"),
+		deprecationWarning("acl_replication_token", "acl.tokens.replication"),
+		deprecationWarning("enable_acl_replication", "acl.enable_token_replication"),
+		deprecationWarning("acl_default_policy", "acl.default_policy"),
+		deprecationWarning("acl_down_policy", "acl.down_policy"),
+		deprecationWarning("acl_ttl", "acl.token_ttl"),
+		deprecationWarning("acl_enable_key_list_policy", "acl.enable_key_list_policy"),
 		`bootstrap_expect > 0: expecting 53 servers`,
 	}
 	expectedWarns = append(expectedWarns, enterpriseConfigKeyWarnings...)
--- a/website/content/docs/agent/options.mdx
+++ b/website/content/docs/agent/options.mdx
@ -759,10 +759,10 @@ Valid time units are 'ns', 'us' (or 'µs'), 'ms', 's', 'm', 'h'."
  running Consul 0.7 or later. When provided, this will enable [ACL replication](https://learn.hashicorp.com/tutorials/consul/access-control-replication-multiple-datacenters)
  using this ACL replication using this token to retrieve and replicate the ACLs
  to the non-authoritative local datacenter. In Consul 0.9.1 and later you can enable
-  ACL replication using [`enable_acl_replication`](#enable_acl_replication) and then
+  ACL replication using [`acl.enable_token_replication`](#acl_enable_token_replication) and then
  set the token later using the [agent token API](/api/agent#update-acl-tokens)
  on each server. If the `acl_replication_token` is set in the config, it will automatically
-  set [`enable_acl_replication`](#enable_acl_replication) to true for backward compatibility.
+  set [`acl.enable_token_replication`](#acl_enable_token_replication) to true for backward compatibility.

  If there's a partition or other outage affecting the authoritative datacenter, and the
  [`acl_down_policy`](/docs/agent/options#acl_down_policy) is set to "extend-cache", tokens not
@ -1447,7 +1447,8 @@ bind_addr = "{{ GetPrivateInterfaces | include \"network\" \"10.0.0.0/8\" | attr

 - `domain` Equivalent to the [`-domain` command-line flag](#_domain).

- `enable_acl_replication` When set on a Consul server, enables ACL replication without having to set
+- `enable_acl_replication` **Deprecated in Consul 1.11. Use the [`acl.enable_token_replication`](#acl_enable_token_replication) field instead.**
+  When set on a Consul server, enables ACL replication without having to set
  the replication token via [`acl_replication_token`](#acl_replication_token). Instead, enable ACL replication
  and then introduce the token using the [agent token API](/api/agent#update-acl-tokens) on each server.
  See [`acl_replication_token`](#acl_replication_token) for more details.