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

patch dendrite microservices with bind config #795

Merged
merged 6 commits into from Oct 2, 2019

Conversation

@aditsachde
Copy link
Contributor

commented Sep 22, 2019

This PR adds a block in the dendrite config for the services to bind to. The microservices should bind to the addresses in the bind block, and will be contacted at the address in the listen block.

This fixes an issue with the microservices and kubernetes services.

Pull Request Checklist

  • I have added any new tests that need to pass to testfile as specified in docs/sytest.md
  • Pull request includes a sign off
Adds bind block to the dendrite config.
microservices bind to address in this block, and talk to other microservices via the address in the listen block
@aditsachde aditsachde force-pushed the aditsachde:bindpatch branch from ba46a15 to 9e95d59 Sep 22, 2019
Allows fallback to listen block if bind block not specified
@aditsachde

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2019

I added a check that causes the service to listen on the address specified in the listen block if no specific bind addresses have been specified.

I'm not really sure how to proceed with tests, it looks like a test config is generated in test/config.go with the function assignAddress(). In a testing environment, the listen address and bind address will likely be the same, so if something like cfg.Bind.ClientAPI = assignAddress() is added alongside cfg.Listen.ClientAPI = assignAddress() for each service, the bind and listen addresses will be different causing the tests to fail.

Also, I noticed the appservice server was listening on the address specified for the federation sender server so that should be fixed.


Signed-off-by: Adit Sachde me@aditsachde.com

@aditsachde aditsachde marked this pull request as ready for review Sep 22, 2019
Copy link
Member

left a comment

I'm not really sure how to proceed with tests ...

Mm, I'm not too familiar with the unit tests myself unfortunately, but my naive thought if you can just modify assignAddress to return the port twice, and assign that to both bind/listen config values. @Cnly Do you have any input here?

Also, I noticed the appservice server was listening on the address specified for the federation sender server so that should be fixed.

Ah! Good catch, I'll fix this up, thanks :)

cmd/dendrite-appservice-server/main.go Outdated Show resolved Hide resolved
@aditsachde

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2019

Mm, I'm not too familiar with the unit tests myself unfortunately, but my naive thought if you can just modify assignAddress to return the port twice, and assign that to both bind/listen config values.

In test/config.go, cfg.Listen.ClientAPI is defined as cfg.Listen.ClientAPI = assignAddress(). Could cfg.Bind.ClientAPI then be defined like cfg.Bind.ClientAPI = cfg.Listen.ClientAPI?

	cfg.Bind.ClientAPI = cfg.Listen.ClientAPI
	cfg.Bind.FederationAPI = cfg.Listen.FederationAPI
	cfg.Bind.MediaAPI = cfg.Listen.MediaAPI
	cfg.Bind.RoomServer = cfg.Listen.RoomServer
	cfg.Bind.SyncAPI = cfg.Listen.SyncAPI
	cfg.Bind.PublicRoomsAPI = cfg.Listen.PublicRoomsAPI
	cfg.Bind.TypingServer = cfg.Listen.TypingServer
Copy link
Member

left a comment

Could cfg.Bind.ClientAPI then be defined like cfg.Bind.ClientAPI = cfg.Listen.ClientAPI?

I believe that would likely work, yes. Otherwise I have just one small suggestion below.

common/basecomponent/base.go Outdated Show resolved Hide resolved
aditsachde and others added 3 commits Oct 1, 2019
Co-Authored-By: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
@aditsachde

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2019

I believe all the changes should be resolved, just need buildkite to run

@aditsachde aditsachde requested a review from anoadragon453 Oct 2, 2019
@anoadragon453 anoadragon453 dismissed their stale review Oct 2, 2019

old

Copy link
Member

left a comment

LGTM. Thanks!

@anoadragon453 anoadragon453 merged commit 7d77538 into matrix-org:master Oct 2, 2019
8 checks passed
8 checks passed
buildkite/dendrite Build #296 passed (11 minutes, 35 seconds)
Details
buildkite/dendrite/build-slash-go-1-dot-11 Passed (1 minute, 10 seconds)
Details
buildkite/dendrite/build-slash-go-1-dot-12 Passed (54 seconds)
Details
buildkite/dendrite/lint-slash-go-1-dot-12 Passed (2 minutes, 47 seconds)
Details
buildkite/dendrite/pipeline Passed (4 seconds)
Details
buildkite/dendrite/sytest-go-1-dot-11-slash-postgres-9-dot-6 Soft failed (exit status 1)
Details
buildkite/dendrite/unit-tests-slash-go-1-dot-11 Passed (1 minute, 17 seconds)
Details
buildkite/dendrite/unit-tests-slash-go-1-dot-12 Passed (56 seconds)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.