Skip to content
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

tenantcapabilities: introduce TenantCapabilities proto #94644

Merged
merged 1 commit into from
Jan 5, 2023

Conversation

arulajmani
Copy link
Collaborator

@arulajmani arulajmani commented Jan 3, 2023

This commit adds the skeletal structure for a TenantCapabilities proto,
which is intended to encapsulate capabilities for a specific tenant.
Capabilities are intended to be stored in the system.tenants table,
in its Info column. To that end, we modify TenantInfo to contain
capabilities. However, actually populating this field through SQL is
left to a future commit.

Future commits will also add the infrastructure required to check a
tenant's requests against its capabilities for "privileged" operations.
For now, I've only accounted for the CanAdminSplit capability -- this
will likely expand to a fuller set as we introduce other capabilities
in the system.

References #94643

Epic: CRDB-18503
Release note: None

@arulajmani arulajmani requested review from knz and ecwall January 3, 2023 19:43
@arulajmani arulajmani requested review from a team as code owners January 3, 2023 19:43
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@arulajmani arulajmani force-pushed the ten-capabilities-protos branch from 2b1f53e to 2ad1571 Compare January 4, 2023 03:05
@arulajmani arulajmani requested a review from a team January 4, 2023 03:05
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR gives us a protobuf but it does not give us an API to test the capability. What would the API look like?

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ecwall)

@postamar
Copy link

postamar commented Jan 4, 2023

It's hard to have an opinion about this change given that it's not exercised anywhere. What we are able to judge appears to be fairly uncontroversial, too. Perhaps this commit would be better reviewed as part of a more substantial changeset?

@postamar
Copy link

postamar commented Jan 4, 2023

As an aside, why are TenantInfo and friends in the descpb package at all? I don't understand why they'd belong there. Consider moving the lot to pkg/multitenant/tenantcapabilities/tenantcapabilitiespb?

Copy link
Collaborator Author

@arulajmani arulajmani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR gives us a protobuf but it does not give us an API to test the capability. What would the API look like?

Here's a rough sketch for what I had in mind. It tries to keep all capability logic inside the tenantcapabilities package, by expecting callers to pass in requests, instead of pulling the concept above in auth_tenant.go. I'm happy to explore other alternatives as well.

// CapabilitiesWatcher presents a consistent snapshot of tenant capabilities
// that is incrementally (and transparently) maintained by watching for changes
// to the global tenant capability state.
type CapabilitiesWatcher interface {
	// HasCapabilityForBatch returns whether a tenant, referenced by its ID, is
	// allowed to execute the supplied batch request given the capabilities it
	// possesses.
	HasCapabilityForBatch(roachpb.TenantID, *roachpb.BatchRequest) bool
}

Perhaps this commit would be better reviewed as part of a more substantial changeset?

I agree. However, there's a bit of parallelization to be had here with the work @ecwall is doing. He's going to be working on the set/alter path for tenant capabilities, which will make use of these protos.

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

@arulajmani
Copy link
Collaborator Author

As an aside, why are TenantInfo and friends in the descpb package at all? I don't understand why they'd belong there. Consider moving the lot to pkg/multitenant/tenantcapabilities/tenantcapabilitiespb?

TenantInfo contains more than just capabilities, so I'm not sure if tenantcapabilitiespb is the right place either. I can create a new package and move it there in a followup commit. @postamar to confirm, it's safe to move the go package here as long as the protobuf package name doesn't change, right? This proto is persisted in the info column of system.tenants.

This commit adds the skeletal structure for a `TenantCapabilities` proto,
which is intended to encapsulate capabilities for a specific tenant.
Capabilities are intended to be stored in the `system.tenants` table,
in its Info column. To that end, we modify `TenantInfo` to contain
capabilities. However, actually populating this field through SQL is
left to a future commit.

Future commits will also add the infrastructure required to check a
tenant's requests against its capabilities for "privileged" operations.
For now, I've only accounted for the `CanAdminSplit` capability -- this
will likely expand to a fuller set as we introduce other capabilities
in the system.

References cockroachdb#94643

Epic: CRDB-18503
Release note: None
@arulajmani arulajmani force-pushed the ten-capabilities-protos branch from 3bd5bd2 to e5a7093 Compare January 5, 2023 16:35
@arulajmani arulajmani requested a review from a team as a code owner January 5, 2023 16:35
@arulajmani arulajmani requested review from benbardin and stevendanna and removed request for a team and benbardin January 5, 2023 16:35
@arulajmani
Copy link
Collaborator Author

TFTR!

bors r=ecwall

@craig
Copy link
Contributor

craig bot commented Jan 5, 2023

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants