-
Notifications
You must be signed in to change notification settings - Fork 692
feat: add support for com.docker.network.bridge.enable_icc network option #4311
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
base: main
Are you sure you want to change the base?
Conversation
return helpers.Command("run", "--rm", "--net", data.Identifier(), | ||
testutil.CommonImage, "ping", "-c", "1", "-W", "1", data.Identifier("c1")) | ||
}, | ||
Expected: test.Expects(1, nil, nil), // Expect ping to fail with exit code 1 |
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.
Wondering if ping
portably returns that same exit code.
Maybe use expect.ExitCodeGenericFail
to check for failure without being specific on exit code?
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.
eg: on macOS, it seems to exit 2
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.
I will change this to just check for any non-zero exit code
pkg/netutil/netutil_unix.go
Outdated
} | ||
|
||
// firewallPluginGEQ110 checks if the firewall plugin is greater than or equal to v1.1.0 | ||
func firewallPluginGEQ110(firewallPath string) (bool, error) { |
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.
Maybe we can just remove this helper and call directly firewallPluginGEQVersion(firewallPath, "v1.1.0")
where needs be?
testutil.CommonImage, "ping", "-c", "1", "-W", "1", data.Identifier("c1")) | ||
}, | ||
Expected: test.Expects(0, nil, nil), // Expect ping to succeed with exit code 0 | ||
}, |
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.
Maybe add a third case where enable_icc is not specified, to verify the default behavior?
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.
Sure, will add
@@ -111,6 +112,11 @@ func (e *CNIEnv) generateCNIPlugins(driver string, name string, ipam map[string] | |||
if err != nil { | |||
return nil, err | |||
} | |||
case "icc", "com.docker.network.bridge.enable_icc": |
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.
Needs documentation
c85e72d
to
bea516f
Compare
|
||
testCase.Require = require.All( | ||
require.Linux, | ||
nerdtest.Rootful, |
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.
Why?
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.
Thanks
bea516f
to
e0f7d38
Compare
Looks like the test disabling ICC is failing on a few platform. Taking a look. |
Moving to the v2.1.4 milestone |
e0f7d38
to
a518963
Compare
546602c
to
9ce8625
Compare
Needs rebase, in addition to fixing the CI |
9ce8625
to
c83c41a
Compare
…tion Signed-off-by: Swagat Bora <[email protected]>
c83c41a
to
8d9aa54
Compare
@AkihiroSuda I had a hard time debugging the CI failures since this was working on my local test host. Finally, figured out that the the test are failing because the ubuntu runners do not have We will need to load the module in our ubuntu runners first for this test to work. Given we have other features that also rely on cni firewall rules, I am thinking of enabling it across all the runners. let me know what you think? |
👍, but maybe you do not need to modprobe it explicitly if you mount
(and in the similar places in other |
I tried this option, but ran into the same errors swagatbora90@06d49d0. Looks like I need to load the br_netfilter module explicitly as they are not loaded by default. |
Opened [https://github.com//pull/4481] to load the br_netfilter module inn the runners. |
This PR adds support for docker compatible
com.docker.network.bridge.enable_icc
or--icc
network create option. This option is used to enable/disable intercontainer connectivity.By default, any bridge network allows connectivity between containers attached to the same network, while containers in different networks are isolated. The enable_icc feature can be further used to enable/disable intra bridge container connectivity.
Requires firewall plugin >= v1.7.1
Testing
com.docker.network.bridge.enable_icc
set to false, saytest-net
test-net
test-net
With icc=true, it will fallback to default behavior