Skip to content

Fix instance profile leak #4879

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

Open
jupierce wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix instance profile leak #4879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9d0315e
Use cloud formation for static instance profile
jupierce Dec 24, 2025
5bd2a02
Retain resources on cloud formation deletion
jupierce Dec 24, 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
44 changes: 38 additions & 6 deletions hack/reduce_nat_gateway_cost/deploy.sh
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,26 @@

set -ux

if [ "${1:-}" != 'destroy' ]; then
# Change to the directory where this script is located
cd "$(dirname "$0")"
Comment on lines +5 to +6
Copy link

@coderabbitai coderabbitai bot Dec 25, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Add error handling for directory change.

The cd command can fail if the directory doesn't exist or is inaccessible, which would cause the script to continue in the wrong directory and potentially deploy to the wrong location.

🔎 Proposed fix 
 # Change to the directory where this script is located
-cd "$(dirname "$0")"
+cd "$(dirname "$0")" || exit 1
📝 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
# Change to the directory where this script is located
cd "$(dirname "$0")"
# Change to the directory where this script is located
cd "$(dirname "$0")" || exit 1
🧰 Tools
🪛 Shellcheck (0.11.0)

[warning] 6-6: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)

🤖 Prompt for AI Agents 
In hack/reduce_nat_gateway_cost/deploy.sh around lines 5-6, the script blindly
runs cd "$(dirname "$0")" which can fail; update the script to check the result
of the cd and abort on failure: attempt to change to the script directory, and
if cd returns a non-zero status print a descriptive error to stderr and exit
with a non-zero status to avoid continuing in the wrong directory.


aws cloudformation deploy \
usage() {
echo "Usage: $0 <aws-profile> [destroy]"
echo " aws-profile: AWS CLI profile name to use"
echo " destroy: Optional - if specified, deletes the stack instead of deploying"
exit 1
}

if [ -z "${1:-}" ]; then
usage
fi

AWS_PROFILE="$1"
ACTION="${2:-deploy}"

if [ "$ACTION" != 'destroy' ]; then

aws --profile "$AWS_PROFILE" cloudformation deploy \
--stack-name use-nat-instance \
--template-file use-nat-instance.yaml \
--region us-east-1 \
Expand All @@ -13,10 +30,13 @@ if [ "${1:-}" != 'destroy' ]; then
rm -f lambda.zip
zip -r lambda.zip replace_nat_with_nat_instance.py

aws lambda update-function-code --function-name use-nat-instance-function --zip-file fileb://lambda.zip
aws --profile "$AWS_PROFILE" lambda update-function-code \
--function-name use-nat-instance-function \
--zip-file fileb://lambda.zip \
--region us-east-1

for region in us-east-2 us-west-1 us-west-2; do
aws cloudformation deploy \
aws --profile "$AWS_PROFILE" cloudformation deploy \
--stack-name use-nat-instance-forwarder \
--template-file use-nat-instance-forwarders.yaml \
--capabilities CAPABILITY_NAMED_IAM \
Expand All @@ -25,11 +45,23 @@ if [ "${1:-}" != 'destroy' ]; then

else

aws cloudformation delete-stack --stack-name use-nat-instance --region us-east-1
# Delete the Lambda function explicitly since it has DeletionPolicy: Retain.
# This is the critical resource - without it, NAT instance replacement stops.
echo "Deleting Lambda function use-nat-instance-function..."
aws --profile "$AWS_PROFILE" lambda delete-function \
--function-name use-nat-instance-function \
--region us-east-1 2>/dev/null || echo "Lambda function not found or already deleted"

# Delete CloudFormation stacks (other resources will be retained due to DeletionPolicy)
aws --profile "$AWS_PROFILE" cloudformation delete-stack --stack-name use-nat-instance --region us-east-1
for region in us-east-2 us-west-1 us-west-2; do
aws cloudformation delete-stack \
aws --profile "$AWS_PROFILE" cloudformation delete-stack \
--stack-name use-nat-instance-forwarder \
--region $region
done

echo ""
echo "NOTE: Other resources (IAM roles, instance profile, log group, event rule) were retained."
echo "To fully clean up, manually delete these resources or redeploy without DeletionPolicy: Retain."

fi
Loading