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

Implement Cloud Trails #1

Merged
merged 11 commits into from
Aug 11, 2017
Merged

Implement Cloud Trails #1

merged 11 commits into from
Aug 11, 2017

Conversation

s2504s
Copy link
Contributor

@s2504s s2504s commented Aug 7, 2017

What

  • Implement a TF module to enable the AWS CloudTrail service and ship logs to an S3 bucket

Why

  • Provide a way to easily enable AWS API audit logs in a consistent fashion

@s2504s s2504s self-assigned this Aug 7, 2017
@s2504s s2504s requested review from osterman and const-bon August 7, 2017 14:57
main.tf Outdated
@@ -0,0 +1,51 @@
module "tf_label" {
source = "../tf_label"
Copy link
Member

Choose a reason for hiding this comment

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

Use git URI syntax

main.tf Outdated
name = "${var.name}"
}

resource "aws_cloudtrail" "ct" {
Copy link
Member

Choose a reason for hiding this comment

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

lets just call it default

main.tf Outdated
include_global_service_events = "${var.include_global_service_events}"
}

resource "aws_s3_bucket" "cloud_trail" {
Copy link
Member

Choose a reason for hiding this comment

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

Let's just call it default since there is only one bucket used.

variables.tf Outdated
}

variable "name" {
default = ""
Copy link
Member

Choose a reason for hiding this comment

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

Default the name to cloudtrails

README.md Outdated
## Usage
```
module "ct" {
Copy link
Member

Choose a reason for hiding this comment

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

call it cloudtrails. normalize the spacing the way that terraform fmt does it.

README.md Outdated
```
module "ct" {
source = "github.com/cloudposse/tf_cloudtrail"
Copy link
Member

Choose a reason for hiding this comment

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

prefer you use explicit notation.

e.g.

source    = "git::https://github.com/cloudposse/tf_label.git?ref=master"

README.md Outdated
#### Optional

* enable_logging: Enable logging, set to 'false' to Pause logging (default = true)
Copy link
Member

Choose a reason for hiding this comment

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

Use markdown formatting for code.
E.g. enable_logging

@comeanother please help format explanation.

Copy link
Member

@osterman osterman left a comment

Choose a reason for hiding this comment

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

See inline comments

@osterman osterman changed the title Created module tf_cloudtrail Implement Cloud Trails Aug 7, 2017
@osterman
Copy link
Member

osterman commented Aug 7, 2017

  • Fix LICENSE copyright.

README.md Outdated
#### Optional

* `enable_logging`: Enable logging, set to 'false' to Pause logging (default = true)
Copy link
Member

Choose a reason for hiding this comment

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

false is also a keyword, so it should be markdown too. Same with default = true

README.md Outdated

#### Optional

Copy link
Member

Choose a reason for hiding this comment

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

Please "borrow" the same language used on the terraform docs. See: https://www.terraform.io/docs/providers/aws/r/cloudtrail.html

Copy link
Member

@osterman osterman left a comment

Choose a reason for hiding this comment

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

Use TF policy document object

main.tf Outdated
bucket = "${module.tf_label.id}"
force_destroy = false

policy = <<POLICY
Copy link
Member

Choose a reason for hiding this comment

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

Don't use HEREDOC when a better resource type exists. https://www.terraform.io/docs/providers/aws/d/iam_policy_document.html

main.tf Outdated
include_global_service_events = "${var.include_global_service_events}"
}

data "aws_iam_policy_document" "cloudtrail" {
Copy link
Member

Choose a reason for hiding this comment

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

call it default since there's only one.

main.tf Outdated
@@ -0,0 +1,67 @@
module "tf_label" {
source = "git::https://github.com/cloudposse/tf_label.git?ref=master"
Copy link
Member

Choose a reason for hiding this comment

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

Let's pin to 0.1.0

outputs.tf Outdated
@@ -0,0 +1,3 @@
output "s3_trail_bucket_name" {
Copy link
Member

Choose a reason for hiding this comment

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

Let's just call it bucket_name since there is no intention to have more than one bucket.

README.md Outdated
Setup and manage CloudTrail

### Argument Reference

Choose a reason for hiding this comment

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

Use second-level headers
## instead of ###

@const-bon const-bon merged commit 43fcba8 into master Aug 11, 2017
@const-bon const-bon deleted the init branch August 11, 2017 14:04
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.

3 participants