Integrations: AWS OIDC - ListDatabases action by marcoandredinis · Pull Request #24460 · gravitational/teleport

marcoandredinis · 2023-04-12T17:22:35Z

This PR adds a new Action for the AWSOIDC Integration: ListDatabases
The goal of this action is to provide the User a list of RDS Databases
from which the User will pick one to be added as a Teleport Database
Resource.
This way, the user doesn't need to copy/paste DB name, endpoints and
labels.

Before being able to call this action, the User has to set up an AWS
OIDC integration.

How does it work:

                    Client (web app)
                     │     ▲
                     │     │4. Returns list of DBs
 1. List Databases   │     │   (name, tags, status, endpoint)
                     │     │
                     │     │
                     │     │
                     │     │
                     ▼     │       3. rds.DescribeDBInstances
               ┌───────────┴────┐     (auth: token)                ┌─────────┐
               │                ├─────────────────────────────────►│         │
               │ Teleport Proxy │                                  │   AWS   │
               │                │     3.1. Get OIDC Config         │         │
               │                │◄─────────────────────────────────┤         │
               │                │     3.2. Get RSA Public Key      │         │
               │                │◄─────────────────────────────────┤         ├─────────┐ 3.3.
               │                │                                  │         │         │Validates token signature
               │                │                                  │         │         │with received public key
               │                │   3.4 Returns list of DBs        │         │◄────────┘
               │                │◄─────────────────────────────────┤         │
               └─┬──────────────┘                                  └─────────┘
                 │
                 │  2. Sign Token
                 │
                 ▼
              ┌───────────────────┐
              │                   │
              │   Teleport Auth   │
              │  RSA Private Key  │
              └───────────────────┘

Eg of an HTTP response from 4.

{
   "databases":[
      {
         "name":"marcodb-test",
         "desc":"RDS instance in us-east-1",
         "protocol":"postgres",
         "type":"rds",
         "labels":[
            {
               "name":"account-id",
               "value":"123456789012"
            },
            {
               "name":"canBeDeleted",
               "value":"yes"
            },
            {
               "name":"endpoint-type",
               "value":"instance"
            },
            {
               "name":"engine",
               "value":"postgres"
            },
            {
               "name":"engine-version",
               "value":"14.6"
            },
            {
               "name":"env",
               "value":"test"
            },
            {
               "name":"project",
               "value":"discover"
            },
            {
               "name":"region",
               "value":"us-east-1"
            },
            {
               "name":"status",
               "value":"available"
            },
            {
               "name":"teleport.dev/origin",
               "value":"cloud"
            }
         ],
         "hostname":"marcodb-test.abcdef123456.us-east-1.rds.amazonaws.com",
         "uri":"marcodb-test.abcdef123456.us-east-1.rds.amazonaws.com:5432",
         "aws":{
            "region":"us-east-1",
            "rds":{
               "instance_id":"marcodb-test",
               "resource_id":"db-123456ABCEDF123456ABCEDF00",
               "iam_auth":false
            },
            "account_id":"123456789012",
            "status":"available"
         }
      }
   ]
}

public-teleport-github-review-bot · 2023-04-17T11:33:49Z

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

marcoandredinis · 2023-04-17T11:37:19Z

@kimlisa @r0mant I can split up the PR
It's not that much production code, but comments, tests and proto auto generated code, leads to a lot of changes

r0mant

Nice job on the test coverage here 👍

Left a few suggestions on reusing existing resource representations and simplifying the API a bit.

r0mant · 2023-04-17T15:13:43Z

Can we just convert it to types.Database resource, we already have a function for that somewhere? Why add another representation of the database. You can just add missing fields (like status) to our resource for example as labels or something.

We are using aws go sdk v1 for rds api everywhere else

teleport/lib/cloud/aws/tags_helpers.go

Line 23 in 05aa048

"github.com/aws/aws-sdk-go/service/rds"

I don't think I can re-use such helpers

I decided to use v2, so most things will not be compatible.
Now, I'm wondering if I should switch to the v1 of the go sdk 🤔

I've created new helpers using the aws sdk go v2 👍

r0mant · 2023-04-17T15:17:48Z

Why did you decide to go with this approach of having a generic "execute action" API instead of just having an API specifically for fetching RDS databases/clusters? I'm just thinking this feels like trying to design another "protocol" on top of our API so the API response can also be different based on the "action" you're requesting, etc.

I'd go for a simpler approach with just separate handlers, would be easier to see all API surface as well based on the registered handlers. What do you think?

Sure 👍
I'll create one endpoint per action.
So in this case we'll have

h.POST("/webapi/sites/:site/integrations/:name/action/list_databases", h.WithClusterAuth(h.awsOIDCListDatabases))

Why do we need "action" at all? Could we just do

/webapi/sites/:site/integrations/aws/:name/databases

Btw, I'd also add /aws to the path to be able to differentiate between handlers for different integrations.

Looks good 👍
I've just replace aws with aws-oidc because that's the name we are using everywhere

r0mant · 2023-04-17T15:19:17Z

Similar question as above about these new resources, can web UI use exiting database types we already have defined? cc @kimlisa Just trying to avoid having too many representations of the same thing.

i think it makes sense to me to be explicit like this, a type that tells me that this isn't a teleport database resource

that said, we can re-use the existing web/ui database type (linked below) and extend it to include fields unique to the aws database if we really want to (in the end it doesn't matter much as long as the front get the values we need b/c the frontend will customize it further to suite the needs of the component that needs it anyways):

teleport/lib/web/ui/server.go

Line 227 in e2df4a3

// Database describes a database server.

no strong opinions

I've added all the types.AWS fields and the Status as well (which comes from a label).

kimlisa · 2023-04-18T20:37:59Z

i'm confused why this type is necessary? can't we use the pb type from the get go? this is what i see in other places, so i am not sure which method/pattern to stick with (or what makes this one the exception)

I did it only to implement the CheckAndSetDefaults
The pb type is created here: api/gen/proto/go/teleport/integration/v1/integration_service.pb.go
I was trying to avoid adding custom code close in those auto generated folders.

kimlisa · 2023-04-18T21:17:25Z

think you meant to put this on top of the mockCAGetter that it is used to implement the CAGetter interface?

Yes! Thank you.

kimlisa · 2023-04-18T21:58:18Z

i'm sorry if i'm butchering my understanding, but regarding Multi-AZ DB clusters, it sounds like for example, if i want a complete list of ALL mysql rds items (whether that's instance or cluster), i would need to query twice?

So I need to make two api calls like this:

first request: get mysql instances

rdsType: 'instance', engines: ['mysql', 'mariadb'],

second request: get mysql clusters

rdsType: 'cluster', engines: ['mysql', 'mariadb'],

or instead of combining all of this into one list, would it make more sense to do a dropdown menu and let user choose if they want a list of instances? or list of clusters?

I think we must have separate tiles for each.
One for Aurora/Cluster MySQL/MariaDB and another one for RDS MySQL/MariaDB.
Looking at the designs, we have them together as AWS | RDS & Aurora
I'll ask the design team if that's ok to have them apart

The issue here is pagination
AWS provides an API for instances and another one for clusters
When doing pagination we only have one nextToken/nextKey/marker and that is either going to be used for Instances or Clusters.
We can make things work ™️ , but we'll need another abstraction

This PR adds a new Action for the AWSOIDC Integration: ListDatabases The goal of this action is to provide the User a list of RDS Databases from which the User will pick one to be added as a Teleport Database Resource. This way, the user doesn't need to copy/paste DB name, endpoints and labels. Before being able to call this action, the User has to set up an AWS OIDC integration. How does it work: ``` Client (web app) │ ▲ │ │4. Returns list of DBs 1. List Databases │ │ (name, tags, status, endpoint) │ │ │ │ │ │ │ │ ▼ │ 3. rds.DescribeDBInstances ┌───────────┴────┐ (auth: token) ┌─────────┐ │ ├─────────────────────────────────►│ │ │ Teleport Proxy │ │ AWS │ │ │ 3.1. Get OIDC Config │ │ │ │◄─────────────────────────────────┤ │ │ │ 3.2. Get RSA Public Key │ │ │ │◄─────────────────────────────────┤ ├─────────┐ 3.3. │ │ │ │ │Validates token signature │ │ │ │ │with received public key │ │ 3.4 Returns list of DBs │ │◄────────┘ │ │◄─────────────────────────────────┤ │ └─┬──────────────┘ └─────────┘ │ │ 2. Sign Token │ ▼ ┌───────────────────┐ │ │ │ Teleport Auth │ │ RSA Private Key │ └───────────────────┘ ```

public-teleport-github-review-bot · 2023-04-19T16:05:21Z

@marcoandredinis See the table below for backport results.

Branch	Result
branch/v12	Failed
branch/v13	Create PR

kimlisa mentioned this pull request Apr 13, 2023

WebDiscover: Create Enroll a RDS Database Screen #24509

Merged

marcoandredinis force-pushed the marco/integration_awsoidc branch 8 times, most recently from bc53c4d to ef52fa7 Compare April 13, 2023 17:09

kimlisa reviewed Apr 14, 2023

View reviewed changes

Comment thread lib/web/ui/integration.go Outdated

marcoandredinis force-pushed the marco/integration_awsoidc branch 8 times, most recently from 55e0ec1 to 6e53f6d Compare April 17, 2023 11:33

marcoandredinis requested review from kimlisa and r0mant April 17, 2023 11:33

marcoandredinis marked this pull request as ready for review April 17, 2023 11:33

github-actions Bot requested a review from nklaassen April 17, 2023 11:33

github-actions Bot added the size/xl label Apr 17, 2023

marcoandredinis removed the request for review from nklaassen April 17, 2023 11:36

marcoandredinis force-pushed the marco/integration_awsoidc branch 2 times, most recently from add3077 to 0702e0f Compare April 17, 2023 14:26

r0mant reviewed Apr 17, 2023

View reviewed changes

marcoandredinis force-pushed the marco/integration_awsoidc branch from 0702e0f to 75f96f2 Compare April 17, 2023 18:32

marcoandredinis force-pushed the marco/integration_awsoidc branch 8 times, most recently from f7e7d90 to d858b7e Compare April 18, 2023 15:53

marcoandredinis requested a review from r0mant April 18, 2023 16:44

r0mant approved these changes Apr 18, 2023

View reviewed changes

kimlisa approved these changes Apr 19, 2023

View reviewed changes

marcoandredinis force-pushed the marco/integration_awsoidc branch 2 times, most recently from a325e40 to 016ebe7 Compare April 19, 2023 08:43

marcoandredinis added 8 commits April 19, 2023 10:53

add resource and account ids to DB resource

b557b02

move api namespaces

195b139

use types.Database instead of custom database format

8d45b7d

add database uri

6a70b96

fix comments and rate limiter

babd36b

test name override when converting RDS V2 DBs

b2c240f

fix webapi database URI field

cb5a180

marcoandredinis force-pushed the marco/integration_awsoidc branch from 016ebe7 to cb5a180 Compare April 19, 2023 09:55

TestClusterDatabasesGet: add parallel

13c2df5

marcoandredinis added this pull request to the merge queue Apr 19, 2023

Merged via the queue into master with commit f3a5d96 Apr 19, 2023

marcoandredinis deleted the marco/integration_awsoidc branch April 19, 2023 16:04

This was referenced Apr 19, 2023

[v13] Integrations: AWS OIDC - ListDatabases action #24814

Merged

[v12] Integrations: AWS OIDC - ListDatabases action #24877

Merged

Conversation

marcoandredinis commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

public-teleport-github-review-bot Bot commented Apr 17, 2023

Uh oh!

marcoandredinis commented Apr 17, 2023

Uh oh!

r0mant left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimlisa Apr 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

public-teleport-github-review-bot Bot commented Apr 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

marcoandredinis commented Apr 12, 2023 •

edited

Loading

kimlisa Apr 18, 2023 •

edited

Loading