Skip to content

Commit

Permalink
Improve error handling in Connect (#47328)
Browse files Browse the repository at this point in the history
* Preserve newlines and tabs in `Alert`

* Use Alert's details to show the status text of an error

* Make methods that show errors retryable

* Add horizontal padding in `DocumentCluster`

* Use Alert's details and primaryAction where appropriate

* Add a story for DocumentAccessRequests

* Add missing types

* Fix lint and tests

* Add license

* Remove cyclic import from `Alert.tsx`

* Add a comment for `white-space: pre-wrap`

* Clean up parsing cluster name, add missing action to retry starting a headless watcher
  • Loading branch information
gzdunek authored Oct 16, 2024
1 parent 1238300 commit d602f89
Show file tree
Hide file tree
Showing 19 changed files with 287 additions and 85 deletions.
38 changes: 23 additions & 15 deletions web/packages/design/src/Alert/Alert.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,14 @@ import React, { useState } from 'react';
import styled, { useTheme } from 'styled-components';
import { style, color, ColorProps } from 'styled-system';

import { space, SpaceProps, width, WidthProps } from 'design/system';
import { Theme } from 'design/theme/themes/types';
import * as Icon from 'design/Icon';
import Flex from 'design/Flex';
import { Text, Button, Box, ButtonIcon } from 'design';
import { IconProps } from 'design/Icon/Icon';
import { ButtonFill, ButtonIntent } from 'design/Button';
import { space, SpaceProps, width, WidthProps } from '../system';
import { Theme } from '../theme';
import * as Icon from '../Icon';
import Flex from '../Flex';
import Text from '../Text';
import Box from '../Box';
import { ButtonFill, ButtonIntent, Button } from '../Button';
import ButtonIcon from '../ButtonIcon';

const linkColor = style({
prop: 'linkColor',
Expand Down Expand Up @@ -112,7 +113,7 @@ interface Props<K> {
/** Additional description to be displayed below the main content. */
details?: React.ReactNode;
/** Overrides the icon specified by {@link AlertProps.kind}. */
icon?: React.ComponentType<IconProps>;
icon?: React.ComponentType<Icon.IconProps>;
/** If specified, causes the alert to display a primary action button. */
primaryAction?: Action;
/** If specified, causes the alert to display a secondary action button. */
Expand Down Expand Up @@ -190,7 +191,15 @@ export const Alert = ({
<IconContainer kind={kind}>
<AlertIcon kind={kind} customIcon={icon} size={alertIconSize} />
</IconContainer>
<Box flex="1">
<Box
flex="1"
css={`
// This preserves white spaces from Go errors (mainly in Teleport Connect).
// Thanks to it, each error line is nicely indented with tab,
// instead od being treated as a one, long line.
white-space: pre-wrap;
`}
>
<Text typography="h3">{children}</Text>
{details}
</Box>
Expand All @@ -217,10 +226,9 @@ const OuterContainer = styled.div<AlertProps>`
${space}
${width}
${alertBorder}
${color}
a {
${alertBorder}
${color}
a {
color: ${({ theme }) => theme.colors.light};
${linkColor}
}
Expand All @@ -243,8 +251,8 @@ const AlertIcon = ({
...otherProps
}: {
kind: AlertKind | BannerKind;
customIcon: React.ComponentType<IconProps>;
} & IconProps) => {
customIcon: React.ComponentType<Icon.IconProps>;
} & Icon.IconProps) => {
const commonProps = { role: 'graphics-symbol', ...otherProps };
if (CustomIcon) {
return <CustomIcon {...commonProps} />;
Expand Down
2 changes: 1 addition & 1 deletion web/packages/design/src/Icon/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ THIS FILE IS GENERATED. DO NOT EDIT.
*/

export { Icon } from './Icon';
export { Icon, type IconProps } from './Icon';

export { Add } from './Icons/Add';
export { AddCircle } from './Icons/AddCircle';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,9 @@ export function ClusterAdd(props: {
</DialogHeader>
<DialogContent mb={2}>
{status === 'error' && (
<Alerts.Danger mb={5} children={statusText} />
<Alerts.Danger mb={5} details={statusText}>
Could not add the cluster
</Alerts.Danger>
)}
<FieldInput
rule={requiredField('Cluster address is required')}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ import * as Alerts from 'design/Alert';
import { StepSlider } from 'design/StepSlider';
import { Attempt } from 'shared/hooks/useAsync';

import { P } from 'design/Text/Text';

import * as types from 'teleterm/ui/services/clusters/types';

import { PromptWebauthn } from './PromptWebauthn';
Expand Down Expand Up @@ -63,8 +61,8 @@ export default function LoginForm(props: Props) {
return (
<FlexBordered p={4} pb={5}>
{loginAttempt.status === 'error' && (
<Alerts.Danger m={5} mb={0}>
{loginAttempt.statusText}
<Alerts.Danger m={5} mb={0} details={loginAttempt.statusText}>
Could not log in
</Alerts.Danger>
)}
<FormSso {...props} />
Expand All @@ -75,11 +73,9 @@ export default function LoginForm(props: Props) {
if (!localAuthEnabled) {
return (
<FlexBordered p={4}>
<Alerts.Danger>Login has not been enabled</Alerts.Danger>
<P>
The ability to login has not been enabled. Please contact your system
administrator for more information.
</P>
<Alerts.Danger details="The ability to login has not been enabled. Please contact your system administrator for more information.">
Login has not been enabled
</Alerts.Danger>
</FlexBordered>
);
}
Expand All @@ -88,8 +84,8 @@ export default function LoginForm(props: Props) {
return (
<FlexBordered>
{loginAttempt.status === 'error' && (
<Alerts.Danger m={4} mb={0}>
{loginAttempt.statusText}
<Alerts.Danger m={4} mb={0} details={loginAttempt.statusText}>
Could not log in
</Alerts.Danger>
)}
<StepSlider<typeof loginViews>
Expand Down
6 changes: 5 additions & 1 deletion web/packages/teleterm/src/ui/ClusterLogout/ClusterLogout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,11 @@ export function ClusterLogout({
</DialogHeader>
<DialogContent mb={4}>
<P color="text.slightlyMuted">Are you sure you want to log out?</P>
{status === 'error' && <Alerts.Danger mb={5} children={statusText} />}
{status === 'error' && (
<Alerts.Danger mb={5} details={statusText}>
Could not log out
</Alerts.Danger>
)}
</DialogContent>
<DialogFooter>
<ButtonWarning
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -467,16 +467,7 @@ function StandardError(props: {
error: string;
mb?: number | string;
}): JSX.Element {
return (
<Alerts.Danger
mb={props.mb || 0}
css={`
white-space: pre-wrap;
`}
>
{props.error}
</Alerts.Danger>
);
return <Alerts.Danger mb={props.mb || 0}>{props.error}</Alerts.Danger>;
}

function ClusterAndHostnameCopy(props: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import {
ButtonPrimary,
Flex,
Label,
Link,
MenuItem,
Text,
ButtonSecondary,
Expand Down Expand Up @@ -162,22 +161,13 @@ export function Status(props: { closeDocument?: () => void }) {
)}
{isAgentConfiguredAttempt.status === 'error' && (
<Alert
css={`
display: block;
`}
primaryAction={{
content: 'Run setup again',
onClick: replaceWithSetup,
}}
details={isAgentConfiguredAttempt.statusText}
>
An error occurred while reading the agent config file:{' '}
{isAgentConfiguredAttempt.statusText}. <br />
You can try to{' '}
<Link
onClick={replaceWithSetup}
css={`
cursor: pointer;
`}
>
run the setup
</Link>{' '}
again.
An error occurred while reading the agent config file
</Alert>
)}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
/**
* Teleport
* Copyright (C) 2024 Gravitational, Inc.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import { AccessRequest } from 'gen-proto-ts/teleport/lib/teleterm/v1/access_request_pb';

import AppContextProvider from 'teleterm/ui/appContextProvider';
import { MockWorkspaceContextProvider } from 'teleterm/ui/fixtures/MockWorkspaceContextProvider';
import { ResourcesContextProvider } from 'teleterm/ui/DocumentCluster/resourcesContext';
import { DocumentAccessRequests } from 'teleterm/ui/DocumentAccessRequests/DocumentAccessRequests';
import * as types from 'teleterm/ui/services/workspacesService';
import { makeRootCluster } from 'teleterm/services/tshd/testHelpers';
import { MockAppContext } from 'teleterm/ui/fixtures/mocks';
import { getEmptyPendingAccessRequest } from 'teleterm/ui/services/workspacesService/accessRequestsService';
import { MockedUnaryCall } from 'teleterm/services/tshd/cloneableClient';

const rootCluster = makeRootCluster();

export default {
title: 'Teleterm/DocumentAccessRequests',
};

const doc: types.DocumentAccessRequests = {
kind: 'doc.access_requests',
uri: '/docs/123',
state: 'browsing',
clusterUri: rootCluster.uri,
requestId: '',
title: 'Access Requests',
};

const mockedAccessRequest: AccessRequest = {
id: '01929070-6886-77eb-90aa-c7223dd73f67',
state: 'PENDING',
resolveReason: '',
requestReason: '',
user: 'requester',
roles: ['access', 'searcheable-resources'],
reviews: [],
suggestedReviewers: ['admin', 'reviewer'],
thresholdNames: ['default'],
resourceIds: [
{
kind: 'kube_cluster',
name: 'minikube',
clusterName: 'main',
subResourceName: '',
},
],
resources: [
{
id: {
kind: 'kube_cluster',
name: 'minikube',
clusterName: 'main',
subResourceName: '',
},
details: { hostname: '', friendlyName: '' },
},
],
promotedAccessListTitle: '',
created: { seconds: 1729000138n, nanos: 886521000 },
expires: { seconds: 1729026573n, nanos: 0 },
maxDuration: { seconds: 1729026573n, nanos: 0 },
requestTtl: { seconds: 1729026573n, nanos: 0 },
sessionTtl: { seconds: 1729026573n, nanos: 0 },
};

export function Loaded() {
const appContext = new MockAppContext();
appContext.tshd.getAccessRequests = () =>
new MockedUnaryCall({
totalCount: 0,
startKey: '',
requests: [mockedAccessRequest],
});
appContext.workspacesService.setState(draftState => {
draftState.rootClusterUri = rootCluster.uri;
draftState.workspaces[rootCluster.uri] = {
localClusterUri: doc.clusterUri,
documents: [doc],
location: doc.uri,
accessRequests: {
pending: getEmptyPendingAccessRequest(),
isBarCollapsed: true,
},
};
});

return (
<AppContextProvider value={appContext}>
<MockWorkspaceContextProvider>
<ResourcesContextProvider>
<DocumentAccessRequests doc={doc} visible={true} />
</ResourcesContextProvider>
</MockWorkspaceContextProvider>
</AppContextProvider>
);
}

export function ErrorLoading() {
const appContext = new MockAppContext();
appContext.tshd.getAccessRequests = () =>
new MockedUnaryCall(null, new Error('network error'));
appContext.workspacesService.setState(draftState => {
draftState.rootClusterUri = rootCluster.uri;
draftState.workspaces[rootCluster.uri] = {
localClusterUri: doc.clusterUri,
documents: [doc],
location: doc.uri,
accessRequests: {
pending: getEmptyPendingAccessRequest(),
isBarCollapsed: true,
},
};
});
appContext.clustersService.setState(draftState => {
draftState.clusters.set(rootCluster.uri, rootCluster);
});

return (
<AppContextProvider value={appContext}>
<MockWorkspaceContextProvider>
<ResourcesContextProvider>
<DocumentAccessRequests doc={doc} visible={true} />
</ResourcesContextProvider>
</MockWorkspaceContextProvider>
</AppContextProvider>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ function Inner(props: { rootCluster: Cluster }) {
updateSearch,
selectedResource,
customSort,
fetch,
fetchStatus,
onAgentLabelClick,
addedResources,
Expand Down Expand Up @@ -119,7 +120,13 @@ function Inner(props: { rootCluster: Cluster }) {
return (
<Layout mx="auto" px={5} pt={3} height="100%">
{attempt.status === 'failed' && (
<Alert kind="danger" children={attempt.statusText} />
<Alert
kind="danger"
details={attempt.statusText}
primaryAction={{ content: 'Retry', onClick: fetch }}
>
Could not load resources
</Alert>
)}
<StyledMain>
<Flex mt={3} mb={3}>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,7 @@ export default function useNewRequest(rootCluster: Cluster) {
agents: fetchedData.agents,
agentFilter,
updateSort,
fetch,
attempt,
fetchStatus,
updateQuery,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,14 @@ export function RequestList({
return (
<Layout mx="auto" px={5} pt={3} height="100%">
{attempt.status === 'failed' && (
<Alert kind="danger" children={attempt.statusText} />
<Alert kind="danger" details={attempt.statusText}>
Could not fetch access requests
</Alert>
)}
{assumeRoleAttempt.status === 'error' && (
<Alert kind="danger" children={assumeRoleAttempt.statusText} />
<Alert kind="danger" details={assumeRoleAttempt.statusText}>
Could not assume the role
</Alert>
)}
<Flex justifyContent="end" pb={4}>
<ButtonPrimary
Expand Down
Loading

0 comments on commit d602f89

Please sign in to comment.