-
Notifications
You must be signed in to change notification settings - Fork 57
Add deployment.GetFullyQualifiedHomeserverName(t, hsName)
to support custom Deployment
#780
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
base: main
Are you sure you want to change the base?
Conversation
Example: ``` alice.MustJoinRoom(t, bobRoomID, []string{ shardDeployment2.GetFullyQualifiedHomeserverName(t, "hs1"), }) ```
0e5756a
to
e5ff236
Compare
deployment.GetFullyQualifiedHomeserverName(hsName)
deployment.GetFullyQualifiedHomeserverName(hsName)
to support custom Deployment
deployment.GetFullyQualifiedHomeserverName(hsName)
to support custom Deployment
deployment.GetFullyQualifiedHomeserverName(t, hsName)
to support custom Deployment
…t.GetFullyQualifiedHomeserverName(...)` refactor
// MustJoinRoom joins the room ID or alias given, else fails the test. Returns the room ID. | ||
func (c *CSAPI) MustJoinRoom(t ct.TestLike, roomIDOrAlias string, serverNames []string) string { | ||
// | ||
// Args: | ||
// - `serverNames`: The list of servers to attempt to join the room through. | ||
// These should be a resolvable addresses within the deployment network. | ||
func (c *CSAPI) MustJoinRoom(t ct.TestLike, roomIDOrAlias string, serverNames []spec.ServerName) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, wherever we expect people to use deployment.GetFullyQualifiedHomeserverName(t, hsName)
, I've updated these function signatures to accept spec.ServerName
instead of just plain strings.
I also think this is more semantically correct for the places because this needs to be a resolvable homeserver address in the federation.
@@ -117,7 +117,7 @@ func MakeRespMakeKnock(s *Server, room *ServerRoom, userID string) (resp fclient | |||
// the current server is returned to the joining server. | |||
func SendJoinRequestsHandler(s *Server, w http.ResponseWriter, req *http.Request, expectPartialState bool, omitServersInRoom bool) { | |||
fedReq, errResp := fclient.VerifyHTTPRequest( | |||
req, time.Now(), spec.ServerName(s.serverName), nil, s.keyRing, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we've converted some of these types to spec.ServerName
, we no longer need the conversion.
// TODO: It feels like `ServersInRoom` should be `[]spec.ServerName` instead of `[]string` | ||
ServersInRoom: serversInRoomStrings, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like a gomatrixserverlib
problem.
(not going to fix in this PR)
@@ -63,7 +64,7 @@ func TestOutboundFederationSend(t *testing.T) { | |||
roomAlias := srv.MakeAliasMapping("flibble", serverRoom.RoomID) | |||
|
|||
// the local homeserver joins the room | |||
alice.MustJoinRoom(t, roomAlias, []string{deployment.GetConfig().HostnameRunningComplement}) | |||
alice.MustJoinRoom(t, roomAlias, []spec.ServerName{srv.ServerName()}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, this was just a mistake from before. Unclear how it worked before since it's missing the randomly assigned port.
// Returns the resolvable server name (host) of a homeserver given its short alias | ||
// (e.g., "hs1", "hs2"). | ||
// | ||
// In the case of the standard Docker deployment, this will be the same `hs1`, `hs2` | ||
// but may be different for other custom deployments (ex. | ||
// `shardDeployment1.GetFullyQualifiedHomeserverName(t, "hs1")` -> `hs1.shard1:8081`). | ||
GetFullyQualifiedHomeserverName(t ct.TestLike, hsName string) spec.ServerName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main change of this PR is here. It's a breaking change to the complement.Deployment
interface
The rest of the diff is essentially just updating to use this new utility.
func (d *Deployment) GetFullyQualifiedHomeserverName(t ct.TestLike, hsName string) spec.ServerName { | ||
_, ok := d.HS[hsName] | ||
if !ok { | ||
ct.Fatalf(t, "Deployment.GetFullyQualifiedHomeserverName - HS name '%s' not found", hsName) | ||
} | ||
// We have network aliases for each Docker container that will resolve the `hsName` to | ||
// the container. | ||
return spec.ServerName(hsName) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the implementation for the new GetFullyQualifiedHomeserverName
method that we added to the complement.Deployment
interface.
@@ -27,7 +28,9 @@ func TestPresence(t *testing.T) { | |||
|
|||
// to share presence alice and bob must be in a shared room | |||
roomID := alice.MustCreateRoom(t, map[string]interface{}{"preset": "public_chat"}) | |||
bob.MustJoinRoom(t, roomID, []string{"hs1"}) | |||
bob.MustJoinRoom(t, roomID, []spec.ServerName{ | |||
deployment.GetFullyQualifiedHomeserverName(t, "hs1"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the reviewer: I've tried to be thorough in updating everything on this list (from the PR description). This would be the main thing to think about. Are there other spots that we need to use GetFullyQualifiedHomeserverName(...)
instead of the hard-coded hs1
values?
- Update tests to use
GetFullyQualifiedHomeserverName(...)
- Join room (
MustJoinRoom
,JoinRoom
)- Knock room (
mustKnockOnRoomSynced
,knockOnRoomWithStatus
)srv.MustJoinRoom
,srv.MustLeaveRoom
,srv.MustSendTransaction
FederationClient
->fedClient.MakeJoin
,fedClient.SendJoin
, etcfclient
,fclient.NewFederationRequest
m.space.child
via
m.space.parent
via
m.room.join_rules
restricted
via
gomatrixserverlib.EDU
Destination
I've also reviewed the diff itself to ensure that I didn't accidentally swap hs1
for hs2
somewhere.
// the container. | ||
return spec.ServerName(hsName) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, I don't think the built-in Complement Deployment
implementation supports multiple Deployments
at the same time (hs1
, hs2
would conflict between them). Since one of the goals of this PR is to unlock that functionality for custom Deployment
's, it could make some sense to also refactor and update that here as well.
I'd rather leave it as-is until we need it or at-least do this in a follow-up PR.
See the PR description for more context on multiple Deployment
.
Split off from #778 per discussion,
Spawning from a real use-case with a custom
Deployment
/Deployer
(Element-internal).Introduce
complement.Deployment.GetFullyQualifiedHomeserverName(hsName)
to allow the per-deployment short homeserver aliases likehs1
to be mapped to something else that's resolvable in your custom deployments context. Example:hs1
->hs1.shard1:8481
.This is useful for situations like the following where you have to specify the via servers in a federated join request during a test:
But why does this have to be part of the
complement.Deployment
interface instead of your own custom deployment?complement.Deployment
interfacecomplement.Deployment
interfacecomplement.Deployment
back toCustomDeployment
in our own tests (and have the helper in theCustomDeployment
instead), if we start using something exotic in our out-of-repo Complement tests, the suite of existing Complement tests in this repo will not be compatible.(also see below)
Motivating custom
Deployment
use casecomplement.Deployment
is an interface that can be backed by anything. For reference, custom deployments were introduced in #750. The defaultDeployment
naming scheme in Complement ishs1
,hs2
, etc. It's really nice and convenient to be able to simply refer to homeservers ashs1
, etc within a deployment. And using consistent names like this makes tests compatible with each other regardless of whichDeployment
is being used.The built-in
Deployment
in Complement has each homeserver in a Docker container which already has network aliases likehs1
,hs2
, etc so no translation is needed from friendly name to resolvable address. When one homeserver needs to federate with the other, it can simply make a request tohttps://hs1:8448/...
per spec on resolving server names.Right-now, we hard-code
hs1
across the tests when we specify "via" servers in join requests but that only works if you follow the strict single-deployment naming scheme.complement/tests/federation_rooms_invite_test.go
Line 112 in 6b63eff
But imagine a case where we have multiple
Deployment
and we want the homeservers to communicate with each other. If we keep using the consistenths1
,hs2
naming scheme for eachDeployment
we're going to have conflicts. This is wheredeployment.GetFullyQualifiedHomeserverName(t, hsName)
comes in handy. We can calldeployment1.GetFullyQualifiedHomeserverName(t, "hs1")
->hs1.shard1
and alsodeployment2.GetFullyQualifiedHomeserverName(t, "hs1")
->hs1.shard2
to get their unique resolvable addresses in the network.Additionally, the helper removes the constraint of needing the network to strictly resolve
hs1
,hs2
hostnames to their respective homeservers. Whenever you need to refer to another homeserver, usedeployment.GetFullyQualifiedHomeserverName(hsName)
to take care of the nuance of environment that the givenDeployment
creates.Todo
GetFullyQualifiedHomeserverName(...)
MustJoinRoom
,JoinRoom
)mustKnockOnRoomSynced
,knockOnRoomWithStatus
)srv.MustJoinRoom
,srv.MustLeaveRoom
,srv.MustSendTransaction
FederationClient
->fedClient.MakeJoin
,fedClient.SendJoin
, etcfclient
,fclient.NewFederationRequest
m.space.child
via
m.space.parent
via
m.room.join_rules
restricted
via
gomatrixserverlib.EDU
Destination
Deployment
implementation to support multiple deployments at the same time, tracked by this discussion belowPull Request Checklist