-
Notifications
You must be signed in to change notification settings - Fork 7
Add read permissions for blacklist file #16
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
Conversation
modules/role/role.tf
Outdated
@@ -1,3 +1,8 @@ | |||
locals { | |||
should_create_s3_policy = var.blacklist_object_arn != null && var.blacklist_object_arn != "" ? true : false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't the ternary operator redundant here?
? true : false
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can set 1: 0
instead of true/false
then you don't need the if in #31,41,51
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another thing is the check for ""
I think we should not check for empty and apply if it is empty without a policy - if user is uaing it - he should provide the arn, otherwise - it fails (and not passing without the blacklist)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eladbash dropped the ""
check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs some clarifications
modules/role/role.tf
Outdated
@@ -1,3 +1,8 @@ | |||
locals { | |||
should_create_s3_policy = var.blacklist_object_arn != null && var.blacklist_object_arn != "" ? true : false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can set 1: 0
instead of true/false
then you don't need the if in #31,41,51
modules/role/role.tf
Outdated
@@ -1,3 +1,8 @@ | |||
locals { | |||
should_create_s3_policy = var.blacklist_object_arn != null && var.blacklist_object_arn != "" ? true : false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another thing is the check for ""
I think we should not check for empty and apply if it is empty without a policy - if user is uaing it - he should provide the arn, otherwise - it fails (and not passing without the blacklist)
Currently supported only in GitHub bot.
When S3_BLACK_LIST_BUCKET_NAME and S3_BLACK_LIST_OBJECT_KEY env vars are specified - we should add a read permission to the blacklist file sitting in S3.
We are allowing GetObject permission for this file's arn specifically.