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

Web: Improve RDS flow 2 #45179

Merged
merged 9 commits into from
Aug 22, 2024
Merged

Web: Improve RDS flow 2 #45179

merged 9 commits into from
Aug 22, 2024

Conversation

kimlisa
Copy link
Contributor

@kimlisa kimlisa commented Aug 6, 2024

resolves #44368
resolves #44366

🛑 requires #45180

  • EKS flow now defaults to single enrollment (ec2 was also touched, but we currently don't use it in our flow atm)
  • Allows user to proceed to deploy step if querying for a database server times out (likely b/c database service shut down but is not expired yet from the back)
  • Allows user to re-deploy database on auto deploy step (this helps single enrollment flows being stuck on the pending state and re-deploying allows them to change their security group and subnet selections)
  • Also as discussed in discover meeting, removed custom label definer for auto deployment
  • Improve pending state hint message
  • Allows user to completely overwrite an existing database
  • Emit "create discovery config" event for other parts of flow (eks and ec2)

image

image

no change log because it's an extension of #44671 (it'll be backported together)

Copy link

github-actions bot commented Aug 6, 2024

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 changelog: followed by the changelog entries for the PR.

@@ -20,4 +20,4 @@ export const timeoutErrorMsg =
'Teleport could not detect your new database in time. Please try again.';

export const dbWithoutDbServerExistsErrorMsg =
'already exists but there are no database servers for it';
'already exists but there are no Teleport agents proxying for it';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'already exists but there are no Teleport agents proxying for it';
'already exists but there are no Teleport agents proxying it';

@kimlisa kimlisa force-pushed the lisa/improve-rds-flow-2 branch from b6a393d to ad6e13c Compare August 9, 2024 04:23
Comment on lines 55 to 64
export const AllowOverwrite = () => (
<CreateDatabaseDialog
{...props}
attempt={{ status: 'failed', statusText: dbWithoutDbServerExistsErrorMsg }}
/>
);
Copy link
Member

Choose a reason for hiding this comment

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

Is it supposed to look like this? It seems like there's some kind of identifier missing at the start of the error message.

allow-overwrite

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh that's just a story example, the actual looks like 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.

I think it'd be good to update the example then so that it's closer to the real thing.

web/packages/teleport/src/services/integrations/types.ts Outdated Show resolved Hide resolved
@kimlisa kimlisa requested a review from ravicious August 9, 2024 15:56
@kimlisa kimlisa force-pushed the lisa/improve-rds-flow-2 branch from f315cb5 to 518a22b Compare August 9, 2024 15:59
Copy link
Member

@ravicious ravicious left a comment

Choose a reason for hiding this comment

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

Thanks for addressing that nit about SuccessContent!

Comment on lines 55 to 64
export const AllowOverwrite = () => (
<CreateDatabaseDialog
{...props}
attempt={{ status: 'failed', statusText: dbWithoutDbServerExistsErrorMsg }}
/>
);
Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be good to update the example then so that it's closer to the real thing.

* Most likely cause of timeout is when we found a matching db_service
* but no db_server heartbeats. Most likely cause is because db_service
* has been stopped but is not removed from teleport yet (there is some
* minutes delay on expiry)
Copy link
Member

Choose a reason for hiding this comment

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

What's missing for me from this comment is an explanation why we show a success message even though there was a timeout. The comment explains what's the most likely cause of a timeout, but as someone who doesn't know much about the problem matter here, I don't quite understand why the user is allowed to proceed anyway.

@kimlisa kimlisa force-pushed the lisa/improve-rds-flow-2 branch from 518a22b to 897e29b Compare August 20, 2024 23:22
@kimlisa
Copy link
Contributor Author

kimlisa commented Aug 20, 2024

PTAL? @rudream 🙏

@kimlisa kimlisa force-pushed the lisa/improve-rds-flow-2 branch from 897e29b to 500a5b3 Compare August 22, 2024 05:58
@kimlisa kimlisa enabled auto-merge August 22, 2024 06:02
@kimlisa kimlisa added this pull request to the merge queue Aug 22, 2024
Merged via the queue into master with commit 5ed1222 Aug 22, 2024
39 checks passed
@kimlisa kimlisa deleted the lisa/improve-rds-flow-2 branch August 22, 2024 06:14
@public-teleport-github-review-bot

@kimlisa See the table below for backport results.

Branch Result
branch/v16 Failed

kimlisa added a commit that referenced this pull request Aug 22, 2024
* 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
kimlisa added a commit that referenced this pull request Aug 22, 2024
* 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
github-merge-queue bot pushed a commit that referenced this pull request Aug 22, 2024
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v16 no-changelog Indicates that a PR does not require a changelog entry size/md ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discover RDS database registration issues Resource enrollment flows should not default to "auto-discovery"
4 participants