Skip to content

Commit fead57f

Browse files
deansheatherdeansheather
authored
fix: allow access to unhealthy/initializing apps (coder#12086)
1 parent ec25fb8 commit fead57f

File tree

4 files changed

+84
-29
lines changed

4 files changed

+84
-29
lines changed

‎coderd/workspaceapps/db.go

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -198,11 +198,9 @@ func (p *DBTokenProvider) Issue(ctx context.Context, rw http.ResponseWriter, r *
198198
return nil, "", false
199199
}
200200

201-
// Check that the app is healthy.
202-
if dbReq.AppHealth != "" && dbReq.AppHealth != database.WorkspaceAppHealthDisabled && dbReq.AppHealth != database.WorkspaceAppHealthHealthy {
203-
WriteWorkspaceAppOffline(p.Logger, p.DashboardURL, rw, r, &appReq, fmt.Sprintf("App health is %q, not %q", dbReq.AppHealth, database.WorkspaceAppHealthHealthy))
204-
return nil, "", false
205-
}
201+
// This is where we used to check app health, but we don't do that anymore
202+
// in case there are bugs with the healthcheck code that lock users out of
203+
// their apps completely.
206204

207205
// As a sanity check, ensure the token we just made is valid for this
208206
// request.

‎coderd/workspaceapps/db_test.go

Lines changed: 76 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,12 @@ func Test_ResolveRequest(t *testing.T) {
3737
appNameAuthed = "app-authed"
3838
appNamePublic = "app-public"
3939
appNameInvalidURL = "app-invalid-url"
40-
appNameUnhealthy = "app-unhealthy"
40+
// Users can access unhealthy and initializing apps (as of 2024-02).
41+
appNameUnhealthy = "app-unhealthy"
42+
appNameInitializing = "app-initializing"
4143

4244
// This agent will never connect, so it will never become "connected".
45+
// Users cannot access unhealthy agents.
4346
agentNameUnhealthy = "agent-unhealthy"
4447
appNameAgentUnhealthy = "app-agent-unhealthy"
4548

@@ -55,6 +58,15 @@ func Test_ResolveRequest(t *testing.T) {
5558
w.WriteHeader(http.StatusInternalServerError)
5659
_, _ = w.Write([]byte("unhealthy"))
5760
}))
61+
t.Cleanup(unhealthySrv.Close)
62+
63+
// Start a listener for a server that never responds.
64+
initializingServer, err := net.Listen("tcp", "localhost:0")
65+
require.NoError(t, err)
66+
t.Cleanup(func() {
67+
_ = initializingServer.Close()
68+
})
69+
initializingURL := fmt.Sprintf("http://%s", initializingServer.Addr().String())
5870

5971
deploymentValues := coderdtest.DeploymentValues(t)
6072
deploymentValues.DisablePathApps = false
@@ -82,7 +94,7 @@ func Test_ResolveRequest(t *testing.T) {
8294
})
8395

8496
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitMedium)
85-
defer cancel()
97+
t.Cleanup(cancel)
8698

8799
firstUser := coderdtest.CreateFirstUser(t, client)
88100
me, err := client.User(ctx, codersdk.Me)
@@ -143,6 +155,17 @@ func Test_ResolveRequest(t *testing.T) {
143155
Threshold: 1,
144156
},
145157
},
158+
{
159+
Slug: appNameInitializing,
160+
DisplayName: appNameInitializing,
161+
SharingLevel: proto.AppSharingLevel_PUBLIC,
162+
Url: appURL,
163+
Healthcheck: &proto.Healthcheck{
164+
Url: initializingURL,
165+
Interval: 30,
166+
Threshold: 1000,
167+
},
168+
},
146169
},
147170
},
148171
{
@@ -805,7 +828,55 @@ func Test_ResolveRequest(t *testing.T) {
805828
require.Contains(t, bodyStr, `Agent state is "`)
806829
})
807830

808-
t.Run("UnhealthyApp", func(t *testing.T) {
831+
// Initializing apps are now permitted to connect anyways. This wasn't
832+
// always the case, but we're testing the behavior to ensure it doesn't
833+
// change back accidentally.
834+
t.Run("InitializingAppPermitted", func(t *testing.T) {
835+
t.Parallel()
836+
837+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
838+
defer cancel()
839+
840+
agent, err := client.WorkspaceAgent(ctx, agentID)
841+
require.NoError(t, err)
842+
843+
for _, app := range agent.Apps {
844+
if app.Slug == appNameInitializing {
845+
t.Log("app is", app.Health)
846+
require.Equal(t, codersdk.WorkspaceAppHealthInitializing, app.Health)
847+
break
848+
}
849+
}
850+
851+
req := (workspaceapps.Request{
852+
AccessMethod: workspaceapps.AccessMethodPath,
853+
BasePath: "/app",
854+
UsernameOrID: me.Username,
855+
WorkspaceNameOrID: workspace.Name,
856+
AgentNameOrID: agentName,
857+
AppSlugOrPort: appNameInitializing,
858+
}).Normalize()
859+
860+
rw := httptest.NewRecorder()
861+
r := httptest.NewRequest("GET", "/app", nil)
862+
r.Header.Set(codersdk.SessionTokenHeader, client.SessionToken())
863+
864+
token, ok := workspaceapps.ResolveRequest(rw, r, workspaceapps.ResolveRequestOptions{
865+
Logger: api.Logger,
866+
SignedTokenProvider: api.WorkspaceAppsProvider,
867+
DashboardURL: api.AccessURL,
868+
PathAppBaseURL: api.AccessURL,
869+
AppHostname: api.AppHostname,
870+
AppRequest: req,
871+
})
872+
require.True(t, ok, "ResolveRequest failed, should pass even though app is initializing")
873+
require.NotNil(t, token)
874+
})
875+
876+
// Unhealthy apps are now permitted to connect anyways. This wasn't always
877+
// the case, but we're testing the behavior to ensure it doesn't change back
878+
// accidentally.
879+
t.Run("UnhealthyAppPermitted", func(t *testing.T) {
809880
t.Parallel()
810881

811882
require.Eventually(t, func() bool {
@@ -850,17 +921,7 @@ func Test_ResolveRequest(t *testing.T) {
850921
AppHostname: api.AppHostname,
851922
AppRequest: req,
852923
})
853-
require.False(t, ok, "request succeeded even though app is unhealthy")
854-
require.Nil(t, token)
855-
856-
w := rw.Result()
857-
defer w.Body.Close()
858-
require.Equal(t, http.StatusBadGateway, w.StatusCode)
859-
860-
body, err := io.ReadAll(w.Body)
861-
require.NoError(t, err)
862-
bodyStr := string(body)
863-
bodyStr = strings.ReplaceAll(bodyStr, """, `"`)
864-
require.Contains(t, bodyStr, `App health is "unhealthy"`)
924+
require.True(t, ok, "ResolveRequest failed, should pass even though app is unhealthy")
925+
require.NotNil(t, token)
865926
})
866927
}

‎coderd/workspaceapps/request.go

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -198,9 +198,6 @@ type databaseRequest struct {
198198
// AppURL is the resolved URL to the workspace app. This is only set for non
199199
// terminal requests.
200200
AppURL *url.URL
201-
// AppHealth is the health of the app. For terminal requests, this is always
202-
// database.WorkspaceAppHealthHealthy.
203-
AppHealth database.WorkspaceAppHealth
204201
// AppSharingLevel is the sharing level of the app. This is forced to be set
205202
// to AppSharingLevelOwner if the access method is terminal.
206203
AppSharingLevel database.AppSharingLevel
@@ -292,7 +289,6 @@ func (r Request) getDatabase(ctx context.Context, db database.Store) (*databaseR
292289
agentNameOrID = r.AgentNameOrID
293290
appURL string
294291
appSharingLevel database.AppSharingLevel
295-
appHealth = database.WorkspaceAppHealthDisabled
296292
portUint, portUintErr = strconv.ParseUint(r.AppSlugOrPort, 10, 16)
297293
)
298294
if portUintErr == nil {
@@ -331,7 +327,6 @@ func (r Request) getDatabase(ctx context.Context, db database.Store) (*databaseR
331327
appSharingLevel = database.AppSharingLevelOwner
332328
}
333329
appURL = app.Url.String
334-
appHealth = app.Health
335330
break
336331
}
337332
}
@@ -377,7 +372,6 @@ func (r Request) getDatabase(ctx context.Context, db database.Store) (*databaseR
377372
Workspace: workspace,
378373
Agent: agent,
379374
AppURL: appURLParsed,
380-
AppHealth: appHealth,
381375
AppSharingLevel: appSharingLevel,
382376
}, nil
383377
}
@@ -430,7 +424,6 @@ func (r Request) getDatabaseTerminal(ctx context.Context, db database.Store) (*d
430424
Workspace: workspace,
431425
Agent: agent,
432426
AppURL: nil,
433-
AppHealth: database.WorkspaceAppHealthHealthy,
434427
AppSharingLevel: database.AppSharingLevelOwner,
435428
}, nil
436429
}

‎site/src/modules/resources/AppLink/AppLink.tsx

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,12 +61,16 @@ export const AppLink: FC<AppLinkProps> = ({ app, workspace, agent }) => {
6161
app,
6262
);
6363

64+
// canClick is ONLY false when it's a subdomain app and the admin hasn't
65+
// enabled wildcard access URL or the session token is being fetched.
66+
//
67+
// To avoid bugs in the healthcheck code locking users out of apps, we no
68+
// longer block access to apps if they are unhealthy/initializing.
6469
let canClick = true;
6570
let icon = <BaseIcon app={app} />;
6671

6772
let primaryTooltip = "";
6873
if (app.health === "initializing") {
69-
canClick = false;
7074
icon = (
7175
// This is a hack to make the spinner appear in the center of the start
7276
// icon space
@@ -85,7 +89,6 @@ export const AppLink: FC<AppLinkProps> = ({ app, workspace, agent }) => {
8589
primaryTooltip = "Initializing...";
8690
}
8791
if (app.health === "unhealthy") {
88-
canClick = false;
8992
icon = <ErrorOutlineIcon css={{ color: theme.palette.warning.light }} />;
9093
primaryTooltip = "Unhealthy";
9194
}

0 commit comments

Comments
 (0)