-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
Per-interface sysctls #47686
Per-interface sysctls #47686
Conversation
79f4458
to
745a4c4
Compare
// Only try to migrate settings for "eth0", anything else would always | ||
// have behaved unpredictably. | ||
if spl[3] != "eth0" { | ||
return "", fmt.Errorf(`unable to determine network endpoint for sysctl %s, use '--network=name=%s,sysctl=%s' or compose 'driver_opts: "%s":"%s"`, |
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 think we shouldn't put CLI-specific or Compose-specific remediation steps here -- the API could be called by other tools where those steps won't make any sense.
OTOH CLI error messages sometimes looks cryptic for users not familiar with our API. I think we don't have a plan for 'augmenting' CLI error messages with remediation steps. Maybe that's something we need to discuss.
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.
Yes, it's not great, but I'm not sure how best to improve it.
I think it's quite important that we give good clues about how to specify per-interface sysctls - here, for the migration case below, and for when we refuse to migrate from the top level --sysctl
in a future release.
A "for example" might help a little, since CLI and compose are probably the common cases. But, not really.
Maybe the best we can do is just delete the hints, and hope the user's able to find the right section of the docs.
I'm not sure how augmented CLI messages would work, perhaps the API would need to return some token that'd tell the client to explain how to set per-interface sysctls in its world (extended --network
syntax for the CLI, or driver-opts
in compose)? I'm probably missing the point (?!), but any sort of mechanism like that sounds like a big change that'd have to be out-of-scope here.
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 changed the message (and updated the examples in PR description to show it) ... now it only mentions the driver-opt label - and the user will have to figure out how to use it.
But I've also updated the CLI PR docker/cli#4994 to get rid of the --network sysctl=
option and document the use of [create|run] --network driver-opt=com.docker.network.endpoint.sysctls=[value]
and network connect --driver-opt=com.docker.network.endpoint.sysctls=
to set multiple sysctls for an endpoint.
return "", nil | ||
} | ||
|
||
// TODO(robmry) - refuse to do the migration, generate an error if API > some-future-version. |
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.
Next API version should be fine.
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.
This change should land in release 27.0 (along with re-removing the SetKey hook that requires it). So, we'd want to deprecate per-interface sysctls in --sysctl
in 27.0, and remove the auto-migration in 28.0.
The current API version is 1.45, and it will be in the upcoming release 26.1. But, it might change in 27.0? In that case, if we make this code check for API version >1.45, we'll have accidentally removed the auto-migration in release 27.0.
So, it's probably best to raise a new issue to say a version check needs to be added, and mark it for milestone 28.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.
So, it's probably best to raise a new issue to say a version check needs to be added, and mark it for milestone 28.0?
#47651 bumped the API version for v27 but AFAICT there are no API changes committed yet. I'm fine with opening a new ticket for that.
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.
Ok, good - I've restricted the migration to API <= 1.46.
745a4c4
to
5d0ab3f
Compare
5d0ab3f
to
5d70d23
Compare
2edd300
to
2681c58
Compare
// TODO(robmry) - refuse to do the migration, generate an error if API > some-future-version. | ||
|
||
newDriverOpt := strings.Join(netIfSysctls, ",") | ||
warning := fmt.Sprintf(`Migrated %s to DriverOpts{"%s":"%s"}.`, |
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.
warning := fmt.Sprintf(`Migrated %s to DriverOpts{"%s":"%s"}.`, | |
warning := fmt.Sprintf(`Migrated sysctl %q to DriverOpts{%q:%q}.`, |
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.
The second example in the description:
# docker run --rm -ti --name c1 --network mynet --sysctl=net.ipv6.conf.eth0.accept_ra=2 --sysctl=net.ipv6.conf.eth0.forwarding=1 alpine
WARNING: Migrated net.ipv6.conf.eth0.accept_ra,net.ipv6.conf.eth0.forwarding to DriverOpts{"com.docker.network.endpoint.sysctls":"ipv6.conf.accept_ra=2,ipv6.conf.forwarding=1"}.
Would become:
# docker run --rm -ti --name c1 --network mynet --sysctl=net.ipv6.conf.eth0.accept_ra=2 --sysctl=net.ipv6.conf.eth0.forwarding=1 alpine
WARNING: Migrated sysctl "net.ipv6.conf.eth0.accept_ra,net.ipv6.conf.eth0.forwarding" to DriverOpts{"com.docker.network.endpoint.sysctls":"ipv6.conf.accept_ra=2,ipv6.conf.forwarding=1"}.
Maybe that's ok, or would be without the quotes around the list, and ignoring sysctl vs. sysctls.
Another option would be:
# docker run --rm -ti --name c1 --network mynet --sysctl=net.ipv6.conf.eth0.accept_ra=2 --sysctl=net.ipv6.conf.eth0.forwarding=1 alpine
WARNING: Migrated sysctl "net.ipv6.conf.eth0.accept_ra" to DriverOpts{"com.docker.network.endpoint.sysctls":"ipv6.conf.accept_ra=2"}.
WARNING: Migrated sysctl "net.ipv6.conf.eth0.forwarding" to DriverOpts{"com.docker.network.endpoint.sysctls":"ipv6.conf.forwarding=1"}.
But, that doesn't show the value that really needs to be passed to DriverOpts.
I think it's unambiguous as-is, the named sysctls won't be interpreted as anything but sysctls from the request.
But, could do Migrated sysctl %s to DriverOpts{%q:%q}.
if you think it's necessary?
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.
But, could do
Migrated sysctl %s to DriverOpts{%q:%q}.
if you think it's necessary?
Yes. You are formatting user-supplied garbage, with the expectation for users to copy-paste them back into docker
commands. What if there are quote characters in the input strings?
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.
Ah right, I see. Thank you.
libnetwork/osl/interface_linux.go
Outdated
scPath = append(scPath, sk[2:]...) | ||
|
||
sysPath := filepath.Join(scPath...) | ||
errC := make(chan error, 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.
n.InvokeFunc
is a synchronous blocking call so there is no need to communicate through a channel.
var errF error
f := func() {
if err := ...; err != nil {
errF = err
return
}
}
if err := n.InvokeFunc(f); err != nil {
return err
}
if errF != nil {
return err
}
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.
Ok, done.
2681c58
to
93e43d5
Compare
Signed-off-by: Rob Murray <[email protected]>
Signed-off-by: Rob Murray <[email protected]>
b178729
to
e29f2f0
Compare
e29f2f0
to
058bf75
Compare
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 discovered some interesting prior art for per-iface sysctls: whatever CNI plugin OpenShift ships with accepts the full dotted sysctl name with the token IFNAME
used as a placeholder for the interface name. It's an interesting contrast to our shortening scheme. Is our scheme too clever for users to figure out?
// Append exiting per-endpoint sysctls to the migrated sysctls (give priority | ||
// to per-endpoint settings). |
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.
// Append exiting per-endpoint sysctls to the migrated sysctls (give priority | |
// to per-endpoint settings). | |
// Append existing per-endpoint sysctls to the migrated sysctls (give priority | |
// to per-endpoint settings). |
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.
Fixed.
|
||
const scName = "net.ipv4.conf.eth0.forwarding" | ||
for _, val := range []string{"0", "1"} { | ||
t.Run("set to "+val, func(t *testing.T) { |
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.
t.Run("set to "+val, func(t *testing.T) { | |
t.Run("ipv4.conf.forwarding="+val, func(t *testing.T) { |
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 missed this - but changed it anyway for the new IFNAME schem, slightly-differently ... the CLI converts driver-opts to lower case (it probably shouldn't) - so the test now checks that both IFNAME
and ifname
are allowed, nesting loops with a structured test name.
daemon/container_operations.go
Outdated
if sysctls, ok := epConfig.DriverOpts[netlabel.EndpointSysctls]; ok { | ||
for _, sysctl := range strings.Split(sysctls, ",") { | ||
scname := strings.SplitN(sysctl, ".", 3) | ||
if len(scname) != 3 || (scname[0] != "ipv4" && scname[0] != "ipv6") { |
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.
On second thought, we should probably also allow mpls
. As MPLS packets can transit Ethernet links, some users (particularly in datacenter and telecom environments) may have use cases for enabling MPLS on e.g. a macvlan iface.
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.
Done.
libnetwork/osl/interface_linux.go
Outdated
scPath := []string{"/proc/sys/net", sk[0], sk[1], ifName} | ||
scPath = append(scPath, sk[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.
scPath := []string{"/proc/sys/net", sk[0], sk[1], ifName} | |
scPath = append(scPath, sk[2:]...) | |
scPath := append([]string{"/proc/sys/net", sk[0], sk[1], ifName}, sk[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.
Done, slightly differently, for the IFNAME scheme.
Maintainer call: consensus is to go forward with specifying the full sysctl name where an |
466bf3a
to
abf6de3
Compare
Done. |
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.
The commit message for abf6de3 needs to be updated to reflect the new IFNAME
scheme.
abf6de3
to
a5dc638
Compare
Thank you - fixed. |
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.
One small nit about an outdated comment but otherwise LGTM.
for k, v := range hostConfig.Sysctls { | ||
// If the sysctl name matches "net.*.*.eth0.*" ... | ||
if spl := strings.SplitN(k, ".", 5); len(spl) == 5 && spl[0] == "net" && strings.HasPrefix(spl[3], "eth") { | ||
// Transform the name to the endpoint-specific short form. |
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.
// Transform the name to the endpoint-specific short form. |
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.
Removed - thank you.
Until now it's been possible to set per-interface sysctls using, for example, '--sysctl net.ipv6.conf.eth0.accept_ra=2'. But, the index in the interface name is allocated serially, and the numbering in a container with more than one interface may change when a container is restarted. The change to make it possible to connect a container to more than one network when it's created increased the ambiguity. This change adds label "com.docker.network.endpoint.sysctls" to the DriverOpts in EndpointSettings. This option is explicitly associated with the interface. Settings in "--sysctl" for "eth0" are migrated to DriverOpts. Because using "--sysctl" with any interface apart from "eth0" would have unpredictable results, it is now an error to use any other interface name in the top level "--sysctl" option. The error message includes a hint at how to use the new per-interface setting. The per-endpoint sysctl name has the interface name replaced by "IFNAME". For example: net.ipv6.conf.eth0.accept_ra=2 becomes: net.ipv6.conf.IFNAME.accept_ra=2 The value of DriverOpts["com.docker.network.endpoint.sysctls"] is a comma separated list. Settings from '--sysctl' are applied by the runtime lib during task creation. So, task creation fails if the endpoint does not exist. Applying per-endpoint settings during interface configuration means the endpoint can be created later, which paves the way for removal of the SetKey OCI prestart hook. Unlike other DriverOpts, the sysctl label itself is not driver-specific, but each driver has a chance to check settings/values and raise an error if a setting would cause it a problem - no such checks have been added in this initial version. As a future extension, if required, it would be possible for the driver to echo back valid/extended/modified settings to libnetwork for it to apply to the interface. (At that point, the syntax for the options could become driver specific to allow, for example, a driver to create more than one interface). Signed-off-by: Rob Murray <[email protected]>
a5dc638
to
0071832
Compare
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.
LGTM. Thanks!
- What I did
Until now it's been possible to set per-interface sysctls using, for example,
--sysctl net.ipv6.conf.eth0.accept_ra=2
. But, the index in the interface name is allocated serially, and the numbering in a container with more than one interface may change when a container is restarted. The change to make it possible to connect a container to more than one network when it's created increased the ambiguity.This change adds label
com.docker.network.endpoint.sysctls
to the DriverOpts in EndpointSettings. This option is explicitly associated with the interface.Settings in
--sysctl
foreth0
are migrated toEndpointSettings.DriverOpts
.Because using
--sysctl
with any interface apart frometh0
would have unpredictable results, it is now an error to use any other interface name in the top level--sysctl
option. The error message includes a hint at how to use the new per-interface setting.The per-endpoint sysctl name has the interface name replaced by the string
IFNAME
, for example:net.ipv6.conf.eth0.accept_ra=2
becomes:
net.ipv6.conf.IFNAME.accept_ra=2
(The string
ifname
is also accepted because, for some reason, the CLI converts names to lower-case.)The value of
DriverOpts["com.docker.network.endpoint.sysctls"]
is a comma separated list of these sysctls.Settings from
--sysctl
are applied by the runtime lib during task creation. So, task creation fails if the endpoint does not exist. Applying per-endpoint settings during interface configuration means the endpoint can be created later, which paves the way for removal of the SetKey OCI prestart hook.Unlike other DriverOpts, the sysctl label itself is not driver-specific, but each driver has a chance to check settings/values and raise an error if a setting would cause it a problem - no such checks have been added in this initial version. As a future extension, if required, it would be possible for the driver to echo back valid/extended/modified settings to libnetwork for it to apply to the interface. (At that point, the syntax for the options could become driver specific to allow, for example, a driver to create more than one interface.)
- How I did it
- How to verify it
New unit and integration tests.
And ...
Migration of one or two top level
--sysctl
settings ...Inspect output ...
No migration for
eth1
...Attempt to set a per-interface sysctl for an unknown protocol ...
Nonexistent
sysctl
(very verbose error message at the moment) ...But, a lot of that waffle will go-away once the prestart hook is removed and settings are applied after task creation. It'll be more like ...
- Description for the changelog