[breaking] Fix aws-redis-node security groups (#149)

[breaking] Fix aws-redis-node security groupsaws-redis-node (and its predecessor in shared-infra, redis-node) had a bug (or at least a naming bug), where the variable named "ingress_security_groups" which ostensibly controlled which security groups were allowed to access the cache, instead assigned the security group that were assigned to Elasticache. In all cases in CZI repos so far, this was set to be the security group assigned to the worker nodes, which happened to allow access to all traffic.

The PR makes this module match the description of ingress_security_group by introducing a new security group in between, assigning the cache the new security group and allowing ingress into that security group from the input security groups, only to the port Redis is listening on.

This PR is breaking because we now need the vpc_id as a new input to be able to create the new intermediate security group. It is also breaking (although not used in this way anywhere in CZI's code base) since it now requires service to be provided, and does not provide a default.

Author: mbarrien

aws-redis-node/README.md

@@ -8,19 +8,20 @@ parameters.
 
 | Name | Description | Type | Default | Required |
 |------|-------------|:----:|:-----:|:-----:|
-| apply\_immediately | Whether changes should be applied immediately or during the next maintenance window. | string | `"true"` | no |
-| availability\_zone | Availability zone in which this instance should run. | string | n/a | yes |
-| engine\_version | The version of Redis to run. See [supported versions](https://docs.aws.amazon.com/AmazonElastiCache/latest/red-ug/supported-engine-versions.html) | string | `"4.0.10"` | no |
+| apply\_immediately | Whether changes should be applied immediately or during the next maintenance window. | bool | `true` | no |
+| availability\_zone | Availability zone in which this instance should run. | string | `null` | no |
+| engine\_version | The version of Redis to run. See [supported versions](https://docs.aws.amazon.com/AmazonElastiCache/latest/red-ug/supported-engine-versions.html) | string | `"5.0.5"` | no |
 | env | Env for tagging and naming. See [doc](../README.md#consistent-tagging). | string | n/a | yes |
 | ingress\_security\_group\_ids | Source security groups which should be able to contact this instance. | list | n/a | yes |
 | instance\_type | The type of instance to run. See [supported node types](https://docs.aws.amazon.com/AmazonElastiCache/latest/red-ug/CacheNodes.SupportedTypes.html) | string | `"cache.m4.large"` | no |
 | owner | Owner for tagging and naming. See [doc](../README.md#consistent-tagging). | string | n/a | yes |
-| parameter\_group\_name |  | string | `"default.redis3.2"` | no |
-| port |  | string | `"6379"` | no |
+| parameter\_group\_name |  | string | `"default.redis5.0"` | no |
+| port | Port to host Redis on. | number | `6379` | no |
 | project | Project for tagging and naming. See [doc](../README.md#consistent-tagging) | string | n/a | yes |
 | resource\_name | If not set, name will be [var.project]-[var.env]-[var.name]. | string | `""` | no |
-| service | Service for tagging and naming. See [doc](../README.md#consistent-tagging) | string | `"redis"` | no |
+| service | Service for tagging and naming. See [doc](../README.md#consistent-tagging) | string | n/a | yes |
 | subnets | List of subnets to which this EC instance should be attached. They should probably be private. | list | n/a | yes |
+| vpc\_id | VPC where the cache will be deployed. | string | n/a | yes |
 
 ## Outputs
 

aws-redis-node/main.tf

@@ -11,6 +11,27 @@ locals {
   }
 }
 
+module "sg" {
+  source      = "terraform-aws-modules/security-group/aws"
+  version     = "3.1.0"
+  name        = local.name
+  description = "Allow traffic to Redis."
+  vpc_id      = var.vpc_id
+  tags        = local.tags
+
+  ingress_with_source_security_group_id = [
+    for sg in var.ingress_security_group_ids : {
+      from_port                = var.port
+      to_port                  = var.port
+      protocol                 = "tcp"
+      description              = "Redis port"
+      source_security_group_id = sg
+    }
+  ]
+
+  egress_rules = ["all-all"]
+}
+
 resource "aws_elasticache_subnet_group" "default" {
   name       = "${var.resource_name != "" ? var.resource_name : local.name}"
   subnet_ids = "${var.subnets}"
@@ -25,7 +46,7 @@ resource "aws_elasticache_cluster" "default" {
   num_cache_nodes      = 1
   parameter_group_name = "${var.parameter_group_name}"
   subnet_group_name    = "${aws_elasticache_subnet_group.default.name}"
-  security_group_ids   = "${var.ingress_security_group_ids}"
+  security_group_ids   = [module.sg.this_security_group_id]
   apply_immediately    = "${var.apply_immediately}"
   availability_zone    = "${var.availability_zone}"
   tags                 = "${local.tags}"

aws-redis-node/module_test.go

@@ -36,6 +36,7 @@ func TestAWSRedisNode(t *testing.T) {
 			"availability_zone":          az,
 			"subnets":                    privateSubnets,
 			"ingress_security_group_ids": []string{sg},
+			"vpc_id":                     vpc,
 		},
 	)
 

aws-redis-node/variables.tf

@@ -1,63 +1,66 @@
 variable "project" {
-  type        = "string"
+  type        = string
   description = "Project for tagging and naming. See [doc](../README.md#consistent-tagging)"
 }
 
 variable "env" {
-  type        = "string"
+  type        = string
   description = "Env for tagging and naming. See [doc](../README.md#consistent-tagging)."
 }
 
+variable "service" {
+  type        = string
+  description = "Service for tagging and naming. See [doc](../README.md#consistent-tagging)"
+  default     = "redis"
+}
+
 variable "owner" {
-  type        = "string"
+  type        = string
   description = "Owner for tagging and naming. See [doc](../README.md#consistent-tagging)."
 }
 
 variable "subnets" {
-  type        = "list"
+  type        = list(string)
   description = "List of subnets to which this EC instance should be attached. They should probably be private."
 }
 
 variable "availability_zone" {
-  type        = "string"
+  type        = string
   description = "Availability zone in which this instance should run."
   default     = null
 }
 
 variable "ingress_security_group_ids" {
-  type        = "list"
+  type        = list(string)
   description = "Source security groups which should be able to contact this instance."
 }
 
-variable "service" {
-  type        = "string"
-  description = "Service for tagging and naming. See [doc](../README.md#consistent-tagging)"
-  default     = "redis"
-}
-
 variable "port" {
-  type    = "string"
-  default = "6379"
+  type        = number
+  description = "Port to host Redis on."
+  default     = 6379
 }
 
 variable "instance_type" {
-  type        = "string"
+  type        = string
   description = "The type of instance to run. See [supported node types](https://docs.aws.amazon.com/AmazonElastiCache/latest/red-ug/CacheNodes.SupportedTypes.html)"
   default     = "cache.m5.large"
 }
 
 variable "parameter_group_name" {
-  default = "default.redis5.0"
+  type        = string
+  description = "Parameter group to use for this Redis cache."
+  default     = "default.redis5.0"
 }
 
 variable "engine_version" {
-  type        = "string"
+  type        = string
   description = "The version of Redis to run. See [supported versions](https://docs.aws.amazon.com/AmazonElastiCache/latest/red-ug/supported-engine-versions.html)"
   default     = "5.0.5"
 }
 
 variable "apply_immediately" {
-  type        = "string"
+  type        = bool
   description = "Whether changes should be applied immediately or during the next maintenance window."
   default     = true
 }
@@ -66,6 +69,11 @@ variable "apply_immediately" {
 # only 20 characters long. Use it only if you get that error.
 variable "resource_name" {
   description = "If not set, name will be [var.project]-[var.env]-[var.name]."
-  type        = "string"
+  type        = string
   default     = ""
 }
+
+variable "vpc_id" {
+  type        = string
+  description = "VPC where the cache will be deployed."
+}