Facilitate specifying non-default VPC (shamelessly ripped off from hashicorp/terraform-aws-consul) #26
Conversation
There was a problem hiding this comment.
Thanks for the PR! The code changes look good, minus one sanity check mentioned in the review.
Also, just to be extra, extra clear: the examples that are currently in this repo are NOT production ready and should NOT be deployed to prod as-is (there are comments all over the code & docs indicating this). They are all examples meant to show how to use the code and get started quickly.
So while I 100% agree that using the default VPC is not a good practice for production, that's not what the current examples are for. I'd gladly accept a PR that adds a more production-ready example (e.g., with locked down security groups, custom VPC, private subnets, etc), but I just want to be crystal clear that this change alone does NOT make the existing examples production ready!
| @@ -123,9 +123,10 @@ data "template_file" "user_data_consul" { | |||
| # --------------------------------------------------------------------------------------------------------------------- | |||
|
|
|||
| data "aws_vpc" "default" { | |||
| default = true | |||
| default = "${var.vpc_id == "" ? true : false}" | |||
| id = "${var.vpc_id}" | |||
There was a problem hiding this comment.
Hm, I'm worried what happens if id is set to an empty string. Does Terraform ignore it, or does it complain? Unfortunately, Terraform is inconsistent with how it treats empty vs omitted params, so I'm not sure if this will cause problems for people trying to use the default VPC. Did you try it that way?
There was a problem hiding this comment.
No, I didn't try it that way - good catch. I'd have to create a default VPC, which I'm not sure is a reversible process in a single AWS account.
There was a problem hiding this comment.
There was a problem hiding this comment.
Hmm. I'm not comfortable changing my present employer's setup to create and delete a default VPC, since it seems to indicate a bunch of other things are touched by that process (subnets, security groups, etc.), and I don't know the implications of that (or the effect on other Terraform users). Is there another way to test this, perhaps with a free-tier?
There was a problem hiding this comment.
Sure, create a brand new AWS account with the free tier and it'll have a default VPC built-in.
|
Re: "examples not production-ready", I get that. However, the main page for this module (as listed in the TF Registry, which is supposed to be production-ready code), refers to the /examples/ directories for working code. Then, in this example's README, it has the NOTE which, to my reading, completely negates what the example code is actually supposed to do (create a private Vault, without public EIPs) and its own description a few paragraphs above it. So, I'm a little confused. |
The code in the |
|
Got it, and the version, 0.0.8, also indicates it's still for development use. Just FYI, if you naively read the docs as presented on the Registry, it doesn't indicate that the root module is an "example". A blurb in the README.md might help clarify. |
Agreed. I believe the HashiCorp folks have work planned to make the separation between "examples" and prod-ready modules a bit clearer. /cc @KFishner |
…functionality, and enable by variable.
…licy_for_AWS_Auth_method Create IAM Instance Profile Role policy to allow Vault AWS Auth full functionality, and enable by variable.
…_EC2_IAM_Auth variable.
…licy_for_AWS_Auth_method Remove duplicate vpc_id variable; set default to false for new enable_EC2_IAM_Auth variable.
…licy_for_AWS_Auth_method Expose more variables from underlying modules.
|
|
Please let me know if you have troubles. I tested this with my local infrastructure (I actually needed it to get past not having a default VPC, which is "AWS Best Practice", at least today. ;) ).