bridge: Add mac field to specify container iface mac #636

EdDev · 2021-06-10T11:46:42Z

Controlling the mac address of the interface (veth peer) in the
container is useful for functionalities that depend on the mac address.
Examples range from dynamic IP allocations based on an identifier (the
mac) and up to firewall rules (e.g. no-mac-spoofing).

Enforcing a mac address at an early stage and not through a chained
plugin assures the configuration does not have wrong intermediate
configuration. This is especially critical when a dynamic IP may be
provided already in this period.
But it also has implications for future abilities that may land on the
bridge plugin, e.g. supporting no-mac-spoofing.

The field name used (mac) fits with other plugins which control the
mac address of the container interface.

The mac address may be specified through the following methods:

CNI_ARGS
Args
RuntimeConfig [1]

The list is ordered by priority, from lowest to higher. The higher
priority method overrides any previous settings.
(e.g. if the mac is specified in RuntimeConfig, it will override any
specifications of the mac mentioned in CNI_ARGS or Args)

[1] To use RuntimeConfig, the network configuration should include the
capabilities field with mac specified ("capabilities": {"mac": true}).

EdDev · 2021-06-16T11:48:24Z

@squeed I have added the support for CNI_ARGS, Args and RunningConfig parameters.

Can you please approve the CI run?

mccv1r0 · 2021-06-16T15:44:21Z

I believe that this would fix #450 as well.

squeed · 2021-06-16T15:53:04Z

This looks pretty good. @s1061123 would you mind giving this a quick review?

squeed · 2021-06-16T15:54:56Z

Personal nit pick - can this be a single commit?

mccv1r0 · 2021-06-16T15:55:12Z

/lgtm

EdDev · 2021-06-17T06:28:43Z

Personal nit pick - can this be a single commit?

The idea was to make it easy for reviewers to go over it in stages. The changes are independent, stand by themselves and give a nicer "story" in the history (the details in the tests may look messy when looked as one change).
But I do not have any strong feelings over this, if you still prefer to squash it all, just let me know.

mars1024 · 2021-06-18T12:04:00Z

pkg/ip/link_linux_test.go

@@ -233,6 +233,37 @@ var _ = Describe("Link", func() {
 			})
 		})

+		It("successfully creates a veth pair with an explicit mac", func() {
+			const mac = "02:00:00:00:01:23"


why this test case is separated into three steps? and I don't see the necessity of the third step

I followed the format of the previous test.
I agree that the first two steps can be merged, they execute on the same netns so there is no reason to split them.
Perhaps the original reason for the split was to emphasize that the first step is the setup while the second is the assertion.
Anyway, I merged them.

The 3rd step is validating that the mac address specified is not assigned to the host side of the veth peer. It seemed a reasonable check to me.

mars1024 · 2021-06-18T12:04:52Z

plugins/main/bridge/bridge.go

+	} `json:"args,omitempty"`
+	RuntimeConfig struct {
+		Mac string `json:"mac,omitempty"`
+	} `json:"runtimeConfig,omit"`


nit: omit --> omitempty

Thanks! Done.

mars1024 · 2021-06-18T12:09:41Z

plugins/main/bridge/bridge.go

@@ -55,6 +55,24 @@ type NetConf struct {
 	HairpinMode  bool   `json:"hairpinMode"`
 	PromiscMode  bool   `json:"promiscMode"`
 	Vlan         int    `json:"vlan"`
+	Mac          string `json:"mac,omitempty"`


Mac field here is internal, it should be ignored when marshalling

Why do you think it is internal?
All existing plugins here (macvlan, tuning) allow setting the mac through the network configuration and not only though the runtime parameters.
At the moment, this looks like a consistent behavior to have the parameters as a subset of the configuration.

Are you suggesting not to expose this parameter at the network configuration level?

@EdDev this is the netconf for the overall Bridge, not the container interface. A 'mac' field here would be the MAC of the bridge itself, not the container veth. It shouldn't be here because per-container options are not specified int he top-level plugin config struct. That's what the runtime args and CNI_ARGS are for.

The difference with macvlan and tuning is they do not have a "parent" interface like the bridge plugin does, so the MAC specified in the config there can only apply to the container interface.

@dcbw for the purpose of original requirement, I can take it out of the netconf. However, it seems to me that by doing so will break an existing convention where the runtime parameters are (always?) a subset of the netconf.

But again, I have no need for it and it is indeed odd to have this mac field defined at the network level.
If there no other opinions on this, I will take it out.

EdDev · 2021-06-20T07:35:00Z

change: Responding to review.

s1061123 · 2021-06-22T13:04:22Z

As far as I looked the diff, the 'mac' field could be provided through following ways:

CNI config
RuntimeConfig
'args'

So if user fills different MAC address in those fields, which MAC will wins?
You may need to add some comments (and document as well).

EdDev · 2021-06-22T13:18:57Z

As far as I looked the diff, the 'mac' field could be provided through following ways:
* CNI config

* RuntimeConfig

* 'args'
So if user fills different MAC address in those fields, which MAC will wins?
You may need to add some comments (and document as well).

I think the order is suppose to be part of the convention.
RuntimeConfig overrides args which overrides CNI_ARGS which overrides CNI config.
The plugins which support all, use this convention.

There is an explicit statement where args override CNI_ARGS here.

@squeed , am I correct here? Is there another specification where this is defined? Do you think it is worth updating the convention document to clarify this?

s1061123 · 2021-06-22T15:23:43Z

In convention.md, it seems not to be mentioned clearly (explicitly) yet, I guess.

@squeed Do you have any plan to mention the priority among Args/CNI_ARGS/RuntimeConfig in spec?

Controlling the mac address of the interface (veth peer) in the container is useful for functionalities that depend on the mac address. Examples range from dynamic IP allocations based on an identifier (the mac) and up to firewall rules (e.g. no-mac-spoofing). Enforcing a mac address at an early stage and not through a chained plugin assures the configuration does not have wrong intermediate configuration. This is especially critical when a dynamic IP may be provided already in this period. But it also has implications for future abilities that may land on the bridge plugin, e.g. supporting no-mac-spoofing. The field name used (`mac`) fits with other plugins which control the mac address of the container interface. The mac address may be specified through the following methods: - CNI_ARGS - Args - RuntimeConfig [1] The list is ordered by priority, from lowest to higher. The higher priority method overrides any previous settings. (e.g. if the mac is specified in RuntimeConfig, it will override any specifications of the mac mentioned in CNI_ARGS or Args) [1] To use RuntimeConfig, the network configuration should include the `capabilities` field with `mac` specified (`"capabilities": {"mac": true}`). Signed-off-by: Edward Haas <[email protected]>

EdDev · 2021-06-29T10:03:20Z

Change: Answered review requests

Squashed all commits into one.
Removed the ability to set the mac address from the plugin network config (netconf).

alonSadan

thank you Eddi. Really Nice!

I am not so familiar with this subject, but (as I asked also in the inline comments) are all macs acceptable for a veth at this stage, or there are limitations ? what happens if the mac being set breaks the limitation?

For example I think that the bridges mac should be the smallest of all macs of the veths that are connected to this bridge. Do we care about this kind of limitation at this stage, and is there something should be done to enforce it ?

alonSadan · 2021-06-30T08:31:09Z

plugins/main/bridge/bridge.go

+}
+
+// MacEnvArgs represents CNI_ARGS
+type MacEnvArgs struct {


Shouldn't the name be something more abstract, because it might contain more kinds of args in the future?
I mean, maybe it is better to drop Mac from the name?

No, this is the type boilerplate to automatically parse CNI_ARGS "MAC=FOO" strings.

plugins/main/bridge/bridge.go

plugins/main/bridge/bridge_test.go

alonSadan · 2021-06-30T09:05:09Z

plugins/main/bridge/bridge_test.go

@@ -1967,6 +2010,66 @@ var _ = Describe("bridge Operations", func() {
 			})
 		}

+		It(fmt.Sprintf("[%s] uses an explicit MAC addresses for the container iface (from CNI_ARGS)", ver), func() {


are all macs acceptable for a veth? if no perhaps it is also worth to add a test that shows what suppose to happen if someone sets a "wrong" mac?

Yeah, they are all "valid" here. The kernel will validate and fail the request if the MAC is wrong in some way (all zeros or 1s) and we shouldn't bother trying to reproduce the validation the kernel does I think.

squeed · 2021-06-30T15:52:36Z

This looks really good, thanks! I think this is ready to go.

A second set of eyes would help, but the testcases are nice.

By the way, I don't care about argument preference. If you try and specify mac two different ways, then you deserve what happens to you :-)

dcbw · 2021-06-30T15:56:34Z

/lgtm

squeed · 2021-06-30T16:00:21Z

@EdDev now that this has merged, can you file a PR on https://github.com/containernetworking/cni.dev/ that updates the bridge documentation (https://github.com/containernetworking/cni.dev/blob/main/content/plugins/current/main/bridge.md)

EdDev · 2021-06-30T16:28:38Z

@EdDev now that this has merged, can you file a PR on https://github.com/containernetworking/cni.dev/ that updates the bridge documentation (https://github.com/containernetworking/cni.dev/blob/main/content/plugins/current/main/bridge.md)

On it first thing tomorrow morning.
Thank you!

Ref: containernetworking/plugins#636 Signed-off-by: Edward Haas <[email protected]>

EdDev · 2021-07-01T10:30:02Z

@EdDev now that this has merged, can you file a PR on https://github.com/containernetworking/cni.dev/ that updates the bridge documentation (https://github.com/containernetworking/cni.dev/blob/main/content/plugins/current/main/bridge.md)

On it first thing tomorrow morning.
Thank you!

Done

mars1024 reviewed Jun 18, 2021

View reviewed changes

EdDev force-pushed the bridge-mac-specification branch from d1bcb40 to 089b7d3 Compare June 20, 2021 07:19

EdDev force-pushed the bridge-mac-specification branch from 089b7d3 to a3cde17 Compare June 29, 2021 07:51

alonSadan reviewed Jun 30, 2021

View reviewed changes

dcbw merged commit f14ff66 into containernetworking:master Jun 30, 2021

EdDev deleted the bridge-mac-specification branch June 30, 2021 16:27

EdDev added a commit to EdDev/cni.dev that referenced this pull request Jul 1, 2021

bridge plugin: Document the mac field argument

c1faa38

Ref: containernetworking/plugins#636 Signed-off-by: Edward Haas <[email protected]>

EdDev mentioned this pull request Jul 1, 2021

bridge plugin: Document the mac field argument containernetworking/cni.dev#87

Merged

This was referenced Sep 27, 2021

Update to CNI & Plugins 1.0.1 with stable 1.0 spec sylabs/singularity#337

Merged

CNI_ARGS with plugin chaining containernetworking/cni#560

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bridge: Add mac field to specify container iface mac #636

bridge: Add mac field to specify container iface mac #636

EdDev commented Jun 10, 2021 •

edited

Loading

EdDev commented Jun 16, 2021

mccv1r0 commented Jun 16, 2021

squeed commented Jun 16, 2021

squeed commented Jun 16, 2021

mccv1r0 commented Jun 16, 2021

EdDev commented Jun 17, 2021

mars1024 Jun 18, 2021

EdDev Jun 20, 2021

mars1024 Jun 18, 2021

EdDev Jun 20, 2021

mars1024 Jun 18, 2021

EdDev Jun 20, 2021

dcbw Jun 23, 2021 •

edited

Loading

EdDev Jun 28, 2021

EdDev Jun 29, 2021

EdDev commented Jun 20, 2021

s1061123 commented Jun 22, 2021

EdDev commented Jun 22, 2021 •

edited

Loading

s1061123 commented Jun 22, 2021

EdDev commented Jun 29, 2021 •

edited

Loading

alonSadan left a comment

alonSadan Jun 30, 2021

squeed Jun 30, 2021

alonSadan Jun 30, 2021

dcbw Jun 30, 2021

squeed commented Jun 30, 2021

dcbw commented Jun 30, 2021

squeed commented Jun 30, 2021

EdDev commented Jun 30, 2021

EdDev commented Jul 1, 2021

bridge: Add mac field to specify container iface mac #636

bridge: Add mac field to specify container iface mac #636

Conversation

EdDev commented Jun 10, 2021 • edited Loading

EdDev commented Jun 16, 2021

mccv1r0 commented Jun 16, 2021

squeed commented Jun 16, 2021

squeed commented Jun 16, 2021

mccv1r0 commented Jun 16, 2021

EdDev commented Jun 17, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbw Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdDev commented Jun 20, 2021

s1061123 commented Jun 22, 2021

EdDev commented Jun 22, 2021 • edited Loading

s1061123 commented Jun 22, 2021

EdDev commented Jun 29, 2021 • edited Loading

alonSadan 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

Choose a reason for hiding this comment

squeed commented Jun 30, 2021

dcbw commented Jun 30, 2021

squeed commented Jun 30, 2021

EdDev commented Jun 30, 2021

EdDev commented Jul 1, 2021

EdDev commented Jun 10, 2021 •

edited

Loading

dcbw Jun 23, 2021 •

edited

Loading

EdDev commented Jun 22, 2021 •

edited

Loading

EdDev commented Jun 29, 2021 •

edited

Loading