From 02da08ce7747aa2386bac4714f4781d4a2c43a12 Mon Sep 17 00:00:00 2001
From: Daniel Nephin <dnephin@hashicorp.com>
Date: Wed, 29 Sep 2021 16:14:36 -0400
Subject: [PATCH] acl: only run startACLUpgrade once

Since legacy ACL tokens can no longer be created we only need to run this upgrade a single
time when leadership is estalbished.
---
 agent/consul/acl.go              |  8 --------
 agent/consul/leader.go           | 29 +++++++++++++++++------------
 agent/consul/state/acl.go        |  1 +
 agent/consul/state/acl_schema.go |  1 +
 4 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/agent/consul/acl.go b/agent/consul/acl.go
index 273c558afb..a5c4010f12 100644
--- a/agent/consul/acl.go
+++ b/agent/consul/acl.go
@@ -54,14 +54,6 @@ const (
 	// are not allowed to be displayed.
 	redactedToken = "<hidden>"
 
-	// aclUpgradeBatchSize controls how many tokens we look at during each round of upgrading. Individual raft logs
-	// will be further capped using the aclBatchUpsertSize. This limit just prevents us from creating a single slice
-	// with all tokens in it.
-	aclUpgradeBatchSize = 128
-
-	// aclUpgradeRateLimit is the number of batch upgrade requests per second allowed.
-	aclUpgradeRateLimit rate.Limit = 1.0
-
 	// aclTokenReapingRateLimit is the number of batch token reaping requests per second allowed.
 	aclTokenReapingRateLimit rate.Limit = 1.0
 
diff --git a/agent/consul/leader.go b/agent/consul/leader.go
index a1b0121635..f5b0cec6af 100644
--- a/agent/consul/leader.go
+++ b/agent/consul/leader.go
@@ -13,7 +13,6 @@ import (
 	"github.com/armon/go-metrics"
 	"github.com/armon/go-metrics/prometheus"
 	"github.com/hashicorp/go-hclog"
-	"github.com/hashicorp/go-memdb"
 	"github.com/hashicorp/go-uuid"
 	"github.com/hashicorp/go-version"
 	"github.com/hashicorp/raft"
@@ -541,9 +540,19 @@ func (s *Server) initializeACLs(ctx context.Context) error {
 	return nil
 }
 
-// This function is only intended to be run as a managed go routine, it will block until
-// the context passed in indicates that it should exit.
+// legacyACLTokenUpgrade runs a single time to upgrade any tokens that may
+// have been created immediately before the Consul upgrade, or any legacy tokens
+// from a restored snapshot.
+// TODO(ACL-Legacy-Compat): remove in phase 2
 func (s *Server) legacyACLTokenUpgrade(ctx context.Context) error {
+	// aclUpgradeRateLimit is the number of batch upgrade requests per second allowed.
+	const aclUpgradeRateLimit rate.Limit = 1.0
+
+	// aclUpgradeBatchSize controls how many tokens we look at during each round of upgrading. Individual raft logs
+	// will be further capped using the aclBatchUpsertSize. This limit just prevents us from creating a single slice
+	// with all tokens in it.
+	const aclUpgradeBatchSize = 128
+
 	limiter := rate.NewLimiter(aclUpgradeRateLimit, int(aclUpgradeRateLimit))
 	for {
 		if err := limiter.Wait(ctx); err != nil {
@@ -552,21 +561,15 @@ func (s *Server) legacyACLTokenUpgrade(ctx context.Context) error {
 
 		// actually run the upgrade here
 		state := s.fsm.State()
-		tokens, waitCh, err := state.ACLTokenListUpgradeable(aclUpgradeBatchSize)
+		tokens, _, err := state.ACLTokenListUpgradeable(aclUpgradeBatchSize)
 		if err != nil {
 			s.logger.Warn("encountered an error while searching for tokens without accessor ids", "error", err)
 		}
 		// No need to check expiration time here, as that only exists for v2 tokens.
 
 		if len(tokens) == 0 {
-			ws := memdb.NewWatchSet()
-			ws.Add(state.AbandonCh())
-			ws.Add(waitCh)
-			ws.Add(ctx.Done())
-
-			// wait for more tokens to need upgrading or the aclUpgradeCh to be closed
-			ws.Watch(nil)
-			continue
+			// No new legacy tokens can be created, so we can exit
+			return nil
 		}
 
 		var newTokens structs.ACLTokens
@@ -615,6 +618,8 @@ func (s *Server) legacyACLTokenUpgrade(ctx context.Context) error {
 	}
 }
 
+// TODO(ACL-Legacy-Compat): remove in phase 2. Keeping it for now so that we
+// can upgrade any tokens created immediately before the upgrade happens.
 func (s *Server) startACLUpgrade(ctx context.Context) {
 	if s.config.PrimaryDatacenter != s.config.Datacenter {
 		// token upgrades should only run in the primary
diff --git a/agent/consul/state/acl.go b/agent/consul/state/acl.go
index fb19377a30..5497dfe92a 100644
--- a/agent/consul/state/acl.go
+++ b/agent/consul/state/acl.go
@@ -728,6 +728,7 @@ func (s *Store) ACLTokenList(ws memdb.WatchSet, local, global bool, policy, role
 	return idx, result, nil
 }
 
+// TODO(ACL-Legacy-Compat): remove in phase 2
 func (s *Store) ACLTokenListUpgradeable(max int) (structs.ACLTokens, <-chan struct{}, error) {
 	tx := s.db.Txn(false)
 	defer tx.Abort()
diff --git a/agent/consul/state/acl_schema.go b/agent/consul/state/acl_schema.go
index 20210182fd..1e8f415f46 100644
--- a/agent/consul/state/acl_schema.go
+++ b/agent/consul/state/acl_schema.go
@@ -107,6 +107,7 @@ func tokensTableSchema() *memdb.TableSchema {
 
 			//DEPRECATED (ACL-Legacy-Compat) - This index is only needed while we support upgrading v1 to v2 acls
 			// This table indexes all the ACL tokens that do not have an AccessorID
+			// TODO(ACL-Legacy-Compat): remove in phase 2
 			"needs-upgrade": {
 				Name:         "needs-upgrade",
 				AllowMissing: false,