Allow users to ignore missing br_netfilter#49293
Conversation
robmry
added
area/networking
robmry
force-pushed
the
49240_ignore_br_netfilter_error
branch
2 times, most recently
from
4bb078f to
7d42585
Compare
| func enableBridgeNetFiltering(nfParam string) (retErr error) { | ||
| defer func() { | ||
| if retErr != nil { | ||
| if ignore, err := strconv.ParseBool(os.Getenv("DOCKER_IGNORE_BR_NETFILTER_ERROR")); err == nil && ignore { |
There was a problem hiding this comment.
Given that this
- is really an escape-hatch and (possibly) temporary
- the default will very unlikely ever be
enabledand the env-var to be used todisable
I think we could simplify to just check for a non-empty value to be set, similar to what we do for some other escape-hatch and/or undocumented env-vars, e.g.
Lines 842 to 845 in 7e78f78
| retErr = nil | ||
| return | ||
| } | ||
| retErr = fmt.Errorf("%w: DOCKER_IGNORE_BR_NETFILTER_ERROR=1 to ignore", retErr) |
There was a problem hiding this comment.
Given that we document it like this, I guess we can even just check for;
if os.Getenv("DOCKER_IGNORE_BR_NETFILTER_ERROR") == "1" {(but just "any non-empty value" would still work))
robmry
force-pushed
the
49240_ignore_br_netfilter_error
branch
from
1b6fd6d to
9c295fd
Compare
| func enableBridgeNetFiltering(nfParam string) (retErr error) { | ||
| defer func() { | ||
| if retErr != nil { | ||
| if _, ok := os.LookupEnv("DOCKER_IGNORE_BR_NETFILTER_ERROR"); ok { |
There was a problem hiding this comment.
oh; I think it's better to look for a non-empty value; this would also match the env when set to an empty value;
export DOCKER_IGNORE_BR_NETFILTER_ERROR=
env | grep DOCKER_IGNORE_BR_NETFILTER_ERROR
DOCKER_IGNORE_BR_NETFILTER_ERROR=There was a problem hiding this comment.
Yes, I updated the description to show that.
I don't like the idea that these would all work, but actually do the opposite of what they look like they'll do ...
export DOCKER_IGNORE_BR_NETFILTER_ERROR=0
export DOCKER_IGNORE_BR_NETFILTER_ERROR=false
export DOCKER_IGNORE_BR_NETFILTER_ERROR=no
But this wouldn't do anything ...
export DOCKER_IGNORE_BR_NETFILTER_ERROR=
I think we've used up 2025's quota for bikeshedding option parsers (!), so I've taken your second suggestion and changed it to check for "1".
There was a problem hiding this comment.
Ah sorry 🙈
So yeah; my motivation for the "empty value" was mostly to prevent cases where these could be set through scripts; I've seen a fair share of export FOO_BAR=$UNDEFINED_VAR.
export DOCKER_IGNORE_BR_NETFILTER_ERROR=0
Yeah, I can slightly more relate to that! I was mostly trying to align with other options, and not parse if not set, but perhaps we should align all of these, and have something similar to
moby/api/server/httputils/form.go
Lines 25 to 30 in 81a2acd
There was a problem hiding this comment.
Oh, no worries! It's worth getting this stuff right and easy to change now.
Allowing only "1" here doesnt rule anything out, we can relax the constraint and make it more bool-y in future, but it'd be more difficult to go the other way. I think it's good, clear and simple.
It does seem like we need think up a policy for option naming and handling, as we keep debating it for individual new settings. But, no doubt, we already have a fair few that won't fit whatever mould we can come up with. (Not that this particular env var should last long, ideally.)
There was a problem hiding this comment.
I suppose we could treat anything apart from "1", no value, or unset as invalid rather than ignoring it ... then it would be easier to change later.
Since commit 0f8fc31, the bridge driver will try to load kernel module br_netfilter if the userland proxy is disabled. If it fails, we're in unknown territory, so it's treated as an error. At the very least, containers will not be able to access host ports mapped to other containers in the same network. Before that, and before commit 5c499fc delayed the module load until it was needed - it was loaded unconditionally, but errors were only logged. So, on systems where the module is not available, or could not be loaded/configured, no error was reported and things "worked" (as long as you didn't try to use something that didn't work). That behaviour has been useful to some. So, make it possible to ignore the problem by setting env var: DOCKER_IGNORE_BR_NETFILTER_ERROR=1 Signed-off-by: Rob Murray <[email protected]>
robmry
force-pushed
the
49240_ignore_br_netfilter_error
branch
from
9c295fd to
e7bd60e
Compare
2 participants
- What I did
Add a workaround for users wanting to run with
br_netfilterunloaded when it's needed.Since commit 0f8fc31, the bridge driver will try to load kernel module br_netfilter if the userland proxy is disabled. If it fails, we're in unknown territory, so it's treated as an error. At the very least, containers will not be able to access host ports mapped to other containers in the same network.
Before that, and before commit 5c499fc delayed the module load until it was needed - it was loaded unconditionally, but errors were only logged.
So, on systems where the module is not available, or could not be loaded/configured, no error was reported and things "worked" (as long as you didn't try to use something that didn't work).
- How I did it
That behaviour has been useful to some. So, make it possible to ignore the problem by setting env var:
DOCKER_IGNORE_BR_NETFILTER_ERROR=1.- How to verify it
br_netfilterand moved asidebr_netfilter.koso that it couldn't be reloaded"userland-proxy": falseexport DOCKER_IGNORE_BR_NETFILTER_ERROR=1- Description for the changelog