Add ability to set a repository permission policy#100
Add ability to set a repository permission policy#100
Conversation
|
@rclark @emilymdubois whenever you get a chance, one of you mind reviewing my change? |
|
|
||
| function set_policy() { | ||
| local region=$1 | ||
| RepositoryPermissionPolicy=$(printenv | grep RepositoryPermissionPolicy | sed 's/.*=//') |
There was a problem hiding this comment.
Why is there this complex pipeline here rather than just relying on the -n condition on the next line?
There was a problem hiding this comment.
Ahh this was probably from where I had originally tried stuffing this in the credentials() function instead. I'll switch it out.
| function set_policy() { | ||
| local region=$1 | ||
| RepositoryPermissionPolicy=$(printenv | grep RepositoryPermissionPolicy | sed 's/.*=//') | ||
| if [ -n "$RepositoryPermissionPolicy" ] |
There was a problem hiding this comment.
Bonus ⭐️ s will be rewarded for consistent { } variable bracketing!
| }, | ||
| RepositoryPermissionPolicy: { | ||
| Type: 'String', | ||
| Description: 'An IAM policy granting access for other AWS accounts to access this repository', |
There was a problem hiding this comment.
Really this policy could be granting anything on the repositories that conex creates. Its just your particular use-case for cross-account access, right?
There was a problem hiding this comment.
Good point, I'll tweak the wording appropriately
|
Made a few changes to address things brought up in the code review. I also added a quick note to the readme for context, let me know if you still see anything else! |
| local region=$1 | ||
| RepositoryPermissionPolicy=$(printenv | grep RepositoryPermissionPolicy | sed 's/.*=//') | ||
| if [ -n "$RepositoryPermissionPolicy" ] | ||
| RepositoryPermissionPolicy=$(printenv RepositoryPermissionPolicy) |
There was a problem hiding this comment.
Even this line is excess, isn't it?
There was a problem hiding this comment.
If I just include the if [ -n "${RepositoryPermissionPolicy}" ] by itself, the script fails to load due to RepositoryPermissionPolicy being an unbound variable (and set -u being set). Perhaps there's another way around that beyond the realm of my shell scripting knowledge.
There was a problem hiding this comment.
O_o that doesn't really make sense to me. Why are these two lines of code different than each other?
RepositoryPermissionPolicy=$(printenv RepositoryPermissionPolicy)
RepositoryPermissionPolicy=${RepositoryPermissionPolicy}
If RepositoryPermissionPolicy is set in the env, then why would set -u be a problem?
There was a problem hiding this comment.
If RepositoryPermissionPolicy is set in the env, then why would set -u be a problem?
Had to chew on this for probably longer than I should've, as it seemed like a totally valid point. And it was. Issue was actually with my test script where I was unsetting this variable to test the "no-policy" case. When I switched to testing a realistic no-policy case (RepositoryPermissionPolicy=""), just having the -n test worked fine!
Sorry it took so long to get to this. 🙃
2 participants
This change enables ecs-conex to set a Permissions Policy on all repositories watched by this instance of conex. ECR Permissions policies enable you to share an ECR repository in one account with another AWS account.
➡️ Documentation on ECR Repository Policies
How this feature was implemented
I've implemented this feature where including an IAM policy document as the
RepositoryPermissionPolicystack parameter will set it as an environment variable on the watchbot worker. When a worker is processing a job, after ensuring the ECR repo exists in a given region, the policy for the repository will be set as well (this is an idempotent operation if the policy does not change).Testing
I added what I think are appropriate unit tests to the
test/utils.test.shfile. I also tested creating an ECR repository using this stack both when theRepositoryPermissionPolicyparameter was blank and when it was set to a valid policy to share with another account. In both cases, the expected result happened (the repository was published on ECR, in the former case with no permissions set, in the latter with the specified policy set).