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.

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):

Done, but I kept some parts of the commentary.

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

Done.

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

Done.

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

Done.

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

Done.

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

Done.

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

Done, and added commentary explaining the motivation behind the "disabled" verbiage.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @benbardin, @knz, and @rharding6373)

pkg/sql/multitenant_admin_function_test.go line 456 at r2 (raw file):

				result: [][]string{{"\xf0\x89\x89", "/1", maxTimestamp}},
			},
			secondary: tenantExpected{

These tenantExpected blocks should not be removed since they are verifying output for certain tenant setups.

pkg/sql/multitenant_admin_function_test.go line 466 at r2 (raw file):

			},
			queryClusterSetting: sql.SecondaryTenantSplitAtEnabled,
			queryCapability:     tenantcapabilities.CanAdminSplit,

Change queryCapability to type capability struct { id CapabilityID, value bool } and set value: false for the split and scatter tests.

pkg/sql/multitenant_admin_function_test.go line 501 at r2 (raw file):

			},
			setupClusterSetting: sql.SecondaryTenantSplitAtEnabled,
			setupCapability:     tenantcapabilities.CanAdminSplit,

The setupCapability field can be removed from type testCase struct now because it was simulating split being enabled by default.

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

pkg/sql/multitenant_admin_function_test.go line 456 at r2 (raw file):

All these test cases that have been removed have functionally been moved to the datadriven tests in pkg/ccl/multitenantccl/tenantcapabilitiesccl. Now that we have that framework, it's a much better place to test stuff like this.

pkg/sql/multitenant_admin_function_test.go line 466 at r2 (raw file):

Why?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @benbardin, @knz, and @rharding6373)

pkg/sql/multitenant_admin_function_test.go line 456 at r2 (raw file):

Can we discuss this change first? It looks like we are replacing data driven testing with a DSL that requires a lot of extra code.

pkg/sql/multitenant_admin_function_test.go line 466 at r2 (raw file):

So that you can make the tests pass without removing test coverage here.

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

pkg/sql/multitenant_admin_function_test.go line 456 at r2 (raw file):

I tried making changes to this test for a little bit before I gave up because of how cumbersome it was. As this code churns and stuff changes, the maintainability burden of this thing is high. I've personally felt it on 2 different occasions, so I finally decided to bite the bullet and move some of the things that were in my way to the datadriven tests. Those are much earier to read and easier to change/write as well.

I'm happy to get @knz to sign off on these changes if you're not happy with them. I do want to get this thing merged soon given it's a release blocker and overall this change is positive.

pkg/sql/multitenant_admin_function_test.go line 466 at r2 (raw file):

I see. Given we haven't lost any test coverage because things have merely moved, I think this is fine.

As discussed elsewhere - either don't change the existing test framework structure (and follow established patterns) OR ensure this is done in a separate commit for clarity.

Reviewed 18 of 18 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @benbardin, @ecwall, and @rharding6373)

-- commits line 27 at r2:
nit cumbersome

I pulled out the test movement changes into a separate commit, like we spoke about in other places.

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

pkg/sql/multitenant_admin_function_test.go line 501 at r2 (raw file):

We still make use of it, so it can't be removed.

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

pkg/sql/multitenant_admin_function_test.go line 501 at r2 (raw file):

My comment no longer applies after the changes to the Authorizer. To add more words there, earlier, we couldn't remove it because we still needed for capabilities to propagate before making these changes -- setupCapability was providing us that mechanism.

With the Authorizer changes, I tacked on a 3rd commit to clean this stuff up. Thanks for the pointer.

I generally like the 2nd commit. It's very good.

I am going to spend a minute more looking at the 1st and last commit. While I do this, please also contribute your thinking (with a concrete example) to #98483.

Reviewed 33 of 33 files at r3, 34 of 35 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @benbardin, @ecwall, and @rharding6373)

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani, @benbardin, @ecwall, and @knz)

pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_unsplit line 24 at r3 (raw file):

ALTER TABLE t UNSPLIT AT VALUES (0)
----
pq: could not UNSPLIT AT (0): key /Tenant/10/Table/104/1/0 is not the start of a range

Could you also add a test that does a successful UNSPLIT?