Skip to content

Commit 6bb1a34

Browse files
EmyrkEmyrk
authored
fix: allow ports in wildcard url configuration (#11657)
* fix: allow ports in wildcard url configuration This just forwards the port to the ui that generates urls. Our existing parsing + regex already supported ports for subdomain app requests.
1 parent 1f0e6ba commit 6bb1a34

File tree

12 files changed

+203
-31
lines changed

12 files changed

+203
-31
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

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

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

‎coderd/workspaceapps/apptest/setup.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ const (
4747
// DeploymentOptions are the options for creating a *Deployment with a
4848
// DeploymentFactory.
4949
type DeploymentOptions struct {
50+
PrimaryAppHost string
5051
AppHost string
5152
DisablePathApps bool
5253
DisableSubdomainApps bool
@@ -407,7 +408,7 @@ func createWorkspaceWithApps(t *testing.T, client *codersdk.Client, orgID uuid.U
407408
Username: me.Username,
408409
}
409410
proxyURL := "http://" + appHost.String() + strings.ReplaceAll(primaryAppHost.Host, "*", "")
410-
require.Equal(t, proxyURL, manifest.VSCodePortProxyURI)
411+
require.Equal(t, manifest.VSCodePortProxyURI, proxyURL)
411412
}
412413
agentCloser := agent.New(agent.Options{
413414
Client: agentClient,

‎coderd/workspaceapps/appurl/appurl.go

+42-3
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
@@ -140,9 +171,7 @@ func CompileHostnamePattern(pattern string) (*regexp.Regexp, error) {
140171
if strings.Contains(pattern, "http:") || strings.Contains(pattern, "https:") {
141172
return nil, xerrors.Errorf("hostname pattern must not contain a scheme: %q", pattern)
142173
}
143-
if strings.Contains(pattern, ":") {
144-
return nil, xerrors.Errorf("hostname pattern must not contain a port: %q", pattern)
145-
}
174+
146175
if strings.HasPrefix(pattern, ".") || strings.HasSuffix(pattern, ".") {
147176
return nil, xerrors.Errorf("hostname pattern must not start or end with a period: %q", pattern)
148177
}
@@ -155,6 +184,16 @@ func CompileHostnamePattern(pattern string) (*regexp.Regexp, error) {
155184
if !strings.HasPrefix(pattern, "*") {
156185
return nil, xerrors.Errorf("hostname pattern must only contain an asterisk at the beginning: %q", pattern)
157186
}
187+
188+
// If there is a hostname:port, we only care about the hostname. For hostname
189+
// pattern reasons, we do not actually care what port the client is requesting.
190+
// Any port provided here is used for generating urls for the ui, not for
191+
// validation.
192+
hostname, _, err := net.SplitHostPort(pattern)
193+
if err == nil {
194+
pattern = hostname
195+
}
196+
158197
for i, label := range strings.Split(pattern, ".") {
159198
if i == 0 {
160199
// We have to allow the asterisk to be a valid hostname label, so

‎coderd/workspaceapps/appurl/appurl_test.go

+7-5
Original file line numberDiff line numberDiff line change
@@ -193,11 +193,6 @@ func TestCompileHostnamePattern(t *testing.T) {
193193
pattern: "https://*.hi.com",
194194
errorContains: "must not contain a scheme",
195195
},
196-
{
197-
name: "Invalid_ContainsPort",
198-
pattern: "*.hi.com:8080",
199-
errorContains: "must not contain a port",
200-
},
201196
{
202197
name: "Invalid_StartPeriod",
203198
pattern: ".hi.com",
@@ -249,6 +244,13 @@ func TestCompileHostnamePattern(t *testing.T) {
249244
errorContains: "contains invalid label",
250245
},
251246

247+
{
248+
name: "Valid_ContainsPort",
249+
pattern: "*.hi.com:8080",
250+
// Although a port is provided, the regex already matches any port.
251+
// So it is ignored for validation purposes.
252+
expectedRegex: `([^.]+)\.hi\.com`,
253+
},
252254
{
253255
name: "Valid_Simple",
254256
pattern: "*.hi",

‎coderd/workspaceproxies.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"github.com/coder/coder/v2/coderd/database"
1212
"github.com/coder/coder/v2/coderd/database/dbauthz"
1313
"github.com/coder/coder/v2/coderd/httpapi"
14+
"github.com/coder/coder/v2/coderd/workspaceapps/appurl"
1415
"github.com/coder/coder/v2/codersdk"
1516
)
1617

@@ -43,7 +44,7 @@ func (api *API) PrimaryRegion(ctx context.Context) (codersdk.Region, error) {
4344
IconURL: proxy.IconUrl,
4445
Healthy: true,
4546
PathAppURL: api.AccessURL.String(),
46-
WildcardHostname: api.AppHostname,
47+
WildcardHostname: appurl.SubdomainAppHost(api.AppHostname, api.AccessURL),
4748
}, nil
4849
}
4950

‎coderd/workspaceproxies_test.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package coderd_test
22

33
import (
4+
"fmt"
45
"testing"
56

67
"github.com/google/uuid"
@@ -44,7 +45,7 @@ func TestRegions(t *testing.T) {
4445
require.NotEmpty(t, regions[0].IconURL)
4546
require.True(t, regions[0].Healthy)
4647
require.Equal(t, client.URL.String(), regions[0].PathAppURL)
47-
require.Equal(t, appHostname, regions[0].WildcardHostname)
48+
require.Equal(t, fmt.Sprintf("%s:%s", appHostname, client.URL.Port()), regions[0].WildcardHostname)
4849

4950
// Ensure the primary region ID is constant.
5051
regions2, err := client.Regions(ctx)

‎enterprise/coderd/workspaceproxy_test.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ func TestRegions(t *testing.T) {
6262
require.NotEmpty(t, regions[0].IconURL)
6363
require.True(t, regions[0].Healthy)
6464
require.Equal(t, client.URL.String(), regions[0].PathAppURL)
65-
require.Equal(t, appHostname, regions[0].WildcardHostname)
65+
require.Equal(t, fmt.Sprintf("%s:%s", appHostname, client.URL.Port()), regions[0].WildcardHostname)
6666

6767
// Ensure the primary region ID is constant.
6868
regions2, err := client.Regions(ctx)
@@ -149,7 +149,7 @@ func TestRegions(t *testing.T) {
149149
require.NotEmpty(t, regions[0].IconURL)
150150
require.True(t, regions[0].Healthy)
151151
require.Equal(t, client.URL.String(), regions[0].PathAppURL)
152-
require.Equal(t, appHostname, regions[0].WildcardHostname)
152+
require.Equal(t, fmt.Sprintf("%s:%s", appHostname, client.URL.Port()), regions[0].WildcardHostname)
153153

154154
// Ensure non-zero fields of the default proxy
155155
require.NotZero(t, primary.Name)

‎enterprise/wsproxy/wsproxy_test.go

+4-1
Original file line numberDiff line numberDiff line change
@@ -449,10 +449,13 @@ func TestWorkspaceProxyWorkspaceApps(t *testing.T) {
449449
<-proxyStatsCollectorFlushDone
450450
}
451451

452+
if opts.PrimaryAppHost == "" {
453+
opts.PrimaryAppHost = "*.primary.test.coder.com"
454+
}
452455
client, closer, api, user := coderdenttest.NewWithAPI(t, &coderdenttest.Options{
453456
Options: &coderdtest.Options{
454457
DeploymentValues: deploymentValues,
455-
AppHostname: "*.primary.test.coder.com",
458+
AppHostname: opts.PrimaryAppHost,
456459
IncludeProvisionerDaemon: true,
457460
RealIPConfig: &httpmw.RealIPConfig{
458461
TrustedOrigins: []*net.IPNet{{

0 commit comments

Comments
 (0)