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.

Sign up for GitHub

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

Jump to bottom

replication: Improve COM_REGISTER_SLAVE #967

Merged
merged 2 commits into from
Jan 10, 2025
Merged

replication: Improve COM_REGISTER_SLAVE #967

merged 2 commits into from
Jan 10, 2025

Conversation

dveeden
Copy link
Collaborator

@dveeden dveeden commented Jan 10, 2025 •
edited
Loading

This:

  • Stops sending the password in COM_REGISTER_SLAVE
  • Sends SET @replica_uuid= before running COM_REGISTER_SLAVE

Impact:

  • SHOW REPLICAS shows an empty password even if --show-replica-auth-info is used.
  • COM_REGISTER_SLAVE won't fail due to too long passwords
  • SHOW REPLICAS now shows the Replica_UUID correctly.

Closes #966

lance6716
lance6716 reviewed Jan 10, 2025
View reviewed changes
replication/binlogsyncer.go
pos++
n = copy(data[pos:], b.cfg.Password)
Copy link
Collaborator

Choose a reason for hiding this comment

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

😂 Don't know why we bother to send password before. Does different version of server has different behaviour?

Will take a look soon

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh interesting, we don't need to send the password?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The password is only for use in the output of SHOW REPLICAS if --show-replica-auth-info is enabled. This makes it a bit of a security hazard and with the length limitation also quite useless.

@Tang8330
Copy link
Contributor

Can confirm that this fixes the issue that I was previously seeing. Thanks for the fix @dveeden !

@dveeden
replication: Fix replica UUID registration
6cbab12 
If we send `SET @replica_uuid=` after `COM_REGISTER_SLAVE` it won't be
picked up in `SHOW REPLICAS`.
@dveeden
Copy link
Collaborator Author

dveeden commented Jan 10, 2025

@lance6716 not sure how to best test this, could you help with that?

  1. Create a user with a password that is >32 chars long.
  2. Run a binlog syncer
  3. Run SHOW REPLICAS, expect one row with Replica_UUID to be a valid UUID

@lance6716
Copy link
Collaborator

Create a user with a password that is >32 chars long.
Run a binlog syncer
Run SHOW REPLICAS, expect one row with Replica_UUID to be a valid UUID

I have checked it. Merging

@lance6716 lance6716 merged commit 14069d3 into go-mysql-org:master Jan 10, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tang8330 Tang8330 Tang8330 left review comments

@lance6716 lance6716 lance6716 approved these changes

@atercattus atercattus Awaiting requested review from atercattus

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Why does binlog syncer need to register as a slave?
3 participants
@dveeden @Tang8330 @lance6716