aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.68k stars 3.93k forks source link

Minify and Merge Resource Policies #7732

Open dcheckoway opened 4 years ago

dcheckoway commented 4 years ago

:question: General Issue

The Question

I'm using CDK to create a stack with:

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

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 commented 4 years ago

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 commented 4 years ago

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());
MrArnoldPalmer commented 4 years ago

@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.

github-actions[bot] commented 2 years ago

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.

fermezz commented 2 years ago

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?

niebloomj commented 2 years ago

The related issue https://github.com/aws/aws-cdk/issues/16303 just closed but I think this is still a pretty big issue. Is no one hitting this?

thesuavehog commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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];
danandreicarp commented 11 months ago

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