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: frontend admitter #57849

Merged
merged 1 commit into from
Dec 15, 2020
Merged

Conversation

darinpp
Copy link
Contributor

@darinpp darinpp commented Dec 12, 2020

This replaces IncomingTLSConfig with a function similar to
BackendDialer that will be called to setup the frontend
connection/pipeline of the proxy.
It requires that all clients replace the calls like this one

  server := sqlproxyccl.NewServer(
    sqlproxyccl.Options{
      IncomingTLSConfig: &tlsConfig
    }
  }

with the analogous code

  server := sqlproxyccl.NewServer(
    sqlproxyccl.Options{
      FrontendAdmitter: func(
        incoming net.Conn,
      ) (net.Conn, *pgproto3.StartupMessage, error) {
        return sqlproxyccl.FrontendAdmit(
          incoming,
          &tls.Config,
        )
      }
    }
  }

While a bit more verbose, this makes possible for the proxy
library users to implement a custom logic to admit or
reject connections based on the client's IP address.

Release note: none

cc: @cockroachdb/sqlproxy-prs

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

:lgtm:

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


pkg/ccl/sqlproxyccl/frontend_admitter.go, line 18 at r1 (raw file):

)

// FrontendAdmit is an example frontend admitter

I don't think this is a good description - it's more than "an example" - it's the default admitter. Beef up this comment with information about what this method does and when and why it would be used. I'd put a bit of example code on how to use it as a library caller.


pkg/ccl/sqlproxyccl/proxy.go, line 84 at r1 (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)

NIT: move this above BackendDialer since it's called first, and so I'd want to read about it first.

@andy-kimball
Copy link
Contributor

Would also like to see frontend_admitter_test.go unit tests (missing backend_dialer_test.go as well, it looks like).

@spaskob
Copy link
Contributor

spaskob commented Dec 14, 2020

I think we are trying to reinvent the wheel. ie https://godoc.org/github.com/jackc/pgconn#ConnectConfig. We should use this API instead - it handles auth correctly (currently sqlproxy does not) and is going to support future changes to the protocol.

Copy link
Contributor Author

@darinpp darinpp left a comment

Choose a reason for hiding this comment

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

I added several unit tests for the frontend admitter. I'll add tests for the backend dialer in another PR as I can combine with swapping it to use PG client that Spas found.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andy-kimball, @imjching, and @spaskob)


pkg/ccl/sqlproxyccl/frontend_admitter.go, line 18 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

I don't think this is a good description - it's more than "an example" - it's the default admitter. Beef up this comment with information about what this method does and when and why it would be used. I'd put a bit of example code on how to use it as a library caller.

I expanded the comment.


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

Previously, andy-kimball (Andy Kimball) wrote…

NIT: move this above BackendDialer since it's called first, and so I'd want to read about it first.

moved

@darinpp
Copy link
Contributor Author

darinpp commented Dec 14, 2020

I think we are trying to reinvent the wheel. ie https://godoc.org/github.com/jackc/pgconn#ConnectConfig. We should use this API instead - it handles auth correctly (currently sqlproxy does not) and is going to support future changes to the protocol.

This library implements a client - I'll try replacing the Backend Dialer code to use it. This PR is more like a server side PG code.

@darinpp darinpp requested a review from andy-kimball December 14, 2020 20:19
@darinpp darinpp force-pushed the frontend-admitter branch 4 times, most recently from 12a429e to a44cbf2 Compare December 14, 2020 23:39
@darinpp
Copy link
Contributor Author

darinpp commented Dec 15, 2020

bors r+

@craig
Copy link
Contributor

craig bot commented Dec 15, 2020

Build failed:

@darinpp
Copy link
Contributor Author

darinpp commented Dec 15, 2020

bors r+

@darinpp
Copy link
Contributor Author

darinpp commented Dec 15, 2020

bors r-

@craig
Copy link
Contributor

craig bot commented Dec 15, 2020

Canceled.

This replaces `IncomingTLSConfig` with a function similar to
`BackendDialer` that will be called to setup the frontend
connection/pipeline of the proxy.
It requires that all clients replace the calls like this one
```
  server := sqlproxyccl.NewServer(
    sqlproxyccl.Options{
      IncomingTLSConfig: &tlsConfig
    }
  }
```
with the analogous code
```
  server := sqlproxyccl.NewServer(
    sqlproxyccl.Options{
      FrontendAdmitter: func(
        incoming net.Conn,
      ) (net.Conn, *pgproto3.StartupMessage, error) {
        return sqlproxyccl.FrontendAdmit(
          incoming,
          &tls.Config,
        )
      }
    }
  }

```
While a bit more verbose, this makes possible for the proxy
library users to implement a custom logic to admit or
reject connections based on the client's IP address.

Release note: none
@darinpp
Copy link
Contributor Author

darinpp commented Dec 15, 2020

bors r+

@craig
Copy link
Contributor

craig bot commented Dec 15, 2020

Build succeeded:

@craig craig bot merged commit 04e8935 into cockroachdb:master Dec 15, 2020
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.

4 participants