Skip to content

Commit a673da8

Browse files
EmyrkEmyrk
committed
fix vscodeuri generation, and add unit tests
1 parent 2b4ea6d commit a673da8

File tree

7 files changed

+172
-18
lines changed

7 files changed

+172
-18
lines changed

‎coderd/agentapi/manifest.go

+14-9
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package agentapi
33
import (
44
"context"
55
"database/sql"
6-
"fmt"
76
"net/url"
87
"strings"
98
"sync/atomic"
@@ -108,19 +107,14 @@ func (a *ManifestAPI) GetManifest(ctx context.Context, _ *agentproto.GetManifest
108107
return nil, xerrors.Errorf("fetching workspace agent data: %w", err)
109108
}
110109

111-
appHost := appurl.ApplicationURL{
110+
appSlug := appurl.ApplicationURL{
112111
AppSlugOrPort: "{{port}}",
113112
AgentName: workspaceAgent.Name,
114113
WorkspaceName: workspace.Name,
115114
Username: owner.Username,
116115
}
117-
vscodeProxyURI := a.AccessURL.Scheme + "://" + strings.ReplaceAll(a.AppHostname, "*", appHost.String())
118-
if a.AppHostname == "" {
119-
vscodeProxyURI += a.AccessURL.Hostname()
120-
}
121-
if a.AccessURL.Port() != "" {
122-
vscodeProxyURI += fmt.Sprintf(":%s", a.AccessURL.Port())
123-
}
116+
117+
vscodeProxyURI := vscodeProxyURI(appSlug, a.AccessURL, a.AppHostname)
124118

125119
var gitAuthConfigs uint32
126120
for _, cfg := range a.ExternalAuthConfigs {
@@ -155,6 +149,17 @@ func (a *ManifestAPI) GetManifest(ctx context.Context, _ *agentproto.GetManifest
155149
}, nil
156150
}
157151

152+
func vscodeProxyURI(app appurl.ApplicationURL, accessURL *url.URL, appHost string) string {
153+
// This will handle the ports from the accessURL or appHost.
154+
appHost = appurl.SubdomainAppHost(appHost, accessURL)
155+
// If there is no appHost, then we want to use the access url as the proxy uri.
156+
if appHost == "" {
157+
appHost = accessURL.Host
158+
}
159+
// Return the url with a scheme and any wildcards replaced with the app slug.
160+
return accessURL.Scheme + "://" + strings.ReplaceAll(appHost, "*", app.String())
161+
}
162+
158163
func dbAgentMetadataToProtoDescription(metadata []database.WorkspaceAgentMetadatum) []*agentproto.WorkspaceAgentMetadata_Description {
159164
ret := make([]*agentproto.WorkspaceAgentMetadata_Description, len(metadata))
160165
for i, metadatum := range metadata {

‎coderd/agentapi/manifest_internal_test.go

+94
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
package agentapi
2+
3+
import (
4+
"fmt"
5+
"net/url"
6+
"testing"
7+
8+
"github.com/stretchr/testify/require"
9+
10+
"github.com/coder/coder/v2/coderd/workspaceapps/appurl"
11+
)
12+
13+
func Test_vscodeProxyURI(t *testing.T) {
14+
t.Parallel()
15+
16+
coderAccessURL, err := url.Parse("https://coder.com")
17+
require.NoError(t, err)
18+
19+
accessURLWithPort, err := url.Parse("https://coder.com:8080")
20+
require.NoError(t, err)
21+
22+
basicApp := appurl.ApplicationURL{
23+
Prefix: "prefix",
24+
AppSlugOrPort: "slug",
25+
AgentName: "agent",
26+
WorkspaceName: "workspace",
27+
Username: "user",
28+
}
29+
30+
cases := []struct {
31+
Name string
32+
App appurl.ApplicationURL
33+
AccessURL *url.URL
34+
AppHostname string
35+
Expected string
36+
}{
37+
{
38+
// No hostname proxies through the access url.
39+
Name: "NoHostname",
40+
AccessURL: coderAccessURL,
41+
AppHostname: "",
42+
App: basicApp,
43+
Expected: coderAccessURL.String(),
44+
},
45+
{
46+
Name: "NoHostnameAccessURLPort",
47+
AccessURL: accessURLWithPort,
48+
AppHostname: "",
49+
App: basicApp,
50+
Expected: accessURLWithPort.String(),
51+
},
52+
{
53+
Name: "Hostname",
54+
AccessURL: coderAccessURL,
55+
AppHostname: "*.apps.coder.com",
56+
App: basicApp,
57+
Expected: fmt.Sprintf("https://%s.apps.coder.com", basicApp.String()),
58+
},
59+
{
60+
Name: "HostnameWithAccessURLPort",
61+
AccessURL: accessURLWithPort,
62+
AppHostname: "*.apps.coder.com",
63+
App: basicApp,
64+
Expected: fmt.Sprintf("https://%s.apps.coder.com:%s", basicApp.String(), accessURLWithPort.Port()),
65+
},
66+
{
67+
Name: "HostnameWithPort",
68+
AccessURL: coderAccessURL,
69+
AppHostname: "*.apps.coder.com:4444",
70+
App: basicApp,
71+
Expected: fmt.Sprintf("https://%s.apps.coder.com:%s", basicApp.String(), "4444"),
72+
},
73+
{
74+
// Port from hostname takes precedence over access url port.
75+
Name: "HostnameWithPortAccessURLWithPort",
76+
AccessURL: accessURLWithPort,
77+
AppHostname: "*.apps.coder.com:4444",
78+
App: basicApp,
79+
Expected: fmt.Sprintf("https://%s.apps.coder.com:%s", basicApp.String(), "4444"),
80+
},
81+
}
82+
83+
for _, c := range cases {
84+
c := c
85+
t.Run(c.Name, func(t *testing.T) {
86+
t.Parallel()
87+
88+
require.NotNilf(t, c.AccessURL, "AccessURL is required")
89+
90+
output := vscodeProxyURI(c.App, c.AccessURL, c.AppHostname)
91+
require.Equal(t, c.Expected, output)
92+
})
93+
}
94+
}

‎coderd/coderd.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ type Options struct {
9393
// AppHostname should be the wildcard hostname to use for workspace
9494
// applications INCLUDING the asterisk, (optional) suffix and leading dot.
9595
// It will use the same scheme and port number as the access URL.
96-
// E.g. "*.apps.coder.com" or "*-apps.coder.com".
96+
// E.g. "*.apps.coder.com" or "*-apps.coder.com" or "*.apps.coder.com:8080".
9797
AppHostname string
9898
// AppHostnameRegex contains the regex version of options.AppHostname as
9999
// generated by appurl.CompileHostnamePattern(). It MUST be set if

‎coderd/workspaceapps.go

+1-7
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package coderd
33
import (
44
"context"
55
"database/sql"
6-
"fmt"
76
"net/http"
87
"net/url"
98
"strings"
@@ -32,13 +31,8 @@ import (
3231
// @Router /applications/host [get]
3332
// @Deprecated use api/v2/regions and see the primary proxy.
3433
func (api *API) appHost(rw http.ResponseWriter, r *http.Request) {
35-
host := api.AppHostname
36-
if host != "" && api.AccessURL.Port() != "" {
37-
host += fmt.Sprintf(":%s", api.AccessURL.Port())
38-
}
39-
4034
httpapi.Write(r.Context(), rw, http.StatusOK, codersdk.AppHostResponse{
41-
Host: host,
35+
Host: appurl.SubdomainAppHost(api.AppHostname, api.AccessURL),
4236
})
4337
}
4438

‎coderd/workspaceapps/apptest/apptest.go

+30
Original file line numberDiff line numberDiff line change
@@ -962,6 +962,36 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
962962
require.Equal(t, http.StatusOK, resp.StatusCode)
963963
})
964964

965+
t.Run("WildcardPortOK", func(t *testing.T) {
966+
t.Parallel()
967+
968+
// Manually specifying a port should override the access url port on
969+
// the app host.
970+
appDetails := setupProxyTest(t, &DeploymentOptions{
971+
AppHost: "*.test.coder.com:4444",
972+
})
973+
974+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
975+
defer cancel()
976+
977+
u := appDetails.SubdomainAppURL(appDetails.Apps.Owner)
978+
t.Logf("url: %s", u)
979+
require.Equal(t, "4444", u.Port(), "port should be 4444")
980+
981+
// Assert the api response the UI uses has the port.
982+
apphost, err := appDetails.SDKClient.AppHost(ctx)
983+
require.NoError(t, err)
984+
require.Equal(t, "*.test.coder.com:4444", apphost.Host, "apphost has port")
985+
986+
resp, err := requestWithRetries(ctx, t, appDetails.AppClient(t), http.MethodGet, u.String(), nil)
987+
require.NoError(t, err)
988+
defer resp.Body.Close()
989+
body, err := io.ReadAll(resp.Body)
990+
require.NoError(t, err)
991+
require.Equal(t, proxyTestAppBody, string(body))
992+
require.Equal(t, http.StatusOK, resp.StatusCode)
993+
})
994+
965995
t.Run("SuffixWildcardNotMatch", func(t *testing.T) {
966996
t.Parallel()
967997

‎coderd/workspaceapps/apptest/setup.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -407,7 +407,7 @@ func createWorkspaceWithApps(t *testing.T, client *codersdk.Client, orgID uuid.U
407407
Username: me.Username,
408408
}
409409
proxyURL := "http://" + appHost.String() + strings.ReplaceAll(primaryAppHost.Host, "*", "")
410-
require.Equal(t, proxyURL, manifest.VSCodePortProxyURI)
410+
require.Equal(t, manifest.VSCodePortProxyURI, proxyURL)
411411
}
412412
agentCloser := agent.New(agent.Options{
413413
Client: agentClient,

‎coderd/workspaceapps/appurl/appurl.go

+31
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package appurl
33
import (
44
"fmt"
55
"net"
6+
"net/url"
67
"regexp"
78
"strings"
89

@@ -20,6 +21,36 @@ var (
2021
validHostnameLabelRegex = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`)
2122
)
2223

24+
// SubdomainAppHost returns the URL of the apphost for subdomain based apps.
25+
// It will omit the scheme.
26+
//
27+
// Arguments:
28+
// apphost: Expected to contain a wildcard, example: "*.coder.com"
29+
// accessURL: The access url for the deployment.
30+
//
31+
// Returns:
32+
// 'apphost:port'
33+
//
34+
// For backwards compatibility and for "accessurl=localhost:0" purposes, we need
35+
// to use the port from the accessurl if the apphost doesn't have a port.
36+
// If the user specifies a port in the apphost, we will use that port instead.
37+
func SubdomainAppHost(apphost string, accessURL *url.URL) string {
38+
if apphost == "" {
39+
return ""
40+
}
41+
42+
if apphost != "" && accessURL.Port() != "" {
43+
// This should always parse if we prepend a scheme. We should add
44+
// the access url port if the apphost doesn't have a port specified.
45+
appHostU, err := url.Parse(fmt.Sprintf("https://%s", apphost))
46+
if err != nil || (err == nil && appHostU.Port() == "") {
47+
apphost += fmt.Sprintf(":%s", accessURL.Port())
48+
}
49+
}
50+
51+
return apphost
52+
}
53+
2354
// ApplicationURL is a parsed application URL hostname.
2455
type ApplicationURL struct {
2556
Prefix string

0 commit comments

Comments
 (0)