Skip to content
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

cni: ensure to setup CNI addresses in deterministic order #17766

Merged
merged 3 commits into from
Jul 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/17766.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:improvement
cni: Ensure to setup CNI addresses in deterministic order
```
48 changes: 27 additions & 21 deletions client/allocrunner/networking_cni.go
Original file line number Diff line number Diff line change
Expand Up @@ -130,46 +130,52 @@ func (c *cniNetworkConfigurator) Setup(ctx context.Context, alloc *structs.Alloc
// cniToAllocNet converts a CNIResult to an AllocNetworkStatus or returns an
// error. The first interface and IP with a sandbox and address set are
// preferred. Failing that the first interface with an IP is selected.
//
// Unfortunately the go-cni library returns interfaces in an unordered map so
// the results may be nondeterministic depending on CNI plugin output.
func (c *cniNetworkConfigurator) cniToAllocNet(res *cni.CNIResult) (*structs.AllocNetworkStatus, error) {
if len(res.Interfaces) == 0 {
return nil, fmt.Errorf("failed to configure network: no interfaces found")
}

netStatus := new(structs.AllocNetworkStatus)

// Unfortunately the go-cni library returns interfaces in an unordered map meaning
// the results may be nondeterministic depending on CNI plugin output so make
// sure we sort them by interface name.
names := make([]string, 0, len(res.Interfaces))
for k := range res.Interfaces {
names = append(names, k)
}
Comment on lines +143 to +146
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sort.Strings(names)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized while this makes interface selection deterministic, I don't think it allows for users to control what interface is used. It seems like whatever interface is first in lexicographical order wins.

Previously you could rely on a crash/restart loop to eventually get the interface you wanted, but now you'll get the same interface every time.

Do we need to add a way to specify an interface name, or am I missing a way interface names can be controlled by users so they can rely on lexicographical ordering to do the right thing automatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, in my case there was no crash restart loop, it just started running with incorrect consul service registration. The reason why I think, at least for now, lexicographical order makes sense is because CNI plug-in configurations are executed in order as well. If someone wanted to change which interface address should be chosen they could move that interface up in the config list. I’m not sure if there are more use cases that would benefit from being able to specify the interface name to use (first).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sort.String() will choose the first by lexicographical ordering though and not match CNI plugin configuration order though right?

It appears we could iterate over the interface names in CNI order via res.Raw().Interfaces and pluck the Config out of the res.Interfaces map based on those names?

Sorry for the complication, I think we'll just want to be able to document something like the first CNI interface returned will be used and not have alphabetical order mess things up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that what I'm doing is kind of a hack as I have a single CNI network configuration file with an additional custom plugin (which is always executed in order as defined by the plugin array in the CNI conflist file) that sets up the eth1 bridge interface to connect to the host, ignoring what is set through CNI_IFNAME.

This custom plugin then returns the previous CNI result with the additional (eth1) interface and its ip/route configuration appended to it. There's no mention in the CNI spec that this is somehow unsupported and the spec allows multiple interfaces and ip assignments in the results. In my situation I could just skip updating the result with the new eth1 interface plus its configuration entirely. In that case Nomad wouldn't ever see that eth1 has been created, however, that would be against the spec that states that you should always update the results with what the plugin has changed. Here's an example result after running my plugin that created eth1:

{
  "cniVersion": "0.4.0",
  "interfaces": [
    {
      "name": "eth0",
      "mac": "8e:f1:74:40:97:b6",
      "sandbox": "/var/run/docker/netns/dce8eb432a25"
    },
    {
      "name": "eth1",
      "mac": "e6:b9:75:cb:39:10",
      "sandbox": "/var/run/docker/netns/dce8eb432a25"
    }
  ],
  "ips": [
    {
      "version": "",
      "interface": 0,
      "address": "10.177.191.183/24",
      "gateway": "10.177.191.1"
    },
    {
      "version": "4",
      "address": "169.254.0.65/32",
      "interface": 1
    }
  ],
  "routes": [
    {
      "dst": "0.0.0.0/0",
      "gw": "10.177.191.1"
    },
    {
      "dst": "169.254.1.1/32",
      "gw": "169.254.0.65"
    }
  ],
  "dns": {}
}

And the CNI configuration file used to create above:

{
  "cniVersion": "0.4.0",
  "name": "confidential",
  "plugins": [
    {
      "type": "macvlan",
      "master": "eth0.20",
      "ipam": {
        "type": "dhcp"
      },
      "mtu": 9000,
      "linkInContainer": false
    },
    {
      "type": "discovery"    <------ this sets up eth1
    }
  ]
}

Obviously whenever an external plugin would create additional interfaces we'd have the problem I'm facing now. I feel that sorting the interfaces in lexicographical order and always trying to use the address of the first interface is reasonable, especially since now it's a guessing game.

Copy link
Contributor Author

@ygersie ygersie Jul 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also work fine in case of using the bridge plugin which would effectively return 3 interfaces:

  • host bridge
  • veth host peer
  • veth container peer

Because we check if an interface has an actual address set in the cniToAllocNet method there's no way that could lead to unexpected results since there's only one interface IP assignment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thorough explanation @ygersie! I'll get this merged. Going from non-deterministic -> deterministic is a great improvement, and we can always add an interface selection option in the future.


// Use the first sandbox interface with an IP address
if len(res.Interfaces) > 0 {
for name, iface := range res.Interfaces {
if iface == nil {
// this should never happen but this value is coming from external
// plugins so we should guard against it
delete(res.Interfaces, name)
}
for _, name := range names {
iface := res.Interfaces[name]
if iface == nil {
// this should never happen but this value is coming from external
// plugins so we should guard against it
delete(res.Interfaces, name)
continue
}

if iface.Sandbox != "" && len(iface.IPConfigs) > 0 {
netStatus.Address = iface.IPConfigs[0].IP.String()
netStatus.InterfaceName = name
break
}
if iface.Sandbox != "" && len(iface.IPConfigs) > 0 {
netStatus.Address = iface.IPConfigs[0].IP.String()
netStatus.InterfaceName = name
break
}
}

// If no IP address was found, use the first interface with an address
// found as a fallback
if netStatus.Address == "" {
var found bool
for name, iface := range res.Interfaces {
for _, name := range names {
iface := res.Interfaces[name]
if len(iface.IPConfigs) > 0 {
ip := iface.IPConfigs[0].IP.String()
c.logger.Debug("no sandbox interface with an address found CNI result, using first available", "interface", name, "ip", ip)
netStatus.Address = ip
netStatus.InterfaceName = name
found = true
break
}
}
if !found {
c.logger.Warn("no address could be found from CNI result")
}
}

// If no IP address could be found, return an error
Expand Down
16 changes: 16 additions & 0 deletions client/allocrunner/networking_cni_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,22 @@ func TestCNI_forceCleanup(t *testing.T) {
})
}

// TestCNI_cniToAllocNet_NoInterfaces asserts an error is returned if CNIResult
// contains no interfaces.
func TestCNI_cniToAllocNet_NoInterfaces(t *testing.T) {
ci.Parallel(t)

cniResult := &cni.CNIResult{}

// Only need a logger
c := &cniNetworkConfigurator{
logger: testlog.HCLogger(t),
}
allocNet, err := c.cniToAllocNet(cniResult)
require.Error(t, err)
require.Nil(t, allocNet)
}

// TestCNI_cniToAllocNet_Fallback asserts if a CNI plugin result lacks an IP on
// its sandbox interface, the first IP found is used.
func TestCNI_cniToAllocNet_Fallback(t *testing.T) {
Expand Down
Loading