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 proxy-protocol #96

Merged
merged 15 commits into from
Sep 30, 2022
Merged

*: support proxy-protocol #96

merged 15 commits into from
Sep 30, 2022

Conversation

xhebox
Copy link
Collaborator

@xhebox xhebox commented Sep 22, 2022

Signed-off-by: xhe xw897002528@gmail.com

What problem does this PR solve?

Issue Number: ref #14, proxy section

Problem Summary: As title, it is coded as minimal as possible. One thing, it is closed by default: because tidb can not automatically check if incoming connections are proxy or not. (start tidb using tiup with proxy-protocol will fail the health check). That said we can only enable it if the upstream server supports.

What is changed and how it works:

  1. misc changes in conf/namespace/example.yaml, pkg/manager/router/router.go, Makefile, pkg/proxy/proxy.go
  2. handshakeFirstTime is rewritten such that handshakes between client and proxy is finished first. Another difference is that the new logic is more strict compare to the old logic, e.g. accurate SSLRequest are sent instead of handshakeResp.
  3. pkg/proxy/backend/util.go a salt generator
  4. fix a length bug in ToBytes(), also avoid panic if callers passed different address family.
  5. set deadline for pkg/proxy/net/packetio_test.go
  6. remove weir in github template

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
1. uncomment proxy-protocol in ./conf/proxy.yaml

2. set config for tidb(tiup ... --db.config ... or ./tidb-server -config ...)
[proxy-protocol]
networks = "*"
timeout = 10

3.  connect without failure
  • No code

Notable changes

  • Has configuration change
  • Has HTTP API interfaces change (Don't forget to add the declarative for API)
  • Has tiproxyctl change
  • Other user behavior changes

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Signed-off-by: xhe <xw897002528@gmail.com>
Signed-off-by: xhe <xw897002528@gmail.com>
Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox xhebox mentioned this pull request Sep 22, 2022
2 tasks
Signed-off-by: xhe <xw897002528@gmail.com>
pkg/proxy/backend/authenticator.go Outdated Show resolved Hide resolved
pkg/proxy/backend/authenticator.go Outdated Show resolved Hide resolved
pkg/proxy/backend/authenticator.go Outdated Show resolved Hide resolved
pkg/proxy/net/proxy.go Show resolved Hide resolved
Signed-off-by: xhe <xw897002528@gmail.com>
Signed-off-by: xhe <xw897002528@gmail.com>
Signed-off-by: xhe <xw897002528@gmail.com>
pkg/proxy/backend/authenticator.go Outdated Show resolved Hide resolved
pkg/proxy/backend/authenticator.go Show resolved Hide resolved
Signed-off-by: xhe <xw897002528@gmail.com>
Signed-off-by: xhe <xw897002528@gmail.com>
Signed-off-by: xhe <xw897002528@gmail.com>
pkg/proxy/backend/authenticator.go Outdated Show resolved Hide resolved
pkg/proxy/backend/authenticator.go Outdated Show resolved Hide resolved
pkg/proxy/net/packetio_mysql.go Outdated Show resolved Hide resolved
logger.Error("require frontend capabilities", zap.Stringer("common", commonCaps), zap.Stringer("required", requiredCapabilities))
return errors.Wrapf(ErrCapabilityNegotiation, "require %s", requiredCapabilities&^commonCaps)
}
commonCaps := frontendCapability & proxyCapability
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should use the capability in the second response from the client (if SSL is enabled). Some clients, such as sysbench, send 2 different capabilities. Only the last one is correct.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should be investigated, I rememered mysql-server sometimes will only use the first capabilities.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed, It is same in my several tests. The behavior should be determined by libmysql linked to sysbench. Sysbench it self does not control this.

pkg/proxy/backend/authenticator.go Show resolved Hide resolved
pkg/proxy/backend/backend_conn_mgr.go Outdated Show resolved Hide resolved
pkg/proxy/backend/util.go Outdated Show resolved Hide resolved
pkg/proxy/client/client_conn.go Show resolved Hide resolved
pkg/proxy/client/client_conn.go Show resolved Hide resolved
pkg/proxy/client/client_conn.go Show resolved Hide resolved
@djshow832
Copy link
Collaborator

HandshakeSecondTime also needs proxy protocol. This requires the authenticator to keep the real client address. You can do it in another PR.

@xhebox
Copy link
Collaborator Author

xhebox commented Sep 29, 2022

HandshakeSecondTime also needs proxy protocol. This requires the authenticator to keep the real client address. You can do it in another PR.

I agree. We have discussed too much on this PR.

Signed-off-by: xhe <xw897002528@gmail.com>
pkg/proxy/client/client_conn.go Outdated Show resolved Hide resolved
pkg/proxy/proxy.go Outdated Show resolved Hide resolved
Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox
Copy link
Collaborator Author

xhebox commented Sep 29, 2022

Looks like some tests failed, let me handle it.

Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox
Copy link
Collaborator Author

xhebox commented Sep 29, 2022

PSMultiresult tests are removed. Since proxy can not handle this and won't send this, it does not matter backend/frontend has this capability flag or not.

@djshow832 djshow832 merged commit 3b6c664 into pingcap:main Sep 30, 2022
@xhebox xhebox deleted the proxy_1 branch November 2, 2022 08:51
xhebox added a commit to xhebox/TiProxy that referenced this pull request Mar 7, 2023
xhebox added a commit to xhebox/TiProxy that referenced this pull request Mar 13, 2023
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.

2 participants