[WebDiscover] Determine if IAM policy setup step can be skipped (#29724)

* Prevent going back from access screen if deploying got skipped * Configuring perms for the user can take some time An error can return if a user tried to deploy before the changes are fully propagated. Notify the user that this can be a reason why an error returned and try again. * Check if IAM policy already configured with new dbs Clarify poller callback with comment Make IAMPolicy screen optional
gravitational · Aug 3, 2023 · 9f24cfa · 9f24cfa
1 parent a03a297
commit 9f24cfa
Show file tree

Hide file tree

Showing 14 changed files with 181 additions and 15 deletions.
diff --git a/web/packages/teleport/src/Discover/Database/CreateDatabase/useCreateDatabase.test.tsx b/web/packages/teleport/src/Discover/Database/CreateDatabase/useCreateDatabase.test.tsx
@@ -32,15 +32,17 @@ import {
   DatabaseEngine,
   DatabaseLocation,
 } from 'teleport/Discover/SelectResource';
+import {
+  IamPolicyStatus,
+  CreateDatabaseRequest,
+} from 'teleport/services/databases';
 
 import {
   useCreateDatabase,
   findActiveDatabaseSvc,
   WAITING_TIMEOUT,
 } from './useCreateDatabase';
 
-import type { CreateDatabaseRequest } from 'teleport/services/databases';
-
 const dbLabels = [
   { name: 'env', value: 'prod' },
   { name: 'os', value: 'mac' },
@@ -312,7 +314,7 @@ describe('registering new databases, mainly error checking', () => {
     jest.clearAllMocks();
   });
 
-  test('with matching service, activates polling', async () => {
+  test('polling until result returns (non aws)', async () => {
     const { result } = renderHook(() => useCreateDatabase(), {
       wrapper,
     });
@@ -342,6 +344,96 @@ describe('registering new databases, mainly error checking', () => {
     expect(discoverCtx.nextStep).toHaveBeenCalledWith(2);
   });
 
+  test('continue polling when poll result returns with iamPolicyStatus field set to "pending"', async () => {
+    jest.spyOn(teleCtx.databaseService, 'fetchDatabases').mockResolvedValue({
+      agents: [
+        {
+          name: 'new-db',
+          aws: { iamPolicyStatus: IamPolicyStatus.Pending },
+        } as any,
+      ],
+    });
+    const { result } = renderHook(() => useCreateDatabase(), {
+      wrapper,
+    });
+
+    await act(async () => {
+      result.current.registerDatabase(newDatabaseReq);
+    });
+    expect(teleCtx.databaseService.createDatabase).toHaveBeenCalledTimes(1);
+    expect(teleCtx.databaseService.fetchDatabaseServices).toHaveBeenCalledTimes(
+      1
+    );
+
+    // The first result will not have the aws marker we are looking for.
+    // Polling should continue.
+    await act(async () => jest.advanceTimersByTime(3000));
+    expect(teleCtx.databaseService.fetchDatabases).toHaveBeenCalledTimes(1);
+    expect(discoverCtx.updateAgentMeta).not.toHaveBeenCalled();
+
+    // Set the marker we are looking for in the next api reply.
+    jest.clearAllMocks();
+    jest.spyOn(teleCtx.databaseService, 'fetchDatabases').mockResolvedValue({
+      agents: [
+        {
+          name: 'new-db',
+          aws: { iamPolicyStatus: IamPolicyStatus.Success },
+        } as any,
+      ],
+    });
+
+    // The second poll result has the marker that should cancel polling.
+    await act(async () => jest.advanceTimersByTime(3000));
+    expect(teleCtx.databaseService.fetchDatabases).toHaveBeenCalledTimes(1);
+    expect(discoverCtx.updateAgentMeta).toHaveBeenCalledWith({
+      resourceName: 'db-name',
+      db: {
+        name: 'new-db',
+        aws: { iamPolicyStatus: IamPolicyStatus.Success },
+      },
+      serviceDeployedMethod: 'skipped',
+    });
+
+    result.current.nextStep();
+    // Skips both deploy service AND IAM policy step.
+    expect(discoverCtx.nextStep).toHaveBeenCalledWith(3);
+  });
+
+  test('stops polling when poll result returns with iamPolicyStatus field set to "unspecified"', async () => {
+    jest.spyOn(teleCtx.databaseService, 'fetchDatabases').mockResolvedValue({
+      agents: [
+        {
+          name: 'new-db',
+          aws: { iamPolicyStatus: IamPolicyStatus.Unspecified },
+        } as any,
+      ],
+    });
+    const { result } = renderHook(() => useCreateDatabase(), {
+      wrapper,
+    });
+
+    await act(async () => {
+      result.current.registerDatabase(newDatabaseReq);
+    });
+    expect(teleCtx.databaseService.createDatabase).toHaveBeenCalledTimes(1);
+    expect(teleCtx.databaseService.fetchDatabaseServices).toHaveBeenCalledTimes(
+      1
+    );
+
+    await act(async () => jest.advanceTimersByTime(3000));
+    expect(teleCtx.databaseService.fetchDatabases).toHaveBeenCalledTimes(1);
+    expect(discoverCtx.updateAgentMeta).toHaveBeenCalledWith({
+      resourceName: 'db-name',
+      db: {
+        name: 'new-db',
+        aws: { iamPolicyStatus: IamPolicyStatus.Unspecified },
+      },
+    });
+
+    result.current.nextStep();
+    expect(discoverCtx.nextStep).toHaveBeenCalledWith(2);
+  });
+
   test('when there are no services, skips polling', async () => {
     jest
       .spyOn(teleCtx.databaseService, 'fetchDatabaseServices')

diff --git a/web/packages/teleport/src/Discover/Database/CreateDatabase/useCreateDatabase.ts b/web/packages/teleport/src/Discover/Database/CreateDatabase/useCreateDatabase.ts
@@ -23,6 +23,8 @@ import { useDiscover } from 'teleport/Discover/useDiscover';
 import { usePoll } from 'teleport/Discover/Shared/usePoll';
 import { compareByString } from 'teleport/lib/util';
 import { ApiError } from 'teleport/services/api/parseError';
+import { DatabaseLocation } from 'teleport/Discover/SelectResource';
+import { IamPolicyStatus } from 'teleport/services/databases';
 
 import { matchLabels } from '../common';
 
@@ -67,6 +69,8 @@ export function useCreateDatabase() {
   //      backend error or network failure)
   const [createdDb, setCreatedDb] = useState<CreateDatabaseRequest>();
 
+  const isAws = resourceSpec.dbMeta.location === DatabaseLocation.Aws;
+
   const dbPollingResult = usePoll<DatabaseResource>(
     signal => fetchDatabaseServer(signal),
     pollActive, // does not poll on init, since the value is false.
@@ -115,11 +119,17 @@ export function useCreateDatabase() {
       resourceName: createdDb.name,
       agentMatcherLabels: dbPollingResult.labels,
       db: dbPollingResult,
+      serviceDeployedMethod:
+        dbPollingResult.aws?.iamPolicyStatus === IamPolicyStatus.Success
+          ? 'skipped'
+          : undefined, // User has to deploy a service (can be auto or manual)
     });
 
     setAttempt({ status: 'success' });
   }, [dbPollingResult]);
 
+  // fetchDatabaseServer is the callback that is run every interval by the poller.
+  // The poller will stop polling once a result returns (a dbServer).
   function fetchDatabaseServer(signal: AbortSignal) {
     const request = {
       search: createdDb.name,
@@ -129,8 +139,21 @@ export function useCreateDatabase() {
       .fetchDatabases(clusterId, request, signal)
       .then(res => {
         if (res.agents.length) {
-          return res.agents[0];
+          const dbServer = res.agents[0];
+          if (
+            !isAws || // If not AWS, then we return the first thing we get back.
+            // If AWS and aws.iamPolicyStatus is undefined or non-pending,
+            // return the dbServer.
+            dbServer.aws?.iamPolicyStatus !== IamPolicyStatus.Pending
+          ) {
+            return dbServer;
+          }
         }
+        // Returning nothing here will continue the polling.
+        // Either no result came back back yet or
+        // a result did come back but we are waiting for a specific
+        // marker to appear in the result. Specifically for AWS dbs,
+        // we wait for a non-pending flag to appear.
         return null;
       });
   }
@@ -285,6 +308,21 @@ export function useCreateDatabase() {
     emitErrorEvent(`${preErrMsg}${message}`);
   }
 
+  function handleNextStep() {
+    if (dbPollingResult) {
+      if (
+        isAws &&
+        dbPollingResult.aws?.iamPolicyStatus === IamPolicyStatus.Success
+      ) {
+        // Skips the deploy db service step AND setting up IAM policy step.
+        return nextStep(3);
+      }
+      // Skips the deploy database service step.
+      return nextStep(2);
+    }
+    nextStep(); // Goes to deploy database service step.
+  }
+
   const access = ctx.storeUser.getDatabaseAccess();
   return {
     createdDb,
@@ -298,9 +336,7 @@ export function useCreateDatabase() {
     dbLocation: resourceSpec.dbMeta.location,
     isDbCreateErr,
     prevStep,
-    // If there was a result from database polling, then
-    // allow user to skip the next step.
-    nextStep: dbPollingResult ? () => nextStep(2) : () => nextStep(),
+    nextStep: handleNextStep,
   };
 }
 

diff --git a/web/packages/teleport/src/Discover/Database/DeployService/AutoDeploy/AutoDeploy.tsx b/web/packages/teleport/src/Discover/Database/DeployService/AutoDeploy/AutoDeploy.tsx
@@ -197,6 +197,12 @@ export function AutoDeploy({ toggleDeployMethod }: DeployServiceProp) {
                     <Icons.Warning ml={1} color="error.main" />
                     Encountered Error: {attempt.statusText}
                   </TextIcon>
+                  <Text mt={2}>
+                    <b>Note:</b> If this is your first attempt, it might be that
+                    AWS has not finished propagating changes from{' '}
+                    <Mark>Step 1</Mark>. Try waiting a minute before attempting
+                    again.
+                  </Text>
                 </Box>
               )}
             </StyledBox>

diff --git a/web/packages/teleport/src/Discover/Database/IamPolicy/IamPolicy.tsx b/web/packages/teleport/src/Discover/Database/IamPolicy/IamPolicy.tsx
@@ -52,6 +52,8 @@ export function IamPolicyView({
       <HeaderSubtitle>
         Teleport needs AWS IAM permissions to be able to discover and register
         RDS instances and configure IAM authentications.
+        <br />
+        Optional if you already have an IAM policy configured.
       </HeaderSubtitle>
       {attempt.status === 'failed' ? (
         <>
@@ -115,10 +117,13 @@ export function IamPolicyView({
           )}
         </Flex>
       )}
-      <ActionButtons
-        onProceed={nextStep}
-        disableProceed={attempt.status !== 'success'}
-      />
+      <Flex>
+        <ActionButtons
+          onProceed={nextStep}
+          disableProceed={attempt.status !== 'success'}
+        />
+        <ActionButtons onSkip={() => nextStep(0)} />
+      </Flex>
     </Box>
   );
 }
diff --git a/web/packages/teleport/src/Discover/Database/SetupAccess/SetupAccess.story.tsx b/web/packages/teleport/src/Discover/Database/SetupAccess/SetupAccess.story.tsx
@@ -88,6 +88,7 @@ const props: State = {
     status: 'success',
     statusText: '',
   },
+  agentMeta: {} as any,
   onProceed: () => null,
   onPrev: () => null,
   fetchUserTraits: () => null,

diff --git a/web/packages/teleport/src/Discover/Database/SetupAccess/SetupAccess.tsx b/web/packages/teleport/src/Discover/Database/SetupAccess/SetupAccess.tsx
@@ -29,6 +29,7 @@ import {
 } from 'teleport/Discover/Shared/SetupAccess';
 import { Mark } from 'teleport/Discover/Shared';
 import { TextSelectCopyMulti } from 'teleport/components/TextSelectCopy';
+import { DbMeta } from 'teleport/Discover/useDiscover';
 
 import { DatabaseEngine, DatabaseLocation } from '../../SelectResource';
 
@@ -47,6 +48,8 @@ export function SetupAccess(props: State) {
     getFixedOptions,
     getSelectableOptions,
     resourceSpec,
+    onPrev,
+    agentMeta,
     ...restOfProps
   } = props;
   const [nameInputValue, setNameInputValue] = useState('');
@@ -105,6 +108,8 @@ export function SetupAccess(props: State) {
   const headerSubtitle =
     'Allow access from your Database names and users to interact with your Database.';
 
+  const dbMeta = agentMeta as DbMeta;
+
   return (
     <SetupAccessWrapper
       {...restOfProps}
@@ -114,6 +119,9 @@ export function SetupAccess(props: State) {
       hasTraits={hasTraits}
       onProceed={handleOnProceed}
       infoContent={<Info dbEngine={engine} dbLocation={location} />}
+      // Don't allow going back to previous screen when deploy db
+      // service got skipped or user auto deployed the db service.
+      onPrev={dbMeta.serviceDeployedMethod === 'manual' ? onPrev : null}
     >
       <Box mb={4}>
         Database Users

diff --git a/web/packages/teleport/src/Discover/Database/common.tsx b/web/packages/teleport/src/Discover/Database/common.tsx
@@ -23,9 +23,10 @@ import { LabelsCreater, Mark, TextIcon } from 'teleport/Discover/Shared';
 import { Regions } from 'teleport/services/integrations';
 
 // serviceDeployedMethod is a flag to determine if user opted to
-// deploy database service automagically (teleport deploys for user)
-// or manually (user has their own server).
-export type ServiceDeployMethod = 'auto' | 'manual';
+// deploy database service automagically (teleport deploys for user),
+// manually (user has their own server), or deploying service was
+// skipped due to an existing one.
+export type ServiceDeployMethod = 'auto' | 'manual' | 'skipped';
 
 export const Labels = ({
   labels,

diff --git a/web/packages/teleport/src/Discover/Kubernetes/SetupAccess/SetupAccess.story.tsx b/web/packages/teleport/src/Discover/Kubernetes/SetupAccess/SetupAccess.story.tsx
@@ -56,6 +56,7 @@ const props: State = {
     status: 'success',
     statusText: '',
   },
+  agentMeta: {} as any,
   onProceed: () => null,
   onPrev: () => null,
   fetchUserTraits: () => null,

diff --git a/web/packages/teleport/src/Discover/Server/SetupAccess/SetupAccess.story.tsx b/web/packages/teleport/src/Discover/Server/SetupAccess/SetupAccess.story.tsx
@@ -56,6 +56,7 @@ const props: State = {
     status: 'success',
     statusText: '',
   },
+  agentMeta: {} as any,
   onProceed: () => null,
   onPrev: () => null,
   fetchUserTraits: () => null,

diff --git a/web/packages/teleport/src/Discover/Shared/ActionButtons.tsx b/web/packages/teleport/src/Discover/Shared/ActionButtons.tsx
@@ -84,6 +84,7 @@ export const AlternateInstructionButton: React.FC<{
       onClick={onClick}
       css={`
         padding-left: 1px;
+        padding-right: 1px;
         color: ${p => p.theme.colors.buttons.link.default};
         text-decoration: underline;
         font-weight: normal;

diff --git a/web/packages/teleport/src/Discover/Shared/SetupAccess/useUserTraits.ts b/web/packages/teleport/src/Discover/Shared/SetupAccess/useUserTraits.ts
@@ -298,6 +298,7 @@ export function useUserTraits(props: AgentStepProps) {
     dynamicTraits,
     staticTraits,
     resourceSpec: props.resourceSpec,
+    agentMeta: props.agentMeta,
   };
 }
 

diff --git a/web/packages/teleport/src/services/databases/databases.test.ts b/web/packages/teleport/src/services/databases/databases.test.ts
@@ -17,7 +17,7 @@ limitations under the License.
 import api from 'teleport/services/api';
 
 import DatabaseService from './databases';
-import { Database } from './types';
+import { Database, IamPolicyStatus } from './types';
 
 test('correct formatting of database fetch response', async () => {
   jest.spyOn(api, 'get').mockResolvedValue(mockResponse);
@@ -46,6 +46,7 @@ test('correct formatting of database fetch response', async () => {
             region: 'us-west-1',
             subnets: ['sn1', 'sn2'],
           },
+          iamPolicyStatus: IamPolicyStatus.Success,
         },
       },
       {
@@ -182,6 +183,7 @@ const mockResponse = {
           region: 'us-west-1',
           subnets: ['sn1', 'sn2'],
         },
+        iam_policy_status: 'IAM_POLICY_STATUS_SUCCESS',
       },
     },
     // non-aws self-hosted

diff --git a/web/packages/teleport/src/services/databases/makeDatabase.ts b/web/packages/teleport/src/services/databases/makeDatabase.ts
@@ -34,6 +34,7 @@ export function makeDatabase(json: any): Database {
         region: aws.rds?.region,
         subnets: aws.rds?.subnets || [],
       },
+      iamPolicyStatus: aws.iam_policy_status,
     };
   }
 

diff --git a/web/packages/teleport/src/services/databases/types.ts b/web/packages/teleport/src/services/databases/types.ts
@@ -20,8 +20,18 @@ import { AgentLabel } from 'teleport/services/agents';
 
 import { AwsRdsDatabase, RdsEngine } from '../integrations';
 
+export enum IamPolicyStatus {
+  // Unspecified flag is most likely a result
+  // from an older service that do not set this state
+  Unspecified = 'IAM_POLICY_STATUS_UNSPECIFIED',
+  Pending = 'IAM_POLICY_STATUS_PENDING',
+  Failed = 'IAM_POLICY_STATUS_FAILED',
+  Success = 'IAM_POLICY_STATUS_SUCCESS',
+}
+
 export type Aws = {
   rds: Pick<AwsRdsDatabase, 'resourceId' | 'region' | 'subnets'>;
+  iamPolicyStatus: IamPolicyStatus;
 };
 
 export interface Database {