You can early-return if the sysctl is already set to 1.

Done.

I'm wondering if we should invert the logic - set the default policy, and then toggle the sysctl. This would remove the need for recovery code.

Setting the sysctl will fail if, for example, "/proc/sys/net" is mounted read-only and, by the time this is happening, if iptables is enabled we'll already have tried to mess with the rules (so the sysctl setting is perhaps more likely to fail).

Swapping them would just mean having to undo the policy change if the sysctl update fails. So I'm not sure it'd help - and it's probably best to do the thing that's most likely to fail first, because then the other thing won't toggle. (But perhaps I missed the point?!)

Maybe it'd be benefitial here too to read both files, early-return if both are already set, and if not then start with ipt default policy and then set both unconditionnally. That'd make the code simpler I think.

As above - we'd still need to undo anything done so-far, so I'm not sure it'd help?

But I've put the undo-sysctl code in the right place now (oops), so perhaps it makes more sense?

It's existing code, but curious; we fail this function if we fail to read, but we don't return an error when failing to update it? Could that have situations where we would not actually be updating the config? (Are those save to be ignored?).

Looks like for IPv4 we have a separate utility (configureIPForwarding()). Wondering if (not for this PR), we should have that utility handle the "changed" or "not changed" case, e.g. something like;

func configureIPForwarding(file string, enable bool) (changed bool, _ error) {
	data, err := os.ReadFile(file)
	if err != nil {
		return false, fmt.Errorf("cannot read IP forwarding setup: %w", err)
	}
	if enabled := data[0] == '1'; enabled == enable {
		return false, nil
	}
	var val byte
	if enable {
		val = '1'
	}
	if err := os.WriteFile(file, []byte{val, '\n'}, 0o644); err != nil {
		return false, fmt.Errorf("cannot write IP forwarding setup: %w", err)
	}
	return true, nil
}

That would return a boolean to indicate we changed anything, but an error when failing to either read or update (but that depends a bit on my previous comment).

The above wraps the errors, so we could detect os.IsnotFound() situations (if those are ok to be ignored).

Right, yes ... things won't work properly if IPv6 forwarding is needed and it can't be enabled, and I can't see why the error handling should be different to IPv4 - so I'll do that, thank you.

👍

That's not needed, we're in bridge_linux_test.go.

Good point!

It's existing code, but curious; we fail this function if we fail to read, but we don't return an error when failing to update it? Could that have situations where we would not actually be updating the config? (Are those save to be ignored?).

Looks like for IPv4 we have a separate utility (configureIPForwarding()). Wondering if (not for this PR), we should have that utility handle the "changed" or "not changed" case, e.g. something like;

func configureIPForwarding(file string, enable bool) (changed bool, _ error) {
	data, err := os.ReadFile(file)
	if err != nil {
		return false, fmt.Errorf("cannot read IP forwarding setup: %w", err)
	}
	if enabled := data[0] == '1'; enabled == enable {
		return false, nil
	}
	var val byte
	if enable {
		val = '1'
	}
	if err := os.WriteFile(file, []byte{val, '\n'}, 0o644); err != nil {
		return false, fmt.Errorf("cannot write IP forwarding setup: %w", err)
	}
	return true, nil
}

That would return a boolean to indicate we changed anything, but an error when failing to either read or update (but that depends a bit on my previous comment).

The above wraps the errors, so we could detect os.IsnotFound() situations (if those are ok to be ignored).

The WriteFile error above is ignored (only logged), but here we try to write to it again in the defer. Wondering if the first write should be returned as well (there's nothing to "undo" if we failed to write, and it's also likely that if we failed to write the first time that it'll fail the second time?)

The configUpdated also may be incorrect here, because if we failed to write, then no config was updated 🤔

Right, yes - I've aligned IPv4/IPv6 error handling as you suggested above.

This utility is only used in a single test; possibly (with the other idea of having a utility for this), we could reuse that utility in the tests as well;

_, err := configureIPForwarding(ipv4ForwardConf, true)
if err != nil {
    t.Fatalf(err)
}

Done.

Perhaps use the same const as for IPv4 (forwardConfPerm) or even consider removing the const entirely.

Removed.

I don't think we have docs for this in man/dockerd.8.md wondering if we should look at that in a follow-up 🤔

Is this an option that can be "live reloaded" ? (i.e., enabled/disabled when reloading the daemon config?)

Added to the manpage.

For config reload ... it's not, like --ip-forward and --iptables/--ip6tables. I think they're all similar in that they affect host config when a network's created, and that host config won't be un-done if the daemon config is changed.

Perhaps someone might want to fix the filter-FORWARD policy manually after creating a network, then reload daemon config so that it's not changed again when the next network is created. But, I think the same applies to IP forwarding and iptables and it's probably best that they all have the same behaviour?

This needs an update in man/dockerd.8.md and the corresponding daemon reference docs in docker/cli

Also curious; should enabling this without the other option be an error-condition? Or is it fine to ignore?

Added to the manpage.

I think it's ok to ignore - it says it'll only do-something if the daemon enables forwarding itself, and it won't if --ip-forward=false (or if iptables/ip6tables is disabled).

Oh, I didn't look at this closely; I just realised we're introducing a new boolean flag, but with true as default. I realise we already have some those, but if we consider this default to be true in the foreseeable future, then perhaps we should have false as default and name the flag accordingly.

Having false makes the UX more usable (instead of --boolean-flag=false), but also can help prevent issues when reading config from daemon.json where a missing field would default to "false" when unmarshaling.

(just a quick comment; about to jump in a call)

Right, yes - the closely related --iptables, --ip6tables and --ip-forward flags all default to true ... I guessed that was probably deliberate! They're all "batteries included" flags ... you need to leave them enabled to make things work, unless you know about the innards and have a good reason to turn them off.

So, I'm not sure a default-false option fits well with those others and, if we do change it, it'd need to be called something like --no-ip-filter-forward-drop. Then it's a bit too double-negative-y, "--no-ip-filter-forward-drop=false" means it will set the filter-FORWARD policy to DROP if it needs to.

(Unless anyone has a better idea for the flag name, it is already a bit cumbersome ... but I thought it was good to start it with "--ip..." so that it's close to those other flags in help text, and I think it'll still make sense for native nftables.)

I don't think there's an issue with merging CLI and daemon.json options, in the same way as for the other options - no option is allowed to be specified in both, if it's in the JSON it'll override the CLI's default. If not present in the JSON or the CLI, the CLI's default wins.

After further discussion in Slack, I've changed the option to --ip-forward-no-drop.

No engineers were harmed in the making of this decision!

Looks like you forgot updating this one 🙈

Nit; avoid future tense; this could be "it also sets the ..."

Done - there are a few existing examples of that in the text, I guess we should tidy those too (--default-address-pool, --ip-forward, --ipv6, --userns-remap, zfs.fsname).

Ah, yes, we could make a pass at doing so. We also still need to change our packaging to use the one from this repository in stead of the copy that still lives in docker/cli 🙈

Oh, right, there are two docker.md.8's! So we should be keeping them in step for now?

Same here; "Docker does not modify ..."