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

added cloudtrailEnable option to fix #173 #243

Merged
merged 1 commit into from
Oct 28, 2021

Conversation

deven96
Copy link
Contributor

@deven96 deven96 commented Oct 22, 2021

No description provided.

@deven96 deven96 requested a review from a team as a code owner October 22, 2021 23:36
@@ -83,6 +83,7 @@ data "aws_iam_policy_document" "operator_access" {
resources = ["arn:aws:s3:::*${local.domain_name}/*"]
}

<% if eq(index .Params `cloudtrailEnable`) "yes" %>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a space between eq and (, this is causing the validation to fail. Need to improve the error output there for sure..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created commitdev/zero#431 to improve error handling

@@ -93,7 +94,7 @@ data "aws_iam_policy_document" "operator_access" {
effect = "Allow"
actions = ["s3:GetObject", "s3:PutObject"]
resources = ["arn:aws:s3:::${data.terraform_remote_state.shared.outputs.cloudtrail_bucket_id}/*"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more permissive but how about instead of making this whole block conditional we just wildcard it to allow access to all cloudtrail buckets. "arn:was:s3:::*-cloudtrail/*" (invalidates the comment above)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes more sense

Copy link
Contributor

@bmonkman bmonkman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

@bmonkman bmonkman merged commit b1101fd into commitdev:main Oct 28, 2021
pmoranga added a commit to pmoranga/zero-aws-eks-stack that referenced this pull request Jan 20, 2022
pmoranga added a commit to pmoranga/zero-aws-eks-stack that referenced this pull request Jan 21, 2022
bmonkman pushed a commit that referenced this pull request Jan 25, 2022
* Fix related to previous #173 #243

* Fix issue where sourcing elasticsearch-logging.sh failed, do not rely on sh

* Fix related to previous #173 #243: return non-empty
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants