Only include loopback address in responses when configured to #173
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
edaniels
commented
Feb 5, 2024
| @@ -178,6 +185,11 @@ func (c *Conn) Query(ctx context.Context, name string) (dnsmessage.ResourceHeade | |||
| case <-c.closed: | |||
| return dnsmessage.ResourceHeader{}, nil, errConnectionClosed | |||
| case res := <-queryChan: | |||
| // Given https://datatracker.ietf.org/doc/html/draft-ietf-mmusic-mdns-ice-candidates#section-3.2.2-2 | |||
There was a problem hiding this comment.
I was wondering why we don't collect all responses and either way, we're not supposed to!
edaniels
commented
Feb 5, 2024
conn.go
Outdated
| @@ -278,7 +290,7 @@ func (c *Conn) writeToSocket(ifIndex int, b []byte, onlyLooback bool) { | |||
| } | |||
| } | |||
|
|
|||
| func (c *Conn) sendAnswer(name string, ifIndex int, dst net.IP) { | |||
| func (c *Conn) sendAnswer(name string, ifIndex int, addr net.IP) { | |||
edaniels
commented
Feb 5, 2024
| @@ -242,14 +254,14 @@ func (c *Conn) sendQuestion(name string) { | |||
| c.writeToSocket(0, rawQuery, false) | |||
| } | |||
|
|
|||
| func (c *Conn) writeToSocket(ifIndex int, b []byte, onlyLooback bool) { | |||
| func (c *Conn) writeToSocket(ifIndex int, b []byte, srcIfcIsLoopback bool) { | |||
edaniels
commented
Feb 5, 2024
| // increased the chance that we send a loopback response to a Query that is | ||
| // unwillingly to use loopback addresses (the default in pion/ice). | ||
| for i := 0; i < 100; i++ { | ||
| _, addr, err = bServer.Query(context.TODO(), "pion-mdns-2.local") |
There was a problem hiding this comment.
This should prevent future mishaps but it's racy. From what we know on pion/webrtc#2625, it easily fails within 10 attempts since loopback has a higher chance of routing back to itself faster than a LAN mDNS response (I think)
edaniels
commented
Feb 5, 2024
| @@ -95,6 +110,81 @@ func TestValidCommunicationWithAddressConfig(t *testing.T) { | |||
| check(aServer.Close(), t) | |||
| } | |||
|
|
|||
| func TestValidCommunicationWithLoopbackAddressConfig(t *testing.T) { | |||
There was a problem hiding this comment.
I should have added this test originally since I was intended on increasing the chances we get loopback addresses for when I was working on some offline code.
edaniels
force-pushed
the
fix_webrtc_2625
branch
from
8284b94 to
070c9b7
Compare
edaniels
commented
Feb 5, 2024
| if err != nil { | ||
| return nil, err | ||
| ifaces := config.Interfaces | ||
| if ifaces == nil { |
edaniels
commented
Feb 5, 2024
| } | ||
|
|
||
| inboundBufferSize := 0 | ||
| joinErrCount := 0 | ||
| ifacesToUse := make([]net.Interface, 0, len(ifaces)) | ||
| for i, ifc := range ifaces { | ||
| if err = conn.JoinGroup(&ifaces[i], &net.UDPAddr{IP: net.IPv4(224, 0, 0, 251)}); err != nil { | ||
| if !config.IncludeLoopback && ifc.Flags&net.FlagLoopback == net.FlagLoopback { |
edaniels
force-pushed
the
fix_webrtc_2625
branch
5 times, most recently
from
829b3fe to
03dbda5
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #173 +/- ##
==========================================
- Coverage 66.09% 61.65% -4.45%
==========================================
Files 1 1
Lines 292 412 +120
==========================================
+ Hits 193 254 +61
- Misses 70 123 +53
- Partials 29 35 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
edaniels
commented
Feb 5, 2024
| continue | ||
| var localAddress net.IP | ||
|
|
||
| // prefer the address of the interface if we know its index, but otherwise |
There was a problem hiding this comment.
this took a while to figure out but will provide further network path efficiencies but really it was to fix tests on linux :)
edaniels
force-pushed
the
fix_webrtc_2625
branch
6 times, most recently
from
1c2dcde to
3503c75
Compare
edaniels
commented
Feb 6, 2024
| t.Fatalf("Expected(%v) and Actual(%v) IP don't match", expectedIP, actualIP4) | ||
| } | ||
|
|
||
| expectedIP = []byte{0, 0, 0, 1} |
There was a problem hiding this comment.
the previous ipToBytes was broken for this case
edaniels
force-pushed
the
fix_webrtc_2625
branch
from
3503c75 to
4481e04
Compare
edaniels
force-pushed
the
fix_webrtc_2625
branch
from
4481e04 to
ac21988
Compare
This was referenced Feb 6, 2024
Closed
enobufs
approved these changes
Feb 8, 2024
| @@ -178,6 +185,11 @@ func (c *Conn) Query(ctx context.Context, name string) (dnsmessage.ResourceHeade | |||
| case <-c.closed: | |||
| return dnsmessage.ResourceHeader{}, nil, errConnectionClosed | |||
| case res := <-queryChan: | |||
| // Given https://datatracker.ietf.org/doc/html/draft-ietf-mmusic-mdns-ice-candidates#section-3.2.2-2 | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes pion/webrtc#2625
Details:
This fixes a regression from 608f20b where by properly sending mDNS responses to all interfaces, we significantly increased the chance that we send a loopback response to a Query that is unwillingly to use loopback addresses (the default in pion/ice).