Drop --no-tls and clean up Homeserver object API (#1014)

This was getting to be a bit of a mess, with unused/duplicate functions, and undocumented methods named the same as undocumented fields with different functions. Clean it all up.

Author: richvdh

lib/SyTest/Homeserver.pm

@@ -116,10 +116,10 @@ Homeserver interface.
 This method should return the server_name for the server (ie, the 'domain' part
 of any Matrix IDs it generates).
 
-=head2 http_api_host
+=head2 federation_host
 
 This method should return the hostname where the homeserver exposes the
-client-server and server-server APIs.
+server-server API.
 
 =head2 federation_port
 
@@ -129,19 +129,12 @@ This method should return the port number where the homeserver exposes a
 server-server API (over HTTPS). It may return undef if there is no known
 federation port.
 
-=head2 secure_port
+=head2 public_baseurl
 
-   $hs->secure_port
+   $hs->public_baseurl
 
-This method should return the port number where the homeserver exposes a
-client-server API over HTTPS.
-
-=head2 unsecure_port
-
-   $hs->unsecure_port
-
-This method should return the port number where the homeserver exposes a
-client-server API over HTTP.
+This method should returns the public base URL for the client-server API for
+this server.
 
 =cut
 
@@ -166,7 +159,7 @@ sub configure
    my %params = @_;
 
    exists $params{$_} and $self->{$_} = delete $params{$_} for qw(
-      public_baseurl recaptcha_config cas_config smtp_server_config
+      recaptcha_config cas_config smtp_server_config
       app_service_config_files
    );
 

lib/SyTest/Homeserver/Dendrite.pm

@@ -69,7 +69,7 @@ sub _check_db_config
    return $self->SUPER::_check_db_config( @_ );
 }
 
-sub http_api_host
+sub federation_host
 {
    my $self = shift;
    return $self->{bind_host};
@@ -332,13 +332,9 @@ sub unsecure_port
 
 sub public_baseurl
 {
-    my $self = shift;
-    # run-tests.pl defines whether TLS should be used or not.
-    my ( $want_tls ) = @_;
-    return $want_tls ?
-       "https://$self->{bind_host}:" . $self->secure_port() :
-       "http://$self->{bind_host}:" . $self->unsecure_port();
- }
+   my $self = shift;
+   return "https://$self->{bind_host}:" . $self->secure_port();
+}
 
 sub start
 {

lib/SyTest/Homeserver/Manual.pm

@@ -52,7 +52,7 @@ sub server_name
    return $self->{server_name};
 }
 
-sub http_api_host
+sub federation_host
 {
    my $self = shift;
    return $self->{host};
@@ -61,33 +61,18 @@ sub http_api_host
 sub federation_port
 {
    my $self = shift;
-   return $self->secure_port;
-}
-
-sub secure_port
-{
-   my $self = shift;
-
    return $self->{secure_port};
 }
 
-sub unsecure_port
+sub public_baseurl
 {
    my $self = shift;
 
-   return $self->{unsecure_port};
+   return defined $self->{secure_port} ?
+      "https://$self->{host}:" . $self->{secure_port} :
+      "http://$self->{host}:" . $self->{unsecure_port};
 }
 
-sub public_baseurl
-{
-    my $self = shift;
-    # run-tests.pl defines whether TLS should be used or not.
-    my ( $want_tls ) = @_;
-    return $want_tls ?
-       "https://$self->{bind_host}:" . $self->secure_port() :
-       "http://$self->{bind_host}:" . $self->unsecure_port();
- }
-
 sub start
 {
    my $self = shift;

lib/SyTest/Homeserver/Synapse.pm

@@ -171,7 +171,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 => $self->{public_baseurl},
+        public_baseurl => $self->public_baseurl,
 
         # We configure synapse to use a TLS cert which is signed by our dummy CA...
         tls_certificate_path => $self->{paths}{cert_file},
@@ -569,7 +569,7 @@ sub server_name
    return $self->{bind_host} . ":" . $self->secure_port;
 }
 
-sub http_api_host
+sub federation_host
 {
    my $self = shift;
    return $self->{bind_host};
@@ -587,20 +587,10 @@ sub secure_port
    return $self->{ports}{synapse};
 }
 
-sub unsecure_port
-{
-   my $self = shift;
-   return $self->{ports}{synapse_unsecure};
-}
-
 sub public_baseurl
 {
    my $self = shift;
-   # run-tests.pl defines whether TLS should be used or not.
-   my ( $want_tls ) = @_;
-   return $want_tls ?
-      "https://$self->{bind_host}:" . $self->secure_port() :
-      "http://$self->{bind_host}:" . $self->unsecure_port();
+   return "https://$self->{bind_host}:" . $self->secure_port;
 }
 
 package SyTest::Homeserver::Synapse::Direct;
@@ -1111,12 +1101,6 @@ sub secure_port
    return $self->{ports}{haproxy};
 }
 
-sub unsecure_port
-{
-   my $self = shift;
-   die "haproxy does not have an unsecure port mode\n";
-}
-
 sub public_baseurl
 {
    my $self = shift;

run-tests.pl

@@ -53,8 +53,6 @@
 
 binmode(STDOUT, ":utf8");
 
-our $WANT_TLS = 1;  # This is shared with the test scripts
-
 our $BIND_HOST = "localhost";
 
 # a unique ID for this test run. It is used in some tests to create user IDs
@@ -101,8 +99,6 @@
 
    'v|verbose+' => \(my $VERBOSE = 0),
 
-   'n|no-tls' => sub { $WANT_TLS = 0 },
-
    'work-directory=s' => \$WORK_DIR,
 
    'room-version=s' => \$TEST_ROOM_VERSION,

tests/05homeserver.pl

@@ -51,19 +51,11 @@
 
          $loop->add( $server );
 
-         my $api_host = $server->http_api_host;
-
-         my $location = $server->public_baseurl($WANT_TLS);
-
          $server->configure(
             smtp_server_config => $mail_server_info,
          );
 
          $server->configure(
-            # Annoyingly we ask the homeserver object for public_baseurl and then
-            # pass it back into the configuration since this is the only location
-            # we have $WANT_TLS.
-            public_baseurl => $location,
             # Config for testing recaptcha. 90jira/SYT-8.pl
             recaptcha_config => {
                siteverify_api   => $test_server_info->client_location .
@@ -75,8 +67,10 @@
             },
          );
 
-         my $info = ServerInfo( $server->server_name, $location,
-                                $api_host, $server->federation_port );
+         my $info = ServerInfo(
+            $server->server_name, $server->public_baseurl,
+            $server->federation_host, $server->federation_port,
+         );
 
          if( $idx == 0 ) {
             # Configure application services on first instance only
@@ -129,7 +123,7 @@
             return Future->done( $info );
          })->on_fail( sub {
             my ( $exn, @details ) = @_;
-            warn( "Error starting server-$idx (on port ${\$server->secure_port}): $exn" );
+            warn( "Error starting server-$idx: $exn" );
 
             # if we can't start the first homeserver, we really might as well go home.
             if( $idx == 0 ) {