ccl/sqlproxyccl: handle implicit auth in OpenTenantConnWithToken#77111
Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom Mar 1, 2022
Merged
ccl/sqlproxyccl: handle implicit auth in OpenTenantConnWithToken#77111craig[bot] merged 1 commit intocockroachdb:masterfrom
craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
Member
|
This change is |
jaylim-crl
added a commit
to jaylim-crl/cockroach
that referenced
this pull request
Feb 28, 2022
Informs cockroachdb#76000. Builds on top of cockroachdb#77109 and cockroachdb#77111. This commit completes the connection migration feature in the the forwarder within sqlproxy. The idea is as described in the RFC. A couple of new sqlproxy metrics have been added as well: - proxy.conn_migration.requested - proxy.conn_migration.accepted - proxy.conn_migration.rejected - proxy.conn_migration.success - proxy.conn_migration.error - proxy.conn_migration.fail - proxy.conn_migration.timeout_closed - proxy.conn_migration.timeout_recoverable For more details, see metrics.go in the sqlproxyccl package. Release note: None
63bd54d to
8b2c82d
Compare
jeffswenson
approved these changes
Feb 28, 2022
| // | ||
| // Discarding messages is fine because this will only be used during connection | ||
| // migration with the token-based authentication, and the proxy is the client. | ||
| var implicitAuthenticate = func(crdbConn net.Conn) error { |
Collaborator
There was a problem hiding this comment.
The name "implicitAuthenticate" feels off to me. The method is not authenticating. It's finishing the connection startup process. What do you think of the name: readUntilReady() or readStartupReply()?
8b2c82d to
1c92d42
Compare
Informs cockroachdb#76000. Extracted from cockroachdb#76805. Previously, we assumed that with the token-based authentication, the server is ready to accept queries the moment we connect to it. This assumption has been proved wrong during the integration tests with the forwarder, and there's an implicit AuthenticateOK step that happens after connecting to the server. During that time, initial connection data such as ParameterStatus and BackendKeyData messages will be sent to the client as well. For now, we will ignore those messages. Once we start implementing query cancellation within the proxy, that has to be updated to cache the new BackendKeyData. This commit also fixes a buglet to handle pgwire messages with no body. pgproto3's Receive methods will still call Next if the body size is 0, and previously, we were returning an io.EOF error. This commit changes that behavior to return an empty slice. Release note: None Release justification: This fixes two buglets: one that was introduced when we added token-based authentication support to the proxy in cockroachdb#76417, and another when we added the interceptors. This is low risk as part of the code is guarded behind the connection migration feature, which is currently not being used in production. To add on, CockroachCloud is the only user of sqlproxy.
1c92d42 to
70478ca
Compare
jaylim-crl
commented
Feb 28, 2022
Contributor
Author
jaylim-crl
left a comment
There was a problem hiding this comment.
TFTR! I also fixed a small buglet within the chunkreader that I found when working on this.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @jeffswenson)
pkg/ccl/sqlproxyccl/authentication.go, line 143 at r1 (raw file):
Previously, JeffSwenson (Jeff Swenson) wrote…
The name "implicitAuthenticate" feels off to me. The method is not authenticating. It's finishing the connection startup process. What do you think of the name: readUntilReady() or readStartupReply()?
Renamed this to readTokenAuthResult based on our discussions. Did not extract the initial connection data part, and left that as a TODO instead given that we're in a stabilization period.
Contributor
Author
|
bors r=JeffSwenson |
jaylim-crl
added a commit
to jaylim-crl/cockroach
that referenced
this pull request
Mar 1, 2022
Informs cockroachdb#76000. Builds on top of cockroachdb#77109 and cockroachdb#77111. This commit completes the connection migration feature in the the forwarder within sqlproxy. The idea is as described in the RFC. A couple of new sqlproxy metrics have been added as well: - proxy.conn_migration.requested - proxy.conn_migration.success - proxy.conn_migration.error_fatal - proxy.conn_migration.error_recoverable - proxy.conn_migration.attempted - proxy.conn_migration.protocol_error For more details, see metrics.go in the sqlproxyccl package. Release note: none Release justification: This completes the first half of the connection migration feature. This is low risk as part of the code is guarded behind the connection migration feature, which is currently not being used in production. To add on, CockroachCloud is the only user of sqlproxy.
Contributor
|
Build failed (retrying...): |
Contributor
|
Build failed (retrying...): |
Contributor
|
Build succeeded: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Informs #76000. Extracted from #76805.
Previously, we assumed that with the token-based authentication, the server is
ready to accept queries the moment we connect to it. This assumption has been
proved wrong during the integration tests with the forwarder, and there's
an implicit AuthenticateOK step that happens after connecting to the server.
During that time, initial connection data such as ParameterStatus and
BackendKeyData messages will be sent to the client as well. For now, we will
ignore those messages. Once we start implementing query cancellation within
the proxy, that has to be updated to cache the new BackendKeyData.
This commit also fixes a buglet to handle pgwire messages with no body.
pgproto3's Receive methods will still call Next if the body size is 0, and
previously, we were returning an io.EOF error. This commit changes that
behavior to return an empty slice.
Release note: None
Release justification: This fixes two buglets: one that was introduced when we
added token-based authentication support to the proxy in #76417, and another
when we added the interceptors. This is low risk as part of the code is
guarded behind the connection migration feature, which is currently not being
used in production. To add on, CockroachCloud is the only user of sqlproxy.