Skip to content
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

[dashboard, supervisor, ws-proxy] Break redirect loop on failing workspaces starts #8125

Merged
merged 1 commit into from
Feb 14, 2022
Merged

[dashboard, supervisor, ws-proxy] Break redirect loop on failing workspaces starts #8125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions components/dashboard/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
"js-cookie": "^3.0.1",
"moment": "^2.29.1",
"monaco-editor": "^0.25.2",
"query-string": "^7.1.1",
"react": "^17.0.1",
"react-dom": "^17.0.1",
"react-router-dom": "^5.2.0",
Expand Down
3 changes: 2 additions & 1 deletion components/dashboard/src/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import { settingsPathAccount, settingsPathIntegrations, settingsPathMain, settin
import { projectsPathInstallGitHubApp, projectsPathMain, projectsPathMainWithParams, projectsPathNew } from './projects/projects.routes';
import { refreshSearchData } from './components/RepositoryFinder';
import { StartWorkspaceModal } from './workspaces/StartWorkspaceModal';
import { parseProps } from './start/StartWorkspace';

const Setup = React.lazy(() => import(/* webpackPrefetch: true */ './Setup'));
const Workspaces = React.lazy(() => import(/* webpackPrefetch: true */ './workspaces/Workspaces'));
Expand Down Expand Up @@ -412,7 +413,7 @@ function App() {
} else if (isCreation) {
toRender = <CreateWorkspace contextUrl={hash} />;
} else if (isWsStart) {
toRender = <StartWorkspace workspaceId={hash} />;
toRender = <StartWorkspace {...parseProps(hash, window.location.search)} />;
} else if (/^(github|gitlab)\.com\/.+?/i.test(window.location.pathname)) {
let url = new URL(window.location.href)
url.hash = url.pathname
Expand Down
4 changes: 2 additions & 2 deletions components/dashboard/src/start/CreateWorkspace.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import Modal from "../components/Modal";
import { getGitpodService, gitpodHostUrl } from "../service/service";
import { UserContext } from "../user-context";
import { StartPage, StartPhase, StartWorkspaceError } from "./StartPage";
import StartWorkspace from "./StartWorkspace";
import StartWorkspace, { parseProps } from "./StartWorkspace";
import { openAuthorizeWindow } from "../provider-utils";
import { SelectAccountPayload } from "@gitpod/gitpod-protocol/lib/auth";
import { SelectAccountModal } from "../settings/SelectAccountModal";
Expand Down Expand Up @@ -154,7 +154,7 @@ export default class CreateWorkspace extends React.Component<CreateWorkspaceProp

const result = this.state?.result;
if (result?.createdWorkspaceId) {
return <StartWorkspace workspaceId={result.createdWorkspaceId} />;
return <StartWorkspace {...parseProps(result?.createdWorkspaceId, window.location.search)} />;
}

else if (result?.existingWorkspaces) {
Expand Down
147 changes: 122 additions & 25 deletions components/dashboard/src/start/StartWorkspace.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { ContextURL, DisposableCollection, GitpodServer, RateLimiterError, Start
import { IDEOptions } from "@gitpod/gitpod-protocol/lib/ide-protocol";
import { ErrorCodes } from "@gitpod/gitpod-protocol/lib/messaging/error";
import EventEmitter from "events";
import * as queryString from "query-string";
import React, { Suspense, useEffect } from "react";
import { v4 } from 'uuid';
import Arrow from "../components/Arrow";
Expand All @@ -21,10 +22,54 @@ const sessionId = v4();
const WorkspaceLogs = React.lazy(() => import('../components/WorkspaceLogs'));

export interface StartWorkspaceProps {
workspaceId: string;
workspaceId: string,
runsInIFrame: boolean,
/**
* This flag is used to break the autostart-cycle explained in https://github.com/gitpod-io/gitpod/issues/8043
*/
dontAutostart: boolean,
}

export function parseProps(workspaceId: string, search?: string): StartWorkspaceProps {
const params = parseParameters(search);
const runsInIFrame = window.top !== window.self;
return {
workspaceId,
runsInIFrame: window.top !== window.self,
// Either:
// - not_found: we were sent back from a workspace cluster/IDE URL where we expected a workspace to be running but it wasn't because either:
// - this is a (very) old tab and the workspace already timed out
// - due to a start error our workspace terminated very quickly between:
// a) us being redirected to that IDEUrl (based on the first ws-manager update) and
// b) our requests being validated by ws-proxy
// - runsInIFrame (IDE case):
// - we assume the workspace has already been started for us
// - we don't know it's instanceId
dontAutostart: params.notFound || runsInIFrame,
}
}

function parseParameters(search?: string): { notFound?: boolean } {
try {
if (search === undefined) {
return {};
}
const params = queryString.parse(search, {parseBooleans: true});
const notFound = !!(params && params["not_found"]);
return {
notFound,
};
} catch (err) {
console.error("/start: error parsing search params", err);
return {};
}
}

export interface StartWorkspaceState {
/**
* This is set to the istanceId we started (think we started on).
* We only receive updates for this particular instance, or none if not set.
*/
startedInstanceId?: string;
workspaceInstance?: WorkspaceInstance;
workspace?: Workspace;
Expand All @@ -34,8 +79,8 @@ export interface StartWorkspaceState {
link: string
label: string
clientID?: string
}
ideOptions?: IDEOptions
};
ideOptions?: IDEOptions;
}

export default class StartWorkspace extends React.Component<StartWorkspaceProps, StartWorkspaceState> {
Expand All @@ -47,7 +92,7 @@ export default class StartWorkspace extends React.Component<StartWorkspaceProps,

private readonly toDispose = new DisposableCollection();
componentWillMount() {
if (this.runsInIFrame()) {
if (this.props.runsInIFrame) {
window.parent.postMessage({ type: '$setSessionId', sessionId }, '*');
const setStateEventListener = (event: MessageEvent) => {
if (event.data.type === 'setState' && 'state' in event.data && typeof event.data['state'] === 'object') {
Expand Down Expand Up @@ -75,8 +120,15 @@ export default class StartWorkspace extends React.Component<StartWorkspaceProps,
this.setState({ error });
}

if (this.props.dontAutostart) {
this.fetchWorkspaceInfo(undefined);
return;
}

// dashboard case (no previous errors): start workspace as quickly as possible
this.startWorkspace();
getGitpodService().server.getIDEOptions().then(ideOptions => this.setState({ ideOptions }))
// query IDE options so we can show them if necessary once the workspace is running
getGitpodService().server.getIDEOptions().then(ideOptions => this.setState({ ideOptions }));
}

componentWillUnmount() {
Expand Down Expand Up @@ -130,14 +182,13 @@ export default class StartWorkspace extends React.Component<StartWorkspaceProps,
}
console.log("/start: started workspace instance: " + result.instanceID);
// redirect to workspaceURL if we are not yet running in an iframe
if (!this.runsInIFrame() && result.workspaceURL) {
if (!this.props.runsInIFrame && result.workspaceURL) {
this.redirectTo(result.workspaceURL);
return;
}
this.setState({ startedInstanceId: result.instanceID });
// Explicitly query state to guarantee we get at least one update
// Start listening too instance updates - and explicitly query state once to guarantee we get at least one update
// (needed for already started workspaces, and not hanging in 'Starting ...' for too long)
this.fetchWorkspaceInfo();
this.fetchWorkspaceInfo(result.instanceID);
} catch (error) {
console.error(error);
if (typeof error === 'string') {
Expand Down Expand Up @@ -180,15 +231,28 @@ export default class StartWorkspace extends React.Component<StartWorkspaceProps,
}
}

async fetchWorkspaceInfo() {
/**
* Fetches initial WorkspaceInfo from the server. If there is a WorkspaceInstance for workspaceId, we feed it
* into "onInstanceUpdate" and start accepting further updates.
*
* @param startedInstanceId The instanceId we want to listen on
*/
async fetchWorkspaceInfo(startedInstanceId: string | undefined) {
// this ensures we're receiving updates for this instance
if (startedInstanceId) {
this.setState({ startedInstanceId });
}

const { workspaceId } = this.props;
try {
const info = await getGitpodService().server.getWorkspace(workspaceId);
if (info.latestInstance) {
this.setState({
workspace: info.workspace
});
this.onInstanceUpdate(info.latestInstance);
const instance = info.latestInstance;
this.setState((s) => ({
workspace: info.workspace,
startedInstanceId: s.startedInstanceId || instance.id, // note: here's a potential mismatch between startedInstanceId and instance.id. TODO(gpl) How to handle this?
}));
this.onInstanceUpdate(instance);
}
} catch (error) {
console.error(error);
Expand All @@ -197,20 +261,35 @@ export default class StartWorkspace extends React.Component<StartWorkspaceProps,
}

notifyDidOpenConnection() {
this.fetchWorkspaceInfo();
this.fetchWorkspaceInfo(undefined);
}

async onInstanceUpdate(workspaceInstance: WorkspaceInstance) {
const startedInstanceId = this.state?.startedInstanceId;
if (workspaceInstance.workspaceId !== this.props.workspaceId || startedInstanceId !== workspaceInstance.id) {
if (workspaceInstance.workspaceId !== this.props.workspaceId) {
return;
}

// Here we filter out updates to instances we haven't started to avoid issues with updates coming in out-of-order
// (e.g., multiple "stopped" events from the older instance, where we already started a fresh one after the first)
// Only exception is when we do the switch from the "old" to the "new" one.
const startedInstanceId = this.state?.startedInstanceId;
if (startedInstanceId !== workspaceInstance.id) {
// do we want to switch to "new" instance we just received an update for?
const switchToNewInstance = this.state.workspaceInstance?.status.phase === "stopped" && workspaceInstance.status.phase !== "stopped";
Copy link
Member

Choose a reason for hiding this comment

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

if previous instances is in stopping or never reaches stopped, this reads like an dead end.

assuming that we always have a single instance running per workspace, let's just focus on the status/phase of the new.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree in general, but think it's not super important. And has been like this for years.

But in light of this change we should watch more sure! 🙏

if (!switchToNewInstance) {
return;
}
this.setState({
startedInstanceId: workspaceInstance.id,
workspaceInstance,
});
}

await this.ensureWorkspaceAuth(workspaceInstance.id);

// Redirect to workspaceURL if we are not yet running in an iframe.
// It happens this late if we were waiting for a docker build.
if (!this.runsInIFrame() && workspaceInstance.ideUrl) {
if (!this.props.runsInIFrame && workspaceInstance.ideUrl && !this.props.dontAutostart) {
this.redirectTo(workspaceInstance.ideUrl);
return;
}
Expand Down Expand Up @@ -255,22 +334,18 @@ export default class StartWorkspace extends React.Component<StartWorkspaceProps,
}

redirectTo(url: string) {
if (this.runsInIFrame()) {
if (this.props.runsInIFrame) {
window.parent.postMessage({ type: 'relocate', url }, '*');
} else {
window.location.href = url;
}
}

runsInIFrame() {
return window.top !== window.self;
}

render() {
const { error } = this.state;
const isHeadless = this.state.workspace?.type !== 'regular';
const isPrebuilt = WithPrebuild.is(this.state.workspace?.context);
let phase = StartPhase.Preparing;
let phase: StartPhase | undefined = StartPhase.Preparing;
let title = undefined;
let statusMessage = !!error ? undefined : <p className="text-base text-gray-400">Preparing workspace …</p>;
const contextURL = ContextURL.getNormalizedURL(this.state.workspace)?.toString();
Expand Down Expand Up @@ -317,7 +392,29 @@ export default class StartWorkspace extends React.Component<StartWorkspaceProps,
}
if (!this.state.desktopIde) {
phase = StartPhase.Running;
statusMessage = <p className="text-base text-gray-400">Opening Workspace …</p>;

if (this.props.dontAutostart) {
// hide the progress bar, as we're already running
phase = undefined;
title = 'Running';

// in case we dontAutostart the IDE we have to provide controls to do so
akosyakov marked this conversation as resolved.
Show resolved Hide resolved
statusMessage = <div>
<div className="flex space-x-3 items-center text-left rounded-xl m-auto px-4 h-16 w-72 mt-4 mb-2 bg-gray-100 dark:bg-gray-800">
<div className="rounded-full w-3 h-3 text-sm bg-green-500">&nbsp;</div>
<div>
<p className="text-gray-700 dark:text-gray-200 font-semibold w-56 truncate">{this.state.workspaceInstance.workspaceId}</p>
<a target="_parent" href={contextURL}><p className="w-56 truncate hover:text-blue-600 dark:hover:text-blue-400" >{contextURL}</p></a>
</div>
</div>
<div className="mt-10 justify-center flex space-x-2">
<a target="_parent" href={gitpodHostUrl.asDashboard().toString()}><button className="secondary">Go to Dashboard</button></a>
<a target="_parent" href={gitpodHostUrl.asStart(this.props.workspaceId).toString() /** move over 'start' here to fetch fresh credentials in case this is an older tab */}><button>Open Workspace</button></a>
</div>
</div>;
} else {
statusMessage = <p className="text-base text-gray-400">Opening Workspace …</p>;
}
} else {
phase = StartPhase.IdeReady;
const openLink = this.state.desktopIde.link;
Expand Down
14 changes: 0 additions & 14 deletions components/supervisor/frontend/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,6 @@ const toStop = new DisposableCollection();

//#region current-frame
let current: HTMLElement = loading.frame;
let stopped = false;
let desktopRedirected = false;
const nextFrame = () => {
const instance = gitpodServiceClient.info.latestInstance;
Expand Down Expand Up @@ -142,19 +141,6 @@ const toStop = new DisposableCollection();
return document.body;
}
}
if (instance.status.phase === 'stopped') {
stopped = true;
}
if (stopped && (
instance.status.phase === 'preparing' ||
instance.status.phase === 'pending' ||
instance.status.phase === 'creating' ||
instance.status.phase === 'initializing')) {
// reload the page if the workspace was restarted to ensure:
Copy link
Member

@AlexTugarev AlexTugarev Feb 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@geropl, how relevant is this removal?
owner token etc. will be fine?

Copy link
Member

Choose a reason for hiding this comment

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

@AlexTugarev see discussions here: #8125 (comment)

I think it is the proper way to go.

Copy link
Member Author

Choose a reason for hiding this comment

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

owner token etc. will be fine?

Yes. as Anton pointed out:

  • the mechanism got pushed into StartWorkspace (where it belonged in the first place)
  • fetchWorkspaceInfo handles owner token renewal (ensureWorkspaceAuth)

Copy link
Member

@akosyakov akosyakov Feb 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We do it on purpose to remove dependencies from supervisor and loading screen. Now a user will need to reload explicitly. We add a new button for that on the loading screen. I think it is fine.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I missed that, as it was resolved.

Nice and clean!

// - graceful reconnection of IDEs
// - new owner token is set
window.location.href = startUrl.toString();
}
}
return loading.frame;
}
Expand Down
2 changes: 1 addition & 1 deletion components/ws-proxy/pkg/proxy/routes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,7 +550,7 @@ func workspaceMustExistHandler(config *Config, infoProvider WorkspaceInfoProvide
info := infoProvider.WorkspaceInfo(coords.ID)
if info == nil {
log.WithFields(log.OWI("", coords.ID, "")).Info("no workspace info found - redirecting to start")
redirectURL := fmt.Sprintf("%s://%s/start/#%s", config.GitpodInstallation.Scheme, config.GitpodInstallation.HostName, coords.ID)
redirectURL := fmt.Sprintf("%s://%s/start/?not_found=true#%s", config.GitpodInstallation.Scheme, config.GitpodInstallation.HostName, coords.ID)
http.Redirect(resp, req, redirectURL, http.StatusFound)
return
}
Expand Down
8 changes: 4 additions & 4 deletions components/ws-proxy/pkg/proxy/routes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,10 +414,10 @@ func TestRoutes(t *testing.T) {
Status: http.StatusFound,
Header: http.Header{
"Content-Type": {"text/html; charset=utf-8"},
"Location": {"https://test-domain.com/start/#blabla-smelt-9ba20cc1"},
"Location": {"https://test-domain.com/start/?not_found=true#blabla-smelt-9ba20cc1"},
"Vary": {"Accept-Encoding"},
},
Body: ("<a href=\"https://test-domain.com/start/#blabla-smelt-9ba20cc1\">Found</a>.\n\n"),
Body: ("<a href=\"https://test-domain.com/start/?not_found=true#blabla-smelt-9ba20cc1\">Found</a>.\n\n"),
},
},
{
Expand All @@ -429,10 +429,10 @@ func TestRoutes(t *testing.T) {
Status: http.StatusFound,
Header: http.Header{
"Content-Type": {"text/html; charset=utf-8"},
"Location": {"https://test-domain.com/start/#blabla-smelt-9ba20cc1"},
"Location": {"https://test-domain.com/start/?not_found=true#blabla-smelt-9ba20cc1"},
"Vary": {"Accept-Encoding"},
},
Body: ("<a href=\"https://test-domain.com/start/#blabla-smelt-9ba20cc1\">Found</a>.\n\n"),
Body: ("<a href=\"https://test-domain.com/start/?not_found=true#blabla-smelt-9ba20cc1\">Found</a>.\n\n"),
},
},
{
Expand Down
10 changes: 10 additions & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14397,6 +14397,16 @@ query-string@^6.13.3:
split-on-first "^1.0.0"
strict-uri-encode "^2.0.0"

query-string@^7.1.1:
version "7.1.1"
resolved "https://registry.yarnpkg.com/query-string/-/query-string-7.1.1.tgz#754620669db978625a90f635f12617c271a088e1"
integrity sha512-MplouLRDHBZSG9z7fpuAAcI7aAYjDLhtsiVZsevsfaHWDS2IDdORKbSd1kWUA+V4zyva/HZoSfpwnYMMQDhb0w==
dependencies:
decode-uri-component "^0.2.0"
filter-obj "^1.1.0"
split-on-first "^1.0.0"
strict-uri-encode "^2.0.0"

querystring-es3@^0.2.0:
version "0.2.1"
resolved "https://registry.yarnpkg.com/querystring-es3/-/querystring-es3-0.2.1.tgz#9ec61f79049875707d69414596fd907a4d711e73"
Expand Down