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

MSC3723: Federation /versions #3723

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ShadowJonathan
Copy link
Contributor

@ShadowJonathan ShadowJonathan commented Feb 11, 2022

@ShadowJonathan ShadowJonathan changed the title MSCXXXX: Federation /versions MSC3723: Federation /versions Feb 11, 2022
Comment on lines +46 to +48
Furthermore, it could be useful that the set of supported matrix
versions are disjointed between the client-version `/versions` and a server-server `/versions`
endpoint, as federation endpoints may have a longer "lifetime" than client-server ones might.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I wrote this, I realised that this is effectively trying to mimic what the pre-global-versioning "server-server" spec tried to do in spirit.

I'm personally convinced that disjointing the version range that a federation-side of a server exposes makes sense, as federation could become much more unstable if changes happen quickly, as it could fragment that federation.

(If the SCT or experienced community members think this is not a good idea (possibly for much the same reasons the global version MSC was instigated in the first place), i'm 100% fine with requiring this to be matched to exactly the same range /client/versions would respond with this.)

Comment on lines +33 to +35
As such, future MSCs can rely on the fact that their intended behaviour does not need explicit
backwards compatibility detection cases, and could reliably assume the other server (can) know this
server supports its endpoint(s) with this version check.
Copy link
Member

Choose a reason for hiding this comment

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

it's still an http hit though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but it's not aiming to eliminate that (though caching helps), it tries to help future MSCs in not having to describe version-incompatibility-related fallback cases. ("If this endpoint returns 404, then assume this server does not understand the endpoint, and fall back to this older endpoint")

With /versions, the server instead can reliably understand what endpoints the other server does support, and model its subsequent server behaviour just off of that, this is entirely analogous to client's /versions in that sense. It could help reduce a lot of spec cruft, and help reduce implementation and S2S specification complexity.

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't help unstable implementations though as this MSC doesn't carry over the unstable_features description. Even if it did though, I don't really see the benefit of using /versions over 404 semantics.

It's the same number of words and same amount of resources to do either.

Copy link
Contributor Author

@ShadowJonathan ShadowJonathan Feb 11, 2022

Choose a reason for hiding this comment

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

I think this variant would (in the long term) be more stable, clokep mentioned in #spec that unknown endpoints don't really have a consistent error value. Plus, an unknown-endpoint-404 can be confused with a resource-not-found-404. I think every endpoint soft-deprecated that way would then have their 404-semantics have to be dealt with individually and separately. They could differ per implementation, which could lead to more implementation and spec complexity to move the spec forward on federation.

If unstable_features does fit this MSC though, I can add it. (Initially I didn't, because I thought that'd be stretching the initial usability of this endpoint, though now im convinced otherwise)

It's the same number of words and same amount of resources to do either.

(Maintenance) resources might, but eventual implementation complexity most likely not.

Even if, I personally believe that dealing with version incompatibilities between different components of the matrix ecosystem, as a fact, shouldn't have to be done by probing the other component, but by mutually agreeing on a set of supported versions within a range, it's a standard and method that's time-tested (with things such as version and cipher-suite selection in SSL/TLS).

Copy link
Contributor Author

@ShadowJonathan ShadowJonathan Feb 11, 2022

Choose a reason for hiding this comment

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

My main concern is the ramping requirement of implementation complexity, the federation API today is fairly simple, and I agree that (now) it's easy to implement fallbacks, but that's likely not possible once we get a sufficiently enough complex federation API where some kind of compatibility needs to be noted up-front, with things such as partial mesh federation.

I'll ask about endpoint probing in #spec later, I think that there's some bits I'm missing, I'm most likely overestimating the issue, but I'm also trying to project all those issues into the future, where they may turn from mere paper cuts to spec cruft and overdue technical debt.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect it might be simpler to spec what an unknown endpoint is supposed to return. The implication of this endpoint is that a sever needs to use it for every other server and then cache that info for an undefined period of time.

As @turt2live already mentioned it doesn't help with experimental stuff so we kind of need to spec that anyway. I'd be in favor of doing that instead.

Copy link
Member

@turt2live turt2live Feb 12, 2022

Choose a reason for hiding this comment

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

To summarize my thoughts: I think this MSC is probably fine, but it doesn't really seem to have a use-case at the moment, so might be best for the backburner while we wait for another MSC which really needs it to help push it forward.

I'm not opposed to the idea of /versions on all the API surfaces, just questioning how practical/fulfilling it would be for a change like this to land today rather than later.

Copy link
Contributor Author

@ShadowJonathan ShadowJonathan Feb 12, 2022

Choose a reason for hiding this comment

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

I'll add in the unstable_features object as well, it seems it's better to add here than to not.

Edit: Added that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect it might be simpler to spec what an unknown endpoint is supposed to return.

To cross-reference, this is essentially #1492.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To cross-reference, this is essentially #1492.

For future reference, this is now matrix-org/matrix-spec#339

@turt2live turt2live added kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal proposal-in-review s2s Server-to-Server API (federation) labels Feb 11, 2022
@turt2live
Copy link
Member

(keeping needs-implementation label until an implementation which demonstrates how this is needed is added - at the moment the current implementations add the endpoint, but the use case isn't really there)

Which returns a JSON object, which will contain at least one key, `versions`, with an array of
supported matrix versions, in the same format as `/client/versions`.

This JSON object can also include a `unstable_features` object, with a mapping of feature names to
Copy link
Contributor

Choose a reason for hiding this comment

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

While I can somewhat see the benefit of unstable_features somewhat, I am totally unconvinced that this MSC actually solves a real problem otherwise.

At best it provides some mechanism for two homeservers to identify that they are trying to talk two different versions of the spec, but without a solution as to how to deal with that case. Unless we're either going to set the expectation that servers have to maintain legacy support for possibly deprecated endpoints, parameters or fields, or we're going to simply refuse to talk to servers that claim to support a version that we don't, what good does it do to know either way?

Compliance with a specific API version is incredibly difficult to prove and a server can simply lie about it, like how Conduit outright lied to the federation about which room versions it supports or how Dendrite claimed the wrong CS API versions for a while and no one noticed, which leaves us in a situation that's exactly the same as today: the only real option you have is to just try hitting the endpoint anyway and see what happens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal s2s Server-to-Server API (federation)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

/federation/versions equivalent to /client/versions is missing.
4 participants