-
Notifications
You must be signed in to change notification settings - Fork 678
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 |
…tion Signed-off-by: Swagat Bora <sbora@amazon.com>
e0f7d38
to
a518963
Compare
7a5c886
to
546602c
Compare
Signed-off-by: Swagat Bora <sbora@amazon.com>
546602c
to
9ce8625
Compare
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