Skip to content

log error instead if disabling IPv6 router advertisement failed#2563

Merged
arkodg merged 1 commit intomoby:masterfrom
thaJeztah:no_error
Jun 12, 2020
Merged

log error instead if disabling IPv6 router advertisement failed#2563
arkodg merged 1 commit intomoby:masterfrom
thaJeztah:no_error

Conversation

@thaJeztah
Copy link
Member

addresses docker/for-linux#1033

Previously, failing to disable IPv6 router advertisement prevented the daemon to start.

An issue was reported by a user that started docker using systemd-nspawn "machine",
which produced an error;

failed to start daemon: Error initializing network controller:
Error creating default "bridge" network: libnetwork:
Unable to disable IPv6 router advertisement:
open /proc/sys/net/ipv6/conf/docker0/accept_ra: read-only file system

This patch changes the error to a log-message instead.

@thaJeztah
Copy link
Member Author

@justincormack @arkodg @samuelkarp PTAL

return fmt.Errorf("libnetwork: Unable to disable IPv6 router advertisement: %v", err)
logrus.WithError(err).Info("unable to disable IPv6 router advertisement")
}
return nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function can no longer return anything except nil so you should change the type to no longer return an error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't change it because it has to match the setupStep type

type setupStep func(*networkConfiguration, *bridgeInterface) error

@arkodg
Copy link
Contributor

arkodg commented Jun 5, 2020

@thaJeztah now that we have incorporated the security patch, can we please utilize this library into setupDefaultSysctl - https://github.com/moby/libnetwork/blob/master/osl/kernel/knobs_linux.go similar to

loadBalancerConfig = map[string]*kernel.OSValue{

its best effort and logs errors

}
if err := ioutil.WriteFile(sysPath, []byte{'0', '\n'}, 0644); err != nil {
return fmt.Errorf("libnetwork: Unable to disable IPv6 router advertisement: %v", err)
logrus.WithError(err).Info("unable to disable IPv6 router advertisement")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to see this as a Warn rather than Info. If we switch this patch to use https://github.com/moby/libnetwork/blob/master/osl/kernel/knobs_linux.go as suggested by @arkodg, failure to write will get logged as Error.

@thaJeztah
Copy link
Member Author

Ok, let me update the Info to a Warn

@arkodg I think I prefer to do the refactor in a follow-up directly after, as this was a regression, and want to keep the diff small for the 19.03 branch (I agree with looking at refactoring though)

Previously, failing to disable IPv6 router advertisement prevented the daemon to
start.

An issue was reported by a user that started docker using `systemd-nspawn "machine"`,
which produced an error;

    failed to start daemon: Error initializing network controller:
    Error creating default "bridge" network: libnetwork:
    Unable to disable IPv6 router advertisement:
    open /proc/sys/net/ipv6/conf/docker0/accept_ra: read-only file system

This patch changes the error to a log-message instead.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

@arkodg @samuelkarp @cpuguy83 updated to print a warning, I'll have a look at the refactor later

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@arkodg arkodg merged commit 9e99af2 into moby:master Jun 12, 2020
@thaJeztah thaJeztah deleted the no_error branch June 12, 2020 18:12
thaJeztah added a commit to thaJeztah/docker that referenced this pull request Jul 8, 2020
full diff: moby/libnetwork@2e24aed...9e99af2

- moby/libnetwork#2548 Add docker interfaces to firewalld docker zone
    - fixes docker/for-linux#957 DNS Not Resolving under Network [CentOS8]
    - fixes moby/libnetwork#2496 Port Forwarding does not work on RHEL 8 with Firewalld running with FirewallBackend=nftables
- store.getNetworksFromStore() remove unused error return
- moby/libnetwork#2554 Fix 'failed to get network during CreateEndpoint'
    - fixes/addresses docker/for-linux#888 failed to get network during CreateEndpoint
- moby/libnetwork#2558 [master] bridge: disable IPv6 router advertisements
- moby/libnetwork#2563 log error instead if disabling IPv6 router advertisement failed
    - fixes docker/for-linux#1033 Shouldn't be fatal: Unable to disable IPv6 router advertisement: open /proc/sys/net/ipv6/conf/docker0/accept_ra: read-only file system

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Jul 13, 2020
full diff: moby/libnetwork@2e24aed...9e99af2

- moby/libnetwork#2548 Add docker interfaces to firewalld docker zone
    - fixes docker/for-linux#957 DNS Not Resolving under Network [CentOS8]
    - fixes moby/libnetwork#2496 Port Forwarding does not work on RHEL 8 with Firewalld running with FirewallBackend=nftables
- store.getNetworksFromStore() remove unused error return
- moby/libnetwork#2554 Fix 'failed to get network during CreateEndpoint'
    - fixes/addresses docker/for-linux#888 failed to get network during CreateEndpoint
- moby/libnetwork#2558 [master] bridge: disable IPv6 router advertisements
- moby/libnetwork#2563 log error instead if disabling IPv6 router advertisement failed
    - fixes docker/for-linux#1033 Shouldn't be fatal: Unable to disable IPv6 router advertisement: open /proc/sys/net/ipv6/conf/docker0/accept_ra: read-only file system

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Upstream-commit: 219e7e7ddcf5f0314578d2a517fc0832f03622c1
Component: engine
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants