Merge pull request #57 from trussworks/mk-refactor

Refactor module to use IAM policy document instead of JSON templating

.circleci/config.yml

@@ -2,7 +2,7 @@ version: 2
 jobs:
   validate:
     docker:
-      - image: trussworks/circleci-docker-primary:40076395a6e6a349f92caa92c4de614e105fe672
+      - image: trussworks/circleci-docker-primary:4013bb8c2428b3e2755d90a922abb2a6cea37ab4
     steps:
       - checkout
       - restore_cache:

.pre-commit-config.yaml

@@ -23,6 +23,6 @@ repos:
       - id: terraform_fmt
 
   - repo: git://github.com/golangci/golangci-lint
-    rev: v1.23.8
+    rev: v1.24.0
     hooks:
       - id: golangci-lint

LICENSE

@@ -1,6 +1,6 @@
 BSD 3-Clause License
 
-Copyright (c) 2019, TrussWorks, Inc.
+Copyright (c) 2020, TrussWorks, Inc.
 All rights reserved.
 
 Redistribution and use in source and binary forms, with or without

README.md

@@ -15,7 +15,7 @@ Logging from the following services is supported for both cases as well as in AW
 
 ## Terraform Versions
 
-Terraform 0.12. Pin module version to ~> 5.1.0 . Submit pull-requests to master branch.
+Terraform 0.12. Pin module version to ~> 7.0.0 . Submit pull-requests to master branch.
 
 Terraform 0.11. Pin module version to ~> 3.5.0 . Submit pull-requests to terraform011 branch.
 
@@ -55,18 +55,6 @@ module "aws_logs" {
 }
 ```
 
-## Usage for a private bucket with no policies
-
-```hcl
-module "aws_logs" {
-  source         = "trussworks/logs/aws"
-  s3_bucket_name = "my-company-aws-logs"
-  s3_bucket_acl  = "private"
-  region         = "us-west-2"
-  default_allow  = false
-}
-```
-
 ## Usage for a single log bucket storing CloudTrail logs from multiple accounts
 
 ```hcl
@@ -90,16 +78,16 @@ module "aws_logs" {
       default_allow     = false
       allow_alb         = true
       allow_nlb         = true
-      alb_logs_prefixes = formatlist(format("alb/%%s/AWSLogs/%s", data.aws_caller_identity.current.account_id), [
-       "alb-hello-world-prod",
-       "alb-hello-world-staging",
-       "alb-hello-world-experimental",
-      ])
-      nlb_logs_prefixes = formatlist(format("nlb/%%s/AWSLogs/%s", data.aws_caller_identity.current.account_id), [
-       "nlb-hello-world-prod",
-       "nlb-hello-world-staging",
-       "nlb-hello-world-experimental",
-      ])
+      alb_logs_prefixes = [
+       "alb/hello-world-prod",
+       "alb/hello-world-staging",
+       "alb/hello-world-experimental",
+      ]
+      nlb_logs_prefixes = [
+       "nlb/hello-world-prod",
+       "nlb/hello-world-staging",
+       "nlb/hello-world-experimental",
+      ]
     }
 ```
 
@@ -109,13 +97,12 @@ module "aws_logs" {
 | Name | Version |
 |------|---------|
 | aws | n/a |
-| template | n/a |
 
 ## Inputs
 
 | Name | Description | Type | Default | Required |
 |------|-------------|------|---------|:-----:|
-| alb\_logs\_prefixes | S3 key prefixes for ALB logs. | `list(string)` | <pre>[<br>  "alb"<br>]<br></pre> | no |
+| alb\_logs\_prefixes | S3 key prefixes for ALB logs. | `list(string)` | <pre>[<br>  "alb"<br>]</pre> | no |
 | allow\_alb | Allow ALB service to log to bucket. | `bool` | `false` | no |
 | allow\_cloudtrail | Allow Cloudtrail service to log to bucket. | `bool` | `false` | no |
 | allow\_cloudwatch | Allow Cloudwatch service to export logs to bucket. | `bool` | `false` | no |
@@ -133,7 +120,7 @@ module "aws_logs" {
 | elb\_accounts | List of accounts for ELB logs.  By default limits to the current account. | `list(string)` | `[]` | no |
 | elb\_logs\_prefix | S3 prefix for ELB logs. | `string` | `"elb"` | no |
 | force\_destroy | A bool that indicates all objects (including any locked objects) should be deleted from the bucket so the bucket can be destroyed without error. | `bool` | `false` | no |
-| nlb\_logs\_prefixes | S3 key prefixes for NLB logs. | `list(string)` | <pre>[<br>  "nlb"<br>]<br></pre> | no |
+| nlb\_logs\_prefixes | S3 key prefixes for NLB logs. | `list(string)` | <pre>[<br>  "nlb"<br>]</pre> | no |
 | redshift\_logs\_prefix | S3 prefix for RedShift logs. | `string` | `"redshift"` | no |
 | region | Region where the AWS S3 bucket will be created. | `string` | n/a | yes |
 | s3\_bucket\_acl | Set bucket ACL per [AWS S3 Canned ACL](<https://docs.aws.amazon.com/AmazonS3/latest/dev/acl-overview.html#canned-acl>) list. | `string` | `"log-delivery-write"` | no |
@@ -153,6 +140,24 @@ module "aws_logs" {
 
 ## Upgrade Paths
 
+### Upgrading from 6.0.0 to 7.x.x
+
+This release simplifies `nlb_logs_prefixes` and `alb_logs_prefixes` to no longer need to pass in a formatted list and instead can be referenced as
+
+```hcl
+nlb_logs_prefixes = [
+ "nlb/hello-world-prod",
+ "nlb/hello-world-staging",
+ "nlb/hello-world-experimental",
+]
+```
+
+This release defines more restrictive bucket policies for ALB and NLB logs to include the AWS account id to the allowed path. Terraform plans with this version of the module will look something like
+
+```text
+~ Resource  = "arn:aws:s3:::bucket-a-us-west-2/nlb/*" -> "arn:aws:s3:::bucket-a-us-west-2/nlb/AWSLogs/480766629331/*"
+```
+
 ### Upgrading from 5.0.0 to 5.1.x
 
 Version 5.1.0 removed the `nlb_logs_prefix` and `nlb_accounts` variables and now uses one `nlb_logs_prefixes` list as input.  If you had not set the `nlb_logs_prefix` or `nlb_accounts` variables, then the default behavior does not change.  If you had set `nlb_logs_prefix`, then simply pass the original value as a 1 item list to `nlb_logs_prefixes` (while watching that path separators are not duplicated).  For example, `nlb_logs_prefixes = ["logs/nlb"]`.

examples/alb/main.tf

@@ -1,20 +1,26 @@
 module "aws_logs" {
-  source         = "../../"
-  s3_bucket_name = var.test_name
-  region         = var.region
-  allow_alb      = "true"
-  force_destroy  = var.force_destroy
+  source = "../../"
+
+  s3_bucket_name    = var.test_name
+  alb_logs_prefixes = var.alb_logs_prefixes
+  region            = var.region
+  allow_alb         = true
+  default_allow     = false
+
+  force_destroy = var.force_destroy
 }
 
 resource "aws_lb" "test_lb" {
-  name               = var.test_name
+  count = length(var.alb_logs_prefixes)
+
+  name               = "${var.test_name}${count.index}"
   internal           = false
   load_balancer_type = "application"
   subnets            = module.vpc.public_subnets
 
   access_logs {
     bucket  = module.aws_logs.aws_logs_bucket
-    prefix  = "alb"
+    prefix  = element(var.alb_logs_prefixes, count.index)
     enabled = true
   }
 }

examples/alb/variables.tf

@@ -14,3 +14,6 @@ variable "force_destroy" {
   type = bool
 }
 
+variable "alb_logs_prefixes" {
+  type = list(string)
+}

examples/cloudtrail/main.tf

@@ -1,13 +1,20 @@
 module "aws_logs" {
-  source         = "../../"
-  s3_bucket_name = var.test_name
-  region         = var.region
-  force_destroy  = var.force_destroy
+  source = "../../"
+
+  s3_bucket_name         = var.test_name
+  region                 = var.region
+  force_destroy          = var.force_destroy
+  cloudtrail_logs_prefix = var.cloudtrail_logs_prefix
+
+  default_allow    = false
+  allow_cloudtrail = true
 }
 
 module "aws_cloudtrail" {
-  source                    = "trussworks/cloudtrail/aws"
-  version                   = "~> 2"
+  source  = "trussworks/cloudtrail/aws"
+  version = "~> 2"
+
   s3_bucket_name            = module.aws_logs.aws_logs_bucket
   cloudwatch_log_group_name = var.test_name
+  s3_key_prefix             = var.cloudtrail_logs_prefix
 }

examples/cloudtrail/variables.tf

@@ -9,3 +9,7 @@ variable "region" {
 variable "force_destroy" {
   type = bool
 }
+
+variable "cloudtrail_logs_prefix" {
+  type = string
+}

examples/combined/main.tf

@@ -1,8 +1,11 @@
 module "aws_logs" {
-  source         = "../../"
+  source = "../../"
+
   s3_bucket_name = var.test_name
   region         = var.region
-  force_destroy  = var.force_destroy
+  default_allow  = true
+
+  force_destroy = var.force_destroy
 }
 
 resource "aws_lb" "test_alb" {
@@ -19,15 +22,18 @@ resource "aws_lb" "test_alb" {
 }
 
 module "aws_cloudtrail" {
-  source                    = "trussworks/cloudtrail/aws"
-  version                   = "~> 2"
+  source  = "trussworks/cloudtrail/aws"
+  version = "~> 2"
+
   s3_bucket_name            = module.aws_logs.aws_logs_bucket
+  s3_key_prefix             = "cloudtrail"
   cloudwatch_log_group_name = var.test_name
 }
 
 module "config" {
-  source             = "trussworks/config/aws"
-  version            = "~> 2"
+  source  = "trussworks/config/aws"
+  version = "~> 2"
+
   config_name        = var.test_name
   config_logs_bucket = module.aws_logs.aws_logs_bucket
   config_logs_prefix = "config"
@@ -65,19 +71,31 @@ resource "aws_lb" "test_nlb" {
 }
 
 resource "aws_redshift_cluster" "test_redshift" {
-  count               = var.test_redshift ? 1 : 0
-  cluster_identifier  = var.test_name
-  node_type           = "dc2.large"
-  cluster_type        = "single-node"
-  master_username     = "testredshiftuser"
-  master_password     = "TestRedshiftpw123"
-  skip_final_snapshot = "true"
+  count = var.test_redshift ? 1 : 0
+
+  cluster_identifier        = var.test_name
+  node_type                 = "dc2.large"
+  cluster_type              = "single-node"
+  master_username           = "testredshiftuser"
+  master_password           = "TestRedshiftpw123"
+  skip_final_snapshot       = true
+  cluster_subnet_group_name = var.test_name
+  publicly_accessible       = false
 
   logging {
     bucket_name   = module.aws_logs.aws_logs_bucket
     s3_key_prefix = "redshift"
     enable        = true
   }
+
+  depends_on = [aws_redshift_subnet_group.test_redshift]
+}
+
+resource "aws_redshift_subnet_group" "test_redshift" {
+  count = var.test_redshift ? 1 : 0
+
+  name       = var.test_name
+  subnet_ids = module.vpc.private_subnets
 }
 
 resource "aws_s3_bucket" "log_source_bucket" {
@@ -92,10 +110,12 @@ resource "aws_s3_bucket" "log_source_bucket" {
 }
 
 module "vpc" {
-  source         = "terraform-aws-modules/vpc/aws"
-  version        = "~> 2"
-  name           = var.test_name
-  cidr           = "10.0.0.0/16"
-  azs            = var.vpc_azs
-  public_subnets = ["10.0.101.0/24", "10.0.102.0/24", "10.0.103.0/24"]
+  source  = "terraform-aws-modules/vpc/aws"
+  version = "~> 2"
+
+  name            = var.test_name
+  cidr            = "10.0.0.0/16"
+  azs             = var.vpc_azs
+  public_subnets  = ["10.0.101.0/24", "10.0.102.0/24", "10.0.103.0/24"]
+  private_subnets = ["10.0.1.0/24", "10.0.2.0/24", "10.0.3.0/24"]
 }

examples/config/main.tf

@@ -1,16 +1,20 @@
 module "aws_logs" {
-  source             = "../../"
+  source = "../../"
+
   s3_bucket_name     = var.test_name
   region             = var.region
-  allow_config       = "true"
-  config_logs_prefix = "config"
-  force_destroy      = var.force_destroy
+  allow_config       = true
+  default_allow      = false
+  config_logs_prefix = var.config_logs_prefix
+
+  force_destroy = var.force_destroy
 }
 
 module "config" {
-  source             = "trussworks/config/aws"
-  version            = "~> 2"
+  source  = "trussworks/config/aws"
+  version = "~> 2"
+
   config_name        = var.test_name
   config_logs_bucket = module.aws_logs.aws_logs_bucket
-  config_logs_prefix = "config"
+  config_logs_prefix = var.config_logs_prefix
 }

examples/config/variables.tf

@@ -10,3 +10,6 @@ variable "force_destroy" {
   type = bool
 }
 
+variable "config_logs_prefix" {
+  type = string
+}

examples/elb/main.tf

@@ -1,9 +1,13 @@
 module "aws_logs" {
-  source         = "../../"
-  s3_bucket_name = var.test_name
-  region         = var.region
-  allow_elb      = "true"
-  force_destroy  = var.force_destroy
+  source = "../../"
+
+  s3_bucket_name  = var.test_name
+  elb_logs_prefix = var.elb_logs_prefix
+  region          = var.region
+  allow_elb       = true
+  default_allow   = false
+
+  force_destroy = var.force_destroy
 }
 
 resource "aws_elb" "test_elb" {
@@ -12,7 +16,7 @@ resource "aws_elb" "test_elb" {
 
   access_logs {
     bucket        = module.aws_logs.aws_logs_bucket
-    bucket_prefix = "elb"
+    bucket_prefix = var.elb_logs_prefix
     enabled       = true
   }
 

examples/elb/variables.tf

@@ -14,3 +14,6 @@ variable "force_destroy" {
   type = bool
 }
 
+variable "elb_logs_prefix" {
+  type = string
+}