Fix ACL Controller Race Conditions

CHANGELOG.md

@@ -20,6 +20,10 @@ FEATURES
 * Add a `envoy-entrypoint` subcommand, which can be used as the entrypoint to the Envoy container running in ECS
   to support graceful shutdown. [[GH-42](https://github.com/hashicorp/consul-ecs/pull/42)]
 
+BUG FIXES:
+* Fix edge cases in the ACL controller where ACL tokens never get cleaned
+  up. [[GH-45](https://github.com/hashicorp/consul-ecs/pull/45)]
+
 ## 0.2.0-beta2 (September 30, 2021)
 IMPROVEMENTS
 * Clean up ACL tokens for services/task families that are deleted. [[GH-30](https://github.com/hashicorp/consul-ecs/pull/30)]

controller/controller.go

@@ -20,21 +20,10 @@ type Controller struct {
 	PollingInterval time.Duration
 	// Log is the logger used by the Controller.
 	Log hclog.Logger
-
-	// resourceState is the internal Controller's state. The Controller
-	// will periodically update the state to reflect the state of the Resources.
-	// The state stores ResourceIDs that have been successfully upserted
-	// by the controller. If the resource with that ID no longer exists in the source,
-	// the resourceState should not have that resource ID in its internal state either.
-	resourceState map[ResourceID]Resource
 }
 
 // Run starts the Controller loop. The loop will exit when ctx is canceled.
 func (c *Controller) Run(ctx context.Context) {
-	if c.resourceState == nil {
-		c.resourceState = make(map[ResourceID]Resource)
-	}
-
 	for {
 		select {
 		case <-time.After(c.PollingInterval):
@@ -55,41 +44,13 @@ func (c *Controller) reconcile() error {
 	if err != nil {
 		return fmt.Errorf("listing resources: %w", err)
 	}
-	currentResourceIDs := make(map[ResourceID]struct{})
 	for _, resource := range resources {
-		resourceID, err := resource.ID()
+		err = resource.Reconcile()
 		if err != nil {
-			return fmt.Errorf("getting resource ID: %w", err)
-		}
-
-		currentResourceIDs[resourceID] = struct{}{}
-
-		if _, ok := c.resourceState[resourceID]; !ok {
-			err = resource.Upsert()
-			if err != nil {
-				c.Log.Error("error upserting resource", "err", err, "id", resourceID)
-				continue
-			}
-			c.resourceState[resourceID] = resource
+			c.Log.Error("error reconciling resource", "err", err)
 		}
 	}
 
-	// Prune any resources in the resource state that no longer exist.
-	for id, resource := range c.resourceState {
-		if _, ok := currentResourceIDs[id]; !ok {
-			// Delete the resource.
-			err = resource.Delete()
-			if err != nil {
-				// If there's an error, log it but don't delete from internal
-				// state so that we can retry on the next iteration of the 'reconcile' loop.
-				c.Log.Error("error deleting resource", "err", err, "id", id)
-				continue
-			}
-
-			// Delete it from internal state.
-			delete(c.resourceState, id)
-		}
-	}
 	c.Log.Info("reconcile finished successfully")
 	return nil
 }

controller/controller_test.go

@@ -2,7 +2,6 @@ package controller
 
 import (
 	"context"
-	"reflect"
 	"testing"
 	"time"
 
@@ -12,94 +11,55 @@ import (
 )
 
 func TestRun(t *testing.T) {
-	cases := map[string]struct {
-		source map[ResourceID]struct{}
-		sink   map[ResourceID]struct{}
-	}{
-		"upsert single": {
-			source: map[ResourceID]struct{}{"foo": {}},
-			sink:   make(map[ResourceID]struct{}),
-		},
-		"upsert multiple": {
-			source: map[ResourceID]struct{}{"foo": {}, "bar": {}},
-			sink:   make(map[ResourceID]struct{}),
-		},
-		"delete": {
-			source: map[ResourceID]struct{}{"foo": {}, "bar": {}},
-			sink:   map[ResourceID]struct{}{"baz": {}},
-		},
+	resource1 := &testResource{
+		name: "resource1",
 	}
 
-	for name, c := range cases {
-		t.Run(name, func(t *testing.T) {
-			resourceLister := &testResourceLister{
-				source: c.source,
-				sink:   c.sink,
-			}
+	resource2 := &testResource{
+		name: "resource2",
+	}
+
+	lister := &testResourceLister{
+		resources: []*testResource{resource1, resource2},
+	}
 
-			ctrl := Controller{
-				Resources:       resourceLister,
-				PollingInterval: 1 * time.Second,
-				Log:             hclog.NewNullLogger(),
-			}
-			// If sink already has resources, then we need to make sure
-			// that the controller's internal state has them too.
-			if c.sink != nil {
-				ctrl.resourceState = make(map[ResourceID]Resource)
-				for id := range c.sink {
-					ctrl.resourceState[id] = testResource{name: string(id), sink: &c.sink}
-				}
-			}
+	ctrl := Controller{
+		Resources:       lister,
+		PollingInterval: 1 * time.Second,
+		Log:             hclog.NewNullLogger(),
+	}
 
-			ctx, cancelFunc := context.WithCancel(context.Background())
-			t.Cleanup(cancelFunc)
+	ctx, cancelFunc := context.WithCancel(context.Background())
+	t.Cleanup(cancelFunc)
 
-			go ctrl.Run(ctx)
+	go ctrl.Run(ctx)
 
-			retry.Run(t, func(r *retry.R) {
-				require.True(r, reflect.DeepEqual(resourceLister.sink, c.source))
-				resourceStateIDs := make(map[ResourceID]struct{})
-				for id := range ctrl.resourceState {
-					resourceStateIDs[id] = struct{}{}
-				}
-				require.True(t, reflect.DeepEqual(resourceStateIDs, c.source))
-			})
-		})
-	}
+	retry.Run(t, func(r *retry.R) {
+		for _, resource := range lister.resources {
+			require.True(r, resource.reconciled)
+		}
+	})
 }
 
 type testResourceLister struct {
-	source map[ResourceID]struct{}
-	sink   map[ResourceID]struct{}
+	resources []*testResource
 }
 
 type testResource struct {
-	name string
-	sink *map[ResourceID]struct{}
+	name       string
+	reconciled bool
 }
 
 func (t testResourceLister) List() ([]Resource, error) {
 	var resources []Resource
-	for k := range t.source {
-		resources = append(resources, testResource{name: string(k), sink: &t.sink})
+	for _, resource := range t.resources {
+		resources = append(resources, resource)
 	}
 	return resources, nil
 }
 
-func (t testResource) ID() (ResourceID, error) {
-	return ResourceID(t.name), nil
-}
-
-func (t testResource) Upsert() error {
-	id, _ := t.ID()
-	(*t.sink)[id] = struct{}{}
-
-	return nil
-}
-
-func (t testResource) Delete() error {
-	id, _ := t.ID()
-	delete(*t.sink, id)
+func (t *testResource) Reconcile() error {
+	t.reconciled = true
 
 	return nil
 }