Skip to content

fix: allow access to unhealthy/initializing apps #12086

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 13, 2024
Merged

fix: allow access to unhealthy/initializing apps #12086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b498cd5
fix: allow access to unhealthy/initializing apps
deansheather Feb 12, 2024
b22bbc2
Merge branch 'main' into dean/unhealthy-app-access
deansheather Feb 12, 2024
bd78f5d
Add test for initializing apps too
deansheather Feb 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions coderd/workspaceapps/db.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,11 +198,9 @@ func (p *DBTokenProvider) Issue(ctx context.Context, rw http.ResponseWriter, r *
return nil, "", false
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// in case there are bugs with the healthcheck code that lock users out of
// in case there are bugs with the healthcheck that lock users out of

could be a lot of things: misconfiguration, app healthcheck endpoint could be misbehaving

// their apps completely.

// As a sanity check, ensure the token we just made is valid for this
// request.
Expand Down
91 changes: 76 additions & 15 deletions coderd/workspaceapps/db_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,12 @@ func Test_ResolveRequest(t *testing.T) {
appNameAuthed = "app-authed"
appNamePublic = "app-public"
appNameInvalidURL = "app-invalid-url"
appNameUnhealthy = "app-unhealthy"
// Users can access unhealthy and initializing apps (as of 2024-02).
appNameUnhealthy = "app-unhealthy"
appNameInitializing = "app-initializing"

// This agent will never connect, so it will never become "connected".
// Users cannot access unhealthy agents.
agentNameUnhealthy = "agent-unhealthy"
appNameAgentUnhealthy = "app-agent-unhealthy"

Expand All @@ -55,6 +58,15 @@ func Test_ResolveRequest(t *testing.T) {
w.WriteHeader(http.StatusInternalServerError)
_, _ = w.Write([]byte("unhealthy"))
}))
t.Cleanup(unhealthySrv.Close)

// Start a listener for a server that never responds.
initializingServer, err := net.Listen("tcp", "localhost:0")
require.NoError(t, err)
t.Cleanup(func() {
_ = initializingServer.Close()
})
initializingURL := fmt.Sprintf("http://%s", initializingServer.Addr().String())

deploymentValues := coderdtest.DeploymentValues(t)
deploymentValues.DisablePathApps = false
Expand Down Expand Up @@ -82,7 +94,7 @@ func Test_ResolveRequest(t *testing.T) {
})

ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitMedium)
defer cancel()
t.Cleanup(cancel)

firstUser := coderdtest.CreateFirstUser(t, client)
me, err := client.User(ctx, codersdk.Me)
Expand Down Expand Up @@ -143,6 +155,17 @@ func Test_ResolveRequest(t *testing.T) {
Threshold: 1,
},
},
{
Slug: appNameInitializing,
DisplayName: appNameInitializing,
SharingLevel: proto.AppSharingLevel_PUBLIC,
Url: appURL,
Healthcheck: &proto.Healthcheck{
Url: initializingURL,
Interval: 30,
Threshold: 1000,
},
},
},
},
{
Expand Down Expand Up @@ -805,7 +828,55 @@ func Test_ResolveRequest(t *testing.T) {
require.Contains(t, bodyStr, `Agent state is "`)
})

t.Run("UnhealthyApp", func(t *testing.T) {
// Initializing apps are now permitted to connect anyways. This wasn't
// always the case, but we're testing the behavior to ensure it doesn't
// change back accidentally.
t.Run("InitializingAppPermitted", func(t *testing.T) {
t.Parallel()

ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
defer cancel()

agent, err := client.WorkspaceAgent(ctx, agentID)
require.NoError(t, err)

for _, app := range agent.Apps {
if app.Slug == appNameInitializing {
t.Log("app is", app.Health)
require.Equal(t, codersdk.WorkspaceAppHealthInitializing, app.Health)
break
}
}

req := (workspaceapps.Request{
AccessMethod: workspaceapps.AccessMethodPath,
BasePath: "/app",
UsernameOrID: me.Username,
WorkspaceNameOrID: workspace.Name,
AgentNameOrID: agentName,
AppSlugOrPort: appNameInitializing,
}).Normalize()

rw := httptest.NewRecorder()
r := httptest.NewRequest("GET", "/app", nil)
r.Header.Set(codersdk.SessionTokenHeader, client.SessionToken())

token, ok := workspaceapps.ResolveRequest(rw, r, workspaceapps.ResolveRequestOptions{
Logger: api.Logger,
SignedTokenProvider: api.WorkspaceAppsProvider,
DashboardURL: api.AccessURL,
PathAppBaseURL: api.AccessURL,
AppHostname: api.AppHostname,
AppRequest: req,
})
require.True(t, ok, "ResolveRequest failed, should pass even though app is initializing")
require.NotNil(t, token)
})

// Unhealthy apps are now permitted to connect anyways. This wasn't always
// the case, but we're testing the behavior to ensure it doesn't change back
// accidentally.
t.Run("UnhealthyAppPermitted", func(t *testing.T) {
t.Parallel()

require.Eventually(t, func() bool {
Expand Down Expand Up @@ -850,17 +921,7 @@ func Test_ResolveRequest(t *testing.T) {
AppHostname: api.AppHostname,
AppRequest: req,
})
require.False(t, ok, "request succeeded even though app is unhealthy")
require.Nil(t, token)

w := rw.Result()
defer w.Body.Close()
require.Equal(t, http.StatusBadGateway, w.StatusCode)

body, err := io.ReadAll(w.Body)
require.NoError(t, err)
bodyStr := string(body)
bodyStr = strings.ReplaceAll(bodyStr, """, `"`)
require.Contains(t, bodyStr, `App health is "unhealthy"`)
require.True(t, ok, "ResolveRequest failed, should pass even though app is unhealthy")
require.NotNil(t, token)
})
}
7 changes: 0 additions & 7 deletions coderd/workspaceapps/request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,9 +198,6 @@ type databaseRequest struct {
// AppURL is the resolved URL to the workspace app. This is only set for non
// terminal requests.
AppURL *url.URL
// AppHealth is the health of the app. For terminal requests, this is always
// database.WorkspaceAppHealthHealthy.
AppHealth database.WorkspaceAppHealth
// AppSharingLevel is the sharing level of the app. This is forced to be set
// to AppSharingLevelOwner if the access method is terminal.
AppSharingLevel database.AppSharingLevel
Expand Down Expand Up @@ -292,7 +289,6 @@ func (r Request) getDatabase(ctx context.Context, db database.Store) (*databaseR
agentNameOrID = r.AgentNameOrID
appURL string
appSharingLevel database.AppSharingLevel
appHealth = database.WorkspaceAppHealthDisabled
portUint, portUintErr = strconv.ParseUint(r.AppSlugOrPort, 10, 16)
)
if portUintErr == nil {
Expand Down Expand Up @@ -331,7 +327,6 @@ func (r Request) getDatabase(ctx context.Context, db database.Store) (*databaseR
appSharingLevel = database.AppSharingLevelOwner
}
appURL = app.Url.String
appHealth = app.Health
break
}
}
Expand Down Expand Up @@ -377,7 +372,6 @@ func (r Request) getDatabase(ctx context.Context, db database.Store) (*databaseR
Workspace: workspace,
Agent: agent,
AppURL: appURLParsed,
AppHealth: appHealth,
AppSharingLevel: appSharingLevel,
}, nil
}
Expand Down Expand Up @@ -430,7 +424,6 @@ func (r Request) getDatabaseTerminal(ctx context.Context, db database.Store) (*d
Workspace: workspace,
Agent: agent,
AppURL: nil,
AppHealth: database.WorkspaceAppHealthHealthy,
AppSharingLevel: database.AppSharingLevelOwner,
}, nil
}
7 changes: 5 additions & 2 deletions site/src/modules/resources/AppLink/AppLink.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,16 @@ export const AppLink: FC<AppLinkProps> = ({ app, workspace, agent }) => {
app,
);

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

let primaryTooltip = "";
if (app.health === "initializing") {
canClick = false;
icon = (
// This is a hack to make the spinner appear in the center of the start
// icon space
Expand All @@ -85,7 +89,6 @@ export const AppLink: FC<AppLinkProps> = ({ app, workspace, agent }) => {
primaryTooltip = "Initializing...";
}
if (app.health === "unhealthy") {
canClick = false;
icon = <ErrorOutlineIcon css={{ color: theme.palette.warning.light }} />;
primaryTooltip = "Unhealthy";
}
Expand Down