Skip to content
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

Move EKS Discover joinToken generation to the ManualHelm dialog. #37730

Merged
merged 8 commits into from
Feb 23, 2024
Merged

Move EKS Discover joinToken generation to the ManualHelm dialog. #37730

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c05349b
Move EKS joinToken generation to the ManualHelm dialog.
AntonAM Feb 2, 2024
52f6cf7
Add container to manual helm dialog.
AntonAM Feb 21, 2024
55f8214
Document join creation delay reason.
AntonAM Feb 21, 2024
5fafad1
Remove usage of msw.
AntonAM Feb 22, 2024
0d9a158
Generate command outside of manualHelmDialog.
AntonAM Feb 22, 2024
e67ad98
Fix setting join token in the manual helm dialog.
AntonAM Feb 23, 2024
8faf471
Refactor dummyDialog.
AntonAM Feb 23, 2024
2f7a401
Use indicator component.
AntonAM Feb 23, 2024
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
127 changes: 112 additions & 15 deletions web/packages/teleport/src/Discover/Kubernetes/EnrollEKSCluster/Dialogs.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import React from 'react';
import React, { useEffect } from 'react';
import { MemoryRouter } from 'react-router';
import { rest } from 'msw';
import { mswLoader } from 'msw-storybook-addon';
Expand All @@ -27,15 +27,45 @@ import { ResourceKind } from 'teleport/Discover/Shared';
import { PingTeleportProvider } from 'teleport/Discover/Shared/PingTeleportContext';
import { ContextProvider } from 'teleport';

import { generateCmd } from 'teleport/Discover/Kubernetes/HelmChart/HelmChart';
const { worker } = window.msw;

import { INTERNAL_RESOURCE_ID_LABEL_KEY } from 'teleport/services/joinToken';
import { clearCachedJoinTokenResult } from 'teleport/Discover/Shared/useJoinTokenSuspender';
import {
DiscoverContextState,
DiscoverProvider,
} from 'teleport/Discover/useDiscover';
import {
IntegrationKind,
IntegrationStatusCode,
} from 'teleport/services/integrations';
import { DiscoverEventResource } from 'teleport/services/userEvent';

import { ManualHelmDialog } from './ManualHelmDialog';
import { AgentWaitingDialog } from './AgentWaitingDialog';
import { EnrollmentDialog } from './EnrollmentDialog';
import { AgentWaitingDialog } from './AgentWaitingDialog';
import { ManualHelmDialog } from './ManualHelmDialog';

export default {
title: 'Teleport/Discover/Kube/EnrollEksClusters/Dialogs',
loaders: [mswLoader],
decorators: [
Story => {
worker.resetHandlers();

useEffect(() => {
// Clean up
return () => {
clearCachedJoinTokenResult([
ResourceKind.Kubernetes,
ResourceKind.Application,
ResourceKind.Discovery,
]);
};
}, []);
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK, worker.resetHandlers is needed only when using res.once. res.once is necessary only you want a scenario where a single response from an endpoint is different from other responses from the same endpoint (https://github.com/gravitational/teleport.e/pull/3256/commits/7dca9032287fe442b540b7437955432092abc622).

Since res.once is not used here, I think we can drop worker.resetHandlers and remove window.msw usage which will save Ryan some work when moving to Storybook 7 (#37331, #34450). Let me know though if the lack of worker.resetHandlers interferes with some other stuff that I don't know about.

useJoinTokenSuspender is used only in ManualHelmDialog, so let's put the cleanup only in that story.

Patch 
diff --git a/web/packages/teleport/src/Discover/Kubernetes/EnrollEKSCluster/Dialogs.story.tsx b/web/packages/teleport/src/Discover/Kubernetes/EnrollEKSCluster/Dialogs.story.tsx
index 3365bd4d0e..466591abec 100644
--- a/web/packages/teleport/src/Discover/Kubernetes/EnrollEKSCluster/Dialogs.story.tsx
+++ b/web/packages/teleport/src/Discover/Kubernetes/EnrollEKSCluster/Dialogs.story.tsx
@@ -27,8 +27,6 @@ import { ResourceKind } from 'teleport/Discover/Shared';
 import { PingTeleportProvider } from 'teleport/Discover/Shared/PingTeleportContext';
 import { ContextProvider } from 'teleport';
 
-const { worker } = window.msw;
-
 import { INTERNAL_RESOURCE_ID_LABEL_KEY } from 'teleport/services/joinToken';
 import { clearCachedJoinTokenResult } from 'teleport/Discover/Shared/useJoinTokenSuspender';
 import {
@@ -48,24 +46,6 @@ import { ManualHelmDialog } from './ManualHelmDialog';
 export default {
   title: 'Teleport/Discover/Kube/EnrollEksClusters/Dialogs',
   loaders: [mswLoader],
-  decorators: [
-    Story => {
-      worker.resetHandlers();
-
-      useEffect(() => {
-        // Clean up
-        return () => {
-          clearCachedJoinTokenResult([
-            ResourceKind.Kubernetes,
-            ResourceKind.Application,
-            ResourceKind.Discovery,
-          ]);
-        };
-      }, []);
-
-      return <Story />;
-    },
-  ],
 };
 
 export const EnrollmentDialogStory = () => (
@@ -199,6 +179,16 @@ export const ManualHelmDialogStory = () => {
     eventState: null,
   };
 
+  useEffect(() => {
+    return () => {
+      clearCachedJoinTokenResult([
+        ResourceKind.Kubernetes,
+        ResourceKind.Application,
+        ResourceKind.Discovery,
+      ]);
+    };
+  }, []);
+
   return (
     <MemoryRouter
       initialEntries={[

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Yes, it seems to be working fine without msw 5fafad1


return <Story />;
},
],
};

export const EnrollmentDialogStory = () => (
Expand Down Expand Up @@ -110,7 +140,7 @@ AgentWaitingDialogSuccess.parameters = {
},
};

const helmCommand = generateCmd({
const helmCommandProps = {
namespace: 'teleport-agent',
clusterName: 'EKS1',
proxyAddr: 'teleport-proxy.example.com:1234',
Expand All @@ -126,15 +156,82 @@ const helmCommand = generateCmd({
{ name: 'region', value: 'us-east-1' },
{ name: 'account-id', value: '1234567789012' },
],
});
};

export const ManualHelmDialogStory = () => (
<MemoryRouter initialEntries={[{ state: { discover: {} } }]}>
<ManualHelmDialog
command={helmCommand}
confirmedCommands={() => {}}
cancel={() => {}}
/>
</MemoryRouter>
);
export const ManualHelmDialogStory = () => {
const discoverCtx: DiscoverContextState = {
agentMeta: {
resourceName: 'kube-name',
agentMatcherLabels: [],
kube: {
kind: 'kube_cluster',
name: '',
labels: [],
},
awsIntegration: {
kind: IntegrationKind.AwsOidc,
name: 'test-oidc',
resourceType: 'integration',
spec: {
roleArn: 'arn:aws:iam::123456789012:role/test-role-arn',
},
statusCode: IntegrationStatusCode.Running,
},
},
currentStep: 0,
nextStep: () => null,
prevStep: () => null,
onSelectResource: () => null,
resourceSpec: {
name: 'Eks',
kind: ResourceKind.Kubernetes,
icon: 'Eks',
keywords: '',
event: DiscoverEventResource.KubernetesEks,
},
exitFlow: () => null,
viewConfig: null,
indexedViews: [],
setResourceSpec: () => null,
updateAgentMeta: () => null,
emitErrorEvent: () => null,
emitEvent: () => null,
eventState: null,
};

return (
<MemoryRouter
initialEntries={[
{ pathname: cfg.routes.discover, state: { entity: 'eks' } },
]}
>
<ContextProvider ctx={createTeleportContext()}>
<DiscoverProvider mockCtx={discoverCtx}>
<ManualHelmDialog
commandProps={helmCommandProps}
setJoinToken={() => {}}
confirmedCommands={() => {}}
cancel={() => {}}
/>
</DiscoverProvider>
</ContextProvider>
</MemoryRouter>
);
};
ManualHelmDialogStory.storyName = 'ManualHelmDialog';
ManualHelmDialogStory.parameters = {
msw: {
handlers: [
rest.post(cfg.api.joinTokenPath, (req, res, ctx) => {
return res(
ctx.json({
id: 'token-id',
suggestedLabels: [
{ name: INTERNAL_RESOURCE_ID_LABEL_KEY, value: 'resource-id' },
],
})
);
}),
],
},
};
31 changes: 13 additions & 18 deletions web/packages/teleport/src/Discover/Kubernetes/EnrollEKSCluster/EnrollEksCluster.tsx
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

When I open the story for EnrollEksCluster and try to open ManualHelmDialog, I get "A component suspended while responding to synchronous input. This will cause the UI to be replaced with a loading indicator. To fix, updates that suspend should be wrapped with startTransition."

I wanted to use the story to see how setJoinToken in ManualHelmDialog interacts with re-rendering EnrollEksCluster since I don't have a cluster with an AWS account set up to test this in a real UI.

storybook-error.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added container to the dialog 52f6cf7

Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,12 @@ import { isIamPermError } from 'teleport/Discover/Shared/Aws/error';
import { AgentStepProps } from 'teleport/Discover/types';
import useTeleport from 'teleport/useTeleport';

import { useJoinTokenSuspender } from 'teleport/Discover/Shared/useJoinTokenSuspender';
import { generateCmd } from 'teleport/Discover/Kubernetes/HelmChart/HelmChart';
import { GenerateCmdProps } from 'teleport/Discover/Kubernetes/HelmChart/HelmChart';
import { Kube } from 'teleport/services/kube';

import { Header, ResourceKind } from '../../Shared';
import { JoinToken } from 'teleport/services/joinToken';

import { Header } from '../../Shared';

import { ClustersList } from './EksClustersList';
import { ManualHelmDialog } from './ManualHelmDialog';
Expand Down Expand Up @@ -97,14 +98,9 @@ export function EnrollEksCluster(props: AgentStepProps) {
useState(false);
const [isManualHelmDialogShown, setIsManualHelmDialogShown] = useState(false);
const [waitingResourceId, setWaitingResourceId] = useState('');
const [joinToken, setJoinToken] = useState<JoinToken>(null);
Copy link
Member

Choose a reason for hiding this comment

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

Let's document the fact that we need a separate state for the join token because while the join token is used mostly by code in EnrollEksCluster, the join token needs to be generated only after ManualHelmDialog gets open.

If I saw this code without any context, my inclination would be to move useJoinTokenSuspender to EnrollEksCluster and pass the token down to ManualHelmDialog, because that's common practice in React – if you want to share a piece of state, you move it up. Here it wouldn't work of course because of the constraints you talk about in the PR description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comment 👍 55f8214

const ctx = useTeleport();

const { joinToken } = useJoinTokenSuspender([
ResourceKind.Kubernetes,
ResourceKind.Application,
ResourceKind.Discovery,
]);

function fetchClustersWithNewRegion(region: Regions) {
setSelectedRegion(region);
// Clear table when fetching with new region.
Expand Down Expand Up @@ -205,8 +201,6 @@ export function EnrollEksCluster(props: AgentStepProps) {
{
region: selectedRegion,
enableAppDiscovery: isAppDiscoveryEnabled,
joinToken: joinToken.id,
resourceId: joinToken.internalResourceId,
Comment on lines -208 to -209
Copy link
Contributor

Choose a reason for hiding this comment

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

why did we remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not used anymore by the backend - we now create our own token in the backend enrollment code and return resourceId that frontend should look for while pinging resource.

clusterNames: [selectedCluster.name],
}
);
Expand Down Expand Up @@ -262,22 +256,22 @@ export function EnrollEksCluster(props: AgentStepProps) {
!selectedCluster ||
enrollmentState.status !== 'notStarted';

let command = '';
let manualCommandProps: GenerateCmdProps = null;
if (selectedCluster) {
command = generateCmd({
manualCommandProps = {
namespace: 'teleport-agent',
clusterName: selectedCluster.name,
proxyAddr: ctx.storeUser.state.cluster.publicURL,
tokenId: joinToken.id,
clusterVersion: ctx.storeUser.state.cluster.authVersion,
resourceId: joinToken.internalResourceId,
tokenId: '', // Filled in by the ManualHelmDialog.
resourceId: '',
Copy link
Member

Choose a reason for hiding this comment

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

Since manualCommandProps are used only in ManualHelmDialog and whenever we set the token, we also generateCmd, another approach we could take is to create here a function called something like setJoinTokenAndGenerateCmd and pass it to ManualHelmDialog. This sort-of leads to better ergonomics (you just need to pass one prop vs an object with many fields; you don't leave holes in data structures that need to be filled in) and it also encapsulates the business logic a little bit better since it's easier to see how and when joinToken gets set.

But I don't think the current approach is bad, I just wanted to show an alternative approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was never a fan of sending all the information into the dialog. Changed to the way you suggested 0d9a158

isEnterprise: ctx.isEnterprise,
isCloud: ctx.isCloud,
automaticUpgradesEnabled: ctx.automaticUpgradesEnabled,
automaticUpgradesTargetVersion: ctx.automaticUpgradesTargetVersion,
joinLabels: [...selectedCluster.labels, ...selectedCluster.joinLabels],
disableAppDiscovery: !isAppDiscoveryEnabled,
});
};
}

return (
Expand Down Expand Up @@ -366,7 +360,8 @@ export function EnrollEksCluster(props: AgentStepProps) {
)}
{isManualHelmDialogShown && (
<ManualHelmDialog
command={command}
commandProps={manualCommandProps}
setJoinToken={setJoinToken}
cancel={() => setIsManualHelmDialogShown(false)}
confirmedCommands={() => {
setEnrollmentState({ status: 'awaitingAgent' });
Expand All @@ -377,7 +372,7 @@ export function EnrollEksCluster(props: AgentStepProps) {
)}
{isAgentWaitingDialogShown && (
<AgentWaitingDialog
joinResourceId={waitingResourceId || joinToken.internalResourceId}
joinResourceId={waitingResourceId || joinToken?.internalResourceId}
status={enrollmentState.status}
clusterName={selectedCluster.name}
updateWaitingResult={(result: Kube) => {
Expand Down
27 changes: 25 additions & 2 deletions web/packages/teleport/src/Discover/Kubernetes/EnrollEKSCluster/ManualHelmDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,20 +23,43 @@ import React from 'react';

import styled from 'styled-components';

import {
GenerateCmdProps,
generateCmd,
} from 'teleport/Discover/Kubernetes/HelmChart/HelmChart';
import { TextSelectCopyMulti } from 'teleport/components/TextSelectCopy';
import { CommandBox } from 'teleport/Discover/Shared/CommandBox';

import { useJoinTokenSuspender } from 'teleport/Discover/Shared/useJoinTokenSuspender';
import { ResourceKind } from 'teleport/Discover/Shared';
import { JoinToken } from 'teleport/services/joinToken';

type ManualHelmDialogProps = {
command: string;
commandProps: GenerateCmdProps;
setJoinToken(token: JoinToken): void;
confirmedCommands(): void;
cancel(): void;
};

export function ManualHelmDialog({
command,
commandProps,
setJoinToken,
cancel,
confirmedCommands,
}: ManualHelmDialogProps) {
const { joinToken } = useJoinTokenSuspender([
ResourceKind.Kubernetes,
ResourceKind.Application,
ResourceKind.Discovery,
]);

setJoinToken(joinToken);
const command = generateCmd({
...commandProps,
tokenId: joinToken.id,
resourceId: joinToken.internalResourceId,
});

return (
<Dialog onClose={cancel} open={true}>
<DialogContent width="850px" mb={0}>
Expand Down
2 changes: 0 additions & 2 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 @@ -317,8 +317,6 @@ export type AwsEksCluster = {
export type EnrollEksClustersRequest = {
region: string;
enableAppDiscovery: boolean;
joinToken: string;
resourceId: string;
clusterNames: string[];
};

Expand Down
Loading