Skip to content
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

Merged
merged 2 commits into from
Apr 29, 2023

Conversation

Otterverse
Copy link
Member

This covers a case where the loopback adapter doesn't support multicast, such as in some docker containers (usually cross-architecture emulation.)

@viambot viambot added the safe to test Mark as safe to test label Apr 28, 2023
@Otterverse Otterverse marked this pull request as draft April 28, 2023 20:35
Copy link
Contributor

@edaniels edaniels left a 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

@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Apr 28, 2023
@Otterverse Otterverse changed the title RSDK-2545 Add check against zero-value net.Interface in mDNS RSDK-2545 Allow fail-through for mDNS Apr 28, 2023
Copy link
Member Author

@Otterverse Otterverse left a 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
Copy link
Member Author

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 {
Copy link
Member Author

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.

@Otterverse Otterverse marked this pull request as ready for review April 28, 2023 23:06
@Otterverse Otterverse requested a review from edaniels April 28, 2023 23:06
@Otterverse Otterverse merged commit 1c8b896 into viamrobotics:main Apr 29, 2023
@Otterverse Otterverse deleted the fix-mdns branch April 29, 2023 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test Mark as safe to test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants