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.

Sign up for GitHub

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

feat: manage default security group #382

Merged
merged 8 commits into from
Jun 21, 2020
Merged

feat: manage default security group #382

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2bb3bd0
Feature / Manage Default Security Group
bryantbiggs Jan 12, 2020
3399914
Merge branch 'master' of github.com:clowdhaus/terraform-aws-vpc into …
bryantbiggs Feb 13, 2020
2d1ee2a
Merge branch 'master' of github.com:clowdhaus/terraform-aws-vpc into …
bryantbiggs Mar 23, 2020
ac1d3e5
Merge branch 'master' into feature/managed-default-security-group
bryantbiggs Mar 24, 2020
99e8d19
Merge branch 'master' into feature/managed-default-security-group
bryantbiggs Apr 9, 2020
5fbcb24
Merge branch 'master' of github.com:clowdhaus/terraform-aws-vpc into …
bryantbiggs Jun 20, 2020
dca165a
- Use `null` as default to preserve current settings and when removin…
bryantbiggs Jun 20, 2020
510c568
Merge branch 'master' into feature/managed-default-security-group
bryantbiggs Jun 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@ These types of resources are supported:
* [VPC Flow Log](https://www.terraform.io/docs/providers/aws/r/flow_log.html)
* [VPC Endpoint](https://www.terraform.io/docs/providers/aws/r/vpc_endpoint.html):
* Gateway: S3, DynamoDB
* Interface: EC2, SSM, EC2 Messages, SSM Messages, SQS, ECR API, ECR DKR, API Gateway, KMS,
ECS, ECS Agent, ECS Telemetry, SES, SNS, STS, Glue, CloudWatch(Monitoring, Logs, Events),
Elastic Load Balancing, CloudTrail, Secrets Manager, Config, CodeBuild, CodeCommit,
Git-Codecommit, Transfer Server, Kinesis Streams, Kinesis Firehose, SageMaker(Notebook, Runtime, API),
* Interface: EC2, SSM, EC2 Messages, SSM Messages, SQS, ECR API, ECR DKR, API Gateway, KMS,
ECS, ECS Agent, ECS Telemetry, SES, SNS, STS, Glue, CloudWatch(Monitoring, Logs, Events),
Elastic Load Balancing, CloudTrail, Secrets Manager, Config, CodeBuild, CodeCommit,
Git-Codecommit, Transfer Server, Kinesis Streams, Kinesis Firehose, SageMaker(Notebook, Runtime, API),
CloudFormation, CodePipeline, Storage Gateway, AppMesh, Transfer, Service Catalog, AppStream,
Athena, Rekognition, Elastic File System (EFS), Cloud Directory, Elastic Beanstalk (+ Health), Elastic Map Reduce(EMR),
DataSync, EBS, SMS, Elastic Inference Runtime, QLDB Session, Step Functions, Access Analyzer, Auto Scaling Plans,
Application Auto Scaling, Workspaces, ACM PCA.
Application Auto Scaling, Workspaces, ACM PCA.

* [RDS DB Subnet Group](https://www.terraform.io/docs/providers/aws/r/db_subnet_group.html)
* [ElastiCache Subnet Group](https://www.terraform.io/docs/providers/aws/r/elasticache_subnet_group.html)
Expand Down Expand Up @@ -314,6 +314,10 @@ It is possible to integrate this VPC module with [terraform-aws-transit-gateway
| default\_network\_acl\_ingress | List of maps of ingress rules to set on the Default Network ACL | `list(map(string))` | <pre>[<br> {<br> "action": "allow",<br> "cidr_block": "0.0.0.0/0",<br> "from_port": 0,<br> "protocol": "-1",<br> "rule_no": 100,<br> "to_port": 0<br> },<br> {<br> "action": "allow",<br> "from_port": 0,<br> "ipv6_cidr_block": "::/0",<br> "protocol": "-1",<br> "rule_no": 101,<br> "to_port": 0<br> }<br>]</pre> | no |
| default\_network\_acl\_name | Name to be used on the Default Network ACL | `string` | `""` | no |
| default\_network\_acl\_tags | Additional tags for the Default Network ACL | `map(string)` | `{}` | no |
| default\_security\_group\_egress | List of maps of egress rules to set on the default security group | `list(map(string))` | <pre>[<br> {<br> "cidr_blocks": "0.0.0.0/0",<br> "from_port": 0,<br> "protocol": "-1",<br> "to_port": 0<br> }<br>]</pre> | no |
| default\_security\_group\_ingress | List of maps of ingress rules to set on the default security group | `list(map(string))` | <pre>[<br> {<br> "from_port": 0,<br> "protocol": "-1",<br> "self": true,<br> "to_port": 0<br> }<br>]</pre> | no |
| default\_security\_group\_name | Name to be used on the default security group | `string` | `""` | no |
| default\_security\_group\_tags | Additional tags for the default security group | `map(string)` | `{}` | no |
| default\_vpc\_enable\_classiclink | Should be true to enable ClassicLink in the Default VPC | `bool` | `false` | no |
| default\_vpc\_enable\_dns\_hostnames | Should be true to enable DNS hostnames in the Default VPC | `bool` | `false` | no |
| default\_vpc\_enable\_dns\_support | Should be true to enable DNS support in the Default VPC | `bool` | `true` | no |
Expand Down Expand Up @@ -494,6 +498,7 @@ It is possible to integrate this VPC module with [terraform-aws-transit-gateway
| logs\_endpoint\_security\_group\_ids | The ID of one or more security groups to associate with the network interface for CloudWatch Logs endpoint | `list(string)` | `[]` | no |
| logs\_endpoint\_subnet\_ids | The ID of one or more subnets in which to create a network interface for CloudWatch Logs endpoint. Only a single subnet within an AZ is supported. If omitted, private subnets will be used. | `list(string)` | `[]` | no |
| manage\_default\_network\_acl | Should be true to adopt and manage Default Network ACL | `bool` | `false` | no |
| manage\_default\_security\_group | Should be true to adopt and manage default security group | `bool` | `false` | no |
| manage\_default\_vpc | Should be true to adopt and manage Default VPC | `bool` | `false` | no |
| map\_public\_ip\_on\_launch | Should be false if you do not want to auto-assign public IP on launch | `bool` | `true` | no |
| monitoring\_endpoint\_private\_dns\_enabled | Whether or not to associate a private hosted zone with the specified VPC for CloudWatch Monitoring endpoint | `bool` | `false` | no |
Expand Down
5 changes: 5 additions & 0 deletions examples/complete-vpc/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,11 @@ module "vpc" {
sqs_endpoint_private_dns_enabled = true
sqs_endpoint_security_group_ids = [data.aws_security_group.default.id]

# Default security group - ingress/egress rules cleared to deny all
manage_default_security_group = true
default_security_group_ingress = [{}]
default_security_group_egress = [{}]

# VPC Flow Logs (Cloudwatch log group and IAM role will be created)
enable_flow_log = true
create_flow_log_cloudwatch_log_group = true
Expand Down
43 changes: 42 additions & 1 deletion main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,48 @@ resource "aws_vpc_ipv4_cidr_block_association" "this" {
cidr_block = element(var.secondary_cidr_blocks, count.index)
}

resource "aws_default_security_group" "this" {
count = var.create_vpc && var.manage_default_security_group ? 1 : 0

vpc_id = element(concat(aws_vpc.this.*.id, [""]), 0)
bryantbiggs marked this conversation as resolved.
Show resolved Hide resolved

dynamic "ingress" {
for_each = var.default_security_group_ingress
content {
self = lookup(ingress.value, "self", null)
cidr_blocks = compact(split(",", lookup(ingress.value, "cidr_blocks", "")))
ipv6_cidr_blocks = compact(split(",", lookup(ingress.value, "ipv6_cidr_blocks", "")))
prefix_list_ids = compact(split(",", lookup(ingress.value, "prefix_list_ids", "")))
description = lookup(ingress.value, "description", null)
from_port = lookup(ingress.value, "from_port", 0)
to_port = lookup(ingress.value, "to_port", 0)
protocol = lookup(ingress.value, "protocol", "-1")
}
}

dynamic "egress" {
for_each = var.default_security_group_egress
content {
self = lookup(egress.value, "self", null)
cidr_blocks = compact(split(",", lookup(egress.value, "cidr_blocks", "")))
ipv6_cidr_blocks = compact(split(",", lookup(egress.value, "ipv6_cidr_blocks", "")))
prefix_list_ids = compact(split(",", lookup(egress.value, "prefix_list_ids", "")))
description = lookup(egress.value, "description", null)
from_port = lookup(egress.value, "from_port", 0)
to_port = lookup(egress.value, "to_port", 0)
protocol = lookup(egress.value, "protocol", "-1")
}
}

tags = merge(
{
"Name" = format("%s", var.default_security_group_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

if the default SG is set to be managed and the default_security_group_name left with no change to default value it creates a an empty Name tag. its not a problem functionally but just FYI if you want to change this.

Copy link
Member Author

Choose a reason for hiding this comment

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

good point - maybe I add a check for a provided name value otherwise provide one like "default"

Copy link
Member Author

Choose a reason for hiding this comment

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

ended up adding a default value of default for the variable

},
var.tags,
var.default_security_group_tags,
)
}

###################
# DHCP Options Set
###################
Expand Down Expand Up @@ -1105,4 +1147,3 @@ resource "aws_default_vpc" "this" {
var.default_vpc_tags,
)
}

46 changes: 46 additions & 0 deletions variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2259,12 +2259,58 @@ variable "elasticache_outbound_acl_rules" {
]
}

variable "manage_default_security_group" {
description = "Should be true to adopt and manage default security group"
type = bool
default = false
}

variable "default_security_group_name" {
description = "Name to be used on the default security group"
type = string
default = ""
}

variable "default_security_group_ingress" {
description = "List of maps of ingress rules to set on the default security group"
type = list(map(string))

default = [
Copy link
Member

Choose a reason for hiding this comment

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

Maybe defaults should be empty, because "When Terraform first adopts the Default Security Group, it immediately removes all ingress and egress rules in the Security Group." (from https://www.terraform.io/docs/providers/aws/r/default_security_group.html )

And, if I understand correctly, Terraform won't put original values back, if the user switches off manage_default_security_group afterwards.

Previously, there were attempts to manage this resource in this module, but it was a bit too advanced for some use-cases and Terraform 0.11 which we had back then.

Copy link
Member Author

Choose a reason for hiding this comment

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

good point - I think you're right, might be better to use null here as it seems to be the safest route usually

{
self = true
from_port = 0
to_port = 0
protocol = "-1"
}
]
}

variable "enable_flow_log" {
description = "Whether or not to enable VPC Flow Logs"
type = bool
default = false
}

variable "default_security_group_egress" {
description = "List of maps of egress rules to set on the default security group"
type = list(map(string))

default = [
{
cidr_blocks = "0.0.0.0/0"
from_port = 0
to_port = 0
protocol = "-1"
}
]
}

variable "default_security_group_tags" {
description = "Additional tags for the default security group"
type = map(string)
default = {}
}

variable "create_flow_log_cloudwatch_log_group" {
description = "Whether to create CloudWatch log group for VPC Flow Logs"
type = bool
Expand Down