Skip to content

WebDiscover: allow setting labels when enrolling aws app #50976

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
kimlisa merged 2 commits into from
Jan 14, 2025
Merged

WebDiscover: allow setting labels when enrolling aws app #50976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5485a1f
WebDiscover: allow setting labels when enrolling aws app
kimlisa Jan 13, 2025
42721df
Address CRs
kimlisa Jan 13, 2025
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
9 changes: 6 additions & 3 deletions ...kages/teleport/src/Discover/AwsMangementConsole/CreateAppAccess/CreateAppAccess.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export const Success = () => <Component />;
Success.parameters = {
msw: {
handlers: [
http.post(cfg.api.awsAppAccessPath, () =>
http.post(cfg.api.awsAppAccess.createV2, () =>
HttpResponse.json({ name: 'app-1' })
),
],
Expand All @@ -59,7 +59,10 @@ export const Loading = () => {
Loading.parameters = {
msw: {
handlers: [
http.post(cfg.api.awsAppAccessPath, async () => await delay('infinite')),
http.post(
cfg.api.awsAppAccess.createV2,
async () => await delay('infinite')
),
],
},
};
Expand All @@ -68,7 +71,7 @@ export const Failed = () => <Component />;
Failed.parameters = {
msw: {
handlers: [
http.post(cfg.api.awsAppAccessPath, () =>
http.post(cfg.api.awsAppAccess.createV2, () =>
HttpResponse.json(
{
message: 'Some kind of error message',
Expand Down
22 changes: 21 additions & 1 deletion ...ckages/teleport/src/Discover/AwsMangementConsole/CreateAppAccess/CreateAppAccess.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,13 @@ import {
DiscoverEventResource,
userEventService,
} from 'teleport/services/userEvent';
import { ProxyRequiresUpgrade } from 'teleport/services/version/unsupported';
import TeleportContext from 'teleport/teleportContext';

import { CreateAppAccess } from './CreateAppAccess';

beforeEach(() => {
jest.spyOn(integrationService, 'createAwsAppAccess').mockResolvedValue(app);
jest.spyOn(integrationService, 'createAwsAppAccessV2').mockResolvedValue(app);
jest
.spyOn(userEventService, 'captureDiscoverEvent')
.mockResolvedValue(undefined as never);
Expand All @@ -60,6 +61,25 @@ test('create app access', async () => {
renderCreateAppAccess(ctx, discoverCtx);
await screen.findByText(/bash/i);

await userEvent.click(screen.getByRole('button', { name: /next/i }));
await screen.findByText(/aws-console/i);
expect(integrationService.createAwsAppAccessV2).toHaveBeenCalledTimes(1);
});

test('create app access with v1 endpoint', async () => {
jest
.spyOn(integrationService, 'createAwsAppAccessV2')
.mockRejectedValueOnce(new Error(ProxyRequiresUpgrade));
jest.spyOn(integrationService, 'createAwsAppAccess').mockResolvedValue(app);

const { ctx, discoverCtx } = getMockedContexts();

renderCreateAppAccess(ctx, discoverCtx);
await screen.findByText(/bash/i);

await userEvent.click(screen.getByRole('button', { name: /next/i }));
await screen.findByText(ProxyRequiresUpgrade);

await userEvent.click(screen.getByRole('button', { name: /next/i }));
await screen.findByText(/aws-console/i);
expect(integrationService.createAwsAppAccess).toHaveBeenCalledTimes(1);
Expand Down
166 changes: 107 additions & 59 deletions web/packages/teleport/src/Discover/AwsMangementConsole/CreateAppAccess/CreateAppAccess.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,29 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import { useState } from 'react';
import styled from 'styled-components';

import { Box, Flex, H3, Link, Mark } from 'design';
import { Danger } from 'design/Alert';
import { P } from 'design/Text/Text';
import { P, Subtitle3 } from 'design/Text/Text';
import { IconTooltip } from 'design/Tooltip';
import TextEditor from 'shared/components/TextEditor';
import Validation, { Validator } from 'shared/components/Validation';
import { useAsync } from 'shared/hooks/useAsync';

import { TextSelectCopyMulti } from 'teleport/components/TextSelectCopy';
import cfg from 'teleport/config';
import { Container } from 'teleport/Discover/Shared/CommandBox';
import { ResourceLabelTooltip } from 'teleport/Discover/Shared/ResourceLabelTooltip';
import { useDiscover } from 'teleport/Discover/useDiscover';
import { ResourceLabel } from 'teleport/services/agents';
import { App } from 'teleport/services/apps/types';
import { integrationService } from 'teleport/services/integrations';
import { splitAwsIamArn } from 'teleport/services/integrations/aws';
import { ProxyRequiresUpgrade } from 'teleport/services/version/unsupported';

import { ActionButtons, Header } from '../../Shared';
import { ActionButtons, Header, LabelsCreater } from '../../Shared';
import { AppCreatedDialog } from './AppCreatedDialog';

const IAM_POLICY_NAME = 'AWSAppAccess';
Expand All @@ -41,23 +47,45 @@ export function CreateAppAccess() {
const { agentMeta, updateAgentMeta, emitErrorEvent, nextStep } =
useDiscover();
const { awsIntegration } = agentMeta;
const [labels, setLabels] = useState<ResourceLabel[]>([]);

// TODO(kimlisa): DELETE IN 19.0
const [requiresProxyUpgrade, setRequiresProxyUpgrade] = useState(false);

const [attempt, createApp] = useAsync(async () => {
const labelsMap: Record<string, string> = {};
labels.forEach(l => (labelsMap[l.name] = l.value));
try {
const app = await integrationService.createAwsAppAccess(
awsIntegration.name
);
let app: App;
if (requiresProxyUpgrade && !labels.length) {
app = await integrationService.createAwsAppAccess(awsIntegration.name);
} else {
app = await integrationService.createAwsAppAccessV2(
awsIntegration.name,
{ labels: labelsMap }
);
}
updateAgentMeta({
...agentMeta,
app,
resourceName: app.name,
});
} catch (err) {
if (err.message.includes(ProxyRequiresUpgrade)) {
setRequiresProxyUpgrade(true);
}
emitErrorEvent(err.message);
throw err;
}
});
Comment on lines +53 to 80
Copy link
Copy Markdown
Contributor

@flyinghermit flyinghermit Jan 13, 2025

Choose a reason for hiding this comment

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

Make V2 the first API call the default one here as well? And looks like we can remove the const [requiresProxyUpgrade, setRequiresProxyUpgrade] = useState(false); state.

const [attempt, createApp] = useAsync(async () => {
    const labelsMap: Record<string, string> = {};
    labels.forEach(l => (labelsMap[l.name] = l.value));
    let app: App;
    try {
      app = await integrationService.createAwsAppAccessV2(
        awsIntegration.name,
        { labels: labelsMap }
      );
      }
    } catch (err) {

      if (err.message.includes(ProxyRequiresUpgrade)) {
      try {
        // retry to V1 endpoint
        app = await integrationService.createAwsAppAccess(awsIntegration.name);
      } catch (e) {
        emitErrorEvent(err.message);
        throw err;
      }
      emitErrorEvent(err.message);
      throw err;
    }
    updateAgentMeta({
      ...agentMeta,
      app,
      resourceName: app.name,
    });
  });

we can abstract v1 async call so catch(err) block is a bit simpler.

Copy link
Copy Markdown
Contributor Author

@kimlisa kimlisa Jan 13, 2025

Choose a reason for hiding this comment

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

hrmmm, wouldn't this re-try for the user? i think it's better if user intentionally retries (by removing labels, which means they understand labels aren't going to be added) or abort altogether

Copy link
Copy Markdown
Contributor

@flyinghermit flyinghermit Jan 13, 2025

Choose a reason for hiding this comment

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

You are right this auto retries. Also letting user manually do that is a good idea.
I didn't see any label/copy updated for user based on requiresProxyUpgrade so recommended that alternative.
How would the retry UX look like?

Copy link
Copy Markdown
Contributor Author

@kimlisa kimlisa Jan 14, 2025

Choose a reason for hiding this comment

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

here is a recording of retry (the error message is there after the first attempt that seemed to have cut off from recording):

Screen.Recording.2025-01-13.at.10.09.52.PM.mov

I didn't see any label/copy updated for user based on requiresProxyUpgrade so recommended that alternative.

unless you feel strongly, i will leave it as is. i think it makes things too complicated

Copy link
Copy Markdown
Contributor

@flyinghermit flyinghermit Jan 14, 2025

Choose a reason for hiding this comment

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

Gotcha. That looks good to me. The only extra thing I could suggest is to mark the labels fields as error so it becomes more apparent but i wouldn't stress that if it begs more changes. Its good as is too.


function onCreateApp(validator: Validator) {
if (!validator.validate()) {
return;
}
createApp();
}

const { awsAccountId: accountID, arnResourceName: iamRoleName } =
splitAwsIamArn(agentMeta.awsIntegration.spec.roleArn);
const scriptUrl = cfg.getAwsIamConfigureScriptAppAccessUrl({
Expand All @@ -66,62 +94,82 @@ export function CreateAppAccess() {
});

return (
<Box maxWidth="800px">
<Header>Enable Access to AWS with Teleport Application Access</Header>
<P mt={1} mb={3}>
An application will be created that will use the selected AWS OIDC
Integration <Mark>{agentMeta.awsIntegration.name}</Mark> for proxying
access to AWS Management Console, AWS CLI, and AWS APIs.
</P>
{attempt.status === 'error' && (
<Danger mt={3}>{attempt.statusText}</Danger>
)}
<Container>
<Flex alignItems="center" gap={1} mb={1}>
<H3>First configure your AWS IAM permissions</H3>
<IconTooltip sticky={true} maxWidth={450}>
The following IAM permissions will be added as an inline policy
named <Mark>{IAM_POLICY_NAME}</Mark> to IAM role{' '}
<Mark>{iamRoleName}</Mark>
<Box mb={2}>
<EditorWrapper $height={250}>
<TextEditor
readOnly={true}
data={[{ content: inlinePolicyJson, type: 'json' }]}
bg="levels.deep"
/>
</EditorWrapper>
</Box>
</IconTooltip>
</Flex>
<P mb={2}>
Run the command below on your{' '}
<Link
href="https://console.aws.amazon.com/cloudshell/home"
target="_blank"
>
AWS CloudShell
</Link>{' '}
to configure your IAM permissions.
</P>
<TextSelectCopyMulti
lines={[{ text: `bash -c "$(curl '${scriptUrl}')"` }]}
/>
</Container>
<Validation>
{({ validator }) => (
<Box maxWidth="800px">
<Header>Enable Access to AWS with Teleport Application Access</Header>
<P mt={1} mb={3}>
An application will be created that will use the selected AWS OIDC
Integration <Mark>{agentMeta.awsIntegration.name}</Mark> for
proxying access to AWS Management Console, AWS CLI, and AWS APIs.
</P>
{attempt.status === 'error' && (
<Danger mt={3}>{attempt.statusText}</Danger>
)}
<Container>
<H3>Step 1</H3>
<Flex alignItems="center" gap={1} mb={1}>
<Subtitle3>Configure your AWS IAM permissions</Subtitle3>
<IconTooltip sticky={true} maxWidth={450}>
The following IAM permissions will be added as an inline policy
named <Mark>{IAM_POLICY_NAME}</Mark> to IAM role{' '}
<Mark>{iamRoleName}</Mark>
<Box mb={2}>
<EditorWrapper $height={250}>
<TextEditor
readOnly={true}
data={[{ content: inlinePolicyJson, type: 'json' }]}
bg="levels.deep"
/>
</EditorWrapper>
</Box>
</IconTooltip>
</Flex>
<P mb={2}>
Run the command below on your{' '}
<Link
href="https://console.aws.amazon.com/cloudshell/home"
target="_blank"
>
AWS CloudShell
</Link>{' '}
to configure your IAM permissions.
</P>
<TextSelectCopyMulti
lines={[{ text: `bash -c "$(curl '${scriptUrl}')"` }]}
/>
</Container>

<Container mt={4}>
<H3>Step 2 (Optional)</H3>
<Flex alignItems="center" gap={1} mb={2}>
<Subtitle3>Add Labels</Subtitle3>
<ResourceLabelTooltip resourceKind="app" />
</Flex>
<LabelsCreater
labels={labels}
setLabels={setLabels}
isLabelOptional={true}
disableBtns={attempt.status === 'processing'}
noDuplicateKey={true}
/>
</Container>

<ActionButtons
onProceed={createApp}
disableProceed={
attempt.status === 'processing' || attempt.status === 'success'
}
/>
{attempt.status === 'success' && (
<AppCreatedDialog
toNextStep={nextStep}
appName={agentMeta.resourceName}
/>
<ActionButtons
onProceed={() => onCreateApp(validator)}
disableProceed={
attempt.status === 'processing' || attempt.status === 'success'
}
/>
{attempt.status === 'success' && (
<AppCreatedDialog
toNextStep={nextStep}
appName={agentMeta.resourceName}
/>
)}
</Box>
)}
</Box>
</Validation>
);
}

Expand Down
1 change: 0 additions & 1 deletion web/packages/teleport/src/Discover/Shared/LabelsCreater/LabelsCreater.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,6 @@ export function LabelsCreater({
label={labels.length === 0 ? 'Add a Label' : 'Add Another Label'}
onClick={addLabel}
disabled={disableBtns}
iconSize="small"
/>
</>
);
Expand Down
19 changes: 16 additions & 3 deletions web/packages/teleport/src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,8 +361,12 @@ const cfg = {
awsSubnetListPath:
'/v1/webapi/sites/:clusterId/integrations/aws-oidc/:name/subnets',

awsAppAccessPath:
'/v1/webapi/sites/:clusterId/integrations/aws-oidc/:name/aws-app-access',
awsAppAccess: {
create:
'/v1/webapi/sites/:clusterId/integrations/aws-oidc/:name/aws-app-access',
createV2:
'/v2/webapi/sites/:clusterId/integrations/aws-oidc/:name/aws-app-access',
},
awsConfigureIamAppAccessPath:
'/v1/webapi/scripts/integrations/configure/aws-app-access-iam.sh?role=:iamRoleName&awsAccountID=:accountID',

Expand Down Expand Up @@ -1057,7 +1061,16 @@ const cfg = {
getAwsAppAccessUrl(integrationName: string) {
const clusterId = cfg.proxyCluster;

return generatePath(cfg.api.awsAppAccessPath, {
return generatePath(cfg.api.awsAppAccess.create, {
clusterId,
name: integrationName,
});
},

getAwsAppAccessUrlV2(integrationName: string) {
const clusterId = cfg.proxyCluster;

return generatePath(cfg.api.awsAppAccess.createV2, {
clusterId,
name: integrationName,
});
Expand Down
16 changes: 16 additions & 0 deletions web/packages/teleport/src/services/integrations/integrations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import {
AwsOidcPingRequest,
AwsOidcPingResponse,
AwsRdsDatabase,
CreateAwsAppAccessRequest,
DeployEc2InstanceConnectEndpointRequest,
DeployEc2InstanceConnectEndpointResponse,
Ec2InstanceConnectEndpoint,
Expand Down Expand Up @@ -292,6 +293,21 @@ export const integrationService = {
.then(resp => resp.serviceDashboardUrl);
},

async createAwsAppAccessV2(
integrationName,
req: CreateAwsAppAccessRequest
): Promise<App> {
return (
api
.post(cfg.getAwsAppAccessUrlV2(integrationName), req)
.then(makeApp)
// TODO(kimlisa): DELETE IN 19.0
.catch(withUnsupportedLabelFeatureErrorConversion)
);
},

// TODO(kimlisa): DELETE IN 19.0
// replaced by createAwsAppAccessV2 that accepts request body
async createAwsAppAccess(integrationName): Promise<App> {
return api
.post(cfg.getAwsAppAccessUrl(integrationName), null)
Expand Down
13 changes: 13 additions & 0 deletions web/packages/teleport/src/services/integrations/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -759,3 +759,16 @@ export type AwsDatabaseVpcsResponse = {
vpcs: Vpc[];
nextToken: string;
};

/**
* Object that contains request fields for
* when requesting to create an AWS console app.
*
* This request object is only supported with v2 endpoint.
*/
export type CreateAwsAppAccessRequest = {
Comment thread
flyinghermit marked this conversation as resolved.
/**
* resource labels that will be set as app_server's labels
*/
labels?: Record<string, string>;
};
4 changes: 3 additions & 1 deletion web/packages/teleport/src/services/version/unsupported.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

import { ApiError } from '../api/parseError';

export const ProxyRequiresUpgrade = 'Ensure all proxies are upgraded';

export function withUnsupportedLabelFeatureErrorConversion(
err: unknown
): never {
Expand All @@ -26,7 +28,7 @@ export function withUnsupportedLabelFeatureErrorConversion(
'We could not complete your request. ' +
'Your proxy may be behind the minimum required version ' +
`(v17.2.0) to support adding resource labels. ` +
'Ensure all proxies are upgraded or remove labels and try again.'
`${ProxyRequiresUpgrade} or remove labels and try again.`
);
}
throw err;
Expand Down