Skip to content

Fix a regression issue that caused the re-login dialog reason and proxy errors to not be displayed#38664

Merged
gzdunek merged 1 commit intomasterfrom
gzdunek/fix-tshd-events-service
Feb 27, 2024
Merged

Fix a regression issue that caused the re-login dialog reason and proxy errors to not be displayed#38664
gzdunek merged 1 commit intomasterfrom
gzdunek/fix-tshd-events-service

Conversation

@gzdunek
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek commented Feb 27, 2024

As Rafał noticed in #38202 (comment), we stopped showing notifications from the tshd events service. This happens since we migrated to @protobuf-ts. The property we were overriding in ReloginRequest was replaced by oneOf, so the overridden one became empty.

The same issue happened to the relogin requests, fortunately we only stopped showing the relogin reason, the dialog still worked (probably because of that we missed this bug).

Changelog: Fixed a regression with Teleport Connect not showing the re-login reason and connection errors when accessing databases, Kube clusters, and apps with an expired cert

@gzdunek gzdunek force-pushed the gzdunek/fix-tshd-events-service branch from 64ae0a5 to 3ee40c4 Compare February 27, 2024 10:03
return event.oneofKind === 'startError';
}

export function notificationRequestOneOfIsCannotProxyGatewayConnection(
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 wish protobuf-ts had some kind of a switch to tell it that strict null checks are off. AFAIK the only reason we need these helper methods is because the generated type looks like this:

    reason: {
        oneofKind: "gatewayCertExpired";
        gatewayCertExpired: GatewayCertExpired;
    } | {
        oneofKind: undefined;
    };

I know that there are some issues with strictNullChecks turned off and boolean fields (microsoft/TypeScript#10564). But I don't get why something like this doesn't narrow the type:

    if (request.reason.oneofKind === 'gatewayCertExpired') {
      const gateway = this.clustersService.findGateway(
        request.reason.gatewayCertExpired.gatewayUri
      );

Since request.reason.oneofKind is "gatewayCertExpired", it cannot be undefined.

The moment I remove the {oneofKind: undefined} variant, type narrowing seems to work.

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.

Yeah, I was wondering why the type narrowing doesn't work, I forgot about that undefined thing :/

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 wish protobuf-ts had some kind of a switch to tell it that strict null checks are off

Same, it's the only downside to the library

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.

Changelog: Fixed a regression issue that caused the re-login dialog reason and proxy errors to not be displayed for database connections in Teleport Connect

I believe this affects all types of connections, doesn't it?

Maybe something like:

Changelog: Fixed a regression with Teleport Connect not showing the re-login reason and connection errors when accessing databases, Kube clusters, and apps with an expired cert

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.

Much better, thanks :)

@gzdunek gzdunek enabled auto-merge February 27, 2024 16:14
@gzdunek gzdunek added this pull request to the merge queue Feb 27, 2024
Merged via the queue into master with commit f4eeb39 Feb 27, 2024
@gzdunek gzdunek deleted the gzdunek/fix-tshd-events-service branch February 27, 2024 16:26
@public-teleport-github-review-bot
Copy link
Copy Markdown

@gzdunek See the table below for backport results.

Branch Result
branch/v15 Create PR

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.

3 participants