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

Support disconnect_expired_cert for database access #6857

Merged
merged 10 commits into from
May 31, 2021

Conversation

smallinsky
Copy link
Contributor

Issue #5476

Purpose:

Add support for the client_idle_timeout and disconnect_expired_cert user role flags in the client db proxy connection.

Implementation:

Use Monitor object to track connection activity.

@smallinsky smallinsky requested a review from r0mant May 13, 2021 15:56
Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

@smallinsky Also, please cover this scenario (disconnecting expired cert and, if possible, idle timeout) with unit tests. See test files in srv/db package for examples, most of the machinery for spinning up dbs should already be there. We also have integration tests in integration/db_integration_test.go.

lib/srv/db/proxyserver.go Outdated Show resolved Hide resolved
lib/srv/db/proxyserver.go Outdated Show resolved Hide resolved
lib/srv/db/proxyserver.go Outdated Show resolved Hide resolved
lib/services/role.go Outdated Show resolved Hide resolved
@smallinsky smallinsky force-pushed the smallinsky/monitor_db_proxy_client_conn branch 4 times, most recently from 2925cca to f30b821 Compare May 17, 2021 09:02
@smallinsky smallinsky requested a review from r0mant May 17, 2021 09:31
@smallinsky smallinsky force-pushed the smallinsky/monitor_db_proxy_client_conn branch from f30b821 to 09de9f3 Compare May 17, 2021 10:32
@smallinsky smallinsky marked this pull request as ready for review May 19, 2021 08:06
@smallinsky smallinsky requested review from nklaassen and awly May 19, 2021 08:07
Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

Implementation looks good to me now, just a few suggestions/nits here and there.

lib/srv/db/common/interfaces.go Outdated Show resolved Hide resolved
lib/srv/db/proxyserver.go Outdated Show resolved Hide resolved
lib/srv/db/proxy_test.go Outdated Show resolved Hide resolved
lib/srv/db/proxy_test.go Outdated Show resolved Hide resolved
lib/srv/db/proxy_test.go Outdated Show resolved Hide resolved
lib/srv/db/proxyserver.go Outdated Show resolved Hide resolved
lib/srv/db/proxyserver.go Outdated Show resolved Hide resolved
lib/srv/db/proxyserver.go Show resolved Hide resolved
lib/srv/monitor.go Outdated Show resolved Hide resolved
lib/srv/monitor.go Outdated Show resolved Hide resolved
@smallinsky smallinsky force-pushed the smallinsky/monitor_db_proxy_client_conn branch from d19c438 to a3fcbf2 Compare May 24, 2021 07:58
@smallinsky smallinsky force-pushed the smallinsky/monitor_db_proxy_client_conn branch 3 times, most recently from 8296673 to 5983c99 Compare May 24, 2021 08:40
@smallinsky smallinsky requested a review from r0mant May 24, 2021 08:59
@smallinsky smallinsky force-pushed the smallinsky/monitor_db_proxy_client_conn branch from 5983c99 to 101ed5b Compare May 24, 2021 09:00
Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

@smallinsky I've just realized something - the way it's currently implemented, with the connection being monitored on the proxy only, won't cover the trusted cluster scenario.

With trusted clusters, connection goes from the root cluster proxy to leaf cluster database service. So we need to have the same connection monitor on the "database service" side as well (lib/srv/db/server.go).

lib/kube/proxy/forwarder.go Outdated Show resolved Hide resolved
lib/srv/db/proxyserver.go Outdated Show resolved Hide resolved
Comment on lines 280 to 281
sync.RWMutex
net.Conn
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we unexport sync.RWMutex and net.Conn? You can also just remove net.Conn completely probably and just use it from cfg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, the mtx can be unexpected but net.Conn uses struct embedding in order to implement all unnecessary net.Conn methods and unexported net.Conn will require explicitly implementation of net.Conn methods like RemoteAddr SetWriteDeadline by theTrackingReadConn struct.

@smallinsky
Copy link
Contributor Author

smallinsky commented May 25, 2021

@r0mant

I've just realized something - the way it's currently implemented, with the connection being monitored on the proxy only, won't cover the trusted cluster scenario.

hm, I'm wondering if monitoring root proxy conn in leaf db service is needed.

Right now in trusted cluster scenario we have following setup:

db client -----> root proxy -------> leaf db service -> db
         (conn monitor) 

As far I know the root proxy ---> leaf db connection depends on db client - > root proxy and in case of idle connection termination by root proxy the root proxy -------> leaf db service connection will be also terminated.

Am I missing something ?

@smallinsky
Copy link
Contributor Author

@r0mant
I have added connection monitor to db service and covered the trusted cluster idle timeout scenario by integration test.

@smallinsky smallinsky requested a review from r0mant May 26, 2021 15:56
Copy link
Contributor

@nklaassen nklaassen left a comment

Choose a reason for hiding this comment

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

Code looks good to me

Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

Mostly lgtm, just one question about integration tests.

integration/db_integration_test.go Outdated Show resolved Hide resolved
integration/db_integration_test.go Outdated Show resolved Hide resolved
lib/srv/db/server.go Show resolved Hide resolved
lib/srv/monitor.go Outdated Show resolved Hide resolved
@smallinsky smallinsky requested a review from r0mant May 28, 2021 09:21
@smallinsky smallinsky merged commit eb7bb01 into master May 31, 2021
@smallinsky smallinsky deleted the smallinsky/monitor_db_proxy_client_conn branch May 31, 2021 08:26
smallinsky added a commit that referenced this pull request May 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants