Fix IMDS autoConfigureHopLimit to scope to VPC instead of region#42
Fix IMDS autoConfigureHopLimit to scope to VPC instead of region#42
Conversation
The tryVPCWideConfiguration() function was querying all running instances in the AWS region instead of filtering by VPC. This adds a vpc-id filter to the DescribeInstances call and resolves the current VPC ID lazily via private IP lookup before invoking the last-resort strategy. Also adds missing ec2:DescribeInstances and ec2:ModifyInstanceMetadataOptions permissions to SECURITY.md. Fixes #33 https://claude.ai/code/session_019cXyAQaLbKyaHWnZupcNUC
|
The CI failure in "Run Tests" is pre-existing and unrelated to this PR. The failing tests ( All tests in |
🤖 Augment PR SummarySummary: Fix IMDS hop-limit auto-configuration to only target instances in the controller’s VPC rather than the whole region. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| }, | ||
| } | ||
|
|
||
| result, err := c.EC2.DescribeInstances(ctx, input) |
There was a problem hiding this comment.
resolveCurrentVPCID will panic if c.EC2 is nil (e.g., when EC2Client is constructed without NewEC2Client), since it unconditionally calls c.EC2.DescribeInstances here; consider returning a clear error when the client isn't initialized.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
|
||
| for _, reservation := range result.Reservations { | ||
| for _, instance := range reservation.Instances { | ||
| if instance.VpcId != nil { |
There was a problem hiding this comment.
This returns the first VpcId from a private-IP lookup; if multiple instances match the same private IP across VPCs (overlapping CIDRs), this could select the wrong VPC and mis-scope the subsequent VPC-wide modifications—consider detecting ambiguity and failing.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| // This will fail because there's no EC2 client and the network interface | ||
| // lookup will either return a local IP or fail - either way, without a | ||
| // real EC2 client the DescribeInstances call would fail. | ||
| _, err := client.resolveCurrentVPCID(context.Background()) |
There was a problem hiding this comment.
This test can be flaky (or panic): if getPrivateIPFromNetworkInterface() finds any private IP on the test host, resolveCurrentVPCID() will reach client.EC2.DescribeInstances(...) while EC2 is nil.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| // returns an error when aggressive configuration is disabled. | ||
| func TestTryVPCWideConfiguration_AggressiveDisabled(t *testing.T) { | ||
| // Ensure aggressive configuration is disabled | ||
| os.Unsetenv("IMDS_AGGRESSIVE_CONFIGURATION") |
There was a problem hiding this comment.
These tests mutate process-wide environment variables via os.Unsetenv without restoring them, which can make other tests in the package order-dependent; consider using t.Setenv (or deferring restore) to avoid leakage.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
- Add nil check for c.EC2 in resolveCurrentVPCID to prevent panic - Detect ambiguous VPC resolution when private IP matches multiple VPCs - Replace os.Unsetenv with t.Setenv for test isolation - Replace flaky resolveCurrentVPCID test with nil EC2 client test https://claude.ai/code/session_019cXyAQaLbKyaHWnZupcNUC
|
augment review |
2 participants
Summary
tryVPCWideConfiguration()to filter by VPC ID instead of querying all running instances in the entire AWS region. Adds avpc-idfilter to theDescribeInstancescall and a newresolveCurrentVPCID()method that determines the VPC via private IP lookup before the last-resort strategy runs.ec2:DescribeInstancesandec2:ModifyInstanceMetadataOptionsIAM permissions.Test plan
go build ./...passesgo vet ./...passesgo test ./pkg/aws/...passesFixes #33