*: Prep work for supporting CONFIGURE ZONE in declarative schema changer #117572
Conversation
|
This change is |
Xiang-Gu
requested review from
adityamaru,
srosenberg,
DarrylWong,
michae2,
fqazi and
a team
and removed request for
a team
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 11 of 11 files at r2, 3 of 3 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @DarrylWong, @michae2, @srosenberg, and @Xiang-Gu)
pkg/sql/schemachanger/scbuild/builder_state.go line 1129 at r4 (raw file):
} if p.RequiredPrivilege != 0 { if err := b.checkPrivilege(rel.GetID(), p.RequiredPrivilege); err != nil {
We could have a helper called requirePrivilege which does the panic, avoid duplication
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/dependencies.go line 226 at r3 (raw file):
// CheckGlobalPrivilege panics if the current user does not have the specified // global privilege. CheckGlobalPrivilege(privilege privilege.Kind) error
I think this fine. Just to play devils advocate is there any case where the element model will be responsible for granting the privilege and we need to be aware?
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/dependencies.go line 338 at r4 (raw file):
// descriptor. // If 0, no privilege checking is performed. RequiredPrivilege privilege.Kind
Have a constant for this behaviour?
There was a problem hiding this comment.
This is all pretty good, just some minor comments / suggestions
Reviewable status:
*tree.SetZoneConfig has type `DCL` (Data Control Language) instead of DDL. Our DSC won't be able to process non-DDL stmt unless it also implements the `tree.canModifySchema` interface, which was introduced to deal with exactly this kind of situation (i.e. something that's not a DDL but modifies database schema). This commit adds *tree.SetZoneConfig as one of the stmt that can modify schema. Release note: None
This commits mechanically moved the following two cluster settings into the sqlclustersettings package: - SecondaryTenantZoneConfigsEnabled - SecondaryTenantsAllZoneConfigsEnabled This will moved one helper function into the sqlclustersettings package: - RequireSystemTenantOrClusterSetting This way, they can be accessed from within the DSC. Release note: None
Previously, in the builder of declarative schema changer, we have a function `CheckPrivilege(scpb.Element, privilege.Kind)` to check whether current user has `privilege` on the descriptor that the element belongs to. This was written on the assumption that privileges are tied to descriptors. However, privileges can also live in the `system.privileges` table (i.e. those so-called system-level privileges) and that function does not allow us to check for those system-level privileges for current user. This commit therefore introduced a new method `CheckGlobalPrivilege` into the interface for this purpose. I have considered somehow augment the current `CheckPrivilege` to do both but it seems kinda ugly, mostly because we'd need a unified function signature that accepts both an element and the syntheticGlobalPrivilegeObject. Besides, for descriptor-homed privileges, the builder layer caches them so there are some bespoke logic as well. I eventually determined that adding a new method for checking global (aka system-level privileges) is much easier, albeit the slightly un-unified interface. Release note: None
Xiang-Gu
force-pushed
the
prep-configure-zone-1
branch
2 times, most recently
from
809ba3a
to
ff881fc
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/schemachanger/scbuild/builder_state.go line 1129 at r4 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
We could have a helper called requirePrivilege which does the panic, avoid duplication
done. In fact, I went a step further to push the check for priv == 0 { return } down into hasPrivileges to further reduce duplication. This will allow us to call into CheckPrivilege with priv==0.
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/dependencies.go line 226 at r3 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
I think this fine. Just to play devils advocate is there any case where the element model will be responsible for granting the privilege and we need to be aware?
I think when we implement GRANT/REVOKE in the declarative schema changer, we will need to introduce elements whose transitioning to PUBLIC/REVOKE emits operations to grant/revoke privileges to descriptors (or to grant/revoke global privileges).
Why? Does that somehow affect/change anything here?
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/dependencies.go line 338 at r4 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
Have a constant for this behaviour?
I have some worries that adding a new constant to the list of privileges in privileges/kind.go is going to make the privilege API more confusing. I discussed this with @rafiss offline and he agrees that we can just do if priv == 0 without introducing a new privilege constant.
In declarative schema changer, we have a series of ResolveXxx function
that resolves a database object by name into its element set. Those
function also performs privilege checking via the input
`p.RequiredPrivilege`.
There are at least two use cases where such a design seems rigid:
1. "I just want to resolve something without checking any privilege"
2. "I want the privilege checking to support boolean logic. E.g.
It's not uncommon that we require current user to have priv1_AND_priv2,
or priv1_OR_priv2 or some combination of them"
The way I propose to enable more flexible privilege checking is as follows:
- Modify hasPrivilege such that if privilegeKind == 0, no privilege
checking is performed.
- Change `CheckPrivilege` function to return an error (instead of panic)
This together will allow the following examplery usage to perform
arbitrary privilege checking on an object:
```
elems := b.ResolveDatabase(dbName, resolveParameter{}) // empty RequiredPrivilege
dbElem := elems.FilterDatabase().MustGetOneElement()
dbPriv1Err := b.CheckPrivilege(dbElem, priv1)
dbPriv2Err := b.CheckPrivilege(dbElem, priv2)
....
// Perform the arbitrary privilege checking here using those dbPrivErrs
// E.g. we require (priv1 OR priv2) to proceed further
if !(dbPriv1Err == nil || dbPriv2Err == nil) {
panic(privilege checking failed; priv1 and priv2 are both not met)
}
```
Xiang-Gu
force-pushed
the
prep-configure-zone-1
branch
from
ff881fc
to
6598a76
Compare
There was a problem hiding this comment.
Reviewed 15 of 15 files at r5, 3 of 11 files at r6.
Reviewable status:
pkg/sql/schemachanger/scbuild/builder_state.go line 1129 at r4 (raw file):
Previously, Xiang-Gu (Xiang Gu) wrote…
done. In fact, I went a step further to push the check for
priv == 0 { return }down intohasPrivilegesto further reduce duplication. This will allow us to call into CheckPrivilege with priv==0.
Nice!
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/dependencies.go line 226 at r3 (raw file):
Previously, Xiang-Gu (Xiang Gu) wrote…
I think when we implement
GRANT/REVOKEin the declarative schema changer, we will need to introduce elements whose transitioning to PUBLIC/REVOKE emits operations to grant/revoke privileges to descriptors (or to grant/revoke global privileges).Why? Does that somehow affect/change anything here?
Nope, just thinking if we need to poke at these interfaces in the future, but this in the builder so it can fetch those elements if needed.
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/dependencies.go line 338 at r4 (raw file):
Previously, Xiang-Gu (Xiang Gu) wrote…
I have some worries that adding a new constant to the list of privileges in
privileges/kind.gois going to make the privilege API more confusing. I discussed this with @rafiss offline and he agrees that we can just doif priv == 0without introducing a new privilege constant.
I was just suggesting a constant in the scbuild package not a global one.
|
TFTR! bors r+ |
|
Build succeeded: |
This PR consists several preparation work that will be needed for properly supporting CONFIGURE ZONE stmts in declarative schema changer. They're separated out for easier review and bc I expect those to be a lot less controversial than the main work. See each commit for details.
Informs #117574
Epic: CRDB-31473