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

*: fix static backends & support route by user #119

Merged
merged 2 commits into from
Oct 24, 2022

Conversation

xhebox
Copy link
Collaborator

@xhebox xhebox commented Oct 20, 2022

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

What problem does this PR solve?

Issue Number: ref #15

Problem Summary: Minimal effort to support multi-namespace routing by user. Also fixed broken static routing. It is initially coded for hackathon, but mergeable anyway.

It also reveals some problems:

  1. The abstraction client -> backend -> auth is a little bit redundant, or too ideal: we need to pass options back and forth. In fact, we only need one class to handle connection with seperated files for readability.
  2. It can not be well tested with the current mockProxy: mp will construct a fixed backend with connectionID = 0, proxy = false, require-backend-tls = false and also namespaceManager = nil now.
    We probably will add more and more options. Yet clientConn and proxyServer is not tested. Need something more flexible than mp/mb/mc: a better API easier for testing instead of more helpers to target different cases.

The next step would be PD and TLS per namespace. Then we can possibly combine client/backend to enable more unit testing on these codes.

What is changed and how it works:

  1. Add GetNamespaceByUser and getBackendIO hook.
  2. Handle EOF in both handshake and packet relay in clientConn.
  3. Do not fail it no instances are specified for the static backend. Instead, report errors on connection.
  4. Check health first for static backends, then sleep.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
    Comment PD addrs to enable static backends first.
tiproxyctl namespace import conf/namespace
tiproxyctl namespace commit default ns2

mycli -u xhe -P 6000
mycli -u root -P 6000
  • 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>
@xhebox xhebox requested a review from djshow832 October 20, 2022 08:20
Comment on lines +105 to +110
for _, ns := range n.nsm {
if ns.User() == user {
return ns, true
}
}
return nil, false
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm afraid distinguishing namespaces by usernames may never be applied. If you have listened to the share from the PM, creating different usernames may still be trouble for users.
For the dev tier, they are getting the tenant information from the user names. It's different with this way.

Copy link
Collaborator

@djshow832 djshow832 Oct 21, 2022

Choose a reason for hiding this comment

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

But I think it's fine to support it now. We may remove it in the future if we find it's unnecessary.

Copy link
Collaborator Author

@xhebox xhebox Oct 21, 2022

Choose a reason for hiding this comment

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

We can go on development on routing while not apply any FD for official releases. And it is natural to develop: this PR merely has 100 LOC. I am still willing to do more refactors to make the code hackable and easier, both for dev and testing.

As for the dev tier, it is same, they are getting tenant information from user names, so does me. It is more of a problem of how to integrate GW and tiproxy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can remove routing by user anytime and we can add it back anytime. I want such a framework instead of a fixed framework for "the current specialized product". So i added getBackendIO to give more flexibility. This it the first motivation.

Also PMs clearly did not have enough energy to cover all aspects of products. A flexible and high-performance GW is absolutely a possible HTAP solution. According to Liu Tang, the reason they rejected "one endpoint solution" is that it is hard to implement and unknown what to do. But from this PR, it may not be that hard and it can be very clear.

@djshow832 djshow832 merged commit 92077f9 into pingcap:main Oct 24, 2022
@xhebox xhebox deleted the proxy_6 branch October 24, 2022 03:26
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