Delete ACL token only for services that are deregistered.

CHANGELOG.md

@@ -27,7 +27,7 @@ BUG FIXES:
   * Connect: Use `AdmissionregistrationV1` instead of `AdmissionregistrationV1beta1` API as it was deprecated in k8s 1.16. [[GH-558](https://github.com/hashicorp/consul-k8s/pull/558)]
   * Connect: Fix bug where environment variables `<NAME>_CONNECT_SERVICE_HOST` and
   `<NAME>_CONNECT_SERVICE_PORT` weren't being set when the upstream annotation was used. [[GH-549](https://github.com/hashicorp/consul-k8s/issues/549)]
-  * Connect: Fix a bug with leaving around ACL tokens after a service has been deregistered. [[GH-571](https://github.com/hashicorp/consul-k8s/issues/540)]
+  * Connect: Fix a bug with leaving around ACL tokens after a service has been deregistered. Note that this will not clean up existing leftover ACL tokens. [[GH-540](https://github.com/hashicorp/consul-k8s/issues/540)][[GH-599](https://github.com/hashicorp/consul-k8s/issues/599)]
   * CRDs: Fix ProxyDefaults and ServiceDefaults resources not syncing with Consul < 1.10.0 [[GH-1023](https://github.com/hashicorp/consul-helm/issues/1023)]
   * Connect: Skip service registration for duplicate services only on Kubernetes. [[GH-581](https://github.com/hashicorp/consul-k8s/pull/581)]
   * Connect: redirect-traffic command passes ACL token when ACLs are enabled. [[GH-576](https://github.com/hashicorp/consul-k8s/pull/576)]

control-plane/connect-inject/endpoints_controller.go

@@ -714,6 +714,7 @@ func (r *EndpointsController) deregisterServiceOnAllAgents(ctx context.Context,
 		for svcID, serviceRegistration := range svcs {
 			// If we selectively deregister, only deregister if the address is not in the map. Otherwise, deregister
 			// every service instance.
+			var serviceDeregistered bool
 			if endpointsAddressesMap != nil {
 				if _, ok := endpointsAddressesMap[serviceRegistration.Address]; !ok {
 					// If the service address is not in the Endpoints addresses, deregister it.
@@ -722,18 +723,20 @@ func (r *EndpointsController) deregisterServiceOnAllAgents(ctx context.Context,
 						r.Log.Error(err, "failed to deregister service instance", "id", svcID)
 						return err
 					}
+					serviceDeregistered = true
 				}
 			} else {
 				r.Log.Info("deregistering service from consul", "svc", svcID)
 				if err = client.Agent().ServiceDeregister(svcID); err != nil {
 					r.Log.Error(err, "failed to deregister service instance", "id", svcID)
 					return err
 				}
+				serviceDeregistered = true
 			}
 
-			if r.AuthMethod != "" {
+			if r.AuthMethod != "" && serviceDeregistered {
 				r.Log.Info("reconciling ACL tokens for service", "svc", serviceRegistration.Service)
-				err = r.reconcileACLTokensForService(client, serviceRegistration.Service, k8sSvcNamespace, endpointPods)
+				err = r.deleteACLTokensForServiceInstance(client, serviceRegistration.Service, k8sSvcNamespace, serviceRegistration.Meta[MetaKeyPodName])
 				if err != nil {
 					r.Log.Error(err, "failed to reconcile ACL tokens for service", "svc", serviceRegistration.Service)
 					return err
@@ -744,11 +747,15 @@ func (r *EndpointsController) deregisterServiceOnAllAgents(ctx context.Context,
 	return nil
 }
 
-// reconcileACLTokensForService finds the ACL tokens that belongs to the service and deletes it from Consul.
+// deleteACLTokensForServiceInstance finds the ACL tokens that belongs to the service instance and deletes it from Consul.
 // It will only check for ACL tokens that have been created with the auth method this controller
-// has been configured with and will only delete tokens for pods that aren't in endpointPods
-// (endpointPods is a set of pods that the endpoints object is pointing to).
-func (r *EndpointsController) reconcileACLTokensForService(client *api.Client, serviceName, k8sNS string, endpointPods mapset.Set) error {
+// has been configured with and will only delete tokens for the provided podName.
+func (r *EndpointsController) deleteACLTokensForServiceInstance(client *api.Client, serviceName, k8sNS, podName string) error {
+	// Skip if podName is empty.
+	if podName == "" {
+		return nil
+	}
+
 	tokens, _, err := client.ACL().TokenList(nil)
 	if err != nil {
 		return fmt.Errorf("failed to get a list of tokens from Consul: %s", err)
@@ -766,10 +773,10 @@ func (r *EndpointsController) reconcileACLTokensForService(client *api.Client, s
 				return fmt.Errorf("failed to parse token metadata: %s", err)
 			}
 
-			podName := strings.TrimPrefix(tokenMeta[TokenMetaPodNameKey], k8sNS+"/")
+			tokenPodName := strings.TrimPrefix(tokenMeta[TokenMetaPodNameKey], k8sNS+"/")
 
 			// If we can't find token's pod, delete it.
-			if !endpointPods.Contains(podName) {
+			if tokenPodName == podName {
 				r.Log.Info("deleting ACL token for pod", "name", podName)
 				_, err = client.ACL().TokenDelete(token.AccessorID, nil)
 				if err != nil {

control-plane/connect-inject/endpoints_controller_ent_test.go

@@ -1233,11 +1233,13 @@ func TestReconcileUpdateEndpointWithNamespaces(t *testing.T) {
 
 							tokensForServices[svc.ID] = token.AccessorID
 
-							// Create another token for the same service but a pod that no longer exists.
-							// This is to test a scenario with orphaned tokens
-							// where we have a token for the pod but the service instance
-							// for that pod no longer exists in Consul.
-							// In that case, the token should still be deleted.
+							// Create another token for the same service but a pod that either no longer exists
+							// or the endpoints controller doesn't know about it yet.
+							// This is to test a scenario with either orphaned tokens
+							// or tokens for services that haven't yet been registered with Consul.
+							// In that case, we have a token for the pod but the service instance
+							// for that pod either no longer exists or is not yet registered in Consul.
+							// This token should not be deleted.
 							token, _, err = consulClient.ACL().Login(&api.ACLLoginParams{
 								AuthMethod:  test.AuthMethod,
 								BearerToken: test.ServiceAccountJWTToken,
@@ -1313,23 +1315,30 @@ func TestReconcileUpdateEndpointWithNamespaces(t *testing.T) {
 
 				if tt.enableACLs {
 					// Put expected services into a map to make it easier to find service IDs.
-					expectedServices := make(map[string]struct{})
+					expectedServices := mapset.NewSet()
 					for _, svc := range tt.expectedConsulSvcInstances {
-						expectedServices[svc.ServiceID] = struct{}{}
+						expectedServices.Add(svc.ServiceID)
 					}
 
+					initialServices := mapset.NewSet()
+					for _, svc := range tt.initialConsulSvcs {
+						initialServices.Add(svc.ID)
+					}
+
+					// We only care about a case when services are deregistered, where
+					// the set of initial services is bigger than the set of expected services.
+					deregisteredServices := initialServices.Difference(expectedServices)
+
 					// Look through the tokens we've created and check that only
 					// tokens for the deregistered services have been deleted.
 					for serviceID, tokenID := range tokensForServices {
 						// Read the token from Consul.
 						token, _, err := consulClient.ACL().TokenRead(tokenID, nil)
-						if _, ok := expectedServices[serviceID]; ok {
-							// If service is expected to still exist in Consul, then the ACL token for it should not be deleted.
-							require.NoError(t, err)
-							require.NotNil(t, token)
-						} else {
-							// If service should no longer exist, then ACL token for it should be deleted.
+						if deregisteredServices.Contains(serviceID) {
 							require.EqualError(t, err, "Unexpected response code: 403 (ACL not found)")
+						} else {
+							require.NoError(t, err, "token should exist for service instance: "+serviceID)
+							require.NotNil(t, token)
 						}
 					}
 				}

control-plane/connect-inject/endpoints_controller_test.go

@@ -2302,11 +2302,13 @@ func TestReconcileUpdateEndpoint(t *testing.T) {
 						require.NoError(t, err)
 						tokensForServices[svc.ID] = token.AccessorID
 
-						// Create another token for the same service but a pod that no longer exists.
-						// This is to test a scenario with orphaned tokens
-						// where we have a token for the pod but the service instance
-						// for that pod no longer exists in Consul.
-						// In that case, the token should still be deleted.
+						// Create another token for the same service but a pod that either no longer exists
+						// or the endpoints controller doesn't know about it yet.
+						// This is to test a scenario with either orphaned tokens
+						// or tokens for services that haven't yet been registered with Consul.
+						// In that case, we have a token for the pod but the service instance
+						// for that pod either no longer exists or is not yet registered in Consul.
+						// This token should not be deleted.
 						token, _, err = consulClient.ACL().Login(&api.ACLLoginParams{
 							AuthMethod:  test.AuthMethod,
 							BearerToken: test.ServiceAccountJWTToken,
@@ -2372,23 +2374,30 @@ func TestReconcileUpdateEndpoint(t *testing.T) {
 
 			if tt.enableACLs {
 				// Put expected services into a map to make it easier to find service IDs.
-				expectedServices := make(map[string]struct{})
+				expectedServices := mapset.NewSet()
 				for _, svc := range tt.expectedConsulSvcInstances {
-					expectedServices[svc.ServiceID] = struct{}{}
+					expectedServices.Add(svc.ServiceID)
 				}
 
+				initialServices := mapset.NewSet()
+				for _, svc := range tt.initialConsulSvcs {
+					initialServices.Add(svc.ID)
+				}
+
+				// We only care about a case when services are deregistered, where
+				// the set of initial services is bigger than the set of expected services.
+				deregisteredServices := initialServices.Difference(expectedServices)
+
 				// Look through the tokens we've created and check that only
 				// tokens for the deregistered services have been deleted.
 				for serviceID, tokenID := range tokensForServices {
 					// Read the token from Consul.
 					token, _, err := consulClient.ACL().TokenRead(tokenID, nil)
-					if _, ok := expectedServices[serviceID]; ok {
-						// If service is expected to still exist in Consul, then the ACL token for it should not be deleted.
+					if deregisteredServices.Contains(serviceID) {
+						require.EqualError(t, err, "Unexpected response code: 403 (ACL not found)")
+					} else {
 						require.NoError(t, err, "token should exist for service instance: "+serviceID)
 						require.NotNil(t, token)
-					} else {
-						// If service should no longer exist, then ACL token for it should be deleted.
-						require.EqualError(t, err, "Unexpected response code: 403 (ACL not found)")
 					}
 				}
 			}