-
Notifications
You must be signed in to change notification settings - Fork 5
Init #1
Conversation
Terraform module name does not follow registry imposed naming convention. Also, I think the better approach may be to use this with gzip turned on. Amazon Linux supports cloud init https://www.terraform.io/docs/providers/template/d/cloudinit_config.html |
main.tf
Outdated
namespace = "${var.namespace}" | ||
name = "${var.name}" | ||
stage = "${var.stage}" | ||
attributes = ["s3", "stored"] |
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.
s/stored/user-data/
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.
drop s3
since I don't think we need that in the bucket name (it's implied)
Aha, I see. I thought it would be used for the bucket name, but it's used for a policy. We can keep it.
main.tf
Outdated
resource "aws_s3_bucket_object" "default" { | ||
bucket = "${var.bucket}" | ||
key = "${var.path}/user_data.sh" | ||
content = "${join("\n", var.user_data)}" |
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.
Use a local
value for the join
output?
user_data.sh
Outdated
|
||
aws s3 cp s3://${s3_path} /tmp/user_data.sh | ||
|
||
eval "$(cat /tmp/user_data.sh)" |
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.
Why eval
as opposed to just running it? .... eval
limits user data to bash
.
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.
because user data does not contains
'#/bin/bash' to specifiy shell
main.tf
Outdated
effect = "Allow" | ||
|
||
resources = [ | ||
"arn:aws:s3:::*", |
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 should not be necessary and is too permissive.
main.tf
Outdated
} | ||
|
||
statement { | ||
actions = [ "s3:*" ] |
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.
The instances should only need to be able to retrieve the object, not manipulate it.
main.tf
Outdated
template = "${file("${path.module}/user_data.sh")}" | ||
|
||
vars { | ||
s3_path = "${aws_s3_bucket_object.default.bucket}${aws_s3_bucket_object.default.key}" |
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.
s/s3_path/s3_user_data_uri/
and add s3://
to the uri
user_data.sh
Outdated
# Install AWS Client | ||
pip install --upgrade awscli | ||
|
||
aws s3 cp s3://${s3_path} /tmp/user_data.sh |
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.
Drop the s3://
and use s3_user_data_uri
user_data.sh
Outdated
|
||
############## | ||
# Install deps | ||
############## |
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 preamble is disproportionately large to the other comments and there is no "main" section. I think you can drop the #######
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.
Rename "stored" to "backend"
What
Why