Skip to content
This repository was archived by the owner on Nov 25, 2024. It is now read-only.

Update monolith -api behaviour#1484

Merged
neilalexander merged 8 commits intomasterfrom
neilalexander/httpapi
Oct 7, 2020
Merged

Update monolith -api behaviour#1484
neilalexander merged 8 commits intomasterfrom
neilalexander/httpapi

Conversation

@neilalexander
Copy link
Contributor

@neilalexander neilalexander commented Oct 6, 2020 •
edited
Loading

This makes some changes to the monolith -api behaviour:

  • The monolith internal listener is now on localhost:18008 by default when -api is enabled
  • Adds -api-bind-address for overriding the above
  • The setup package now prefers setting up the external listener over the internal one, which ultimately makes more sense because that's where the majority of useful HTTP routing goes

This probably requires a patch to Sytest to set -api-bind-address to something non-conflicting when we use -api.

@neilalexander neilalexander requested a review from kegsay October 6, 2020 16:33
neilalexander added a commit to matrix-org/sytest that referenced this pull request Oct 6, 2020
@neilalexander
Set -api-bind-address
d145f34 
ref. matrix-org/dendrite#1484
@neilalexander neilalexander mentioned this pull request Oct 6, 2020
Merged
@neilalexander neilalexander marked this pull request as draft October 6, 2020 16:38
@kegsay
Copy link
Member

kegsay commented Oct 6, 2020

Why do we do this though?

@neilalexander neilalexander marked this pull request as ready for review October 7, 2020 15:05
@neilalexander
Copy link
Contributor Author

neilalexander commented Oct 7, 2020

So there are "external" and "internal" listeners. The external listeners are where /_matrix APIs go. The internal ones are where we put /api, /metrics. etc. Depending on whether we run in monolith mode or polylith mode, the behaviour is a bit different.

In monolith mode, we were previously defaulting to both listeners being the same, so /api, /metrics and other sensitive endpoints could end up on the public listener.

In polylith mode, whether a component starts both external+internal listeners or just the internal listener depends on whether it has public-facing APIs (like the client API, sync API, federation API, media API) or not.

This PR changes things around so that the internal listener now listens on localhost by default, ensuring that the internal routing now stays on the internal listener, unless explicitly overridden.

kegsay
kegsay reviewed Oct 7, 2020
View reviewed changes
internal/setup/base.go Outdated
const HTTPClientTimeout = time.Second * 30

const NoInternalListener = ""
const NoExternalListener = ""
Copy link
Member

@kegsay kegsay Oct 7, 2020

Choose a reason for hiding this comment

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

What's going on here? You need some actual values here else NoInternalListener is the same as NoExternalListener - https://play.golang.org/p/bIAL8JumJtK

Copy link
Contributor Author

@neilalexander neilalexander Oct 7, 2020

Choose a reason for hiding this comment

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

They're just designed to be human-readable rather than leaving "" littered about the place, e.g.

dendrite/cmd/dendrite-monolith-server/main.go

Lines 164 to 168 in 759bab6

base.SetupAndServeHTTP(
setup.NoInternalListener, // internal API
httpsAddr, // external API
certFile, keyFile, // TLS settings
)

dendrite/cmd/dendrite-room-server/main.go

Lines 35 to 39 in 759bab6

base.SetupAndServeHTTP(
base.Cfg.RoomServer.InternalAPI.Listen,
setup.NoExternalListener,
nil, nil,
)

Copy link
Member

@kegsay kegsay Oct 7, 2020

Choose a reason for hiding this comment

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

Maybe setup.NoListener instead?

neilalexander added a commit to matrix-org/sytest that referenced this pull request Oct 7, 2020
@neilalexander
Set -api-bind-address (#967)
45e087f 
ref. matrix-org/dendrite#1484
@neilalexander neilalexander merged commit 8bca7a8 into master Oct 7, 2020
@neilalexander neilalexander deleted the neilalexander/httpapi branch October 7, 2020 15:59
@matrixbot matrixbot mentioned this pull request Oct 31, 2024
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@kegsay kegsay kegsay approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@neilalexander @kegsay