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

Disabling AWS RDS topology auto-discovery by default #4578

Merged
merged 4 commits into from
Jul 9, 2024

Conversation

nielsvogell
Copy link

This pull request provides an option to disable the AWS RDS MySQL Multi-AZ DB Cluster auto-discovery feature, added in v2.6.2. It also disables the feature by default and adds an additional check.

Background
Currently, ProxySql regularly (by default every 1000th time checking the read_only status of a server) queries the mysql.rds_topology table on any AWS RDS DB instance or cluster in its server list to find information about database endpoints. It is designed to work with AWS RDS Multi-AZ DB Clusters, which consist of three individual RDS instances whose endpoints are stored in the mysql.rds_topology table and whose names end in -instance-1, -instance-2, and -instance-3, respectively.

Issue
The mysql.rds_topology table may however be used for other purposes in the future, so the endpoints in the table cannot by default be assumed to be instances in a Multi-AZ DB Cluster. If the table contains an endpoint, ProxySQL automatically adds this endpoint to the runtime_mysql_servers table and starts routing traffic to it. Depending on what kind of endpoint is in the mysql.rds_topology, this may result in undesired behavior.

Solution
To prevent accidentally reading from the mysql.rds_topology table, this PR disables the auto-discovery by default and adds an additional check to only process the information in the table if the topology matches a Multi-AZ DB Cluster deployment.
To (re-)enable the auto-discovery for Multi-AZ DB Clusters, in the ProxySql Admin DB, set the global variable mysql-monitor_aws_rds_topology_discovery_interval to a value greater than zero (it used to be 1000) and load the variables to runtime. (As per the the code, the variable determines "How frequently a topology discovery is performed, e.g. a value of 500 means one topology discovery every 500 read-only checks".)

@mirostauder
Copy link
Collaborator

Automated message: PR pending admin approval for build testing

@renecannao
Copy link
Contributor

add to whitelist

@mirostauder
Copy link
Collaborator

retest this please

@nielsvogell
Copy link
Author

Sorry, when making the commit, I accidentally changed the wrong default value. This may have affected some of the tests. Others seem to fail because of missing access to the secrets for pull-requests from forks.

Will run another local test to make sure I didn't miss anything else.

@nielsvogell
Copy link
Author

Done, functionality works as expected. Please let me know if there is anything else in my code that may cause tests to fail. (I don't see any logs so am not sure what causes the failures.)

Copy link
Collaborator

@JavierJF JavierJF left a comment

Choose a reason for hiding this comment

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

Hi, first thanks for your contribution. The review just contains a couple of minor details that would be nice to address (maybe later), and are left as notes. No blockers, PR can be merged already.

lib/MySQL_Monitor.cpp Outdated Show resolved Hide resolved
lib/MySQL_Monitor.cpp Show resolved Hide resolved
@nielsvogell
Copy link
Author

Thanks for reviewing. I have addressed the two comments. Let me know if there are any more issues.

@JavierJF
Copy link
Collaborator

JavierJF commented Jul 5, 2024

Hi @nielsvogell, yw! Thanks for the contribution and for addressing the minor feedback. I have no further comments, the PR is ready to be merged.

@mirostauder
Copy link
Collaborator

retest this please

@renecannao renecannao merged commit 3ce6f99 into sysown:v2.x Jul 9, 2024
16 of 49 checks passed
@nielsvogell nielsvogell deleted the aws_auto-discovery_patch branch July 9, 2024 09:19
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.

4 participants