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

ccl/sqlproxyccl: developer provided proxy handler #58203

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

darinpp
Copy link
Contributor

@darinpp darinpp commented Dec 22, 2020

This PR changes the sqlproxyccl's Proxy implementation. Previously, it
was expected that the developer will populate an options struct that
will customize how the provided proxy will operate. With this PR, it is
expected that the developer will provide a proxy handler instead that
will do everything needed to proxy an SQL connection from a frontend to
a backend. To help with the implementation, the code has been split into
number of helper methods that can be combined and that make easy bulding
a custom proxy. The existing implementation is now moved into the test
file and is just one example of how a complicated proxy can be
constructed.

Release note: none

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@andy-kimball andy-kimball left a comment

Choose a reason for hiding this comment

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

Should we call this Handler rather than Interceptor? I think Handler is the usual Go term for what this is doing.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball and @darinpp)


pkg/ccl/sqlproxyccl/proxy.go, line 223 at r1 (raw file):

	}

	go func() {

Why are we introducing another goroutine here? I'd like to find a design that doesn't require an extra goroutine for every connection.

@darinpp darinpp force-pushed the proxy-connection-handler branch 2 times, most recently from f38d532 to 1688058 Compare December 22, 2020 23:06
Copy link
Contributor

@andy-kimball andy-kimball left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @darinpp)


pkg/ccl/sqlproxyccl/proxy.go, line 83 at r2 (raw file):

	// is accepted. It can optionally negotiate SSL, provide admittance control or
	// other types of frontend connection filtering.
	FrontendAdmitter func(incoming net.Conn) (net.Conn, *pgproto3.StartupMessage, error)

Let's get rid of FrontendAdmitter and BackendDialer, since they're obsolete with the new proxy pattern.


pkg/ccl/sqlproxyccl/proxy.go, line 96 at r2 (raw file):

// information to the end user in case of a problem.
func (s *Server) SendErrToClient(conn net.Conn, code ErrorCode, msg string) {
	if s.opts.OnSendErrToClient != nil {

Let's get rid of OnSendErrToClient, since now the caller can do that on their own if they need it.


pkg/ccl/sqlproxyccl/proxy.go, line 233 at r2 (raw file):

}

func (s *Server) updateMetricsForError(err error) {

How come this is not exported? Wouldn't hosts need to use it?

@darinpp darinpp changed the title ccl/sqlproxyccl: developer provided proxy interceptor ccl/sqlproxyccl: developer provided proxy handler Jan 6, 2021
@darinpp darinpp force-pushed the proxy-connection-handler branch 3 times, most recently from cff406c to 22f52fb Compare March 1, 2021 18:40
This PR changes the sqlproxyccl's Proxy implementation. Previously, it
was expected that the developer will populate an options struct that
will customize how the provided proxy will operate. With this PR, it is
expected that the developer will provide a proxy handler instead that
will do everything needed to proxy an SQL connection from a frontend to
a backend. To help with the implementation, the code has been split into
number of helper methods that can be combined and that make easy bulding
a custom proxy. The existing implementation is now moved into the test
file and is just one example of how a complicated proxy can be
constructed.

Release note: none

Release justification:
@tbg tbg added the X-noremind Bots won't notify about PRs with X-noremind label May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
X-noremind Bots won't notify about PRs with X-noremind
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants