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

MSC2379: Add /versions endpoint to Appservice API #2379

Open
wants to merge 8 commits into
base: old_master
Choose a base branch
from

Conversation

Half-Shot
Copy link
Contributor

@Half-Shot Half-Shot commented Dec 4, 2019

@Half-Shot Half-Shot changed the title MSC XXXX: Add supported appservice version to registration information MSC 2379: Add supported appservice version to registration information Dec 4, 2019
@Half-Shot Half-Shot added proposal A matrix spec change proposal proposal-in-review labels Dec 4, 2019
key SHOULD be specified by all bridges. When set, the homeserver MUST send requests to the endpoints
specified by that version of the AS spec.

Homeservers may optionally support an omitted value, which will make it support the legacy paths used
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This stuff is necessary so that we do not kill all bridges everywhere if we start supporting this, but it does suck from a spec standpoint of having to include it. Perhaps we don't need to spec this and leave it as an undocumented synapse feature to be removed at some point in the future?

@turt2live turt2live self-requested a review December 4, 2019 17:13
proposals/2379-appservice-versions.md Outdated Show resolved Hide resolved

## Proposal

The `registration` file should contain one new key: `as_version`.
Copy link
Member

Choose a reason for hiding this comment

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

Overall I'm not convinced we need this in the spec. Synapse already supports arbitrary properties on the registration file, and the registration file in the spec is not a super strict thing (like anything, it's extensible). It doesn't even have to be YAML.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but this isn't just about Synapse. This is about ensuring that bridges have a single way to tell any homeserver impl which paths they should be hitting, rather than each homeserver specifying their own way of doing this.

Right now any homeserver that wishes to implement the appservice API is not going to be following the spec anyway, because at least the matrix-org bridges do not implement the correct endpoints. Presumably there are other bridges out there too that may not have bothered to follow the spec, because synapse doesn't use it. Dendrite is also non-compliant, for ex.

Yes, we could fix the bug we have right now for Synapse onlu by ensuring that every bridge developer out there appends a synapse-only key to their registration format (or fixes their shit), but that seems a like a waste of time if we have this problem with other homeservers down the line.

Furthermore, this change also future proof the API when we inevitably change the format of the transaction format.

@Half-Shot
Copy link
Contributor Author

Spoke IRL to @anoadragon453 and came to the conclusion that this should be GET /versions, because it can live outside registration which the bridge may not have control over. It also lines up nicely with our other APIs.

@Half-Shot Half-Shot changed the title MSC 2379: Add supported appservice version to registration information MSC 2379: Add /versions endpoint to Appservice API Dec 5, 2019
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

I'm not entirely convinced we need to have the endpoint stuff in this MSC. Say an AS /versions is introduced into the spec through an MSC. You then release a new version of the AS API which has corrected paths. It's then implied that homeserver authors would need to deal with the legacy considerations that the /versions endpoint provides. If the bridge says it only supports vNext of the AS API, the homeserver should send to the new endpoints. If it doesn't or doesn't have a /versions endpoint, then the homeserver should use the legacy endpoints.

We don't need to include this transition information in the spec, it can all be done in implementation.

Additionally, the `{version}` for the Third party network routes is always set to `unstable`.

It should be reiterated that support for this is up to the homeserver implemetor. Homeservers may
refuse to load appservices that do not include this `key`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
refuse to load appservices that do not include this `key`.
refuse to load appservices that do not include this endpoint.

?

Keeping a 'legacy' mode around in the spec sucks, because it's horribly non-compliant to the version system.
However, most of the ecosystem has been modeled over Synapse behaviours which means this spec change would break
support for bridges if implemented by Synapse. This option remains the most pragmatic option. In a future version
of the spec, this mode could be removed.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
of the spec, this mode could be removed.
of the spec, this legacy behaviour could be removed.

@Half-Shot
Copy link
Contributor Author

I'm not entirely convinced we need to have the endpoint stuff in this MSC. Say an AS /versions is introduced into the spec through an MSC. You then release a new version of the AS API which has corrected paths. It's then implied that homeserver authors would need to deal with the legacy considerations that the /versions endpoint provides.

Sure. I'm also not convinced we needed this information in the MSC, but wanted it there for context. I guess we can leave this as an easter egg in synapse.

If the bridge says it only supports vNext of the AS API, the homeserver should send to the new endpoints. If it doesn't or doesn't have a /versions endpoint, then the homeserver should use the legacy endpoints.

That's the plan, yep.

We don't need to include this transition information in the spec, it can all be done in implementation.

Sure thing.

Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

Thanks, I think this will also be much less contentious.

but lacks a `unstable_features` key, and is hosted by the appservice rather than the homeserver.

All bridges MUST implement this endpoint and specify which version(s) of the `AS` API they support.
The homeserver MUST send requests to the endpoints specified by that version of the AS spec.
Copy link
Member

@anoadragon453 anoadragon453 Dec 5, 2019

Choose a reason for hiding this comment

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

Last thing I think I'd put in is a "Legacy Considerations" section that states if an appservice hasn't implemented /versions, a homeserver can choose to either assume it supports all AS API versions up until the one where this /versions endpoint was introduced, or just deny connecting to it at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...but that's what I just deleted 😢

Copy link
Member

Choose a reason for hiding this comment

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

I wanted the stuff about specifying new endpoints deleted, but this bit we should keep.

@turt2live turt2live changed the title MSC 2379: Add /versions endpoint to Appservice API MSC2379: Add /versions endpoint to Appservice API Dec 5, 2019
@turt2live turt2live added the kind:maintenance MSC which clarifies/updates existing spec label Apr 20, 2020
@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants