Skip to content

WebDiscover: Hookup AWS RDS Flow#24873

Merged
kimlisa merged 14 commits intomasterfrom
lisa/hookup-rds-flow
Apr 25, 2023
Merged

WebDiscover: Hookup AWS RDS Flow#24873
kimlisa merged 14 commits intomasterfrom
lisa/hookup-rds-flow

Conversation

@kimlisa
Copy link
Copy Markdown
Contributor

@kimlisa kimlisa commented Apr 20, 2023

part of #22130

  • Implement logic for resuming discover flow from where user left of
  • Replaces the old aws rds flow with the new rds flow (connect aws integration and enroll rds databases)
  • Enabled integration resource access
  • Fixed a db agent label matching bug

@marcoandredinis
Copy link
Copy Markdown
Contributor

We are listing RDS DB Instances and RDS DB Clusters, so the role must include the following policy:

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Action": [
                "rds:DescribeDBInstances",
                "rds:DescribeDBClusters"
            ]
            "Resource": "*"
        }
    ]
}

@marcoandredinis
Copy link
Copy Markdown
Contributor

When setting up the Integration we should strip out the :443 from the Provider URL
I think it gives us a better UX
If the port is anything != then :443 then we should add it 👍
If we decide to keep the :443, then we must change this method to not strip it

func (h *Handler) issuerFromPublicAddr() (string, error) {
addr := h.cfg.PublicProxyAddr
// Add protocol if not present.
if !strings.HasPrefix(addr, "https://") {
addr = "https://" + addr
}
result, err := url.Parse(addr)
if err != nil {
return "", trace.Wrap(err)
}
if result.Port() == "443" {
// Cut off redundant :443
result.Host = result.Hostname()
}
return result.String(), nil
}

@kimlisa kimlisa force-pushed the lisa/hookup-rds-flow branch 3 times, most recently from f4630d4 to fb1365e Compare April 21, 2023 16:19
@kimlisa kimlisa requested a review from ryanclark April 21, 2023 16:24
@kimlisa kimlisa marked this pull request as ready for review April 21, 2023 16:24
@github-actions github-actions Bot requested review from avatus and ravicious April 21, 2023 16:25
@kimlisa kimlisa requested review from marcoandredinis and removed request for ravicious April 21, 2023 16:25
Base automatically changed from lisa/finish-up-enroll-db-screen to master April 24, 2023 20:03
kimlisa added 11 commits April 24, 2023 13:08
When storing state into location URL, it doesn't allow storing
ReactElement, so I changed the icon element into string that
refers to the correct icon. Also adds rds aurora tiles to
Select Resources screen.
Helpful when user makes changes to the RDS instance
(eg. tags) and needs to get the most up to date listing
Previously we required the agent matcher labels
be an exact match of registered db labels otherwise we
prevented the user from deploying an agent (which was wrong).

Now the only requirement is that the matcher labels are all
able to match against registered db labels.
@kimlisa kimlisa force-pushed the lisa/hookup-rds-flow branch from 1c16a75 to ccc5575 Compare April 24, 2023 20:09
- Make label matching error less confusing by showing
  error upon user trying to generate command
- Make label messaging clearer
- Emit errors when failing to fetch rds dbs
@kimlisa kimlisa force-pushed the lisa/hookup-rds-flow branch from ccc5575 to b1217f8 Compare April 25, 2023 03:05
@kimlisa
Copy link
Copy Markdown
Contributor Author

kimlisa commented Apr 25, 2023

@ryanclark friendly ping

@kimlisa
Copy link
Copy Markdown
Contributor Author

kimlisa commented Apr 25, 2023

@avatus friendly ping

Copy link
Copy Markdown
Contributor

@avatus avatus left a comment

Choose a reason for hiding this comment

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

Didn't get to test locally yet but the code looks great. Just a few nits

Comment on lines +262 to +265
You can define your own labels that this database service will use
to identify your registered database. The labels you define must
match with the labels that was defined for the registered database
(from previous step):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
You can define your own labels that this database service will use
to identify your registered database. The labels you define must
match with the labels that was defined for the registered database
(from previous step):
You can define your own labels that this database service will use
to identify your registered database. The labels you define must
match the labels that were defined for the registered database
(from previous step):

Comment on lines +294 to +295
The matcher labels must be able to match with the labels defined for
the registered database. Use wildcards to match by any labels.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
The matcher labels must be able to match with the labels defined for
the registered database. Use wildcards to match by any labels.
The matcher labels must be able to match with the labels defined for
the registered database. Use wildcards to match with any labels.

</Link>{' '}
in order to be used with Database Access for RDS. To enable, users
must have a <Mark>rds_iam</Mark> role:
Users must have a <Mark>rds_iam</Mark> role:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know this said "a" rds_iam role previously, and "a" is correct if we removed the role name and just said "a role". But it reads weird and if this will always be rds_iam here, I think we should change to Users must have an rds_iam role". However, feel free to skip this as i can see it working both ways

// and then coming back to resume the flow.)
export type DiscoverUrlLocState = {
// discover contains the fields necessary to be able to resume
// the flow from where user left of.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
// the flow from where user left of.
// the flow from where user left off.

// react routes (eg. in RDS database flow, it is required of user
// to create a AWS OIDC integration which requires changing route
// and then coming back to resume the flow.)
export type DiscoverUrlLocState = {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should change this to DiscoverUrlLocationState, just to be more clear. When we are working with it and setting it above, it's called locationState so makes more sense to keep it that way.

@kimlisa kimlisa requested a review from avatus April 25, 2023 19:04
@kimlisa kimlisa enabled auto-merge April 25, 2023 19:51
@kimlisa kimlisa added this pull request to the merge queue Apr 25, 2023
Merged via the queue into master with commit 61d0aaa Apr 25, 2023
@kimlisa kimlisa deleted the lisa/hookup-rds-flow branch April 25, 2023 20:11
@public-teleport-github-review-bot
Copy link
Copy Markdown

@kimlisa See the table below for backport results.

Branch Result
branch/v13 Failed

kimlisa added a commit that referenced this pull request May 1, 2023
* Clean up aws oidc integration instructions

* Change ResourceSpec icon type from ReactElement to string

When storing state into location URL, it doesn't allow storing
ReactElement, so I changed the icon element into string that
refers to the correct icon. Also adds rds aurora tiles to
Select Resources screen.

* Fix the expected backend aws status value for RDS list

* For RDS list, allow refreshing the table

Helpful when user makes changes to the RDS instance
(eg. tags) and needs to get the most up to date listing

* Update rds db setup access text info

* Make create database dialog more consistent btwn states

* Fix label matching

Previously we required the agent matcher labels
be an exact match of registered db labels otherwise we
prevented the user from deploying an agent (which was wrong).

Now the only requirement is that the matcher labels are all
able to match against registered db labels.

* Implement resuming discover flow from where user left of

* Enable integration access and rds flow

* Strip 443 ports from cluster uri

* Use the labels returned from polling db instaed

* Various touch ups

- Make label matching error less confusing by showing
  error upon user trying to generate command
- Make label messaging clearer
- Emit errors when failing to fetch rds dbs

* Address CR and update test
kimlisa added a commit that referenced this pull request May 1, 2023
* Clean up aws oidc integration instructions

* Change ResourceSpec icon type from ReactElement to string

When storing state into location URL, it doesn't allow storing
ReactElement, so I changed the icon element into string that
refers to the correct icon. Also adds rds aurora tiles to
Select Resources screen.

* Fix the expected backend aws status value for RDS list

* For RDS list, allow refreshing the table

Helpful when user makes changes to the RDS instance
(eg. tags) and needs to get the most up to date listing

* Update rds db setup access text info

* Make create database dialog more consistent btwn states

* Fix label matching

Previously we required the agent matcher labels
be an exact match of registered db labels otherwise we
prevented the user from deploying an agent (which was wrong).

Now the only requirement is that the matcher labels are all
able to match against registered db labels.

* Implement resuming discover flow from where user left of

* Enable integration access and rds flow

* Strip 443 ports from cluster uri

* Use the labels returned from polling db instaed

* Various touch ups

- Make label matching error less confusing by showing
  error upon user trying to generate command
- Make label messaging clearer
- Emit errors when failing to fetch rds dbs

* Address CR and update test
kimlisa added a commit that referenced this pull request May 4, 2023
kimlisa added a commit that referenced this pull request May 6, 2023
kimlisa added a commit that referenced this pull request May 6, 2023
* WebDiscover: Create Enroll a RDS Database Screen (#24509)

* WebDiscover: Finish implementing Enroll Database Screen (#24710)

* WebDiscover: Hookup AWS RDS Flow (#24873)

* WebDiscover: Put back IamPolicy screen and some tweaks (#25438)

* Remove unused get url

* Update snapshot
@r0mant r0mant mentioned this pull request Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants