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

MDEV-15935 Add redirection support #1472

Closed
wants to merge 15 commits into from

Conversation

shih-che
Copy link

@shih-che shih-che commented Mar 20, 2020

Background

This pull request implements server side redirection mechanism as per discussion in mariadb-corporation/mariadb-connector-j#134, which contains the client side change.

Design

Mainly changes are made to the OK packet sent by server, where redirection info (host, port, user etc.) will be appended to the end of the packet if redirect_enabled sysvar is set to ON. The info will be sent back to client, and client will then leverage such info to establish connection directly with server.

Implementation

The following sysvars are added to mysqld:

redirect_enabled; redirect_server_host; redirect_server_port; redirect_server_ttl;

where redirect_enabled means whether the server supports redirection, and other variables hold values that constitute redirection info and guide client to connect to the server.

After the change, when an incoming connection is authenticated, server now checks whether redirect_enabled is set to ON. If so, server will make use of redirect_server_host, redirect_server_port and redirect_server_ttl and append those variable values to the end of the OK packet. If redirect_enabled is OFF server has exactly the same behaviour as it had before.

@CLAassistant
Copy link

CLAassistant commented Mar 20, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@grooverdan grooverdan left a comment

Choose a reason for hiding this comment

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

just a quick look.

sql/sql_acl.cc Outdated Show resolved Hide resolved
sql/sql_acl.cc Outdated Show resolved Hide resolved
sql/sql_acl.cc Outdated Show resolved Hide resolved
sql/sql_acl.cc Outdated Show resolved Hide resolved
sql/sql_acl.cc Outdated Show resolved Hide resolved
sql/sql_acl.cc Outdated Show resolved Hide resolved
sql/sys_vars.cc Outdated Show resolved Hide resolved
@an3l an3l added this to the 10.5 milestone Mar 23, 2020
@robertbindar
Copy link
Contributor

@markus456, can you also please take a look at this contribution?

Copy link
Contributor

@markus456 markus456 left a comment

Choose a reason for hiding this comment

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

Looks OK from my point of view. There are probably some details that I missed as I'm not that "fluent" in server code.

sql/sys_vars.cc Outdated Show resolved Hide resolved
sql/sql_acl.cc Outdated Show resolved Hide resolved
sql/sys_vars.cc Outdated Show resolved Hide resolved
sql/sys_vars.cc Outdated Show resolved Hide resolved
sql/sql_acl.cc Outdated Show resolved Hide resolved
@shih-che
Copy link
Author

shih-che commented Apr 1, 2020

Hi all, I've signed CLA, resolved all comments and fixed all failed checks that are reproducible in my dev environment. Would anyone please educate me on the proper next step to get my pull request completed?

@robertbindar
Copy link
Contributor

Hi @shih-che! As far as I saw, @vuvova mentioned in the J connector PR that server support should be landed first to facilitate testing of the redirection feature in the J connector. So as soon as both @markus456 and @vaintroub give their OK on this contribution, the PR will be merged. The J-connector side will follow once reviewers there will make sure the code is as bug-free as possible.

Copy link
Contributor

@markus456 markus456 left a comment

Choose a reason for hiding this comment

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

Looks OK.

sql/sys_vars.cc Outdated Show resolved Hide resolved
sql/sql_acl.cc Outdated Show resolved Hide resolved
Co-Authored-By: Bradley Grainger <bgrainger@gmail.com>
@shih-che
Copy link
Author

shih-che commented Apr 9, 2020

Hi @vaintroub, I was told your OK is required for the pull request to be completed, would you please help review the PR at your convenience?

@shih-che shih-che requested a review from vaintroub April 10, 2020 09:16
@vaintroub
Copy link
Member

I wrote some comments. Now, my general question is why do we need it in the server? As far as I understand, for Azure, it is inside the proxy, not inside the server. Or is it?

sql/sql_acl.cc Show resolved Hide resolved
sql/sql_acl.cc Outdated Show resolved Hide resolved
sql/mysqld.h Outdated Show resolved Hide resolved
@shih-che shih-che requested a review from vaintroub April 13, 2020 04:17
sql/sql_acl.cc Outdated Show resolved Hide resolved
sql/sys_vars.cc Outdated Show resolved Hide resolved
@shih-che
Copy link
Author

@vaintroub I've made changes according to your comments in previous reviews, would you mind taking another look at the PR at your convenience?

sql/sql_acl.cc Outdated Show resolved Hide resolved
@vuvova
Copy link
Member

vuvova commented Apr 20, 2020

A few more questions still:

  • why does it require four server variables, instead of one, say, redirect_url. If it's empty — there's no redirect, if it's set to something like mysql://foobar:1234/ttl=56 then there is a redirect.
  • why there's a user in the redirect url?
  • can the client ignore the redirect url or is it required to follow it?

and I still don't understand the use case yet 😞

@hzhang87
Copy link

A few more questions still:

  • why does it require four server variables, instead of one, say, redirect_url. If it's empty — there's no redirect, if it's set to something like mysql://foobar:1234/ttl=56 then there is a redirect.

Technically we can, we put them four is because of user experience consideration. Due to our experience in public cloud of hosting MariaDB, some users are easy to configure parameter by a wrong string format if one parameter has a strict format like the case you told. We received several similar support cases like this before. For example, we saw numbers of user case that they miss '/' or ':' or ';' or included incorrect character when giving a param value so that MariaDB server failed to start or feature didn't work as they expected.. So, a good experience we think is to have a simple format for each parameter
Secondly, hiding the protocol in parameter but implement in code gives its ability to extend the protocol in the future

  • why there's a user in the redirect url?

I explained to vaintroub before. In general, this is not because of implementation, but because give feasibility to cloud provider what they can do with this protocol.
(1) Because for some cloud providers with GW or proxy as middle man, like Azure, part of host name is encoded as part of username. Otherwise gateway has no idea which mysql server it will talk to giving current protocol. So, the username after redirection is a different one. In this case, we have to make username as part of protocol and mariadb cloud provider is feasible to have their logic about how the username looks like with redirection.
(2) Although we don't have it for now. But having username as part of protocol also makes it feasible to extend the usage. Like, redirect connection to a hidden replica with same user but a different username tells where the replica is where primary and replica shares same public IP (different private IP in VNet). In other words, having username in protocol give feasibility to cloud provider to extend their network topology

  • can the client ignore the redirect url or is it required to follow it?

It depends on how client implement. In protocol respect, we will help PHP/MariaDB-J/MariaDB-c (already working on), and possibly other drivers in the future. There are 3 options in PHP driver which we released as a PECL extension: off/preferred/on. For off, the client ignore the redirect url. For on or forced, it takes and force to go this way. For preferred, it will fallback to original one if redirected network failed.

and I still don't understand the use case yet 😞

@vuvova In general, the use case is mostly for MariaDB service with a Gateway as we discussed before. There are two problems here: (1) the query latency will be larger when connection go through gateway; (2) new connection set up will take longer because of client->gateway + gateway -> MariaDB. So, this solution is to solve this problem.

For your questions, I left the answer above.

@vuvova
Copy link
Member

vuvova commented Apr 23, 2020

Thanks for the detailed reply!
Sorry for asking the question you've already answered, I suspected it was the case, but I wasn't able to find the answer in the pull request, that's why I asked.

I understand the use case with the gateway, but not quite why it's needed in the server, particularly as a static string. Is it only for testing purposes?

@hzhang87
Copy link

Thanks for the detailed reply!
Sorry for asking the question you've already answered, I suspected it was the case, but I wasn't able to find the answer in the pull request, that's why I asked.

It is OK. These should be asked if the purpose is not clear :)

I understand the use case with the gateway, but not quite why it's needed in the server, particularly as a static string. Is it only for testing purposes?

The major reason that we let MariaDB server return redirection info string instead let Gateway does is because there is numbers of Gateway in the world. If server support that, the MariaDB service providers don't need implement it again in Gateway even if they choose a different one if the Gateway is implemented as a package-bypassed one. Other reasons are, for some providers, they use Gateway instances to support MariaDB server for multiple customers. It is not easy for Gateway to turn on the feature for part of customers but turn off for left ones in same GW instance.

@shih-che
Copy link
Author

shih-che commented May 6, 2020

Hi @vuvova, do you have any further questions/suggestions on this pull request?

@vuvova
Copy link
Member

vuvova commented May 14, 2020

I'm still not convinced that four parameters are better than one. One is extendable, we just add a validity check inside the server to complain, like, "invalid redirection url". And one can use any valid JDBC redirection, potentially. Presuming clients support it. For example, you redirect to a different user. It is quite possible that at some point somebody will want to redirect to a different database. Or use more parameters in addition to ttl. We don't really want 10-20 redirection server parameters to assemble one url when we can just ask for the url in the first place.

Also, I still don't see what's the point of having one static redirection setting in the server. It will redirect all clients to another server, why is it useful? It can be useful to redirect based on the user name, or server load, or incoming client ip adress. I don't know, based on some configurable rules. But one static "redirect everybody there" configuration? I don't understand why one might need it.

just to clarify, this, above, is a discussion, I'm just asking questions, please, don't take anything above as a suggestion that you need to implement anything — for now this is only a discussion, exchange of ideas

@hzhang87
Copy link

I'm still not convinced that four parameters are better than one. One is extendable, we just add a validity check inside the server to complain, like, "invalid redirection url". And one can use any valid JDBC redirection, potentially. Presuming clients support it. For example, you redirect to a different user. It is quite possible that at some point somebody will want to redirect to a different database. Or use more parameters in addition to ttl. We don't really want 10-20 redirection server parameters to assemble one url when we can just ask for the url in the first place.

The major reason is user experience.
The POS part of using one parameter is we will not involve too many parameters. The NEG part is this gives flexible to end user input any format of string. We can of course add a validity check inside the server to complain, but this is not friendly to cloud users. Today most cloud provider, like AWS, Microsoft, GCP, Ali RDS allows user input server parameter by multiples ways like web portal, command line, JSON/XML-based template and so on. If end user inputs an incorrect format of a parameter and start server, the server will fail. In this case, I believe numbers of provider have not provided an straightforward way as on-premise MariaDB let customer quickly check why the server cannot start in short time through console. Then end-user may suffer. Of course, cloud provider can validate the format of parameter again when user input, but it will duplicate the logic as server side and possibly has hole because they are different pieces of logic .
Moreover, the user also need to read the document carefully to learn the format of string before input parameter. That may slightly increase learning curve.
In short, my point of view is if we only consider on-premise mariadb, I would say technically one parameter could help. But if we also consider user experience using ones from database providers, four with each one is simple will be more friendly.

Also, I still don't see what's the point of having one static redirection setting in the server. It will redirect all clients to another server, why is it useful? It can be useful to redirect based on the user name, or server load, or incoming client ip adress. I don't know, based on some configurable rules. But one static "redirect everybody there" configuration? I don't understand why one might need it.

Because we have found a requirement, the major purpose is to bypass Gateway when database provider uses it. At least in our scenario, the IP of backend server may change dynamically caused by failover, but Gateway not. This could be typical architecture of single server HA. In this situation, there is a needs to bypass Gateway for some set of users who expect low latency of connection set up and low query latency but meanwhile accept losing features of Gateway. For these users, we will redirect all connections to server directly (bypass Gateway).
Technically, it can be redirected based on conditional facts as you mentioned like user name, server load, or incoming client IP. We can also add more parameter in additional to TTL . But this will make the feature complicated while we don't see any user requirement when supporting our customers... Of course it can be discussed if you know any requirement..

just to clarify, this, above, is a discussion, I'm just asking questions, please, don't take anything above as a suggestion that you need to implement anything — for now this is only a discussion, exchange of ideas
Sure. Of course.

@ebrandsberg
Copy link

Heimdall data has joined the conversation...

We wanted to provide some context for why redirection will be important, for very different reasons than the Azure Gateway situation. Use cases:

  1. Customer moves a database from one server cluster to another server cluster. We need to be able to redirect clients connecting to the original cluster to the new cluster without disrupting connections for the other databases;
  2. In the case of a multi-master Galera cluster or similar, to be able to redirect based on user or database in order to balance traffic out between nodes, or in the event a node is going to auto-scale (up or down), to be able to shift traffic on or off of servers.
  3. In the event of a gateway or proxy that performs additional function such as caching (our use case), to be able to shift traffic between proxy nodes based on autoscaling, and/or to provide localization of cache based on important criteria.

One of the important aspects that we have considered that I don't think has been discussed here is providing information to the client on WHY a redirect was generated. Was it because of the username, the database, or the source client IP? Or something else? This is extremely important, as a single client server may be connecting to multiple databases or using multiple users, and may need to be redirected to multiple servers at the same time for different data, despite initially connecting to one server to begin with.

We are working on the same type of logic for Postgres (as is Microsoft), and our concept is to separate each field of a redirect out, make each optional, and then include a "selector" field that provides one or more field names that were used as part of the decision making logic. This will allow the client to apply the proper redirect to new connections without having to be redirected again, reducing the latency of new connects. We are working to engage Microsoft in the next few weeks to hopefully come up with a clean and consistent methodology that supports a wide variety of use cases, and is easily adapted to new use cases as well. As the intention is to do this for the MySQL protocol and Postgres, IMHO, the methodology should be as close to identical as possible across database types for consistency reasons as well.

@robertbindar robertbindar self-assigned this Dec 6, 2021
Copy link
Contributor

@robertbindar robertbindar left a comment

Choose a reason for hiding this comment

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

There seems to be no consensus here on whether to use a single system variable or multiple system variables for configuring the redirection feature.
I see @vuvova 's concern on how this feature has the potential to introduce many many system vars for configuring future usecases.
I also see the contributors side on how a configurable URL option will result in lots of tickets from clients getting the URL wrong / duplication of validity check logic / etc.
It's such a pity though that we got stuck into this detail when the code itself is reviewed and in good condition. Can we @vuvova @hzhang87 @shih-che find a middle solution to this? Maybe:

  • redirect_enabled system var
  • redirect_enabled=0 (default) means redirection is disabled
  • redirect_enabled=1 means redirection is enabled and
  • redirect_config="host=localhost, port=123, ttl=15, database= db" configures the redirection itself in an extensible way, but maybe a bit more user-friendly than a URL (feel free to propose a more intuitive/better format here)

@vuvova, is there an existing system var class that can support something like above or this might need a new implementation?

@montywi
Copy link
Contributor

montywi commented Dec 14, 2021

I am all for using multiple variables, especially as this will be what will be used for other databases (as far as I understand from the discussion above).
This is better for the user, easier to document and allows us to get better error messages.

@montywi
Copy link
Contributor

montywi commented Dec 14, 2021

Better to get this done, the patch has waited way too long!

@vuvova
Copy link
Member

vuvova commented Dec 17, 2021

@ebrandsberg, you wrote "working to engage Microsoft in the next few weeks to hopefully come up with a clean and consistent methodology" — how did that work out? what did you end up with?

@grooverdan grooverdan changed the title Add redirection support MDEV-15935 Add redirection support Jun 21, 2022
@grooverdan
Copy link
Member

Some discussion on a more general and flexible design is in MDEV-15935. Feedback would be much appreciated.

@vuvova
Copy link
Member

vuvova commented Apr 21, 2023

the specs have changed, this implementation no longer corresponds to what we want to do

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.