Completely ignore Instance Metadata when in SQS Queue mode. #735
Conversation
pkg/ec2metadata/ec2metadata.go
Outdated
| } else { | ||
| metadata.AccountId = "" | ||
| metadata.InstanceID = "" | ||
| metadata.InstanceLifeCycle = "" | ||
| metadata.InstanceType = "" | ||
| metadata.PublicHostname = "" | ||
| metadata.PublicIP = "" | ||
| metadata.LocalHostname = "" | ||
| metadata.LocalIP = "" | ||
| metadata.AvailabilityZone = "" | ||
| metadata.Region = "" | ||
| } |
There was a problem hiding this comment.
Is this else branch necessary? When imdsDisabled=false, metadata should still be empty, since no properties were set. When you were testing, did you discover this was required?
There was a problem hiding this comment.
He complained about metadata being uninitialized so I've set it to at least empty string values.
There was a problem hiding this comment.
The aws-node-termination-handler process. I ran it both on AWS EKS and on my workstation's local k8s cluster.
There was a problem hiding this comment.
Ah, right. If you replace line 329 with the following, it will initialize the whole struct and you won't need to set each property inidividually:
metadata := NodeMetadata{}
There was a problem hiding this comment.
This unit test needs an update: https://github.com/aws/aws-node-termination-handler/blob/main/pkg/ec2metadata/ec2metadata_test.go#L583
Also, if possible, please add a new unit test to verify that IMDS does not get called when calling GetNodeMetadata() in Queue Processor mode.
cmd/node-termination-handler.go
Outdated
|
|
||
| imds := ec2metadata.New(nthConfig.MetadataURL, nthConfig.MetadataTries) | ||
|
|
||
| interruptionEventStore := interruptioneventstore.New(nthConfig) | ||
| nodeMetadata := imds.GetNodeMetadata() | ||
| nodeMetadata := imds.GetNodeMetadata(!imdsDisabled) |
There was a problem hiding this comment.
Given that both the variable and the function parameter are called "disabled", shouldn't this line be the following?
| nodeMetadata := imds.GetNodeMetadata(!imdsDisabled) | |
| nodeMetadata := imds.GetNodeMetadata(imdsDisabled) |
There was a problem hiding this comment.
Right, good catch
There was a problem hiding this comment.
To clarify: GetNodeMetadata() does get called in queue mode with the current code as per this MR. Unfortunately my Gelang skills are not sufficient to come up with an elegant refactoring that truly separates the code parts.
|
Unfortunately I am not able to run the tests locally, as localstack doesn't come up properly: 🥑 Using localstack pod localstack-7f5f94f966-qbdr5 Could not connect to the endpoint URL: "http://localhost:4597/" |
There was a problem hiding this comment.
Three changes still needed:
- Update the
NodeMetadatainitialization logic (see this comment) - Add a unit test for
GetNodeMetadata()when IMDS is disabled - Update the README to indicate that we no longer query IMDS on startup when using Queue Processor mode
I made these changes in a commit on my fork: snay2@d2b2256
You can cherry-pick that commit onto your branch and update the PR. Otherwise, I can push the commit directly to your branch, whichever you prefer.
pkg/ec2metadata/ec2metadata.go
Outdated
| } else { | ||
| metadata.AccountId = "" | ||
| metadata.InstanceID = "" | ||
| metadata.InstanceLifeCycle = "" | ||
| metadata.InstanceType = "" | ||
| metadata.PublicHostname = "" | ||
| metadata.PublicIP = "" | ||
| metadata.LocalHostname = "" | ||
| metadata.LocalIP = "" | ||
| metadata.AvailabilityZone = "" | ||
| metadata.Region = "" | ||
| } |
There was a problem hiding this comment.
Ah, right. If you replace line 329 with the following, it will initialize the whole struct and you won't need to set each property inidividually:
metadata := NodeMetadata{}
Update README to reflect new behavior. Fix NodeMetadata initialization logic.
|
@snay2 I cherry-picked your commit. Your assistance on this matter is much appreciated. |
Issue #, if available:
Fixes #732
When IMDSv2 is enabled and aws-note-termination-handler is ran inside a container accessing IMDSv2 is not possible due to the IMDSv2 HTTP endpoint's default hop limit of 1. This makes the process exit.
Description of changes:
Completely disables the communication with the IMDS endpoint when in SQS queue mode.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.