Skip to content

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

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 1 commit into from
Feb 27, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 25 additions & 0 deletions web/packages/teleterm/src/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,12 @@ import {
PtyEventStartError,
PtyServerEvent,
} from 'teleterm/sharedProcess/api/protogen/ptyHostService_pb';
import {
ReloginRequest,
SendNotificationRequest,
CannotProxyGatewayConnection,
GatewayCertExpired,
} from 'teleterm/services/tshdEvents';

export function resourceOneOfIsServer(
resource: PaginatedResource['resource']
Expand Down Expand Up @@ -107,6 +113,7 @@ export function ptyEventOneOfIsExit(
} {
return event.oneofKind === 'exit';
}

export function ptyEventOneOfIsStartError(
event: PtyClientEvent['event'] | PtyServerEvent['event']
): event is {
Expand All @@ -116,6 +123,24 @@ export function ptyEventOneOfIsStartError(
return event.oneofKind === 'startError';
}

export function notificationRequestOneOfIsCannotProxyGatewayConnection(
Copy link
Copy Markdown
Member

@ravicious ravicious Feb 27, 2024

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

@gzdunek gzdunek Feb 27, 2024

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

@ryanclark ryanclark Feb 27, 2024

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

subject: SendNotificationRequest['subject']
): subject is {
oneofKind: 'cannotProxyGatewayConnection';
cannotProxyGatewayConnection: CannotProxyGatewayConnection;
} {
return subject.oneofKind === 'cannotProxyGatewayConnection';
}

export function reloginReasonOneOfIsGatewayCertExpired(
reason: ReloginRequest['reason']
): reason is {
oneofKind: 'gatewayCertExpired';
gatewayCertExpired: GatewayCertExpired;
} {
return reason.oneofKind === 'gatewayCertExpired';
}

export function connectEventOneOfIsClusterLogin(
event: prehog.SubmitConnectEventRequest['event']
): event is {
Expand Down
6 changes: 1 addition & 5 deletions web/packages/teleterm/src/services/tshdEvents/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,22 +31,18 @@ import { filterSensitiveProperties } from 'teleterm/services/tshd/interceptors';

export interface ReloginRequest extends api.ReloginRequest {
rootClusterUri: uri.RootClusterUri;
gatewayCertExpired?: GatewayCertExpired;
}
export interface GatewayCertExpired extends api.GatewayCertExpired {
gatewayUri: uri.GatewayUri;
targetUri: uri.DatabaseUri;
}

export interface SendNotificationRequest extends api.SendNotificationRequest {
cannotProxyGatewayConnection?: CannotProxyGatewayConnection;
}
export type SendNotificationRequest = api.SendNotificationRequest;
export interface CannotProxyGatewayConnection
extends api.CannotProxyGatewayConnection {
gatewayUri: uri.GatewayUri;
targetUri: uri.DatabaseUri;
}

export type PromptMfaRequest = api.PromptMFARequest & {
rootClusterUri: uri.RootClusterUri;
};
Expand Down
7 changes: 4 additions & 3 deletions web/packages/teleterm/src/ui/services/relogin/reloginService.ts
View file
Open in desktop
Copy link
Copy Markdown
Member

@ravicious ravicious Feb 27, 2024

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

@gzdunek gzdunek Feb 27, 2024

Choose a reason for hiding this comment

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

Much better, thanks :)

Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
ClusterConnectReason,
} from 'teleterm/ui/services/modals';
import { ClustersService } from 'teleterm/ui/services/clusters';
import { reloginReasonOneOfIsGatewayCertExpired } from 'teleterm/helpers';

export class ReloginService {
constructor(
Expand All @@ -38,13 +39,13 @@ export class ReloginService {
this.mainProcessClient.forceFocusWindow();
let reason: ClusterConnectReason;

if (request.gatewayCertExpired) {
if (reloginReasonOneOfIsGatewayCertExpired(request.reason)) {
const gateway = this.clustersService.findGateway(
request.gatewayCertExpired.gatewayUri
request.reason.gatewayCertExpired.gatewayUri
);
reason = {
kind: 'reason.gateway-cert-expired',
targetUri: request.gatewayCertExpired.targetUri,
targetUri: request.reason.gatewayCertExpired.targetUri,
gateway: gateway,
};
}
Expand Down
7 changes: 5 additions & 2 deletions web/packages/teleterm/src/ui/services/tshdNotifications/tshdNotificationService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { SendNotificationRequest } from 'teleterm/services/tshdEvents';
import { ClustersService } from 'teleterm/ui/services/clusters';
import { NotificationsService } from 'teleterm/ui/services/notifications';
import { routing } from 'teleterm/ui/uri';
import { notificationRequestOneOfIsCannotProxyGatewayConnection } from 'teleterm/helpers';

export class TshdNotificationsService {
constructor(
Expand All @@ -29,9 +30,11 @@ export class TshdNotificationsService {
) {}

sendNotification(request: SendNotificationRequest) {
if (request.cannotProxyGatewayConnection) {
if (
notificationRequestOneOfIsCannotProxyGatewayConnection(request.subject)
) {
const { gatewayUri, targetUri, error } =
request.cannotProxyGatewayConnection;
request.subject.cannotProxyGatewayConnection;
const gateway = this.clustersService.findGateway(gatewayUri);
const clusterName = routing.parseClusterName(targetUri);
let targetName: string;
Expand Down