-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Conversation
This token is used only for manul enrollment, automatic one generates token on the backend.
6dd1d66
to
c05349b
Compare
The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with |
@@ -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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comment 👍 55f8214
clusterVersion: ctx.storeUser.state.cluster.authVersion, | ||
resourceId: joinToken.internalResourceId, | ||
tokenId: '', // Filled in by the ManualHelmDialog. | ||
resourceId: '', |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
worker.resetHandlers(); | ||
|
||
useEffect(() => { | ||
// Clean up | ||
return () => { | ||
clearCachedJoinTokenResult([ | ||
ResourceKind.Kubernetes, | ||
ResourceKind.Application, | ||
ResourceKind.Discovery, | ||
]); | ||
}; | ||
}, []); |
There was a problem hiding this comment.
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={[
There was a problem hiding this comment.
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
@ravicious @kimlisa PTAL |
joinToken: joinToken.id, | ||
resourceId: joinToken.internalResourceId, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
showSpinner?: boolean; | ||
}; | ||
|
||
const DummyDialog = ({ error, cancel, showSpinner }: DummyDialogProps) => { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍 8faf471
web/packages/teleport/src/Discover/Kubernetes/EnrollEKSCluster/ManualHelmDialog.tsx
Outdated
Show resolved
Hide resolved
@@ -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)` |
There was a problem hiding this comment.
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? 🤔
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
ResourceKind.Discovery, | ||
]); | ||
|
||
const command = setJoinTokenAndGetCommand(joinToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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}>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after the spinner comment lgtm!
@@ -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)` |
There was a problem hiding this comment.
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?
When enrolling EKS clusters through Discover UI user was asked to confirm admin action with MFA immediately after selecting integration step, because joinToken for the manual fallback was generated then. This PR moves it to the manual dialog itself, so user is asked to confirm token generation only if they open the dialog.
Changelog: Don't require MFA approval in the beginning of EKS Discover flow.