Add Cognito user pool policy and terminator#332
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for cleaning up AWS Cognito user pool resources created during Ansible integration tests. It introduces a new CognitoUserPool terminator class that discovers and deletes user pools, properly handling the deletion of user pool domains before the pools themselves. The PR also adds IAM policy permissions for Cognito IDP operations needed by both integration tests and the cleanup terminator.
Changes:
- Added
CognitoUserPoolterminator class to automatically clean up stale user pools - Added IAM policy with permissions for Cognito user pool, client, domain, and managed login branding operations
- Implemented proper deletion ordering (domains before pools) as required by AWS API
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| aws/terminator/cognito.py | New terminator class that discovers user pools via pagination and handles domain deletion before pool deletion |
| aws/policy/cognito.yaml | IAM policy granting permissions for Cognito IDP operations, with resource-scoped and global action statements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b8ba3c6 to
3c6fb70
Compare
alinabuzachis
left a comment
There was a problem hiding this comment.
Could you please move these files into existing ones? Maybe application-services?
| - cognito-idp:CreateUserPool | ||
| - cognito-idp:CreateUserPoolClient | ||
| - cognito-idp:CreateUserPoolDomain |
There was a problem hiding this comment.
| - cognito-idp:CreateUserPool | |
| - cognito-idp:CreateUserPoolClient | |
| - cognito-idp:CreateUserPoolDomain | |
| - cognito-idp:CreateUserPool* |
| - cognito-idp:DeleteManagedLoginBranding | ||
| - cognito-idp:DeleteUserPool | ||
| - cognito-idp:DeleteUserPoolClient | ||
| - cognito-idp:DeleteUserPoolDomain |
There was a problem hiding this comment.
Maybe we could also wildcard - cognito-idp:Describe*, but you can also have:
- cognito-idp:DescribeManagedLoginBranding*
- cognito-idp:DescribeUserPool*
| return self.instance['CreationDate'] | ||
|
|
||
| def terminate(self): | ||
| # User pool domains must be deleted before the user pool can be deleted |
There was a problem hiding this comment.
Shouldn't the ManagedLoginBranding be deleted too?
There was a problem hiding this comment.
⏺ No, you likely don't need a separate ManagedLoginBranding section in the terminator. Here's my reasoning:
- ManagedLoginBranding is a sub-resource of a User Pool — it exists only within a user pool and has no independent lifecycle. When the user pool is
deleted, its branding should be cleaned up automatically by AWS. - No list API exists — Unlike user pools (which have list_user_pools), there's no ListManagedLoginBrandings API. You can only DescribeManagedLoginBranding
by ID or DescribeManagedLoginBrandingByClient by client+pool ID, making it impractical to create a standalone Terminator subclass that discovers these
resources. - The domain precedent is different — The existing CognitoUserPool.terminate() at aws/terminator/cognito.py:24-32 explicitly deletes user pool domains
before the pool because domains are globally unique DNS resources that must be released before pool deletion. ManagedLoginBranding doesn't have that
constraint.
If it turns out that delete_user_pool fails when ManagedLoginBranding is present (which would be surprising), the right fix would be to add cleanup in the
existing CognitoUserPool.terminate() method — enumerate branding via ListUserPoolClients + DescribeManagedLoginBrandingByClient and delete each one before
calling delete_user_pool — similar to the domain handling pattern already there.
| - cognito-idp:DeleteUserPool | ||
| - cognito-idp:DeleteUserPoolClient | ||
| - cognito-idp:DeleteUserPoolDomain |
There was a problem hiding this comment.
| - cognito-idp:DeleteUserPool | |
| - cognito-idp:DeleteUserPoolClient | |
| - cognito-idp:DeleteUserPoolDomain | |
| - cognito-idp:DeleteUserPool* |
There was a problem hiding this comment.
Not worth it for now -- there's plenty of room in this file and I dislike wildcarding any write-capable permissions.
There was a problem hiding this comment.
(In truth, I dislike wildcarding ANY permissions, but you do what you have to do.)
4005c30 to
b422c56
Compare
Add IAM policy for cognito-idp operations (user pools, clients, domains, and managed login branding) and a CognitoUserPool terminator class to clean up stale user pools left behind by integration tests.
Summary
cognito-idpoperations (user pools, clients, domains, and managed login branding)CognitoUserPoolterminator class to clean up stale user pools left by integration testsRelated
Supports ansible-collections/community.aws#2412, which adds new Cognito modules (
cognito_user_pool,cognito_user_pool_client,cognito_user_pool_domain,cognito_managed_login_branding) with integration tests that create user pool resources.Test plan