terraform-aws-ecr
terraform-aws-ecr copied to clipboard
cloudposse
add optional policy allowing push access
what
- adds the ability to give push-only access to the repository
why
- full access was more than we wanted in our situation (CI pushing images to the repo) so we added a
principals_push_accessto give push-only access.
references
- policy is based on this AWS doc
Thanks for the contribution. I'm just trying to get a second set of eyes on this to make sure it's what we want in this module.
Either we should move lambda condition to independent or add in push policy as welll
I prefer it should be independent
Use distinct function for override policy document
I'm not sure I follow. Where do you think there should be a distinct function?
Use distinct function for override policy document
I'm not sure I follow. Where do you think there should be a distinct function?
Main.tf line no 250
Use distinct function for override policy document
I'm not sure I follow. Where do you think there should be a distinct function?
Main.tf line no 250
Maybe I'm missing something but I think that would require restructuring the module so that the all (push, readonly, full)actions are combined/distinct before they get used in a policy document. That would also mean losing the sid on the different policy blocks. That would make it harder to understand the policy when looking at it from the AWS side.
Or I'm looking at the wrong thing...
i see the bridgecrew code analysis is failing but i'm not able to see the output of the job
@kpankonen currently lambda access is granted using dynamic statements in each group and it may be problematic if someone want to create and ECR with just lambda access as resource policy. then policy might be missing because we are checking if the system has push or pull or full variables then only create not based on lambda too. We should have independent staement for lambda access policy.
@aknysh @nitrocode is there anything else i can do to help get this merged?
@kpankonen currently lambda access is granted using dynamic statements in each group and it may be problematic if someone want to create and ECR with just lambda access as resource policy. then policy might be missing because we are checking if the system has push or pull or full variables then only create not based on lambda too. We should have independent statement for lambda access policy.
@kpankonen @ngoyal16 I agree with this. Lambda permissions are already in the 2 existing policies, and it should be separated into its own policy. We'll not do it in this PR, but @kpankonen please update your new push-access policy to include the lambda principals as @ngoyal16 mentioned to make all 3 policies symmetrical and allow Lambdas to push. Otherwise, the PR looks good to me, thanks
@kpankonen Apologies, but can you fix the out of date README? Do the following:
make init
make github/init
make readme
and commit the changes (if any)
@kpankonen Apologies, but can you fix the out of date README? Do the following:
make init make github/init make readmeand commit the changes (if any)
done!
Argh @kpankonen -- we're doing you wrong here with multiple of us trying to move it forward, but then dropping the ball (we get drowned in a sea of GH notification emails as part of being on the maintainer team, so that is our excuse 😅 ).
Can you please address the conflicts or try rebasing again? Once this is ready to go, ping me here or better yet in the SweetOps slack and I will be sure to get this over the line for you. Thanks for the work and patience!
@aknysh I believe we need your approval to move this forward.
FYI, I am going to do a fast follow on this PR today or tomorrow to break out the principals_lambda policies into their own policy doc so they don't need to be included with each of these full vs read vs write docs.