[dev] support multiple addresses / shadow addresses in interface info #10942
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Given that we may potentially have multiple addresses for an interface, the general convention is to place the *primary* address at the top of the address list. The PrimaryAddress method on the InterfaceInfo can be used to fetch the primary address (if defined). The majority of providers only support a single address so this change is backwards-compatible. The ec2 and gce providers in particular seem to support assigning multiple private IPs to a NIC although only one can be flagged as the primary.
The updated package version allows us to access IP associations for private IP addresses when fetching NIC information.
MAAS NICs seem to support the concept of multiple links. However, throughout the code it seems that we apply a last-write-wins policy. The changes from this commit preserve this functionality but I have added TODOs in the relevant locations so we can revisit (if required) in the future.
manadart
approved these changes
Nov 22, 2019
provider/maas/devices.go
Outdated
| @@ -230,7 +230,11 @@ func (env *maasEnviron) deviceInterfaceInfo(deviceID instance.Id, nameToParentNa | |||
| } | |||
|
|
|||
| nicInfo.CIDR = link.Subnet.CIDR | |||
| nicInfo.Address = corenetwork.NewProviderAddressInSpace(link.Subnet.Space, link.IPAddress) | |||
| // NOTE(achilleasa): the original code used a last-write-wins | |||
There was a problem hiding this comment.
This might be a little misleading to someone unfamiliar reading it.
Maybe something like:
This preserves the long-standing last-write-wins behaviour...
provider/maas/interfaces.go
Outdated
| @@ -254,7 +254,12 @@ func maasObjectNetworkInterfaces( | |||
| } else { | |||
| // We set it here initially without a space, just so we don't | |||
| // lose it when we have no linked subnet below. | |||
| nicInfo.Address = corenetwork.NewProviderAddress(link.IPAddress) | |||
| // | |||
| // NOTE(achilleasa): the original code used a last-write-wins | |||
|
|
achilleasa
deleted the
dev-support-mutliple-address-in-interface-info
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of change
This PR changes the
network.InterfaceInfostruct so that instead of a singleAddressfield, it contains two lists:Addresses: a list of provider addresses that encodes the IPs (typically but not necessarily private) assigned to the NIC. The majority of providers supporting networking only provide a single private IP. However, some providers (e.g. ec2 and gce) seem to support multiple private IPs where one is always tagged as primary.ShadowAddresses. A list of shadow/virtual IPs that are associated with this interface. These IPs are usually not visible to the machine that hosts the NIC.This split will allow us to pair shadow IPs with individual NICs and will be used by the upcoming instance-poller changes to detect public IPs.
As part of this PR the following networking-capable providers have been updated to populate the shadow address list:
Ipassociationsfield contains IP association URNs so the provider will try to look their details up via the API and populate the shadow IP field.QA steps
As this is an internal change and the
ShadowAddressesfield is not currently used there isn't a clear way to QA this. I have however manually verified that it works (i.e. we get both the private and the public IP) on ec2, gce and oci by bootstrapping a patched juju that logs the obtained interfaces.I have not been able to test the old oracle provider as I don't have access to credentials.