Skip to content

*: support proxy-protocol#96

Merged
djshow832 merged 15 commits into
pingcap:mainfrom
xhebox:proxy_1
Sep 30, 2022
Merged

*: support proxy-protocol#96
djshow832 merged 15 commits into
pingcap:mainfrom
xhebox:proxy_1

Conversation

@xhebox

@xhebox xhebox commented Sep 22, 2022

Copy link
Copy Markdown
Contributor

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 requested a review from djshow832 September 22, 2022 09:50
@xhebox xhebox mentioned this pull request Sep 22, 2022
2 tasks
Comment thread pkg/proxy/backend/authenticator.go Outdated
Signed-off-by: xhe <xw897002528@gmail.com>
Comment thread pkg/proxy/backend/authenticator.go Outdated
Comment thread pkg/proxy/backend/authenticator.go Outdated
Comment thread pkg/proxy/backend/authenticator.go Outdated
Comment thread pkg/proxy/net/proxy.go
Signed-off-by: xhe <xw897002528@gmail.com>
Signed-off-by: xhe <xw897002528@gmail.com>
Signed-off-by: xhe <xw897002528@gmail.com>
Comment thread pkg/proxy/backend/authenticator.go Outdated
Comment thread pkg/proxy/backend/authenticator.go
Signed-off-by: xhe <xw897002528@gmail.com>
Signed-off-by: xhe <xw897002528@gmail.com>
Signed-off-by: xhe <xw897002528@gmail.com>
Comment thread pkg/proxy/backend/authenticator.go Outdated
Comment thread pkg/proxy/backend/authenticator.go Outdated
Comment thread pkg/proxy/net/packetio_mysql.go Outdated
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
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
Contributor 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.

Comment thread pkg/proxy/backend/authenticator.go
Comment thread pkg/proxy/backend/backend_conn_mgr.go Outdated
@xhebox
xhebox requested a review from djshow832 September 29, 2022 09:48
Comment thread pkg/proxy/backend/util.go Outdated
Comment thread pkg/proxy/client/client_conn.go
Comment thread pkg/proxy/client/client_conn.go
Comment thread pkg/proxy/client/client_conn.go
@djshow832

Copy link
Copy Markdown
Collaborator

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

@xhebox

xhebox commented Sep 29, 2022

Copy link
Copy Markdown
Contributor Author

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>
Comment thread pkg/proxy/client/client_conn.go Outdated
Comment thread pkg/proxy/proxy.go Outdated
Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox

xhebox commented Sep 29, 2022

Copy link
Copy Markdown
Contributor Author

Looks like some tests failed, let me handle it.

Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox

xhebox commented Sep 29, 2022

Copy link
Copy Markdown
Contributor Author

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