Skip to content

Commit

Permalink
Address CRs
Browse files Browse the repository at this point in the history
  • Loading branch information
@kimlisa
kimlisa committed Oct 22, 2024
1 parent 9ff1f70 commit 8e795be
Show file tree
Hide file tree
Showing 5 changed files with 220 additions and 127 deletions.
6 changes: 3 additions & 3 deletions web/packages/teleterm/src/ui/AccessRequestCheckout/AccessRequestCheckout.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import * as Icon from 'design/Icon';
import { pluralize } from 'shared/utils/text';

import { RequestCheckoutWithSlider } from 'shared/components/AccessRequests/NewRequest';
import { excludeKubeClusterWithNamespaces } from 'shared/components/AccessRequests/NewRequest/kube';
import { isKubeClusterWithNamespaces } from 'shared/components/AccessRequests/NewRequest/kube';

import useAccessRequestCheckout from './useAccessRequestCheckout';
import { AssumedRolesBar } from './AssumedRolesBar';
Expand Down Expand Up @@ -114,8 +114,8 @@ export function AccessRequestCheckout() {
setShowCheckout(false);
}

const filteredData = pendingAccessRequests.filter(d =>
excludeKubeClusterWithNamespaces(d, pendingAccessRequests)
const filteredData = pendingAccessRequests.filter(
d => !isKubeClusterWithNamespaces(d, pendingAccessRequests)
);

const numAddedResources = filteredData.length;
Expand Down
120 changes: 120 additions & 0 deletions web/packages/teleterm/src/ui/AccessRequestCheckout/useAccessRequestCheckout.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,14 @@ import { renderHook, waitFor } from '@testing-library/react';
import {
makeRootCluster,
makeServer,
makeKube,
rootClusterUri,
} from 'teleterm/services/tshd/testHelpers';
import { MockAppContext } from 'teleterm/ui/fixtures/mocks';
import { MockAppContextProvider } from 'teleterm/ui/fixtures/MockAppContextProvider';

import { mapRequestToKubeNamespaceUri } from '../services/workspacesService/accessRequestsService';

import useAccessRequestCheckout from './useAccessRequestCheckout';

test('fetching requestable roles for servers uses UUID, not hostname', async () => {
Expand Down Expand Up @@ -64,3 +67,120 @@ test('fetching requestable roles for servers uses UUID, not hostname', async ()
})
);
});

test('fetching requestable roles kube_cluster resource without namespace request', async () => {
const kube = makeKube();
const cluster = makeRootCluster();
const appContext = new MockAppContext();
appContext.clustersService.setState(draftState => {
draftState.clusters.set(rootClusterUri, cluster);
});
await appContext.workspacesService.setActiveWorkspace(rootClusterUri);
await appContext.workspacesService
.getWorkspaceAccessRequestsService(rootClusterUri)
.addOrRemoveResource({
kind: 'kube',
resource: kube,
});

jest.spyOn(appContext.tshd, 'getRequestableRoles');

const wrapper = ({ children }) => (
<MockAppContextProvider appContext={appContext}>
{children}
</MockAppContextProvider>
);

renderHook(useAccessRequestCheckout, { wrapper });

await waitFor(() =>
expect(appContext.tshd.getRequestableRoles).toHaveBeenCalledWith({
clusterUri: rootClusterUri,
resourceIds: [
{
clusterName: 'teleport-local',
kind: 'kube_cluster',
name: kube.name,
subResourceName: '',
},
],
})
);
});

test(`fetching requestable roles for a kube cluster's namespaces only creates resource IDs for its namespaces`, async () => {
const kube1 = makeKube();
const kube2 = makeKube({
name: 'kube2',
uri: `${rootClusterUri}/kubes/kube2`,
});
const cluster = makeRootCluster();
const appContext = new MockAppContext();
appContext.clustersService.setState(draftState => {
draftState.clusters.set(rootClusterUri, cluster);
});
await appContext.workspacesService.setActiveWorkspace(rootClusterUri);
await appContext.workspacesService
.getWorkspaceAccessRequestsService(rootClusterUri)
.addOrRemoveResource({
kind: 'kube',
resource: kube1,
});
await appContext.workspacesService
.getWorkspaceAccessRequestsService(rootClusterUri)
.addOrRemoveResource({
kind: 'kube',
resource: kube2,
});

await appContext.workspacesService
.getWorkspaceAccessRequestsService(rootClusterUri)
.addOrRemoveKubeNamespaces([
mapRequestToKubeNamespaceUri({
clusterUri: rootClusterUri,
id: kube1.name,
name: 'namespace1',
}),
mapRequestToKubeNamespaceUri({
clusterUri: rootClusterUri,
id: kube1.name,
name: 'namespace2',
}),
]);

jest.spyOn(appContext.tshd, 'getRequestableRoles');

const wrapper = ({ children }) => (
<MockAppContextProvider appContext={appContext}>
{children}
</MockAppContextProvider>
);

renderHook(useAccessRequestCheckout, { wrapper });

await waitFor(() =>
expect(appContext.tshd.getRequestableRoles).toHaveBeenCalledWith({
clusterUri: rootClusterUri,
resourceIds: [
{
clusterName: 'teleport-local',
kind: 'namespace',
name: kube1.name,
subResourceName: 'namespace1',
},
{
clusterName: 'teleport-local',
kind: 'namespace',
name: kube1.name,
subResourceName: 'namespace2',
},
{
clusterName: 'teleport-local',
kind: 'kube_cluster',
name: kube2.name,
subResourceName: '',
},
],
})
);
});
81 changes: 35 additions & 46 deletions web/packages/teleterm/src/ui/AccessRequestCheckout/useAccessRequestCheckout.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import { useSpecifiableFields } from 'shared/components/AccessRequests/NewReques

import { CreateRequest } from 'shared/components/AccessRequests/Shared/types';
import {
excludeKubeClusterWithNamespaces,
isKubeClusterWithNamespaces,
KubeNamespaceRequest,
} from 'shared/components/AccessRequests/NewRequest/kube';
import { PendingKubeResourceItem } from 'shared/components/AccessRequests/NewRequest/RequestCheckout/RequestCheckout';
Expand All @@ -39,7 +39,8 @@ import {
PendingAccessRequest,
extractResourceRequestProperties,
ResourceRequest,
toResourceRequest,
mapRequestToKubeNamespaceUri,
mapKubeNamespaceUriToRequest,
} from 'teleterm/ui/services/workspacesService/accessRequestsService';
import { retryWithRelogin } from 'teleterm/ui/utils';
import {
Expand Down Expand Up @@ -96,6 +97,14 @@ export default function useAccessRequestCheckout() {
const pendingAccessRequest =
workspaceAccessRequest?.getPendingAccessRequest();

const pendingAccessRequestsPerResource =
getPendingAccessRequestsPerResource(pendingAccessRequest);

const pendingAccessRequestsPerResourceWithoutParentResources =
pendingAccessRequestsPerResource.filter(
p => !isKubeClusterWithNamespaces(p, pendingAccessRequestsPerResource)
);

useEffect(() => {
// Do a new dry run per changes to pending access requests
// to get the latest time options and latest calculated
Expand All @@ -112,15 +121,11 @@ export default function useAccessRequestCheckout() {
return;
}

const pendingAccessRequests = getPendingAccessRequestsPerResource({
pendingRequest: pendingAccessRequest,
excludeSubResourceParentResource: true,
});
runFetchResourceRoles(() =>
retryWithRelogin(ctx, clusterUri, async () => {
const { response } = await ctx.tshd.getRequestableRoles({
clusterUri: rootClusterUri,
resourceIds: pendingAccessRequests
resourceIds: pendingAccessRequestsPerResourceWithoutParentResources
.filter(d => d.kind !== 'role')
.map(d => ({
// We have to use id, not name.
Expand Down Expand Up @@ -162,13 +167,9 @@ export default function useAccessRequestCheckout() {
* if a kube_cluster resource has a list of namespaces (subresources),
* then if this flag is true, kube_cluster will be excluded from the result.
*/
function getPendingAccessRequestsPerResource({
pendingRequest,
excludeSubResourceParentResource = false,
}: {
pendingRequest: PendingAccessRequest;
excludeSubResourceParentResource?: boolean;
}): PendingListItemWithOriginalItem[] {
function getPendingAccessRequestsPerResource(
pendingRequest: PendingAccessRequest
): PendingListItemWithOriginalItem[] {
const pendingAccessRequests: PendingListItemWithOriginalItem[] = [];
if (!workspaceAccessRequest) {
return pendingAccessRequests;
Expand Down Expand Up @@ -197,25 +198,22 @@ export default function useAccessRequestCheckout() {
resourceRequest.resource.namespaces?.size > 0
) {
// Process each namespace.
resourceRequest.resource.namespaces.forEach(namespaceRequest => {
resourceRequest.resource.namespaces.forEach(namespaceRequestUri => {
const { kind, id, name } =
extractResourceRequestProperties(namespaceRequest);
mapKubeNamespaceUriToRequest(namespaceRequestUri);

const item = {
kind,
id,
name,
subResourceName: name,
originalItem: namespaceRequest,
clusterName: ctx.clustersService.findClusterByResource(
namespaceRequest.resource.uri
)?.name,
originalItem: resourceRequest,
clusterName:
ctx.clustersService.findClusterByResource(namespaceRequestUri)
?.name,
};
pendingAccessRequests.push(item);
});
if (excludeSubResourceParentResource) {
return;
}
}

const { kind, id, name } =
Expand Down Expand Up @@ -260,15 +258,14 @@ export default function useAccessRequestCheckout() {
items: PendingKubeResourceItem[],
kubeCluster: PendingListKubeClusterWithOriginalItem
) {
await workspaceAccessRequest.addOrRemoveResources(
items.map(item => {
return toResourceRequest({
kind: item.kind,
resourceId: item.id,
resourceName: item.subResourceName,
await workspaceAccessRequest.addOrRemoveKubeNamespaces(
items.map(item =>
mapRequestToKubeNamespaceUri({
id: item.id,
name: item.subResourceName,
clusterUri: kubeCluster.originalItem.resource.uri,
});
})
})
)
);
}

Expand All @@ -287,19 +284,13 @@ export default function useAccessRequestCheckout() {
* Shared logic used both during dry runs and regular access request creation.
*/
function prepareAndCreateRequest(req: CreateRequest) {
const pendingAccessRequests = getPendingAccessRequestsPerResource({
pendingRequest: pendingAccessRequest,
excludeSubResourceParentResource: true,
});

const params: CreateAccessRequestRequest = {
rootClusterUri,
reason: req.reason,
suggestedReviewers: req.suggestedReviewers || [],
dryRun: req.dryRun,
resourceIds: pendingAccessRequests
resourceIds: pendingAccessRequestsPerResourceWithoutParentResources
.filter(d => d.kind !== 'role')
.filter(d => excludeKubeClusterWithNamespaces(d, pendingAccessRequests))
.map(d => {
if (d.kind === 'namespace') {
return {
Expand All @@ -316,7 +307,7 @@ export default function useAccessRequestCheckout() {
subResourceName: '',
};
}),
roles: pendingAccessRequests
roles: pendingAccessRequestsPerResourceWithoutParentResources
.filter(d => d.kind === 'role')
.map(d => d.name),
assumeStartTime: req.start && Timestamp.fromDate(req.start),
Expand All @@ -340,9 +331,9 @@ export default function useAccessRequestCheckout() {
ctx.clustersService.createAccessRequest(params).then(({ response }) => {
return {
accessRequest: response.request,
requestedCount: pendingAccessRequests.filter(d =>
excludeKubeClusterWithNamespaces(d, pendingAccessRequests)
).length,
requestedCount:
pendingAccessRequestsPerResourceWithoutParentResources.filter
.length,
};
})
).catch(e => {
Expand Down Expand Up @@ -449,9 +440,7 @@ export default function useAccessRequestCheckout() {
isCollapsed,
assumedRequests: getAssumedRequests(),
toggleResource,
data: getPendingAccessRequestsPerResource({
pendingRequest: pendingAccessRequest,
}),
data: pendingAccessRequestsPerResource,
shouldShowClusterNameColumn,
createRequest,
reset,
Expand Down Expand Up @@ -497,5 +486,5 @@ type PendingListItemWithOriginalItem = Omit<PendingListItem, 'kind'> &

type PendingListKubeClusterWithOriginalItem = Omit<PendingListItem, 'kind'> & {
kind: Extract<ResourceKind, 'kube_cluster'>;
originalItem: ResourceRequest;
originalItem: Extract<ResourceRequest, { kind: 'kube' }>;
};
6 changes: 6 additions & 0 deletions web/packages/teleterm/src/ui/DocumentAccessRequests/NewRequest/useNewRequest.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,12 @@ export default function useNewRequest(rootCluster: Cluster) {
return;
}

if (kind === 'namespace') {
// It is not possible to request a kube namespace through this function.
// The type should be corrected.
return;
}

accessRequestsService.addOrRemoveResource(
toResourceRequest({
kind,
Expand Down
Loading

0 comments on commit 8e795be

Please sign in to comment.