From b8da06a34d029cdd54043f04b48f0574b623e874 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 22 Sep 2021 18:55:53 -0400 Subject: [PATCH] acl: remove ACL upgrading from Clients As part of removing the legacy ACL system ACL upgrading and the flag for legacy ACLs is removed from Clients. This commit also removes the 'acls' serf tag from client nodes. The tag is only ever read from server nodes. This commit also introduces a constant for the acl serf tag, to make it easier to track where it is used. --- agent/acl_test.go | 4 ---- agent/agent.go | 1 - agent/consul/acl.go | 17 +++-------------- agent/consul/acl_client.go | 33 --------------------------------- agent/consul/acl_server.go | 1 + agent/consul/client.go | 17 +---------------- agent/consul/client_serf.go | 8 -------- agent/consul/server_serf.go | 5 ++--- agent/delegate_mock_test.go | 9 +++------ agent/metadata/server.go | 4 +++- 10 files changed, 13 insertions(+), 86 deletions(-) diff --git a/agent/acl_test.go b/agent/acl_test.go index 41407f61d..a97b9dbd9 100644 --- a/agent/acl_test.go +++ b/agent/acl_test.go @@ -76,10 +76,6 @@ func NewTestACLAgent(t *testing.T, name string, hcl string, resolveAuthz authzRe return a } -func (a *TestACLAgent) UseLegacyACLs() bool { - return false -} - func (a *TestACLAgent) ResolveToken(secretID string) (acl.Authorizer, error) { if a.resolveAuthzFn == nil { return nil, fmt.Errorf("ResolveToken call is unexpected - no authz resolver callback set") diff --git a/agent/agent.go b/agent/agent.go index 5a665c6cb..e5385abbc 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -143,7 +143,6 @@ type delegate interface { ResolveTokenAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (acl.Authorizer, error) RPC(method string, args interface{}, reply interface{}) error - UseLegacyACLs() bool SnapshotRPC(args *structs.SnapshotRequest, in io.Reader, out io.Writer, replyFn structs.SnapshotReplyFn) error Shutdown() error Stats() map[string]map[string]string diff --git a/agent/consul/acl.go b/agent/consul/acl.go index b84b100bf..1504627b3 100644 --- a/agent/consul/acl.go +++ b/agent/consul/acl.go @@ -83,10 +83,12 @@ const ( // currently used will backoff as it detects that it is remaining in legacy mode. // However the initial min value is kept small so that new cluster creation // can enter into new ACL mode quickly. + // TODO(ACL-Legacy-Compat): remove aclModeCheckMinInterval = 50 * time.Millisecond // aclModeCheckMaxInterval controls the maximum interval for how often the agent // checks if it should be using the new or legacy ACL system. + // TODO(ACL-Legacy-Compat): remove aclModeCheckMaxInterval = 30 * time.Second // Maximum number of re-resolution requests to be made if the token is modified between @@ -170,7 +172,6 @@ func tokenSecretCacheID(token string) string { type ACLResolverDelegate interface { ACLDatacenter(legacy bool) string - UseLegacyACLs() bool ResolveIdentityFromToken(token string) (bool, structs.ACLIdentity, error) ResolvePolicyFromID(policyID string) (bool, *structs.ACLPolicy, error) ResolveRoleFromID(roleID string) (bool, *structs.ACLRole, error) @@ -442,6 +443,7 @@ func (r *ACLResolver) fetchAndCacheTokenLegacy(token string, cached *structs.Aut } } +// TODO: remove func (r *ACLResolver) resolveTokenLegacy(token string) (structs.ACLIdentity, acl.Authorizer, error) { defer metrics.MeasureSince([]string{"acl", "resolveTokenLegacy"}, time.Now()) @@ -1244,13 +1246,6 @@ func (r *ACLResolver) ResolveTokenToIdentityAndAuthorizer(token string) (structs return ident, authz, nil } - if r.delegate.UseLegacyACLs() { - // TODO(partitions,acls): do we have to care about legacy acls? - identity, authorizer, err := r.resolveTokenLegacy(token) - r.handleACLDisabledError(err) - return identity, authorizer, err - } - defer metrics.MeasureSince([]string{"acl", "ResolveToken"}, time.Now()) identity, policies, err := r.resolveTokenToIdentityAndPolicies(token) @@ -1310,12 +1305,6 @@ func (r *ACLResolver) ResolveTokenToIdentity(token string) (structs.ACLIdentity, return ident, nil } - if r.delegate.UseLegacyACLs() { - identity, _, err := r.resolveTokenLegacy(token) - r.handleACLDisabledError(err) - return identity, err - } - defer metrics.MeasureSince([]string{"acl", "ResolveTokenToIdentity"}, time.Now()) return r.resolveIdentityFromToken(token) diff --git a/agent/consul/acl_client.go b/agent/consul/acl_client.go index 36e1f2e5b..8a33c6e46 100644 --- a/agent/consul/acl_client.go +++ b/agent/consul/acl_client.go @@ -1,9 +1,6 @@ package consul import ( - "sync/atomic" - "time" - "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/lib/serf" @@ -27,36 +24,6 @@ var clientACLCacheConfig *structs.ACLCachesConfig = &structs.ACLCachesConfig{ Roles: 128, } -func (c *Client) UseLegacyACLs() bool { - return atomic.LoadInt32(&c.useNewACLs) == 0 -} - -func (c *Client) monitorACLMode() { - waitTime := aclModeCheckMinInterval - for { - foundServers, mode, _ := ServersGetACLMode(c, "", c.config.Datacenter) - if foundServers && mode == structs.ACLModeEnabled { - c.logger.Debug("transitioned out of legacy ACL mode") - c.updateSerfTags("acls", string(structs.ACLModeEnabled)) - atomic.StoreInt32(&c.useNewACLs, 1) - return - } - - select { - case <-c.shutdownCh: - return - case <-time.After(waitTime): - // do nothing - } - - // calculate the amount of time to wait for the next round - waitTime = waitTime * 2 - if waitTime > aclModeCheckMaxInterval { - waitTime = aclModeCheckMaxInterval - } - } -} - func (c *Client) ACLDatacenter(legacy bool) string { // For resolution running on clients, when not in // legacy mode the servers within the current datacenter diff --git a/agent/consul/acl_server.go b/agent/consul/acl_server.go index 40ae430ef..025012c1f 100644 --- a/agent/consul/acl_server.go +++ b/agent/consul/acl_server.go @@ -95,6 +95,7 @@ func (s *Server) updateSerfTags(key, value string) { s.updateEnterpriseSerfTags(key, value) } +// TODO: func (s *Server) updateACLAdvertisement() { // One thing to note is that once in new ACL mode the server will // never transition to legacy ACL mode. This is not currently a diff --git a/agent/consul/client.go b/agent/consul/client.go index 57047973b..f3e337e12 100644 --- a/agent/consul/client.go +++ b/agent/consul/client.go @@ -58,10 +58,6 @@ type Client struct { // acls is used to resolve tokens to effective policies acls *ACLResolver - // DEPRECATED (ACL-Legacy-Compat) - Only needed while we support both - // useNewACLs is a flag to indicate whether we are using the new ACL system - useNewACLs int32 - // Connection pool to consul servers connPool *pool.ConnPool @@ -121,7 +117,6 @@ func NewClient(config *Config, deps Deps) (*Client, error) { return nil, err } - c.useNewACLs = 0 aclConfig := ACLResolverConfig{ Config: config.ACLResolverSettings, Delegate: c, @@ -154,12 +149,6 @@ func NewClient(config *Config, deps Deps) (*Client, error) { // handlers depend on the router and the router depends on Serf. go c.lanEventHandler() - // This needs to happen after initializing c.router to prevent a race - // condition where the router manager is used when the pointer is nil - if c.acls.ACLsEnabled() { - go c.monitorACLMode() - } - return c, nil } @@ -365,11 +354,7 @@ func (c *Client) Stats() map[string]map[string]string { } if c.config.ACLsEnabled { - if c.UseLegacyACLs() { - stats["consul"]["acl"] = "legacy" - } else { - stats["consul"]["acl"] = "enabled" - } + stats["consul"]["acl"] = "enabled" } else { stats["consul"]["acl"] = "disabled" } diff --git a/agent/consul/client_serf.go b/agent/consul/client_serf.go index 3dd1989b1..3b632f6fd 100644 --- a/agent/consul/client_serf.go +++ b/agent/consul/client_serf.go @@ -9,7 +9,6 @@ import ( "github.com/hashicorp/serf/serf" "github.com/hashicorp/consul/agent/metadata" - "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/lib" libserf "github.com/hashicorp/consul/lib/serf" "github.com/hashicorp/consul/logging" @@ -32,13 +31,6 @@ func (c *Client) setupSerf(conf *serf.Config, ch chan serf.Event, path string) ( if c.config.AdvertiseReconnectTimeout != 0 { conf.Tags[libserf.ReconnectTimeoutTag] = c.config.AdvertiseReconnectTimeout.String() } - if c.acls.ACLsEnabled() { - // we start in legacy mode and then transition to normal - // mode once we know the cluster can handle it. - conf.Tags["acls"] = string(structs.ACLModeLegacy) - } else { - conf.Tags["acls"] = string(structs.ACLModeDisabled) - } // We use the Intercept variant here to ensure that serf and memberlist logs // can be streamed via the monitor endpoint diff --git a/agent/consul/server_serf.go b/agent/consul/server_serf.go index e5d6d662f..f72c64c34 100644 --- a/agent/consul/server_serf.go +++ b/agent/consul/server_serf.go @@ -73,10 +73,9 @@ func (s *Server) setupSerf(conf *serf.Config, ch chan serf.Event, path string, w } if s.acls.ACLsEnabled() { - // we start in legacy mode and allow upgrading later - conf.Tags["acls"] = string(structs.ACLModeLegacy) + conf.Tags[metadata.TagACLs] = string(structs.ACLModeEnabled) } else { - conf.Tags["acls"] = string(structs.ACLModeDisabled) + conf.Tags[metadata.TagACLs] = string(structs.ACLModeDisabled) } // feature flag: advertise support for federation states diff --git a/agent/delegate_mock_test.go b/agent/delegate_mock_test.go index 208313aed..2273fbd84 100644 --- a/agent/delegate_mock_test.go +++ b/agent/delegate_mock_test.go @@ -3,12 +3,13 @@ package agent import ( "io" + "github.com/hashicorp/serf/serf" + "github.com/stretchr/testify/mock" + "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/consul" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/lib" - "github.com/hashicorp/serf/serf" - "github.com/stretchr/testify/mock" ) type delegateMock struct { @@ -65,10 +66,6 @@ func (m *delegateMock) RPC(method string, args interface{}, reply interface{}) e return m.Called(method, args, reply).Error(0) } -func (m *delegateMock) UseLegacyACLs() bool { - return m.Called().Bool(0) -} - func (m *delegateMock) SnapshotRPC(args *structs.SnapshotRequest, in io.Reader, out io.Writer, replyFn structs.SnapshotReplyFn) error { return m.Called(args, in, out, replyFn).Error(0) } diff --git a/agent/metadata/server.go b/agent/metadata/server.go index c85135d29..b77d1d6d0 100644 --- a/agent/metadata/server.go +++ b/agent/metadata/server.go @@ -98,7 +98,7 @@ func IsConsulServer(m serf.Member) (bool, *Server) { } var acls structs.ACLMode - if aclMode, ok := m.Tags["acls"]; ok { + if aclMode, ok := m.Tags[TagACLs]; ok { acls = structs.ACLMode(aclMode) } else { acls = structs.ACLModeUnknown @@ -194,6 +194,8 @@ func IsConsulServer(m serf.Member) (bool, *Server) { return true, parts } +const TagACLs = "acls" + const featureFlagPrefix = "ft_" // AddFeatureFlags to the tags. The tags map is expected to be a serf.Config.Tags.