Skip to content

Commit

Permalink
Add health check when creating integration
Browse files Browse the repository at this point in the history
  • Loading branch information
kimlisa authored and github-actions committed Nov 14, 2024
1 parent 501b3b5 commit abdbb79
Show file tree
Hide file tree
Showing 3 changed files with 97 additions and 8 deletions.
4 changes: 4 additions & 0 deletions lib/web/integrations_awsoidc.go
Original file line number Diff line number Diff line change
Expand Up @@ -1454,6 +1454,10 @@ func (h *Handler) awsOIDCPing(w http.ResponseWriter, r *http.Request, p httprout
return nil, trace.Wrap(err)
}

if req.RoleARN != "" {
integrationName = ""
}

pingResp, err := clt.IntegrationAWSOIDCClient().Ping(ctx, &integrationv1.PingRequest{
Integration: integrationName,
RoleArn: req.RoleARN,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ import { MemoryRouter } from 'react-router';

import { userEventService } from 'teleport/services/userEvent';

import { integrationService } from 'teleport/services/integrations';
import { ApiError } from 'teleport/services/api/parseError';

import { AwsOidc } from './AwsOidc';

test('render', async () => {
Expand Down Expand Up @@ -56,6 +59,14 @@ test('generate command', async () => {
.spyOn(userEventService, 'captureIntegrationEnrollEvent')
.mockImplementation();

let spyPing = jest
.spyOn(integrationService, 'pingAwsOidcIntegration')
.mockResolvedValue({} as any); // response doesn't matter

const spyCreate = jest
.spyOn(integrationService, 'createIntegration')
.mockResolvedValue({} as any); // response doesn't matter

window.prompt = jest.fn();

render(
Expand Down Expand Up @@ -95,4 +106,42 @@ test('generate command', async () => {
const clipboardText = await navigator.clipboard.readText();
expect(clipboardText).toContain(`integrationName=${pluginConfig.name}`);
expect(clipboardText).toContain(`role=${pluginConfig.roleName}`);

// Fill out arn.
fireEvent.change(screen.getByLabelText(/role arn/i), {
target: {
value: `arn:aws:iam::123456789012:role/${pluginConfig.roleName}`,
},
});

// Test ping is called before create.
fireEvent.click(screen.getByRole('button', { name: /create integration/i }));
await waitFor(() => expect(spyPing).toHaveBeenCalledTimes(1));
await waitFor(() => expect(spyCreate).toHaveBeenCalledTimes(1));

let pingOrder = spyPing.mock.invocationCallOrder[0];
let createOrder = spyCreate.mock.invocationCallOrder[0];
expect(pingOrder).toBeLessThan(createOrder);

// Test create is still called with 404 ping error.
jest.clearAllMocks();
let error = new ApiError('', { status: 404 } as Response);
spyPing = jest
.spyOn(integrationService, 'pingAwsOidcIntegration')
.mockRejectedValue(error);

fireEvent.click(screen.getByRole('button', { name: /create integration/i }));
await waitFor(() => expect(spyPing).toHaveBeenCalledTimes(1));
await waitFor(() => expect(spyCreate).toHaveBeenCalledTimes(1));

// Test create isn't called with non 404 error
jest.clearAllMocks();
error = new ApiError('', { status: 400 } as Response);
spyPing = jest
.spyOn(integrationService, 'pingAwsOidcIntegration')
.mockRejectedValue(error);

fireEvent.click(screen.getByRole('button', { name: /create integration/i }));
await waitFor(() => expect(spyPing).toHaveBeenCalledTimes(1));
await waitFor(() => expect(spyCreate).toHaveBeenCalledTimes(0));
});
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@
import { useEffect, useState } from 'react';
import { useLocation } from 'react-router';
import { Validator } from 'shared/components/Validation';
import { useAsync } from 'shared/hooks/useAsync';
import {
makeErrorAttempt,
makeProcessingAttempt,
useAsync,
} from 'shared/hooks/useAsync';

import { DiscoverUrlLocationState } from 'teleport/Discover/useDiscover';
import {
Expand All @@ -36,6 +40,8 @@ import {
integrationService,
AwsOidcPolicyPreset,
} from 'teleport/services/integrations';
import useStickyClusterId from 'teleport/useStickyClusterId';
import { ApiError } from 'teleport/services/api/parseError';

type integrationConfig = {
name: string;
Expand All @@ -53,6 +59,7 @@ export function useAwsOidcIntegration() {
);
const [scriptUrl, setScriptUrl] = useState('');
const [createdIntegration, setCreatedIntegration] = useState<Integration>();
const { clusterId } = useStickyClusterId();

const location = useLocation<DiscoverUrlLocationState>();

Expand Down Expand Up @@ -80,18 +87,47 @@ export function useAwsOidcIntegration() {
});
}

const [createIntegrationAttempt, runCreateIntegration] = useAsync(
async (req: IntegrationCreateRequest) => {
const resp = await integrationService.createIntegration(req);
setCreatedIntegration(resp);
return resp;
}
);
const [
createIntegrationAttempt,
runCreateIntegration,
setCreateIntegrationAttempt,
] = useAsync(async (req: IntegrationCreateRequest) => {
const resp = await integrationService.createIntegration(req);
setCreatedIntegration(resp);
return resp;
});

async function handleOnCreate(validator: Validator) {
if (!validator.validate()) {
return;
}
setCreateIntegrationAttempt(makeProcessingAttempt());

try {
await integrationService.pingAwsOidcIntegration(
{
integrationName: integrationConfig.name,
clusterId,
},
{ roleArn: integrationConfig.roleArn }
);
} catch (err) {
// DELETE IN v18.0
// Ignore not found error and just allow it to create which
// is how it used to work before anyways.
//
// If this request went to an older proxy, that didn't set the
// the integrationName empty if roleArn isn't empty, then the backend
// will never be able to successfully health check b/c it expects
// integration to exist first before creating.
const isNotFoundErr =
err instanceof ApiError && err.response.status === 404;

if (!isNotFoundErr) {
setCreateIntegrationAttempt(makeErrorAttempt(err));
return;
}
}

const [, err] = await runCreateIntegration({
name: integrationConfig.name,
Expand Down

0 comments on commit abdbb79

Please sign in to comment.