sql: add CHECK EXTERNAL CONNECTION command

This patch adds the `CHECK EXTERNAL CONNECTION` command and replaces the old `SHOW BACKUP CONNECTION` syntax. Epic: None Release note: None
cockroachdb · Jan 13, 2025 · a5d986e · a5d986e
1 parent ae2c24b
commit a5d986e
Show file tree

Hide file tree

Showing 36 changed files with 522 additions and 431 deletions.
diff --git a/docs/generated/sql/bnf/BUILD.bazel b/docs/generated/sql/bnf/BUILD.bazel
@@ -117,6 +117,8 @@ FILES = [
     "create_trigger",
     "create_type",
     "create_view_stmt",
+    "check_stmt",
+    "check_external_connection_stmt",
     "deallocate_stmt",
     "declare_cursor_stmt",
     "default_value_column_level",

diff --git a/docs/generated/sql/bnf/check_external_connection_stmt.bnf b/docs/generated/sql/bnf/check_external_connection_stmt.bnf
@@ -0,0 +1,2 @@
+check_external_connection_stmt ::=
+	'CHECK' 'EXTERNAL' 'CONNECTION' string_or_placeholder opt_with_check_external_connection_options_list
diff --git a/docs/generated/sql/bnf/check_stmt.bnf b/docs/generated/sql/bnf/check_stmt.bnf
@@ -0,0 +1,3 @@
+check_stmt ::=
+	check_external_connection_stmt
+	| 'CHECK' 'ERROR'
diff --git a/docs/generated/sql/bnf/preparable_stmt.bnf b/docs/generated/sql/bnf/preparable_stmt.bnf
@@ -3,6 +3,7 @@ preparable_stmt ::=
 	| backup_stmt
 	| cancel_stmt
 	| create_stmt
+	| check_stmt
 	| delete_stmt
 	| drop_stmt
 	| explain_stmt

diff --git a/docs/generated/sql/bnf/stmt_block.bnf b/docs/generated/sql/bnf/stmt_block.bnf
@@ -42,6 +42,7 @@ preparable_stmt ::=
 	| backup_stmt
 	| cancel_stmt
 	| create_stmt
+	| check_stmt
 	| delete_stmt
 	| drop_stmt
 	| explain_stmt
@@ -208,6 +209,10 @@ create_stmt ::=
 	| create_logical_replication_stream_stmt
 	| create_schedule_stmt
 
+check_stmt ::=
+	check_external_connection_stmt
+	| 'CHECK' 'ERROR'
+
 delete_stmt ::=
 	opt_with_clause 'DELETE' opt_batch_clause 'FROM' table_expr_opt_alias_idx opt_using_clause opt_where_clause opt_sort_clause opt_limit_clause returning_clause
 
@@ -628,6 +633,9 @@ create_schedule_stmt ::=
 	create_schedule_for_changefeed_stmt
 	| create_schedule_for_backup_stmt
 
+check_external_connection_stmt ::=
+	'CHECK' 'EXTERNAL' 'CONNECTION' string_or_placeholder opt_with_check_external_connection_options_list
+
 opt_with_clause ::=
 	with_clause
 	| 
@@ -1895,6 +1903,11 @@ create_schedule_for_changefeed_stmt ::=
 create_schedule_for_backup_stmt ::=
 	'CREATE' 'SCHEDULE' schedule_label_spec 'FOR' 'BACKUP' opt_backup_targets 'INTO' string_or_placeholder_opt_list opt_with_backup_options cron_expr opt_full_backup_clause opt_with_schedule_options
 
+opt_with_check_external_connection_options_list ::=
+	'WITH' check_external_connection_options_list
+	| 'WITH' 'OPTIONS' '(' check_external_connection_options_list ')'
+	| 
+
 with_clause ::=
 	'WITH' cte_list
 	| 'WITH' 'RECURSIVE' cte_list
@@ -2714,6 +2727,9 @@ opt_full_backup_clause ::=
 	| 'FULL' 'BACKUP' 'ALWAYS'
 	| 
 
+check_external_connection_options_list ::=
+	( check_external_connection_options ) ( ( ',' check_external_connection_options ) )*
+
 cte_list ::=
 	( common_table_expr ) ( ( ',' common_table_expr ) )*
 
@@ -3349,6 +3365,11 @@ logical_replication_create_table_options ::=
 	| 'DISCARD' '=' string_or_placeholder
 	| 'LABEL' '=' string_or_placeholder
 
+check_external_connection_options ::=
+	'TRANSFER' '=' string_or_placeholder
+	| 'TIME' '=' string_or_placeholder
+	| 'CONCURRENTLY' '=' a_expr
+
 common_table_expr ::=
 	table_alias_name opt_col_def_list_no_types 'AS' materialize_clause '(' preparable_stmt ')'
 

diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel
@@ -1036,10 +1036,7 @@ GO_TARGETS = [
     "//pkg/cloud/amazon:amazon_test",
     "//pkg/cloud/azure:azure",
     "//pkg/cloud/azure:azure_test",
-    "//pkg/cloud/cloudcheck:cloudcheck",
     "//pkg/cloud/cloudpb:cloudpb",
-    "//pkg/cloud/cloudprivilege:cloudprivilege",
-    "//pkg/cloud/cloudprivilege:privileges",
     "//pkg/cloud/cloudtestutils:cloudtestutils",
     "//pkg/cloud/externalconn/connectionpb:connectionpb",
     "//pkg/cloud/externalconn/providers:providers",

diff --git a/pkg/backup/BUILD.bazel b/pkg/backup/BUILD.bazel
@@ -51,9 +51,7 @@ go_library(
         "//pkg/ccl/multiregionccl",
         "//pkg/ccl/storageccl",
         "//pkg/cloud",
-        "//pkg/cloud/cloudcheck",
         "//pkg/cloud/cloudpb",
-        "//pkg/cloud/cloudprivilege",
         "//pkg/clusterversion",
         "//pkg/featureflag",
         "//pkg/jobs",

diff --git a/pkg/backup/backup_planning.go b/pkg/backup/backup_planning.go
@@ -14,7 +14,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/backup/backupbase"
 	"github.com/cockroachdb/cockroach/pkg/backup/backupresolver"
 	"github.com/cockroachdb/cockroach/pkg/cloud"
-	"github.com/cockroachdb/cockroach/pkg/cloud/cloudprivilege"
 	"github.com/cockroachdb/cockroach/pkg/featureflag"
 	"github.com/cockroachdb/cockroach/pkg/jobs"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
@@ -271,7 +270,7 @@ func checkPrivilegesForBackup(
 					pgcode.InsufficientPrivilege,
 					"only users with the admin role or the BACKUP system privilege are allowed to perform full cluster backups")
 			}
-			return cloudprivilege.CheckDestinationPrivileges(ctx, p, to)
+			return sql.CheckDestinationPrivileges(ctx, p, to)
 		}
 	}
 
@@ -328,7 +327,7 @@ func checkPrivilegesForBackup(
 	// If a user has the BACKUP privilege on the target databases or tables we can
 	// move on to checking the destination URIs.
 	if hasRequiredBackupPrivileges {
-		return cloudprivilege.CheckDestinationPrivileges(ctx, p, to)
+		return sql.CheckDestinationPrivileges(ctx, p, to)
 	}
 
 	// The following checks are to maintain compatability with pre-22.2 privilege
@@ -362,7 +361,7 @@ func checkPrivilegesForBackup(
 		}
 	}
 
-	return cloudprivilege.CheckDestinationPrivileges(ctx, p, to)
+	return sql.CheckDestinationPrivileges(ctx, p, to)
 }
 
 func backupTypeCheck(

diff --git a/pkg/backup/restore_planning.go b/pkg/backup/restore_planning.go
@@ -21,7 +21,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/ccl/multiregionccl"
 	"github.com/cockroachdb/cockroach/pkg/ccl/storageccl"
 	"github.com/cockroachdb/cockroach/pkg/cloud"
-	"github.com/cockroachdb/cockroach/pkg/cloud/cloudprivilege"
 	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/featureflag"
 	"github.com/cockroachdb/cockroach/pkg/jobs"
@@ -1436,7 +1435,7 @@ func restorePlanHook(
 func checkRestoreDestinationPrivileges(
 	ctx context.Context, p sql.PlanHookState, from []string,
 ) error {
-	return cloudprivilege.CheckDestinationPrivileges(ctx, p, from)
+	return sql.CheckDestinationPrivileges(ctx, p, from)
 }
 
 // checkRestorePrivilegesOnDatabase check that the user has adequate privileges

diff --git a/pkg/backup/show.go b/pkg/backup/show.go
@@ -21,8 +21,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/backup/backuputils"
 	"github.com/cockroachdb/cockroach/pkg/ccl/storageccl"
 	"github.com/cockroachdb/cockroach/pkg/cloud"
-	"github.com/cockroachdb/cockroach/pkg/cloud/cloudcheck"
-	"github.com/cockroachdb/cockroach/pkg/cloud/cloudprivilege"
 	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
 	"github.com/cockroachdb/cockroach/pkg/keys"
@@ -45,7 +43,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
-	"github.com/cockroachdb/cockroach/pkg/util/humanizeutil"
 	"github.com/cockroachdb/cockroach/pkg/util/json"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/mon"
@@ -208,9 +205,6 @@ func showBackupTypeCheck(
 	); err != nil {
 		return false, nil, err
 	}
-	if backup.Details == tree.BackupConnectionTest {
-		return true, cloudcheck.Header, nil
-	}
 	infoReader := getBackupInfoReader(p, backup)
 	return true, infoReader.header(), nil
 }
@@ -225,45 +219,6 @@ func showBackupPlanHook(
 	}
 	exprEval := p.ExprEvaluator("SHOW BACKUP")
 
-	// TODO(dt): find move this to its own hook.
-	if showStmt.Details == tree.BackupConnectionTest {
-		loc, err := exprEval.String(ctx, showStmt.Path)
-		if err != nil {
-			return nil, nil, false, err
-		}
-		var params cloudcheck.Params
-		if showStmt.Options.CheckConnectionTransferSize != nil {
-			transferSizeStr, err := exprEval.String(ctx, showStmt.Options.CheckConnectionTransferSize)
-			if err != nil {
-				return nil, nil, false, err
-			}
-			parsed, err := humanizeutil.ParseBytes(transferSizeStr)
-			if err != nil {
-				return nil, nil, false, err
-			}
-			params.TransferSize = parsed
-		}
-		if showStmt.Options.CheckConnectionDuration != nil {
-			durationStr, err := exprEval.String(ctx, showStmt.Options.CheckConnectionDuration)
-			if err != nil {
-				return nil, nil, false, err
-			}
-			parsed, err := time.ParseDuration(durationStr)
-			if err != nil {
-				return nil, nil, false, err
-			}
-			params.MinDuration = parsed
-		}
-		if showStmt.Options.CheckConnectionConcurrency != nil {
-			concurrency, err := exprEval.Int(ctx, showStmt.Options.CheckConnectionConcurrency)
-			if err != nil {
-				return nil, nil, false, err
-			}
-			params.Concurrency = concurrency
-		}
-		return cloudcheck.ShowCloudStorageTestPlanHook(ctx, p, loc, params)
-	}
-
 	if showStmt.Path == nil && showStmt.InCollection != nil {
 		collection, err := exprEval.StringArray(
 			ctx, tree.Exprs(showStmt.InCollection),
@@ -319,7 +274,7 @@ func showBackupPlanHook(
 					"https://www.cockroachlabs.com/docs/stable/show-backup.html"))
 		}
 
-		if err := cloudprivilege.CheckDestinationPrivileges(ctx, p, dest); err != nil {
+		if err := sql.CheckDestinationPrivileges(ctx, p, dest); err != nil {
 			return err
 		}
 
@@ -1517,7 +1472,7 @@ func showBackupsInCollectionPlanHook(
 	ctx context.Context, collection []string, showStmt *tree.ShowBackup, p sql.PlanHookState,
 ) (sql.PlanHookRowFn, colinfo.ResultColumns, bool, error) {
 
-	if err := cloudprivilege.CheckDestinationPrivileges(ctx, p, collection); err != nil {
+	if err := sql.CheckDestinationPrivileges(ctx, p, collection); err != nil {
 		return nil, nil, false, err
 	}
 

diff --git a/pkg/backup/testdata/backup-restore/show_backup b/pkg/backup/testdata/backup-restore/show_backup
@@ -63,17 +63,3 @@ query-sql regex=No\sproblems\sfound!
 SELECT * FROM [SHOW BACKUP VALIDATE FROM 'valid-22.2-with-job' IN 'nodelocal://1/'];
 ----
 true
-
-query-sql
-SELECT node, locality, ok, error, can_delete FROM [SHOW BACKUP CONNECTION 'nodelocal://1/conn-test' WITH TRANSFER='1'] ORDER BY node;
-----
-1 region=eu-central-1,availability-zone=eu-central-1 true  true
-2 region=eu-north-1,availability-zone=eu-north-1 true  true
-3 region=us-east-1,availability-zone=us-east1 true  true
-
-query-sql
-SELECT node, locality, ok, error, can_delete FROM [SHOW BACKUP CONNECTION 'nodelocal://1/conn-test' WITH TRANSFER='1', TIME = '1ms'] ORDER BY node;
-----
-1 region=eu-central-1,availability-zone=eu-central-1 true  true
-2 region=eu-north-1,availability-zone=eu-north-1 true  true
-3 region=us-east-1,availability-zone=us-east1 true  true
diff --git a/pkg/cloud/cloudcheck/BUILD.bazel b/pkg/cloud/cloudcheck/BUILD.bazel
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		check_external_connection_stmt ::=
		'CHECK' 'EXTERNAL' 'CONNECTION' string_or_placeholder opt_with_check_external_connection_options_list