Add UI changes for kube exec functionality. #41466

AntonAM · 2024-05-13T14:59:46Z

This PR adds UI for the Kube Exec functionality (backend part was added in #41144).
Mainly it changes appearance of connect dialog for the kube resource and adds new type of document for the console - DocumentKubeExec, which is very similar to the DocumentSsh. Also we allow new option for the terminal ConvertEol that correctly displays output for non-interactive shell, because we get just \n instead of \r\n newlines in that mode.

UI changes for the Connect dialog:

Demo in action:

kube_exec_demo.mov

Changelog: Add ability to exec into a pod on a Kubernetes cluster using Web UI.

github-actions · 2024-05-16T16:16:40Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

public-teleport-github-review-bot · 2024-05-16T16:16:46Z

@AntonAM - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes.

ravicious

I'll continue the review on Monday.

ravicious · 2024-05-17T13:30:56Z

web/packages/teleport/src/Console/DocumentKubeExec/DocumentKubeExec.story.test.tsx

+  const { container } = render(
+    <DocumentKubeExecWrapper ctx={ctx} consoleCtx={consoleCtx} doc={baseDoc} />
+  );
+  expect(container.firstChild).toMatchSnapshot();


I don't view this strictly as a requirement since I don't think you usually work on frontend stuff, but if you have time then I'd timebox this to like 30–60 minutes and see how far you can get with the idea below, it'd help us out in the long term! If not then I guess the snapshot test is fine.

I understand that this was mostly copied from DocumentSsh, but it'd be nice to add a regular test instead of a snapshot test, as we generally want to reduce the number of snapshot tests.

It could boil down to duplicating the setup from the story (so that the test does not depend on the story) and then performing a simple check. It could be just writing to the tty and then checking if that gets rendered on the screen.

If all else fails, the check could end up as data-testid="terminal" added to the div rendered by Terminal:

teleport/web/packages/teleport/src/Console/DocumentSsh/Terminal/Terminal.tsx

Lines 169 to 177 in 43d050b

<Flex

flexDirection="column"

height="100%"

width="100%"

px="2"

style={{ overflow: 'auto' }}

>

<StyledXterm ref={elementRef} />

</Flex>

…and then checking if the div got rendered on the screen:

teleport/web/packages/teleport/src/components/ToolTipNoPermBadge/ToolTipNoPermBadge.test.tsx

Line 37 in 43d050b

expect(screen.getByTestId('tooltip-msg')).toBeInTheDocument();

(Though using data-testid should be the last resort.)

ravicious · 2024-05-17T13:34:06Z

web/packages/teleport/src/Console/DocumentKubeExec/DocumentKubeExec.story.tsx

+export const Connected = () => {
+  const { ctx, consoleCtx } = getContexts();
+
+  return (
+    <DocumentKubeExecWrapper ctx={ctx} consoleCtx={consoleCtx} doc={baseDoc} />
+  );
+};
+
+export const NotFound = () => {
+  const { ctx, consoleCtx } = getContexts();
+
+  const disconnectedDoc = {
+    ...baseDoc,
+    status: 'disconnected' as const,
+  };
+
+  return (
+    <DocumentKubeExecWrapper
+      ctx={ctx}
+      consoleCtx={consoleCtx}
+      doc={disconnectedDoc}
+    />
+  );
+};
+
+export const ServerError = () => {
+  const { ctx, consoleCtx } = getContexts();
+
+  const noSidDoc = {
+    ...baseDoc,
+    sid: '',
+  };
+
+  return (
+    <DocumentKubeExecWrapper ctx={ctx} consoleCtx={consoleCtx} doc={noSidDoc} />
+  );
+};


In storybook, these three all render as the same to me, I'm not sure if that's what's supposed to happen.

I wanted to compare this against DocumentSsh stories, but it looks like stories for that component have been broken for some time.

Yeah, DocumentSsh was broken, I've fixed it (PR fixing it: #41877 ) and followed same pattern, it also renders basically same 🤷

ravicious · 2024-05-17T13:34:22Z

web/packages/teleport/src/Console/DocumentKubeExec/DocumentKubeExec.story.tsx

+export type Props = {
+  ctx: TeleportContext;
+  consoleCtx: ConsoleCtx;
+  doc: stores.DocumentKubeExec;
+};


This is not used and can be removed I suppose.

Forgot to use in the DocumentKubeExecWrapper 😅 , fixed 27fc01c

ravicious · 2024-05-17T13:38:43Z

web/packages/teleport/src/Console/DocumentKubeExec/DocumentKubeExec.story.tsx

+export const DocumentKubeExecWrapper = ({ ctx, consoleCtx, doc }) => {
+  return (
+    <ContextProvider ctx={ctx}>
+      <TestLayout ctx={consoleCtx}>
+        <DocumentKubeExec doc={doc} visible={true} />
+      </TestLayout>
+    </ContextProvider>
+  );
+};
+
+export function getContexts() {
+  const ctx = createTeleportContext();
+  const consoleCtx = new ConsoleCtx();
+  const tty = consoleCtx.createTty(session);
+  tty.connect = () => null;
+  consoleCtx.createTty = () => tty;
+  consoleCtx.storeUser = ctx.storeUser;
+
+  return { ctx, consoleCtx };
+}
+
+export const baseDoc = {
+  kind: 'kubeExec',
+  status: 'connected',
+  sid: 'sid-value',
+  clusterId: 'clusterId-value',
+  serverId: 'serverId-value',
+  login: 'login-value',
+  kubeCluster: 'kubeCluster1',
+  kubeNamespace: 'namespace1',
+  pod: 'pod1',
+  container: '',
+  id: 3,
+  url: 'fd',
+  created: new Date(),
+  command: '/bin/bash',
+  isInteractive: true,
+} as const;


That's one of the problems with exporting stuff out of stories and then importing it in tests. On a conceputal level, I think it's not what one's supposed to (though that's the pattern I think we've been following in a few places).

But a practical problem with it is that Storybook expects each exported member to be a React component, so we end up with a bunch of items that look like stories, but they just don't work.

ravicious · 2024-05-17T13:54:06Z

web/packages/teleport/src/Kubes/ConnectDialog/ConnectDialog.tsx

+                </Toggle>
+                <ButtonPrimary onClick={startKubeExecSession}>
+                  Run Command
+                </ButtonPrimary>


If we wrap the form into <form>, add onSubmit on it and change the button's type to submit and then add autofocus on the first field, then it'll be possible to navigate the form using keyboard and submit it with Enter.

Patch

Copy then pbpaste | git apply

diff --git a/web/packages/teleport/src/Kubes/ConnectDialog/ConnectDialog.tsx b/web/packages/teleport/src/Kubes/ConnectDialog/ConnectDialog.tsx index eb06d90ba8..ed7e858f08 100644 --- a/web/packages/teleport/src/Kubes/ConnectDialog/ConnectDialog.tsx +++ b/web/packages/teleport/src/Kubes/ConnectDialog/ConnectDialog.tsx @@ -56,7 +56,8 @@ function ConnectDialog(props: Props) { const [execCommand, setExecCommand] = useState(''); const [execInteractive, setExecInteractive] = useState(true); - const startKubeExecSession = () => { + const startKubeExecSession = event => { + event.preventDefault(); const splitPath = execPath.split('/'); const url = cfg.getKubeExecConnectRoute( @@ -136,58 +137,59 @@ function ConnectDialog(props: Props) { <Validation> {() => ( <Box borderTop={1} mb={4} mt={4}> - <Text mt={3} bold> - Or exec into a pod on this Kubernetes cluster - </Text> - <Flex gap={3}> - <FieldInput - value={execPath} - placeholder="namespace/pod" - label="Pod to exec into" - width="50%" - onChange={e => setExecPath(e.target.value.trim())} - toolTipContent={ - <Text> - Specify namespace and pod you want to exec into. - Optionally you can also specify container by adding it at - the end: '/namespace/pod/container' - </Text> - } - /> - <FieldInput - rule={requiredField('Command to execute is required')} - value={execCommand} - placeholder="/bin/bash" - label="Command to execute" - width="50%" - onChange={e => setExecCommand(e.target.value)} - toolTipContent={ - <Text> - The command that will be executed inside the target pod. - </Text> - } - /> - </Flex> - <Flex justifyContent="space-between" gap={3}> - <Toggle - isToggled={execInteractive} - onToggle={() => { - setExecInteractive(b => !b); - }} - > - <Box ml={2} mr={1}> - Interactive shell - </Box> - <ToolTipInfo> - You can start an interactive shell and have a bidirectional - communication with the target pod, or you can run one-off - command and see its output. - </ToolTipInfo> - </Toggle> - <ButtonPrimary onClick={startKubeExecSession}> - Run Command - </ButtonPrimary> - </Flex> + <form onSubmit={startKubeExecSession}> + <Text mt={3} bold> + Or exec into a pod on this Kubernetes cluster + </Text> + <Flex gap={3}> + <FieldInput + value={execPath} + placeholder="namespace/pod" + autoFocus + label="Pod to exec into" + width="50%" + onChange={e => setExecPath(e.target.value.trim())} + toolTipContent={ + <Text> + Specify namespace and pod you want to exec into. + Optionally you can also specify container by adding it + at the end: '/namespace/pod/container' + </Text> + } + /> + <FieldInput + rule={requiredField('Command to execute is required')} + value={execCommand} + placeholder="/bin/bash" + label="Command to execute" + width="50%" + onChange={e => setExecCommand(e.target.value)} + toolTipContent={ + <Text> + The command that will be executed inside the target pod. + </Text> + } + /> + </Flex> + <Flex justifyContent="space-between" gap={3}> + <Toggle + isToggled={execInteractive} + onToggle={() => { + setExecInteractive(b => !b); + }} + > + <Box ml={2} mr={1}> + Interactive shell + </Box> + <ToolTipInfo> + You can start an interactive shell and have a + bidirectional communication with the target pod, or you + can run one-off command and see its output. + </ToolTipInfo> + </Toggle> + <ButtonPrimary type="submit">Run Command</ButtonPrimary> + </Flex> + </form> </Box> )} </Validation>

ravicious

The biggest problem for me with this PR is that it adds the ability to execute an arbitrary command in a pod by opening the app with a specifically crafted address, but the PR description doesn't mention any considerations regarding the security of this feature.

In Connect, we had support for something like this (but with ssh servers), but we since removed it (see https://github.com/gravitational/teleport-private/issues/176, though the vulnerability persists).

web/packages/teleport/src/Console/DocumentKubeExec/useKubeExecSession.tsx

web/packages/teleport/src/Console/consoleContext.tsx

ravicious · 2024-05-20T13:52:03Z

web/packages/teleport/src/Console/consoleContext.tsx

+          pod: splits[1],
+          container: splits?.[2] || '',
+          isInteractive,
+          command,


As far as I understand, this accepts any command over a GET request and executes it on the pod without any further input. Have we considered the security implications of it? This feels worthy of an RFD. I tried looking for one, but I couldn't find any about this feature.

We had a slack thread about a similar request from a customer about a month ago.

It's not visible in the demo video for this PR, but in another one ( https://drive.google.com/file/d/1x517hgCgGsYsp89rzPOo3BQNfpFAasGe/view?usp=drive_link ) it's visible that session MFA is supported for this and you are required to perform the check, according to the cluster/role rules. So there's additional layer of protection for risk-averse clients. (we also until very recently had an assist endpoint to run a ssh command on nodes, but looks like it was deleted)
As I mentioned above, this is an MVP of the feature, and generally we want to change it into more shell-like experience, where you've given a terminal and you can run kubectl ... commands there, which would eliminate "command in the url" pattern.

I understand the desire to validate an MVP and the support for MFA is a good thing to have there. But at the moment it seems to be a feature that's enabled by default which we didn't put under as much scrutiny as features that went through the RFD process.

If it was an opt-in feature, that would somewhat change things, but I do realize that having to turn it on would make it much less appealing.

ravicious · 2024-05-20T13:57:03Z

web/packages/teleport/src/Kubes/ConnectDialog/ConnectDialog.tsx

+                  value={execPath}
+                  placeholder="namespace/pod"
+                  label="Pod to exec into"
+                  width="50%"


Nit, but the parent of this element is already a flex container, so you can use flex="1" instead to give both inputs the same weight and distribute their width accordingly within the parent.

It doesn't make much difference here, but it's a better approach for any other more complex set of divs (as you don't have to calculate the percentages yourself and operate on weights instead, plus a whole other slew of features that flexbox gives you).

ravicious · 2024-05-20T13:58:53Z

web/packages/teleport/src/UnifiedResources/ResourceActionButton.tsx

Those changes to imports in this file seem unnecessary.

It looks like some kind of editor plugin for sorting them alphabetically, which I'd love to enforce down the line with some kind of autofixable eslint rule.

ravicious · 2024-05-20T14:00:07Z

web/packages/teleport/src/config.ts

+    if (!params.container) {
+      params.container = undefined;
+    }


Why is this needed?

rosstimothy · 2024-05-22T15:38:28Z

Couple of high level notes without having looked too much at the code:

UX

The input with namespace/pod was a hacky thing we did during the initial POC. I think we should split them into two separate inputs here.

Security

The biggest problem for me with this PR is that it adds the ability to execute an arbitrary command in a pod by opening the app with a specifically crafted address, but the PR description doesn't mention any considerations regarding the security of this feature.

I also agree with Rafal on the above. I think we need some user interaction to prevent people from being phished with a malicious link. One option that jumps to mind would be to split the command input and interactive toggle out of the connect modal and be required input on the new tab. So the flow would be the following:

Connect button determines namespace and pod
New tab is opened and connection upgrade occurs
Server side waits for a message via the WS with the rest of the information
UI presents some way for users to determine the command and whether an interactive session is required
UI sends users additional input via the WS
Server proceeds with connection to kube

AntonAM · 2024-05-22T16:03:51Z

@rosstimothy

UX:
I don't really see benefit of splitting input into two, namespace/pod seems like a natural format, also we have optional container one could specify with namespace/pod/container, which would require third input.

Security:
Ok, I'll look into splitting the flow 👍

rosstimothy · 2024-05-22T16:25:20Z

UX: I don't really see benefit of splitting input into two, namespace/pod seems like a natural format, also we have optional container one could specify with namespace/pod/container, which would require third input.

@roraback do you have opinions about the design/UX in question?

dmsergeevN26 · 2024-05-23T13:46:27Z

Amazing! Is it possible to add support for kubectl debug in addition/instead of exec? Debug is the preferred way to debug pods nowadays. Does this support moderation? I hope my questions are not out of place! Thank you!

AntonAM · 2024-05-28T14:34:29Z

@dmsergeevN26 We're currently evaluating paths to expand kubernetes access in Web UI, this is our first step in this direction. Debug functionality might be implemented in the future. You're feedback is welcomed!

AntonAM added the kubernetes-access label May 13, 2024

AntonAM force-pushed the kweb-ui branch 5 times, most recently from 5a2b16a to acc7bba Compare May 16, 2024 14:43

AntonAM marked this pull request as ready for review May 16, 2024 16:16

github-actions bot requested review from ravicious and rudream May 16, 2024 16:16

github-actions bot added size/xl ui labels May 16, 2024

AntonAM force-pushed the kweb-ui branch from acc7bba to 62cfcd6 Compare May 16, 2024 18:14

Add UI changes for kube exec functionality.

264c777

AntonAM force-pushed the kweb-ui branch from 62cfcd6 to 264c777 Compare May 17, 2024 12:29

ravicious reviewed May 17, 2024

View reviewed changes

ravicious self-requested a review May 17, 2024 13:57

ravicious reviewed May 20, 2024

View reviewed changes

AntonAM added 3 commits May 22, 2024 00:50

Remove unneeded commented code.

41aa624

Revert imports changes for ResourceActionButton.tsx

cd6e2fa

Add type annotation to DocumentKubeExecWrapper props

27fc01c

AntonAM force-pushed the kweb-ui branch from d01e652 to 06b82ad Compare May 28, 2024 16:10

wip: split flow of kube exec

ec3d5c2

AntonAM force-pushed the kweb-ui branch from 06b82ad to ec3d5c2 Compare May 29, 2024 22:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UI changes for kube exec functionality. #41466

Add UI changes for kube exec functionality. #41466

AntonAM commented May 13, 2024 •

edited

github-actions bot commented May 16, 2024

public-teleport-github-review-bot bot commented May 16, 2024

ravicious left a comment

ravicious May 17, 2024

ravicious May 17, 2024

AntonAM May 22, 2024

ravicious May 17, 2024

AntonAM May 22, 2024

ravicious May 17, 2024

ravicious May 17, 2024

ravicious left a comment

ravicious May 20, 2024

AntonAM May 22, 2024

ravicious May 22, 2024

ravicious May 20, 2024

ravicious May 20, 2024 •

edited

ravicious May 20, 2024

rosstimothy commented May 22, 2024

AntonAM commented May 22, 2024

rosstimothy commented May 22, 2024

dmsergeevN26 commented May 23, 2024

AntonAM commented May 28, 2024

	<Flex
	flexDirection="column"
	height="100%"
	width="100%"
	px="2"
	style={{ overflow: 'auto' }}
	>
	<StyledXterm ref={elementRef} />
	</Flex>

Add UI changes for kube exec functionality. #41466

Are you sure you want to change the base?

Add UI changes for kube exec functionality. #41466

Conversation

AntonAM commented May 13, 2024 • edited

github-actions bot commented May 16, 2024

public-teleport-github-review-bot bot commented May 16, 2024

ravicious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravicious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravicious May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosstimothy commented May 22, 2024

UX

Security

AntonAM commented May 22, 2024

rosstimothy commented May 22, 2024

dmsergeevN26 commented May 23, 2024

AntonAM commented May 28, 2024

AntonAM commented May 13, 2024 •

edited

ravicious May 20, 2024 •

edited