Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aws-lambda: Defining a deadLetterQueue or deadLetterTopic will *always* add a policy to the function's execution role #29879

Open
nathandines opened this issue Apr 17, 2024 · 4 comments
Labels
@aws-cdk/aws-lambda Related to AWS Lambda bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@nathandines
Copy link

nathandines commented Apr 17, 2024

Describe the bug

As indicated in the title, if you define a deadLetterQueue or deadLetterTopic on a Function, it will always append an inline policy to the execution role which is associated with the function.

I would suggest that this is expected behaviour if the Function creates the IAM role, but it may be unexpected if the IAM role is defined elsewhere. It feels a lot like a side-effect. This feels like a problem within the same CloudFormation stack, but would probably be even more unexpected if the IAM role being referenced originated outside the stack.

Expected Behavior

I created an IAM role and SQS queue adjacent to a Lambda Function. I would have expected that if my IAM role were lacking a permission, it would:

  • Ideally, throw an error due to a missing permission (although this may be hard to implement, especially for non-stack IAM roles); or
  • Simply fail to drop failing events to the DLQ due to a lacking permission (this is less ideal because it may go unnoticed)

Current Behavior

IAM policies for SQS permission will indiscriminately be added to IAM roles no matter where that role originates from

Reproduction Steps

Relevant CDK Source Code

this.addToRolePolicy(new iam.PolicyStatement({
actions: ['sqs:SendMessage'],
resources: [deadLetterQueue.queueArn],
}));

public addToRolePolicy(statement: iam.PolicyStatement) {
if (!this.role) {
return;
}
this.role.addToPrincipalPolicy(statement);
}

Partial Code to Reproduce Behaviour

queue = aws_sqs.Queue(self, "queue")
role = aws_iam.Role(
    self,
    "role",
    managed_policies=[
        aws_iam.ManagedPolicy.from_aws_managed_policy_name(
            "service-role/AWSLambdaVPCAccessExecutionRole"
        ),
    ],
    assumed_by=aws_iam.ServicePrincipal("lambda.amazonaws.com"),
)
function = aws_lambda.Function(
    ...
    role=role,
    dead_letter_queue=queue,
    ...
)

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.114.1

Framework Version

2.114.1

Node.js Version

20.8.1

OS

macOS 14.2.1

Language

Python

Language Version

3.11.6

Other information

No response

@nathandines nathandines added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 17, 2024
@github-actions github-actions bot added the @aws-cdk/aws-lambda Related to AWS Lambda label Apr 17, 2024
@nathandines nathandines changed the title aws-lambda: Defining a deadLetterQueue or deadLetterTopic will *always* add a policy to the function's execution role aws-lambda: Defining a deadLetterQueue or deadLetterTopic will *always* add a policy to the function's execution role Apr 18, 2024
@khushail khushail added investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-triage This issue or PR still needs to be triaged. labels Apr 18, 2024
@khushail
Copy link
Contributor

Hi @nathandines , thanks for reaching out and sharing the repro code. I am able to repro the issue and can see the policy being added despite importing role. Marking the issue as appropriate.

@khushail khushail added p2 effort/small Small work item – less than a day of effort and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Apr 18, 2024
@sidharth15
Copy link

  1. Isn't this a necessary abstraction an L2 Construct should provide? In what way does providing the necessary permissions on the role create a problem with managing the role resource (even if it's in a separate stack)?
  2. If we accept this as an unexpected behaviour, should this extend to other permissions added to the role by the function Construct? For e.g.,
    1. // add additional managed policies when necessary
      if (props.filesystem) {
      const config = props.filesystem.config;
      if (config.policies) {
      config.policies.forEach(p => {
      this.role?.addToPrincipalPolicy(p);
      });
      }
      }
    2. props.profilingGroup.grantPublish(this.role);
    3. this.role?.addManagedPolicy(iam.ManagedPolicy.fromAwsManagedPolicyName('CloudWatchLambdaInsightsExecutionRolePolicy'));

@nathandines
Copy link
Author

  1. It certainly should be expected behaviour if you haven't defined a role, and are allowing the construct to manage it for you. The scenario which I'm talking about is when you've defined a self-managed role elsewhere, and then the construct manipulates it as a side-effect—I don't believe the construct should mutate resources which aren't under its purview.

    The problem is that a developer has presumably written the role out-of-band from the construct because they want/need some additional control over the management of it. To manipulate that role without the developer's explicit involvement is undermining that requirement.

    It could be for a variety of reasons: compliance, separation of concerns, specific permissions, existing deployment patterns, etc. It's not the tooling's job to judge whether a use-case is valid, but if the role is being managed out-of-band from the construct, the construct mustn't violate that intention.

  2. That's a valid consideration. I haven't explored the other permissions, but I would suggest that whatever behaviour is considered to be correct by the maintainers should be consistent across all permissions being applied.

@sidharth15
Copy link

Yes, your explanation makes sense to me. Looking at the pattern followed in the Lambda L2 Construct, and possibly other L2 constructs as well, it makes me think it was a conscious design choice. It is going to be a major change as it's likely that a lot of projects depend on these implicit permissions that the Construct is providing.

Personally, I rely on the permissions that Lambda adds automatically if I add resources like a source SQS queue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-lambda Related to AWS Lambda bug This issue is a bug. effort/small Small work item – less than a day of effort p2
Projects
None yet
Development

No branches or pull requests

3 participants