sql: disallow tenants from setting zone configs by default

[arulajmani]

Previously, sql.zone_configs.allow_for_secondary_tenant.enabled was a
regular old cluster setting that tenants could control. Now that we have
a notion of tenant read-only setting, this feels like a great candidate
for it. In addition to making this switch, this patch also changes this
setting's default value to be false. See the linked issue for the
motivation around why.

The change to switch this setting to be tenant read-only necessitates
changes in how we run logic tests. This is because logic tests that
run as secondary tenants can no longer change this cluster setting
in the test file. We introduce a new
tenant-cluster-setting-override-opt logic test directive with an
option for allow-zone-configs-for-secondary-tenants to get this
working. When configured, the host alters this cluster setting
during test setup. This can later be extended for other read-only
settings in the future.

References #77344

Release note (sql change): Changes the default value of
sql.zone_configs.allow_for_secondary_tenant.enabled to be false.
Moreover, this setting is no longer settable by secondary tenants.
Instead, it's now a tenant read-only cluster setting.

[cockroach-teamcity]

This change is 

[ajwerner]

Do you think that we should do something to, say, treat the translator to use the RANGE TENANTS value it has from the gossip connector as the zone config for all tables if we revert this? That'd be a follow-up task, please don't do that here. Just thinking through the case where we decide, whoops, actually, no more splits for you. Or, perhaps we even do something more drastic like have the translator revert the span configs to one configuration that covers the entire tenant using that configuration. That could be handy.

Other than that, neat logictest addition.

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

---

pkg/sql/logictest/logic.go, line 1746 at r1 (raw file):

			// setting override can propagate to the tenant faster.
			if _, err := conn.Exec(
				"SET CLUSTER SETTING kv.closed_timestamp.target_duration = '50ms'",

if you're going to do this, you may as well also lower kv.closed_timestamp.side_transport_interval to 50ms too

---

pkg/sql/logictest/logic.go, line 2172 at r1 (raw file):

// and parses that line into a set of tenantClusterSettingOverrideArgs that need
// to be overriden by the host cluster before the test begins.
func readTenantClusterSettingOverrideArgs(

should there be some code sharing with readClusterOptions? Maybe something that just parses out the directives or gives you a callback? I don't want to be too pushy about this, it just feels like there's a bit of an opportunity to abstract out some structure.

[arulajmani]

Do you think that we should do something to, say, treat the translator to use the RANGE TENANTS value it has from the gossip connector as the zone config for all tables if we revert this? That'd be a follow-up task, please don't do that here. Just thinking through the case where we decide, whoops, actually, no more splits for you.

I'm not sure just asking the translator to use RANGE TENANTS for all tables will help on the splits front. I think what you're asking for will also require us to address #72389. Or, in the short term, we provide this drastic knob.

Or, perhaps we even do something more drastic like have the translator revert the span configs to one configuration that covers the entire tenant using that configuration. That could be handy.

This sounds doable, but it'll affect some of the new span config fields we've added recently. Luckily, we built a fast path for tenants to write PTS records over their entire keyspace, so this will only get rid of PTS fields set on databases/tables. Maybe that's palatable given this is a "last resort escape hatch".

Another field that comes to mind is this ExcludeDataFromBackup we recently added. @adityamaru do you think there could be a hazard here if this suddenly stops being set for some tables?

Anyway, I think it's worth filing an issue about all of this. Maybe we want to consider another alternative here is to do this entirely through the system tenant. I can write something up in a bit.

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

---

pkg/sql/logictest/logic.go, line 1746 at r1 (raw file):

Previously, ajwerner wrote…

if you're going to do this, you may as well also lower kv.closed_timestamp.side_transport_interval to 50ms too

Done

---

pkg/sql/logictest/logic.go, line 2172 at r1 (raw file):

Previously, ajwerner wrote…

should there be some code sharing with readClusterOptions? Maybe something that just parses out the directives or gives you a callback? I don't want to be too pushy about this, it just feels like there's a bit of an opportunity to abstract out some structure.

I'd be lying if the thought I said the thought hadn't crossed my mind and I wasn't just being lazy. Done!

[arulajmani]

TFTR!

bors r=ajwerner

[craig]

This PR was included in a batch that was canceled, it will be automatically retried

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[arulajmani]

blathers backport release22.1

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error getting backport branch release-release22.1: GET https://api.github.com/repos/cockroachdb/cockroach/branches/release-release22.1: 404 Branch not found []

Backport to branch release22.1 failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[arulajmani]

blathers backport release-22.1