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.

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

Bug 1838658: Use new proxy to connect to cloudshell in terminal #5428

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
import * as React from 'react';
import { connect } from 'react-redux';
import { Base64 } from 'js-base64';
import { StatusBox } from '@console/internal/components/utils';
import { connectToFlags, WithFlagsProps } from '@console/internal/reducers/features';
import { impersonateStateToProps } from '@console/internal/reducers/ui';
import { FLAGS } from '@console/shared';
import { WSFactory } from '@console/internal/module/ws-factory';
import { resourceURL } from '@console/internal/module/k8s';
import { PodModel } from '@console/internal/models';
import Terminal from './Terminal';
import TerminalLoadingBox from './TerminalLoadingBox';

// pod exec WS protocol is FD prefixed, base64 encoded data (sometimes json stringified)

// Channel 0 is STDIN, 1 is STDOUT, 2 is STDERR (if TTY is not requested), and 3 is a special error channel - 4 is C&C
// The server only reads from STDIN, writes to the other three.
// see also: https://github.com/kubernetes/kubernetes/pull/13885

type Props = {
container: string;
podname: string;
namespace: string;
shcommand?: string[];
Comment on lines +21 to +24
Copy link
Contributor

Choose a reason for hiding this comment

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

All of these props affect the web socket but none of them will cause the web socket to reconnect if they change.

We may not get into this use case for the current implementation however we should be creating reactive components that update according to their incoming props.

};

type StateProps = {
impersonate?: {
subprotocols: string[];
};
};

type CloudShellExecProps = Props & StateProps & WithFlagsProps;

type CloudShellExecState = {
open: boolean;
error: string;
};

const NO_SH =
'starting container process caused "exec: \\"sh\\": executable file not found in $PATH"';

class CloudShellExec extends React.PureComponent<CloudShellExecProps, CloudShellExecState> {
private terminal;

private ws;
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
private terminal;
private ws;
private terminal = React.createRef<Terminal>();
private ws: WSFactory;


constructor(props) {
super(props);
this.state = {
open: false,
error: null,
};
this.terminal = React.createRef();
}

componentDidMount() {
this.connect();
}

componentWillUnmount() {
this.ws && this.ws.destroy();
delete this.ws;
}

onData = (data: string): void => {
this.ws && this.ws.send(`0${Base64.encode(data)}`);
};

connect() {
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
connect() {
private connect() {

const { container, podname, namespace, shcommand, flags, impersonate } = this.props;
const usedClient = flags[FLAGS.OPENSHIFT] ? 'oc' : 'kubectl';
const cmd = shcommand || ['sh', '-i', '-c', 'TERM=xterm sh'];

const params = {
ns: namespace,
name: podname,
path: 'exec',
queryParams: {
stdout: '1',
stdin: '1',
stderr: '1',
tty: '1',
container,
command: cmd.map((c) => encodeURIComponent(c)).join('&command='),
},
};

if (this.ws) {
this.ws.destroy();
const currentTerminal = this.terminal.current;
currentTerminal && currentTerminal.onConnectionClosed(`connecting to ${container}`);
}

const subprotocols = (impersonate?.subprotocols || []).concat('base64.channel.k8s.io');

let previous;
this.ws = new WSFactory(`${podname}-terminal`, {
host: 'auto',
reconnect: true,
path: resourceURL(PodModel, params),
jsonParse: false,
subprotocols,
})
.onmessage((raw) => {
const currentTerminal = this.terminal.current;
// error channel
if (raw[0] === '3') {
if (previous.includes(NO_SH)) {
currentTerminal.reset();
Copy link
Contributor

Choose a reason for hiding this comment

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

Inconsistent. Missing null check as per rest of usage of this.terminal.current.

currentTerminal.onConnectionClosed(
`This container doesn't have a /bin/sh shell. Try specifying your command in a terminal with:\r\n\r\n ${usedClient} -n ${this.props.namespace} exec ${this.props.podname} -ti <command>`,
);
this.ws.destroy();
previous = '';
return;
}
}
const data = Base64.decode(raw.slice(1));
currentTerminal && currentTerminal.onDataReceived(data);
previous = data;
})
.onopen(() => {
const currentTerminal = this.terminal.current;
currentTerminal && currentTerminal.reset();
previous = '';
this.setState({ open: true, error: null });
})
.onclose((evt) => {
if (!evt || evt.wasClean === true) {
return;
}
const error = evt.reason || 'The terminal connection has closed.';
this.setState({ error });
this.terminal.current && this.terminal.current.onConnectionClosed(error);
Copy link
Contributor

Choose a reason for hiding this comment

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

Call setState as the very last item in this function otherwise the terminal becomes nullified immediately (surprisingly).

this.ws.destroy();
}) // eslint-disable-next-line no-console
.onerror((evt) => console.error(`WS error?! ${evt}`));
}

render() {
const { open, error } = this.state;
if (error) {
return <StatusBox loadError={error} label="OpenShift command line terminal" />;
Copy link
Contributor

Choose a reason for hiding this comment

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

StatusBox is incorrectly used here because the error needs to confirm to a specific format.
Use LoadError instead.
Probably need to also pass canRetry={false} because refreshing the page doesn't bring the user back to an open terminal. Ideally we make the the link configurable and restart the terminal instead of refreshing the page (something for future consideration).

}
if (open) {
return <Terminal onData={this.onData} ref={this.terminal} />;
}
return <TerminalLoadingBox />;
}
}

export default connect<StateProps>(impersonateStateToProps)(
connectToFlags<CloudShellExecProps & WithFlagsProps>(FLAGS.OPENSHIFT)(CloudShellExec),
);
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
.odc-cloudshell-terminal {
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
.odc-cloudshell-terminal {
.co-cloudshell-terminal {

&__container {
background-color: var(--pf-global--BackgroundColor--dark-100);
Copy link
Contributor

Choose a reason for hiding this comment

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

This background color is incorrect. We need pure black.

Copy link
Contributor

Choose a reason for hiding this comment

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

So back to black-10000?

Copy link
Contributor

Choose a reason for hiding this comment

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

The default terminal theme background color is pure black. So whatever gets us to pure black.
Unless we choose to override. But at this point being consistent with the other terminal is probably best.

color: var(--pf-global--Color--light-100);
width: 100%;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: width shouldn't be needed on a block element div

height: 100%;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,19 @@ import { connect } from 'react-redux';
import { RootState } from '@console/internal/redux';
import { referenceForModel } from '@console/internal/module/k8s/k8s';
import { useK8sWatchResource } from '@console/internal/components/utils/k8s-watch-hook';
import { LoadingBox, StatusBox } from '@console/internal/components/utils/status-box';
import { StatusBox } from '@console/internal/components/utils/status-box';
import { WorkspaceModel } from '../../models';
import CloudShellTerminalFrame from './CloudShellTerminalFrame';
import CloudshellExec from './CloudShellExec';
import TerminalLoadingBox from './TerminalLoadingBox';
import {
CLOUD_SHELL_LABEL,
CLOUD_SHELL_USER_ANNOTATION,
CloudShellResource,
TerminalInitData,
initTerminal,
} from './cloud-shell-utils';
import CloudShellSetup from './setup/CloudShellSetup';
import './CloudShellTerminal.scss';

type StateProps = {
username: string;
Expand All @@ -32,37 +36,81 @@ const resource = {
};

const CloudShellTerminal: React.FC<CloudShellTerminalProps> = ({ username, onCancel }) => {
const [data, loaded, loadError] = useK8sWatchResource<CloudShellResource>(resource);
const [data, loaded, loadError] = useK8sWatchResource<CloudShellResource[]>(resource);
const [initData, setInitData] = React.useState<TerminalInitData>();
const [initDataLoading, setInitDataLoading] = React.useState<boolean>(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is needed if you use initData null checks instead.

const [initError, setInitError] = React.useState<string>();
const [workspaceNamespace, setWorkspaceNamespace] = React.useState<string>();

if (loadError) {
React.useEffect(() => {
let destroy = false;
if (Array.isArray(data)) {
const workspace = data.find(
(ws) => ws?.metadata?.annotations?.[CLOUD_SHELL_USER_ANNOTATION] === username,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Workspace reference is needed outside of the effect to show CloudShellSetup.

const running = workspace?.status?.phase === 'Running';

if (running) {
setInitDataLoading(true);
const { name, namespace } = workspace.metadata;
initTerminal(username, name, namespace)
.then((res: TerminalInitData) => {
if (destroy) return;
setInitData(res);
setInitDataLoading(false);
setWorkspaceNamespace(namespace);
})
.catch(() => {
if (destroy) return;
setInitDataLoading(false);
setInitError('Failed to connect to your OpenShift command line terminal');
});
}
}

return () => {
destroy = true;
};
}, [data, username]);
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably only need to enter this effect if username or workspace phase or namespace changes.


if (loadError || initError) {
return (
<StatusBox loaded={loaded} loadError={loadError} label="OpenShift command line terminal" />
<StatusBox
loaded={loaded}
loadError={loadError || initError}
Copy link
Contributor

Choose a reason for hiding this comment

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

loadError cannot be a string as is the case for initError.

label="OpenShift command line terminal"
/>
);
}

if (!loaded) {
return <LoadingBox />;
if (!loaded || initDataLoading) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sometime between data being loaded, but not having run the effect to make the call to init, we skip this if condition resulting in a flash of the CloudShellSetup component.

We need to show the CloudShellSetup if data is loaded and the workspace is not found. For this reason finding the workspace in the data should probably be moved out of the effect.

return (
<div className="odc-cloudshell-terminal__container">
<TerminalLoadingBox />
</div>
);
}

if (Array.isArray(data)) {
const workspace = data.find(
(d) => d?.metadata?.annotations?.[CLOUD_SHELL_USER_ANNOTATION] === username,
if (initData && workspaceNamespace) {
return (
<div className="odc-cloudshell-terminal__container">
<CloudshellExec
namespace={workspaceNamespace}
container={initData.container}
podname={initData.pod}
shcommand={initData.cmd || []}
/>
</div>
);
if (workspace) {
const running = workspace.status?.phase === 'Running';
const url = workspace.status?.ideUrl;
return <CloudShellTerminalFrame loading={!running} url={url} />;
}
}

return <CloudShellSetup onCancel={onCancel} />;
};

// For testing
export const InternalCloudShellTerminal = CloudShellTerminal;

const stateToProps = (state: RootState): StateProps => ({
username: state.UI.get('user')?.metadata?.name || '',
});

// exposed for testing
export const InternalCloudShellTerminal = CloudShellTerminal;

export default connect(stateToProps)(CloudShellTerminal);

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
import * as React from 'react';
import { Terminal as XTerminal } from 'xterm';
import * as fit from 'xterm/lib/addons/fit/fit';

XTerminal.applyAddon(fit);

const terminalOptions = {
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
const terminalOptions = {
const terminalOptions: ITerminalOptions = {

fontFamily: 'monospace',
fontSize: 16,
cursorBlink: false,
cols: 80,
rows: 25,
padding: 4,
Copy link
Contributor

Choose a reason for hiding this comment

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

Padding is not an option on ITerminalOptions. This must be a feature only available in later releases.
We need to find a way to add it by some other means or for now just remove the padding.

};

type TerminalProps = {
onData: (data: string) => void;
};

class Terminal extends React.Component<TerminalProps> {
private terminalRef;

private terminal;

private resizeObserver;
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
private terminalRef;
private terminal;
private resizeObserver;
private terminalRef = React.createRef<HTMLDivElement>();
private terminal: XTerminal = new XTerminal(terminalOptions);
private resizeObserver: ResizeObserver;


constructor(props) {
super(props);
this.terminalRef = React.createRef<HTMLDivElement>();
this.terminal = new XTerminal(terminalOptions);
this.terminal.on('data', this.props.onData);
}

componentDidMount() {
this.terminal.open(this.terminalRef.current);
this.resizeObserver = new ResizeObserver(() => {
window.requestAnimationFrame(() => this.terminal.fit());
Copy link
Contributor

Choose a reason for hiding this comment

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

To work with typescript.
Import fit like this:
import { fit } from 'xterm/lib/addons/fit/fit';

Remove XTerminal.applyAddon(fit);

Suggested change
window.requestAnimationFrame(() => this.terminal.fit());
window.requestAnimationFrame(() => fit(this.terminal));

});
this.resizeObserver.observe(this.terminalRef.current);
this.focus();
}

componentWillUnmount() {
this.terminal.destroy();
this.resizeObserver.disconnect();
}

focus() {
this.terminal && this.terminal.focus();
Copy link
Contributor

Choose a reason for hiding this comment

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

this.terminal is always defined.

Suggested change
this.terminal && this.terminal.focus();
this.terminal.focus();

}

reset() {
this.terminal.reset();
this.terminal.clear();
this.terminal.setOption('disableStdin', false);
}

onDataReceived(data) {
this.terminal && this.terminal.write(data);
Copy link
Contributor

Choose a reason for hiding this comment

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

this.terminal is always defined.

Suggested change
this.terminal && this.terminal.write(data);
this.terminal.write(data);

}

onConnectionClosed = (reason) => {
this.terminal.write(`\x1b[31m${reason || 'disconnected'}\x1b[m\r\n`);
this.terminal.cursorHidden = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

cursorHidden doesn't exist in the type def of Terminal.
There is isCursorHidden on core service but unsure how to reach it...

this.terminal.setOption('disableStdin', true);
this.terminal.refresh(this.terminal.y, this.terminal.y);
Copy link
Contributor

Choose a reason for hiding this comment

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

y don't exist in the type def of Terminal

};

render() {
return <div ref={this.terminalRef} style={{ width: '100%', height: '100%' }} />;
}
}

export default Terminal;