Skip to content

replication: Improve COM_REGISTER_SLAVE #967

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
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
data[pos] = uint8(len(b.cfg.Password))
data[pos] = uint8(0)
pos++
n = copy(data[pos:], b.cfg.Password)
Copy link
Collaborator

@lance6716 lance6716 Jan 10, 2025

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

@Tang8330 Tang8330 Jan 10, 2025

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

@dveeden dveeden Jan 10, 2025

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

Tang8330 commented Jan 10, 2025

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

lance6716 commented Jan 10, 2025

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

@lance6716 lance6716 lance6716 approved these changes

@atercattus atercattus Awaiting requested review from atercattus

+1 more reviewer

@Tang8330 Tang8330 Tang8330 left review comments

Reviewers whose approvals may not affect merge requirements

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