Client: Add SRV-based virtual hosting support #2930

hoffie · 2022-10-20T21:27:05Z

Short description of changes

Internal: Refactor NetworkUtil::ParseNetworkAddress
This moves the host elaborate and reusable host name lookup logic to its own ParseNetworkAddressName method.
Client: Add SRV-based virtual hosting support
When connecting to host names without port (e.g. example.org), the client will now perform an SRV lookup to _jamulus._udp.example.org. If this lookup returns exactly one result, the result is used to select the actual target address/port.

If the lookup does not return a usable result, the regular connect logic kicks in (A lookup, default port).

CHANGELOG: Client: Add SRV-based virtual hosting support

Context: Fixes an issue?
Related: https://github.com/orgs/jamulussoftware/discussions/1772

Does this change need documentation? What needs to be documented and how?
Website, Server administration

Status of this Pull Request
Ready for testing/discussion.

What is missing until this pull request can be merged?
Ready for testing/discussion.

Checklist

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

hoffie · 2022-10-20T21:28:39Z

src/util.cpp

+
+bool NetworkUtil::ParseNetworkAddressWithSrvDiscovery ( QString strAddress, CHostAddress& HostAddress, bool bEnableIPv6 )
+{
+    // Try SRV-based discovery first:


Trying SRV first is the saner behavior. It comes at the cost that everyone has to do the SRV lookup and wait for the (possibly empty) reply.

Agreed. The DNS server should fail fairly fast if the entry isn't there. At worst it should double the lookup time.

src/util.cpp

This moves the host elaborate and reusable host name lookup logic to its own ParseNetworkAddressName method. Co-authored-by: Peter L Jones <peter@drealm.info>

ann0see

Could you please #ifndef the code? I think some "raw jamulus" people might not want this feature (although it's small)

hoffie · 2022-10-30T20:45:32Z

Could you please #ifndef the code? I think some "raw jamulus" people might not want this feature (although it's small)

Done now (QMAKE_CXXFLAGS+="-DCLIENT_NO_SRV_CONNECT"). I don't think it warrants its own qmake flag though. I've also skipped #ifdef'ing one constant, which will be optimized away by the compiler anyway.

ann0see · 2022-11-11T20:57:29Z

As soon as the DNS records are propagated (and I didn't make a mistake in the format), I can test this.

ann0see · 2022-11-11T21:44:04Z

Ok. Tested ok.

src/util.cpp

ann0see · 2022-11-12T21:06:52Z

src/util.cpp

+    // This is not nice and blocks the UI, but is similar to what
+    // the regular resolve function does as well.
+    QTime dieTime = QTime::currentTime().addMSecs ( DNS_SRV_RESOLVE_TIMEOUT_MS );
+    while ( QTime::currentTime() < dieTime && !dns->isFinished() )


Not a fan of busy waiting (even if it's limited). Isn't there any blocking function or something like a signal?

You're asking for a Qt equivalent of AA/AAAA lookup for SRV names? (I'd guess all the Qt AA/AAAA lookup does is something very like this.)

Isn't there any blocking function

The existing code path uses QHostInfo::fromName, which is a blocking function. I haven't found any equivalent for SRV lookups.

or something like a signal?

The QDnsLookup, which I've used here, is supposed to be used via signals. However, this requires -- as far as I can see -- an inversion of the control flow as the existing logic relies on blocking. Even when that refactoring is done, new questions come up: How should the UI behave? Currently it blocks. If it continues to stay responsive during lookup, we would need some other kind of indicator, so that's kind of a rabbit hole. Not saying that it shouldn't be done, but rather that it's a larger refactoring which I didn't want to do as part of this PR.

In short: That's the best and least intrusive way I've found. If someone finds a better way, I'm certainly open to it. :)

ann0see · 2022-11-12T21:09:14Z

src/util.cpp

+    {
+        // RFC2782 says that "." indicates that the service is not available.
+        // (Qt strips the trailing dot, which is why we check for empty string
+        // as well)


Does this mean we might have two dots? (When and why) is this valid? Why don't we check just for the empty string?

The code basically covers both what the RFC states plus what I've practically observed during testing on Linux. As I haven't tested all other platforms or different DNS implementations, I'd go with this duplicate check -- it's about the failure path anyway. It might be sufficient to check for the empty string, but it would be some effort to prove, I guess?

At least add a comment here that the dot check might not be needed.

src/util.cpp

When connecting to host names without port (e.g. example.org), the client will now perform an SRV lookup to _jamulus._udp.example.org. If this lookup returns exactly one result, the result is used to select the actual target address/port. If the lookup does not return a usable result, the regular connect logic kicks in (A lookup, default port). Related: https://github.com/orgs/jamulussoftware/discussions/1772

ann0see · 2022-11-15T06:41:37Z

I still don't feel comfortable with the dot/empty string workaround. The Qt docs don't seem to give much insight. Why does the dot get removed?

hoffie · 2022-11-15T06:55:22Z

I still don't feel comfortable with the dot/empty string workaround. The Qt docs don't seem to give much insight. Why does the dot get removed?

Qt uses the OS' libresolv on *nix (usually glibc). glibc has logic to remove a leading dot, which also covers names consisting only of a dot. Finding that this is (potentially) platform-specific behavior confirms that it is a good idea to handle both cases, IMO.

ann0see · 2022-11-15T11:52:09Z

Hmm ok. Approving and merging then.

ann0see · 2023-07-24T20:32:32Z

Dropped needs documentation as the next-release page is up to date: https://github.com/jamulussoftware/jamuluswebsite/blob/next-release/wiki/en/Unregistered-Servers.md#dns-srv-record-support

hoffie added the needs documentation PRs requiring documentation changes or additions label Oct 20, 2022

hoffie added this to the Release 3.10.0 milestone Oct 20, 2022

hoffie added this to Triage in Tracking (old) via automation Oct 20, 2022

hoffie commented Oct 20, 2022

View reviewed changes

pljones reviewed Oct 21, 2022

View reviewed changes

src/util.cpp Outdated Show resolved Hide resolved

pljones reviewed Oct 21, 2022

View reviewed changes

src/util.cpp Outdated Show resolved Hide resolved

pljones reviewed Oct 21, 2022

View reviewed changes

src/util.cpp Show resolved Hide resolved

Internal: Refactor NetworkUtil::ParseNetworkAddress

24adc20

This moves the host elaborate and reusable host name lookup logic to its own ParseNetworkAddressName method. Co-authored-by: Peter L Jones <peter@drealm.info>

hoffie force-pushed the client-srv-support branch from b5ceddd to be70a6d Compare October 21, 2022 21:57

ann0see reviewed Oct 22, 2022

View reviewed changes

hoffie force-pushed the client-srv-support branch from be70a6d to 4ccd0b4 Compare October 30, 2022 20:44

hoffie moved this from Triage to Waiting on Team in Tracking (old) Oct 30, 2022

hoffie requested a review from pljones November 10, 2022 11:54

pljones approved these changes Nov 10, 2022

View reviewed changes

hoffie requested a review from ann0see November 10, 2022 20:08

ann0see reviewed Nov 12, 2022

View reviewed changes

hoffie force-pushed the client-srv-support branch from 4ccd0b4 to ed18ad6 Compare November 14, 2022 20:38

ann0see approved these changes Nov 15, 2022

View reviewed changes

Tracking (old) automation moved this from Waiting on Team to In Progress Nov 15, 2022

ann0see merged commit 26701d9 into jamulussoftware:master Nov 15, 2022

Tracking (old) automation moved this from In Progress to Done Nov 15, 2022

ginjo mentioned this pull request Nov 17, 2022

Updates Running-a-server.md to cover DNS SRV support jamulussoftware/jamuluswebsite#874

Closed

3 tasks

ann0see removed the needs documentation PRs requiring documentation changes or additions label Jul 24, 2023

pljones removed this from Done in Tracking (old) Jul 28, 2023

pljones assigned hoffie Jul 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client: Add SRV-based virtual hosting support #2930

Client: Add SRV-based virtual hosting support #2930

hoffie commented Oct 20, 2022 •

edited

Loading

hoffie Oct 20, 2022

pljones Oct 21, 2022

ann0see left a comment

hoffie commented Oct 30, 2022

ann0see commented Nov 11, 2022

ann0see commented Nov 11, 2022

ann0see Nov 12, 2022

pljones Nov 12, 2022 •

edited

Loading

hoffie Nov 13, 2022

ann0see Nov 12, 2022

hoffie Nov 13, 2022

ann0see Nov 14, 2022

hoffie Nov 14, 2022

ann0see commented Nov 15, 2022

hoffie commented Nov 15, 2022 •

edited

Loading

ann0see commented Nov 15, 2022

ann0see commented Jul 24, 2023

Client: Add SRV-based virtual hosting support #2930

Client: Add SRV-based virtual hosting support #2930

Conversation

hoffie commented Oct 20, 2022 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ann0see left a comment

Choose a reason for hiding this comment

hoffie commented Oct 30, 2022

ann0see commented Nov 11, 2022

ann0see commented Nov 11, 2022

Choose a reason for hiding this comment

pljones Nov 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ann0see commented Nov 15, 2022

hoffie commented Nov 15, 2022 • edited Loading

ann0see commented Nov 15, 2022

ann0see commented Jul 24, 2023

hoffie commented Oct 20, 2022 •

edited

Loading

pljones Nov 12, 2022 •

edited

Loading

hoffie commented Nov 15, 2022 •

edited

Loading