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

IAM Diff Thrash with Lambda Permissions #1495

Closed
moofish32 opened this issue Jan 8, 2019 · 1 comment · Fixed by #1542
Closed

IAM Diff Thrash with Lambda Permissions #1495

moofish32 opened this issue Jan 8, 2019 · 1 comment · Fixed by #1542
Assignees
Labels
bug This issue is a bug. package/tools Related to AWS CDK Tools or CLI

Comments

@moofish32
Copy link
Contributor

Enhanced IAM diff generates the following when I'm replacing a lambda function (changing function name).

screen shot 2019-01-08 at 1 40 00 am

The permission is new, but should we capture that source account? It leads a user to think the source account is lost, but it's added back by default.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 8, 2019

Good call, thanks for the report.

@rix0rrr rix0rrr added bug This issue is a bug. package/tools Related to AWS CDK Tools or CLI labels Jan 8, 2019
@rix0rrr rix0rrr self-assigned this Jan 14, 2019
rix0rrr added a commit that referenced this issue Jan 14, 2019
We used to modify the new template in place, causing changes to
"logicalIDs" of replaced resources, which would trigger downstream
changes and potential downstream replacements, etc.

This would lead to the changed logical ID popping up in the IAM
diff which was not desirable.

In this change, we do the same processing but we throw away the
template after all changes have been propagated, and only copy
the resultant property STATUSES onto the diff object that gets
returned to the user. This leads to the same displayed result
without the changes to the template actually propagating.

In the course of this modification, the diff classes have been changed
to also have objects in places of resources and properties that
didn't actually change (so that they could be modified in-place),
and diff objects have a boolean telling whether they are actual
changes or not.

Fixes #1495.
rix0rrr added a commit that referenced this issue Jan 16, 2019
Contains the following changes:

- Print table to STDOUT instead of STDERR. This ensures the diff output and the "confirm (y/n)" prompt that follows it don't interleave on the terminal, degrading readability.
- Control table width to not exceed the terminal width, so that it doesn't wrap and lead to a mess of lines.
- Switch back to the new version of the `table` library (instead of `cli-table`) which now has in-cell newline support, to get rid of the rendering bugs in `cli-table` that would occasionally eat newlines.
- Render resource `replace` impact as red instead of yellow, to indicate that data loss will be happening here (fixes #1458).
- Change replacement diffing in order to not trigger IAM changes dialog (fixes #1495).
- Print a message instead of an empty table if 'cdk context' has no information in it (fixes #1549).

Explanation: 

We used to modify the new target template in place, causing changes to
the logical ids of replaced resources, which would trigger downstream
changes, which would further trigger potential downstream replacements, etc. 

This would properly calculate the set of impacted resources, but would also lead to the modified logical ID popping up in the IAM diff, which was not desirable.

In this change, we do the same processing but we throw away the
template after all changes have been propagated, and only copy
the resultant property *statuses* onto the diff object that gets
returned to the user. This leads to the same displayed result
without the changes to the template actually propagating.

In the course of this modification, the diff classes have been changed
to also have objects in places of resources and properties that
didn't actually change (so that they could be modified in-place),
and diff objects have a boolean telling whether they are actual
changes or not.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. package/tools Related to AWS CDK Tools or CLI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants