Web: Improve RDS enrollment flow #44671

kimlisa · 2024-07-26T07:16:01Z

part of #44366
part of #41004

recommend reviewing by commit

🛑 requires e2e once all the required backend changes are done:

update iam permissions for describing vpcs and subnets (will also have to update the tooltip rendering the IAM perm in the web UI)
add ListVPCs to AWS OIDC integration service #44313

Improvements:

rds flow does not default to auto discovery
for self-hosted, separated configuring discovery service into it's own step
added a vpc selector before showing a list of rds databases
when opting for auto enrolling, the rds table will now show both rds instances and clusters (since auto enrolling enrolls all rds types)
added subnet selector in the auto deploying service
auto enrolling will now also allow selecting both security groups and subnets
fixed a bug where when checking for databases already enrolled for single enrollment, not all existing databases came back from the query b/c we didn't account for duplicates (which can happen when user single enrolls a db, then auto enrolls), fixed by removing limit

Test plan:

self hosted rds flow single enrollment
self hosted rds flow auto enrollment (check proper event emit)
cloud rds flow single enrollment
cloud rds flow auto enrollment (check proper event emit)
ec2 ssm self-hosted flow (since the discovery config file got touched)

Story locations for all the changes made in this PR:

configure discovery service: https://localhost:9002/?path=/story/teleport-discover-shared-configurediscoveryservice--server
enrolling with vpc selector: https://localhost:9002/?path=/story/teleport-discover-database-enrollrds--self-hosted-flow
auto deploy with subnet & security group: https://localhost:9002/?path=/story/teleport-discover-database-deploy-auto--init

changelog: Improve web UI enroll RDS flow where VPC, subnets, and security groups are now selectable

public-teleport-github-review-bot · 2024-07-26T07:16:30Z

@kimlisa - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes.

ravicious

I reviewed the first three commits, I'll continue the review tomorrow.

web/packages/teleport/src/services/integrations/fetchAllAwsRdsEnginesDatabase.test.ts

web/packages/teleport/src/services/integrations/integrations.ts

web/packages/teleport/src/Discover/Database/DeployService/AutoDeploy/SelectSecurityGroups.tsx

web/packages/teleport/src/Discover/Database/DeployService/AutoDeploy/SelectSubnetIds.tsx

...ackages/teleport/src/Discover/Shared/ConfigureDiscoveryService/ConfigureDiscoveryService.tsx

...ages/teleport/src/Discover/Shared/ConfigureDiscoveryService/CreatedDiscoveryConfigDialog.tsx

ravicious

Looks good, I left more minor comments, mostly around the use of useEffect.

There is a bunch of logic in there that seems to be related to the subject matter (deploying stuff on AWS) that I didn't verify very thoroughly.

web/packages/teleport/src/services/integrations/fetchAllAwsRdsEnginesDatabase.test.ts

ravicious · 2024-07-30T12:51:51Z

...ages/teleport/src/Discover/Shared/ConfigureDiscoveryService/CreatedDiscoveryConfigDialog.tsx

-        <Flex>
-          <ButtonPrimary mr={3} width="50%" onClick={retry}>
+        <Flex gap={3} width="100%">
+          <ButtonPrimary style={{ flex: 1 }} onClick={retry}>


Ah damn, I didn't realize <Button> doesn't support flex as an arg. Long term it'd definitely be a good idea to add this.

web/packages/teleport/src/Discover/Database/EnrollRdsDatabase/VpcSelector.tsx

web/packages/teleport/src/Discover/Database/EnrollRdsDatabase/AutoEnrollment.tsx

ravicious · 2024-07-30T13:33:23Z

web/packages/teleport/src/Discover/Database/DeployService/AutoDeploy/AutoDeploy.tsx

+    if (selectedSubnetIds.length === 0) {
+      setHasNoSubnets(true);
+      return;
+    } else {
+      setHasNoSubnets(false);
+    }


It feels like long term it'd be good to have a way to include this validation within validator.validate(). Otherwise we're going to end up with dozens of places doing such validations by hand while they could be using a dedicated mechanism for this instead.

web/packages/teleport/src/Discover/Database/CreateDatabase/useCreateDatabase.ts

...ackages/teleport/src/Discover/Shared/ConfigureDiscoveryService/ConfigureDiscoveryService.tsx

web/packages/teleport/src/Discover/Database/DeployService/AutoDeploy/SelectSubnetIds.tsx

ravicious · 2024-07-30T13:46:46Z

web/packages/teleport/src/Discover/Database/EnrollRdsDatabase/AutoDiscoverToggle.tsx

+      <Toggle
+        isToggled={wantAutoDiscover}
+        onToggle={toggleWantAutoDiscover}
+        disabled={disabled}
+      >


There's a lot of flashing when this toggle is clicked. For example, I shouldn't see "No results" for a split second when switching between toggle states. Presumably it's because of those extra effects used in AutoEnrollment and SingleEnrollment that I mentioned. Maybe addressing them will make the UI feel more stable as well.

i made it less jumpy, but couldn't resolve why no results keep showing up before the actual data, it's strange, these are the render ticks. the table for some reason gets an empty item first, then the data comes on next render...

it's like setDataTable gets partially updated (because the fetchStatus: disabled is updated but not the items)...

web/packages/teleport/src/Discover/Database/EnrollRdsDatabase/AutoEnrollment.tsx

ravicious · 2024-07-31T12:24:36Z

web/packages/teleport/src/Discover/Database/EnrollRdsDatabase/EnrollRdsDatabase.tsx

+        />
+      )}
+      {showVpcSelector && !hasVpcs && (
+        <P mt={-3}>


No worries, it's a minor thing in the grand scheme of things. It only becomes a problem when you want to reuse the component in different contexts where the hardcoded margins might not be necessary.

web/packages/teleport/src/services/integrations/integrations.ts

web/packages/teleport/src/Discover/Database/DeployService/AutoDeploy/SelectSubnetIds.tsx

kimlisa · 2024-08-01T00:07:30Z

ping @rudream

ravicious · 2024-08-01T14:58:06Z

web/packages/teleport/src/Discover/Database/DeployService/AutoDeploy/SelectSubnetIds.tsx

-              </P3>
-            )}
+            <P3>
+              {`${selectedSubnetIds.length} ${pluralize(selectedSubnetIds.length, 'subnet')} selected`}


FYI, I just submitted this small PR which fixes my mistake in pluralize. ;) #44949

Since the count is no longer hidden behind a zero check, AFAIK it should say "0 subnets selected" instead of "0 subnet selected".

GavinFrazar · 2024-08-01T16:20:41Z

web/packages/teleport/src/Discover/Database/DeployService/AutoDeploy/SelectSecurityGroups.tsx

+            The security groups you pick must meet the following requirements:
+            <ul>
+              <li>
+                all security groups must allow outbound connectivity (eg:
+                0.0.0.0/0) to reach Teleport cluster
+              </li>
+              <li>
+                all security groups must allow inbound connectivity from the ECS
+                task security group or from the ECS task subnet(s) CIDR blocks
+              </li>
+            </ul>


Suggested change

The security groups you pick must meet the following requirements:

<ul>

<li>

all security groups must allow outbound connectivity (eg:

0.0.0.0/0) to reach Teleport cluster

</li>

<li>

all security groups must allow inbound connectivity from the ECS

task security group or from the ECS task subnet(s) CIDR blocks

</li>

</ul>

Select security group(s) based on the following requirements:

<ul>

<li>

The selected security group(s) must allow all outbound traffic

(eg: 0.0.0.0/0)

</li>

<li>

A security group attached to your database(s) must allow inbound

traffic from a security group you select or from all IPs in the

subnets you selected

</li>

Security groups are additive, so only one of the ECS task security groups needs to allow (0.0.0.0/0) outbound.
No inbound rules are needed for these either, but the databases' security groups need to allow inbound from the ECS task.

GavinFrazar · 2024-08-01T16:44:21Z

web/packages/teleport/src/Discover/Database/DeployService/AutoDeploy/SelectSecurityGroups.tsx

+        running the database access agent. If you don't select any security
+        groups, the default one for the VPC will be used.


Could we make security group selection a requirement instead of optional?

I think it will be better to make the user think about and pick a security group than let them skip an optional step and then likely have to troubleshoot a failing deployment

GavinFrazar · 2024-08-01T16:47:25Z

web/packages/teleport/src/Discover/Database/DeployService/AutoDeploy/SelectSubnetIds.tsx

+
+      <Text mb={2}>
+        Select subnets to assign to the Fargate service that will be running the
+        database access agent. All of the subnets you select must have an


Suggested change

database access agent. All of the subnets you select must have an

Teleport Database Service. All of the subnets you select must have an

rudream · 2024-08-01T19:14:33Z

web/packages/teleport/src/Discover/Database/EnrollRdsDatabase/AutoDiscoverToggle.tsx

+        disabled={disabled}
+      >
+        <Box ml={2} mr={1}>
+          Auto-enroll all databases for selected region


Suggested change

Auto-enroll all databases for selected region

Auto-enroll all databases for the selected region

r0mant

🎉

- user selects a vpc to see RDS's - restrict deploying service to user selected vpc - allow and require selecting subnets and security groups - refactor EnrollRdsDatabase.tsx into SingleEnrollment and AutoEnrollment - for self hosted, move configuring discovery config into own step - fixes a query bug where not all db servers were returning because possible duplicates were not accounted for (fixed by removing limit, default limit is 1k which should be plenty)

public-teleport-github-review-bot · 2024-08-01T21:54:45Z

@kimlisa See the table below for backport results.

Branch	Result
branch/v16	Failed

- user selects a vpc to see RDS's - restrict deploying service to user selected vpc - allow and require selecting subnets and security groups - refactor EnrollRdsDatabase.tsx into SingleEnrollment and AutoEnrollment - for self hosted, move configuring discovery config into own step - fixes a query bug where not all db servers were returning because possible duplicates were not accounted for (fixed by removing limit, default limit is 1k which should be plenty)

* Web: Improve RDS enrollment flow (#44671) - user selects a vpc to see RDS's - restrict deploying service to user selected vpc - allow and require selecting subnets and security groups - refactor EnrollRdsDatabase.tsx into SingleEnrollment and AutoEnrollment - for self hosted, move configuring discovery config into own step - fixes a query bug where not all db servers were returning because possible duplicates were not accounted for (fixed by removing limit, default limit is 1k which should be plenty) * Web: Improve RDS flow 2 (#45179) * Don't default to auto discover * Allow redpeploy when auto deploying and improve pending deploy hints * Allow overwrite existing dbs and skip on heartbeat timeout during enrollment * Remove allowing custom labels * Emit rest of discovery config event * Remove blocking if db_service exists for a vpc Previously intended to determine if user already auto enrolled, but now that single and auto enrollment sets the same kind of labels for db_service, there is no distinction and can let user be blocked incorrectly.

github-actions bot requested review from ravicious and rudream July 26, 2024 07:16

This comment was marked as resolved.

Sign in to view

github-actions bot added size/xl ui labels Jul 26, 2024

ravicious reviewed Jul 29, 2024

View reviewed changes

ravicious self-requested a review July 29, 2024 13:27

GavinFrazar mentioned this pull request Jul 29, 2024

update aws oidc db enrollment role permissions #44717

Merged

kimlisa force-pushed the lisa/improve-rds-flow branch from fd78099 to 852298b Compare July 30, 2024 06:05

ravicious reviewed Jul 30, 2024

View reviewed changes

kimlisa requested a review from ravicious July 31, 2024 07:52

ravicious approved these changes Jul 31, 2024

View reviewed changes

kimlisa force-pushed the lisa/improve-rds-flow branch from 6ea394c to 9649082 Compare July 31, 2024 20:30

ravicious reviewed Aug 1, 2024

View reviewed changes

GavinFrazar reviewed Aug 1, 2024

View reviewed changes

rudream approved these changes Aug 1, 2024

View reviewed changes

kimlisa force-pushed the lisa/improve-rds-flow branch from 9649082 to 561b5eb Compare August 1, 2024 20:46

kimlisa added the backport/branch/v16 label Aug 1, 2024

kimlisa force-pushed the lisa/improve-rds-flow branch from 561b5eb to 93641ef Compare August 1, 2024 20:56

r0mant approved these changes Aug 1, 2024

View reviewed changes

kimlisa force-pushed the lisa/improve-rds-flow branch from 93641ef to 584821c Compare August 1, 2024 21:16

kimlisa force-pushed the lisa/improve-rds-flow branch from 584821c to 6c96447 Compare August 1, 2024 21:32

kimlisa added this pull request to the merge queue Aug 1, 2024

Merged via the queue into master with commit c9489b5 Aug 1, 2024
38 checks passed

kimlisa deleted the lisa/improve-rds-flow branch August 1, 2024 21:53

kimlisa mentioned this pull request Aug 6, 2024

Web: Improve RDS flow 2 #45179

Merged

kimlisa mentioned this pull request Aug 22, 2024

[v16] WebDiscover: Improve RDS enrollment flow #45688

Merged

5 tasks

GavinFrazar mentioned this pull request Aug 26, 2024

Allow Subnet and SecurityGroup selection when using Discover Auto-Enroll RDS flow (AWS OIDC Integration) #40174

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Web: Improve RDS enrollment flow #44671

Web: Improve RDS enrollment flow #44671

kimlisa commented Jul 26, 2024 •

edited

Loading

public-teleport-github-review-bot bot commented Jul 26, 2024

This comment was marked as resolved.

ravicious left a comment

ravicious left a comment

ravicious Jul 30, 2024

ravicious Jul 30, 2024

ravicious Jul 30, 2024

kimlisa Jul 31, 2024

ravicious Jul 31, 2024

kimlisa commented Aug 1, 2024

ravicious Aug 1, 2024

GavinFrazar Aug 1, 2024

GavinFrazar Aug 1, 2024

GavinFrazar Aug 1, 2024

rudream Aug 1, 2024

r0mant left a comment

public-teleport-github-review-bot bot commented Aug 1, 2024

-            The security groups you pick must meet the following requirements:
-            <ul>
-              <li>
-                all security groups must allow outbound connectivity (eg:
-.0.0.0/0) to reach Teleport cluster
-              </li>
-              <li>
-                all security groups must allow inbound connectivity from the ECS
-                task security group or from the ECS task subnet(s) CIDR blocks
-              </li>
-            </ul>
+            Select security group(s) based on the following requirements:
+            <ul>
+              <li>
+                The selected security group(s) must allow all outbound traffic
+                (eg: 0.0.0.0/0)
+              </li>
+              <li>
+                A security group attached to your database(s) must allow inbound
+                traffic from a security group you select or from all IPs in the
+                subnets you selected
+              </li>

		running the database access agent. If you don't select any security
		groups, the default one for the VPC will be used.

	database access agent. All of the subnets you select must have an
	Teleport Database Service. All of the subnets you select must have an

	Auto-enroll all databases for selected region
	Auto-enroll all databases for the selected region

Web: Improve RDS enrollment flow #44671

Web: Improve RDS enrollment flow #44671

Conversation

kimlisa commented Jul 26, 2024 • edited Loading

public-teleport-github-review-bot bot commented Jul 26, 2024

This comment was marked as resolved.

ravicious left a comment

Choose a reason for hiding this comment

ravicious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimlisa commented Aug 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r0mant left a comment

Choose a reason for hiding this comment

public-teleport-github-review-bot bot commented Aug 1, 2024

kimlisa commented Jul 26, 2024 •

edited

Loading