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

Add MariaDB to AWS RDS auto discovery #10333

Merged
merged 9 commits into from
Mar 9, 2022
Merged

Conversation

jakule
Copy link
Contributor

@jakule jakule commented Feb 13, 2022

This PR adds auto-discovery to MariaDB RDS including docs changes. The minimum supported version is MariaDB 10.6 as AWS doesn't support IAM on older versions: https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/UsingWithRDS.IAMDBAuth.html

@jakule jakule force-pushed the jakule/mariadb-rds-discovery branch 2 times, most recently from bc08bdc to 2ee9f83 Compare March 3, 2022 16:57
@jakule jakule marked this pull request as ready for review March 3, 2022 16:57
@github-actions github-actions bot added database-access Database access related issues and PRs documentation labels Mar 3, 2022
lib/services/database.go Outdated Show resolved Hide resolved
lib/services/database_test.go Outdated Show resolved Hide resolved
docs/pages/database-access/guides/rds.mdx Outdated Show resolved Hide resolved
@@ -14,6 +14,11 @@ This guide will help you to:
writing so it can't be used with Database Access.
</Admonition>

<Admonition type="note" title="MariaDB supported versions">
Copy link
Contributor

Choose a reason for hiding this comment

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

The two back-to-back Admonitions are kind of hard to follow. I think we should include a single Admonition that starts with a general statement like, "The following products are not compatible with Database Access," followed by a two-item list that includes the products mentioned in these Admonitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I merged and re-phrase the block. Let me know what do you think.

docs/pages/database-access/guides/rds.mdx Outdated Show resolved Hide resolved
@@ -14,6 +14,11 @@ This guide will help you to:
writing so it can't be used with Database Access.
</Admonition>

<Admonition type="note" title="MariaDB supported versions">
The minimum supported version of MariaDB is 10.6. Older versions don't support
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I would clarify that this is specifically for RDS MariaDB. Might be clear enough as it is given that this is RDS specific guide but I wonder if somebody might get confused by it anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also thinking about it, but decided to skip as this is RDS only guide, but I don't see any harm with adding it.

// Min supported MariaDB version that supports IAM is 10.6
// https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/UsingWithRDS.IAMDBAuth.html
minIAMSupportedVer := semver.New("10.6.0")
return ver.Compare(*minIAMSupportedVer) >= 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I think using LessThan is a bit more readable.

@@ -106,6 +106,14 @@ func (f *rdsDBInstancesFetcher) getRDSDatabases(ctx context.Context) (types.Data
}
databases := make(types.Databases, 0, len(instances))
for _, instance := range instances {
if !services.IsRDSInstanceSupported(instance) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about Aurora clusters? Does Aurora provide MariaDB engine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see MariaDB in Aurora (checked website + docs).

@r0mant r0mant requested review from greedy52 and removed request for zmb3, lxea and xinding33 March 3, 2022 21:49
lib/services/database.go Outdated Show resolved Hide resolved
lib/services/database.go Outdated Show resolved Hide resolved
@jakule
Copy link
Contributor Author

jakule commented Mar 4, 2022

Checking the updated docs page I noticed something. Here is a screenshot
Screen Shot 2022-03-04 at 6 07 58 PM
So the page is targeting Teleport 9 release and everything described here (including support for RDS MariaDB) applies to v9, except Install Teleport 7.0.0. In my opinion is confusing. Any thoughts? @r0mant @ptgott

@jakule jakule force-pushed the jakule/mariadb-rds-discovery branch from 80bdfa7 to 426202d Compare March 4, 2022 23:26
@jakule jakule requested review from ptgott, greedy52 and r0mant March 4, 2022 23:27
@greedy52
Copy link
Contributor

greedy52 commented Mar 7, 2022

So the page is targeting Teleport 9 release and everything described here (including support for RDS MariaDB) applies to v9, except Install Teleport 7.0.0

teleport/docs/config.json

Lines 814 to 823 in b8cae1a

"teleport": {
"version": "7.0.0",
"golang": "1.16",
"plugin": {
"version": "7.0.0"
},
"helm_repo_url": "https://charts.releases.teleport.dev",
"latest_oss_docker_image": "quay.io/gravitational/teleport:7",
"latest_ent_docker_image": "quay.io/gravitational/teleport-ent:7"
}

We haven't updated the version for docs?

@@ -1,6 +1,6 @@
---
title: Database Access with AWS RDS and Aurora for PostgreSQL and MySQL
description: How to configure Teleport Database Access with AWS RDS and Aurora for PostgreSQL and MySQL.
title: Database Access with AWS RDS and Aurora for PostgreSQL, MySQL and MariaDB
Copy link
Contributor

Choose a reason for hiding this comment

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

To prevent this title from being truncated in Google's search results, we should get this to around 55 chars or less, e.g., by making it "Database Access with AWS RDS and Aurora." We can then add an h1 attribute to the frontmatter that uses the original title, "Database Access with AWS RDS and Aurora for PostgreSQL, MySQL and MariaDB." The h1 attribute is not used to populate the page's title element, while title is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good tip. I'd never think about it. Changed.

docs/pages/database-access/guides/rds.mdx Outdated Show resolved Hide resolved
@jakule jakule requested a review from ptgott March 8, 2022 03:30
docs/pages/database-access/guides/rds.mdx Outdated Show resolved Hide resolved
@jakule jakule enabled auto-merge (squash) March 9, 2022 16:13
@jakule jakule merged commit 6eebe8d into master Mar 9, 2022
@jakule jakule deleted the jakule/mariadb-rds-discovery branch March 9, 2022 16:32
jakule added a commit that referenced this pull request Mar 9, 2022
Add support for MariaDB AWS RDS with IAM authentication version 10.6+.

Co-authored-by: Paul Gottschling <[email protected]>
Co-authored-by: Alan Parra <[email protected]>
Co-authored-by: Roman Tkachenko <[email protected]>
jakule added a commit that referenced this pull request Mar 9, 2022
Add support for MariaDB AWS RDS with IAM authentication version 10.6+.

Backport #10333

Co-authored-by: Paul Gottschling <[email protected]>
Co-authored-by: Alan Parra <[email protected]>
Co-authored-by: Roman Tkachenko <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database-access Database access related issues and PRs documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants