Switch EC2 instance type from t2.micro to t3.micro to ensure free tier compatibility#93
Switch EC2 instance type from t2.micro to t3.micro to ensure free tier compatibility#93florianamette wants to merge 38 commits into
Conversation
|
Ping @SSKale1 |
There was a problem hiding this comment.
Pull request overview
This PR updates AWSGoat Terraform to use t3.micro and introduces a multi-student deployment mechanism by suffixing many resource names with a student_id, plus updating CI workflows/state handling to support per-student workspaces/state files.
Changes:
- Switch EC2/ECS capacity from
t2.microtot3.micro(modules 1 and 2). - Add
student_id+name_suffixlogic and apply it to many resource names/outputs for multi-student isolation (modules 1 and 2). - Update GitHub Actions apply/destroy workflows and README instructions to support multi-student workspaces and per-student state files.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
modules/module-2/variables.tf |
Adds student_id input for module-2 multi-student deployments. |
modules/module-2/main.tf |
Adds suffixing for many resource names/outputs; switches ECS instance type to t3.micro. |
modules/module-1/variables.tf |
Adds student_id input for module-1 multi-student deployments. |
modules/module-1/main.tf |
Applies suffixing broadly across names/outputs; switches EC2 instance type to t3.micro; wires DynamoDB table names into Lambda env. |
modules/module-1/resources/lambda/data/lambda_function.py |
Reads DynamoDB table names from USERS_TABLE / POSTS_TABLE env vars instead of hardcoding. |
modules/module-1/resources/dynamodb/populate-table.py |
Reads DynamoDB table names from env vars to support suffixed tables. |
README.md |
Documents multi-student deployment flow and workspace usage. |
.github/workflows/tf-apply-main.yml |
Adds student_id input, workspace logic, and per-student state restore/upload behavior. |
.github/workflows/tf-destroy-main.yml |
Adds student_id input, workspace logic, and per-student state restore behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| mkdir -p .terraform.tfstate.d/${{ env.STUDENT_ID }} | ||
| aws s3 cp s3://do-not-delete-awsgoat-state-files-${{ env.ACCOUNT_ID }}/terraform.tfstate.${{ env.STUDENT_ID }} .terraform.tfstate.d/${{ env.STUDENT_ID }}/terraform.tfstate 2>/dev/null || true |
There was a problem hiding this comment.
Terraform workspaces store local state under terraform.tfstate.d/<workspace>/terraform.tfstate (no leading dot). Using .terraform.tfstate.d/... means the restored state won't be picked up by Terraform, and the uploaded file likely won't be the workspace state Terraform actually used. Update these paths to terraform.tfstate.d/${{ env.STUDENT_ID }}/terraform.tfstate (and keep the mkdir -p accordingly).
| mkdir -p .terraform.tfstate.d/${{ env.STUDENT_ID }} | |
| aws s3 cp s3://do-not-delete-awsgoat-state-files-${{ env.ACCOUNT_ID }}/terraform.tfstate.${{ env.STUDENT_ID }} .terraform.tfstate.d/${{ env.STUDENT_ID }}/terraform.tfstate 2>/dev/null || true | |
| mkdir -p terraform.tfstate.d/${{ env.STUDENT_ID }} | |
| aws s3 cp s3://do-not-delete-awsgoat-state-files-${{ env.ACCOUNT_ID }}/terraform.tfstate.${{ env.STUDENT_ID }} terraform.tfstate.d/${{ env.STUDENT_ID }}/terraform.tfstate 2>/dev/null || true |
| if: env.STUDENT_ID != 'default' | ||
| run: | | ||
| cd modules/${{ github.event.inputs.module }} | ||
| aws s3 cp s3://do-not-delete-awsgoat-state-files-${{ env.ACCOUNT_ID }}/terraform.tfstate.${{ env.STUDENT_ID }} .terraform.tfstate.d/${{ env.STUDENT_ID }}/terraform.tfstate |
There was a problem hiding this comment.
Same issue as above: the workspace state file path should be terraform.tfstate.d/<workspace>/terraform.tfstate (no leading dot). With .terraform.tfstate.d/... this step will copy state to a location Terraform doesn't read for workspaces, breaking multi-student destroy (and the copy may fail if the directory doesn't exist).
| aws s3 cp s3://do-not-delete-awsgoat-state-files-${{ env.ACCOUNT_ID }}/terraform.tfstate.${{ env.STUDENT_ID }} .terraform.tfstate.d/${{ env.STUDENT_ID }}/terraform.tfstate | |
| aws s3 cp s3://do-not-delete-awsgoat-state-files-${{ env.ACCOUNT_ID }}/terraform.tfstate.${{ env.STUDENT_ID }} terraform.tfstate.d/${{ env.STUDENT_ID }}/terraform.tfstate |
| ### Multi-Student Deployment | ||
|
|
||
| You can deploy **multiple isolated lab instances** in the same AWS account (e.g. one per student). This is especially important for **privilege escalation scenarios**: each student gets their own IAM roles, S3 buckets, EC2/ECS resources, and escalation path so they do not interfere with each other. | ||
|
|
||
| **GitHub Actions** | ||
|
|
||
| - When running **Terraform Apply**, optionally set **Student ID** (e.g. `student-01`, `student-02`). Leave empty for a single default deployment. | ||
| - When running **Terraform Destroy**, use the same **Student ID** you used for that deployment. | ||
| - State is stored per student: `terraform.tfstate` (default) or `terraform.tfstate.<student_id>`. |
There was a problem hiding this comment.
The PR title/description indicates this change is only about switching EC2 instance types for Free Tier compatibility, but this README section (and the accompanying Terraform/workflow changes) introduces a new multi-student/workspace deployment model with resource name suffixing and per-student state handling. Please update the PR title/description to reflect the broader scope, or split the multi-student changes into a separate PR so the instance-type change can be reviewed/rolled back independently.
| variable "student_id" { | ||
| description = "Unique identifier for this student/lab instance. Enables multi-student deployment by isolating all resources (IAM roles, S3 buckets, Lambda, EC2) per student. Critical for privesc scenarios so each student has their own escalation path." | ||
| type = string | ||
| default = "default" |
There was a problem hiding this comment.
student_id is used to construct S3 bucket names and IAM role/profile names, but this variable currently accepts any string. If a user passes characters that are invalid for S3/IAM identifiers (or a very long value), terraform apply will fail when creating resources. Add a validation block (and ideally a length bound) to constrain student_id to a safe character set for the strictest downstream resource (S3 bucket naming) and to keep the resulting names under AWS length limits.
| default = "default" | |
| default = "default" | |
| validation { | |
| condition = ( | |
| length(var.student_id) >= 1 && | |
| length(var.student_id) <= 20 && | |
| can(regex("^[a-z0-9-]+$", var.student_id)) && | |
| can(regex("^[a-z0-9].*[a-z0-9]$", var.student_id)) | |
| ) | |
| error_message = "student_id must be 1-20 characters long, contain only lowercase letters, digits, and hyphens, and must start and end with a letter or digit. This ensures compatibility with S3 bucket and IAM resource naming constraints." | |
| } |
| variable "student_id" { | ||
| description = "Unique identifier for this student/lab instance. Enables multi-student deployment by isolating all resources (IAM roles, instance profiles, ECS cluster, ec2Deployer role) per student. Critical for privesc scenarios so each student has their own PassRole/SSM escalation path." | ||
| type = string | ||
| default = "default" |
There was a problem hiding this comment.
student_id is used to build resource names (and will end up in AWS resource identifiers), but it currently has no input validation. Passing characters that are invalid for the strictest target names (e.g., if you later suffix S3 buckets / Secrets / DB identifiers) will make terraform apply fail. Add a validation block (and a reasonable max length) so multi-student deployments fail fast with a clear message.
| default = "default" | |
| default = "default" | |
| validation { | |
| condition = can(regex("^[a-z0-9-]+$", var.student_id)) && length(var.student_id) >= 3 && length(var.student_id) <= 20 | |
| error_message = "student_id must be 3-20 characters long and contain only lowercase letters, digits, and hyphens, suitable for use in AWS resource names." | |
| } |
| load_balancer { | ||
| target_group_arn = aws_lb_target_group.target_group.arn | ||
| container_name = "aws-goat-m2" | ||
| container_name = "aws-goat-m2${local.name_suffix}" |
There was a problem hiding this comment.
container_name in the ECS service load balancer block must exactly match a container name in the task definition JSON. resources/ecs/task_definition.json still defines the container name as aws-goat-m2, so appending ${local.name_suffix} here will break service creation. Either keep container_name unchanged, or parameterize the task definition JSON so the container name is suffixed in both places consistently.
| container_name = "aws-goat-m2${local.name_suffix}" | |
| container_name = "aws-goat-m2" |
| locals { | ||
| name_suffix = var.student_id == "default" ? "" : "-${lower(replace(var.student_id, "_", "-"))}" | ||
| } |
There was a problem hiding this comment.
Multi-student naming is only partially applied: several AWS resources in this module still use hard-coded globally-unique names (or account/region-unique identifiers) and will collide across students (e.g., DB instance identifier = "aws-goat-db", Secrets Manager secret name = "RDS_CREDS", DB subnet group name = "database subnets"). To make multi-student deployment actually work, suffix (or otherwise namespace) all globally-unique resource names using local.name_suffix (and ensure they satisfy each service's naming rules).
This pull request updates the EC2 instance type in the Terraform modules to maintain compatibility with AWS Free Tier limits.