Skip to content

Prevent crashing Teleport Connect after downgrading from v15 #36730

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
gzdunek merged 4 commits into from
Jan 17, 2024
Merged

Prevent crashing Teleport Connect after downgrading from v15 #36730

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b2a7e99
Backport changes from #36393
gzdunek Jan 16, 2024
280071d
Show UNKNOWN for app connections
gzdunek Jan 16, 2024
ede82d5
Use `connection satisfies never`
gzdunek Jan 17, 2024
8a02ae8
Remove `connection satisfies never` check
gzdunek Jan 17, 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
24 changes: 20 additions & 4 deletions web/packages/teleterm/src/ui/Documents/DocumentsRenderer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import React, { useMemo } from 'react';
import { createPortal } from 'react-dom';

import styled from 'styled-components';
import { Text } from 'design';
/* eslint-disable @typescript-eslint/ban-ts-comment*/
// @ts-ignore
import { DocumentAccessRequests } from 'e-teleterm/ui/DocumentAccessRequests/DocumentAccessRequests';
Expand All @@ -41,7 +42,7 @@ import {
import { DocumentGatewayKube } from 'teleterm/ui/DocumentGatewayKube';

import Document from 'teleterm/ui/Document';
import { RootClusterUri } from 'teleterm/ui/uri';
import { RootClusterUri, isDatabaseUri } from 'teleterm/ui/uri';

import { ResourcesContextProvider } from '../DocumentCluster/resourcesContext';

Expand Down Expand Up @@ -121,8 +122,21 @@ function MemoizedDocument(props: { doc: types.Document; visible: boolean }) {
switch (doc.kind) {
case 'doc.cluster':
return <DocumentCluster doc={doc} visible={visible} />;
case 'doc.gateway':
return <DocumentGateway doc={doc} visible={visible} />;
case 'doc.gateway': {
if (isDatabaseUri(doc.targetUri)) {
return <DocumentGateway doc={doc} visible={visible} />;
}
return (
<Document visible={visible}>
<Text m="auto" mt={10} textAlign="center">
Cannot create a gateway for the target "{doc.targetUri}".
<br />
Only database and kube targets are supported.
</Text>
</Document>
);
}

case 'doc.gateway_cli_client':
return <DocumentGatewayCliClient doc={doc} visible={visible} />;
case 'doc.gateway_kube':
Expand All @@ -140,7 +154,9 @@ function MemoizedDocument(props: { doc: types.Document; visible: boolean }) {
default:
return (
<Document visible={visible}>
Document kind "{doc.kind}" is not supported
<Text m="auto" mt={10} textAlign="center">
Document kind "{doc.kind}" is not supported.
</Text>
</Document>
);
}
Expand Down
21 changes: 15 additions & 6 deletions web/packages/teleterm/src/ui/TopBar/Connections/ConnectionsFilterableList/ConnectionItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { Trash, Unlink } from 'design/Icon';

import { ExtendedTrackedConnection } from 'teleterm/ui/services/connectionTracker';
import { ListItem } from 'teleterm/ui/components/ListItem';
import { assertUnreachable } from 'teleterm/ui/utils';
import { isDatabaseUri } from 'teleterm/ui/uri';

import { useKeyboardArrowsNavigation } from 'teleterm/ui/components/KeyboardArrowsNavigation';

Expand Down Expand Up @@ -105,7 +105,7 @@ export function ConnectionItem(props: ConnectionItemProps) {
border-radius: 4px;
`}
>
{getKindName(props.item.kind)}
{getKindName(props.item)}
</span>
<span
css={`
Expand Down Expand Up @@ -138,15 +138,24 @@ export function ConnectionItem(props: ConnectionItemProps) {
);
}

function getKindName(kind: ExtendedTrackedConnection['kind']): string {
switch (kind) {
function getKindName(connection: ExtendedTrackedConnection): string {
Copy link
Copy Markdown
Member

@ravicious ravicious Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

With a few modifications, we can gracefully handle downgrades without compromising type safety using foo satisfies never:

function getKindName(connection: ExtendedTrackedConnection): string {
  switch (connection.kind) {
    case 'connection.gateway':
      if (isDatabaseUri(connection.targetUri)) {
        return 'DB';
      }
      return 'UNKNOWN';
    case 'connection.server':
      return 'SSH';
    case 'connection.kube':
      return 'KUBE';
    default:
      // TODO: Explain why and when the default branch might get triggered.
      connection satisfies never
      return 'UNKNOWN';
  }
}

https://stackoverflow.com/a/75217377

Going forward, in places that should gracefully degrade rather than throw an error, we should probably use this approach over assertUnreachable.

But there are multiple ways to approach this of course. If we were parsing input data with zod at the point where it enters the system, we could ensure type safety all the way and just throw an error if there's no matching case.

However, this would mean that the app would need to reject invalid documents and connections from the state, meaning that the user would probably see some kind of a warning notification on app startup but that would be it. The advantage of the current approach is that we're not losing any data and if the user upgrades the app again, they'll see the existing documents for app access.

Copy link
Copy Markdown
Contributor Author

@gzdunek gzdunek Jan 17, 2024

Choose a reason for hiding this comment

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

With a few modifications, we can gracefully handle downgrades without compromising type safety using foo satisfies never:

Wow, this is cool.
I had to assign the result to a value and then add eslint-disable-next-line @typescript-eslint/no-unused-vars. Otherwise I got errors from linter that expression statement is not assignment or call.

The advantage of the current approach is that we're not losing any data and if the user upgrades the app again, they'll see the existing documents for app access.

Right, I forgot that for app_state.json we overwrite the entire file.

Copy link
Copy Markdown
Member

@ravicious ravicious Jan 17, 2024

Choose a reason for hiding this comment

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

I had to assign the result to a value and then add eslint-disable-next-line @typescript-eslint/no-unused-vars. Otherwise I got errors from linter that expression statement is not assignment or call.

This is not the case for me, yarn eslint does not complain about this.

Copy link
Copy Markdown
Member

@ravicious ravicious Jan 17, 2024

Choose a reason for hiding this comment

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

I'd say don't bother with upgrading prettier and just use your solution for now. I didn't see that prettier doesn't handle it because my editor plugin seems to just fail silently if there's a prettier error.

If we decide to upgrade prettier, we should do this across all supported branches etc., and at the moment there's other work that needs our attention.

Copy link
Copy Markdown
Contributor Author

@gzdunek gzdunek Jan 17, 2024

Choose a reason for hiding this comment

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

Makes sense, done.

switch (connection.kind) {
case 'connection.gateway':
return 'DB';
if (isDatabaseUri(connection.targetUri)) {
return 'DB';
}
return 'UNKNOWN';
case 'connection.server':
return 'SSH';
case 'connection.kube':
return 'KUBE';
default:
assertUnreachable(kind);
// The default branch is triggered when the state read from the disk
// contains a connection not supported by the given Connect version.
//
// For example, the user can open an app connection in Connect v15
// and then downgrade to a version that doesn't support apps.
// That connection should be shown as 'UNKNOWN' in the connection list.
return 'UNKNOWN';
}
}
2 changes: 1 addition & 1 deletion ...packages/teleterm/src/ui/services/connectionTracker/trackedConnectionOperationsFactory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ export class TrackedConnectionOperationsFactory {
private getConnectionGatewayOperations(
connection: TrackedGatewayConnection
): TrackedConnectionOperations {
const { rootClusterId, leafClusterId } = routing.parseDbUri(
const { rootClusterId, leafClusterId } = routing.parseClusterUri(
connection.targetUri
).params;
const { rootClusterUri, leafClusterUri } = this.getClusterUris({
Expand Down
2 changes: 1 addition & 1 deletion web/packages/teleterm/src/ui/services/workspacesService/documentsService/documentsService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ export class DocumentsService {
origin,
} = opts;
const uri = routing.getDocUri({ docId: unique() });
const title = `${targetUser}@${targetName}`;
const title = targetUser ? `${targetUser}@${targetName}` : targetName;

return {
uri,
Expand Down
12 changes: 12 additions & 0 deletions web/packages/teleterm/src/ui/uri.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,18 @@ export const routing = {
},
};

export function isDatabaseUri(uri: string): uri is DatabaseUri {
return !!routing.parseDbUri(uri);
}

export function isServerUri(uri: string): uri is ServerUri {
return !!routing.parseServerUri(uri);
}

export function isKubeUri(uri: string): uri is KubeUri {
return !!routing.parseKubeUri(uri);
}

export type Params = {
rootClusterId?: string;
leafClusterId?: string;
Expand Down