-
Notifications
You must be signed in to change notification settings - Fork 15
docs: add port forwarding example #61
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.
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThis pull request updates the README.md documentation to include a new prerequisites section specifying required IAM permissions for SSM StartSession and EC2 DescribeInstances operations. Additionally, a new section introduces port forwarding guidance through the SSM Agent, featuring a detailed script that locates an SSM gateway, retrieves the RDS endpoint and port information, and establishes a port-forwarding session. The changes enhance the documentation's coverage of permissions and network configuration requirements. Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
README.md (1)
93-130: Enhance script robustness with error handling and input validation.The script lacks error handling for common failure scenarios (missing instances, API errors) and uses partial substring matching for the RDS instance query without safeguards. Users should add validation to prevent unexpected behavior.
Consider adding basic error checks and result validation:
+set -e # Exit on any error + REGION=us-east-1 DB_INSTANCE_SUBSTRING="polygon" LOCAL_PORT=15432 # 1) Find the running SSM gateway instance ID by tag INSTANCE_ID="$( aws ec2 describe-instances \ --filters "Name=tag:Name,Values=*ssm*" "Name=instance-state-name,Values=running" \ --query 'Reservations[*].Instances[*].InstanceId' \ --region ${REGION} \ --output text)" +[ -z "$INSTANCE_ID" ] && echo "Error: No running SSM instance found" >&2 && exit 1 # 2) Find the RDS instance endpoint RDS_ENDPOINT="$( aws rds describe-db-instances \ --region ${REGION} \ --query "DBInstances[?contains(DBInstanceIdentifier, '${DB_INSTANCE_SUBSTRING}')].Endpoint.Address | [0]" \ --output text)" +[ -z "$RDS_ENDPOINT" ] && echo "Error: No RDS instance found matching '$DB_INSTANCE_SUBSTRING'" >&2 && exit 1 # 3) Dynamically get the RDS port RDS_PORT="$( aws rds describe-db-instances \ --region ${REGION} \ --query "DBInstances[?contains(DBInstanceIdentifier, '${DB_INSTANCE_SUBSTRING}')].Endpoint.Port | [0]" \ --output text)" +[ -z "$RDS_PORT" ] && echo "Error: Could not retrieve RDS port" >&2 && exit 1 echo "EC2 Instance ID: $INSTANCE_ID" echo "RDS Endpoint: $RDS_ENDPOINT" echo "Setting up port forwarding (ec2) ${RDS_PORT} -> (localhost) ${LOCAL_PORT}"This helps users identify configuration issues early rather than experiencing silent failures.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(2 hunks)
🔇 Additional comments (3)
README.md (3)
71-78: ✓ Clear prerequisites documentation added.The new prerequisites section clearly documents the required IAM permissions for basic SSM connectivity. This is a helpful addition.
88-91: ✓ Comprehensive prerequisites for port forwarding.The prerequisites clearly document required IAM permissions and mention security group validation, which is important for end-to-end connectivity.
84-130: ✓ Practical port-forwarding example with clear guidance.The new section effectively documents a realistic use case, provides clear prerequisites, and includes a detailed end-to-end script. This is a valuable addition to the documentation.
README.md
Outdated
| - Ensure the network architecture and Security Group permissions allow inbound traffic from the SSM Agent EC2 host. | ||
|
|
||
| ```bash | ||
| REGION=us-east-1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix variable name mismatch in the port-forwarding script.
Line 94 sets REGION=us-east-1, but lines 104, 110, and 117 reference the undefined variable ${AWS_REGION}, which will result in malformed AWS CLI commands.
Apply this diff to fix the variable reference:
REGION=us-east-1
# Partial match for RDS instance name (e.g., "polygon" matches "acme-prod-polygon-data")
DB_INSTANCE_SUBSTRING="polygon"
LOCAL_PORT=15432
# 1) Find the running SSM gateway instance ID by tag
INSTANCE_ID="$(
aws ec2 describe-instances \
--filters "Name=tag:Name,Values=*ssm*" "Name=instance-state-name,Values=running" \
--query 'Reservations[*].Instances[*].InstanceId' \
- --region ${AWS_REGION} \
+ --region ${REGION} \
--output text)"
# 2) Find the RDS instance endpoint
RDS_ENDPOINT="$(
aws rds describe-db-instances \
- --region ${AWS_REGION} \
+ --region ${REGION} \
--query "DBInstances[?contains(DBInstanceIdentifier, '${DB_INSTANCE_SUBSTRING}')].Endpoint.Address | [0]" \
--output text)"
# 3) Dynamically get the RDS port
RDS_PORT="$(
aws rds describe-db-instances \
- --region ${AWS_REGION} \
+ --region ${REGION} \
--query "DBInstances[?contains(DBInstanceIdentifier, '${DB_INSTANCE_SUBSTRING}')].Endpoint.Port | [0]" \
--output text)"Also applies to: 104-104, 110-110, 117-117
🤖 Prompt for AI Agents
In README.md around lines 94, 104, 110 and 117, the script defines
REGION=us-east-1 but later references the undefined variable ${AWS_REGION};
update the references at lines 104, 110 and 117 to use ${REGION} (or change the
definition to AWS_REGION=us-east-1 if you prefer that name) so the AWS CLI
commands use the defined variable consistently; ensure all occurrences in those
lines are corrected to the same variable name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed the env var. I'm using AWS_REGION in all places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@westonplatter, thank you for fixing the variable naming! Using AWS_REGION consistently throughout the script will ensure the AWS CLI commands execute correctly. This makes the script more robust and prevents the errors that would have occurred with undefined variables.
🐰✨
((
( -.-)
o_(")(")
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
what
references
Summary by CodeRabbit