multitenant: allow secondary tenants to split/scatter by default #98546

arulajmani · 2023-03-13T23:51:20Z

AdminSplit and AdminScatter requests are subject to capability checks.
Previously, these capabilities were codified in the "enabled" form. As
such, by default, secondary tenants did not have the ability to perform
these operations. This is in violation of what secondary tenants could
do prior to 23.1, at a time before capabilities existed. Moreover,
RESTORE/IMPORT rely on performing these operations for performance.
This made disallowing these operations by default a performance
regression.

This patch flips the phrasing of how these capabilities are stored on
the proto to use the "disable" verbiage. As such, secondary tenants are
able to perform splits and scatters by default. However, no change is
made to the public interface -- users above the tenantcapabilitiespb
package continue to interact with these capabilities as they were
before, oblivious to how these things are stored on disk.

There's a few testing changes here:

As part of this change, we also clean up a testing knob that was used
by various backup, CDC, and logictests to override capability checks in
the authorizer. This isn't required with the new default behaviour.
We also add some missing E2E tests for the CanAdminUnsplit capability
which were missing when it was introduced.

Fixes #96736

Release note: None

cockroach-teamcity · 2023-03-13T23:51:32Z

This change is

knz

I like the simplification here.

However, I'd like to request one more step. It's good that we inverted the meaning in the protobuf, but IMHO the public interface should remain unchanged. See my comments below.

Reviewed 36 of 36 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @benbardin, @ecwall, and @rharding6373)

pkg/multitenant/tenantcapabilities/capabilities.go line 165 at r1 (raw file):

	_ CapabilityID = iota

	// DisableAdminScatter disallows a secondary tenant from being able to scatter

I recommend you undo the changes to this file. The original code is easier to read and understand.

pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go line 85 at r1 (raw file):

			continue
		}
		switch request.Method() {

You can revert this change too. Let's keep all the checks under one conditional.

pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go line 152 at r1 (raw file):

	// The following are authorized via specific capabilities.
	kvpb.AdminScatter: tenantcapabilities.DisableAdminScatter,
	kvpb.AdminSplit:   tenantcapabilities.DisableAdminSplit,

ditto

pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.go line 56 at r1 (raw file):

	switch capabilityID {
	case tenantcapabilities.DisableAdminScatter:
		return boolCap{&t.DisableAdminScatter}

case tenantcapabilities.CanAdminScatter:
   return invertedBoolCap{&t.DisableAdminScatter}

pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.go line 58 at r1 (raw file):

		return boolCap{&t.DisableAdminScatter}
	case tenantcapabilities.DisableAdminSplit:
		return boolCap{&t.DisableAdminSplit}

ditto

pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.go line 76 at r1 (raw file):

	switch capabilityID {
	case tenantcapabilities.DisableAdminScatter:
		return t.DisableAdminScatter

case tenantcapabilities.CanAdminScatter:
   return !t.DisableAdminScatter

ditto below

pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto line 34 at r1 (raw file):

  // DisableAdminSplit, if set to true, revokes the tenants ability to
  // successfully perform `AdminSplit` requests.

I recommend you group the two disable fields close to each other in the proto definition then add a comment that explains why they are different from the rest. We must teach people coming after us that any new cap should be added at the end with a default-not-granted definition.

arulajmani

Thanks for the suggestion. I quite like how this ended up looking after going with the invertedBoolField thing. RFAL.

Dismissed @knz from 5 discussions.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @benbardin, @ecwall, @knz, and @rharding6373)

pkg/multitenant/tenantcapabilities/capabilities.go line 165 at r1 (raw file):