-
Notifications
You must be signed in to change notification settings - Fork 46
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
RSDK-2545 Allow fail-through for mDNS #163
Conversation
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 if tested locally in and out of docker on linux+mac
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.
Rewrote from first version with fail-through. (Previous solution disabled mDNS even on working linux systems, so was a no-go.)
logger, | ||
) | ||
if err != nil { | ||
return nil, err | ||
logger.Warnw(mDNSerr, "error", err) | ||
sOpts.disableMDNS = true |
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.
Technically not needed, but code is complex enough that IF any other logic were added to look at this value later, it'd be good to have it reflect the actual situation IMHO.
if (ifc.Flags&net.FlagUp) == 0 || | ||
(ifc.Flags&net.FlagLoopback) == 0 || | ||
(ifc.Flags&net.FlagMulticast) == 0 { | ||
if (ifc.Flags&net.FlagUp) == 0 || (ifc.Flags&net.FlagLoopback) == 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 by default linux doesn't have the "multicast" flag set on the loopback interface, so I removed that check. It appears that you can join a group on an interface without that being set explicitly, and all tests still pass.
This covers a case where the loopback adapter doesn't support multicast, such as in some docker containers (usually cross-architecture emulation.)