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
Add firewalld policy "docker-forwarding". #47745
base: master
Are you sure you want to change the base?
Conversation
e840a6a
to
93149a2
Compare
libnetwork/iptables/firewalld.go
Outdated
if derr.Name == "org.fedoraproject.FirewallD1.Exception" && strings.HasPrefix(err.Error(), "NAME_CONFLICT") { | ||
log.G(context.TODO()).Debugf("Firewalld: %s policy already exists", dockerFwdPolicy) | ||
return false, nil | ||
} |
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.
if derr.Name == "org.fedoraproject.FirewallD1.Exception" && strings.HasPrefix(err.Error(), "NAME_CONFLICT") { | |
log.G(context.TODO()).Debugf("Firewalld: %s policy already exists", dockerFwdPolicy) | |
return false, nil | |
} | |
if derr.Name == dbusInterface+".Exception" && len(derr.Args) > 0 { | |
if msg, ok := derr.Args[0].(string); ok && strings.HasPrefix(msg, "NAME_CONFLICT") { | |
log.G(context.TODO()).Debugf("Firewalld: %s policy already exists", dockerFwdPolicy) | |
return false, nil | |
} | |
} |
Probably overkill to unpack the error args, but I was curious how awkward it would be to type-assert. Honestly, it's not too awful with the length check lifted into the outer condition.
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, when dbus's Error.Error() is already doing it? ...
moby/vendor/github.com/godbus/dbus/v5/conn.go
Lines 704 to 712 in faf84d7
func (e Error) Error() string { | |
if len(e.Body) >= 1 { | |
s, ok := e.Body[0].(string) | |
if ok { | |
return s | |
} | |
} | |
return e.Name | |
} |
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.
Ahh, you know something I didn'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.
if derr.Name == "org.fedoraproject.FirewallD1.Exception" && strings.HasPrefix(err.Error(), "NAME_CONFLICT") { | |
log.G(context.TODO()).Debugf("Firewalld: %s policy already exists", dockerFwdPolicy) | |
return false, nil | |
} | |
if derr.Name == dbusInterface+".Exception" && strings.HasPrefix(err.Error(), "NAME_CONFLICT") { | |
log.G(context.TODO()).Debugf("Firewalld: %s policy already exists", dockerFwdPolicy) | |
return false, nil | |
} |
Allow forwarding from any firewalld zone to the 'docker' zone. This makes it possible to use routable IPv6 addresses on a bridge network, with masquerading disabled, and have the host forward packets to it. Signed-off-by: Rob Murray <rob.murray@docker.com>
93149a2
to
ff8de5e
Compare
- What I did
Allow forwarding from any firewalld zone to the 'docker' zone.
This makes it possible to use routable IPv6 addresses on a bridge network, with masquerading disabled, and have the host forward packets to it.
- How I did it
Use firewalld's
addPolicy
method ... ignore a NAME_CONFLICT error (policy already exists), and an unknown-method error (becauseaddPolicy
was added in firewalld 0.9.0, older versions don't need the policy). If something else goes wrong, log the error but still reload firewalld - there's no need to regress existing functionality by bailing out early. Being cautious, because we don't have good test coverage for firewalld - errors are only logged anyway, normally at debug level.- How to verify it
Without this change...
Using OpenSUSE (firewalld 2.1.1-1.4)
Policy created (but not the pre-existing zone) on first run with the change ...
The policy ends up here ...
Repeat the
curl
test ...Daemon restart ...
CentOS 7 (firewalld 0.6.3-11.el7) -
addPolicy
doesn't work, but it isn't needed ...- Description for the changelog