Skip to content

feat: add VPC and subnet name support #54

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

Merged
merged 2 commits into from
Aug 29, 2025
Merged

feat: add VPC and subnet name support #54

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2c0ec7b
feat: allow passing VPC and Subnet names
gberenice Aug 28, 2025
21f6285
chore: security best-practices + minor improvements
gberenice Aug 29, 2025
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
86 changes: 85 additions & 1 deletion data.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,26 @@
data "aws_region" "current" {}
data "aws_caller_identity" "current" {}

# VPC lookup by name (when vpc_name is provided)
data "aws_vpc" "selected" {
count = length(var.vpc_name) > 0 ? 1 : 0

filter {
name = "tag:Name"
values = [var.vpc_name]
}
}

# Individual subnet lookup by name (when subnet_names are provided)
data "aws_subnet" "selected" {
for_each = toset(var.subnet_names)

filter {
name = "tag:Name"
values = [each.value]
}
}

# Most recent Amazon Linux 2023 AMI
data "aws_ami" "amazon_linux_2023" {
most_recent = true
Expand Down Expand Up @@ -33,7 +53,7 @@ data "aws_ami" "amazon_linux_2023" {
# This rule was introduced in the following PR:
# https://github.com/masterpointio/terraform-aws-ssm-agent/pull/43.
#
# trunk-ignore(trivy/AVD-AWS-0344)
# trivy:ignore:AVD-AWS-0344
data "aws_ami" "instance" {
count = length(var.ami) > 0 ? 1 : 0

Expand All @@ -44,3 +64,67 @@ data "aws_ami" "instance" {
values = [var.ami]
}
}

# IAM policy document for EC2 instances to assume the SSM Agent role
data "aws_iam_policy_document" "default" {

statement {
effect = "Allow"
actions = ["sts:AssumeRole"]

principals {
type = "Service"
identifiers = ["ec2.amazonaws.com"]
}
}
}

# S3 bucket for session logging (when using existing bucket)
data "aws_s3_bucket" "logs_bucket" {
count = var.session_logging_enabled ? 1 : 0
bucket = try(coalesce(var.session_logging_bucket_name, module.logs_bucket.bucket_id), "")
}
Copy link

@coderabbitai coderabbitai bot Aug 28, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Tighten session logging IAM policy to least privilege and remove data source dependency

  • Avoid “*” where feasible: scope CloudWatch Logs actions to the specific log group, S3 GetEncryptionConfiguration to the target bucket, and kms:GenerateDataKey to the KMS key.
  • Optional: Construct S3 bucket ARN directly to drop the aws_s3_bucket data source (reduces IAM read perms and planning overhead).

Apply:

- data "aws_s3_bucket" "logs_bucket" {
-  count  = var.session_logging_enabled ? 1 : 0
-  bucket = try(coalesce(var.session_logging_bucket_name, module.logs_bucket.bucket_id), "")
- }
+ # Optional: remove this data source entirely by constructing the ARN directly.
+ # S3 bucket ARN format: arn:aws:s3:::<bucket-name>
   statement {
     sid    = "SSMAgentSessionAllowS3Logging"
     effect = "Allow"
     actions = [
       "s3:PutObject"
     ]
-    resources = ["${join("", data.aws_s3_bucket.logs_bucket.*.arn)}/*"]
+    # If keeping the data source:
+    resources = formatlist("%s/*", data.aws_s3_bucket.logs_bucket[*].arn)
+    # If removing the data source, use:
+    # resources = ["arn:aws:s3:::${local.session_logging_bucket_name}/*"]
   }

   statement {
     sid    = "SSMAgentSessionAllowCloudWatchLogging"
     effect = "Allow"
     actions = [
       "logs:CreateLogStream",
       "logs:PutLogEvents",
       "logs:DescribeLogGroups",
       "logs:DescribeLogStreams"
     ]
-    resources = ["*"]
+    resources = [
+      "arn:aws:logs:${data.aws_region.current.name}:${data.aws_caller_identity.current.account_id}:log-group:${module.logs_label.id}:*"
+    ]
   }

   statement {
     sid    = "SSMAgentSessionAllowGetEncryptionConfig"
     effect = "Allow"
     actions = [
       "s3:GetEncryptionConfiguration"
     ]
-    resources = ["*"]
+    # Scope to the session logs bucket only
+    resources = data.aws_s3_bucket.logs_bucket[*].arn
+    # Or: ["arn:aws:s3:::${local.session_logging_bucket_name}"]
   }

   statement {
     sid    = "SSMAgentSessionAllowKMSDataKey"
     effect = "Allow"
     actions = [
       "kms:GenerateDataKey"
     ]
-    resources = ["*"]
+    resources = compact([local.session_logging_kms_key_arn])
   }

Also applies to: 88-130

🤖 Prompt for AI Agents 
In data.tf around lines 83-86 (and similarly 88-130), the session logging IAM
policy is overly permissive and relies on the aws_s3_bucket data source; tighten
permissions by scoping actions to exact ARNs and remove the data source
dependency: replace "*" resources with the specific CloudWatch Logs log group
ARN
(arn:aws:logs:${var.region}:${data.aws_caller_identity.current.account_id}:log-group:${var.session_log_group_name}:*),
the S3 bucket ARN (arn:aws:s3:::${var.session_logging_bucket_name != "" ?
var.session_logging_bucket_name : module.logs_bucket.bucket_id}) for
s3:GetEncryptionConfiguration, and the KMS key ARN for kms:GenerateDataKey;
optionally delete the data "aws_s3_bucket" resource and compute the bucket ARN
directly from the input/module output, then update all IAM policy resource
blocks to reference those concrete ARNs rather than wildcards so permissions are
least-privilege.


# https://docs.aws.amazon.com/systems-manager/latest/userguide/getting-started-create-iam-instance-profile.html#create-iam-instance-profile-ssn-logging
data "aws_iam_policy_document" "session_logging" {
count = var.session_logging_enabled ? 1 : 0

statement {
sid = "SSMAgentSessionAllowS3Logging"
effect = "Allow"
actions = [
"s3:PutObject"
]
resources = ["${join("", data.aws_s3_bucket.logs_bucket.*.arn)}/*"]
}

statement {
sid = "SSMAgentSessionAllowCloudWatchLogging"
effect = "Allow"
actions = [
"logs:CreateLogStream",
"logs:PutLogEvents",
"logs:DescribeLogGroups",
"logs:DescribeLogStreams"
]
resources = ["*"]
}

statement {
sid = "SSMAgentSessionAllowGetEncryptionConfig"
effect = "Allow"
actions = [
"s3:GetEncryptionConfiguration"
]
resources = ["*"]
}

statement {
sid = "SSMAgentSessionAllowKMSDataKey"
effect = "Allow"
actions = [
"kms:GenerateDataKey"
]
resources = ["*"]
}
}
75 changes: 75 additions & 0 deletions examples/with-names/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
provider "aws" {
region = var.region
}

# Create VPC and subnets with specific names for demonstration
module "vpc" {
source = "cloudposse/vpc/aws"
version = "2.1.0"

namespace = var.namespace
stage = var.stage
name = "example-vpc"

ipv4_primary_cidr_block = "10.0.0.0/16"
assign_generated_ipv6_cidr_block = true
}

module "subnets" {
source = "cloudposse/dynamic-subnets/aws"
version = "2.3.0"
namespace = var.namespace
stage = var.stage
name = "example-subnets"

availability_zones = var.availability_zones
vpc_id = module.vpc.vpc_id
igw_id = [module.vpc.igw_id]
ipv4_cidr_block = [module.vpc.vpc_cidr_block]
ipv6_enabled = var.ipv6_enabled
}

# Example 1: Using VPC and subnet IDs (traditional approach)
module "ssm_agent_with_ids" {
source = "../../"
stage = var.stage
namespace = var.namespace
name = "ssm-with-ids"

vpc_id = module.vpc.vpc_id
subnet_ids = module.subnets.private_subnet_ids
}

# Example 2: Using VPC and subnet names (new functionality)
module "ssm_agent_with_names" {
source = "../../"
stage = var.stage
namespace = var.namespace
name = "ssm-with-names"

vpc_name = module.vpc.vpc_id_tag_name # This would be the Name tag value
subnet_names = [
# These would be the actual Name tag values of the subnets
# In a real scenario, you'd know these names or get them from data sources
"example-private-subnet-1",
"example-private-subnet-2"
]

depends_on = [module.subnets]
}
Comment on lines +58 to +59
Copy link

@coderabbitai coderabbitai bot Aug 28, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

❓ Verification inconclusive

depends_on here won’t fix plan-time data lookups

Data sources inside the child module resolve during planning; they won’t “wait” for module.subnets. This example will often fail on first plan/apply when the subnets don’t yet exist. Document this limitation or prefer IDs when creating the subnets in the same run.

-  depends_on = [module.subnets]
+  # Note: Name-based lookups plan-time resolve and can fail on first apply if subnets are created in the same run.
+  # Consider using IDs when creating subnets, or run in two stages (create subnets, then apply this module with names).

Document plan-time data lookup limitation and prefer IDs or two-stage applies
Terraform data sources inside modules resolve during the planning phase, so a depends_on on module.subnets won’t delay those lookups until after subnet creation—this can cause failures on the first apply when subnets don’t yet exist. Update the example to either:

  • Use subnet IDs directly in this module (so the lookup happens against created resources)
  • Or split into two runs: first apply the subnets module, then apply the module relying on name-based lookups.
🤖 Prompt for AI Agents 
In examples/with-names/main.tf around lines 58-59, the current depends_on =
[module.subnets] comment is misleading because Terraform data sources inside
modules are evaluated at plan time and won’t wait for the subnets to be created;
update the example to either (A) accept and pass the subnet IDs into this module
so the module uses explicit IDs (avoiding plan-time lookups), or (B) document
and demonstrate a two-stage apply workflow (first terraform apply for the
subnets module, then apply for the dependent module) and remove/clarify the
depends_on that implies data lookups will be delayed until after creation.


# Example 3: Mixed configuration (VPC by ID, subnets by name)
module "ssm_agent_mixed" {
source = "../../"
stage = var.stage
namespace = var.namespace
name = "ssm-mixed"

vpc_id = module.vpc.vpc_id
subnet_names = [
"example-private-subnet-1",
"example-private-subnet-2"
]

depends_on = [module.subnets]
}
Comment on lines +74 to +75
Copy link

@coderabbitai coderabbitai bot Aug 28, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Same plan-time lookup caveat for mixed example

Remove depends_on and add a note, or switch the example to consume existing infra names.

-  depends_on = [module.subnets]
+  # See note above regarding plan-time name lookups when subnets are created in the same run.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
depends_on = [module.subnets]
}
# See note above regarding plan-time name lookups when subnets are created in the same run.
}
🤖 Prompt for AI Agents 
In examples/with-names/main.tf around lines 74-75, the explicit depends_on =
[module.subnets] introduces a plan-time lookup caveat; remove the depends_on
line and either (a) add a short comment/note in the example explaining the
caveat and recommending consuming named existing infra (or using data sources)
instead of forcing depends_on, or (b) modify the example to consume existing
infra names/IDs (e.g., accept variables or use data sources for subnets) so the
example no longer needs depends_on. Ensure the file reflects the chosen approach
and update any adjacent README/example text to explain the behavior.

14 changes: 14 additions & 0 deletions examples/with-names/outputs.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
output "ssm_agent_with_ids_instance_profile_arn" {
description = "ARN of the IAM instance profile for SSM agent (using IDs)"
value = module.ssm_agent_with_ids.iam_instance_profile_arn
}

output "ssm_agent_with_names_instance_profile_arn" {
description = "ARN of the IAM instance profile for SSM agent (using names)"
value = module.ssm_agent_with_names.iam_instance_profile_arn
}

output "ssm_agent_mixed_instance_profile_arn" {
description = "ARN of the IAM instance profile for SSM agent (mixed approach)"
value = module.ssm_agent_mixed.iam_instance_profile_arn
}
29 changes: 29 additions & 0 deletions examples/with-names/variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
variable "region" {
type = string
description = "AWS region"
default = "us-east-1"
}

variable "namespace" {
type = string
description = "Namespace for resource naming"
default = "mp"
}

variable "stage" {
type = string
description = "Stage for resource naming"
default = "test"
}

variable "availability_zones" {
type = list(string)
description = "List of availability zones"
default = ["us-east-1a", "us-east-1b"]
}

variable "ipv6_enabled" {
type = bool
description = "Enable IPv6"
default = false
}
114 changes: 40 additions & 74 deletions main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ locals {
length(var.ami) > 0 ? element(data.aws_ami.instance, 0).root_device_name : "/dev/xvda"
)

# VPC and Subnet ID resolution - names take precedence over IDs
vpc_id = length(var.vpc_name) > 0 ? one(data.aws_vpc.selected[*].id) : var.vpc_id
subnet_ids = length(var.subnet_names) > 0 ? values(data.aws_subnet.selected)[*].id : var.subnet_ids

}

resource "null_resource" "validate_instance_type" {
Expand All @@ -27,6 +31,34 @@ resource "null_resource" "validate_instance_type" {
}
}

# Validation using terraform_data to halt execution if requirements aren't met
resource "terraform_data" "vpc_subnet_validation" {
lifecycle {
precondition {
condition = length(var.vpc_name) > 0 || length(var.vpc_id) > 0
error_message = "Either vpc_name or vpc_id must be provided."
}

precondition {
condition = length(var.subnet_names) > 0 || length(var.subnet_ids) > 0
error_message = "Either subnet_names or subnet_ids must be provided."
}
}
}

# Warning checks for VPC and subnet configuration (non-blocking)
check "vpc_subnet_warnings" {
assert {
condition = !(length(var.vpc_name) > 0 && length(var.vpc_id) > 0)
error_message = "Both vpc_name and vpc_id are provided. When vpc_name is specified, vpc_id will be ignored."
}

assert {
condition = !(length(var.subnet_names) > 0 && length(var.subnet_ids) > 0)
error_message = "Both subnet_names and subnet_ids are provided. When subnet_names are specified, subnet_ids will be ignored."
}
}

Copy link
Contributor

@oycyc oycyc Aug 28, 2025

Choose a reason for hiding this comment

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

I'm wondering, does this belong in the main.tf? Is there another file that this might best live in?

Copy link
Member Author

@gberenice gberenice Aug 29, 2025

Choose a reason for hiding this comment

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

Move to validations.tf for clarity 👍

module "role_label" {
source = "cloudposse/label/null"
version = "0.25.0"
Expand Down Expand Up @@ -57,68 +89,6 @@ locals {
## SSM AGENT ROLE ##
###################

data "aws_iam_policy_document" "default" {

statement {
effect = "Allow"
actions = ["sts:AssumeRole"]

principals {
type = "Service"
identifiers = ["ec2.amazonaws.com"]
}
}
}

data "aws_s3_bucket" "logs_bucket" {
count = var.session_logging_enabled ? 1 : 0
bucket = try(coalesce(var.session_logging_bucket_name, module.logs_bucket.bucket_id), "")
}

# https://docs.aws.amazon.com/systems-manager/latest/userguide/getting-started-create-iam-instance-profile.html#create-iam-instance-profile-ssn-logging
data "aws_iam_policy_document" "session_logging" {
count = var.session_logging_enabled ? 1 : 0

statement {
sid = "SSMAgentSessionAllowS3Logging"
effect = "Allow"
actions = [
"s3:PutObject"
]
resources = ["${join("", data.aws_s3_bucket.logs_bucket.*.arn)}/*"]
}

statement {
sid = "SSMAgentSessionAllowCloudWatchLogging"
effect = "Allow"
actions = [
"logs:CreateLogStream",
"logs:PutLogEvents",
"logs:DescribeLogGroups",
"logs:DescribeLogStreams"
]
resources = ["*"]
}

statement {
sid = "SSMAgentSessionAllowGetEncryptionConfig"
effect = "Allow"
actions = [
"s3:GetEncryptionConfiguration"
]
resources = ["*"]
}

statement {
sid = "SSMAgentSessionAllowKMSDataKey"
effect = "Allow"
actions = [
"kms:GenerateDataKey"
]
resources = ["*"]
}
}

resource "aws_iam_role" "default" {
name = module.role_label.id
assume_role_policy = data.aws_iam_policy_document.default.json
Expand Down Expand Up @@ -157,19 +127,21 @@ resource "aws_iam_instance_profile" "default" {
###################

resource "aws_security_group" "default" {
vpc_id = var.vpc_id
vpc_id = local.vpc_id
name = module.this.id
description = "Allow ALL egress from SSM Agent."
tags = module.this.tags
}

# trivy:ignore:aws-vpc-no-public-egress-sgr SSM Agent requires broad egress to communicate with AWS services
resource "aws_security_group_rule" "allow_all_egress" {
type = "egress"
from_port = 0
to_port = 0
protocol = "-1"
cidr_blocks = ["0.0.0.0/0"]
ipv6_cidr_blocks = ["::/0"]
description = "Allow all outbound traffic for SSM Agent communication with AWS services"
security_group_id = aws_security_group.default.id
}

Expand Down Expand Up @@ -246,21 +218,15 @@ DOC

module "logs_bucket" {
source = "cloudposse/s3-bucket/aws"
version = "3.1.2"
version = "4.10.0"

enabled = local.logs_bucket_enabled
context = module.logs_label.context

# Encryption / Security
acl = "private"
sse_algorithm = "aws:kms"
kms_master_key_arn = local.session_logging_kms_key_arn
allow_encrypted_uploads_only = false
force_destroy = true

# Feature enablement
user_enabled = false
versioning_enabled = true
sse_algorithm = "aws:kms"
kms_master_key_arn = local.session_logging_kms_key_arn
force_destroy = true

lifecycle_configuration_rules = [{
enabled = true
Expand Down Expand Up @@ -385,7 +351,7 @@ resource "aws_autoscaling_group" "default" {
# By default, we don't care to protect from scale in as we want to roll instances frequently
protect_from_scale_in = var.protect_from_scale_in

vpc_zone_identifier = var.subnet_ids
vpc_zone_identifier = local.subnet_ids

default_cooldown = 180
health_check_grace_period = 180
Expand Down
Loading