Skip to content

roachpb,server,spanconfig: introduce RPCs for SystemSpanConfigs#75615

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
arulajmani:protectedts-rpcs-mechanical
Jan 29, 2022
Merged

roachpb,server,spanconfig: introduce RPCs for SystemSpanConfigs#75615
craig[bot] merged 1 commit intocockroachdb:masterfrom
arulajmani:protectedts-rpcs-mechanical

Conversation

@arulajmani
Copy link
Copy Markdown
Collaborator

@arulajmani arulajmani commented Jan 27, 2022

This patch adds two new RPCs -- UpdateSystemSpanConfigs and
GetSystemSpanConfigs to interact with system span configurations.
They're available to tenants via the Connector; we ensure secondary
tenants do not target other tenants.

The KVAccessor returns unimplemented errors for now. Pulling this
mechanical change into its own patch for ease of review.

Release note: None

@arulajmani arulajmani requested a review from a team as a code owner January 27, 2022 15:34
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Jan 27, 2022

This change is Reviewable

@arulajmani arulajmani force-pushed the protectedts-rpcs-mechanical branch 3 times, most recently from cf0610d to 9c3b070 Compare January 27, 2022 21:31
irfansharif
irfansharif approved these changes Jan 28, 2022
View reviewed changes
pkg/rpc/auth_tenant.go

validate := func(target roachpb.SystemSpanConfigTarget) error {
if target.TenantID != nil {
return authError("secondary tenants cannot target tenants for system span configurations")
Copy link
Copy Markdown
Contributor

@irfansharif irfansharif Jan 28, 2022

Choose a reason for hiding this comment

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

Does this check also prevent the system tenant from issuing these RPCs? Not sure where the check happens to see if the fellow issuing the RPC is the system tenant or not.

Copy link
Copy Markdown
Collaborator Author

@arulajmani arulajmani Jan 28, 2022

Choose a reason for hiding this comment

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

Good call, added a check.

@arulajmani arulajmani mentioned this pull request Jan 28, 2022
Closed
@arulajmani
roachpb,server,spanconfig: introduce RPCs for SystemSpanConfigs
5177417 
This patch adds two new RPCs -- `UpdateSystemSpanConfigs` and
`GetSystemSpanConfigs` to interact with system span configurations.
They're available to tenants via the `Connector`; we ensure secondary
tenants do not target other tenants.

The `KVAccessor` returns unimplemented errors for now. Pulling this
mechanical change into its own patch for ease of review.

Release note: None
@arulajmani arulajmani force-pushed the protectedts-rpcs-mechanical branch from 9c3b070 to 5177417 Compare January 28, 2022 16:26
@arulajmani
Copy link
Copy Markdown
Collaborator Author

arulajmani commented Jan 28, 2022

TFTR!

bors r=irfansharif

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jan 28, 2022

Build failed:

@arulajmani
Copy link
Copy Markdown
Collaborator Author

arulajmani commented Jan 28, 2022

bors r+

arulajmani added a commit to arulajmani/cockroach that referenced this pull request Jan 28, 2022
@arulajmani
systemspanconfig: introduce Target and encoding/decoding methods
f987775 
This patch introduces a new `systemspanconfig.Target` type which is
intended as in interemediate representation for system span config
targets throughout the `spanconfig` package. It ties together a
targeter tenant ID with a targetee tenant ID (along with validation
to ensure secondary tenants don't target other tenants).

We also reserve a `SystemSpanConfigSpan` at the end of the System
range. This allows us to assign special meaning to key-prefixes carved
out of this range (when stored in `system.span_configurations`).
By ensuring this span is carved at the end of the system range, we can
ensure that the host tenant does not install span configurations to this
span directly.

We define special key prefixes for system span configurations in this
range and establish a mapping between a Target <-> span using `Encode`
and `Decode` methods. This is useful when we store sysytem span
configurations in `system.span_configurations`  given its schema. Ditto
for reading from the table.

We'll make use of the `Target` in upcoming PRs, specifically in the
`KVAccessor`, `KVSubscriber`, and (the upcoming)
`SystemSpanConfigStore`.

The `KVAccessor` will construct `Targets` using tenant information from
the context as the targeter tenant. It'll use the
`roachpb.SystemSpanConfigTargets` supplied to it by RPCs (See cockroachdb#75615).
The encoding/decoding methods will be used when writing to/reading from
`system.span_configurations`.

While the existing `roachpb.SystemSpanConfigTarget` is sufficient
for our use in the `KVAccessor` (which runs on SQL pods), the richer
structure is motivated by the desire to also use this type in the
`KVSubscriber` (when receiving system span config events) and
`SystemSpanConfigStore` (which is an in-memory representation of the
system span config state). Down in KV we want to distinguish between a
tenant installed system span config on its entire keyspace vs. a host
tenant installed system span config over a tenants keyspace.

Release note: None
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jan 28, 2022

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jan 29, 2022

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jan 29, 2022

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jan 29, 2022

Build succeeded:

@craig craig bot merged commit d3cd817 into cockroachdb:master Jan 29, 2022
arulajmani added a commit to arulajmani/cockroach that referenced this pull request Feb 1, 2022
@arulajmani
systemspanconfig: introduce Target and encoding/decoding methods
f4b5fc8 
This patch introduces a new `systemspanconfig.Target` type which is
intended as in interemediate representation for system span config
targets throughout the `spanconfig` package. It ties together a
targeter tenant ID with a targetee tenant ID (along with validation
to ensure secondary tenants don't target other tenants).

We also reserve a `SystemSpanConfigSpan` at the end of the System
range. This allows us to assign special meaning to key-prefixes carved
out of this range (when stored in `system.span_configurations`).
By ensuring this span is carved at the end of the system range, we can
ensure that the host tenant does not install span configurations to this
span directly.

We define special key prefixes for system span configurations in this
range and establish a mapping between a Target <-> span using `Encode`
and `Decode` methods. This is useful when we store sysytem span
configurations in `system.span_configurations`  given its schema. Ditto
for reading from the table.

We'll make use of the `Target` in upcoming PRs, specifically in the
`KVAccessor`, `KVSubscriber`, and (the upcoming)
`SystemSpanConfigStore`.

The `KVAccessor` will construct `Targets` using tenant information from
the context as the targeter tenant. It'll use the
`roachpb.SystemSpanConfigTargets` supplied to it by RPCs (See cockroachdb#75615).
The encoding/decoding methods will be used when writing to/reading from
`system.span_configurations`.

While the existing `roachpb.SystemSpanConfigTarget` is sufficient
for our use in the `KVAccessor` (which runs on SQL pods), the richer
structure is motivated by the desire to also use this type in the
`KVSubscriber` (when receiving system span config events) and
`SystemSpanConfigStore` (which is an in-memory representation of the
system span config state). Down in KV we want to distinguish between a
tenant installed system span config on its entire keyspace vs. a host
tenant installed system span config over a tenants keyspace.

Release note: None
arulajmani added a commit to arulajmani/cockroach that referenced this pull request Feb 1, 2022
@arulajmani
systemspanconfig: introduce Target and encoding/decoding methods
63b4f2f 
This patch introduces a new `systemspanconfig.Target` type which is
intended as in interemediate representation for system span config
targets throughout the `spanconfig` package. It ties together a
targeter tenant ID with a targetee tenant ID (along with validation
to ensure secondary tenants don't target other tenants).

We also reserve a `SystemSpanConfigSpan` at the end of the System
range. This allows us to assign special meaning to key-prefixes carved
out of this range (when stored in `system.span_configurations`).
By ensuring this span is carved at the end of the system range, we can
ensure that the host tenant does not install span configurations to this
span directly.

We define special key prefixes for system span configurations in this
range and establish a mapping between a Target <-> span using `Encode`
and `Decode` methods. This is useful when we store sysytem span
configurations in `system.span_configurations`  given its schema. Ditto
for reading from the table.

We'll make use of the `Target` in upcoming PRs, specifically in the
`KVAccessor`, `KVSubscriber`, and (the upcoming)
`SystemSpanConfigStore`.

The `KVAccessor` will construct `Targets` using tenant information from
the context as the targeter tenant. It'll use the
`roachpb.SystemSpanConfigTargets` supplied to it by RPCs (See cockroachdb#75615).
The encoding/decoding methods will be used when writing to/reading from
`system.span_configurations`.

While the existing `roachpb.SystemSpanConfigTarget` is sufficient
for our use in the `KVAccessor` (which runs on SQL pods), the richer
structure is motivated by the desire to also use this type in the
`KVSubscriber` (when receiving system span config events) and
`SystemSpanConfigStore` (which is an in-memory representation of the
system span config state). Down in KV we want to distinguish between a
tenant installed system span config on its entire keyspace vs. a host
tenant installed system span config over a tenants keyspace.

Release note: None
@arulajmani arulajmani mentioned this pull request Feb 1, 2022
Closed
23 tasks
arulajmani added a commit to arulajmani/cockroach that referenced this pull request Feb 7, 2022
@arulajmani
systemspanconfig: introduce Target and encoding/decoding methods
28fd0e0 
This patch introduces a new `systemspanconfig.Target` type which is
intended as in interemediate representation for system span config
targets throughout the `spanconfig` package. It ties together a
targeter tenant ID with a targetee tenant ID (along with validation
to ensure secondary tenants don't target other tenants).

We also reserve a `SystemSpanConfigSpan` at the end of the System
range. This allows us to assign special meaning to key-prefixes carved
out of this range (when stored in `system.span_configurations`).
By ensuring this span is carved at the end of the system range, we can
ensure that the host tenant does not install span configurations to this
span directly.

We define special key prefixes for system span configurations in this
range and establish a mapping between a Target <-> span using `Encode`
and `Decode` methods. This is useful when we store sysytem span
configurations in `system.span_configurations`  given its schema. Ditto
for reading from the table.

We'll make use of the `Target` in upcoming PRs, specifically in the
`KVAccessor`, `KVSubscriber`, and (the upcoming)
`SystemSpanConfigStore`.

The `KVAccessor` will construct `Targets` using tenant information from
the context as the targeter tenant. It'll use the
`roachpb.SystemSpanConfigTargets` supplied to it by RPCs (See cockroachdb#75615).
The encoding/decoding methods will be used when writing to/reading from
`system.span_configurations`.

While the existing `roachpb.SystemSpanConfigTarget` is sufficient
for our use in the `KVAccessor` (which runs on SQL pods), the richer
structure is motivated by the desire to also use this type in the
`KVSubscriber` (when receiving system span config events) and
`SystemSpanConfigStore` (which is an in-memory representation of the
system span config state). Down in KV we want to distinguish between a
tenant installed system span config on its entire keyspace vs. a host
tenant installed system span config over a tenants keyspace.

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ajwerner ajwerner Awaiting requested review from ajwerner

@nvb nvb Awaiting requested review from nvb

@adityamaru adityamaru Awaiting requested review from adityamaru

1 more reviewer

@irfansharif irfansharif irfansharif approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@arulajmani @cockroach-teamcity @irfansharif