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

Use Dba user when Vexec is runAsAdmin #7731

Merged
merged 1 commit into from
Apr 6, 2021

Conversation

makinje16
Copy link
Contributor

Signed-off-by: Malcolm Akinje makinje@slack-corp.com

Description

This PR helps solve issue #7585 by allowing VEngine.Exec to take a runAsAdmin parameter which will decide whether to use the Dba or Filtered db user.

Related Issue(s)

Checklist

  • Should this PR be backported?
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

Impacted Areas in Vitess

Components that this PR will affect:

  • Query Serving
  • VReplication
  • Cluster Management
  • Build/CI
  • VTAdmin

@makinje16 makinje16 requested a review from rafael March 23, 2021 20:09
@makinje16 makinje16 marked this pull request as ready for review March 23, 2021 22:46
Copy link
Member

@rafael rafael left a comment

Choose a reason for hiding this comment

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

Added coupled comments.

One extra thing is that we should add a test to assert this is working as expected. An idea to test this is to add a test in go/vt/vttablet/tabletmanager/vreplication/engine_test.go where before running ExecWithDBa you set up bad credentials for the dba user and expect it to fail.

We can also check with @deepthi if she has other recommendations on how to test this.

go/vt/vttablet/tabletmanager/rpc_vreplication.go Outdated Show resolved Hide resolved
Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

LGTM overall.
If you wanted to do some more testing you might consider writing a test that explicitly uses the MockDbaClient.

Signed-off-by: Malcolm Akinje <makinje@slack-corp.com>
Copy link
Member

@rafael rafael left a comment

Choose a reason for hiding this comment

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

All comments have been addressed. This looks good @makinje16 !

@rafael rafael merged commit 2c361ea into vitessio:master Apr 6, 2021
@rafael rafael deleted the vitess-7585-dba-vrep branch April 7, 2021 16:53
rafael added a commit to tinyspeck/vitess that referenced this pull request Apr 7, 2021
@systay systay added Component: VReplication Type: Enhancement Logical improvement (somewhere between a bug and feature) labels Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: VReplication Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants