Skip to content

Prevent crashing Teleport Connect after downgrading from v15#36730

Merged
gzdunek merged 4 commits intobranch/v14from
gzdunek/gracefully-downgrade-app/v14
Jan 17, 2024
Merged

Prevent crashing Teleport Connect after downgrading from v15#36730
gzdunek merged 4 commits intobranch/v14from
gzdunek/gracefully-downgrade-app/v14

Conversation

@gzdunek
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek commented Jan 16, 2024

This PR handles app connections and documents created in Connect v15.

This is how it looks when an app connection is opened in v14:
image

Ideally, we would like to parse the app state in a tool like zod when the app launches, but it is too time consuming task for now.
I will also backport this to v13.

Important: This will only work when downgrading from v15 to whatever the next v14 and v13 will be. If someone is on 14.0.0 and they upgrade then downgrade back to 14.0.0, they'll not benefit from it.

Changelog: Fixed a potential crash in Teleport Connect after downgrading the app from v15+


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

@ravicious ravicious Jan 16, 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:

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

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

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

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

Choose a reason for hiding this comment

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

Makes sense, done.

Comment thread web/packages/teleterm/src/ui/Documents/DocumentsRenderer.tsx
Comment thread web/packages/teleterm/src/ui/Documents/DocumentsRenderer.tsx
@gzdunek gzdunek requested a review from ravicious January 17, 2024 09:37
@gzdunek
Copy link
Copy Markdown
Contributor Author

gzdunek commented Jan 17, 2024

Oops, prettier@2.5.0 doesn't support satisfies. I will have to upgrade it first.

@gzdunek gzdunek added this pull request to the merge queue Jan 17, 2024
Merged via the queue into branch/v14 with commit 368ad2b Jan 17, 2024
@gzdunek gzdunek deleted the gzdunek/gracefully-downgrade-app/v14 branch January 17, 2024 10:42
gzdunek added a commit that referenced this pull request Jan 17, 2024
* Backport changes from #36393

* Show UNKNOWN for app connections

* Use `connection satisfies never`

* Remove `connection satisfies never` check
github-merge-queue Bot pushed a commit that referenced this pull request Jan 17, 2024
…36798)

* Prevent crashing Teleport Connect after downgrading from v15 (#36730)

* Backport changes from #36393

* Show UNKNOWN for app connections

* Use `connection satisfies never`

* Remove `connection satisfies never` check

* Remove "kube" from supported gateway targets
@fheinecke fheinecke mentioned this pull request Aug 6, 2024
@fheinecke fheinecke mentioned this pull request Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants