-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Support IPv6 addresses during fingerprinting #2536
Conversation
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.
Can we special case loopback:
"Resources": {
"CPU": 8709,
"DiskMB": 32814,
"IOPS": 0,
"MemoryMB": 2000,
"Networks": [
{
"CIDR": "127.0.0.1/32",
"DynamicPorts": null,
"IP": "127.0.0.1",
"MBits": 1000,
"Public": false,
"ReservedPorts": null
},
{
"CIDR": "::1/128",
"DynamicPorts": null,
"IP": "::1",
"MBits": 1000,
"Public": false,
"ReservedPorts": null
}
]
},
client/fingerprint/network.go
Outdated
// Add the network resources to the node | ||
for _, nwResource := range nwResources { | ||
node.Resources.Networks = append(node.Resources.Networks, nwResource) | ||
f.logger.Printf("[DEBUG] fingerprint.network: Detected interface %v with IP: %v, CIDR: %v during fingerprinting", |
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.
Remove during fingerprinting. The prefix is fingerprint.network
:)
client/fingerprint/network.go
Outdated
var ip net.IP | ||
switch v := (addr).(type) { | ||
case *net.IPNet: | ||
ip = v.IP | ||
newNetwork.IP = v.IP.String() |
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.
Doesn't this get overrides right below?
client/fingerprint/network.go
Outdated
node.Resources.Networks = append(node.Resources.Networks, newNetwork) | ||
// Add the network resources to the node | ||
for _, nwResource := range nwResources { | ||
node.Resources.Networks = append(node.Resources.Networks, nwResource) |
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.
node.Resources.Networks = nwResource
outside of the loop?
case *net.IPAddr: | ||
ip = v.IP | ||
} | ||
|
||
// If the ip is link-local then we ignore it | ||
if ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() { |
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.
Can we get a test around these and IPv4/6 CIDRs
return false, fmt.Errorf("Unable to find IP address of interface: %s, err: %v", intf.Name, err) | ||
} | ||
|
||
newNetwork.Device = intf.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.
Need to set device 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.
@dadgar Look at https://github.com/hashicorp/nomad/pull/2536/files#diff-e2a9235d588163b6bfb8e29d322e7e84R117 where we are initializing the structs.NetworkResource
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.
LGTM
client/fingerprint/network.go
Outdated
@@ -95,6 +95,7 @@ func (f *NetworkFingerprint) Fingerprint(cfg *config.Config, node *structs.Node) | |||
f.logger.Printf("[DEBUG] fingerprint.network: Detected interface %v with IP: %v", intf.Name, nwResource.IP) | |||
} | |||
|
|||
// Deprectaed, setting the first IP as unique IP for the node |
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.
Deprectaed -> Deprecated
69afadb
to
b0a20b4
Compare
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This enables the Nomad fingerprinter to pick up IPv6 addresses. Once this is merged I will add a note in the documentation that Nomad may pick either an IPv4 or an IPv6 address on a dual stack system if an interface has both ipv4 and ipv6 addresses.