-
Notifications
You must be signed in to change notification settings - Fork 4k
sql: add REPLICATION roleoption for replication mode #106082
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
Conversation
rafiss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do a full review in a bit, but one thing to note: in this 23.2 we want to be making sure there is a system privilege for each role option (even the ones that are PG only).
interestingly, in ccdbc76 it seems that we already added a REPLICATION system privilege, for a different purpose. i think it's OK to reuse that name, but when we do the role option checks, we should also be checking the system privilege, like how do privs.CheckPrivilege(user, privilege.NOSQLLOGIN) further up in user.go
|
nice of you to work on a public holiday 😂
gotcha, i added what i think you meant |
|
i'm ok to re-use it; the replication privilege can mean anything to do with replication |
rafiss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good! my comments are all minor
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @nkodali and @otan)
-- commits line 7 at r5:
nit: could the release note mention that there is also a corresponding system privilege
pkg/sql/user.go line 349 at r6 (raw file):
aInfo.CanLoginSQLRoleOpt = false case "REPLICATION": fmt.Printf(">>> opt replication\n")
nit: extra print
pkg/sql/pgrepl/connect_test.go line 70 at r6 (raw file):
t.Run(fmt.Sprintf("hasPrivilege=%t/useRoot=%t/replicationMode=%s", tc.hasPrivilege, tc.useRoot, tc.replicationMode), func(t *testing.T) { if tc.hasPrivilege { sqlDB.Exec(t, `ALTER USER testuser REPLICATION`)
can we do something here to test that the system privilege works too?
otan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @nkodali and @rafiss)
pkg/sql/pgrepl/connect_test.go line 70 at r6 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
can we do something here to test that the system privilege works too?
Done.
rafiss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 6 files at r1, 1 of 9 files at r5, 9 of 9 files at r8, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @otan)
|
bors r=rafiss thx! |
|
Build failed (retrying...): |
|
bors r- Rebase drift |
|
Canceled. |
Matches PostgreSQL implementation of the REPLICATION roleoption. Release note (sql change): Added the REPLICATION role option for a user, which allows a user to use the streaming replication protocol. There is a corresponding REPLICATION system privilege to this as well.
In PG, the REPLICATION roleoption is required to use streaming replication mode. Enforce the same constraint. Release note: None
|
bors r=rafiss |
|
Build succeeded: |
These commits add the REPLICATION roleoption (as per PG), and then uses it to authenticate whether a user can use the replication protocol.
Informs #105130
sql: add REPLICATION roleoption
Matches PostgreSQL implementation of the REPLICATION roleoption.
Release note (sql change): Added the REPLICATION role option for a user,
which allows a user to use the streaming replication protocol.
sql: only allow REPLICATION users to login with replication mode
In PG, the REPLICATION roleoption is required to use streaming
replication mode. Enforce the same constraint.
Release note: None