fix vscodeuri generation, and add unit tests

coder · Emyrk · Jan 18, 2024 · Jan 17, 2024 · Jan 17, 2024 · Jan 17, 2024
commit ed4410c5649b1ffab20b8c02b2613eb1da05642e
diff --git a/coderd/agentapi/manifest.go b/coderd/agentapi/manifest.go
@@ -3,7 +3,6 @@ package agentapi
 import (
 	"context"
 	"database/sql"
-	"fmt"
 	"net/url"
 	"strings"
 	"sync/atomic"
@@ -108,19 +107,14 @@ func (a *ManifestAPI) GetManifest(ctx context.Context, _ *agentproto.GetManifest
 		return nil, xerrors.Errorf("fetching workspace agent data: %w", err)
 	}
 
-	appHost := appurl.ApplicationURL{
+	appSlug := appurl.ApplicationURL{
 		AppSlugOrPort: "{{port}}",
 		AgentName:     workspaceAgent.Name,
 		WorkspaceName: workspace.Name,
 		Username:      owner.Username,
 	}
-	vscodeProxyURI := a.AccessURL.Scheme + "://" + strings.ReplaceAll(a.AppHostname, "*", appHost.String())
-	if a.AppHostname == "" {
-		vscodeProxyURI += a.AccessURL.Hostname()
-	}
-	if a.AccessURL.Port() != "" {
-		vscodeProxyURI += fmt.Sprintf(":%s", a.AccessURL.Port())
-	}
+
+	vscodeProxyURI := vscodeProxyURI(appSlug, a.AccessURL, a.AppHostname)
 
 	var gitAuthConfigs uint32
 	for _, cfg := range a.ExternalAuthConfigs {
@@ -155,6 +149,17 @@ func (a *ManifestAPI) GetManifest(ctx context.Context, _ *agentproto.GetManifest
 	}, nil
 }
 
+func vscodeProxyURI(app appurl.ApplicationURL, accessURL *url.URL, appHost string) string {
+	// This will handle the ports from the accessURL or appHost.
+	appHost = appurl.SubdomainAppHost(appHost, accessURL)
+	// If there is no appHost, then we want to use the access url as the proxy uri.
+	if appHost == "" {
+		appHost = accessURL.Host
+	}
+	// Return the url with a scheme and any wildcards replaced with the app slug.
+	return accessURL.Scheme + "://" + strings.ReplaceAll(appHost, "*", app.String())
+}
+
 func dbAgentMetadataToProtoDescription(metadata []database.WorkspaceAgentMetadatum) []*agentproto.WorkspaceAgentMetadata_Description {
 	ret := make([]*agentproto.WorkspaceAgentMetadata_Description, len(metadata))
 	for i, metadatum := range metadata {

diff --git a/coderd/agentapi/manifest_internal_test.go b/coderd/agentapi/manifest_internal_test.go
@@ -0,0 +1,94 @@
+package agentapi
+
+import (
+	"fmt"
+	"net/url"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/coder/coder/v2/coderd/workspaceapps/appurl"
+)
+
+func Test_vscodeProxyURI(t *testing.T) {
+	t.Parallel()
+
+	coderAccessURL, err := url.Parse("https://coder.com")
+	require.NoError(t, err)
+
+	accessURLWithPort, err := url.Parse("https://coder.com:8080")
+	require.NoError(t, err)
+
+	basicApp := appurl.ApplicationURL{
+		Prefix:        "prefix",
+		AppSlugOrPort: "slug",
+		AgentName:     "agent",
+		WorkspaceName: "workspace",
+		Username:      "user",
+	}
+
+	cases := []struct {
+		Name        string
+		App         appurl.ApplicationURL
+		AccessURL   *url.URL
+		AppHostname string
+		Expected    string
+	}{
+		{
+			// No hostname proxies through the access url.
+			Name:        "NoHostname",
+			AccessURL:   coderAccessURL,
+			AppHostname: "",
+			App:         basicApp,
+			Expected:    coderAccessURL.String(),
+		},
+		{
+			Name:        "NoHostnameAccessURLPort",
+			AccessURL:   accessURLWithPort,
+			AppHostname: "",
+			App:         basicApp,
+			Expected:    accessURLWithPort.String(),
+		},
+		{
+			Name:        "Hostname",
+			AccessURL:   coderAccessURL,
+			AppHostname: "*.apps.coder.com",
+			App:         basicApp,
+			Expected:    fmt.Sprintf("https://%s.apps.coder.com", basicApp.String()),
+		},
+		{
+			Name:        "HostnameWithAccessURLPort",
+			AccessURL:   accessURLWithPort,
+			AppHostname: "*.apps.coder.com",
+			App:         basicApp,
+			Expected:    fmt.Sprintf("https://%s.apps.coder.com:%s", basicApp.String(), accessURLWithPort.Port()),
+		},
+		{
+			Name:        "HostnameWithPort",
+			AccessURL:   coderAccessURL,
+			AppHostname: "*.apps.coder.com:4444",
+			App:         basicApp,
+			Expected:    fmt.Sprintf("https://%s.apps.coder.com:%s", basicApp.String(), "4444"),
+		},
+		{
+			// Port from hostname takes precedence over access url port.
+			Name:        "HostnameWithPortAccessURLWithPort",
+			AccessURL:   accessURLWithPort,
+			AppHostname: "*.apps.coder.com:4444",
+			App:         basicApp,
+			Expected:    fmt.Sprintf("https://%s.apps.coder.com:%s", basicApp.String(), "4444"),
+		},
+	}
+
+	for _, c := range cases {
+		c := c
+		t.Run(c.Name, func(t *testing.T) {
+			t.Parallel()
+
+			require.NotNilf(t, c.AccessURL, "AccessURL is required")
+
+			output := vscodeProxyURI(c.App, c.AccessURL, c.AppHostname)
+			require.Equal(t, c.Expected, output)
+		})
+	}
+}
diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -93,7 +93,7 @@ type Options struct {
 	// AppHostname should be the wildcard hostname to use for workspace
 	// applications INCLUDING the asterisk, (optional) suffix and leading dot.
 	// It will use the same scheme and port number as the access URL.
-	// E.g. "*.apps.coder.com" or "*-apps.coder.com".
+	// E.g. "*.apps.coder.com" or "*-apps.coder.com" or "*.apps.coder.com:8080".
 	AppHostname string
 	// AppHostnameRegex contains the regex version of options.AppHostname as
 	// generated by appurl.CompileHostnamePattern(). It MUST be set if

diff --git a/coderd/workspaceapps.go b/coderd/workspaceapps.go
@@ -3,7 +3,6 @@ package coderd
 import (
 	"context"
 	"database/sql"
-	"fmt"
 	"net/http"
 	"net/url"
 	"strings"
@@ -32,13 +31,8 @@ import (
 // @Router /applications/host [get]
 // @Deprecated use api/v2/regions and see the primary proxy.
 func (api *API) appHost(rw http.ResponseWriter, r *http.Request) {
-	host := api.AppHostname
-	if host != "" && api.AccessURL.Port() != "" {
-		host += fmt.Sprintf(":%s", api.AccessURL.Port())
-	}
-
 	httpapi.Write(r.Context(), rw, http.StatusOK, codersdk.AppHostResponse{
-		Host: host,
+		Host: appurl.SubdomainAppHost(api.AppHostname, api.AccessURL),
 	})
 }
 

diff --git a/coderd/workspaceapps/apptest/apptest.go b/coderd/workspaceapps/apptest/apptest.go
@@ -962,6 +962,36 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
 			require.Equal(t, http.StatusOK, resp.StatusCode)
 		})
 
+		t.Run("WildcardPortOK", func(t *testing.T) {
+			t.Parallel()
+
+			// Manually specifying a port should override the access url port on
+			// the app host.
+			appDetails := setupProxyTest(t, &DeploymentOptions{
+				AppHost: "*.test.coder.com:4444",
+			})
+
+			ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
+			defer cancel()
+
+			u := appDetails.SubdomainAppURL(appDetails.Apps.Owner)
+			t.Logf("url: %s", u)
+			require.Equal(t, "4444", u.Port(), "port should be 4444")
+
+			// Assert the api response the UI uses has the port.
+			apphost, err := appDetails.SDKClient.AppHost(ctx)
+			require.NoError(t, err)
+			require.Equal(t, "*.test.coder.com:4444", apphost.Host, "apphost has port")
+
+			resp, err := requestWithRetries(ctx, t, appDetails.AppClient(t), http.MethodGet, u.String(), nil)
+			require.NoError(t, err)
+			defer resp.Body.Close()
+			body, err := io.ReadAll(resp.Body)
+			require.NoError(t, err)
+			require.Equal(t, proxyTestAppBody, string(body))
+			require.Equal(t, http.StatusOK, resp.StatusCode)
+		})
+
 		t.Run("SuffixWildcardNotMatch", func(t *testing.T) {
 			t.Parallel()
 

diff --git a/coderd/workspaceapps/apptest/setup.go b/coderd/workspaceapps/apptest/setup.go
@@ -407,7 +407,7 @@ func createWorkspaceWithApps(t *testing.T, client *codersdk.Client, orgID uuid.U
 			Username:      me.Username,
 		}
 		proxyURL := "http://" + appHost.String() + strings.ReplaceAll(primaryAppHost.Host, "*", "")
-		require.Equal(t, proxyURL, manifest.VSCodePortProxyURI)
+		require.Equal(t, manifest.VSCodePortProxyURI, proxyURL)
 	}
 	agentCloser := agent.New(agent.Options{
 		Client: agentClient,

diff --git a/coderd/workspaceapps/appurl/appurl.go b/coderd/workspaceapps/appurl/appurl.go
@@ -3,6 +3,7 @@ package appurl
 import (
 	"fmt"
 	"net"
+	"net/url"
 	"regexp"
 	"strings"
 
@@ -20,6 +21,36 @@ var (
 	validHostnameLabelRegex = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`)
 )
 
+// SubdomainAppHost returns the URL of the apphost for subdomain based apps.
+// It will omit the scheme.
+//
+// Arguments:
+// apphost: Expected to contain a wildcard, example: "*.coder.com"
+// accessURL: The access url for the deployment.
+//
+// Returns:
+// 'apphost:port'
+//
+// For backwards compatibility and for "accessurl=localhost:0" purposes, we need
+// to use the port from the accessurl if the apphost doesn't have a port.
+// If the user specifies a port in the apphost, we will use that port instead.
+func SubdomainAppHost(apphost string, accessURL *url.URL) string {
+	if apphost == "" {
+		return ""
+	}
+
+	if apphost != "" && accessURL.Port() != "" {
+		// This should always parse if we prepend a scheme. We should add
+		// the access url port if the apphost doesn't have a port specified.
+		appHostU, err := url.Parse(fmt.Sprintf("https://%s", apphost))
+		if err != nil || (err == nil && appHostU.Port() == "") {
+			apphost += fmt.Sprintf(":%s", accessURL.Port())
+		}
+	}
+
+	return apphost
+}
+
 // ApplicationURL is a parsed application URL hostname.
 type ApplicationURL struct {
 	Prefix        string