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

Minify and Merge Resource Policies #7732

Open
dcheckoway opened this issue May 1, 2020 · 10 comments
Open

Minify and Merge Resource Policies #7732

dcheckoway opened this issue May 1, 2020 · 10 comments
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management @aws-cdk/aws-sqs Related to Amazon Simple Queue Service effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2

Comments

@dcheckoway
Copy link

❓ General Issue

The Question

I'm using CDK to create a stack with:

  • one SQS Queue
  • numerous CloudWatch Rules with cron schedules that send a message to that one queue

The AWS::SQS::QueuePolicy ends up having one policy Statement per rule, each having an ArnEquals condition allowing the given rule to access/send to the queue.

This results in an OverLimit error when trying to deploy the stack, due to Submitted policy is over max allowed size. Which makes complete sense, given how many statements there end up being.

I've been trying to find a way to "collapse" all of the policy statements into a single one. I did manage to add a policy statement that would cover it, but CDK still adds all the individual statements as well. I can't figure out how to prevent the addition of those policy statements.

Any advice? Thanks in advance!

Environment

  • CDK CLI Version: 1.36.1 (build 4df7dac)
  • Module Version: 1.36.1
  • OS: OSX Catalina
  • Language: Java

Other information

A couple of extra notes:

  1. I noticed in the doc that Queue supposedly has autoCreatePolicy. First of all, this appears to be read-only in Java, since there's only a documented getter, no setter. Secondly, this method isn't even public, it's protected. It looked tantalizingly promising, but inaccessible. I'm temped to subclass Queue and override it, but that feels like a rabbit hole down which I shouldn't be going.

  2. I also noticed IPostProcessor and got excited, thinking I might be able to post-process the stack & strip out the unwanted policy statements. But I don't see anywhere in the Java API where I could tap into this. I assume this is a core CDK concept.

Anyway, the ability to post-process during the synth would be amazing, if there's no other way to achieve what I'm after.

Thanks!

@dcheckoway dcheckoway added the needs-triage This issue or PR still needs to be triaged. label May 1, 2020
@dcheckoway
Copy link
Author

BTW, I did end up subclassing Queue and overriding getAutoCreatePolicy to return false, and that did squelch the creation of the policy. But now I'm struggling to find a way to create a policy (via CDK) that will allow any of these rules to send messsages to the queue.

@dcheckoway
Copy link
Author

dcheckoway commented May 1, 2020

Woohoo! Success!

        // Since we created the queue with autoCreatePolicy=false, we still need to allow the rules to send
        // messages to the queue.  This creates a policy with one statement, one compound condition.
        QueuePolicy.Builder.create(this, "scheduled-task-invocation-queue-policy")
                .queues(Arrays.asList(q))
                .build()
                .getDocument().addStatements(PolicyStatement.Builder.create()
                .resources(Arrays.asList(q.getQueueArn()))
                .effect(Effect.ALLOW)
                .actions(Arrays.asList(
                        "sqs:GetQueueAttributes",
                        "sqs:GetQueueUrl",
                        "sqs:SendMessage"
                ))
                .principals(Arrays.asList(ServicePrincipal.Builder.create("events").build()))
                .conditions(ImmutableMap.of("ArnEquals", ImmutableMap.of("aws:SourceArn",
                        rules.stream().map(Rule::getRuleArn).collect(Collectors.toList()))))
                .build());

@SomayaB SomayaB added @aws-cdk/aws-sqs Related to Amazon Simple Queue Service guidance Question that needs advice or information. labels May 5, 2020
@MrArnoldPalmer
Copy link
Contributor

@dcheckoway this is a good workaround, appreciate the example. Making constructs more aware of limits in policy size seems like a good feature for the future so I'm gonna keep this open for now. grant* methods could track principals/arns assigned to various permissions and merge them together.

Something like this could potentially touch constructs in aws-iam as well.

@MrArnoldPalmer MrArnoldPalmer added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. @aws-cdk/aws-iam Related to AWS Identity and Access Management and removed guidance Question that needs advice or information. needs-triage This issue or PR still needs to be triaged. labels May 6, 2020
@MrArnoldPalmer MrArnoldPalmer removed their assignment Jun 21, 2021
@github-actions
Copy link

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jun 21, 2022
@fermezz
Copy link

fermezz commented Jun 23, 2022

How can I do this in Python? It's crazy that an individual policy is generated for each target –in my case, an EventBridge rule triggers many instances for the same lambda– without any way of preventing it. Is there any way I can stop this from happening?

@github-actions github-actions bot removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jun 24, 2022
@niebloomj
Copy link

The related issue #16303 just closed but I think this is still a pretty big issue. Is no one hitting this?

@thesuavehog
Copy link

I hit this all the time. It's incredibly annoying since I end up just making Queue2 and splitting up the producers to avoid the limit issue. It makes for a MUCH messier setup and doesn't scale well.

Seems like a major issue that IAM also faced and so IAM added the @aws-cdk/aws-iam:minimizePolicies setting ... seems like the same solution could be implemented for all Resource Policies (not just Queue Policy).

@MrArnoldPalmer
Copy link
Contributor

@thesuavehog that's a good callout, since we have policy merging logic for various policies we can look into leveraging that on resource policies. I'll take a look at what is required to get that working.

@yattoni
Copy link
Contributor

yattoni commented Nov 18, 2022

You can override the policy using the escape hatches. Here's what I did after making my queue and attaching everything to it

        const queuePolicy = queue.node.findChild('Policy').node.findChild('Resource') as CfnQueuePolicy;
        queuePolicy.policyDocument = new PolicyDocument({
            statements: [
                new PolicyStatement({
                    actions: ['sqs:SendMessage'],
                    principals: [new ServicePrincipal('events.amazonaws.com')],
                    resources: [queue.queueArn],
                    sid: 'AllowEventBridgeSendMessage'
                }),
            ],
        });
        queuePolicy.queues = [queue.queueUrl];

@comcalvi comcalvi changed the title How to avoid zillions of conditional policy statements? Minify and Merge Resource Policies Jan 26, 2023
@danandreicarp
Copy link

Ran into this problem recently. Upvoting for a fix here.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management @aws-cdk/aws-sqs Related to Amazon Simple Queue Service effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

No branches or pull requests

8 participants