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.

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
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
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,11 +27,23 @@ import { ResourceKind } from 'teleport/Discover/Shared';
import { PingTeleportProvider } from 'teleport/Discover/Shared/PingTeleportContext';
import { ContextProvider } from 'teleport';

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 { generateCmd } from 'teleport/Discover/Kubernetes/HelmChart/HelmChart';

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',
Expand Down Expand Up @@ -110,7 +122,7 @@ AgentWaitingDialogSuccess.parameters = {
},
};

const helmCommand = generateCmd({
const helmCommandProps = {
namespace: 'teleport-agent',
clusterName: 'EKS1',
proxyAddr: 'teleport-proxy.example.com:1234',
Expand All @@ -126,15 +138,91 @@ 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,
};

useEffect(() => {
return () => {
clearCachedJoinTokenResult([
ResourceKind.Kubernetes,
ResourceKind.Application,
ResourceKind.Discovery,
]);
};
}, []);

return (
<MemoryRouter
initialEntries={[
{ pathname: cfg.routes.discover, state: { entity: 'eks' } },
]}
>
<ContextProvider ctx={createTeleportContext()}>
<DiscoverProvider mockCtx={discoverCtx}>
<ManualHelmDialog
setJoinTokenAndGetCommand={() => generateCmd(helmCommandProps)}
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' },
],
})
);
}),
],
},
};
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,14 +39,15 @@ 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 { 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';
import ManualHelmDialog from './ManualHelmDialog';
import { EnrollmentDialog } from './EnrollmentDialog';
import { AgentWaitingDialog } from './AgentWaitingDialog';

Expand Down Expand Up @@ -97,14 +98,11 @@ export function EnrollEksCluster(props: AgentStepProps) {
useState(false);
const [isManualHelmDialogShown, setIsManualHelmDialogShown] = useState(false);
const [waitingResourceId, setWaitingResourceId] = useState('');
// join token will be set only if user opens ManualHelmDialog,
// we delay it to avoid premature admin action MFA confirmation request.
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 +203,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,23 +258,23 @@ export function EnrollEksCluster(props: AgentStepProps) {
!selectedCluster ||
enrollmentState.status !== 'notStarted';

let command = '';
if (selectedCluster) {
command = generateCmd({
const setJoinTokenAndGetCommand = (token: JoinToken) => {
setJoinToken(token);
return generateCmd({
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: token.id,
resourceId: token.internalResourceId,
isEnterprise: ctx.isEnterprise,
isCloud: ctx.isCloud,
automaticUpgradesEnabled: ctx.automaticUpgradesEnabled,
automaticUpgradesTargetVersion: ctx.automaticUpgradesTargetVersion,
joinLabels: [...selectedCluster.labels, ...selectedCluster.joinLabels],
disableAppDiscovery: !isAppDiscoveryEnabled,
});
}
};

return (
<Box maxWidth="1000px">
Expand Down Expand Up @@ -366,7 +362,7 @@ export function EnrollEksCluster(props: AgentStepProps) {
)}
{isManualHelmDialogShown && (
<ManualHelmDialog
command={command}
setJoinTokenAndGetCommand={setJoinTokenAndGetCommand}
cancel={() => setIsManualHelmDialogShown(false)}
confirmedCommands={() => {
setEnrollmentState({ status: 'awaitingAgent' });
Expand All @@ -377,7 +373,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
Original file line number Diff line number Diff line change
Expand Up @@ -19,24 +19,98 @@
import Dialog, { DialogContent, DialogFooter } from 'design/DialogConfirmation';
import { Box, Flex, ButtonPrimary, ButtonSecondary, Text } from 'design';

import React from 'react';
import React, { Suspense } from 'react';

import styled from 'styled-components';

import { Spinner } from 'design/Icon';

import * as Icons from 'design/Icon';

import { TextSelectCopyMulti } from 'teleport/components/TextSelectCopy';
import { CommandBox } from 'teleport/Discover/Shared/CommandBox';

import { useJoinTokenSuspender } from 'teleport/Discover/Shared/useJoinTokenSuspender';
import { ResourceKind, TextIcon } from 'teleport/Discover/Shared';
import { JoinToken } from 'teleport/services/joinToken';
import { CatchError } from 'teleport/components/CatchError';

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

export default function Container(props: ManualHelmDialogProps) {
return (
<CatchError
fallbackFn={fallbackProps => (
<DummyDialog error={fallbackProps.error} cancel={props.cancel} />
AntonAM marked this conversation as resolved.
Show resolved Hide resolved
)}
>
<Suspense
fallback={<DummyDialog showSpinner={true} cancel={props.cancel} />}
>
<ManualHelmDialog {...props} />
</Suspense>
</CatchError>
);
}

type DummyDialogProps = {
cancel: () => void;
error?: Error;
showSpinner?: boolean;
};

const DummyDialog = ({ error, cancel, showSpinner }: DummyDialogProps) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

i'd make smaller re-usable component blocks so we aren't duplicating the dialog parts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍 8faf471

return (
<Dialog onClose={cancel} open={true}>
<DialogContent width="850px" mb={0}>
<Text bold caps mb={4}>
Manual EKS Cluster Enrollment
</Text>
{showSpinner && (
<Flex mb={4} justifyContent="center">
<Spin>
<Spinner />
</Spin>
</Flex>
)}
{error && (
<Box mb={4}>
<TextIcon mt={3}>
<Icons.Warning size="medium" ml={1} mr={2} color="error.main" />
Encountered an error: {error.message}
</TextIcon>
</Box>
)}
</DialogContent>
<DialogFooter alignItems="center" as={Flex} gap={4}>
<ButtonPrimary width="50%" onClick={() => {}} disabled>
I ran these commands
</ButtonPrimary>
<ButtonSecondary width="50%" onClick={cancel}>
Cancel
</ButtonSecondary>
</DialogFooter>
</Dialog>
);
};

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

const command = setJoinTokenAndGetCommand(joinToken);
Copy link
Contributor

Choose a reason for hiding this comment

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

my knee jerk response is to wrap this in useEffect since setJoinTokenAndGetCommand calls a react setter (worst case this could result in infinite re-render), i'm betting this is why you get this

image

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I wanted to test the behavior of that setter used in a component body, but I couldn't get the story to work. It seems that the story doesn't caputer that anyway, because ManualHelmDialog receives setJoinTokenAndGetCommand that merely generates a command, but doesn't call a setter that updates parent's state.

useEffect would be one way to solve this is probably the only way to solve it. I wanted to suggest using another piece of state to call the setter only once (like in Storing information from previous renders. However, this would still cause one component to update the state of another component during rendering.

It seems like this works though:

Patch

Copy then pbpaste | git apply to apply.

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 7e5682ba13..39e9ad759c 100644
--- a/web/packages/teleport/src/Discover/Kubernetes/EnrollEKSCluster/Dialogs.story.tsx
+++ b/web/packages/teleport/src/Discover/Kubernetes/EnrollEKSCluster/Dialogs.story.tsx
@@ -16,7 +16,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-import React, { useEffect } from 'react';
+import React, { useEffect, useState } from 'react';
 import { MemoryRouter } from 'react-router';
 import { rest } from 'msw';
 import { mswLoader } from 'msw-storybook-addon';
@@ -27,7 +27,10 @@ import { ResourceKind } from 'teleport/Discover/Shared';
 import { PingTeleportProvider } from 'teleport/Discover/Shared/PingTeleportContext';
 import { ContextProvider } from 'teleport';
 
-import { INTERNAL_RESOURCE_ID_LABEL_KEY } from 'teleport/services/joinToken';
+import {
+  INTERNAL_RESOURCE_ID_LABEL_KEY,
+  JoinToken,
+} from 'teleport/services/joinToken';
 import { clearCachedJoinTokenResult } from 'teleport/Discover/Shared/useJoinTokenSuspender';
 import {
   DiscoverContextState,
@@ -191,6 +194,8 @@ export const ManualHelmDialogStory = () => {
     };
   }, []);
 
+  const [, setToken] = useState<JoinToken>();
+
   return (
     <MemoryRouter
       initialEntries={[
@@ -200,7 +205,12 @@ export const ManualHelmDialogStory = () => {
       <ContextProvider ctx={createTeleportContext()}>
         <DiscoverProvider mockCtx={discoverCtx}>
           <ManualHelmDialog
-            setJoinTokenAndGetCommand={() => generateCmd(helmCommandProps)}
+            setJoinTokenAndGetCommand={token => {
+              // Emulate real usage of ManualHelmDialog where setJoinTokenAndGetCommand updates the
+              // state of a parent.
+              setToken(token);
+              return generateCmd(helmCommandProps);
+            }}
             confirmedCommands={() => {}}
             cancel={() => {}}
           />
diff --git a/web/packages/teleport/src/Discover/Kubernetes/EnrollEKSCluster/EnrollEksCluster.tsx b/web/packages/teleport/src/Discover/Kubernetes/EnrollEKSCluster/EnrollEksCluster.tsx
index ccecd12b36..699e3456d7 100644
--- a/web/packages/teleport/src/Discover/Kubernetes/EnrollEKSCluster/EnrollEksCluster.tsx
+++ b/web/packages/teleport/src/Discover/Kubernetes/EnrollEKSCluster/EnrollEksCluster.tsx
@@ -16,7 +16,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-import React, { useState } from 'react';
+import React, { useState, useCallback } from 'react';
 import { Box, ButtonSecondary, ButtonText, Text, Toggle } from 'design';
 import styled from 'styled-components';
 import { FetchStatus } from 'design/DataTable/types';
@@ -258,23 +258,34 @@ export function EnrollEksCluster(props: AgentStepProps) {
     !selectedCluster ||
     enrollmentState.status !== 'notStarted';
 
-  const setJoinTokenAndGetCommand = (token: JoinToken) => {
-    setJoinToken(token);
-    return generateCmd({
-      namespace: 'teleport-agent',
-      clusterName: selectedCluster.name,
-      proxyAddr: ctx.storeUser.state.cluster.publicURL,
-      clusterVersion: ctx.storeUser.state.cluster.authVersion,
-      tokenId: token.id,
-      resourceId: token.internalResourceId,
-      isEnterprise: ctx.isEnterprise,
-      isCloud: ctx.isCloud,
-      automaticUpgradesEnabled: ctx.automaticUpgradesEnabled,
-      automaticUpgradesTargetVersion: ctx.automaticUpgradesTargetVersion,
-      joinLabels: [...selectedCluster.labels, ...selectedCluster.joinLabels],
-      disableAppDiscovery: !isAppDiscoveryEnabled,
-    });
-  };
+  const setJoinTokenAndGetCommand = useCallback(
+    (token: JoinToken) => {
+      setJoinToken(token);
+      return generateCmd({
+        namespace: 'teleport-agent',
+        clusterName: selectedCluster.name,
+        proxyAddr: ctx.storeUser.state.cluster.publicURL,
+        clusterVersion: ctx.storeUser.state.cluster.authVersion,
+        tokenId: token.id,
+        resourceId: token.internalResourceId,
+        isEnterprise: ctx.isEnterprise,
+        isCloud: ctx.isCloud,
+        automaticUpgradesEnabled: ctx.automaticUpgradesEnabled,
+        automaticUpgradesTargetVersion: ctx.automaticUpgradesTargetVersion,
+        joinLabels: [...selectedCluster.labels, ...selectedCluster.joinLabels],
+        disableAppDiscovery: !isAppDiscoveryEnabled,
+      });
+    },
+    [
+      ctx.automaticUpgradesEnabled,
+      ctx.automaticUpgradesTargetVersion,
+      ctx.isCloud,
+      ctx.isEnterprise,
+      ctx.storeUser.state.cluster,
+      isAppDiscoveryEnabled,
+      selectedCluster,
+    ]
+  );
 
   return (
     <Box maxWidth="1000px">
diff --git a/web/packages/teleport/src/Discover/Kubernetes/EnrollEKSCluster/ManualHelmDialog.tsx b/web/packages/teleport/src/Discover/Kubernetes/EnrollEKSCluster/ManualHelmDialog.tsx
index 0bde343c3c..832822c432 100644
--- a/web/packages/teleport/src/Discover/Kubernetes/EnrollEKSCluster/ManualHelmDialog.tsx
+++ b/web/packages/teleport/src/Discover/Kubernetes/EnrollEKSCluster/ManualHelmDialog.tsx
@@ -19,7 +19,7 @@
 import Dialog, { DialogContent, DialogFooter } from 'design/DialogConfirmation';
 import { Box, Flex, ButtonPrimary, ButtonSecondary, Text } from 'design';
 
-import React, { Suspense } from 'react';
+import React, { Suspense, useState, useEffect } from 'react';
 
 import styled from 'styled-components';
 
@@ -108,8 +108,13 @@ export function ManualHelmDialog({
     ResourceKind.Application,
     ResourceKind.Discovery,
   ]);
+  const [command, setCommand] = useState('');
 
-  const command = setJoinTokenAndGetCommand(joinToken);
+  useEffect(() => {
+    if (joinToken && !command) {
+      setCommand(setJoinTokenAndGetCommand(joinToken));
+    }
+  }, [joinToken, command, setJoinTokenAndGetCommand]);
 
   return (
     <Dialog onClose={cancel} open={true}>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied 👍


return (
<Dialog onClose={cancel} open={true}>
<DialogContent width="850px" mb={0}>
Expand Down Expand Up @@ -89,3 +163,17 @@ const StyledBox = styled(Box)`
padding: ${props => `${props.theme.space[3]}px`};
border-radius: ${props => `${props.theme.space[2]}px`};
`;

const Spin = styled(Box)`
Copy link
Contributor

Choose a reason for hiding this comment

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

what's different with this spinner than the spinner we already have? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

bump, we have an Indicator component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, missed this one. I just didn't find spinner component 😅 I searched for "spinner" and found only an icon and then some different css implementations sprinkled around the code. Thanks for pointing out the indicator component!

line-height: 12px;
font-size: 24px;
animation: spin 1s linear infinite;
@keyframes spin {
from {
transform: rotate(0deg);
}
to {
transform: rotate(360deg);
}
}
`;
2 changes: 0 additions & 2 deletions web/packages/teleport/src/services/integrations/types.ts
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