diff --git a/.changelog/4060.txt b/.changelog/4060.txt new file mode 100644 index 0000000000..2756563e2f --- /dev/null +++ b/.changelog/4060.txt @@ -0,0 +1,3 @@ +```release-note:bug +api-gateway: fix issue where API Gateway specific acl roles/policy were not being cleaned up on deletion of an api-gateway +``` diff --git a/acceptance/framework/environment/environment.go b/acceptance/framework/environment/environment.go index 9ceecf3e96..75b89f09c8 100644 --- a/acceptance/framework/environment/environment.go +++ b/acceptance/framework/environment/environment.go @@ -5,6 +5,8 @@ package environment import ( "fmt" + + "github.com/go-logr/logr" "github.com/gruntwork-io/terratest/modules/k8s" "github.com/hashicorp/consul-k8s/acceptance/framework/config" "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" @@ -15,6 +17,7 @@ import ( "k8s.io/client-go/kubernetes" clientgoscheme "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client" + logf "sigs.k8s.io/controller-runtime/pkg/log" gwv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" gwv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" ) @@ -187,6 +190,7 @@ func (k kubernetesContext) ControllerRuntimeClient(t testutil.TestingTB) client. client, err := client.New(config, client.Options{Scheme: s}) require.NoError(t, err) + logf.SetLogger(logr.New(nil)) k.runtimeClient = client diff --git a/acceptance/go.mod b/acceptance/go.mod index bb83e69ae3..4957ba0cea 100644 --- a/acceptance/go.mod +++ b/acceptance/go.mod @@ -50,6 +50,7 @@ require ( github.com/go-jose/go-jose/v3 v3.0.3 // indirect github.com/go-logr/logr v1.2.4 // indirect github.com/go-logr/stdr v1.2.2 // indirect + github.com/go-logr/zapr v1.2.4 // indirect github.com/go-openapi/analysis v0.21.4 // indirect github.com/go-openapi/errors v0.20.3 // indirect github.com/go-openapi/jsonpointer v0.19.6 // indirect @@ -121,6 +122,8 @@ require ( go.opentelemetry.io/otel/metric v1.19.0 // indirect go.opentelemetry.io/otel/sdk v1.19.0 // indirect go.opentelemetry.io/otel/trace v1.19.0 // indirect + go.uber.org/multierr v1.11.0 // indirect + go.uber.org/zap v1.25.0 // indirect golang.org/x/crypto v0.22.0 // indirect golang.org/x/exp v0.0.0-20230817173708-d852ddb80c63 // indirect golang.org/x/mod v0.14.0 // indirect diff --git a/acceptance/go.sum b/acceptance/go.sum index 82e6969ecd..ffde55b356 100644 --- a/acceptance/go.sum +++ b/acceptance/go.sum @@ -20,6 +20,9 @@ github.com/asaskevich/govalidator v0.0.0-20210307081110-f21760c49a8d h1:Byv0BzEl github.com/asaskevich/govalidator v0.0.0-20210307081110-f21760c49a8d/go.mod h1:WaHUgvxTVq04UNunO+XhnAqY/wQc+bxr74GqbsZ/Jqw= github.com/aws/aws-sdk-go v1.44.262 h1:gyXpcJptWoNkK+DiAiaBltlreoWKQXjAIh6FRh60F+I= github.com/aws/aws-sdk-go v1.44.262/go.mod h1:aVsgQcEevwlmQ7qHE9I3h+dtQgpqhFB+i8Phjh7fkwI= +github.com/benbjohnson/clock v1.1.0/go.mod h1:J11/hYXuz8f4ySSvYwY0FKfm+ezbsZBKZxNJlLklBHA= +github.com/benbjohnson/clock v1.3.0 h1:ip6w0uFQkncKQ979AypyG0ER7mqUSBdKLOgAle/AT8A= +github.com/benbjohnson/clock v1.3.0/go.mod h1:J11/hYXuz8f4ySSvYwY0FKfm+ezbsZBKZxNJlLklBHA= github.com/beorn7/perks v0.0.0-20180321164747-3a771d992973/go.mod h1:Dwedo/Wpr24TaqPxmxbtue+5NUziq4I4S80YR8gNf3Q= github.com/beorn7/perks v1.0.0/go.mod h1:KWe93zE9D1o94FZ5RNwFwVgaQK1VOXiVxmqh+CedLV8= github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= @@ -454,8 +457,14 @@ go.opentelemetry.io/otel/trace v1.19.0 h1:DFVQmlVbfVeOuBRrwdtaehRrWiL1JoVs9CPIQ1 go.opentelemetry.io/otel/trace v1.19.0/go.mod h1:mfaSyvGyEJEI0nyV2I4qhNQnbBOUUmYZpYojqMnX2vo= go.opentelemetry.io/proto/otlp v1.0.0 h1:T0TX0tmXU8a3CbNXzEKGeU5mIVOdf0oykP+u2lIVU/I= go.opentelemetry.io/proto/otlp v1.0.0/go.mod h1:Sy6pihPLfYHkr3NkUbEhGHFhINUSI/v80hjKIs5JXpM= +go.uber.org/atomic v1.7.0/go.mod h1:fEN4uk6kAWBTFdckzkM89CLk9XfWZrxpCo0nPH17wJc= +go.uber.org/goleak v1.1.11/go.mod h1:cwTWslyiVhfpKIDGSZEM2HlOvcqm+tG4zioyIeLoqMQ= +go.uber.org/goleak v1.2.1 h1:NBol2c7O1ZokfZ0LEU9K6Whx/KnwvepVetCUhtKja4A= +go.uber.org/goleak v1.2.1/go.mod h1:qlT2yGI9QafXHhZZLxlSuNsMw3FFLxBr+tBRlmO1xH4= +go.uber.org/multierr v1.6.0/go.mod h1:cdWPpRnG4AhwMwsgIHip0KRBQjJy5kYEpYjJxpXp9iU= go.uber.org/multierr v1.11.0 h1:blXXJkSxSSfBVBlC76pxqeO+LN3aDfLQo+309xJstO0= go.uber.org/multierr v1.11.0/go.mod h1:20+QtiLqy0Nd6FdQB9TLXag12DsQkrbs3htMFfDN80Y= +go.uber.org/zap v1.24.0/go.mod h1:2kMP+WWQ8aoFoedH3T2sq6iJ2yDWpHbP0f6MQbS9Gkg= go.uber.org/zap v1.25.0 h1:4Hvk6GtkucQ790dqmj7l1eEnRdKm3k3ZUrUMS2d5+5c= go.uber.org/zap v1.25.0/go.mod h1:JIAUzQIH94IC4fOJQm7gMmBJP5k7wQfdcnYdPoEXJYk= golang.org/x/crypto v0.0.0-20180904163835-0709b304e793/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4= @@ -472,6 +481,7 @@ golang.org/x/crypto v0.22.0 h1:g1v0xeRhjcugydODzvb3mEM9SQ0HGp9s/nh3COQ/C30= golang.org/x/crypto v0.22.0/go.mod h1:vr6Su+7cTlO45qkww3VDJlzDn0ctJvRgYbC2NvXHt+M= golang.org/x/exp v0.0.0-20230817173708-d852ddb80c63 h1:m64FZMko/V45gv0bNmrNYoDEq8U5YUhetc9cBWKS1TQ= golang.org/x/exp v0.0.0-20230817173708-d852ddb80c63/go.mod h1:0v4NqG35kSWCMzLaMeX+IQrlSnVE/bqGSyC2cz/9Le8= +golang.org/x/lint v0.0.0-20190930215403-16217165b5de/go.mod h1:6SW0HCj/g11FgYtHlgUYUwCkIfeOF89ocIRzGO/8vkc= golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.4.2/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= @@ -574,6 +584,7 @@ golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= golang.org/x/time v0.3.0 h1:rg5rLMjNzMS1RkNLzCG38eapWhnYLFYXDXj2gOlr8j4= golang.org/x/time v0.3.0/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= +golang.org/x/tools v0.0.0-20190311212946-11955173bddd/go.mod h1:LCzVGOaR6xXOjkQ3onu1FJEFr0SW1gC7cKk1uF8kGRs= golang.org/x/tools v0.0.0-20190329151228-23e29df326fe/go.mod h1:LCzVGOaR6xXOjkQ3onu1FJEFr0SW1gC7cKk1uF8kGRs= golang.org/x/tools v0.0.0-20190416151739-9c9e1878f421/go.mod h1:LCzVGOaR6xXOjkQ3onu1FJEFr0SW1gC7cKk1uF8kGRs= golang.org/x/tools v0.0.0-20190420181800-aa740d480789/go.mod h1:LCzVGOaR6xXOjkQ3onu1FJEFr0SW1gC7cKk1uF8kGRs= @@ -582,6 +593,7 @@ golang.org/x/tools v0.0.0-20190907020128-2ca718005c18/go.mod h1:b+2E5dAYhXwXZwtn golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/tools v0.0.0-20200619180055-7c47624df98f/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= golang.org/x/tools v0.0.0-20210106214847-113979e3529a/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA= +golang.org/x/tools v0.1.5/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk= golang.org/x/tools v0.1.6-0.20210726203631-07bc1bf47fb2/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk= golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc= golang.org/x/tools v0.6.0/go.mod h1:Xwgl3UAJ/d3gWutnCtw505GrjyAbvKui8lOU390QaIU= diff --git a/acceptance/tests/api-gateway/api_gateway_lifecycle_test.go b/acceptance/tests/api-gateway/api_gateway_lifecycle_test.go index f6f66ed995..e8fbc945f1 100644 --- a/acceptance/tests/api-gateway/api_gateway_lifecycle_test.go +++ b/acceptance/tests/api-gateway/api_gateway_lifecycle_test.go @@ -27,8 +27,10 @@ func TestAPIGateway_Lifecycle(t *testing.T) { ctx := suite.Environment().DefaultContext(t) cfg := suite.Config() helmValues := map[string]string{ - "global.logLevel": "trace", - "connectInject.enabled": "true", + "global.logLevel": "trace", + "connectInject.enabled": "true", + "global.acls.manageSystemACLs": "true", + "global.tls.enabled": "true", } releaseName := helpers.RandomName() @@ -37,7 +39,7 @@ func TestAPIGateway_Lifecycle(t *testing.T) { consulCluster.Create(t) k8sClient := ctx.ControllerRuntimeClient(t) - consulClient, _ := consulCluster.SetupConsulClient(t, false) + consulClient, _ := consulCluster.SetupConsulClient(t, true) defaultNamespace := "default" @@ -141,6 +143,13 @@ func TestAPIGateway_Lifecycle(t *testing.T) { logger.Log(t, "creating route two") routeTwo := createRoute(t, k8sClient, routeTwoName, defaultNamespace, controlledGatewayTwoName, targetName) + // Scenario: Ensure ACL roles/policies are set correctly + logger.Log(t, "checking that ACL roles/policies are set correctly for controlled gateway one") + checkACLRolesPolicies(t, consulClient, controlledGatewayOneName) + + logger.Log(t, "checking that ACL roles/policies are set correctly for controlled gateway two") + checkACLRolesPolicies(t, consulClient, controlledGatewayTwoName) + // Scenario: Swapping a route to another controlled gateway should clean up the old parent statuses and references on Consul resources // check that the route is bound properly and objects are reflected in Consul @@ -210,6 +219,7 @@ func TestAPIGateway_Lifecycle(t *testing.T) { checkConsulNotExists(t, consulClient, api.HTTPRoute, routeOneName) // Scenario: Deleting a gateway should result in routes only referencing it to get cleaned up from Consul and their statuses/finalizers cleared, but routes referencing another controlled gateway should still exist in Consul and only have their statuses cleaned up from referencing the gateway we previously controlled. Any referenced certificates should also get cleaned up. + // and acl roles and policies for that gateway should be cleaned up // delete gateway two logger.Log(t, "deleting gateway two in Kubernetes") @@ -224,6 +234,12 @@ func TestAPIGateway_Lifecycle(t *testing.T) { logger.Log(t, "checking that http route one is cleaned up in Kubernetes") checkEmptyRoute(t, k8sClient, routeOneName, defaultNamespace) + logger.Log(t, "checking that ACL roles/policies are set correctly for controlled gateway one") + checkACLRolesPolicies(t, consulClient, controlledGatewayOneName) + + logger.Log(t, "checking that ACL roles/policies are removed for controlled gateway two") + checkACLRolesPoliciesDontExist(t, consulClient, controlledGatewayTwoName) + // Scenario: Changing a gateway class name on a gateway to something we don’t control should have the same affect as deleting it with the addition of cleaning up our finalizer from the gateway. // reset route one to point to our first gateway and check that it's bound properly @@ -442,3 +458,27 @@ func createGatewayClass(t *testing.T, client client.Client, name, controllerName err := client.Create(context.Background(), gatewayClass) require.NoError(t, err) } + +func checkACLRolesPolicies(t *testing.T, client *api.Client, gatewayName string) { + t.Helper() + retryCheck(t, 60, func(r *retry.R) { + role, _, err := client.ACL().RoleReadByName(fmt.Sprint("managed-gateway-acl-role-", gatewayName), nil) + require.NoError(r, err) + require.NotNil(r, role) + policy, _, err := client.ACL().PolicyReadByName(fmt.Sprint("api-gateway-policy-for-", gatewayName), nil) + require.NoError(r, err) + require.NotNil(r, policy) + }) +} + +func checkACLRolesPoliciesDontExist(t *testing.T, client *api.Client, gatewayName string) { + t.Helper() + retryCheck(t, 60, func(r *retry.R) { + role, _, err := client.ACL().RoleReadByName(fmt.Sprint("managed-gateway-acl-role-", gatewayName), nil) + require.NoError(r, err) + require.Nil(r, role) + policy, _, err := client.ACL().PolicyReadByName(fmt.Sprint("api-gateway-policy-for-", gatewayName), nil) + require.NoError(r, err) + require.Nil(r, policy) + }) +} diff --git a/control-plane/api-gateway/cache/consul.go b/control-plane/api-gateway/cache/consul.go index bb42a99493..8d2f5de9c9 100644 --- a/control-plane/api-gateway/cache/consul.go +++ b/control-plane/api-gateway/cache/consul.go @@ -6,6 +6,7 @@ package cache import ( "bytes" "context" + "errors" "fmt" "strings" "sync" @@ -83,11 +84,14 @@ type Cache struct { subscribers map[string][]*Subscription subscriberMutex *sync.Mutex - gatewayNameToPolicy map[string]*api.ACLPolicy - policyMutex *sync.Mutex + gatewayNameToACLPolicy map[string]*api.ACLPolicy + policyMutex *sync.Mutex - gatewayNameToRole map[string]*api.ACLRole - aclRoleMutex *sync.Mutex + gatewayNameToACLRole map[string]*api.ACLRole + aclRoleMutex *sync.Mutex + + gatewayNameToACLBindingRule map[string]*api.ACLBindingRule + bindingRuleMutex *sync.Mutex namespacesEnabled bool crossNamespaceACLPolicy string @@ -108,22 +112,24 @@ func New(config Config) *Cache { config.ConsulClientConfig.APITimeout = apiTimeout return &Cache{ - config: config.ConsulClientConfig, - serverMgr: config.ConsulServerConnMgr, - namespacesEnabled: config.NamespacesEnabled, - cache: cache, - cacheMutex: &sync.Mutex{}, - subscribers: make(map[string][]*Subscription), - subscriberMutex: &sync.Mutex{}, - gatewayNameToPolicy: make(map[string]*api.ACLPolicy), - policyMutex: &sync.Mutex{}, - gatewayNameToRole: make(map[string]*api.ACLRole), - aclRoleMutex: &sync.Mutex{}, - kinds: Kinds, - synced: make(chan struct{}, len(Kinds)), - logger: config.Logger, - crossNamespaceACLPolicy: config.CrossNamespaceACLPolicy, - datacenter: config.Datacenter, + config: config.ConsulClientConfig, + serverMgr: config.ConsulServerConnMgr, + namespacesEnabled: config.NamespacesEnabled, + cache: cache, + cacheMutex: &sync.Mutex{}, + subscribers: make(map[string][]*Subscription), + subscriberMutex: &sync.Mutex{}, + gatewayNameToACLPolicy: make(map[string]*api.ACLPolicy), + policyMutex: &sync.Mutex{}, + gatewayNameToACLRole: make(map[string]*api.ACLRole), + aclRoleMutex: &sync.Mutex{}, + gatewayNameToACLBindingRule: make(map[string]*api.ACLBindingRule), + bindingRuleMutex: &sync.Mutex{}, + kinds: Kinds, + synced: make(chan struct{}, len(Kinds)), + logger: config.Logger, + crossNamespaceACLPolicy: config.CrossNamespaceACLPolicy, + datacenter: config.Datacenter, } } @@ -364,7 +370,7 @@ func (c *Cache) ensurePolicy(client *api.Client, gatewayName string) (string, er } // on an upgrade the cache will be empty so we need to write the policy to the cache - c.gatewayNameToPolicy[gatewayName] = existing + c.gatewayNameToACLPolicy[gatewayName] = existing return existing.ID, nil } @@ -372,11 +378,11 @@ func (c *Cache) ensurePolicy(client *api.Client, gatewayName string) (string, er return "", err } - c.gatewayNameToPolicy[gatewayName] = created + c.gatewayNameToACLPolicy[gatewayName] = created return created.ID, nil } - cachedPolicy, found := c.gatewayNameToPolicy[gatewayName] + cachedPolicy, found := c.gatewayNameToACLPolicy[gatewayName] if !found { return createPolicy() @@ -393,7 +399,7 @@ func (c *Cache) ensurePolicy(client *api.Client, gatewayName string) (string, er } // update cache with existing policy - c.gatewayNameToPolicy[gatewayName] = existing + c.gatewayNameToACLPolicy[gatewayName] = existing return existing.ID, nil } @@ -420,7 +426,7 @@ func (c *Cache) ensureRole(client *api.Client, gatewayName string) (string, erro _, _, err = client.ACL().RoleCreate(role, &api.WriteOptions{}) if err != nil && !isRoleExistsErr(err, aclRoleName) { - //don't error out in the case that the role already exists. + // don't error out in the case that the role already exists. return "", err } @@ -435,15 +441,16 @@ func (c *Cache) ensureRole(client *api.Client, gatewayName string) (string, erro if err != nil { return "", err } - c.gatewayNameToRole[gatewayName] = role + + c.gatewayNameToACLRole[gatewayName] = role return aclRoleName, err } - c.gatewayNameToRole[gatewayName] = role + c.gatewayNameToACLRole[gatewayName] = role return aclRoleName, nil } - cachedRole, found := c.gatewayNameToRole[gatewayName] + cachedRole, found := c.gatewayNameToACLRole[gatewayName] if !found { return createRole() @@ -455,7 +462,7 @@ func (c *Cache) ensureRole(client *api.Client, gatewayName string) (string, erro } if aclRole != nil { - c.gatewayNameToRole[gatewayName] = aclRole + c.gatewayNameToACLRole[gatewayName] = aclRole return aclRole.Name, nil } @@ -568,10 +575,140 @@ func (c *Cache) EnsureRoleBinding(authMethod, service, namespace string) error { if bindingRule.ID == "" { _, _, err := client.ACL().BindingRuleCreate(bindingRule, &api.WriteOptions{}) - return err + if err != nil { + return err + } + + c.bindingRuleMutex.Lock() + defer c.bindingRuleMutex.Unlock() + c.gatewayNameToACLBindingRule[service] = bindingRule + + return nil } _, _, err = client.ACL().BindingRuleUpdate(bindingRule, &api.WriteOptions{}) - return err + if err != nil { + return err + } + + c.bindingRuleMutex.Lock() + defer c.bindingRuleMutex.Unlock() + c.gatewayNameToACLBindingRule[service] = bindingRule + + return nil +} + +var ( + ErrFailedToDeleteBindingRule = errors.New("failed to delete ACLBindingRule") + ErrFailedToDeleteRole = errors.New("failed to delete ACLRole") + ErrFailedToDeletePolicy = errors.New("failed to delete ACLPolicy") + ErrACLSDisabled = errors.New("ACLs are disabled") +) + +func (c *Cache) RemoveRoleBinding(authMethod, service, namespace string) error { + client, err := consul.NewClientFromConnMgr(c.config, c.serverMgr) + if err != nil { + return err + } + + // acquire locks + c.bindingRuleMutex.Lock() + defer c.bindingRuleMutex.Unlock() + + c.aclRoleMutex.Lock() + defer c.aclRoleMutex.Unlock() + + c.policyMutex.Lock() + defer c.policyMutex.Unlock() + + deleteFns := make([]func() error, 0, 3) + + if rule, ok := c.gatewayNameToACLBindingRule[service]; ok { + deleteFns = append(deleteFns, c.bindingRuleDelete(client, rule, service)) + } + + if role, ok := c.gatewayNameToACLRole[service]; ok { + deleteFns = append(deleteFns, c.roleDelete(client, role, service)) + } + + if policy, ok := c.gatewayNameToACLPolicy[service]; ok { + deleteFns = append(deleteFns, c.policyDelete(client, policy, service)) + } + + for _, fn := range deleteFns { + err := fn() + if errors.Is(err, ErrACLSDisabled) { + return nil + } + if err != nil { + return err + } + } + + return nil +} + +func (c *Cache) bindingRuleDelete(client *api.Client, rule *api.ACLBindingRule, service string) func() error { + return func() error { + _, err := client.ACL().BindingRuleDelete(rule.ID, &api.WriteOptions{}) + if err != nil { + if ignoreNotFound(err) == nil { + delete(c.gatewayNameToACLBindingRule, service) + return nil + } + + if ignoreACLsDisabled(err) == nil { + delete(c.gatewayNameToACLBindingRule, service) + return ErrACLSDisabled + } + return fmt.Errorf("%w: %s", ErrFailedToDeleteBindingRule, err) + } + + delete(c.gatewayNameToACLBindingRule, service) + + return nil + } +} + +func (c *Cache) roleDelete(client *api.Client, role *api.ACLRole, service string) func() error { + return func() error { + _, err := client.ACL().RoleDelete(role.ID, &api.WriteOptions{}) + if err != nil { + if ignoreNotFound(err) == nil { + delete(c.gatewayNameToACLRole, service) + return nil + } + + if ignoreACLsDisabled(err) == nil { + delete(c.gatewayNameToACLBindingRule, service) + return ErrACLSDisabled + } + return fmt.Errorf("%w: %s", ErrFailedToDeleteRole, err) + } + delete(c.gatewayNameToACLRole, service) + + return nil + } +} + +func (c *Cache) policyDelete(client *api.Client, policy *api.ACLPolicy, service string) func() error { + return func() error { + _, err := client.ACL().PolicyDelete(policy.ID, &api.WriteOptions{}) + if err != nil { + if ignoreNotFound(err) == nil { + delete(c.gatewayNameToACLPolicy, service) + return nil + } + + if ignoreACLsDisabled(err) == nil { + delete(c.gatewayNameToACLBindingRule, service) + return ErrACLSDisabled + } + return fmt.Errorf("%w: %s", ErrFailedToDeletePolicy, err) + } + delete(c.gatewayNameToACLPolicy, service) + + return nil + } } // Register registers a service in Consul. @@ -600,6 +737,17 @@ func (c *Cache) Deregister(ctx context.Context, deregistration api.CatalogDeregi return err } +func ignoreNotFound(err error) error { + if err == nil { + return nil + } + if strings.Contains(err.Error(), "Unexpected response code: 404") { + return nil + } + + return err +} + func ignoreACLsDisabled(err error) error { if err == nil { return nil diff --git a/control-plane/api-gateway/cache/consul_test.go b/control-plane/api-gateway/cache/consul_test.go index d206c0a8a8..6ff8661498 100644 --- a/control-plane/api-gateway/cache/consul_test.go +++ b/control-plane/api-gateway/cache/consul_test.go @@ -2045,6 +2045,201 @@ func TestCache_Delete(t *testing.T) { } } +func TestCache_RemoveRoleBinding(t *testing.T) { + t.Parallel() + successFn := func(w http.ResponseWriter) { + w.WriteHeader(200) + fmt.Fprintln(w, `{deleted: true}`) + } + + notFoundFn := func(w http.ResponseWriter) { + w.WriteHeader(404) + } + + aclDisabledFn := func(w http.ResponseWriter) { + w.WriteHeader(401) + fmt.Fprintln(w, `ACL support disabled`) + } + + errorFn := func(w http.ResponseWriter) { + w.WriteHeader(500) + fmt.Fprintln(w, `error`) + } + + testCases := map[string]struct { + bindingRule *api.ACLBindingRule + role *api.ACLRole + policy *api.ACLPolicy + bindingRuleRespFn func(w http.ResponseWriter) + roleRespFn func(w http.ResponseWriter) + policyRespFn func(w http.ResponseWriter) + expectedErr error + }{ + "delete is successful": { + bindingRule: &api.ACLBindingRule{ + ID: "binding-rule-id", + }, + role: &api.ACLRole{ + ID: "role-id", + }, + policy: &api.ACLPolicy{ + ID: "policy-id", + }, + bindingRuleRespFn: successFn, + roleRespFn: successFn, + policyRespFn: successFn, + expectedErr: nil, + }, + "binding rule is not found": { + bindingRule: &api.ACLBindingRule{ + ID: "binding-rule-id", + }, + role: &api.ACLRole{ + ID: "role-id", + }, + policy: &api.ACLPolicy{ + ID: "policy-id", + }, + bindingRuleRespFn: notFoundFn, + roleRespFn: successFn, + policyRespFn: successFn, + expectedErr: nil, + }, + "role is not found": { + bindingRule: &api.ACLBindingRule{ + ID: "binding-rule-id", + }, + role: &api.ACLRole{ + ID: "role-id", + }, + policy: &api.ACLPolicy{ + ID: "policy-id", + }, + bindingRuleRespFn: successFn, + roleRespFn: notFoundFn, + policyRespFn: successFn, + expectedErr: nil, + }, + "policy is not found": { + bindingRule: &api.ACLBindingRule{ + ID: "binding-rule-id", + }, + role: &api.ACLRole{ + ID: "role-id", + }, + policy: &api.ACLPolicy{ + ID: "policy-id", + }, + bindingRuleRespFn: successFn, + roleRespFn: successFn, + policyRespFn: notFoundFn, + expectedErr: nil, + }, + "acl support is disabled": { + bindingRule: &api.ACLBindingRule{ + ID: "binding-rule-id", + }, + role: &api.ACLRole{ + ID: "role-id", + }, + policy: &api.ACLPolicy{ + ID: "policy-id", + }, + bindingRuleRespFn: aclDisabledFn, + expectedErr: nil, + }, + "failed to delete binding rule": { + bindingRule: &api.ACLBindingRule{ + ID: "binding-rule-id", + }, + role: &api.ACLRole{ + ID: "role-id", + }, + policy: &api.ACLPolicy{ + ID: "policy-id", + }, + bindingRuleRespFn: errorFn, + expectedErr: ErrFailedToDeleteBindingRule, + }, + "failed to delete role": { + bindingRule: &api.ACLBindingRule{ + ID: "binding-rule-id", + }, + role: &api.ACLRole{ + ID: "role-id", + }, + policy: &api.ACLPolicy{ + ID: "policy-id", + }, + bindingRuleRespFn: successFn, + roleRespFn: errorFn, + expectedErr: ErrFailedToDeleteRole, + }, + "failed to delete policy": { + bindingRule: &api.ACLBindingRule{ + ID: "binding-rule-id", + }, + role: &api.ACLRole{ + ID: "role-id", + }, + policy: &api.ACLPolicy{ + ID: "policy-id", + }, + bindingRuleRespFn: successFn, + roleRespFn: successFn, + policyRespFn: errorFn, + expectedErr: ErrFailedToDeletePolicy, + }, + } + for name, tt := range testCases { + t.Run(name, func(t *testing.T) { + consulServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case fmt.Sprintf("/v1/acl/binding-rule/%s", tt.bindingRule.ID): + tt.bindingRuleRespFn(w) + case fmt.Sprintf("/v1/acl/role/%s", tt.role.ID): + tt.roleRespFn(w) + case fmt.Sprintf("/v1/acl/policy/%s", tt.policy.ID): + tt.policyRespFn(w) + default: + w.WriteHeader(500) + fmt.Fprintln(w, "Mock Server not configured for this route: "+r.URL.Path) + } + })) + defer consulServer.Close() + + serverURL, err := url.Parse(consulServer.URL) + require.NoError(t, err) + + port, err := strconv.Atoi(serverURL.Port()) + require.NoError(t, err) + + c := New(Config{ + ConsulClientConfig: &consul.Config{ + APIClientConfig: &api.Config{}, + HTTPPort: port, + GRPCPort: port, + APITimeout: 0, + }, + ConsulServerConnMgr: test.MockConnMgrForIPAndPort(t, serverURL.Hostname(), port, false), + NamespacesEnabled: false, + Logger: logrtest.NewTestLogger(t), + }) + + authMethod := "k8s-auth-method" + gatewayName := "my-api-gateway" + namespace := "ns" + // file the acl binding rule, acl policy, and acl role maps with the necessary data + c.gatewayNameToACLBindingRule[gatewayName] = tt.bindingRule + c.gatewayNameToACLRole[gatewayName] = tt.role + c.gatewayNameToACLPolicy[gatewayName] = tt.policy + + err = c.RemoveRoleBinding(authMethod, gatewayName, namespace) + require.ErrorIs(t, err, tt.expectedErr) + }) + } +} + func loadedReferenceMaps(entries []api.ConfigEntry) map[string]*common.ReferenceMap { refs := make(map[string]*common.ReferenceMap) diff --git a/control-plane/api-gateway/controllers/gateway_controller.go b/control-plane/api-gateway/controllers/gateway_controller.go index 1dfe3be3af..8fb6a1faec 100644 --- a/control-plane/api-gateway/controllers/gateway_controller.go +++ b/control-plane/api-gateway/controllers/gateway_controller.go @@ -244,6 +244,12 @@ func (r *GatewayController) Reconcile(ctx context.Context, req ctrl.Request) (ct log.Error(err, "error deregistering services") return ctrl.Result{}, err } + + err = r.cache.RemoveRoleBinding(r.HelmConfig.AuthMethod, gateway.Name, gateway.Namespace) + if err != nil { + log.Error(err, "error removing acl role bindings") + return ctrl.Result{}, err + } } for _, deletion := range updates.Consul.Deletions {