sqlproxyccl: add proxy protocol listener

[DuskEagle]

To support GCP Private Service Connect, we need to have a listener in
SQLProxy which expects packets to contain proxy protocol headers. This
listener will be used for all traffic inbound from PSC. At the same
time, SQLProxy must continue to accept connections through the public
Internet which will not contain proxy protocol headers, and for which
any proxy protocol headers we receive cannot be trusted.

This commit introduces an optional second listener in SQLProxy,
controlled by --proxy-protocol-listen-addr, which requires
proxy protocol even as the primary listener doesn't. Private Service
Connect will direct traffic to this second listener.

Resolves #117240

Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[jaylim-crl]

Looping @jeffswenson for visibility as I'll be OOO next week onwards. This will be used for PSC in serverless. The existing public load balancers do not support the proxy protocol, so we'll need to create a separate listener for the internal LB.

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

---

pkg/ccl/cliccl/mt_proxy.go line 114 at r1 (raw file):

	}

	return waitForSignals(ctx, server, stopper, proxyLn, errChan)

If we took this approach, we will want to make sure we gracefully drain proxyProtocolLn.

[DuskEagle]

If we took this approach, we will want to make sure we gracefully drain proxyProtocolLn

Thanks for the heads up!

[adwittumuluri]

This LGTM to me, but admittedly i'm not familiar with SQL proxy code, so will defer to @jaylim-crl and @jeffswenson

[jaylim-crl]

Approach LGTM, but I think we should add some tests (e.g. connecting via the additional listener with proxy protocol + main listener without proxy protocol). There's an existing TestProxyProtocol that we can extend: https://github.com/cockroachdb/cockroach/blob/58e70d86125601165e254e351eb69ab3188005c7/pkg/ccl/sqlproxyccl/proxy_handler_test.go#L130

We will also want to backport this to 23.1 and 23.2.

Reviewed 4 of 5 files at r1, 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DuskEagle and @jeffswenson)

[jeffswenson]

+1 to Jay's request for tests. That may be easier if you move most of this logic into the pkg/ccl/sqlproxyccl package that contains the majority of the sql proxy code.

[DuskEagle]

I've moved some logic around and added a test for how the main listener and the proxy protocol listener interact with the presence or absence of proxy headers. PTAL!

[jeffswenson]

The combination of flags here is a bit redundant. What do you think of making the existing --listen-addr flag as optional and adding a TODO to delete the require proxy protocol flag once we migrate everything over to --proxy-protocol?

[DuskEagle]

Done!

[jeffswenson]

I added a back port to 23.2 label. How urgently do you need this change? We probably won't roll out 23.2 to serverless prod until early March. If that is too late, we will also need to back port this to 23.1.

We need the two TLs with the most context on a component to sign off on all back ports. For SQL Proxy that is probably me and @rafiss.

https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/900005932/Backporting+a+change+to+a+release+branch

LGTM

[DuskEagle]

We're trying to deliver this feature by end of January. It seems like we'll need a backport to 23.1 then. The other option would be to run a different version of the Cockroach image for SQLProxy alongside a 23.1 cluster. That seems to work in my tests, but are there issues you see with that approach?

[jeffswenson]

We're trying to deliver this feature by end of January. It seems like we'll need a backport to 23.1 then. The other option would be to run a different version of the Cockroach image for SQLProxy alongside a 23.1 cluster. That seems to work in my tests, but are there issues you see with that approach?

We should back port to 23.1. Your timeline is more aggressive than the db release timeline, so once the change is backported we can cut a custom 23.1 build and roll that out. I'm against rolling out any changes to production that have not made it to the corresponding release branch. Rolling out builds with non-merged changes makes it easy to accidentally roll back the change when the next minor release is cut.

How to cust custom builds:
https://cockroachlabs.atlassian.net/wiki/spaces/ENG/pages/846299518

The Serverless team updates this confluence page whenever we ship a custom build. It helps us track which commits are in the build:
https://cockroachlabs.atlassian.net/wiki/spaces/CCSER/pages/3190980784/Serverless+Custom+Release

[DuskEagle]

TFTRs!

bors r+

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 8d752fd to blathers/backport-release-23.1-117241: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.

---

error setting reviewers, but backport branch blathers/backport-release-23.2-117241 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/117842/requested_reviewers: 422 Reviews may only be requested from collaborators. One or more of the teams you specified is not a collaborator of the cockroachdb/cockroach repository. []

Backport to branch 23.2.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}