Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Implement MSC1708 (.well-known lookups for server routing) #4489

Merged
merged 14 commits into from
Jan 29, 2019

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Jan 28, 2019

Based on #4488.

Turns out that the library does a better job of parsing URIs than our
reinvented wheel. Who knew.

There are two things going on here. The first is that, unlike
parse_server_name, URI.fromBytes will strip off square brackets from IPv6
literals, which means that it is valid input to ClientTLSOptionsFactory and
HostnameEndpoint.

The second is that we stay in `bytes` throughout (except for the argument to
ClientTLSOptionsFactory), which avoids the weirdness of (sometimes) ending up
with idna-encoded values being held in `unicode` variables. TBH it probably
would have been ok but it made the tests fragile.
This is going to get too big and unmanageable.
We don't want to be doing .well-known lookups on these guys.
@richvdh richvdh requested a review from a team January 28, 2019 00:14
@codecov-io
Copy link

codecov-io commented Jan 28, 2019

Codecov Report

Merging #4489 into develop will decrease coverage by <.01%.
The diff coverage is 67.39%.

@@             Coverage Diff             @@
##           develop    #4489      +/-   ##
===========================================
- Coverage    74.76%   74.75%   -0.01%     
===========================================
  Files          336      336              
  Lines        34180    34224      +44     
  Branches      5559     5567       +8     
===========================================
+ Hits         25553    25583      +30     
- Misses        7051     7061      +10     
- Partials      1576     1580       +4

Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

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

Looks sane, would be nice to have some more comments/docstrings about what's going on thought 👍


body = yield make_deferred_yieldable(readBody(response))

logger.info("Response from .well-known: %s", body)
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be debug

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm minded to leave it as info for now as otherwise it's quite hard to see what's going on. I've moved it down a bit to print out the parsed response though.

@richvdh richvdh force-pushed the rav/fed_routing/well-known branch 2 times, most recently from 82b31b9 to f3b50e0 Compare January 28, 2019 13:00
@richvdh richvdh requested a review from erikjohnston January 28, 2019 13:52
two reasons for this. One, it saves a bunch of boilerplate. Two, it squashes
unicode to IDNA-in-a-`str` (even on python 3) in a way that it turns out we
rely on to give consistent behaviour between python 2 and 3.
@richvdh
Copy link
Member Author

richvdh commented Jan 28, 2019

(have merged in #4497 in an attempt to get the tests to work)

@richvdh richvdh merged commit 99e36d5 into develop Jan 29, 2019
@richvdh richvdh deleted the rav/fed_routing/well-known branch January 29, 2019 13:53
turt2live added a commit to matrix-org/matrix-spec-proposals that referenced this pull request Jan 31, 2019
Original proposals:
* #1708 (note: the JSON requirements were softened by #1824)
* #1711

Implementation proofs:
* matrix-org/synapse#4489
* No explicit PRs for MSC1711 could be found, however Synapse is known to implement it.

There are no intentional changes which differ from the proposals in this commit, however the author has relied upon various historical conversations outside of the proposals to gain the required context. Inaccuracies introduced by the author are purely accidental.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants