sql,server: create toString to role option #75271

maryliag · 2022-01-21T15:41:27Z

Previously, the code was using roleOption.String() to
create a string to be used on queries. This was causing
an issue with roles that contained spaces within it,
e.g. role option VALIDUNTIL was being translate to
VALIDUNTIL instead of the correct way VALID UNTIL.
This commit updates the String() function to correctly
add the space on the VALID UNTIL case.

Release note (bug fix): Update the String() function of
roleOption to add a space on the role VALID UNTIL.

cockroach-teamcity · 2022-01-21T15:41:34Z

This change is

RichardJCai · 2022-01-21T17:51:51Z

pkg/server/admin.go

@@ -3209,7 +3209,7 @@ func (c *adminPrivilegeChecker) hasRoleOption(
 	row, err := c.ie.QueryRowEx(
 		ctx, "check-role-option", nil, /* txn */
 		sessiondata.InternalExecutorOverride{User: user},
-		"SELECT crdb_internal.has_role_option($1)", roleOption.String())
+		"SELECT crdb_internal.has_role_option($1)", roleoption.ToString[roleOption])


nit: I'd prefer if we added a method to RoleOption called ToString() instead of using the map directly

rafiss

can you add a test that would fail without this change?

maryliag

Test added

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai)

pkg/server/admin.go, line 3212 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

nit: I'd prefer if we added a method to RoleOption called ToString() instead of using the map directly

Done

matthewtodd

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

pkg/sql/roleoption/role_option.go, line 169 at r2 (raw file):

// It should be used instead of roleOption.String() to accommodate the cases
// with space on the role (e.g. `VALID UNTIL`).
func (o Option) ToString() string {

Looks like there's some precedence for the name SQLString here, WDYT?

maryliag

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @matthewtodd)

pkg/sql/roleoption/role_option.go, line 169 at r2 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

Looks like there's some precedence for the name SQLString here, WDYT?

Are you referring to the toSQLStmt mapping? I was thinking is just easier to understand/remember ToString than ToSQLString

matthewtodd

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

pkg/sql/roleoption/role_option.go, line 169 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

Are you referring to the toSQLStmt mapping? I was thinking is just easier to understand/remember ToString than ToSQLString

I was talking about renaming this method to SQLString. If you git grep SQLString pkg there are some examples. I was thinking I would have a hard time understanding the difference between String and ToString from the names alone.

maryliag

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @matthewtodd)

pkg/sql/roleoption/role_option.go, line 169 at r2 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

I was talking about renaming this method to SQLString. If you git grep SQLString pkg there are some examples. I was thinking I would have a hard time understanding the difference between String and ToString from the names alone.

Done

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @matthewtodd)

pkg/sql/roleoption/role_option.go, line 169 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

Done

on the other hand, now i wonder if we ever want to use the String() method at all? should we just make String() be the only one?

pkg/sql/roleoption/role_option.go, line 170 at r3 (raw file):

// with space on the role (e.g. `VALID UNTIL`).
func (o Option) SQLString() string {
	return toSQLStringMap[o]

is VALID UNTIL the only one that needs to be handled differently?

i wonder if we should just add a special case here in this function, instead of making a whole new map that needs to be maintained separately (and might be easy to forget to update)

pkg/server/user_test.go, line 41 at r3 (raw file):

	for name := range roleoption.ByName {
		_, err := s.(*TestServer).status.baseStatusServer.privilegeChecker.hasRoleOption(ctx, fooUser, roleoption.ByName[name])
		require.NoError(t, err)

ah i didn't realize it was erroring before; i thought it was returning true/false incorrectly.

should we also add a test to make sure true/false is calculated correctly too? up to you

maryliag

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @matthewtodd, @rafiss, and @RichardJCai)

pkg/sql/roleoption/role_option.go, line 169 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

on the other hand, now i wonder if we ever want to use the String() method at all? should we just make String() be the only one?

Changed the original String function,using the //go:generate stringer -type=Option -linecomment
Thanks Rafi for the tip!

pkg/sql/roleoption/role_option.go, line 170 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

is VALID UNTIL the only one that needs to be handled differently?

i wonder if we should just add a special case here in this function, instead of making a whole new map that needs to be maintained separately (and might be easy to forget to update)

The only one different, so I added the comment on the role with the space

pkg/server/user_test.go, line 41 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

ah i didn't realize it was erroring before; i thought it was returning true/false incorrectly.

should we also add a test to make sure true/false is calculated correctly too? up to you

Add the check if the role existed after being added

matthewtodd

Reviewed 6 of 6 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @maryliag and @rafiss)

-- commits, line 10 at r4:
That new //go:generate line is awesome! Probably worth rewriting the commit message now? Otherwise

rafiss

this looks great! thanks for all the work on this

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @maryliag and @rafiss)

pkg/server/user_test.go, line 62 at r4 (raw file):

		require.NoError(t, err)

		hasRole, err := s.(*TestServer).status.baseStatusServer.privilegeChecker.hasRoleOption(ctx, fooUser, roleoption.ByName[name])

this could also do a check using crdb_internal.has_role_option directly, but not necessary

maryliag

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @matthewtodd and @rafiss)

-- commits, line 10 at r4:

Previously, matthewtodd (Matthew Todd) wrote…

That new //go:generate line is awesome! Probably worth rewriting the commit message now? Otherwise

Updated

pkg/server/user_test.go, line 62 at r4 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

this could also do a check using crdb_internal.has_role_option directly, but not necessary

I prefer keeping the hasRole function because it does a few extra checks and in the end that was the funcional that was causing issues for me and I wanted to test.

Previously, the code was using roleOption.String() to create a string to be used on queries. This was causing an issue with roles that contained spaces within it, e.g. role option `VALIDUNTIL` was being translate to `VALIDUNTIL` instead of the correct way `VALID UNTIL`. This commit updates the String() function to correctly add the space on the `VALID UNTIL` case. Release note (bug fix): Update the String() function of roleOption to add a space on the role `VALID UNTIL`.

maryliag · 2022-01-24T21:02:00Z

TFTR!

bors r+

craig · 2022-01-25T00:44:23Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-01-25T08:21:27Z

Build succeeded:

GitHub CI (Cockroach)

blathers-crl · 2022-01-25T08:21:50Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from 29211b6 to blathers/backport-release-21.1-75271: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.1.x failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

75274: ui: hide statement diagnostics when role VIEWACTIVITYREDACTED r=maryliag a=maryliag When the sql user has the role VIEWACTIVITYREDACTED, all the information about Statement Diagnostics Bundle will be hidden. That happens on: Statement - diagnostics column Statement Details - diagnostics tab Advanced Debug - diagnostics history Fixes #74817 Note: Can only be merged after #75271 and #75273 Release note (ui change): If the user has the role VIEWACTIVITYREDACTED, we hide the Statement Diagnostics bundle info on Statement page (diagnostics column), Statement Details page (diagnostics tab) and Advanced Debug page (diagnostics history). 75479: colflow: finish the span after closing the batch flow coordinator r=yuzefovich a=yuzefovich Previously, the coordinator would be closed after the tracing span is finished. However, this is incorrect in some cases where some components might use the span up until they are closed. This is now fixed by closing the coordinator before finishing the span. Fixes: #75425. Release note: None 75493: backupccl: give each restore goroutine its own rewriter r=dt a=dt Release note: none. 75506: migrations: bump size of `migrations` test r=rail a=rickystewart Release note: None Co-authored-by: Marylia Gutierrez <marylia@cockroachlabs.com> Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com> Co-authored-by: David Taylor <tinystatemachine@gmail.com> Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com>

maryliag requested review from a team January 21, 2022 15:41

maryliag mentioned this pull request Jan 21, 2022

ui: hide statement diagnostics when role VIEWACTIVITYREDACTED #75274

Merged

maryliag added backport-21.1.x 21.1 is EOL backport-21.2.x labels Jan 21, 2022

RichardJCai reviewed Jan 21, 2022

View reviewed changes

rafiss requested changes Jan 21, 2022

View reviewed changes

maryliag force-pushed the fix-roles branch from 6539edd to ff23a9d Compare January 21, 2022 20:01

maryliag commented Jan 21, 2022

View reviewed changes

maryliag force-pushed the fix-roles branch from ff23a9d to a866f0c Compare January 21, 2022 20:31

matthewtodd suggested changes Jan 24, 2022

View reviewed changes

maryliag commented Jan 24, 2022

View reviewed changes

matthewtodd suggested changes Jan 24, 2022

View reviewed changes

maryliag force-pushed the fix-roles branch 2 times, most recently from e511eaa to 3520455 Compare January 24, 2022 17:10

maryliag commented Jan 24, 2022

View reviewed changes

rafiss requested changes Jan 24, 2022

View reviewed changes

maryliag force-pushed the fix-roles branch from 3520455 to 1da0247 Compare January 24, 2022 18:42

maryliag commented Jan 24, 2022

View reviewed changes

matthewtodd approved these changes Jan 24, 2022

View reviewed changes

rafiss approved these changes Jan 24, 2022

View reviewed changes

maryliag force-pushed the fix-roles branch from 1da0247 to de44554 Compare January 24, 2022 19:22

maryliag commented Jan 24, 2022

View reviewed changes

maryliag force-pushed the fix-roles branch from de44554 to 29211b6 Compare January 24, 2022 19:58

craig bot merged commit 44b2ba8 into cockroachdb:master Jan 25, 2022

blathers-crl bot mentioned this pull request Jan 25, 2022

release-21.2: sql,server: create toString to role option #75482

Closed

maryliag deleted the fix-roles branch January 25, 2022 13:32

maryliag mentioned this pull request Jan 25, 2022

release-21.2: sql,server: create toString to role option #75494

Merged

maryliag removed the backport-21.1.x 21.1 is EOL label Jan 25, 2022

maryliag mentioned this pull request Jan 25, 2022

release-21.2: ui: hide statement diagnostics when role VIEWACTIVITYREDACTED #75521

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql,server: create toString to role option #75271

sql,server: create toString to role option #75271

maryliag commented Jan 21, 2022 •

edited

Loading

cockroach-teamcity commented Jan 21, 2022

RichardJCai Jan 21, 2022

rafiss left a comment

maryliag left a comment

matthewtodd left a comment

maryliag left a comment

matthewtodd left a comment

maryliag left a comment

rafiss left a comment

maryliag left a comment

matthewtodd left a comment

rafiss left a comment

maryliag left a comment

maryliag commented Jan 24, 2022

craig bot commented Jan 25, 2022

craig bot commented Jan 25, 2022

blathers-crl bot commented Jan 25, 2022

sql,server: create toString to role option #75271

sql,server: create toString to role option #75271

Conversation

maryliag commented Jan 21, 2022 • edited Loading

cockroach-teamcity commented Jan 21, 2022

RichardJCai Jan 21, 2022

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

matthewtodd left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

matthewtodd left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

matthewtodd left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

maryliag commented Jan 24, 2022

craig bot commented Jan 25, 2022

craig bot commented Jan 25, 2022

blathers-crl bot commented Jan 25, 2022

maryliag commented Jan 21, 2022 •

edited

Loading