Merge cockroachdb#85957 cockroachdb#85984

85957: externalconn: encrypt,decrpyt when creating KMS external connection r=benbardin a=adityamaru

This patch does a couple of things:

`CREATE EXTERNAL CONNECTION` for gcp KMS now recognizes the `gcp`
scheme instead of the `gs` scheme. This is motivated by a future change
where we will add support for google ExternalStorage which is also
represented by the `gs` scheme today. Since we cannot have a collision
of schemes and this is new functionality we are okay asking users to
use `gcp` when referring to a KMS URI, and `gs` when referring to a
storage URI. For uniformity, the existing raw URI google KMS will also
start recognizing `gcp` in addition to the existing `gs` scheme.

This patch also adds a step in the creation of a KMS external connection
that encrypts and decrypts some sentinel content to validate that the resource
is reachable and correctly configured.

Release note (sql change): Google Cloud KMS will now accept the `gcp`
scheme alongwith the existing `gs` scheme. External Connections will only
recognize the `gcp` scheme when being created to represent a KMS resource.

85984: kvserver/batcheval: add `IdempotentTombstone` for `DeleteRange` r=ajwerner,nicktrav a=erikgrinaker

This patch adds the parameter `IdempotentTombstone` for `DeleteRange`.
When enabled, a `DeleteRange` request with `UseRangeTombstone` will
first look for any point keys/tombstones in the span that aren't already
covered by an MVCC range tombstone, and omit writing the tombstone if
none are found. As a convenience, it considers empty spans equivalent to
being covered by an MVCC range tombstone, so it will omit the write
across an entirely empty span too.

Resolves cockroachdb#85725.

Release note: None

Co-authored-by: Aditya Maru <[email protected]>
Co-authored-by: Erik Grinaker <[email protected]>

pkg/ccl/cloudccl/externalconn/datadriven_test.go

@@ -38,10 +38,14 @@ func TestDataDriven(t *testing.T) {
 		defer dirCleanupFn()
 
 		var skipCheckExternalStorageConnection bool
+		var skipCheckKMSConnection bool
 		ecTestingKnobs := &externalconn.TestingKnobs{
 			SkipCheckingExternalStorageConnection: func() bool {
 				return skipCheckExternalStorageConnection
 			},
+			SkipCheckingKMSConnection: func() bool {
+				return skipCheckKMSConnection
+			},
 		}
 		tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{
 			ServerArgs: base.TestServerArgs{
@@ -82,6 +86,12 @@ func TestDataDriven(t *testing.T) {
 			case "disable-check-external-storage":
 				skipCheckExternalStorageConnection = true
 
+			case "enable-check-kms":
+				skipCheckKMSConnection = false
+
+			case "disable-check-kms":
+				skipCheckKMSConnection = true
+
 			case "exec-sql":
 				if d.HasArg("user") {
 					var user string

pkg/ccl/cloudccl/externalconn/testdata/create_drop_external_connection

@@ -140,29 +140,29 @@ DROP EXTERNAL CONNECTION 'privileged-dup'
 
 subtest end
 
-subtest basic-gs-kms
+subtest basic-gcp-kms
 
-disable-check-external-storage
+disable-check-kms
 ----
 
 exec-sql
-CREATE EXTERNAL CONNECTION "foo-kms" AS 'gs:///cmk?AUTH=implicit&CREDENTIALS=baz&ASSUME_ROLE=ronaldo,rashford,bruno&BEARER_TOKEN=foo';
+CREATE EXTERNAL CONNECTION "foo-kms" AS 'gcp-kms:///cmk?AUTH=implicit&CREDENTIALS=baz&ASSUME_ROLE=ronaldo,rashford,bruno&BEARER_TOKEN=foo';
 ----
 
 # Reject invalid KMS URIs.
 exec-sql
-CREATE EXTERNAL CONNECTION "missing-cmk-kms" AS 'gs:///?AUTH=implicit&CREDENTIALS=baz&ASSUME_ROLE=ronaldo,rashford,bruno&BEARER_TOKEN=foo';
+CREATE EXTERNAL CONNECTION "missing-cmk-kms" AS 'gcp-kms:///?AUTH=implicit&CREDENTIALS=baz&ASSUME_ROLE=ronaldo,rashford,bruno&BEARER_TOKEN=foo';
 ----
-pq: failed to construct External Connection details: host component of the GCS KMS cannot be empty; must contain the Customer Managed Key
+pq: failed to construct External Connection details: failed to create GCP KMS external connection: host component of the KMS cannot be empty; must contain the Customer Managed Key
 
 exec-sql
-CREATE EXTERNAL CONNECTION "invalid-params-kms" AS 'gs:///cmk?AUTH=implicit&INVALIDPARAM=baz';
+CREATE EXTERNAL CONNECTION "invalid-params-kms" AS 'gcp-kms:///cmk?AUTH=implicit&INVALIDPARAM=baz';
 ----
-pq: failed to construct External Connection details: unknown query parameters: INVALIDPARAM for gs URI
+pq: failed to construct External Connection details: failed to create GCP KMS external connection: unknown KMS query parameters: INVALIDPARAM
 
 inspect-system-table
 ----
-foo-kms KMS {"provider": "gs_kms", "simpleUri": {"uri": "gs:///cmk?AUTH=implicit&CREDENTIALS=baz&ASSUME_ROLE=ronaldo,rashford,bruno&BEARER_TOKEN=foo"}}
+foo-kms KMS {"provider": "gcp_kms", "simpleUri": {"uri": "gcp-kms:///cmk?AUTH=implicit&CREDENTIALS=baz&ASSUME_ROLE=ronaldo,rashford,bruno&BEARER_TOKEN=foo"}}
 
 exec-sql
 DROP EXTERNAL CONNECTION "foo-kms";
@@ -171,7 +171,7 @@ DROP EXTERNAL CONNECTION "foo-kms";
 inspect-system-table
 ----
 
-enable-check-external-storage
+enable-check-kms
 ----
 
 subtest end

pkg/ccl/cloudccl/externalconn/testdata/multi-tenant/create_drop_external_connection

@@ -145,16 +145,16 @@ subtest end
 
 subtest basic-gs-kms
 
-disable-check-external-storage
+disable-check-kms
 ----
 
 exec-sql
-CREATE EXTERNAL CONNECTION "foo-kms" AS 'gs:///cmk?AUTH=implicit&CREDENTIALS=baz&ASSUME_ROLE=ronaldo,rashford,bruno&BEARER_TOKEN=foo';
+CREATE EXTERNAL CONNECTION "foo-kms" AS 'gcp-kms:///cmk?AUTH=implicit&CREDENTIALS=baz&ASSUME_ROLE=ronaldo,rashford,bruno&BEARER_TOKEN=foo';
 ----
 
 inspect-system-table
 ----
-foo-kms KMS {"provider": "gs_kms", "simpleUri": {"uri": "gs:///cmk?AUTH=implicit&CREDENTIALS=baz&ASSUME_ROLE=ronaldo,rashford,bruno&BEARER_TOKEN=foo"}}
+foo-kms KMS {"provider": "gcp_kms", "simpleUri": {"uri": "gcp-kms:///cmk?AUTH=implicit&CREDENTIALS=baz&ASSUME_ROLE=ronaldo,rashford,bruno&BEARER_TOKEN=foo"}}
 
 exec-sql
 DROP EXTERNAL CONNECTION "foo-kms";
@@ -163,10 +163,16 @@ DROP EXTERNAL CONNECTION "foo-kms";
 inspect-system-table
 ----
 
+enable-check-kms
+----
+
 subtest end
 
 subtest basic-s3
 
+disable-check-external-storage
+----
+
 exec-sql
 CREATE EXTERNAL CONNECTION "foo-s3" AS 's3://foo/bar?AUTH=implicit&AWS_ACCESS_KEY_ID=123&AWS_SECRET_ACCESS_KEY=456&ASSUME_ROLE=ronaldo,rashford,bruno';
 ----
@@ -193,6 +199,9 @@ DROP EXTERNAL CONNECTION "foo-s3";
 inspect-system-table
 ----
 
+enable-check-external-storage
+----
+
 subtest end
 
 subtest basic-kafka-sink

pkg/ccl/cloudccl/gcp/BUILD.bazel

@@ -4,7 +4,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_test")
 go_test(
     name = "gcp_test",
     srcs = [
-        "gcs_kms_connection_test.go",
+        "gcp_kms_connection_test.go",
         "main_test.go",
     ],
     deps = [

pkg/ccl/cloudccl/gcp/gcs_kms_connection_test.go → pkg/ccl/cloudccl/gcp/gcp_kms_connection_test.go

@@ -93,7 +93,7 @@ func TestGCSKMSExternalConnection(t *testing.T) {
 		params := make(url.Values)
 		params.Add(cloud.AuthParam, cloud.AuthParamImplicit)
 
-		kmsURI := fmt.Sprintf("gs:///%s?%s", keyID, params.Encode())
+		kmsURI := fmt.Sprintf("gcp-kms:///%s?%s", keyID, params.Encode())
 		createExternalConnection("auth-implicit-kms", kmsURI)
 		backupAndRestoreFromExternalConnection(backupExternalConnectionName, "auth-implicit-kms")
 	})
@@ -111,7 +111,7 @@ func TestGCSKMSExternalConnection(t *testing.T) {
 		// Set AUTH to specified.
 		q.Set(cloud.AuthParam, cloud.AuthParamSpecified)
 
-		kmsURI := fmt.Sprintf("gs:///%s?%s", keyID, q.Encode())
+		kmsURI := fmt.Sprintf("gcp-kms:///%s?%s", keyID, q.Encode())
 		createExternalConnection("auth-specified-kms", kmsURI)
 		backupAndRestoreFromExternalConnection(backupExternalConnectionName, "auth-specified-kms")
 	})
@@ -136,7 +136,7 @@ func TestGCSKMSExternalConnection(t *testing.T) {
 		// Set AUTH to specified.
 		q.Set(cloud.AuthParam, cloud.AuthParamSpecified)
 
-		kmsURI := fmt.Sprintf("gs:///%s?%s", keyID, q.Encode())
+		kmsURI := fmt.Sprintf("gcp-kms:///%s?%s", keyID, q.Encode())
 		createExternalConnection("auth-specified-bearer-token", kmsURI)
 		backupAndRestoreFromExternalConnection(backupExternalConnectionName,
 			"auth-specified-bearer-token")
@@ -178,6 +178,10 @@ func TestGCSExternalConnectionAssumeRole(t *testing.T) {
 	createExternalConnection := func(externalConnectionName, uri string) {
 		sqlDB.Exec(t, fmt.Sprintf(`CREATE EXTERNAL CONNECTION '%s' AS '%s'`, externalConnectionName, uri))
 	}
+	disallowedCreateExternalConnection := func(externalConnectionName, uri string) {
+		sqlDB.ExpectErr(t, "(PermissionDenied|AccessDenied|PERMISSION_DENIED)",
+			fmt.Sprintf(`CREATE EXTERNAL CONNECTION '%s' AS '%s'`, externalConnectionName, uri))
+	}
 	backupAndRestoreFromExternalConnection := func(backupExternalConnectionName, kmsExternalConnectionName string) {
 		backupURI := fmt.Sprintf("external://%s", backupExternalConnectionName)
 		kmsURI := fmt.Sprintf("external://%s", kmsExternalConnectionName)
@@ -187,12 +191,6 @@ func TestGCSExternalConnectionAssumeRole(t *testing.T) {
 		sqlDB.CheckQueryResults(t, `SELECT * FROM crdb_internal.invalid_objects`, [][]string{})
 		sqlDB.Exec(t, `DROP DATABASE bar CASCADE`)
 	}
-	disallowedBackupToExternalConnection := func(backupExternalConnectionName, kmsExternalConnectionName string) {
-		backupURI := fmt.Sprintf("external://%s", backupExternalConnectionName)
-		kmsURI := fmt.Sprintf("external://%s", kmsExternalConnectionName)
-		sqlDB.ExpectErr(t, "(PermissionDenied|AccessDenied|PERMISSION_DENIED)",
-			fmt.Sprintf(`BACKUP INTO '%s' WITH kms='%s'`, backupURI, kmsURI))
-	}
 
 	envVars := []string{
 		"GOOGLE_CREDENTIALS_JSON",
@@ -217,31 +215,29 @@ func TestGCSExternalConnectionAssumeRole(t *testing.T) {
 	createExternalConnection(backupExternalConnectionName, backupURI)
 
 	t.Run("auth-assume-role-implicit", func(t *testing.T) {
-		disallowedKMSURI := fmt.Sprintf("gs:///%s?%s=%s", keyID, cloud.AuthParam, cloud.AuthParamImplicit)
+		disallowedKMSURI := fmt.Sprintf("gcp-kms:///%s?%s=%s", keyID, cloud.AuthParam, cloud.AuthParamImplicit)
 		disallowedECName := "auth-assume-role-implicit-disallowed"
-		createExternalConnection(disallowedECName, disallowedKMSURI)
-		disallowedBackupToExternalConnection(backupExternalConnectionName, disallowedECName)
+		disallowedCreateExternalConnection(disallowedECName, disallowedKMSURI)
 
 		q := make(url.Values)
 		q.Set(cloud.AuthParam, cloud.AuthParamImplicit)
 		q.Set(gcp.AssumeRoleParam, assumedAccount)
-		uri := fmt.Sprintf("gs:///%s?%s", keyID, q.Encode())
+		uri := fmt.Sprintf("gcp-kms:///%s?%s", keyID, q.Encode())
 		createExternalConnection("auth-assume-role-implicit", uri)
 		backupAndRestoreFromExternalConnection(backupExternalConnectionName, "auth-assume-role-implicit")
 	})
 
 	t.Run("auth-assume-role-specified", func(t *testing.T) {
-		disallowedKMSURI := fmt.Sprintf("gs:///%s?%s=%s&%s=%s", keyID, cloud.AuthParam,
+		disallowedKMSURI := fmt.Sprintf("gcp-kms:///%s?%s=%s&%s=%s", keyID, cloud.AuthParam,
 			cloud.AuthParamSpecified, gcp.CredentialsParam, url.QueryEscape(encodedCredentials))
 		disallowedECName := "auth-assume-role-specified-disallowed"
-		createExternalConnection(disallowedECName, disallowedKMSURI)
-		disallowedBackupToExternalConnection(backupExternalConnectionName, disallowedECName)
+		disallowedCreateExternalConnection(disallowedECName, disallowedKMSURI)
 
 		q := make(url.Values)
 		q.Set(cloud.AuthParam, cloud.AuthParamSpecified)
 		q.Set(gcp.AssumeRoleParam, assumedAccount)
 		q.Set(gcp.CredentialsParam, encodedCredentials)
-		uri := fmt.Sprintf("gs:///%s?%s", keyID, q.Encode())
+		uri := fmt.Sprintf("gcp-kms:///%s?%s", keyID, q.Encode())
 		createExternalConnection("auth-assume-role-specified", uri)
 		backupAndRestoreFromExternalConnection(backupExternalConnectionName, "auth-assume-role-specified")
 	})
@@ -262,14 +258,13 @@ func TestGCSExternalConnectionAssumeRole(t *testing.T) {
 		for i, role := range roleChain {
 			i := i
 			q.Set(gcp.AssumeRoleParam, role)
-			disallowedKMSURI := fmt.Sprintf("gs:///%s?%s", keyID, q.Encode())
+			disallowedKMSURI := fmt.Sprintf("gcp-kms:///%s?%s", keyID, q.Encode())
 			disallowedECName := fmt.Sprintf("auth-assume-role-chaining-disallowed-%d", i)
-			createExternalConnection(disallowedECName, disallowedKMSURI)
-			disallowedBackupToExternalConnection(backupExternalConnectionName, disallowedECName)
+			disallowedCreateExternalConnection(disallowedECName, disallowedKMSURI)
 		}
 
 		q.Set(gcp.AssumeRoleParam, roleChainStr)
-		uri := fmt.Sprintf("gs:///%s?%s", keyID, q.Encode())
+		uri := fmt.Sprintf("gcp-kms:///%s?%s", keyID, q.Encode())
 		createExternalConnection("auth-assume-role-chaining", uri)
 		backupAndRestoreFromExternalConnection(backupExternalConnectionName, "auth-assume-role-chaining")
 	})

pkg/cloud/externalconn/connectionpb/connection.go

@@ -17,7 +17,7 @@ func (d *ConnectionDetails) Type() ConnectionType {
 	switch d.Provider {
 	case ConnectionProvider_nodelocal, ConnectionProvider_s3, ConnectionProvider_userfile:
 		return TypeStorage
-	case ConnectionProvider_gs_kms:
+	case ConnectionProvider_gcp_kms:
 		return TypeKMS
 	case ConnectionProvider_kafka:
 		return TypeStorage

pkg/cloud/externalconn/connectionpb/connection.proto

@@ -23,7 +23,7 @@ enum ConnectionProvider {
   userfile = 5;
 
   // KMS providers.
-  gs_kms = 2;
+  gcp_kms = 2;
 
   // Sink providers.
   kafka = 3;

pkg/cloud/externalconn/impl_registry.go

@@ -60,6 +60,10 @@ type TestingKnobs struct {
 	// CONNECTION` should skip the step that writes, lists and reads a sentinel
 	// file from the underlying ExternalStorage.
 	SkipCheckingExternalStorageConnection func() bool
+	// SkipCheckingKMSConnection returns whether `CREATE EXTERNAL CONNECTION`
+	// should skip the step that encrypts and decrypts a sentinel file from the
+	// underlying KMS.
+	SkipCheckingKMSConnection func() bool
 }
 
 // ModuleTestingKnobs is part of the base.ModuleTestingKnobs interface.

pkg/cloud/externalconn/utils/BUILD.bazel

@@ -7,10 +7,15 @@ go_library(
     importpath = "github.com/cockroachdb/cockroach/pkg/cloud/externalconn/utils",
     visibility = ["//visibility:public"],
     deps = [
+        "//pkg/base",
         "//pkg/cloud",
+        "//pkg/kv",
         "//pkg/security/username",
+        "//pkg/settings/cluster",
         "//pkg/sql",
+        "//pkg/sql/sqlutil",
         "//pkg/util/ioctx",
+        "//pkg/util/log",
         "@com_github_cockroachdb_errors//:errors",
     ],
 )