Skip to content

Database Service to validate URL of database resources from Discovery Service#30054

Merged
greedy52 merged 8 commits intomasterfrom
STeve/p471_discovered_db_url_validation_p2
Aug 10, 2023
Merged

Database Service to validate URL of database resources from Discovery Service#30054
greedy52 merged 8 commits intomasterfrom
STeve/p471_discovered_db_url_validation_p2

Conversation

@greedy52
Copy link
Copy Markdown
Contributor

@greedy52 greedy52 commented Aug 4, 2023 •
edited
Loading

Related issue:

To validate database resource from Discovery Service

  • Do DescribeXXX calls to fetch AWS resources and compare URLs
  • Do simple AWS endpoint validation if no permissions to DescribeXXX
  • Do simple Azure endpoint validation for Azure databases

Did some basic testing with RDS instance. I will do a few more but likely I won't cover all database types. Hopefully we can catch any problems during release testing.

Testing:

  • AWS
    • RDS instance
    • RDS cluster (primary, reader, custom)
    • RDS proxy (primary, custom)
    • Redshift
    • Redshift Serverless (workgroup, vpc)
    • ElastiCache (primary, reader, configuration)
    • MemoryDB
    • OpenSearch (primary, vpc, custom)
  • Azure
    • MySQL (or managed)
    • Posrtgres (or managed)
    • SQL server (or managed)
    • Redis
    • Redis Enterprise

@greedy52 greedy52 added security Security Issues database-access Database access related issues and PRs do-not-merge changelog labels Aug 4, 2023
@greedy52 greedy52 self-assigned this Aug 4, 2023
@greedy52 greedy52 force-pushed the STeve/p471_discovered_db_url_validation_p2 branch from 2b7dc2b to 41fbc3b Compare August 6, 2023 18:27
@greedy52 greedy52 changed the base branch from master to STeve/p471_discovered_db_url_validation_p15 August 6, 2023 18:28
@greedy52 greedy52 force-pushed the STeve/p471_discovered_db_url_validation_p2 branch from 41fbc3b to 6919355 Compare August 6, 2023 19:22
Base automatically changed from STeve/p471_discovered_db_url_validation_p15 to master August 8, 2023 12:24
@greedy52 greedy52 force-pushed the STeve/p471_discovered_db_url_validation_p2 branch from 6919355 to 66c3b91 Compare August 8, 2023 14:47
@greedy52 greedy52 marked this pull request as ready for review August 8, 2023 15:38
smallinsky
smallinsky reviewed Aug 9, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@smallinsky smallinsky left a comment

Choose a reason for hiding this comment

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

Nice work. The code structure looks very neat.
First pass - I have left few nit comments and tested some paths and LGTM but I would like to take a carful look tomorrow before approving.

Comment thread lib/srv/db/cloud/resource_checker_url_aws.go Outdated
Comment thread lib/srv/db/cloud/resource_checker_url_aws.go Outdated
Comment thread lib/srv/db/cloud/resource_checker_url_aws.go
Comment thread lib/srv/db/cloud/resource_checker_url_aws.go
Comment thread lib/srv/db/cloud/resource_checker_url_aws.go
Comment thread lib/srv/db/cloud/resource_checker_url_aws.go
Comment thread lib/srv/db/cloud/resource_checker_url_aws.go
@greedy52 greedy52 mentioned this pull request Aug 9, 2023
Merged
gabrielcorado
gabrielcorado approved these changes Aug 9, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@gabrielcorado gabrielcorado left a comment

Choose a reason for hiding this comment

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

LGTM (I have only tested with RDS Proxy MSSQL).

Do simple AWS endpoint validation if no permissions to DescribeXXX

Does that mean we now recommend database agents to have DescribeXXX permissions? So that users can have "enhanced" validation?

Comment thread lib/srv/db/cloud/resource_checker_url_aws.go
return trace.BadParameter("expect 1 domain but got %v", domains.DomainStatusList)
}

databases, err := services.NewDatabasesFromOpenSearchDomain(domains.DomainStatusList[0], nil)
Copy link
Copy Markdown
Contributor

@gabrielcorado gabrielcorado Aug 9, 2023

Choose a reason for hiding this comment

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

Nit: Should we do the same for other databases? Use our NewDatabaseFromXXX to compare both databases.

Copy link
Copy Markdown
Contributor Author

@greedy52 greedy52 Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

we certainly can, and probably should if we want absolute consistency =D. I'd prefer just comparing the address directly as we don't need other things from types.Database generated by NewDatabaseXXX. I only use NewDatabasessXXX for the plural cases as otherwise it would be too much logic to compare all URLs generated from one resource.

@greedy52 greedy52 force-pushed the STeve/p471_discovered_db_url_validation_p2 branch from f5ddbfc to 6d37cf5 Compare August 9, 2023 20:05
@greedy52
Copy link
Copy Markdown
Contributor Author

greedy52 commented Aug 9, 2023 •
edited
Loading

Does that mean we now recommend database agents to have DescribeXXX permissions? So that users can have "enhanced" validation?

Yes. Actually this reminds a missing change on configurator. Added now. Thanks!
32dc295

@greedy52 greedy52 force-pushed the STeve/p471_discovered_db_url_validation_p2 branch from 6d37cf5 to 32dc295 Compare August 9, 2023 20:12
@greedy52 greedy52 enabled auto-merge August 10, 2023 16:31
@greedy52 greedy52 added this pull request to the merge queue Aug 10, 2023
Merged via the queue into master with commit 116d880 Aug 10, 2023
@greedy52 greedy52 deleted the STeve/p471_discovered_db_url_validation_p2 branch August 10, 2023 17:06
@jentfoo jentfoo mentioned this pull request Aug 14, 2023
Merged
@greedy52 greedy52 mentioned this pull request Aug 14, 2023
Merged
@greedy52 greedy52 mentioned this pull request Aug 14, 2023
Merged
greedy52 added a commit that referenced this pull request Aug 15, 2023
@greedy52
Database Service to validate URL of database resources from Discovery…
5cfbd85 
… Service (#30054)

* url checker

* add ut

* add comment in test plan

* warnOnError only when err is not nil

* fix typos etc.

* enable metadata service permission for discovered databases

* add a debug log when azure db url is validated
github-merge-queue Bot pushed a commit that referenced this pull request Aug 16, 2023
@greedy52
[v13] Database Service to validate URL of database resources from Dis…
9b49037 
…covery Service (#30462)

* Database Service to validate URL of database resources from Discovery Service (#30054)

* url checker

* add ut

* add comment in test plan

* warnOnError only when err is not nil

* fix typos etc.

* enable metadata service permission for discovered databases

* add a debug log when azure db url is validated

* fix services.NewDatabasesFromOpenSearchDomain typo
@GavinFrazar GavinFrazar mentioned this pull request Aug 19, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gabrielcorado gabrielcorado gabrielcorado approved these changes

@smallinsky smallinsky smallinsky approved these changes

Assignees

@greedy52 greedy52

Labels

database-access Database access related issues and PRs security Security Issues size/lg

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@greedy52 @gabrielcorado @smallinsky