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 support for automatic database users for Postgres #25614

Merged
merged 1 commit into from May 18, 2023
Merged

Conversation

r0mant
Copy link
Collaborator

@r0mant r0mant commented May 4, 2023

Implements RFD 113.

A couple of minor optional things mentioned in the RFD are missing from the implementation (like auditing activate/deactivate queries). I'll add them later while doing more testing during review or as a follow up.

@r0mant r0mant requested review from greedy52 and smallinsky May 4, 2023 03:33
@github-actions github-actions bot added database-access Database access related issues and PRs size/lg labels May 4, 2023
Copy link
Contributor

@greedy52 greedy52 left a comment

Choose a reason for hiding this comment

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

First round. Will test it out next round.

api/constants/constants.go Outdated Show resolved Hide resolved
lib/srv/db/common/role/role.go Outdated Show resolved Hide resolved
lib/srv/db/postgres/users.go Show resolved Hide resolved
lib/services/role.go Show resolved Hide resolved
lib/srv/db/postgres/users.go Show resolved Hide resolved
Copy link
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.

First pass.

lib/config/fileconf.go Show resolved Hide resolved
lib/service/servicecfg/database.go Show resolved Hide resolved
lib/srv/db/common/interfaces.go Outdated Show resolved Hide resolved
lib/srv/db/common/autousers.go Outdated Show resolved Hide resolved
lib/srv/db/postgres/engine.go Show resolved Hide resolved
@r0mant
Copy link
Collaborator Author

r0mant commented May 10, 2023

@smallinsky @greedy52 I've addressed your feedback folks, mind taking another look?

@r0mant r0mant requested a review from greedy52 May 10, 2023 23:59
lib/auth/auth.go Outdated Show resolved Hide resolved
Copy link
Contributor

@greedy52 greedy52 left a comment

Choose a reason for hiding this comment

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

tested with RDS today and this feature is awesome!

One small nit on a corner case, when admin user is not setup properly, message from tsh is not very obvious what's wrong sometimes:

$ tsh db connect --db-user STeve --db-name test postgres-rds
psql: error: connection to server at "localhost" (::1), port 58348 failed: Connection refused
	Is the server running on that host and accepting TCP/IP connections?
connection to server at "localhost" (127.0.0.1), port 58348 failed: FATAL:  password authentication failed for user "not-found"
ERROR: exit status 2

Comment on lines +872 to +880
if db.GetAdminUser() != "" && username == "" {
log.Debugf("Defaulting to Teleport username %q as database username.", tc.Username)
username = tc.Username
}
Copy link
Contributor

@greedy52 greedy52 May 11, 2023

Choose a reason for hiding this comment

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

a common problem where Teleport Connect code base may require extra changes.

@r0mant r0mant requested a review from smallinsky May 16, 2023 00:45
@r0mant
Copy link
Collaborator Author

r0mant commented May 16, 2023

@smallinsky PTAL?

@r0mant r0mant force-pushed the roman/dbuser branch 8 times, most recently from ed4f7a6 to 9717c48 Compare May 18, 2023 18:27
@r0mant r0mant added this pull request to the merge queue May 18, 2023
Merged via the queue into master with commit 79b54d8 May 18, 2023
32 checks passed
@r0mant r0mant deleted the roman/dbuser branch May 18, 2023 23:39
@public-teleport-github-review-bot

@r0mant See the table below for backport results.

Branch Result
branch/v13 Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v13 database-access Database access related issues and PRs size/lg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants