Improve IAM role statements, policies and principals internal handling and resolution #8511
Conversation
There was a problem hiding this comment.
Thank you @zaldanaraul !
Sorry for late response, but I also decided to specify necessary test changes, before posting final review here, and that took me a while
Please see my comments
| functionObject.iamConfig = { | ||
| principals: new Set(), | ||
| policyStatements: [], | ||
| managedPolicies: [], | ||
| }; |
There was a problem hiding this comment.
This ideally should be initialized in mergeIamTemplates.js (so we have this configuration setup upfront, while this plugin is complied as some next item in order afaik)
There was a problem hiding this comment.
Hi @medikoo . I tried this before but it was giving me multiple errors (~63), which is why I opted for including this code in functions/index.js instead. I put the code in mergeIamTemplates.js in my latest commit so you can take a look. Maybe there's something I'm missing?
There was a problem hiding this comment.
Hi @medikoo . I tried this before but it was giving me multiple errors (~63)
What errors specifically? Is this about failing tests, if so in that case, please list all test files - we need to refactor them to runServelress variant.
Technically this upgrade should not require any tests to be written or updated, and just work, but for that we need to have many tests refactored to new approach
There was a problem hiding this comment.
The errors only occur when the code to create the functions[].iamConfig objects are included in mergeIamTemplates.js instead of functions/index.js. All the errors are from index.test.js and they all say this:
TypeError: Cannot read property \'policyStatements\' of undefined
so I think it means the iamConfig object hasn't been created yet. That's why I opted for including this code in functions/index.js instead since I think mergeIamTemplates.js hasn't yet run by the time functions/index.js is executed.
There was a problem hiding this comment.
All the errors are from index.test.js
This means that we need to address #8523. After having that, there won't be such error.
Again, let's not get blocked, or have our implementation influenced by failing (for wrong reason) tests. We have very bad tests in many places, and all that fail should be simply refactored (as proposed in above issue)
|
|
||
| // add all awsProvider.iamConfig.policyStatements and functions[].iamConfig.policyStatements to IamRoleLambdaExecution resource (ensuring no duplicates) | ||
|
|
||
| let policyStatements = this.serverless.service.provider.iamConfig.policyStatements; |
There was a problem hiding this comment.
Let's also add those which are on iamRoleProperties.Policies[0].PolicyDocument.Statement (there can be some added by plugins).
Then before assigning filtered statements, it'll be good to filter out those with empty Resources section (our template is prefilled with such
There was a problem hiding this comment.
I think we don't need to filter statements with empty Resources section since we're adding all statements from iamRoleProperties.Policies[0].PolicyDocument.Statement to the temporary array and then filtering out any duplicates. Take a look at lines 83-91 in latest commit and let me know!
There was a problem hiding this comment.
Will filtering the duplicates, filter statements as that one:
My point was, to not have such statements at all
There was a problem hiding this comment.
Oh I see. In that case, I will add some code to take those statements out completely.
| @@ -2525,6 +2525,94 @@ describe('AwsCompileFunctions #2', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('IamConfig object tests', async () => { | |||
There was a problem hiding this comment.
Concerning tests, as indicated in main issue description, let's not put any effort in updating them in current shape.
Technically we're just refactoring internals, and result CloudFormation template should remain same (at least it should produce very same effect)
To avoid tests breaking on internal changes, we need to refactor affected tests to runServerless variant.
I've outlined on how to do that for two tests files that you're trying to update here Check: #8523 and #8524
Until those are addressed, we need assume that this PR cannot be merged (but it doesn't stop us from finalizing this refactor - it just cannot be properly validated until tests are refactored).
After we refactor all affected tests, the only test changes that may be wanted in scope of this PR is moving some of the tests to other files, or improving the coverage, that's it, but I would decide that once we have all tests refactored, and functionality in this PR ready
|
@zaldanaraul I see there were some changes since last review. What exactly is the status of that? |
|
@zaldanaraul I'm closing this due to lack of activity. Still if you want to continue work on this, just let us know and we will be happy to reopen! |
Addresses: #8396
Hi @medikoo sorry for long delay! So far, I have implemented steps 1, 3, and 4 of your proposed solution on #8396 . Just wanted to get your feedback on the overall logic of what I've done before implementing step 2 and breaking old tests. In particular, just wanted to make sure the tests I wrote were appropriate or see if there are some extra tests I should include.
Some things to note:
functions[].iamConfigobject in lib/plugins/aws/package/compile/functions/index.js rather than inmergeIamTemplatescause I was running into some issues where the iamConfig object for each function had not been created by the timecompileFunction()ran