Pass interface name and MAC back to runtime #145

dcbw · 2016-03-02T19:12:29Z

Add the infrastructure to pass interface names back from the plugin to the runtime. This lays the groundwork for letting runtimes not pass CNI_IFNAME and allowing plugins to determine that themsevles. This should be backwards compatible; if the runtime sends CNI_IFNAME then plugins must honor that and return an error if they cannot. If the runtime does not send CNI_IFNAME, the plugins currently default to 'eth0'.

Example output:

{
    "interface": {
        "hostIfname": "veth511119b6",
        "hostMac": "56:95:eb:4c:a0:91",
        "containerIfname": "eth0",
        "containerMac": "6a:1f:52:95:86:2c"
    },
    "ip4": {
        "ip": "10.1.0.152/24",
        "gateway": "10.1.0.1",
        "routes": [
            {
                "dst": "0.0.0.0/0"
            }
        ]
    },
    "dns": {}

dcbw · 2016-03-02T19:14:02Z

For example, with OpenShift we'd use the hostIfname so that we don't have to jump into the container with 'nsenter' and grab the peer ifindex, then look that up on the host side. We also use nsenter to get the container veth MAC address, so this would also remove that step.

I'm going to plumb this through Kubernetes too, since it assumes interfaces are always called 'eth0' which may not always be the case, like with the Calico CNI plugin.

dcbw · 2016-03-03T20:43:07Z

@steveej fixed the issue, PTAL

caseydavenport · 2016-03-04T23:52:20Z

Looks useful :)

Wanted to chime in with a few thoughts:

This is quite nice for CNI plugins which don't use multiple interfaces to implement multiple CNI networks (e.g Calico). We should make sure the spec includes that orchestrators should allow for multiple CNI networks to return the same interface fields .
This does put a bit more work on the backs of network plugins in terms of determining what interface they should use, but it isn't a whole lot of work and gives the plugin more flexibility which is nice.

@dcbw - Also wanted to correct the point above - the Calico CNI plugin does respect the CNI_IFNAME variable. I believe you're thinking of Mike Spreitzer's experimentation with mapping his own CNI plugin for Kubernetes to the Calico libnetwork plugin (which doesn't respect CNI_IFNAME for obvious reasons).

dcbw · 2016-03-08T22:20:29Z

@caseydavenport thanks for the clarification, and sorry for blaming the Calico plugin where I shouldn't have :)

zachgersh · 2016-03-15T16:33:14Z

hey @dcbw would it be possible to include some tests of this functionality? We've started backfilling tests and it would be great to merge new code with tests.

If you need examples or want to chat about how to test this I'd be happy to help.

steveej · 2016-03-15T18:05:47Z

SPEC.md

-The specification does not declare how this information must be processed by CNI consumers.
-Examples include generating an `/etc/resolv.conf` file to be injected into the container filesystem or running a DNS forwarder on the host.
+- `cniVersion` specifies a [Semantic Version 2.0](http://semver.org) of CNI specification used by the plugin.
+- `dns` field contains a dictionary consisting of common DNS information that this network is aware of. The result is returned in the same format as specified in the [configuration](#network-configuration). The specification does not declare how this information must be processed by CNI consumers. Examples include generating an `/etc/resolv.conf` file to be injected into the container filesystem or running a DNS forwarder on the host.


Did you reformat this section intentionally? We've been writing one sentence (separated by .) per line in markdown files.

dcbw · 2016-03-17T21:29:34Z

@steveej unintentional reformatting, I'll fix and repush with some testcases.

steveej · 2016-03-18T09:05:26Z

@steveej unintentional reformatting, I'll fix and repush with some testcases.

Thanks :-)

Also, please make sure the commit message conforms to https://github.com/appc/cni/blob/master/CONTRIBUTING.md#format-of-the-commit-message. We should go back to insisting on this, as git log is starting to get out of shape :-| I wonder if I can get some bot to do this for us.

dcbw · 2016-03-24T18:20:18Z

@steveej repushed with the SPEC.md reformat fix. Also, instead of hardcoding "eth0" I have updated all plugins to use a prefix+index method and retry with an incremented index if the interface creation fails with EEXIST. This should make all plugins handle multiple interfaces in the same container as long as the runtime doesn't pass CNI_IFNAME.

I have not added testcases yet, so this isn't ready to merge. But I wanted to get your opinion on this updated approach.

dcbw · 2016-03-24T18:20:42Z

Also, please make sure the commit message conforms to https://github.com/appc/cni/blob/master/CONTRIBUTING.md#format-of-the-commit-message. We should go back to insisting on this, as git log is starting to get out of shape :-| I wonder if I can get some bot to do this for us.

Also updated the git commit message, is that better?

dcbw · 2016-04-07T16:58:10Z

Rebased on top of #167 with additional tests.

dcbw · 2017-01-19T15:21:05Z

@tgraf I just pushed an update that move Routes out of the IPs structure and updates the spec to clarify what IPAM plugins return. See #145 (comment) for more details on immediate plans.

@containernetworking/cni-maintainers this push has these changes:

moves Routes out of the IPs structure to a top-level alongside IPs and DNS
renames the "IP" list in Result to "IPs" to be consistent with Routes and Interfaces
adds a testcase to pkg/ipam to ensure we don't break it's Gateway handling functionality in the future
updates the Spec with some forgotten things and to clarify what IPAM plugins should returnd

PTAL thanks!

tomdee · 2017-01-20T01:20:24Z

The spec changes look OK to me, with just one simple fix:

You should bump the spec version at the top of the SPEC.md (might need a rebase)

That being said, I'm a little fuzzy on what the routes (or gateway) are actually used for or needed for. Is that information consumed by any runtimes or is it just just used for passing info between IPAM and network plugins?

tgraf · 2017-01-20T04:08:10Z

@dcbw Given that []nexthops can be added later on while staying compatible with the non-array gw I believe what is being proposed a good course of action.

dcbw · 2017-01-20T04:11:00Z

That being said, I'm a little fuzzy on what the routes (or gateway) are actually used for or needed for. Is that information consumed by any runtimes or is it just just used for passing info between IPAM and network plugins?

@tomdee mostly passed between plugins. I can't see why anything except the thing that configures the container's networking (eg, the IPAM-wrapping plugin) would use it.

dcbw · 2017-01-20T04:19:26Z

@tomdee updated the spec version, I don't know why I hadn't done that before...

dcbw · 2017-01-20T19:48:16Z

@tomdee @rosenhouse @squeed rebased on top of ordered chaining.

rosenhouse · 2017-01-23T01:47:05Z

SPEC.md

+          "version": "<4-or-6>",
+          "address": "<ip-and-prefix-in-CIDR>",
+          "gateway": "<ip-address-of-the-gateway>",          (optional)
+          "interface": "<index into 'interfaces' list>"


Totally minor nit-pick, but the quotes probably don't belong around the numeric index value.

@rosenhouse done

rosenhouse · 2017-01-23T01:48:42Z

SPEC.md

+          "version": "<4-or-6>",
+          "address": "<ip-and-prefix-in-CIDR>",
+          "gateway": "<ip-address-of-the-gateway>",      (optional)
+          "interface": "<index into 'interfaces' list>" (not required for IPAM plugins)


Again, I think this numeric index for interface shouldn't have quotes around the value.

@rosenhouse done

…t for tests

…package

Otherwise the test fails, since Go's JSON marshaller prints dict items alphabetically in its String() call.

…untime Updates the spec and plugins to return an array of interfaces and IP details to the runtime including: - interface names and MAC addresses configured by the plugin - whether the interfaces are sandboxed (container/VM) or host (bridge, veth, etc) - multiple IP addresses configured by IPAM and which interface they have been assigned to Returning interface details is useful for runtimes, as well as allowing more flexible chaining of CNI plugins themselves. For example, some meta plugins may need to know the host-side interface to be able to apply firewall or traffic shaping rules to the container.

dcbw · 2017-01-25T17:33:45Z

@rosenhouse fix up the spec; if this looks good mind hitting the merge button? Thanks!

rosenhouse · 2017-01-25T19:20:50Z

It would be an honor...

AlexeyKasatkin

Seems, I'm a bit too late with comments.

AlexeyKasatkin · 2017-01-25T16:40:05Z

pkg/types/current/types.go

+	return err
+}
+
+// String returns a formatted string in the form of "[Interfaces: $1,][ IP: $2,] DNS: $3" where


and "Routes"

AlexeyKasatkin · 2017-01-25T17:13:55Z

plugins/ipam/dhcp/daemon.go

-	result.IP4 = &types.IPConfig{
-		IP:      *ipn,
+	result.IPs = []*current.IPConfig{{
+		Version: "4",


Just to be in context: Will Allocate() be called for ip4 nets only? Or how does this behave now?

@AlexeyKasatkin nothing about the Allocate() call and the lease acquisition process changes. The DHCP plugin does not currently support DHCPv6 at all, and thus it's still limited to DHCPv4. Any invocation of the DHCP plugin will inherently try only IPv4.

But IPv6 patches would be gratefully accepted!

dcbw force-pushed the pass-ifname-back branch from 3995ced to 2e180b9 Compare March 2, 2016 20:35

steveej added needs review reviewed/needs rework labels Mar 3, 2016

dcbw force-pushed the pass-ifname-back branch from 2e180b9 to 8221cae Compare March 3, 2016 19:03

steveej removed the reviewed/needs rework label Mar 4, 2016

steveej mentioned this pull request Mar 4, 2016

Increasing CNI's scope? #147

Closed

zachgersh added the missing/tests label Mar 15, 2016

steveej reviewed Mar 15, 2016
View reviewed changes

steveej added reviewed/needs rework and removed needs review labels Mar 16, 2016

steveej mentioned this pull request Mar 18, 2016

plugin's should return interface information in the result #113

Closed

dcbw force-pushed the pass-ifname-back branch 2 times, most recently from 3c82107 to bd0d7b2 Compare March 24, 2016 18:20

dcbw mentioned this pull request Mar 30, 2016

Add e2e testing of ipvlan, macvlan, and bridge plugins #167

Closed

dcbw force-pushed the pass-ifname-back branch from bd0d7b2 to 960feef Compare March 30, 2016 22:15

steveej added needs review and removed reviewed/needs rework labels Mar 31, 2016

steveej added this to the v0.3.0 milestone Mar 31, 2016

steveej added the needs rebase label Apr 6, 2016

dcbw force-pushed the pass-ifname-back branch from 960feef to 882b54f Compare April 7, 2016 16:57

tgraf approved these changes Jan 20, 2017

View reviewed changes

dcbw force-pushed the pass-ifname-back branch from da8e2c9 to 0bd0a58 Compare January 20, 2017 04:19

feiskyer mentioned this pull request Jan 20, 2017

Support CNI network plugin in Frakti kubernetes-retired/frakti#58

Closed

5 tasks

dcbw force-pushed the pass-ifname-back branch from 0bd0a58 to d3606f4 Compare January 20, 2017 19:47

rosenhouse reviewed Jan 23, 2017

View reviewed changes

rosenhouse approved these changes Jan 23, 2017

View reviewed changes

dcbw added 6 commits January 25, 2017 11:31

testutils: pass netConf in for version operations; pass raw result ou…

cb4cd0e

…t for tests

types: make Result an interface and move existing Result to separate …

befb959

…package

macvlan/ipvlan: use common RenameLink method

ad2a5cc

plugins/flannel: organize test JSON alphabetically

b0b896f

Otherwise the test fails, since Go's JSON marshaller prints dict items alphabetically in its String() call.

pkg/ipam: add testcases

befad17

dcbw force-pushed the pass-ifname-back branch from d3606f4 to d5acb12 Compare January 25, 2017 17:33

rosenhouse merged commit c4271db into containernetworking:master Jan 25, 2017

AlexeyKasatkin reviewed Jan 26, 2017

View reviewed changes

This was referenced Feb 16, 2017

upgrade cni in vendor to support pass interface info back to runtime kubernetes/kubernetes#41552

Closed

Update cni in vendor and refactor cniNetworkPlugin for hypervisor runtime kubernetes/kubernetes#41745

Closed

dcbw mentioned this pull request Feb 23, 2017

plugins/main/ptp: set the Sandbox property on the response #377

Merged

asridharan mentioned this pull request Feb 24, 2017

Question / Feature req: Attach container to multiple networks / specify "network-name" on a per-interface basis #381

Open

dcbw mentioned this pull request Mar 22, 2017

spec/plugins: fix 'ip'->'ips' in the spec, bump to 0.3.1 #413

Merged

dcbw mentioned this pull request Aug 2, 2017

ipam: Make ConfigureIface handle IPv6 #234

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass interface name and MAC back to runtime #145

Pass interface name and MAC back to runtime #145

dcbw commented Mar 2, 2016

dcbw commented Mar 2, 2016

dcbw commented Mar 3, 2016

caseydavenport commented Mar 4, 2016

dcbw commented Mar 8, 2016

zachgersh commented Mar 15, 2016

steveej Mar 15, 2016

dcbw commented Mar 17, 2016

steveej commented Mar 18, 2016

dcbw commented Mar 24, 2016

dcbw commented Mar 24, 2016

dcbw commented Apr 7, 2016

dcbw commented Jan 19, 2017

tomdee commented Jan 20, 2017

tgraf commented Jan 20, 2017

dcbw commented Jan 20, 2017

dcbw commented Jan 20, 2017

dcbw commented Jan 20, 2017

rosenhouse Jan 23, 2017

dcbw Jan 25, 2017

rosenhouse Jan 23, 2017

dcbw Jan 25, 2017

dcbw commented Jan 25, 2017

rosenhouse commented Jan 25, 2017

AlexeyKasatkin left a comment

AlexeyKasatkin Jan 25, 2017

AlexeyKasatkin Jan 25, 2017

dcbw Feb 15, 2017

Pass interface name and MAC back to runtime #145

Pass interface name and MAC back to runtime #145

Conversation

dcbw commented Mar 2, 2016

dcbw commented Mar 2, 2016

dcbw commented Mar 3, 2016

caseydavenport commented Mar 4, 2016

dcbw commented Mar 8, 2016

zachgersh commented Mar 15, 2016

Choose a reason for hiding this comment

dcbw commented Mar 17, 2016

steveej commented Mar 18, 2016

dcbw commented Mar 24, 2016

dcbw commented Mar 24, 2016

dcbw commented Apr 7, 2016

dcbw commented Jan 19, 2017

tomdee commented Jan 20, 2017

tgraf commented Jan 20, 2017

dcbw commented Jan 20, 2017

dcbw commented Jan 20, 2017

dcbw commented Jan 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbw commented Jan 25, 2017

rosenhouse commented Jan 25, 2017

AlexeyKasatkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment