Bug fixes for endpoints (#155)

Author: djthorpe

pkg/cmd/global.go

@@ -14,6 +14,7 @@ import (
 	kong "github.com/alecthomas/kong"
 	client "github.com/mutablelogic/go-client"
 	server "github.com/mutablelogic/go-server"
+	types "github.com/mutablelogic/go-server/pkg/types"
 	metric "go.opentelemetry.io/otel/metric"
 	trace "go.opentelemetry.io/otel/trace"
 )
@@ -95,7 +96,7 @@ func (g *global) Meter() metric.Meter {
 // ClientEndpoint returns the HTTP endpoint URL and client options derived
 // from the global HTTP flags.
 func (g *global) ClientEndpoint() (string, []client.ClientOpt, error) {
-	scheme := "http"
+	scheme := types.SchemeInsecure
 	host, port, err := net.SplitHostPort(g.HTTP.Addr)
 	if err != nil {
 		return "", nil, err
@@ -107,8 +108,14 @@ func (g *global) ClientEndpoint() (string, []client.ClientOpt, error) {
 	if err != nil {
 		return "", nil, err
 	}
-	if portn == 443 {
-		scheme = "https"
+	hostaddr := net.JoinHostPort(host, strconv.FormatUint(portn, 10))
+	switch portn {
+	case 80:
+		scheme = types.SchemeInsecure
+		hostaddr = host
+	case 443:
+		scheme = types.SchemeSecure
+		hostaddr = host
 	}
 	opts := []client.ClientOpt{}
 	if g.Debug || g.Verbose {
@@ -120,7 +127,7 @@ func (g *global) ClientEndpoint() (string, []client.ClientOpt, error) {
 	if g.HTTP.Timeout > 0 {
 		opts = append(opts, client.OptTimeout(g.HTTP.Timeout))
 	}
-	return fmt.Sprintf("%s://%s%s", scheme, net.JoinHostPort(host, strconv.FormatUint(portn, 10)), g.HTTP.Prefix), opts, nil
+	return fmt.Sprintf("%s://%s%s", scheme, hostaddr, g.HTTP.Prefix), opts, nil
 }
 
 func (g *global) URL() *url.URL {

pkg/cmd/server.go

@@ -16,8 +16,8 @@ import (
 	errgroup "golang.org/x/sync/errgroup"
 )
 
-// RegisterFunc is called after the router is created but before the server
-// starts listening. Use it to add routes and wire up handlers.
+// RegisterFunc is called after the server URL has been resolved but before the
+// server starts serving requests. Use it to add routes and wire up handlers.
 type RegisterFunc func(*httprouter.Router) error
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -87,11 +87,19 @@ func (s *RunServer) Run(ctx server.Cmd) error {
 		tlsCfg = &tls.Config{ServerName: s.TLS.ServerName}
 	}
 
-	// Create a new server and start listening. The server will run until the context is cancelled.
+	// Create a new server. The listener is bound before registration so callbacks
+	// can rely on the final advertised URL, including :0 port allocation.
 	srv, err := httpserver.New(ctx.HTTPAddr(), tlsCfg, serverOpts...)
 	if err != nil {
 		return fmt.Errorf("httpserver: %w", err)
-	} else if url := srv.URL(); url != nil {
+	}
+
+	// Bind to the server's address to ensure the advertised URL is final before
+	// route registration runs.
+	if err := srv.Listen(); err != nil {
+		return err
+	}
+	if url := srv.URL(); url != nil {
 		url.Path = types.NormalisePath(ctx.HTTPPrefix())
 		ctx.(*global).url = url
 	}
@@ -126,11 +134,6 @@ func (s *RunServer) Run(ctx server.Cmd) error {
 		return fmt.Errorf("catchall: %w", err)
 	}
 
-	// Bind to the server's address to ensure it's available
-	if err := srv.Listen(); err != nil {
-		return err
-	}
-
 	// Set the server URL in the OpenAPI spec now that the bound address is known.
 	// When a TLS server name is configured (e.g. behind a reverse proxy) it is
 	// used as the public hostname; otherwise the bound listen address is used.

pkg/cmd/server_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"log/slog"
 	"os"
+	"strings"
 	"testing"
 	"time"
 
@@ -128,3 +129,94 @@ func Test_ClientEndpoint_Verbose(t *testing.T) {
 		t.Error("expected opts when Verbose=true, got none")
 	}
 }
+
+func Test_RunServer_AdvertisedURL(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	g := &global{
+		ctx:    ctx,
+		logger: slog.New(slog.NewTextHandler(os.Stderr, nil)),
+	}
+	g.HTTP.Addr = ":0"
+	g.HTTP.Prefix = "/api"
+
+	s := &RunServer{}
+	var registerURL string
+	s.Register(func(_ *httprouter.Router) error {
+		if url := g.URL(); url != nil {
+			registerURL = url.String()
+		}
+		return nil
+	})
+
+	done := make(chan error, 1)
+	go func() {
+		done <- s.Run(g)
+	}()
+
+	time.Sleep(100 * time.Millisecond)
+	if registerURL == "" {
+		t.Fatal("expected URL to be available during route registration")
+	}
+	if !strings.HasPrefix(registerURL, "http://localhost:") {
+		t.Fatalf("register URL = %q, want localhost prefix", registerURL)
+	}
+	if strings.Contains(registerURL, ":0/") {
+		t.Fatalf("register URL = %q, want bound port instead of :0", registerURL)
+	}
+
+	if url := g.URL(); url == nil {
+		t.Fatal("expected URL after listen")
+	} else if strings.Contains(url.String(), ":0/") {
+		t.Fatalf("final URL = %q, want bound port", url.String())
+	}
+
+	cancel()
+	if err := <-done; err != nil {
+		t.Fatal(err)
+	}
+}
+
+func Test_RunServer_AdvertisedURL_TLSName(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	g := &global{
+		ctx:    ctx,
+		logger: slog.New(slog.NewTextHandler(os.Stderr, nil)),
+	}
+	g.HTTP.Addr = ":0"
+	g.HTTP.Prefix = "/api"
+
+	s := &RunServer{}
+	s.TLS.ServerName = "auth.example.com"
+	var registerURL string
+	s.Register(func(_ *httprouter.Router) error {
+		if url := g.URL(); url != nil {
+			registerURL = url.String()
+		}
+		return nil
+	})
+
+	done := make(chan error, 1)
+	go func() {
+		done <- s.Run(g)
+	}()
+
+	time.Sleep(100 * time.Millisecond)
+	if registerURL != "https://auth.example.com/api" {
+		t.Fatalf("register URL = %q, want %q", registerURL, "https://auth.example.com/api")
+	}
+
+	if url := g.URL(); url == nil {
+		t.Fatal("expected URL after listen")
+	} else if got := url.String(); got != "https://auth.example.com/api" {
+		t.Fatalf("final URL = %q, want %q", got, "https://auth.example.com/api")
+	}
+
+	cancel()
+	if err := <-done; err != nil {
+		t.Fatal(err)
+	}
+}

pkg/httpserver/httpserver.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"net"
 	"net/http"
+	"net/netip"
 	"net/url"
 	"strings"
 	"sync"
@@ -129,7 +130,7 @@ func (server *server) URL() *url.URL {
 	var url url.URL
 	url.Scheme = defaultListenPortHttp
 	url.Path = "/"
-	url.Host = server.Addr()
+	url.Host = normalizeURLHost(server.Addr())
 	if server.serverName != "" {
 		url.Scheme = defaultListenPortHttps
 		url.Host = server.serverName
@@ -203,3 +204,23 @@ func (server *server) shutdown() error {
 		return err
 	}
 }
+
+func normalizeURLHost(addr string) string {
+	host, port, err := net.SplitHostPort(addr)
+	if err != nil {
+		return addr
+	}
+	if host == "" || isUnspecifiedHost(host) {
+		host = defaultListenHost
+	}
+	return net.JoinHostPort(host, port)
+}
+
+func isUnspecifiedHost(host string) bool {
+	host = strings.TrimPrefix(strings.TrimSuffix(strings.TrimSpace(host), "]"), "[")
+	if host == "" {
+		return true
+	}
+	addr, err := netip.ParseAddr(host)
+	return err == nil && addr.IsUnspecified()
+}

pkg/httpserver/httpserver_test.go

@@ -361,6 +361,29 @@ func Test_Listen_002(t *testing.T) {
 	assert.Equal("localhost:9999", s.Addr())
 }
 
+func Test_URL_001(t *testing.T) {
+	assert := assert.New(t)
+
+	// Empty listen host should advertise localhost rather than a bare :port URL.
+	s, err := httpserver.New(":8084", nil)
+	assert.NoError(err)
+	if assert.NotNil(s.URL()) {
+		assert.Equal("http://localhost:8084/", s.URL().String())
+	}
+}
+
+func Test_URL_002(t *testing.T) {
+	assert := assert.New(t)
+
+	// Wildcard listener addresses should advertise localhost for local development.
+	s, err := httpserver.New(":0", nil)
+	assert.NoError(err)
+	assert.NoError(s.Listen())
+	if assert.NotNil(s.URL()) {
+		assert.Contains(s.URL().String(), "http://localhost:")
+	}
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // TESTS - RUN