Fix instance profile leak #4879
Conversation
The old method of creating a deleting instance profiles was complex and led to a leak which debilitated AWS temporarily. Have all instances share the same instance profile and manage it via cloud formations.
Cloud formation IDs are too difficult to keep in sync with PCO pruning. Retain resources and just delete the lambda explicitly on "destroy"
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughAdds a monitoring script, enhances the deploy script with profile/action handling, refactors NAT-instance provisioning to rely on an existing instance profile and stronger guards, updates CloudFormation templates to add IAM role/profile and DeletionPolicy: Retain entries, and adds AGENTS.md documentation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
hack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.py (1)
659-740: Enhanced error handling with iterative instance type selection.The refactored logic properly handles instance type availability by iterating through fallback options and ensures security group cleanup on failures. The approach aligns well with the CloudFormation-managed instance profile strategy.
Consider these improvements based on static analysis:
- Line 724: Use
logging.exceptioninstead oflogging.errorto automatically include stack trace.- Lines 728, 738: Replace bare
Exceptioncatches with specific exception types or at least log the exception details.🔎 Proposed refinements
except Exception as e: # If instance creation fails, clean up the security group we created - logger.error(f'Failed to create NAT instance: {e}. Cleaning up security group {nat_sg_id}') + logger.exception(f'Failed to create NAT instance: {e}. Cleaning up security group {nat_sg_id}') try: ec2_client.delete_security_group(GroupId=nat_sg_id) logger.info(f'Deleted orphaned security group {nat_sg_id}') - except Exception as cleanup_error: + except botocore.exceptions.ClientError as cleanup_error: logger.warning(f'Failed to clean up security group {nat_sg_id}: {cleanup_error}') raise if not nat_instance: # All instance types failed - clean up the security group logger.error(f'All instance types failed. Cleaning up security group {nat_sg_id}') try: ec2_client.delete_security_group(GroupId=nat_sg_id) logger.info(f'Deleted orphaned security group {nat_sg_id}') - except Exception as cleanup_error: + except botocore.exceptions.ClientError as cleanup_error: logger.warning(f'Failed to clean up security group {nat_sg_id}: {cleanup_error}') return Nonehack/reduce_nat_gateway_cost/monitor_resources.py (1)
343-466: Well-structured orchestration with good visibility.The main check function properly coordinates all monitoring checks and provides clear, color-coded output. The aggregation of issues across profiles is well done.
Consider addressing these minor style improvements:
- Line 369: The
present_resourcesvariable is unpacked but never used- Line 374: The
ip_countvariable is unpacked but never used- Line 448: The
all_healthyvariable is assigned but never used🔎 Proposed refinements
# Check expected resources exist - missing_resources, present_resources = check_expected_resources(profile) + missing_resources, _ = check_expected_resources(profile) if missing_resources: profile_issues.extend([f" {r}" for r in missing_resources]) # Check instance profile count - ip_count, ip_warning = check_instance_profile_count(profile) + _, ip_warning = check_instance_profile_count(profile) if ip_warning: profile_issues.append(ip_warning)For line 448, either remove the variable or use it:
# Display infrastructure health summary print(f"{CYAN}Infrastructure Health:{NC}") - all_healthy = True for profile in PROFILES: missing, present = check_expected_resources(profile) if missing: print(f" {profile}: {RED}UNHEALTHY - {len(missing)} missing resource(s){NC}") - all_healthy = False else: print(f" {profile}: {GREEN}OK ({len(present)} resources){NC}") print()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
hack/reduce_nat_gateway_cost/deploy.shhack/reduce_nat_gateway_cost/monitor_resources.pyhack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.pyhack/reduce_nat_gateway_cost/use-nat-instance-forwarders.yamlhack/reduce_nat_gateway_cost/use-nat-instance.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
hack/reduce_nat_gateway_cost/deploy.shhack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.pyhack/reduce_nat_gateway_cost/monitor_resources.pyhack/reduce_nat_gateway_cost/use-nat-instance-forwarders.yamlhack/reduce_nat_gateway_cost/use-nat-instance.yaml
🧬 Code graph analysis (1)
hack/reduce_nat_gateway_cost/monitor_resources.py (1)
hack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.py (1)
main(1061-1081)
🪛 Ruff (0.14.10)
hack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.py
724-724: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
728-728: Do not catch blind exception: Exception
(BLE001)
738-738: Do not catch blind exception: Exception
(BLE001)
hack/reduce_nat_gateway_cost/monitor_resources.py
49-49: Starting a process with a partial executable path
(S607)
55-55: Starting a process with a partial executable path
(S607)
65-65: Starting a process with a partial executable path
(S607)
86-86: Consider moving this statement to an else block
(TRY300)
256-256: Consider moving this statement to an else block
(TRY300)
369-369: Unpacked variable present_resources is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
374-374: Unpacked variable ip_count is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
401-401: Consider [f"Profile {profile}:", *profile_issues] instead of concatenation
Replace with [f"Profile {profile}:", *profile_issues]
(RUF005)
413-413: Consider ["NAT Instance Age Issues:", *age_issues] instead of concatenation
Replace with ["NAT Instance Age Issues:", *age_issues]
(RUF005)
448-448: Local variable all_healthy is assigned to but never used
Remove assignment to unused variable all_healthy
(F841)
🪛 Shellcheck (0.11.0)
hack/reduce_nat_gateway_cost/deploy.sh
[warning] 6-6: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🔇 Additional comments (31)
hack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.py (8)
118-119: LGTM!The updated comments accurately reflect the simplified IAM permission model where the Lambda only needs
iam:PassRolefor the pre-existing CloudFormation-managed instance profile, rather than the broader IAM permissions previously required for dynamic profile creation.
151-154: LGTM!The constant correctly references the CloudFormation-managed instance profile and aligns with the profile name defined in
use-nat-instance.yaml.
278-280: LGTM!Good defensive coding to handle cases where the NAT gateway may not have been fully tagged during creation.
301-301: LGTM!Proper guard against
Nonefromget_tagprevents potentialAttributeErroron string operations.
333-333: LGTM!Proper guard against
Nonefromget_tagprevents potentialAttributeErroron string operations.
500-502: LGTM!Proper error handling with early return prevents downstream issues when NAT instance creation fails. Security group cleanup is handled within
create_nat_instance, ensuring no resource leaks.
644-645: LGTM!Improved error logging with appropriate severity level and context.
963-963: LGTM!More specific exception handling improves code clarity and prevents accidentally catching unrelated exceptions.
hack/reduce_nat_gateway_cost/use-nat-instance-forwarders.yaml (2)
7-7: LGTM!
DeletionPolicy: Retainon the IAM role prevents disruption to event forwarding when the CloudFormation stack is deleted. This aligns with the PR's strategy to avoid synchronization issues between CloudFormation and PCO pruning.
37-37: LGTM!
DeletionPolicy: Retainon the EventBridge rule ensures continued event forwarding even when the CloudFormation stack is deleted. This is consistent with the PR's retention policy approach.hack/reduce_nat_gateway_cost/deploy.sh (3)
8-21: LGTM!Clear usage instructions and proper argument validation improve the script's usability and safety for multi-account deployments.
24-44: LGTM!Consistent use of
--profileensures operations target the correct AWS account. The deployment flow is well-structured.
48-66: LGTM!The destroy logic properly handles the DeletionPolicy: Retain resources by explicitly deleting the Lambda function (the critical component) while providing clear user guidance about retained resources. The approach is safe and well-documented.
hack/reduce_nat_gateway_cost/use-nat-instance.yaml (7)
11-49: LGTM!The new IAM role and instance profile correctly implement the least-privilege principle for NAT instance operations. The permissions are appropriately scoped:
ec2:ReplaceRouteallows the NAT instance to update route tablesec2:CreateTagsis restricted to route-table resourcesec2:ModifyInstanceAttributeenables disabling source/dest checksThe
DeletionPolicy: Retainprevents resource cleanup issues during stack deletion, aligning with the PR's strategy to avoid CloudFormation/PCO pruning synchronization problems.
52-52: LGTM!Retention policy prevents accidental deletion of the Lambda execution role, maintaining infrastructure stability.
83-83: LGTM!Narrowing the
PassRolepermission from wildcard to the specificNatInstanceRoleARN improves security by following the principle of least privilege.
86-86: LGTM!Retention policy ensures Lambda logs are preserved even after stack deletion.
92-92: LGTM!Retention policy on the Lambda function is critical - as the deploy script notes, this is explicitly deleted on destroy. Retaining it in CloudFormation prevents accidental deletion while allowing controlled removal.
121-121: LGTM!Retention policy prevents accidental deletion of the Lambda execution role used by EventBridge.
147-147: LGTM!Retention policy ensures the EventBridge rule continues forwarding events even after stack deletion.
hack/reduce_nat_gateway_cost/monitor_resources.py (11)
1-41: LGTM!Well-structured monitoring script with clear configuration constants and proper documentation. The multi-account, multi-region approach is appropriate for the use case.
43-75: LGTM!The alarm function provides useful feedback with graceful fallbacks across different operating systems. Good user experience.
77-91: LGTM!Utility functions are well-implemented with proper error handling for VPC existence checks.
93-157: LGTM!Thorough orphaned resource detection with proper VPC validation. The function correctly returns both issues and NAT instance details for age checking.
159-187: LGTM!IAM resource leak detection properly identifies the "Created-" prefix pattern that indicates dynamically created resources that should have been cleaned up.
189-206: LGTM!Instance profile count monitoring with appropriate threshold warning helps prevent hitting AWS service limits.
208-226: LGTM!NAT instance age checking with proper timezone handling ensures instances aren't left running indefinitely.
228-259: LGTM!CloudWatch metrics integration for Lambda error tracking is well-implemented with appropriate error handling.
261-269: LGTM!Simple region-wise counting provides useful visibility into resource distribution.
271-341: LGTM!Comprehensive resource existence checks across Lambda, IAM, and EventBridge resources with proper error handling for different AWS service exceptions.
468-515: LGTM!Clean CLI interface with appropriate argument parsing and proper signal handling for continuous monitoring mode.
| # Change to the directory where this script is located | ||
| cd "$(dirname "$0")" |
There was a problem hiding this comment.
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.
| # 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.
|
/retest |
|
/test all |
|
Scheduling required tests: Scheduling tests matching the |
|
/test e2e |
Prucek
left a comment
Prucek
left a comment
There was a problem hiding this comment.
/hold
You can unhold whenever you are ready
openshift-ci
bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jupierce, Prucek The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
approved
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
hack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.py (2)
301-301: Consider refactoring for readability.The control plane detection logic is correct but complex. Consider extracting it into a helper function for improved clarity:
def is_control_plane_instance(cluster_role: Optional[str], instance_name: Optional[str]) -> bool: """Check if instance is a control plane node based on tags.""" # Modern identification via cluster_role tag if cluster_role: return cluster_role in ['master', 'control-plane'] # Legacy identification via instance name return instance_name is not None and '-master' in instance_name # Then use: if not is_control_plane_instance(cluster_role, instance_name): return
696-777: Consider using logging.exception for better error diagnostics.The retry and cleanup logic is well-structured, but Line 761 should use
logging.exceptioninstead oflogging.errorto include the full traceback, which aids debugging.🔎 Suggested improvement
except Exception as e: # If instance creation fails, clean up the security group we created - logger.error(f'Failed to create NAT instance: {e}. Cleaning up security group {nat_sg_id}') + logger.exception(f'Failed to create NAT instance. Cleaning up security group {nat_sg_id}') try: ec2_client.delete_security_group(GroupId=nat_sg_id) logger.info(f'Deleted orphaned security group {nat_sg_id}')The broad exception handling is appropriate here to ensure security group cleanup occurs even for unexpected errors.
Based on learnings, using logging.exception provides better diagnostics.
hack/reduce_nat_gateway_cost/monitor_resources.py (1)
458-615: Address unused variables and optimize string operations.The monitoring logic is comprehensive, but several minor issues flagged by static analysis should be addressed:
- Lines 484, 489: Unused unpacked variables should be prefixed with underscore
- Lines 516, 528: Use spread operator instead of concatenation
- Line 547: Remove unnecessary f-string prefix
- Line 597: Variable
all_healthyis computed but never used🔎 Proposed fixes
# Check expected resources exist - missing_resources, present_resources = check_expected_resources(profile) + missing_resources, _ = check_expected_resources(profile) if missing_resources: profile_issues.extend([f" {r}" for r in missing_resources]) # Check instance profile count - ip_count, ip_warning = check_instance_profile_count(profile) + _, ip_warning = check_instance_profile_count(profile) if ip_warning: profile_issues.append(ip_warning)if profile_issues: print(f"{RED}ISSUES FOUND:{NC}") for issue in profile_issues: print(issue) - all_issues.extend([f"Profile {profile}:"] + profile_issues) + all_issues.extend([f"Profile {profile}:", *profile_issues]) has_problems = Trueif age_issues: print(f"{RED}NAT INSTANCE AGE ISSUES:{NC}") for issue in age_issues: print(issue) - all_issues.extend(["NAT Instance Age Issues:"] + age_issues) + all_issues.extend(["NAT Instance Age Issues:", *age_issues]) has_problems = Trueeffectiveness = check_nat_instance_effectiveness(all_nat_instances) if effectiveness["eligible_count"] == 0: - print(f" No NAT instances old enough to evaluate") + print(" No NAT instances old enough to evaluate") else:For the
all_healthyvariable at line 597: either use it to affect the has_problems flag or remove it if not needed:- all_healthy = True for profile in PROFILES: missing, present = check_expected_resources(profile) if missing: print(f" {profile}: {RED}UNHEALTHY - {len(missing)} missing resource(s){NC}") - all_healthy = False + has_problems = True else: print(f" {profile}: {GREEN}OK ({len(present)} resources){NC}")hack/reduce_nat_gateway_cost/AGENTS.md (1)
50-59: Fix fenced code block language specification.The file structure block should have a language identifier for proper rendering.
🔎 Suggested fix
-``` +```text hack/reduce_nat_gateway_cost/ ├── replace_nat_with_nat_instance.py # Main Lambda function code ├── use-nat-instance.yaml # CloudFormation template for us-east-1 ├── use-nat-instance-forwarders.yaml # CloudFormation template for other regions ├── deploy.sh # Deployment script ├── monitor_resources.py # Python monitoring script ├── lambda.zip # Packaged Lambda (auto-generated) └── AGENTS.md # This file</details> Otherwise, the documentation is comprehensive and well-organized. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Cache: Disabled due to data retention organization setting** **Knowledge base: Disabled due to `Reviews -> Disable Knowledge Base` setting** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 5bd2a021d651ce3628f9b662fcb3b05fa4409b6a and 81cd85662a7c4be37469ed6da52ba252131a9553. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `hack/reduce_nat_gateway_cost/AGENTS.md` * `hack/reduce_nat_gateway_cost/monitor_resources.py` * `hack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.py` </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (1)</summary> <details> <summary>**</summary> **⚙️ CodeRabbit configuration file** > -Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity. Files: - `hack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.py` - `hack/reduce_nat_gateway_cost/monitor_resources.py` - `hack/reduce_nat_gateway_cost/AGENTS.md` </details> </details><details> <summary>🧬 Code graph analysis (1)</summary> <details> <summary>hack/reduce_nat_gateway_cost/monitor_resources.py (1)</summary><blockquote> <details> <summary>hack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.py (1)</summary> * `main` (1116-1136) </details> </blockquote></details> </details><details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>hack/reduce_nat_gateway_cost/AGENTS.md</summary> 50-50: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> <details> <summary>🪛 Ruff (0.14.10)</summary> <details> <summary>hack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.py</summary> 761-761: Use `logging.exception` instead of `logging.error` Replace with `exception` (TRY400) --- 765-765: Do not catch blind exception: `Exception` (BLE001) --- 775-775: Do not catch blind exception: `Exception` (BLE001) </details> <details> <summary>hack/reduce_nat_gateway_cost/monitor_resources.py</summary> 50-50: Starting a process with a partial executable path (S607) --- 56-56: Starting a process with a partial executable path (S607) --- 66-66: Starting a process with a partial executable path (S607) --- 87-87: Consider moving this statement to an `else` block (TRY300) --- 257-257: Consider moving this statement to an `else` block (TRY300) --- 484-484: Unpacked variable `present_resources` is never used Prefix it with an underscore or any other dummy variable pattern (RUF059) --- 489-489: Unpacked variable `ip_count` is never used Prefix it with an underscore or any other dummy variable pattern (RUF059) --- 516-516: Consider `[f"Profile {profile}:", *profile_issues]` instead of concatenation Replace with `[f"Profile {profile}:", *profile_issues]` (RUF005) --- 528-528: Consider `["NAT Instance Age Issues:", *age_issues]` instead of concatenation Replace with `["NAT Instance Age Issues:", *age_issues]` (RUF005) --- 547-547: f-string without any placeholders Remove extraneous `f` prefix (F541) --- 597-597: Local variable `all_healthy` is assigned to but never used Remove assignment to unused variable `all_healthy` (F841) </details> </details> </details> <details> <summary>🔇 Additional comments (17)</summary><blockquote> <details> <summary>hack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.py (10)</summary><blockquote> `118-119`: **LGTM!** The updated permission comments accurately document the Lambda's IAM requirements for SSM parameter access and passing the NAT instance role to EC2. --- `151-154`: **LGTM!** The NAT_INSTANCE_PROFILE_NAME constant improves maintainability by centralizing the CloudFormation-created instance profile name, and the VERSION bump to v1.4 appropriately reflects the scope of changes. --- `278-280`: **LGTM!** The VPC ID validation prevents cleanup() from being called with missing data, which could cause errors downstream. The defensive check with early return is appropriate. --- `333-336`: **LGTM!** Filtering out bootstrap instance terminations prevents premature NAT instance cleanup, as bootstrap nodes are terminated early in the cluster lifecycle while other nodes continue running. --- `377-379`: **LGTM!** The validation ensures nat_instance_id is an actual EC2 instance ID (starts with 'i-') before using it to update routes. This prevents errors if the userData script failed and the tag still contains a NAT Gateway ID. --- `502-505`: **LGTM!** The error handling prevents downstream failures by checking for None return from create_nat_instance and logging appropriately. --- `539-597`: **LGTM!** The race condition handling for security group creation is well-implemented. It checks for existing groups, handles InvalidGroup.Duplicate errors gracefully, and falls back to fetching the existing group when concurrent invocations create duplicates. --- `681-682`: **LGTM!** The timeout error handling for route table availability is appropriate and returns None to signal failure, which is properly checked by the caller. --- `897-926`: **LGTM!** The race condition handling in the userData script is a critical improvement. It waits up to 5 minutes for the 0.0.0.0/0 route to exist before attempting to replace it, which addresses the timing issue where the NAT instance may start before the cluster installer creates the route. --- `1018-1018`: **LGTM!** The specific exception type (ClientError) is more precise than broad exception handling, which is a best practice improvement. </blockquote></details> <details> <summary>hack/reduce_nat_gateway_cost/monitor_resources.py (7)</summary><blockquote> `44-76`: **LGTM!** The alarm function has appropriate fallback logic and timeout protection. The static analysis warnings about subprocess paths (S607) are false positives here, as the commands and file paths are hardcoded system utilities, not user input. --- `83-92`: **LGTM!** The VPC existence check is correct and clear. The static analysis suggestion (TRY300) about using an else block is a minor style preference; the current structure is fine. --- `94-158`: **LGTM!** The orphaned resource detection logic correctly identifies resources whose VPCs no longer exist, with appropriate error handling and useful metadata collection for downstream analysis. --- `229-260`: **LGTM!** The Lambda error count retrieval correctly uses CloudWatch metrics with appropriate error handling. The static analysis suggestion (TRY300) is a minor style preference; the current structure is clear. --- `272-384`: **LGTM!** The effectiveness check is well-designed with efficient API batching, proper route validation (checking for ENI in 0.0.0.0/0 routes), and comprehensive error handling. --- `386-456`: **LGTM!** The resource validation comprehensively checks all expected infrastructure components across regions with appropriate error handling. --- `617-664`: **LGTM!** The main function correctly handles both one-shot and continuous monitoring modes with appropriate exit codes and keyboard interrupt handling. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
Scheduling required tests: Scheduling tests matching the |
jupierce
force-pushed
the
fix_instance_profile_leak
branch
from
81cd856 to
f7fe819
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.py (1)
550-633: Security group race condition handling is improved, but ingress rules may fail on reuse.When an existing security group is reused (lines 558-561 or 590-593), the subsequent
authorize_security_group_ingresscalls (lines 600-632) will fail withInvalidPermission.Duplicateif the rules already exist.🔎 Proposed fix to handle duplicate ingress rules
+ # Skip ingress rules if we reused an existing security group + # (rules would already be configured) + if existing_sgs.get("SecurityGroups"): + return nat_sg_id + # Allow inbound traffic from the private subnet - ec2_client.authorize_security_group_ingress( + try: + ec2_client.authorize_security_group_ingress(Alternatively, wrap each
authorize_security_group_ingresscall in a try/except that handlesInvalidPermission.Duplicate:# Allow inbound traffic from the private subnet - ec2_client.authorize_security_group_ingress( - GroupId=nat_sg_id, - IpPermissions=[ - { - "IpProtocol": "-1", # Allow all traffic - "IpRanges": [{"CidrIp": private_cidr}] - } - ] - ) + try: + ec2_client.authorize_security_group_ingress( + GroupId=nat_sg_id, + IpPermissions=[ + { + "IpProtocol": "-1", # Allow all traffic + "IpRanges": [{"CidrIp": private_cidr}] + } + ] + ) + except botocore.exceptions.ClientError as e: + if 'InvalidPermission.Duplicate' not in str(e): + raise
🧹 Nitpick comments (4)
hack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.py (1)
759-777: Good cleanup on failure, but uselogger.exceptionfor better diagnostics.The cleanup pattern correctly prevents orphaned security groups. Per Ruff TRY400, use
logger.exceptioninstead oflogger.errorto automatically include the stack trace.🔎 Proposed fix
except Exception as e: # If instance creation fails, clean up the security group we created - logger.error(f'Failed to create NAT instance: {e}. Cleaning up security group {nat_sg_id}') + logger.exception(f'Failed to create NAT instance. Cleaning up security group {nat_sg_id}') try: ec2_client.delete_security_group(GroupId=nat_sg_id) logger.info(f'Deleted orphaned security group {nat_sg_id}') - except Exception as cleanup_error: + except botocore.exceptions.ClientError as cleanup_error: logger.warning(f'Failed to clean up security group {nat_sg_id}: {cleanup_error}') raisehack/reduce_nat_gateway_cost/monitor_resources.py (3)
567-572: Minor: Unused variables could be prefixed with underscore.Per Ruff RUF059,
present_resourcesandip_countare unpacked but never used. Either use them or prefix with underscore to indicate intentional discard.🔎 Proposed fix
# Check expected resources exist - missing_resources, present_resources = check_expected_resources(profile) + missing_resources, _present_resources = check_expected_resources(profile) if missing_resources: profile_issues.extend([f" {r}" for r in missing_resources]) # Check instance profile count - ip_count, ip_warning = check_instance_profile_count(profile) + _ip_count, ip_warning = check_instance_profile_count(profile)
630-630: Minor: f-string without placeholders.Per Ruff F541, remove the extraneous
fprefix.🔎 Proposed fix
- print(f" No NAT instances old enough to evaluate") + print(" No NAT instances old enough to evaluate")
688-698:all_healthyvariable is assigned but never used.Consider either removing it or using it (e.g., to influence the return value or final summary).
🔎 Proposed fix - remove unused variable
# Display infrastructure health summary print(f"{CYAN}Infrastructure Health:{NC}") - all_healthy = True for profile in PROFILES: missing, present = check_expected_resources(profile) if missing: print(f" {profile}: {RED}UNHEALTHY - {len(missing)} missing resource(s){NC}") - all_healthy = False else: print(f" {profile}: {GREEN}OK ({len(present)} resources){NC}")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
hack/reduce_nat_gateway_cost/AGENTS.mdhack/reduce_nat_gateway_cost/monitor_resources.pyhack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.pyhack/reduce_nat_gateway_cost/use-nat-instance.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
hack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.pyhack/reduce_nat_gateway_cost/use-nat-instance.yamlhack/reduce_nat_gateway_cost/monitor_resources.pyhack/reduce_nat_gateway_cost/AGENTS.md
🧬 Code graph analysis (1)
hack/reduce_nat_gateway_cost/monitor_resources.py (1)
hack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.py (1)
main(1116-1136)
🪛 markdownlint-cli2 (0.18.1)
hack/reduce_nat_gateway_cost/AGENTS.md
50-50: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.10)
hack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.py
761-761: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
765-765: Do not catch blind exception: Exception
(BLE001)
775-775: Do not catch blind exception: Exception
(BLE001)
hack/reduce_nat_gateway_cost/monitor_resources.py
50-50: Starting a process with a partial executable path
(S607)
56-56: Starting a process with a partial executable path
(S607)
66-66: Starting a process with a partial executable path
(S607)
87-87: Consider moving this statement to an else block
(TRY300)
257-257: Consider moving this statement to an else block
(TRY300)
286-286: Consider moving this statement to an else block
(TRY300)
340-340: Consider moving this statement to an else block
(TRY300)
567-567: Unpacked variable present_resources is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
572-572: Unpacked variable ip_count is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
599-599: Consider [f"Profile {profile}:", *profile_issues] instead of concatenation
Replace with [f"Profile {profile}:", *profile_issues]
(RUF005)
611-611: Consider ["NAT Instance Age Issues:", *age_issues] instead of concatenation
Replace with ["NAT Instance Age Issues:", *age_issues]
(RUF005)
630-630: f-string without any placeholders
Remove extraneous f prefix
(F541)
695-695: Local variable all_healthy is assigned to but never used
Remove assignment to unused variable all_healthy
(F841)
🔇 Additional comments (14)
hack/reduce_nat_gateway_cost/AGENTS.md (1)
1-314: Comprehensive agent reference documentation - LGTM!This is a well-structured operational guide covering deployment, monitoring, troubleshooting, and common issues. The documentation aligns well with the code changes in this PR.
hack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.py (7)
151-154: Key fix for instance profile leak - using static CloudFormation-managed profile.This change eliminates the dynamic instance profile creation that was causing leaks. The profile name correctly matches the
NatInstanceProfileresource in use-nat-instance.yaml.
278-281: Good defensive guard against missing VPC tag.This prevents the cleanup function from running with a None vpc_id, which could cause issues or incorrect resource targeting.
301-302: Complex but correct control plane detection logic.The conditional handles both the explicit
cluster-api-provider-aws/roletag and falls back to name-based detection with-masterin the name. Consider extracting this to a helper function for readability in a future refactor.
333-336: Good safeguard against premature NAT cleanup during bootstrap.Correctly prevents NAT instance termination when the bootstrap machine is terminated, which is expected cluster lifecycle behavior.
377-379: Good validation to prevent route misconfiguration.This check ensures the route table update uses an actual instance ID rather than a NAT Gateway ID, which could happen if the userData script failed.
680-682: Good use of logger for timeout error.Consistent with the rest of the codebase's logging approach.
901-926: Excellent fix for the route creation race condition.The retry loop (up to 5 minutes) correctly handles the timing issue where the NAT instance may start before the cluster installer creates the 0.0.0.0/0 route. This addresses the "There is no route defined" error mentioned in AGENTS.md.
hack/reduce_nat_gateway_cost/use-nat-instance.yaml (3)
11-50: Core fix for instance profile leak - static IAM resources.The new
NatInstanceRoleandNatInstanceProfilereplace the dynamic creation approach. TheDeletionPolicy: Retainis important to prevent accidental service disruption. The profile name correctly matchesNAT_INSTANCE_PROFILE_NAMEin the Lambda code.
81-84: Good security improvement - PassRole restriction.Restricting
iam:PassRoleto only the specificNatInstanceRoleARN instead of a wildcard pattern follows the principle of least privilege.
51-168: Consistent DeletionPolicy: Retain pattern across all critical resources.This prevents accidental service disruption during stack updates or deletions, as documented in AGENTS.md. The
destroycommand in deploy.sh explicitly handles cleanup when needed.hack/reduce_nat_gateway_cost/monitor_resources.py (3)
19-34: Configuration aligns well with the CloudFormation template and AGENTS.md.The expected resource names correctly match the CloudFormation resources, enabling accurate health checks.
44-75: Alarm function has good fallback chain.The partial executable paths (
paplay,afplay) are acceptable for a developer utility script. The fallback to terminal bell ensures functionality across environments.
429-448: Fix route checking to match route creation method.The effectiveness check looks for
NetworkInterfaceIdin routes (line 434), but routes are created usingInstanceIdparameter (replace_nat_with_nat_instance.py line 395). When routes are created withInstanceId, the route object will have theInstanceIdfield populated, notNetworkInterfaceId. This causes the condition to always fail, reporting all NAT instances as ineffective even when routes have been properly updated.Change line 434 to check
route.get("InstanceId") == instance_idinstead, sinceinstance_idis already available from line 400.
jupierce
force-pushed
the
fix_instance_profile_leak
branch
from
f7fe819 to
7f40fa2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @hack/reduce_nat_gateway_cost/monitor_resources.py:
- Around line 429-448: The route-check incorrectly compares
route["NetworkInterfaceId"] to eni_id; change it to check
route.get("InstanceId") against the NAT instance's InstanceId (e.g.,
inst.get("InstanceId")) and optionally fall back to NetworkInterfaceId==eni_id
for ENI-based routes so effective NAT instances created via --instance-id are
detected; update the condition inside the loop that inspects route.get(...)
accordingly and keep the existing found_route/ineffective_instances logic.
🧹 Nitpick comments (2)
hack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.py (1)
759-777: Good cleanup on instance creation failure.Cleaning up the security group when instance creation fails prevents orphaned resources.
Consider using
logging.exceptioninstead oflogging.erroron line 761 to include the traceback in logs, which aids debugging.🔎 Optional: Use logging.exception for better traceability
except Exception as e: # If instance creation fails, clean up the security group we created - logger.error(f'Failed to create NAT instance: {e}. Cleaning up security group {nat_sg_id}') + logger.exception(f'Failed to create NAT instance. Cleaning up security group {nat_sg_id}') try: ec2_client.delete_security_group(GroupId=nat_sg_id) logger.info(f'Deleted orphaned security group {nat_sg_id}') except Exception as cleanup_error: logger.warning(f'Failed to clean up security group {nat_sg_id}: {cleanup_error}') raisehack/reduce_nat_gateway_cost/monitor_resources.py (1)
567-572: Minor: Unused variables and unnecessary f-string.A few unused variables flagged by static analysis:
- Line 567:
present_resourcesis unused in the first call (but used on line 692)- Line 572:
ip_countis computed but never used- Line 630: f-string without placeholders
- Line 695:
all_healthyis assigned but never read🔎 Proposed cleanup
# Check expected resources exist - missing_resources, present_resources = check_expected_resources(profile) + missing_resources, _present_resources = check_expected_resources(profile) if missing_resources: profile_issues.extend([f" {r}" for r in missing_resources]) # Check instance profile count - ip_count, ip_warning = check_instance_profile_count(profile) + _ip_count, ip_warning = check_instance_profile_count(profile) if ip_warning: profile_issues.append(ip_warning)if effectiveness["eligible_count"] == 0: - print(f" No NAT instances old enough to evaluate") + print(" No NAT instances old enough to evaluate")print(f"{CYAN}Infrastructure Health:{NC}") - all_healthy = True for profile in PROFILES: missing, present = check_expected_resources(profile) if missing: print(f" {profile}: {RED}UNHEALTHY - {len(missing)} missing resource(s){NC}") - all_healthy = False else: print(f" {profile}: {GREEN}OK ({len(present)} resources){NC}")Also applies to: 630-630, 695-695
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
hack/reduce_nat_gateway_cost/AGENTS.mdhack/reduce_nat_gateway_cost/monitor_resources.pyhack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.pyhack/reduce_nat_gateway_cost/use-nat-instance.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
hack/reduce_nat_gateway_cost/use-nat-instance.yamlhack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.pyhack/reduce_nat_gateway_cost/monitor_resources.pyhack/reduce_nat_gateway_cost/AGENTS.md
🧬 Code graph analysis (1)
hack/reduce_nat_gateway_cost/monitor_resources.py (1)
hack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.py (1)
main(1116-1136)
🪛 LanguageTool
hack/reduce_nat_gateway_cost/AGENTS.md
[grammar] ~339-~339: Use a hyphen to join words.
Context: ...y:** EventBridge retries once with max 5 minute event age. 5. Regions: Lambd...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
hack/reduce_nat_gateway_cost/AGENTS.md
50-50: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.10)
hack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.py
761-761: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
765-765: Do not catch blind exception: Exception
(BLE001)
775-775: Do not catch blind exception: Exception
(BLE001)
hack/reduce_nat_gateway_cost/monitor_resources.py
50-50: Starting a process with a partial executable path
(S607)
56-56: Starting a process with a partial executable path
(S607)
66-66: Starting a process with a partial executable path
(S607)
87-87: Consider moving this statement to an else block
(TRY300)
257-257: Consider moving this statement to an else block
(TRY300)
286-286: Consider moving this statement to an else block
(TRY300)
340-340: Consider moving this statement to an else block
(TRY300)
567-567: Unpacked variable present_resources is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
572-572: Unpacked variable ip_count is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
599-599: Consider [f"Profile {profile}:", *profile_issues] instead of concatenation
Replace with [f"Profile {profile}:", *profile_issues]
(RUF005)
611-611: Consider ["NAT Instance Age Issues:", *age_issues] instead of concatenation
Replace with ["NAT Instance Age Issues:", *age_issues]
(RUF005)
630-630: f-string without any placeholders
Remove extraneous f prefix
(F541)
695-695: Local variable all_healthy is assigned to but never used
Remove assignment to unused variable all_healthy
(F841)
🔇 Additional comments (15)
hack/reduce_nat_gateway_cost/AGENTS.md (1)
1-356: Comprehensive documentation for the NAT Gateway cost reduction solution.This is well-structured documentation covering the solution overview, deployment, monitoring, and troubleshooting. The content aligns with the code changes in this PR.
Minor static analysis hints (optional fixes):
- Line 50: Consider adding a language specifier to the fenced code block (e.g.,
```text)- Line 339: "5 minute" → "5-minute" (hyphenation)
hack/reduce_nat_gateway_cost/replace_nat_with_nat_instance.py (8)
151-154: Good fix: Static instance profile replaces dynamic creation.Using a CloudFormation-managed instance profile (
use-nat-instance-profile) instead of dynamically creating profiles per NAT instance directly addresses the instance profile leak issue.
278-281: Good guard against missing VPC tag during cleanup.This prevents cleanup failures when the NAT gateway's VPC association is lost during deletion, which is a sensible defensive measure.
301-302: Control plane identification logic is correct.The conditional properly handles both the
sigs.k8s.io/cluster-api-provider-aws/roletag and fallback to name-based detection for master nodes.
333-336: Correct guard to prevent premature NAT cleanup on bootstrap termination.Bootstrap machine termination is a normal part of cluster installation and should not trigger NAT instance cleanup.
377-398: Good validation to ensure NAT instance ID is actually an instance.This prevents attempting to route traffic to a NAT Gateway ID if the userData script failed to update the route table tag properly.
548-597: Robust race condition handling for security group creation.The two-phase approach (check before create + handle duplicate error) properly handles concurrent Lambda invocations that might try to create the same security group.
901-926: Race condition fix: Wait for route before replacing.The retry loop (up to 5 minutes) properly handles the race condition where the NAT instance might start before the cluster installer creates the
0.0.0.0/0route. This aligns with the v1.3 fix mentioned in AGENTS.md.
1018-1026: Good: Specific exception handling for AWS API errors.Using
botocore.exceptions.ClientErrorinstead of a broadExceptioncatch is more appropriate here.hack/reduce_nat_gateway_cost/use-nat-instance.yaml (3)
11-50: Good: Static instance profile and role with minimal permissions.The
NatInstanceRolehas appropriately scoped permissions for what NAT instances need:
ec2:ReplaceRouteandec2:DescribeRouteTablesfor route managementec2:CreateTagsrestricted to route-table resourcesec2:ModifyInstanceAttributefor disabling source/dest check
DeletionPolicy: Retainon these resources prevents accidental deletion during stack updates.
81-84: Security improvement: PassRole now scoped to specific role.Restricting
iam:PassRoleto onlyNatInstanceRole.Arninstead of a wildcard pattern follows the principle of least privilege.
51-168: Consistent DeletionPolicy: Retain across all resources.This aligns with the documentation in AGENTS.md (line 108) explaining that retained resources prevent accidental service disruption. The
destroyaction in deploy.sh explicitly handles cleanup.hack/reduce_nat_gateway_cost/monitor_resources.py (3)
19-34: Good configuration with reasonable thresholds.The instance profile threshold of 500 provides adequate buffer before reaching the AWS limit of 1000. The NAT age threshold of 8 hours and effectiveness check at 15 minutes are sensible defaults.
44-75: Alarm function with appropriate fallbacks.The function handles different platforms (Linux/macOS) and falls back to terminal bell. The partial executable paths flagged by static analysis are appropriate for well-known system utilities.
715-761: Well-designed CLI with proper signal handling.The script supports both single-run (
--once) and continuous monitoring modes with appropriate exit codes for scripted use.
| # Check if any route table has a route pointing to this ENI | ||
| found_route = False | ||
| for rt in route_tables_response.get("RouteTables", []): | ||
| for route in rt.get("Routes", []): | ||
| # Check if this route points to our NAT instance's ENI | ||
| if route.get("NetworkInterfaceId") == eni_id: | ||
| # Verify it's for the default route or a significant route | ||
| if route.get("DestinationCidrBlock") == "0.0.0.0/0": | ||
| found_route = True | ||
| break | ||
| if found_route: | ||
| break | ||
|
|
||
| if found_route: | ||
| effective_count += 1 | ||
| else: | ||
| ineffective_instances.append({ | ||
| **inst, | ||
| "reason": "No route table updated to use this instance", | ||
| }) |
There was a problem hiding this comment.
Bug: Route check uses wrong attribute.
The route replacement in replace_nat_with_nat_instance.py (lines 392-396, 919) uses InstanceId, but this effectiveness check looks for NetworkInterfaceId. Routes created with --instance-id will have the InstanceId attribute set, not NetworkInterfaceId.
This will cause all effective NAT instances to be incorrectly reported as ineffective.
🔎 Proposed fix
# Check if any route table has a route pointing to this ENI
found_route = False
for rt in route_tables_response.get("RouteTables", []):
for route in rt.get("Routes", []):
- # Check if this route points to our NAT instance's ENI
- if route.get("NetworkInterfaceId") == eni_id:
+ # Check if this route points to our NAT instance
+ if route.get("InstanceId") == instance_id:
# Verify it's for the default route or a significant route
if route.get("DestinationCidrBlock") == "0.0.0.0/0":
found_route = True
break
if found_route:
break📝 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.
| # Check if any route table has a route pointing to this ENI | |
| found_route = False | |
| for rt in route_tables_response.get("RouteTables", []): | |
| for route in rt.get("Routes", []): | |
| # Check if this route points to our NAT instance's ENI | |
| if route.get("NetworkInterfaceId") == eni_id: | |
| # Verify it's for the default route or a significant route | |
| if route.get("DestinationCidrBlock") == "0.0.0.0/0": | |
| found_route = True | |
| break | |
| if found_route: | |
| break | |
| if found_route: | |
| effective_count += 1 | |
| else: | |
| ineffective_instances.append({ | |
| **inst, | |
| "reason": "No route table updated to use this instance", | |
| }) | |
| # Check if any route table has a route pointing to this ENI | |
| found_route = False | |
| for rt in route_tables_response.get("RouteTables", []): | |
| for route in rt.get("Routes", []): | |
| # Check if this route points to our NAT instance | |
| if route.get("InstanceId") == instance_id: | |
| # Verify it's for the default route or a significant route | |
| if route.get("DestinationCidrBlock") == "0.0.0.0/0": | |
| found_route = True | |
| break | |
| if found_route: | |
| break | |
| if found_route: | |
| effective_count += 1 | |
| else: | |
| ineffective_instances.append({ | |
| **inst, | |
| "reason": "No route table updated to use this instance", | |
| }) |
🤖 Prompt for AI Agents
In @hack/reduce_nat_gateway_cost/monitor_resources.py around lines 429 - 448,
The route-check incorrectly compares route["NetworkInterfaceId"] to eni_id;
change it to check route.get("InstanceId") against the NAT instance's InstanceId
(e.g., inst.get("InstanceId")) and optionally fall back to
NetworkInterfaceId==eni_id for ENI-based routes so effective NAT instances
created via --instance-id are detected; update the condition inside the loop
that inspects route.get(...) accordingly and keep the existing
found_route/ineffective_instances logic.
4 participants
No description provided.