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

Use a consistent public baseurl. #1011

Merged
merged 5 commits into from
Feb 11, 2021
Merged

Use a consistent public baseurl. #1011

merged 5 commits into from
Feb 11, 2021

Conversation

clokep
Copy link
Member

@clokep clokep commented Feb 8, 2021

This sets public_baseurl in the Synapse config to match what is provided via ServerInfo.

Note that the cas_config.service_url was deprecated in matrix-org/synapse#9199 so we don't need to provide it anymore.

Hopefully will fix the build issues in matrix-org/synapse#9313.

@clokep
Copy link
Member Author

clokep commented Feb 8, 2021

The failing logs in this case seems to have an additional:

# localhost:8800 - connect: Cannot assign requested address failed [Cannot assign requested address] at /sytest/run-tests.pl line 775.

But not sure why that's happening from this change.

@clokep
Copy link
Member Author

clokep commented Feb 9, 2021

But not sure why that's happening from this change.

This was because $self->{ports}{synapse} is only the public port when not using haproxy. When using haproxy this should be $self->{ports}{haproxy}, that logic is already encapsulated in the secure_port helper.

@clokep clokep requested a review from a team February 9, 2021 14:24
@@ -172,7 +172,7 @@ sub start
my $config_path = $self->{paths}{config} = $self->write_yaml_file( "config.yaml" => {
server_name => $self->server_name,
log_config => $log_config_file,
public_baseurl => "http://${bind_host}:$unsecure_port",
public_baseurl => "https://${bind_host}:$secure_port",
Copy link
Member Author

Choose a reason for hiding this comment

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

I do wonder if this has to take into account the $WANT_TLS option somewhere? 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Does it work with --no-tls still with this? Can we use the unsecure port if its enabled or does that break stuff?

Copy link
Member Author

Choose a reason for hiding this comment

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

TIL that was a flag. 😄 It did break with that, I pushed a commit fixing it.

Copy link
Member

Choose a reason for hiding this comment

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

Me too! I was like "WTF is WANT_TLS??"

@clokep clokep merged commit 9c8791d into develop Feb 11, 2021
@clokep clokep deleted the clokep/base-url branch February 11, 2021 15:45
@neilalexander
Copy link
Contributor

Looks like this PR broke Sytest for Dendrite:

fixture failed - Can't locate object method "public_baseurl" via package "SyTest::Homeserver::Dendrite::Monolith" at tests/05homeserver.pl line 56.

Apparently it wasn’t highlighted in the checks here because the Dendrite pipelines are soft-failed — we may want to do something about that too.

@clokep
Copy link
Member Author

clokep commented Feb 12, 2021

Oh, sorry about that! I saw all green ✅ and assumed it was OK. 😢 I agree that we should update the pipelines to make Dendrite pass.

valkum added a commit to valkum/sytest_conduit that referenced this pull request Feb 18, 2021
@richvdh
Copy link
Member

richvdh commented Feb 23, 2021

I'm really not thrilled about this change: it's very confusing, and lacks documentation.

Homeserver now has both a $hs->public_baseurl method, and a $hs->{public_baseurl} member, which are subtly different, and neither are documented.

Adding required methods to a base class without checking the implementations of that class is pretty poor form too.

Finally, @neilalexander: the commit comment on eed37c2 is confusing: it looks like it was part of this PR, whereas it wasn't. Please don't commit anything direct to develop that isn't completely trivial, and please don't add PR numbers to the subject unless it's actually part of the PR in question.

I think I'm going to have a go at cleaning up public_baseurl.

@richvdh
Copy link
Member

richvdh commented Feb 23, 2021

#1014 sets out to improve this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants