From d12dd48c61e39c7d11ece660eb616169973db2ae Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 22 Sep 2021 18:55:53 -0400 Subject: [PATCH 1/3] 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 41407f61d4..a97b9dbd98 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 5a665c6cb5..e5385abbce 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 b84b100bf4..1504627b3e 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 36e1f2e5bb..8a33c6e461 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 40ae430efb..025012c1f9 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 57047973be..f3e337e12e 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 3dd1989b15..3b632f6fd7 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 e5d6d662fe..f72c64c346 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 208313aed4..2273fbd84f 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 c85135d294..b77d1d6d06 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. From 8e9773e20b0ee92cc2a5d34f9b5b52546a4c7b58 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 22 Sep 2021 19:13:55 -0400 Subject: [PATCH 2/3] acl: remove ACL.GetPolicy endpoint and resolve legacy acls And all code that was no longer used once those two were removed. --- acl/policy.go | 48 ----- agent/consul/acl.go | 142 ------------ agent/consul/acl_endpoint.go | 50 ----- agent/consul/acl_endpoint_legacy.go | 6 + agent/consul/acl_endpoint_test.go | 24 --- agent/consul/acl_test.go | 324 ---------------------------- agent/structs/acl.go | 9 - agent/structs/acl_cache.go | 11 +- agent/structs/acl_legacy.go | 25 --- agent/structs/structs_test.go | 1 - 10 files changed, 8 insertions(+), 632 deletions(-) diff --git a/acl/policy.go b/acl/policy.go index d4f9c6088c..ed1c74f23c 100644 --- a/acl/policy.go +++ b/acl/policy.go @@ -453,54 +453,6 @@ func NewPolicyFromSource(id string, revision uint64, rules string, syntax Syntax return policy, err } -// TODO(ACL-Legacy): remove this -func (policy *Policy) ConvertToLegacy() *Policy { - converted := &Policy{ - ID: policy.ID, - Revision: policy.Revision, - PolicyRules: PolicyRules{ - ACL: policy.ACL, - Keyring: policy.Keyring, - Operator: policy.Operator, - }, - } - - converted.Agents = append(converted.Agents, policy.Agents...) - converted.Agents = append(converted.Agents, policy.AgentPrefixes...) - converted.Keys = append(converted.Keys, policy.Keys...) - converted.Keys = append(converted.Keys, policy.KeyPrefixes...) - converted.Nodes = append(converted.Nodes, policy.Nodes...) - converted.Nodes = append(converted.Nodes, policy.NodePrefixes...) - converted.Services = append(converted.Services, policy.Services...) - converted.Services = append(converted.Services, policy.ServicePrefixes...) - converted.Sessions = append(converted.Sessions, policy.Sessions...) - converted.Sessions = append(converted.Sessions, policy.SessionPrefixes...) - converted.Events = append(converted.Events, policy.Events...) - converted.Events = append(converted.Events, policy.EventPrefixes...) - converted.PreparedQueries = append(converted.PreparedQueries, policy.PreparedQueries...) - converted.PreparedQueries = append(converted.PreparedQueries, policy.PreparedQueryPrefixes...) - return converted -} - -// TODO(ACL-Legacy): remove this -func (policy *Policy) ConvertFromLegacy() *Policy { - return &Policy{ - ID: policy.ID, - Revision: policy.Revision, - PolicyRules: PolicyRules{ - AgentPrefixes: policy.Agents, - KeyPrefixes: policy.Keys, - NodePrefixes: policy.Nodes, - ServicePrefixes: policy.Services, - SessionPrefixes: policy.Sessions, - EventPrefixes: policy.Events, - PreparedQueryPrefixes: policy.PreparedQueries, - Keyring: policy.Keyring, - Operator: policy.Operator, - }, - } -} - // takesPrecedenceOver returns true when permission a // should take precedence over permission b func takesPrecedenceOver(a, b string) bool { diff --git a/agent/consul/acl.go b/agent/consul/acl.go index 1504627b3e..59ee3e71e4 100644 --- a/agent/consul/acl.go +++ b/agent/consul/acl.go @@ -383,135 +383,6 @@ func (r *ACLResolver) Close() { r.aclConf.Close() } -func (r *ACLResolver) fetchAndCacheTokenLegacy(token string, cached *structs.AuthorizerCacheEntry) (acl.Authorizer, error) { - req := structs.ACLPolicyResolveLegacyRequest{ - Datacenter: r.delegate.ACLDatacenter(true), - ACL: token, - } - - cacheTTL := r.config.ACLTokenTTL - if cached != nil { - cacheTTL = cached.TTL - } - - var reply structs.ACLPolicyResolveLegacyResponse - err := r.delegate.RPC("ACL.GetPolicy", &req, &reply) - if err == nil { - parent := acl.RootAuthorizer(reply.Parent) - if parent == nil { - var authorizer acl.Authorizer - if cached != nil { - authorizer = cached.Authorizer - } - r.cache.PutAuthorizerWithTTL(token, authorizer, cacheTTL) - return authorizer, acl.ErrInvalidParent - } - - var policies []*acl.Policy - policy := reply.Policy - if policy != nil { - policies = append(policies, policy.ConvertFromLegacy()) - } - - authorizer, err := acl.NewPolicyAuthorizerWithDefaults(parent, policies, r.aclConf) - - r.cache.PutAuthorizerWithTTL(token, authorizer, reply.TTL) - return authorizer, err - } - - if acl.IsErrNotFound(err) { - // Make sure to remove from the cache if it was deleted - r.cache.PutAuthorizerWithTTL(token, nil, cacheTTL) - return nil, acl.ErrNotFound - - } - - // some other RPC error - switch r.config.ACLDownPolicy { - case "allow": - r.cache.PutAuthorizerWithTTL(token, acl.AllowAll(), cacheTTL) - return acl.AllowAll(), nil - case "async-cache", "extend-cache": - if cached != nil { - r.cache.PutAuthorizerWithTTL(token, cached.Authorizer, cacheTTL) - return cached.Authorizer, nil - } - fallthrough - default: - r.cache.PutAuthorizerWithTTL(token, acl.DenyAll(), cacheTTL) - return acl.DenyAll(), nil - } -} - -// TODO: remove -func (r *ACLResolver) resolveTokenLegacy(token string) (structs.ACLIdentity, acl.Authorizer, error) { - defer metrics.MeasureSince([]string{"acl", "resolveTokenLegacy"}, time.Now()) - - // Attempt to resolve locally first (local results are not cached) - // This is only useful for servers where either legacy replication is being - // done or the server is within the primary datacenter. - if done, identity, err := r.delegate.ResolveIdentityFromToken(token); done { - if err == nil && identity != nil { - policies, err := r.resolvePoliciesForIdentity(identity) - if err != nil { - return identity, nil, err - } - - authz, err := policies.Compile(r.cache, r.aclConf) - if err != nil { - return identity, nil, err - } - - return identity, acl.NewChainedAuthorizer([]acl.Authorizer{authz, acl.RootAuthorizer(r.config.ACLDefaultPolicy)}), nil - } - - return nil, nil, err - } - - identity := &missingIdentity{ - reason: "legacy-token", - token: token, - } - - // Look in the cache prior to making a RPC request - entry := r.cache.GetAuthorizer(token) - - if entry != nil && entry.Age() <= minTTL(entry.TTL, r.config.ACLTokenTTL) { - metrics.IncrCounter([]string{"acl", "token", "cache_hit"}, 1) - if entry.Authorizer != nil { - return identity, entry.Authorizer, nil - } - return identity, nil, acl.ErrNotFound - } - - metrics.IncrCounter([]string{"acl", "token", "cache_miss"}, 1) - - // Resolve the token in the background and wait on the result if we must - waitChan := r.legacyGroup.DoChan(token, func() (interface{}, error) { - authorizer, err := r.fetchAndCacheTokenLegacy(token, entry) - return authorizer, err - }) - - waitForResult := entry == nil || r.config.ACLDownPolicy != "async-cache" - if !waitForResult { - // waitForResult being false requires the cacheEntry to not be nil - if entry.Authorizer != nil { - return identity, entry.Authorizer, nil - } - return identity, nil, acl.ErrNotFound - } - - // block waiting for the async RPC to finish. - res := <-waitChan - - var authorizer acl.Authorizer - if res.Val != nil { // avoid a nil-not-nil bug - authorizer = res.Val.(acl.Authorizer) - } - - return identity, authorizer, res.Err -} - func (r *ACLResolver) fetchAndCacheIdentityFromToken(token string, cached *structs.IdentityCacheEntry) (structs.ACLIdentity, error) { cacheID := tokenSecretCacheID(token) @@ -1326,19 +1197,6 @@ func (r *ACLResolver) ACLsEnabled() bool { return true } -func (r *ACLResolver) GetMergedPolicyForToken(token string) (structs.ACLIdentity, *acl.Policy, error) { - ident, policies, err := r.resolveTokenToIdentityAndPolicies(token) - if err != nil { - return nil, nil, err - } - if len(policies) == 0 { - return nil, nil, acl.ErrNotFound - } - - policy, err := policies.Merge(r.cache, r.aclConf) - return ident, policy, err -} - // aclFilter is used to filter results from our state store based on ACL rules // configured for the provided token. type aclFilter struct { diff --git a/agent/consul/acl_endpoint.go b/agent/consul/acl_endpoint.go index 37d9494cef..4311c4434d 100644 --- a/agent/consul/acl_endpoint.go +++ b/agent/consul/acl_endpoint.go @@ -1372,56 +1372,6 @@ func (a *ACL) PolicyResolve(args *structs.ACLPolicyBatchGetRequest, reply *struc return nil } -// makeACLETag returns an ETag for the given parent and policy. -func makeACLETag(parent string, policy *acl.Policy) string { - return fmt.Sprintf("%s:%s", parent, policy.ID) -} - -// GetPolicy is used to retrieve a compiled policy object with a TTL. Does not -// support a blocking query. -// -// TODO(ACL-Legacy): remove this -func (a *ACL) GetPolicy(args *structs.ACLPolicyResolveLegacyRequest, reply *structs.ACLPolicyResolveLegacyResponse) error { - if done, err := a.srv.ForwardRPC("ACL.GetPolicy", args, reply); done { - return err - } - - // Verify we are allowed to serve this request - if a.srv.config.PrimaryDatacenter != a.srv.config.Datacenter { - return acl.ErrDisabled - } - - // Get the policy via the cache - parent := a.srv.config.ACLResolverSettings.ACLDefaultPolicy - - ident, policy, err := a.srv.acls.GetMergedPolicyForToken(args.ACL) - if err != nil { - return err - } - - if token, ok := ident.(*structs.ACLToken); ok && token.Type == structs.ACLTokenTypeManagement { - parent = "manage" - } - - // translates the structures internals to most closely match what could be expressed in the original rule language - policy = policy.ConvertToLegacy() - - // Generate an ETag - etag := makeACLETag(parent, policy) - - // Setup the response - reply.ETag = etag - reply.TTL = a.srv.config.ACLResolverSettings.ACLTokenTTL - a.srv.setQueryMeta(&reply.QueryMeta) - - // Only send the policy on an Etag mis-match - if args.ETag != etag { - reply.Parent = parent - reply.Policy = policy - } - return nil -} - // ReplicationStatus is used to retrieve the current ACL replication status. func (a *ACL) ReplicationStatus(args *structs.DCSpecificRequest, reply *structs.ACLReplicationStatus) error { diff --git a/agent/consul/acl_endpoint_legacy.go b/agent/consul/acl_endpoint_legacy.go index 069fb1db2a..a132786fd0 100644 --- a/agent/consul/acl_endpoint_legacy.go +++ b/agent/consul/acl_endpoint_legacy.go @@ -6,6 +6,12 @@ import ( "github.com/hashicorp/consul/agent/structs" ) +type LegacyACLGetPolicy struct{} + +func (a *ACL) GetPolicy(*LegacyACLGetPolicy, *LegacyACLGetPolicy) error { + return fmt.Errorf("ACL.GetPolicy: the legacy ACL system has been removed") +} + func (a *ACL) Bootstrap(*structs.DCSpecificRequest, *structs.ACL) error { return fmt.Errorf("ACL.Bootstrap: the legacy ACL system has been removed") } diff --git a/agent/consul/acl_endpoint_test.go b/agent/consul/acl_endpoint_test.go index 10574dc0f7..e216ee01c3 100644 --- a/agent/consul/acl_endpoint_test.go +++ b/agent/consul/acl_endpoint_test.go @@ -74,30 +74,6 @@ func TestACLEndpoint_BootstrapTokens(t *testing.T) { require.Equal(t, out.CreateIndex, out.ModifyIndex) } -func TestACLEndpoint_GetPolicy_Management(t *testing.T) { - if testing.Short() { - t.Skip("too slow for testing.Short") - } - - t.Parallel() - _, srv, codec := testACLServerWithConfig(t, nil, false) - - // wait for leader election and leader establishment to finish. - // after this the global management policy, master token and - // anonymous token will have been injected into the state store - // and we will be ready to resolve the master token - waitForLeaderEstablishment(t, srv) - - req := structs.ACLPolicyResolveLegacyRequest{ - Datacenter: srv.config.Datacenter, - ACL: TestDefaultMasterToken, - } - - var resp structs.ACLPolicyResolveLegacyResponse - require.NoError(t, msgpackrpc.CallWithCodec(codec, "ACL.GetPolicy", &req, &resp)) - require.Equal(t, "manage", resp.Parent) -} - func TestACLEndpoint_ReplicationStatus(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") diff --git a/agent/consul/acl_test.go b/agent/consul/acl_test.go index ab3a2beaae..c1f4816b47 100644 --- a/agent/consul/acl_test.go +++ b/agent/consul/acl_test.go @@ -434,7 +434,6 @@ type ACLResolverTestDelegate struct { localTokens bool localPolicies bool localRoles bool - getPolicyFn func(*structs.ACLPolicyResolveLegacyRequest, *structs.ACLPolicyResolveLegacyResponse) error tokenReadFn func(*structs.ACLTokenGetRequest, *structs.ACLTokenResponse) error policyResolveFn func(*structs.ACLPolicyBatchGetRequest, *structs.ACLPolicyBatchResponse) error roleResolveFn func(*structs.ACLRoleBatchGetRequest, *structs.ACLRoleBatchResponse) error @@ -675,12 +674,6 @@ func (d *ACLResolverTestDelegate) ResolveRoleFromID(roleID string) (bool, *struc func (d *ACLResolverTestDelegate) RPC(method string, args interface{}, reply interface{}) error { switch method { - case "ACL.GetPolicy": - atomic.AddInt32(&d.remoteLegacyResolutions, 1) - if d.getPolicyFn != nil { - return d.getPolicyFn(args.(*structs.ACLPolicyResolveLegacyRequest), reply.(*structs.ACLPolicyResolveLegacyResponse)) - } - panic("Bad Test Implementation: should provide a getPolicyFn to the ACLResolverTestDelegate") case "ACL.TokenRead": atomic.AddInt32(&d.remoteTokenResolutions, 1) if d.tokenReadFn != nil { @@ -1571,49 +1564,6 @@ func TestACLResolver_Client(t *testing.T) { require.EqualValues(t, 0, delegate.remoteLegacyResolutions) }) - t.Run("Resolve-Identity-Legacy", func(t *testing.T) { - t.Parallel() - - delegate := &ACLResolverTestDelegate{ - enabled: true, - datacenter: "dc1", - legacy: true, - localTokens: false, - localPolicies: false, - getPolicyFn: func(_ *structs.ACLPolicyResolveLegacyRequest, reply *structs.ACLPolicyResolveLegacyResponse) error { - reply.Parent = "deny" - reply.TTL = 30 - reply.ETag = "nothing" - reply.Policy = &acl.Policy{ - ID: "not-needed", - PolicyRules: acl.PolicyRules{ - Nodes: []*acl.NodeRule{ - { - Name: "foo", - Policy: acl.PolicyWrite, - }, - }, - }, - } - return nil - }, - } - - r := newTestACLResolver(t, delegate, nil) - - ident, err := r.ResolveTokenToIdentity("found-policy-and-role") - require.NoError(t, err) - require.NotNil(t, ident) - require.Equal(t, "legacy-token", ident.ID()) - require.EqualValues(t, 0, delegate.localTokenResolutions) - require.EqualValues(t, 0, delegate.remoteTokenResolutions) - require.EqualValues(t, 0, delegate.localPolicyResolutions) - require.EqualValues(t, 0, delegate.remotePolicyResolutions) - require.EqualValues(t, 0, delegate.localRoleResolutions) - require.EqualValues(t, 0, delegate.remoteRoleResolutions) - require.EqualValues(t, 1, delegate.remoteLegacyResolutions) - }) - t.Run("Concurrent-Token-Resolve", func(t *testing.T) { t.Parallel() @@ -2231,280 +2181,6 @@ func testACLResolver_variousTokens(t *testing.T, delegate *ACLResolverTestDelega }) } -func TestACLResolver_Legacy(t *testing.T) { - t.Parallel() - - t.Run("Cached", func(t *testing.T) { - t.Parallel() - cached := false - delegate := &ACLResolverTestDelegate{ - enabled: true, - datacenter: "dc1", - legacy: true, - localTokens: false, - localPolicies: false, - getPolicyFn: func(_ *structs.ACLPolicyResolveLegacyRequest, reply *structs.ACLPolicyResolveLegacyResponse) error { - if !cached { - reply.Parent = "deny" - reply.TTL = 30 - reply.ETag = "nothing" - reply.Policy = &acl.Policy{ - ID: "not-needed", - PolicyRules: acl.PolicyRules{ - Nodes: []*acl.NodeRule{ - { - Name: "foo", - Policy: acl.PolicyWrite, - }, - }, - }, - } - cached = true - return nil - } - return errRPC - }, - } - r := newTestACLResolver(t, delegate, nil) - - authz, err := r.ResolveToken("foo") - require.NoError(t, err) - require.NotNil(t, authz) - // there is a bit of translation that happens - require.Equal(t, acl.Allow, authz.NodeWrite("foo", nil)) - require.Equal(t, acl.Allow, authz.NodeWrite("foo/bar", nil)) - require.Equal(t, acl.Deny, authz.NodeWrite("fo", nil)) - - // this should be from the cache - authz, err = r.ResolveToken("foo") - require.NoError(t, err) - require.NotNil(t, authz) - // there is a bit of translation that happens - require.Equal(t, acl.Allow, authz.NodeWrite("foo", nil)) - require.Equal(t, acl.Allow, authz.NodeWrite("foo/bar", nil)) - require.Equal(t, acl.Deny, authz.NodeWrite("fo", nil)) - }) - - t.Run("Cache-Expiry-Extend", func(t *testing.T) { - t.Parallel() - cached := false - delegate := &ACLResolverTestDelegate{ - enabled: true, - datacenter: "dc1", - legacy: true, - localTokens: false, - localPolicies: false, - getPolicyFn: func(_ *structs.ACLPolicyResolveLegacyRequest, reply *structs.ACLPolicyResolveLegacyResponse) error { - if !cached { - reply.Parent = "deny" - reply.TTL = 0 - reply.ETag = "nothing" - reply.Policy = &acl.Policy{ - ID: "not-needed", - PolicyRules: acl.PolicyRules{ - Nodes: []*acl.NodeRule{ - { - Name: "foo", - Policy: acl.PolicyWrite, - }, - }, - }, - } - cached = true - return nil - } - return errRPC - }, - } - r := newTestACLResolver(t, delegate, func(config *ACLResolverConfig) { - config.Config.ACLTokenTTL = 0 - }) - - authz, err := r.ResolveToken("foo") - require.NoError(t, err) - require.NotNil(t, authz) - // there is a bit of translation that happens - require.Equal(t, acl.Allow, authz.NodeWrite("foo", nil)) - require.Equal(t, acl.Allow, authz.NodeWrite("foo/bar", nil)) - require.Equal(t, acl.Deny, authz.NodeWrite("fo", nil)) - - // this should be from the cache - authz, err = r.ResolveToken("foo") - require.NoError(t, err) - require.NotNil(t, authz) - // there is a bit of translation that happens - require.Equal(t, acl.Allow, authz.NodeWrite("foo", nil)) - require.Equal(t, acl.Allow, authz.NodeWrite("foo/bar", nil)) - require.Equal(t, acl.Deny, authz.NodeWrite("fo", nil)) - }) - - t.Run("Cache-Expiry-Allow", func(t *testing.T) { - t.Parallel() - cached := false - delegate := &ACLResolverTestDelegate{ - enabled: true, - datacenter: "dc1", - legacy: true, - localTokens: false, - localPolicies: false, - getPolicyFn: func(_ *structs.ACLPolicyResolveLegacyRequest, reply *structs.ACLPolicyResolveLegacyResponse) error { - if !cached { - reply.Parent = "deny" - reply.TTL = 0 - reply.ETag = "nothing" - reply.Policy = &acl.Policy{ - ID: "not-needed", - PolicyRules: acl.PolicyRules{ - Nodes: []*acl.NodeRule{ - { - Name: "foo", - Policy: acl.PolicyWrite, - }, - }, - }, - } - cached = true - return nil - } - return errRPC - }, - } - r := newTestACLResolver(t, delegate, func(config *ACLResolverConfig) { - config.Config.ACLDownPolicy = "allow" - config.Config.ACLTokenTTL = 0 - }) - - authz, err := r.ResolveToken("foo") - require.NoError(t, err) - require.NotNil(t, authz) - // there is a bit of translation that happens - require.Equal(t, acl.Allow, authz.NodeWrite("foo", nil)) - require.Equal(t, acl.Allow, authz.NodeWrite("foo/bar", nil)) - require.Equal(t, acl.Deny, authz.NodeWrite("fo", nil)) - - // this should be from the cache - authz, err = r.ResolveToken("foo") - require.NoError(t, err) - require.NotNil(t, authz) - // there is a bit of translation that happens - require.Equal(t, acl.Allow, authz.NodeWrite("foo", nil)) - require.Equal(t, acl.Allow, authz.NodeWrite("foo/bar", nil)) - require.Equal(t, acl.Allow, authz.NodeWrite("fo", nil)) - }) - - t.Run("Cache-Expiry-Deny", func(t *testing.T) { - t.Parallel() - cached := false - delegate := &ACLResolverTestDelegate{ - enabled: true, - datacenter: "dc1", - legacy: true, - localTokens: false, - localPolicies: false, - getPolicyFn: func(_ *structs.ACLPolicyResolveLegacyRequest, reply *structs.ACLPolicyResolveLegacyResponse) error { - if !cached { - reply.Parent = "deny" - reply.TTL = 0 - reply.ETag = "nothing" - reply.Policy = &acl.Policy{ - ID: "not-needed", - PolicyRules: acl.PolicyRules{ - Nodes: []*acl.NodeRule{ - { - Name: "foo", - Policy: acl.PolicyWrite, - }, - }, - }, - } - cached = true - return nil - } - return errRPC - }, - } - r := newTestACLResolver(t, delegate, func(config *ACLResolverConfig) { - config.Config.ACLDownPolicy = "deny" - config.Config.ACLTokenTTL = 0 - }) - - authz, err := r.ResolveToken("foo") - require.NoError(t, err) - require.NotNil(t, authz) - // there is a bit of translation that happens - require.Equal(t, acl.Allow, authz.NodeWrite("foo", nil)) - require.Equal(t, acl.Allow, authz.NodeWrite("foo/bar", nil)) - require.Equal(t, acl.Deny, authz.NodeWrite("fo", nil)) - - // this should be from the cache - authz, err = r.ResolveToken("foo") - require.NoError(t, err) - require.NotNil(t, authz) - // there is a bit of translation that happens - require.Equal(t, acl.Deny, authz.NodeWrite("foo", nil)) - require.Equal(t, acl.Deny, authz.NodeWrite("foo/bar", nil)) - require.Equal(t, acl.Deny, authz.NodeWrite("fo", nil)) - }) - - t.Run("Cache-Expiry-Async-Cache", func(t *testing.T) { - t.Parallel() - cached := false - delegate := &ACLResolverTestDelegate{ - enabled: true, - datacenter: "dc1", - legacy: true, - localTokens: false, - localPolicies: false, - getPolicyFn: func(_ *structs.ACLPolicyResolveLegacyRequest, reply *structs.ACLPolicyResolveLegacyResponse) error { - if !cached { - reply.Parent = "deny" - reply.TTL = 0 - reply.ETag = "nothing" - reply.Policy = &acl.Policy{ - ID: "not-needed", - PolicyRules: acl.PolicyRules{ - Nodes: []*acl.NodeRule{ - { - Name: "foo", - Policy: acl.PolicyWrite, - }, - }, - }, - } - cached = true - return nil - } - return acl.ErrNotFound - }, - } - r := newTestACLResolver(t, delegate, func(config *ACLResolverConfig) { - config.Config.ACLDownPolicy = "async-cache" - config.Config.ACLTokenTTL = 0 - }) - - authz, err := r.ResolveToken("foo") - require.NoError(t, err) - require.NotNil(t, authz) - // there is a bit of translation that happens - require.Equal(t, acl.Allow, authz.NodeWrite("foo", nil)) - require.Equal(t, acl.Allow, authz.NodeWrite("foo/bar", nil)) - require.Equal(t, acl.Deny, authz.NodeWrite("fo", nil)) - - // delivered from the cache - authz2, err := r.ResolveToken("foo") - require.NoError(t, err) - require.NotNil(t, authz) - require.True(t, authz == authz2) - - // the go routine spawned will eventually return and this will be a not found error - retry.Run(t, func(t *retry.R) { - authz3, err := r.ResolveToken("foo") - assert.Error(t, err) - assert.True(t, acl.IsErrNotFound(err)) - assert.Nil(t, authz3) - }) - }) -} func TestACL_filterHealthChecks(t *testing.T) { t.Parallel() // Create some health checks. diff --git a/agent/structs/acl.go b/agent/structs/acl.go index 4394356bdb..576199262c 100644 --- a/agent/structs/acl.go +++ b/agent/structs/acl.go @@ -811,15 +811,6 @@ func (policies ACLPolicies) Compile(cache *ACLCaches, entConf *acl.Config) (acl. return authorizer, nil } -func (policies ACLPolicies) Merge(cache *ACLCaches, entConf *acl.Config) (*acl.Policy, error) { - parsed, err := policies.resolveWithCache(cache, entConf) - if err != nil { - return nil, err - } - - return acl.MergePolicies(parsed), nil -} - type ACLRoles []*ACLRole // HashKey returns a consistent hash for a set of roles. diff --git a/agent/structs/acl_cache.go b/agent/structs/acl_cache.go index 1494727070..dd996242cd 100644 --- a/agent/structs/acl_cache.go +++ b/agent/structs/acl_cache.go @@ -3,8 +3,9 @@ package structs import ( "time" - "github.com/hashicorp/consul/acl" lru "github.com/hashicorp/golang-lru" + + "github.com/hashicorp/consul/acl" ) type ACLCachesConfig struct { @@ -218,14 +219,6 @@ func (c *ACLCaches) PutAuthorizer(id string, authorizer acl.Authorizer) { c.authorizers.Add(id, &AuthorizerCacheEntry{Authorizer: authorizer, CacheTime: time.Now()}) } -func (c *ACLCaches) PutAuthorizerWithTTL(id string, authorizer acl.Authorizer, ttl time.Duration) { - if c == nil || c.authorizers == nil { - return - } - - c.authorizers.Add(id, &AuthorizerCacheEntry{Authorizer: authorizer, CacheTime: time.Now(), TTL: ttl}) -} - func (c *ACLCaches) PutRole(roleID string, role *ACLRole) { if c == nil || c.roles == nil { return diff --git a/agent/structs/acl_legacy.go b/agent/structs/acl_legacy.go index 330213321f..c64dd425e4 100644 --- a/agent/structs/acl_legacy.go +++ b/agent/structs/acl_legacy.go @@ -8,9 +8,6 @@ package structs import ( "fmt" - "time" - - "github.com/hashicorp/consul/acl" ) const ( @@ -114,25 +111,3 @@ type ACLBootstrap struct { RaftIndex } - -// ACLPolicyResolveLegacyRequest is used to request an ACL by Token SecretID, conditionally -// filtering on an ID -type ACLPolicyResolveLegacyRequest struct { - Datacenter string // The Datacenter the RPC may be sent to - ACL string // The Tokens Secret ID - ETag string // Caching ETag to prevent resending the policy when not needed - QueryOptions -} - -// RequestDatacenter returns the DC this request is targeted to. -func (r *ACLPolicyResolveLegacyRequest) RequestDatacenter() string { - return r.Datacenter -} - -type ACLPolicyResolveLegacyResponse struct { - ETag string - Parent string - Policy *acl.Policy - TTL time.Duration - QueryMeta -} diff --git a/agent/structs/structs_test.go b/agent/structs/structs_test.go index b3835a67ae..29d7692efa 100644 --- a/agent/structs/structs_test.go +++ b/agent/structs/structs_test.go @@ -64,7 +64,6 @@ func TestStructs_Implements(t *testing.T) { _ RPCInfo = &SessionRequest{} _ RPCInfo = &SessionSpecificRequest{} _ RPCInfo = &EventFireRequest{} - _ RPCInfo = &ACLPolicyResolveLegacyRequest{} _ RPCInfo = &ACLPolicyBatchGetRequest{} _ RPCInfo = &ACLPolicyGetRequest{} _ RPCInfo = &ACLTokenGetRequest{} From a1e3fa818c42e443f2f8362dc1d260b249f3127a Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 29 Sep 2021 14:36:55 -0400 Subject: [PATCH 3/3] acl: fix test failures caused by remocving legacy ACLs This commit two test failures: 1. Remove check for "in legacy ACL mode", the actual upgrade will be removed in a following commit. 2. Remove the early WaitForLeader in dc2, because with it the test was failing with ACL not found. --- agent/consul/acl_endpoint.go | 5 ----- agent/consul/leader_federation_state_ae_test.go | 1 - 2 files changed, 6 deletions(-) diff --git a/agent/consul/acl_endpoint.go b/agent/consul/acl_endpoint.go index 4311c4434d..c9bbeaabc5 100644 --- a/agent/consul/acl_endpoint.go +++ b/agent/consul/acl_endpoint.go @@ -170,11 +170,6 @@ func (a *ACL) aclPreCheck() error { if !a.srv.config.ACLsEnabled { return acl.ErrDisabled } - - if a.srv.UseLegacyACLs() { - return fmt.Errorf("The ACL system is currently in legacy mode.") - } - return nil } diff --git a/agent/consul/leader_federation_state_ae_test.go b/agent/consul/leader_federation_state_ae_test.go index 8971334969..514ead1c12 100644 --- a/agent/consul/leader_federation_state_ae_test.go +++ b/agent/consul/leader_federation_state_ae_test.go @@ -376,7 +376,6 @@ func TestLeader_FederationStateAntiEntropyPruning_ACLDeny(t *testing.T) { c.ACLMasterToken = "root" c.ACLResolverSettings.ACLDefaultPolicy = "deny" }) - testrpc.WaitForLeader(t, s2.RPC, "dc2") defer os.RemoveAll(dir2) defer s2.Shutdown()