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.

Sign up for GitHub

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

Jump to bottom

examples: athena terraform #29895

Merged
merged 1 commit into from
Aug 10, 2023
Merged

examples: athena terraform #29895

merged 1 commit into from
Aug 10, 2023

Conversation

tobiaszheller
Copy link
Contributor

@tobiaszheller tobiaszheller commented Aug 2, 2023 •
edited
Loading

This PR adds terrafrom script necessary to bootstrap infra for self-hosted deployments of teleport.

Part of https://github.com/gravitational/teleport.e/issues/894
RFD: https://github.com/gravitational/teleport/blob/master/rfd/0118-scalable-audit-logs.md

@tobiaszheller tobiaszheller mentioned this pull request Aug 3, 2023
Merged
1 task
logand22
logand22 reviewed Aug 4, 2023
View reviewed changes
Copy link
Contributor

@logand22 logand22 left a comment

Choose a reason for hiding this comment

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

This is looking good. I wonder if we should simplify this by removing things like the kms keys and extra configuration of the s3 buckets and instead provide the minimal terraform required to get them up and running and add a note about it not being production ready.

I also recommend running this through a security linter such as tfsec or trivy to see if it returns anything (If you want a complete setup).

@tobiaszheller tobiaszheller force-pushed the tobiaszheller/athena-terraform branch from dd833c6 to de3f74c Compare August 7, 2023 14:01
@tobiaszheller
Copy link
Contributor Author

This is looking good. I wonder if we should simplify this by removing things like the kms keys and extra configuration of the s3 buckets and instead provide the minimal terraform required to get them up and running and add a note about it not being production ready.

I think we should keep it secure by default.

I also recommend running this through a security linter such as tfsec or trivy to see if it returns anything (If you want a complete setup).

Used tfsec and applied fixes, only thing which left is medium Bucket does not have logging enabled

@tobiaszheller tobiaszheller requested a review from logand22 August 7, 2023 14:04
@tobiaszheller tobiaszheller marked this pull request as ready for review August 7, 2023 14:04
@github-actions github-actions bot requested review from lxea and smallinsky August 7, 2023 14:05
@tobiaszheller
Copy link
Contributor Author

@logand22 PTAL

logand22
logand22 approved these changes Aug 7, 2023
View reviewed changes
Copy link
Contributor

@logand22 logand22 left a comment

Choose a reason for hiding this comment

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

Assuming this works and has been tested, it LGTM.

I would say that if you leave object lock and lifecycle configuration in I'd recommend adding comments to them as someone new to this wouldn't know that they aren't required.

examples/athena/athena.tf Outdated
restrict_public_buckets = true
}

resource "aws_s3_bucket_lifecycle_configuration" "transient_storage_lifecycle" {
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't cleanup versioned buckets. It will only mark them with a delete marker.

On the other hand I recommend removing this for simplification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed it and added comment inside bucket that it's recommended

examples/athena/athena.tf Outdated
restrict_public_buckets = true
}

resource "aws_s3_bucket_object_lock_configuration" "long_term_storage" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Without some type of comment I'd say object locking with an arbitrary mode and time frame is more confusing then leaving it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed it and added comment inside bucket that it's recommended

@tobiaszheller tobiaszheller force-pushed the tobiaszheller/athena-terraform branch from de3f74c to 126b0f4 Compare August 8, 2023 14:58
@tobiaszheller
Copy link
Contributor Author

@lxea @smallinsky PTAL

@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from lxea August 10, 2023 08:48
@tobiaszheller tobiaszheller force-pushed the tobiaszheller/athena-terraform branch from 126b0f4 to 5b76186 Compare August 10, 2023 10:18
@tobiaszheller tobiaszheller added this pull request to the merge queue Aug 10, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 10, 2023
@tobiaszheller tobiaszheller added this pull request to the merge queue Aug 10, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 10, 2023
@tobiaszheller tobiaszheller added this pull request to the merge queue Aug 10, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 10, 2023
@tobiaszheller tobiaszheller added this pull request to the merge queue Aug 10, 2023
Merged via the queue into master with commit 597ae4d Aug 10, 2023
@tobiaszheller tobiaszheller deleted the tobiaszheller/athena-terraform branch August 10, 2023 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tigrato tigrato tigrato approved these changes

@smallinsky smallinsky smallinsky approved these changes

@logand22 logand22 logand22 approved these changes

Assignees
No one assigned
Labels
size/md
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tobiaszheller @tigrato @smallinsky @logand22