Skip to content

Commit

Permalink
Delete ACL token only for services that are deregistered. Fixes #599.
Browse files Browse the repository at this point in the history
  • Loading branch information
@ishustava
ishustava committed Aug 12, 2021
1 parent 400ef6e commit 7457ea4
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 33 deletions.
23 changes: 15 additions & 8 deletions control-plane/connect-inject/endpoints_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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 {
Expand Down
35 changes: 22 additions & 13 deletions control-plane/connect-inject/endpoints_controller_ent_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)
}
}
}
Expand Down
33 changes: 21 additions & 12 deletions control-plane/connect-inject/endpoints_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)")
}
}
}
Expand Down

0 comments on commit 7457ea4

Please sign in to comment.