Skip to content

Refactor database DiscoveryResourceChecker#29864

Merged
greedy52 merged 2 commits intomasterfrom
STeve/p471_discovered_db_url_validation_p1
Aug 4, 2023
Merged

Refactor database DiscoveryResourceChecker#29864
greedy52 merged 2 commits intomasterfrom
STeve/p471_discovered_db_url_validation_p1

Conversation

@greedy52
Copy link
Copy Markdown
Contributor

@greedy52 greedy52 commented Aug 1, 2023

Moving discoveryResourceChecker from lib/srv/db/watcher.go to lib/srv/db/cloud/resource_checker.go

This changes prepares for adding database URL validation as more checkers will be implemented

@greedy52 greedy52 added the database-access Database access related issues and PRs label Aug 1, 2023
@greedy52 greedy52 self-assigned this Aug 1, 2023
@github-actions github-actions Bot requested review from AntonAM and ryanclark August 1, 2023 15:48
//
// Note that this checker warns the user with suggestions on how to configure
// the credentials correctly instead of returning errors.
type credentialsChecker struct {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

moved from watcher.go without logic change

type DiscoveryResourceChecker interface {
// Check performs required checks on provided database resource before it
// gets registered.
Check(ctx context.Context, database types.Database) error
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

moved from watcher.go but now returns error

Copy link
Copy Markdown
Contributor

@GavinFrazar GavinFrazar left a comment

Choose a reason for hiding this comment

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

LGTM

as for the data race detected during the test, I believe this is caused by the resource watcher's collector returning all of its collected resources every time a database is created:

case types.OpPut:
database, ok := event.Resource.(types.Database)
if !ok {
p.Log.Warnf("Unexpected resource type %T.", event.Resource)
return
}
p.current[database.GetName()] = database
select {
case <-ctx.Done():
case p.DatabasesC <- resourcesToSlice(p.current):

Not sure why the extra test case you added caused the race detector to trip now - I can't reproduce a data race failure if I remove the extra test case. I don't think it's caused by these changes though

@greedy52
Copy link
Copy Markdown
Contributor Author

greedy52 commented Aug 2, 2023

Not sure why the extra test case you added caused the race detector to trip now - I can't reproduce a data race failure if I remove the extra test case. I don't think it's caused by these changes though

I believe the race happens because the last test case (launching heartbeat/iam service etc.) and the new test case (applyResourceMatchersToDatabases) race on db5 that's stored in the test auth server. Let me work on a fix.

@greedy52
Copy link
Copy Markdown
Contributor Author

greedy52 commented Aug 3, 2023

@GavinFrazar @smallinsky

I've added a fix for the race d77047f.

The idea is to ensure that we don't change the attributes of databases in s.monitoredDatabases. And we use database.Copy() for proxiedDatabases. Attributes of databases in proxiedDatabases can be changed but only to "status" not "spec".

Let me know what you think. Thanks!

@GavinFrazar
Copy link
Copy Markdown
Contributor

we should backport the race fix to v13 as well

@greedy52 greedy52 added this pull request to the merge queue Aug 4, 2023
Merged via the queue into master with commit f79906b Aug 4, 2023
@greedy52 greedy52 deleted the STeve/p471_discovered_db_url_validation_p1 branch August 4, 2023 14:00
@public-teleport-github-review-bot
Copy link
Copy Markdown

@greedy52 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

database-access Database access related issues and PRs refactoring size/md

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants