[AWSCORE-221] Remove enumerated IAM permissions from aws_quickstart template #250
Conversation
aws_quickstart/version.txt
Outdated
| @@ -1 +1 @@ | |||
| v4.1.4 | |||
There was a problem hiding this comment.
I think this warrants a major version update
There was a problem hiding this comment.
I think that depends: is it backwards compatible? e.g., will customers updating from an older version to this experience a breaking change?
There was a problem hiding this comment.
This shouldn't be a breaking change, the only thing changing currently is the naming of the resource collection permissions managed policies, from {role name}-ManagedPolicy-* to datadog-aws-integration-resource-collection-permissions-*. To be fully backwards compatible we could also keep the original naming
aws_quickstart/CHANGELOG.md
Outdated
| # 4.2.4 (October 24, 2025) | ||
|
|
||
| - Remove enumerated IAM permissions | ||
| - Add Lambda function to attach IAM permissions |
There was a problem hiding this comment.
Maybe let's write down a more comprehensive changelog for customers, similar to the PR description
Co-authored-by: Katie McKew <5915468+ktmq@users.noreply.github.com>
| # Remove resource collection permissions | ||
| for i in range(max_policies): | ||
| policy_name = f"{BASE_POLICY_PREFIX_RESOURCE_COLLECTION}-{i+1}" | ||
| policy_arn = f"arn:aws:iam::{account_id}:policy/{policy_name}" |
There was a problem hiding this comment.
💭 anywhere we construct policy arns we should use ${AWS::Partition} so it works for govcloud and china
| handle_delete(event, context, role_name, account_id) | ||
| else: | ||
| handle_create_update(event, context, role_name, account_id, should_install_security_audit_policy) | ||
| DatadogAttachIntegrationPermissionsFunctionTrigger: |
There was a problem hiding this comment.
💭 should this have a dependency on DatadogIntegrationRole to ensure the role is created first?
There was a problem hiding this comment.
we should, there's an implicit dependency on the Lambda resource but this would make it completely safe
| - iam:PutRolePolicy | ||
| Resource: | ||
| - !Sub arn:aws:iam::${AWS::AccountId}:role/${IAMRoleName} | ||
| - !Sub arn:aws:iam::${AWS::AccountId}:policy/datadog-aws-integration-resource-collection-permissions-* |
There was a problem hiding this comment.
💭 re: the name datadog-aws-integration-resource-collection-permissions-* -- these aren't all necessarily resource collection related, right? these are also the standard and core permissions
There was a problem hiding this comment.
or are we just missing the policies containing the standard perms here?
There was a problem hiding this comment.
these permissions datadog-aws-integration-resource-collection-permissions-* are all resource collection related (there is some overlap with standard but only the ones necessary for resource collection). since standard permissions are attached here as an inline policy, we don't need to add the naming here since we can just create it directly
raymondeah
deleted the
ray.eah/awscore-221-quickstart-remove-iam-permissions
branch
3 participants
What does this PR do?
Removes all enumerated IAM permissions from the
datadog_integration_rolenested stack. Adds a Lambda function to retrieve the same permissions from the Datadog API and attach them to the integration role.Motivation
We want to cut down as much as possible all places where we enumerate IAM permissions. Since we recently released public API endpoints returning the IAM permissions required by the integration, we can use this instead.
Testing Guidelines
link to new template
Tested following cases:
Additional Notes
Anything else we should know when reviewing?