fix(iam): IAM Policies are too large to deploy #19114
Conversation
The policies we generate sometimes have a lot of duplication between statements. This duplication can lead to the policy going over the size limit an IAM policy (either 2k, 6k or 10k bytes, depending on the resource type). This change combines multiple statements together, as long as it doesn't change the meaning of the final policy. Because doing so for all existing stacks will probably provoke minor heart attacks in operators everywhere, the new behavior is gated behind a feature flag. It can be retroactively switched on by people currently being bit by the size issues: ``` @aws-cdk/aws-iam:minimizePolicies ``` Fixes #18774, fixes #16350, fixes #18457.
|
|
github-actions
bot
added
the
@aws-cdk/aws-iam
|
Still a draft as this will need a lot of snapshot updates and still needs unit tests for the new functionality. |
rix0rrr
added
the
pr/requires-two-approvers
| @@ -92,6 +92,7 @@ | |||
| }, | |||
| "dependencies": { | |||
| "@aws-cdk/core": "0.0.0", | |||
| "@aws-cdk/cx-api": "0.0.0", | |||
There was a problem hiding this comment.
Should this also be added to peerDependencies?
There was a problem hiding this comment.
No, it doesn't mean anything to do that.
| * | ||
| * @default false, unless the feature flag `@aws-cdk/aws-iam:minimizePolicies` is set | ||
| */ | ||
| readonly minimize?: boolean; |
There was a problem hiding this comment.
Do we really need this property? Is the feature flag not enough?
I would get rid of it for now. We can always come back later, and add it.
There was a problem hiding this comment.
I like this better on principle. The boolean toggles the feature, and the default value for the boolean is taken from the flag for cases where you don't control the Policy initialization.
Co-authored-by: Adam Ruka <adamruka@amazon.com>
|
In fact I can prove using the Alloy model that there is even more optimization to be had: It is safe to merge Statements on
|
| /** | ||
| * Merge as many statements as possible to shrink the total policy doc, modifying the input array in place | ||
| * | ||
| * If there are multiple merges to be made (which may involve the same |
There was a problem hiding this comment.
From the description in PolicyDocumentProps, it sounds like the merge is associative, commutative and idempotent (but I may be missing something). If that's the case, the order shouldn't matter. And if it's not the case, can we make it so?
There was a problem hiding this comment.
That's a good question. I can ask the Alloy model whether that is true as well.
There was a problem hiding this comment.
It is! Obvious in hindsight, as always 😆. Thanks for pointing this out. Combined with doing subset analysis (instead of == analysis), I think this is now much simpler and more optimal.
Had to move some code around for statement normalization.
There was a problem hiding this comment.
Well nevermind. That subset-based merging I said we could do is in fact NOT safe, and leads to the exact same error as the one we had last time around.
There was a problem hiding this comment.
But merging is still associative.
There was a problem hiding this comment.
What's the counterexample for commutativity? My intuition is that we can define a merge operation that forms a semilattice, which induces a partial order (if not the subset relation you tried, maybe some variation of it). Then, navigate the DAG and keep only the statements that are "leaves" in the graph (i.e., not a predecessor of any other statement).
packages/@aws-cdk/aws-codebuild/test/integ.project-secondary-sources-artifacts.expected.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
LGTM! I could see some value in adding unit tests for the makeComparable and renderComparable functions since they have a lot of logic stuffed in them now, but I get that it's just an implementation detail, so I'll leave the call up to you. 🙂
Disclaimer: I did not check the integration test snapshots. There are so many of them... 😱
| if (Array.isArray(x) || typeof x === 'string') { | ||
| x = { AWS: x }; | ||
| } |
There was a problem hiding this comment.
Why are we performing normalization here / can we normalize the statements in this way before they are passed to mergeStatements? I feel like normalization and statement merging should be logically separate steps, to the extent that it's possible anyways.
There was a problem hiding this comment.
This is not where we merge. This is where we "lift" the IAM statement into a representation that is amenable to merging. Is this not enough separation?
rix0rrr
removed
the
pr/requires-two-approvers
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
The policies we generate sometimes have a lot of duplication between statements. This duplication can lead to the policy going over the size limit an IAM policy (either 2k, 6k or 10k bytes, depending on the resource type). This change combines multiple statements together, as long as it doesn't change the meaning of the final policy. Because doing so for all existing stacks will probably provoke minor heart attacks in operators everywhere, the new behavior is gated behind a feature flag. It can be retroactively switched on by people currently being bit by the size issues: ``` @aws-cdk/aws-iam:minimizePolicies ``` We will merge 2 statements if their effects are the same, and they are otherwise exactly the same apart from their `Action`, `Resource` or `Principal` declarations. We will not merge `NotXxx` statements, because doing so will change the meaning of the statement (`not A or not B ≠ not (A or B)`). There may be multiple possible merges that apply and we are not guaranteed to find the smallest merging, nor do we take effort to find all possible merges and do simplifications like `*`-subsumption. This is a starting point that should help out in the common case. Fixes #18774, fixes #16350, fixes #18457, fixes #18564, fixes #19276. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
The policies we generate sometimes have a lot of duplication between
statements. This duplication can lead to the policy going over the size
limit an IAM policy (either 2k, 6k or 10k bytes, depending on the resource
type).
This change combines multiple statements together, as long as it
doesn't change the meaning of the final policy.
Because doing so for all existing stacks will probably provoke minor
heart attacks in operators everywhere, the new behavior is gated
behind a feature flag. It can be retroactively switched on by
people currently being bit by the size issues:
We will merge 2 statements if their effects are the same, and they are otherwise exactly the same apart from their
Action,ResourceorPrincipaldeclarations. We will not mergeNotXxxstatements, because doing so will change the meaning of the statement (not A or not B ≠ not (A or B)). There may be multiple possible merges that apply and we are not guaranteed to find the smallest merging, nor do we take effort to find all possible merges and do simplifications like*-subsumption. This is a starting point that should help out in the common case.Fixes #18774, fixes #16350, fixes #18457, fixes #18564, fixes #19276.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license