From 7498055bf590f5cb083399c3352262e32de6569c Mon Sep 17 00:00:00 2001
From: Faizan Qazi <faizan@cockroachlabs.com>
Date: Thu, 14 Nov 2024 09:30:19 -0500
Subject: [PATCH] sql: validate zone config before multi region database DDL

Previously, schema changes for databases involving zone configuration
modifications could hang. This occurred because the system didn't
validate the existing zone configuration's validity before initiating
these operations. Since users can manually modify zone configurations,
they might inadvertently introduce invalid states. To address this, this
patch validates the zone configuration before the following database
operations: setting the primary region, setting the secondary region,
adding a region, and dropping a region.

Fixes: #131342

Release note (bug fix): Prevent ALTER DATABASE operations that modify
the zone config from hanging if an invalid zone config already exists.
---
 .../logic_test/multi_region_zone_configs      | 47 +++++++++++++++++++
 pkg/sql/alter_database.go                     | 28 +++++++++++
 2 files changed, 75 insertions(+)

diff --git a/pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs b/pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs
index efb2c5959f76..4caf1f86f31b 100644
--- a/pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs
+++ b/pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs
@@ -2102,3 +2102,50 @@ statement ok
 DROP TABLE db1.t1_with_index_zone CASCADE;
 
 subtest end
+
+
+subtest invalid_zone_config
+
+statement ok
+CREATE DATABASE db_invalid_zone_config PRIMARY REGION "ca-central-1" REGIONS "ap-southeast-2"
+
+let $database_id
+select id from system.namespace WHERE name='db_invalid_zone_config'
+
+statement ok
+UPSERT
+INTO
+	system.zones (id, config)
+VALUES
+	(
+		$database_id,
+		(
+			SELECT
+				crdb_internal.json_to_pb(
+					'cockroach.config.zonepb.ZoneConfig',
+					jsonb_set(
+						crdb_internal.pb_to_json(
+							'cockroach.config.zonepb.ZoneConfig',
+							config
+						),
+						ARRAY['rangeMinBytes'],
+						'-1'::JSONB
+					)
+				)
+			FROM
+				system.zones
+			WHERE
+				id = $database_id
+		)
+	);
+
+statement error RangeMinBytes -1 less than minimum allowed 0
+ALTER DATABASE db_invalid_zone_config ADD REGION 'us-east-1'
+
+statement error RangeMinBytes -1 less than minimum allowed 0
+ALTER DATABASE db_invalid_zone_config DROP REGION 'ap-southeast-2'
+
+statement error RangeMinBytes -1 less than minimum allowed 0
+ALTER DATABASE db_invalid_zone_config SET SECONDARY REGION 'us-east-1'
+
+subtest end
diff --git a/pkg/sql/alter_database.go b/pkg/sql/alter_database.go
index 75485a9b77f7..15b06393d810 100644
--- a/pkg/sql/alter_database.go
+++ b/pkg/sql/alter_database.go
@@ -198,6 +198,16 @@ var GetMultiRegionEnumAddValuePlacementCCL = func(
 	)
 }
 
+// validateExistingZoneCfg validates existing zone configs for a given descriptor
+// ID.
+func (p *planner) validateExistingZoneCfg(ctx context.Context, id descpb.ID) error {
+	zc, err := p.Descriptors().GetZoneConfig(ctx, p.txn, id)
+	if err != nil || zc == nil {
+		return err
+	}
+	return zc.ZoneConfigProto().Validate()
+}
+
 func (n *alterDatabaseAddRegionNode) startExec(params runParams) error {
 	if err := params.p.validateZoneConfigForMultiRegionDatabaseWasNotModifiedByUser(
 		params.ctx,
@@ -216,6 +226,10 @@ func (n *alterDatabaseAddRegionNode) startExec(params runParams) error {
 		return err
 	}
 
+	if err := params.p.validateExistingZoneCfg(params.ctx, n.desc.ID); err != nil {
+		return err
+	}
+
 	// Get the type descriptor for the multi-region enum.
 	typeDesc, err := params.p.Descriptors().MutableByID(params.p.txn).Type(params.ctx, n.desc.RegionConfig.RegionEnumID)
 	if err != nil {
@@ -721,6 +735,11 @@ func (n *alterDatabaseDropRegionNode) startExec(params runParams) error {
 	if n.n == nil {
 		return nil
 	}
+
+	if err := params.p.validateExistingZoneCfg(params.ctx, n.desc.ID); err != nil {
+		return err
+	}
+
 	typeDesc, err := params.p.Descriptors().MutableByID(params.p.txn).Type(params.ctx, n.desc.RegionConfig.RegionEnumID)
 	if err != nil {
 		return err
@@ -1132,6 +1151,11 @@ func (n *alterDatabasePrimaryRegionNode) setInitialPrimaryRegion(params runParam
 
 func (n *alterDatabasePrimaryRegionNode) startExec(params runParams) error {
 
+	// Validate if the existing zone config is good.
+	if err := params.p.validateExistingZoneCfg(params.ctx, n.desc.ID); err != nil {
+		return err
+	}
+
 	// There are two paths to consider here: either this is the first setting of
 	// the primary region, OR we're updating the primary region. In the case where
 	// this is the first setting of the primary region, the call will turn the
@@ -1893,6 +1917,10 @@ func (n *alterDatabaseSecondaryRegion) startExec(params runParams) error {
 		)
 	}
 
+	if err := params.p.validateExistingZoneCfg(params.ctx, n.desc.ID); err != nil {
+		return err
+	}
+
 	// Verify that the secondary region is part of the region list.
 	prevRegionConfig, err := SynthesizeRegionConfig(params.ctx, params.p.txn, n.desc.ID, params.p.Descriptors())
 	if err != nil {