-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Sync terraform EKS module with the reference architecture #11995
Conversation
b8b2682
to
794754c
Compare
15fe320
to
513f2e2
Compare
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.
Did not fully review all of the settings and resources used to make sure that they match the reference architecture, but I did already find that there is a deviation in the DB set up and wanted to already mention that :)
--> For those thar review this: please make sure to review that the infrastructure created via this terraform matches the infrastructure described in the reference architecture guide exactly.
513f2e2
to
a6c79d3
Compare
@lucasvaltl I have addressed the comments. Sorry on the back and forth on this. I kinda of went easy on the configurable settings considering it can/will be changed. I will keep this in mind and try to do an exact 1:1 config. |
👍 Thank you! And no worries - maybe the intent here was not clear enough from my side. We want this terraform to effectively create exactly what the guide describes :) |
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.
Summary
Looks good overall, I see a few hot spots but nothing major.
- Security group inline rules vs
aws_security_group_rule
instances - the docs say that inline rules and separate rule resources may not play well together. Can we check this out and see if we have any risk here? We have a couple of egress rules addressed to0.0.0.0/0
which deviates from the reference architecture; I'd like to confirm that they're not in use so we're testing the strict policies defined in the reference architecture. - RDS
publicly_accessible
setting - can we drop this setting? - S3 security policies - can we restrict the policy to the object storage and container registry buckets?
As noted inline I have a handful of opportunistic or style comments; if any of them resonate with you then we can address them but can and should skip if the review process starts running long.
Edit: one more quick topic - it seems like the only way to keep track of AWS resources is with the copious use of labels on every resource. Are we using labels to identify associated resources? (If not, give me a yell and I'll spin up a ticket to add that later.)
@@ -0,0 +1,6 @@ | |||
terraform { | |||
backend "s3" { | |||
bucket = "gitpod-tf" |
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.
Question: If a user directly uses this module, will this backend definition conflict with their environment?
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.
This is a good point! I could leave the bucket field empty since I cannot use variables in bucket definition. But I am also thinking, the chances of them having a bucket named gitpod-tf
is unlikely, so it will error out saying no such bucket is present. Not sure what the right route is, I might leave this as is.
61530de
to
4722752
Compare
f5dc572
to
cbc3fbe
Compare
👋 It seems that errors can still creep up during the |
cbc3fbe
to
ac857a9
Compare
Co-authored-by: Adrien Thebo <adrien@lagrange-automation.io>
ac857a9
to
c3e6b78
Compare
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.
Copied from Slack:
I see some issues that may pose some additional complications if users try to use it, such as the declaration of terraform backends within the modules and the presence of terraform.tfvars
files that could be moved to terraform.tfvars.example
. Nothing's a blocker but there are some sharp edges that we can remove.
This also represents a significant amount of work; getting it merged will make everyone's lives easier, and holding this back will block related PRs.
Let's move ahead with this PR and chase down some of these issues in follow-up PRs. Great work! 🍻
/unhold |
Description
This PR adds a single-cluster AWS terraform module, corresponding to our reference architecture for the same cloud provider.
Related Issue(s)
Fixes #11533
How to test
You can follow the README added to this PR to test this terraform module.
Release Notes
Documentation
Werft options: