sql: validate constraints when secondary tenants set zone configs #69275

arulajmani · 2021-08-24T02:49:31Z

Previousl, we would validate constraints/voter_constraints/
lease_preferences fields only for the system tenant. This patch extends
this validation for secondary tenants as well.

Crucially, the validation of these fields differs for secondary tenants
in a couple of ways. Firstly, secondary tenants are only allowed to set
locality attributes in their constraints clause. Even then, they're
only allowed to set zone/region tiers. All other constraints
(such as disk type, or rack locality) are rejected. This is because
secondary tenants have a very narrow view of the cluster via the
RegionServer.

Secondly, we validate both positive and negative constraints for
secondary tenants (unlike the system tenant where we validate only
positive constraints). This follows from the "narrow view" secondary
tenants get -- we don't want them configuring arbitrary constraints,
positive or negative.

Closes #69199

Release justification: Non production code change
Release note: None

cockroach-teamcity · 2021-08-24T02:49:37Z

This change is

pkg/sql/set_zone_config_test.go

pkg/sql/set_zone_config.go

ajstorm

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, and @irfansharif)

pkg/sql/set_zone_config.go, line 989 at r1 (raw file):

// view exposed via the RegionServer and are not allowed to configure arbitrary
// constraints (required or otherwise).
func validateZoneLocalitiesForSecondaryTenants(

Drive by comment: Does it make sense to have this behaviour overridable via a cluster setting? While now we can't envision a reason why we'd want to allow other tiers or missing regions/zones, if something comes up, it might be helpful for support to have an escape valve.

arulajmani

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @ajwerner, and @irfansharif)

pkg/sql/set_zone_config.go, line 989 at r1 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Drive by comment: Does it make sense to have this behaviour overridable via a cluster setting? While now we can't envision a reason why we'd want to allow other tiers or missing regions/zones, if something comes up, it might be helpful for support to have an escape valve.

Are we worried at all that tenants could abuse this cluster setting to deliberately configure unsatisfiable constraints? Couldn't this have bad effects downstream?

ajstorm

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, and @irfansharif)

pkg/sql/set_zone_config.go, line 989 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Are we worried at all that tenants could abuse this cluster setting to deliberately configure unsatisfiable constraints? Couldn't this have bad effects downstream?

Is there a way in serverless to hide a cluster setting from the secondary tenant but allow it to be injected by the host tenant? I was thinking we'd want this only set by SREs in emergencies.

irfansharif · 2021-08-24T14:14:12Z

pkg/sql/set_zone_config.go

+func validateZoneAttrsAndLocalities(
+	ctx context.Context, execCfg *ExecutorConfig, zone *zonepb.ZoneConfig,
+) error {
+	// Avoid RPC calls to the Node/Region server if we don't have anything to


s/RPC calls/RPCs

irfansharif · 2021-08-24T14:16:44Z

pkg/sql/set_zone_config.go

+// information via the RegionServer. Even then, they're only allowed to
+// reference the "region" and "zone" tiers.
+//
+// Unlike the system tenant, we also catch typos in and validate prohibited


"tenant, we also validate prohibited constraints."

irfansharif · 2021-08-24T14:22:30Z

pkg/sql/set_zone_config.go

+		default:
+			return pgerror.Newf(
+				pgcode.CheckViolation,
+				"invalid constraint attribute: %q for tenants",


Mention that only "zone" and "region" are allowed.

irfansharif · 2021-08-24T14:24:04Z

pkg/sql/set_zone_config.go

+			if !found {
+				return pgerror.Newf(
+					pgcode.CheckViolation,
+					"region %q matches no existing nodes within the cluster",


Here and above, instead of mentioning "nodes" (which is not exposed to serverless users), just say that "region %q not found". Speaking of, is there a SHOW ALL REGIONS/ZONES or something tenants would be able to use to figure out what they can use?

irfansharif · 2021-08-24T14:25:18Z

pkg/sql/set_zone_config_test.go

+			errRe: "",
+		},
+		{
+			cfg:   `constraints: ["+zone=us-east1"]`,


s/us-east1/non-existent-zone
s/us-east1-a/non-existent-region
...

irfansharif · 2021-08-24T14:29:20Z

pkg/sql/set_zone_config.go

+		case "":
+			return pgerror.Newf(
+				pgcode.CheckViolation,
+				"non-locality constraints are not allowed for tenants",


Aren't these user visible errors? I don't think we want to use "tenants" (here and below). Also, what is a "non-locality constraint"? I don't see it in https://www.cockroachlabs.com/docs/v21.1/configure-replication-zones. Should we instead say something along the lines of "missing constraint attribute", and ask them to specify either region or zone?

arulajmani

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @ajwerner, and @irfansharif)

pkg/sql/set_zone_config.go, line 989 at r1 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Is there a way in serverless to hide a cluster setting from the secondary tenant but allow it to be injected by the host tenant? I was thinking we'd want this only set by SREs in emergencies.

I'm not sure how "change setting on host tenant to have effect on all secondary tenants" works (or if it even exists). Let's talk about it in todays pod meeting and if we want this escape hatch?

pkg/sql/set_zone_config.go, line 912 at r2 (raw file):

Previously, irfansharif (irfan sharif) wrote…

s/RPC calls/RPCs

Done.

pkg/sql/set_zone_config.go, line 985 at r2 (raw file):

Previously, irfansharif (irfan sharif) wrote…

"tenant, we also validate prohibited constraints."

Done.

pkg/sql/set_zone_config.go, line 1022 at r2 (raw file):

Speaking off, is there a SHOW ALL REGIONS/ZONES or something tenants would be able to use to figure out what they can use?

Yeah, tenants can use SHOW REGIONS because that thing uses this RegionServer.

pkg/sql/set_zone_config.go, line 1029 at r2 (raw file):

Also, what is a "non-locality constraint"?

Something that references a node/store attribute, such as constraints = '[+ssd]'. I don't think "missing constraint attribute" makes sense in this case. Wdyt of this new revision?

pkg/sql/set_zone_config.go, line 1034 at r2 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Mention that only "zone" and "region" are allowed.

Added a hint.

pkg/sql/set_zone_config_test.go, line 111 at r2 (raw file):

Previously, irfansharif (irfan sharif) wrote…

s/us-east1/non-existent-zone
s/us-east1-a/non-existent-region
...

Done.

irfansharif

Reviewed 1 of 4 files at r1, 1 of 3 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajstorm, @ajwerner, and @arulajmani)

-- commits, line 4 at r3:
Typo: Previously

pkg/sql/set_zone_config.go, line 989 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

I'm not sure how "change setting on host tenant to have effect on all secondary tenants" works (or if it even exists). Let's talk about it in todays pod meeting and if we want this escape hatch?

I can't think of a scenario where we'd want this very specific escape hatch. It's also easy enough to add after the fact should something come up; I'd err on the side of not introducing Yet Another Cluster Setting until then.

pkg/sql/set_zone_config.go, line 985 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Done.

We have an unnecessary "catch" in there.

pkg/sql/set_zone_config.go, line 1029 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Also, what is a "non-locality constraint"?

Something that references a node/store attribute, such as constraints = '[+ssd]'. I don't think "missing constraint attribute" makes sense in this case. Wdyt of this new revision?

I figured, I just meant it's not terminology we've used before/elsewhere. To me this error should read the same as the one below (invalid constraint attribute: ""), and missing constraint attribute: specify either "region" or "zone" seems like the next best thing.

I'm not a fan of the "for tenants" qualifier given it bubbles up to end users. Ideally tenants wouldn't know they're tenants. The right place to document this limitation would be in our docs. I'm not sure where our we generally spell out the differences between on-prem CRDB and serverless (if we do), but worth asking Rich or someone about it.

pkg/sql/set_zone_config.go, line 1038 at r3 (raw file):

				constraint.Key,
			),
				`only "zone" and "region" are allowed in constraints`,

s/in constraints//

Previously, we would validate constraints/voter_constraints/ lease_preferences fields only for the system tenant. This patch extends this validation for secondary tenants as well. Crucially, the validation of these fields differs for secondary tenants in a couple of ways. Firstly, secondary tenants are only allowed to set locality attributes in their constraints clause. Even then, they're only allowed to set zone/region tiers. All other constraints (such as disk type, or rack locality) are rejected. This is because secondary tenants have a very narrow view of the cluster via the `RegionServer`. Secondly, we validate both positive and negative constraints for secondary tenants (unlike the system tenant where we validate only positive constraints). This follows from the "narrow view" secondary tenants get -- we don't want them configuring arbitrary constraints, positive or negative. Closes cockroachdb#69199 Release justification: Non production code change Release note: None

arulajmani

Will bors once CI is green.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajstorm, @ajwerner, and @irfansharif)

-- commits, line 4 at r3:

Previously, irfansharif (irfan sharif) wrote…

Typo: Previously

Done.

pkg/sql/set_zone_config.go, line 985 at r2 (raw file):

Previously, irfansharif (irfan sharif) wrote…

We have an unnecessary "catch" in there.

Oops.

pkg/sql/set_zone_config.go, line 1029 at r2 (raw file):

Previously, irfansharif (irfan sharif) wrote…

I figured, I just meant it's not terminology we've used before/elsewhere. To me this error should read the same as the one below (invalid constraint attribute: ""), and missing constraint attribute: specify either "region" or "zone" seems like the next best thing.

I'm not a fan of the "for tenants" qualifier given it bubbles up to end users. Ideally tenants wouldn't know they're tenants. The right place to document this limitation would be in our docs. I'm not sure where our we generally spell out the differences between on-prem CRDB and serverless (if we do), but worth asking Rich or someone about it.

Meant to remove the "for tenants" part, my bad. I'm fine not differentiating between these two cases.

pkg/sql/set_zone_config.go, line 1038 at r3 (raw file):

Previously, irfansharif (irfan sharif) wrote…

s/in constraints//

Done.

arulajmani · 2021-08-24T19:34:12Z

TFTRs!

borsr=otan,irfansharif

arulajmani · 2021-08-25T13:51:52Z

I can't type.

bors r=otan,irfansharif

craig · 2021-08-25T14:46:44Z

Build succeeded:

GitHub CI (Cockroach)

arulajmani requested review from ajwerner, otan, irfansharif and a team August 24, 2021 02:49

otan approved these changes Aug 24, 2021

View reviewed changes

pkg/sql/set_zone_config_test.go Outdated Show resolved Hide resolved

pkg/sql/set_zone_config.go Outdated Show resolved Hide resolved

ajstorm requested changes Aug 24, 2021

View reviewed changes

arulajmani force-pushed the zcfg-validate-localities branch from bee6938 to 6dd101f Compare August 24, 2021 13:46

arulajmani commented Aug 24, 2021

View reviewed changes

ajstorm requested changes Aug 24, 2021

View reviewed changes

irfansharif reviewed Aug 24, 2021

View reviewed changes

irfansharif mentioned this pull request Aug 24, 2021

*: rework zone configurations to support tenants #67679

Closed

21 tasks

arulajmani force-pushed the zcfg-validate-localities branch from 6dd101f to 989477a Compare August 24, 2021 17:57

blathers-crl bot requested a review from irfansharif August 24, 2021 17:57

arulajmani commented Aug 24, 2021

View reviewed changes

irfansharif approved these changes Aug 24, 2021

View reviewed changes

arulajmani force-pushed the zcfg-validate-localities branch from 989477a to 66336a3 Compare August 24, 2021 18:29

arulajmani commented Aug 24, 2021

View reviewed changes

craig bot merged commit cf02add into cockroachdb:master Aug 25, 2021

irfansharif mentioned this pull request Aug 30, 2021

sql: enable locality validation when setting zone configs for secondary tenants #67678

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: validate constraints when secondary tenants set zone configs #69275

sql: validate constraints when secondary tenants set zone configs #69275

arulajmani commented Aug 24, 2021

cockroach-teamcity commented Aug 24, 2021

ajstorm left a comment

arulajmani left a comment

ajstorm left a comment

irfansharif Aug 24, 2021

irfansharif Aug 24, 2021

irfansharif Aug 24, 2021

irfansharif Aug 24, 2021 •

edited

Loading

irfansharif Aug 24, 2021

irfansharif Aug 24, 2021

arulajmani left a comment

irfansharif left a comment

arulajmani left a comment

arulajmani commented Aug 24, 2021

arulajmani commented Aug 25, 2021

craig bot commented Aug 25, 2021

sql: validate constraints when secondary tenants set zone configs #69275

sql: validate constraints when secondary tenants set zone configs #69275

Conversation

arulajmani commented Aug 24, 2021

cockroach-teamcity commented Aug 24, 2021

ajstorm left a comment

Choose a reason for hiding this comment

arulajmani left a comment

Choose a reason for hiding this comment

ajstorm left a comment

Choose a reason for hiding this comment

irfansharif Aug 24, 2021

Choose a reason for hiding this comment

irfansharif Aug 24, 2021

Choose a reason for hiding this comment

irfansharif Aug 24, 2021

Choose a reason for hiding this comment

irfansharif Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

irfansharif Aug 24, 2021

Choose a reason for hiding this comment

irfansharif Aug 24, 2021

Choose a reason for hiding this comment

arulajmani left a comment

Choose a reason for hiding this comment

irfansharif left a comment

Choose a reason for hiding this comment

arulajmani left a comment

Choose a reason for hiding this comment

arulajmani commented Aug 24, 2021

arulajmani commented Aug 25, 2021

craig bot commented Aug 25, 2021

irfansharif Aug 24, 2021 •

edited

Loading