Skip to content

Commit

Permalink
tenantcapabilities: restructure interfaces
Browse files Browse the repository at this point in the history
This commit is mostly just a refactoring of what we have. The previous
iteration had some unfortunate coupling between the protobufs and the
top-level package. Having the protobuf generated code not be a leaf hurts.

Another pain point was the lack of strong typing when interacting with
capabilities. Given this is go, somewhere there's a need to do some
type assertion when dealing sets of variant instance types.

The biggest change is that the `TenantCapabilities` concept is no longer
wrapped in an interface implemented by the protobuf. Instead, the `Capability`
implementations now live inside the `tenantcapabilities` package, and can
be looked up with `FromName` and `FromID`, without any access to concrete
capabilities. These capabilies then know how to look up values in a strongly
typed way from the concrete capabilities.

Epic: none

Release note: None
  • Loading branch information
ajwerner committed Mar 31, 2023
1 parent 56bb002 commit f466673
Show file tree
Hide file tree
Showing 60 changed files with 634 additions and 541 deletions.
4 changes: 2 additions & 2 deletions pkg/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -255,8 +255,8 @@ ALL_TESTS = [
"//pkg/kv/kvserver:kvserver_test",
"//pkg/kv:kv_test",
"//pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer:tenantcapabilitiesauthorizer_test",
"//pkg/multitenant/tenantcapabilities/tenantcapabilitiespb:tenantcapabilitiespb_test",
"//pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher:tenantcapabilitieswatcher_test",
"//pkg/multitenant/tenantcapabilities:tenantcapabilities_test",
"//pkg/multitenant/tenantcostmodel:tenantcostmodel_test",
"//pkg/obsservice/obslib/ingest:ingest_test",
"//pkg/roachpb:roachpb_disallowed_imports_test",
Expand Down Expand Up @@ -1360,11 +1360,11 @@ GO_TARGETS = [
"//pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer:tenantcapabilitiesauthorizer",
"//pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer:tenantcapabilitiesauthorizer_test",
"//pkg/multitenant/tenantcapabilities/tenantcapabilitiespb:tenantcapabilitiespb",
"//pkg/multitenant/tenantcapabilities/tenantcapabilitiespb:tenantcapabilitiespb_test",
"//pkg/multitenant/tenantcapabilities/tenantcapabilitiestestutils:tenantcapabilitiestestutils",
"//pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher:tenantcapabilitieswatcher",
"//pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher:tenantcapabilitieswatcher_test",
"//pkg/multitenant/tenantcapabilities:tenantcapabilities",
"//pkg/multitenant/tenantcapabilities:tenantcapabilities_test",
"//pkg/multitenant/tenantcostmodel:tenantcostmodel",
"//pkg/multitenant/tenantcostmodel:tenantcostmodel_test",
"//pkg/multitenant:multitenant",
Expand Down
4 changes: 3 additions & 1 deletion pkg/bench/foreachdb.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,9 @@ func benchmarkSharedProcessTenantCockroach(b *testing.B, f BenchmarkFn) {
if !found {
return errors.Newf("capabilities not yet ready")
}
if !capabilities.GetBool(tenantcapabilities.ExemptFromRateLimiting) {
if !tenantcapabilities.MustGetBoolByID(
capabilities, tenantcapabilities.ExemptFromRateLimiting,
) {
return errors.Newf("capabilities not yet ready")
}
return nil
Expand Down
9 changes: 9 additions & 0 deletions pkg/ccl/logictestccl/testdata/logic_test/tenant_capability
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ can_admin_unsplit false
can_view_node_info false
can_view_tsdb_metrics false
exempt_from_rate_limiting false
span_config_bounds {}

subtest end

Expand All @@ -60,6 +61,7 @@ can_admin_unsplit false
can_view_node_info false
can_view_tsdb_metrics false
exempt_from_rate_limiting false
span_config_bounds {}

statement ok
ALTER TENANT "bool-capability-no-value-tenant" REVOKE CAPABILITY can_admin_split
Expand All @@ -75,6 +77,7 @@ can_admin_unsplit false
can_view_node_info false
can_view_tsdb_metrics false
exempt_from_rate_limiting false
span_config_bounds {}

subtest end

Expand All @@ -97,6 +100,7 @@ can_admin_unsplit false
can_view_node_info false
can_view_tsdb_metrics false
exempt_from_rate_limiting false
span_config_bounds {}

subtest end

Expand All @@ -119,6 +123,7 @@ can_admin_unsplit false
can_view_node_info false
can_view_tsdb_metrics false
exempt_from_rate_limiting false
span_config_bounds {}

subtest end

Expand All @@ -141,6 +146,7 @@ can_admin_unsplit false
can_view_node_info true
can_view_tsdb_metrics false
exempt_from_rate_limiting false
span_config_bounds {}

statement ok
ALTER TENANT "multiple-capability-tenant" REVOKE CAPABILITY can_admin_split, can_view_node_info
Expand All @@ -156,6 +162,7 @@ can_admin_unsplit false
can_view_node_info false
can_view_tsdb_metrics false
exempt_from_rate_limiting false
span_config_bounds {}

statement ok
ALTER TENANT "multiple-capability-tenant" GRANT CAPABILITY exempt_from_rate_limiting
Expand All @@ -171,6 +178,7 @@ can_admin_unsplit false
can_view_node_info false
can_view_tsdb_metrics false
exempt_from_rate_limiting true
span_config_bounds {}

statement ok
ALTER TENANT "multiple-capability-tenant" REVOKE CAPABILITY exempt_from_rate_limiting
Expand All @@ -186,5 +194,6 @@ can_admin_unsplit false
can_view_node_info false
can_view_tsdb_metrics false
exempt_from_rate_limiting false
span_config_bounds {}

subtest end
2 changes: 1 addition & 1 deletion pkg/gen/stringer.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ STRINGER_SRCS = [
"//pkg/kv/kvpb:method_string.go",
"//pkg/kv/kvserver/closedts/sidetransport:cantclosereason_string.go",
"//pkg/kv/kvserver:refreshraftreason_string.go",
"//pkg/multitenant/tenantcapabilities:capabilityid_string.go",
"//pkg/multitenant/tenantcapabilities:id_string.go",
"//pkg/sql/catalog/catalogkeys:commenttype_string.go",
"//pkg/sql/catalog/catpb:privilegedescversion_string.go",
"//pkg/sql/catalog/descpb:formatversion_string.go",
Expand Down
1 change: 0 additions & 1 deletion pkg/kv/kvserver/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,6 @@ go_test(
"//pkg/settings",
"//pkg/settings/cluster",
"//pkg/spanconfig",
"//pkg/spanconfig/spanconfigbounds",
"//pkg/spanconfig/spanconfigptsreader",
"//pkg/spanconfig/spanconfigstore",
"//pkg/sql",
Expand Down
8 changes: 5 additions & 3 deletions pkg/kv/kvserver/client_spanconfigs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
"github.com/cockroachdb/cockroach/pkg/spanconfig"
"github.com/cockroachdb/cockroach/pkg/spanconfig/spanconfigbounds"
"github.com/cockroachdb/cockroach/pkg/spanconfig/spanconfigstore"
"github.com/cockroachdb/cockroach/pkg/sql/isql"
"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
Expand All @@ -42,7 +41,7 @@ func TestSpanConfigUpdateAppliedToReplica(t *testing.T) {
spanConfigStore := spanconfigstore.New(
roachpb.TestingDefaultSpanConfig(),
cluster.MakeTestingClusterSettings(),
spanconfigbounds.NewEmptyReader(),
spanconfigstore.NewEmptyBoundsReader(),
nil,
)
var t0 = time.Date(2000, time.January, 1, 0, 0, 0, 0, time.UTC)
Expand Down Expand Up @@ -110,7 +109,10 @@ func TestFallbackSpanConfigOverride(t *testing.T) {

st := cluster.MakeTestingClusterSettings()
spanConfigStore := spanconfigstore.New(
roachpb.TestingDefaultSpanConfig(), st, spanconfigbounds.NewEmptyReader(), nil,
roachpb.TestingDefaultSpanConfig(),
st,
spanconfigstore.NewEmptyBoundsReader(),
nil,
)
var t0 = time.Date(2000, time.January, 1, 0, 0, 0, 0, time.UTC)
mockSubscriber := newMockSpanConfigSubscriber(t0, spanConfigStore)
Expand Down
23 changes: 19 additions & 4 deletions pkg/multitenant/tenantcapabilities/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,31 +1,46 @@
load("@io_bazel_rules_go//go:def.bzl", "go_library")
load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
load("//build/bazelutil/unused_checker:unused.bzl", "get_x_data")
load("//build:STRINGER.bzl", "stringer")

go_library(
name = "tenantcapabilities",
srcs = [
"capabilities.go",
"capability.go",
"interfaces.go",
"testingknobs.go",
":capabilityid-stringer", # keep
"value.go",
"values.go",
":id-stringer", # keep
],
importpath = "github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilities",
visibility = ["//visibility:public"],
deps = [
"//pkg/base",
"//pkg/kv/kvpb",
"//pkg/multitenant/tenantcapabilities/tenantcapabilitiespb",
"//pkg/roachpb",
"//pkg/spanconfig/spanconfigbounds",
"//pkg/util/stringerutil",
"@com_github_cockroachdb_errors//:errors",
"@com_github_cockroachdb_redact//:redact",
"@com_github_cockroachdb_redact//interfaces",
],
)

stringer(
name = "capabilityid-stringer",
name = "id-stringer",
src = "capabilities.go",
additional_args = ["--linecomment"],
typ = "CapabilityID",
typ = "ID",
)

go_test(
name = "tenantcapabilities_test",
srcs = ["values_test.go"],
args = ["-test.timeout=295s"],
embed = [":tenantcapabilities"],
deps = ["@com_github_stretchr_testify//require"],
)

get_x_data(name = "get_x_data")
Loading

0 comments on commit f466673

Please sign in to comment.