Merge #87302 #87311 #87316 #87446 #87459 #87506

87302: pkg/security: relax requirement to follow CRDB URI SAN cert scheme r=knz a=abarganier

Recently, a customer upgraded to v22.1.6, a recent patch release which
contains the new tenant-scoped client certificates and asssociated
authorization logic updates.

The new authz logic *required* that the SAN URIs included in the client
certificate followed the URI SAN scheme:
`crdb://tenant/<tenant_id>/user/<tenant_username>`

However, for customers that use URI SANs that do not follow this
convention or do not have the flexibility to alter the URI SAN,
this was preventing them from using their existing certificates.
This would generate an error when attempting to connect to a
SQL shell.

One example URI SAN is as follows:
`mycompany:sv:rootclient:dev:usw1`

This is a certificate that worked with the legacy behavior, but
is rejected by the new authz logic.

We should update the authz logic to be less strict about the URI SAN
following our own scheme. If we are unable to parse the URI SAN then
we should fallback to using the globally scoped client certificate
instead, enabling backwards compatibility.

This patch does just that, logging an error in the case where we are
unable to parse the URI SAN and instead falling back to the legacy
behavior, producing a global user scope for the certificate.

Release note: none

Release justification: low risk, necessary fix to enable customers
using custom URI SAN schemes to continue using their existing certificates
on newer CRDB versions.

Addresses #87235

87311: backupccl: parallelize loading of manifests from External Storage r=benbardin a=adityamaru

This change is a targetted change to parallelize the loading
of backup manifests for each incremental layer of a backup.
This method is shared by both restore as well as `SHOW BACKUP`.

Fixes: #87183

Release note: None

Release justification: low risk performance improvement required
for making `SHOW BACKUP` in the presence of many incremental layers
more performant

87316: sctest: Augmented BACKUP/RESTORE tests with table-level restore r=Xiang-Gu a=Xiang-Gu

Commit 1: non-code minor changes (added comments, moved function a little bit)
Commit 2:
   Previously, the Backup test in declarative schema changer backups the
    whole database and restore the whole database with `RESTORE DATABASE`.
    This PR augments the test by adding another flavor to restore:
    `RESTORE TABLE tbl1,...,tblN` where `tblx` are *all* the tables in the
    backup. This will nicely give us coverage for `RESTORE TABLE` when
    a declarative schema changer job is restored.

   Note that ideally we want to randomly restore only a subset of all the
    table. Indeed I tried to implement that but realize it was blocked by
    one limitation in the declarative shcema changer: We don't yet support
    restore schema changer job that skips missing sequences (E.g. if we
    have a table `t` and a sequence `s`, and I want to
    `ALTER TABLE t ADD COLUMN c DEFAULT nextval('s')`, we backup database
    in PostCommit phase. Later when we restore just `t`, the schema changer
    job will run into error `error executing 'missing rewrite for id 109 in
    <column_default_expression:<table_id:108 column_id:2
    embedded_expr:<expr:"nextval(109:::REGCLASS)" uses_sequence_ids:109 >)`)
    This issue is tracked in #87518.

Fixes: #86835

   Release justification: test-only changes
   Release note: None

87446: authors: add faizaanmadhani to authors r=faizaanmadhani a=faizaanmadhani

Release note: None
Release justification: non-production code change

87459: scbuildstmt: fallback if adding a virtual column with NOT NULL r=Xiang-Gu a=Xiang-Gu

We found a regression in the new schema changer for the following stmt: `ALTER TABLE t ADD COLUMN j INT AS (NULL::INT) VIRTUAL NOT NULL;` incorrectly succeeded. This PR made `ADD COLUMN` fall back if the to-be-added column is a virtual column with NOT NULL constraint.

Surprisingly, we actually have logic tests in place for this case but it has incorrect expected output so we also changed the exsiting tests.

Fix: #87457

Release justification: bug fix for GA blocker.

Release note: None

87506: ci: add some extra environment variables for sqllogic corpus nightly r=healthy-pod a=rickystewart

Without these extra environment variables, GitHub posting doesn't work correctly.

Release justification: Non-production code changes
Release note: None

Co-authored-by: Alex Barganier <[email protected]>
Co-authored-by: adityamaru <[email protected]>
Co-authored-by: Xiang Gu <[email protected]>
Co-authored-by: Faizaan Madhani <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>

AUTHORS

@@ -168,6 +168,7 @@ Evan Wall <[email protected]> <[email protected]>
 Evgeniy Vasilev <[email protected]>
 fabio <[email protected]>
 Faizan Qazi <[email protected]> <[email protected]>
+Faizaan Madhani <[email protected]>
 fangwens <[email protected]>
 Francis Bergin <[email protected]>
 Fenil Patel <[email protected]>

build/teamcity/cockroach/nightlies/sqllogic_corpus_nightly.sh

@@ -8,6 +8,6 @@ source "$dir/teamcity-support.sh"  # For $root
 source "$dir/teamcity-bazel-support.sh"  # For run_bazel
 
 tc_start_block "Run SQL Logic Test with Declarative Corpus Generation"
-BAZEL_SUPPORT_EXTRA_DOCKER_ARGS="-e TC_BUILD_BRANCH -e GITHUB_API_TOKEN -e GOOGLE_EPHEMERAL_CREDENTIALS -e BUILD_VCS_NUMBER -e TC_BUILD_ID -e TC_SERVER_URL" \
+BAZEL_SUPPORT_EXTRA_DOCKER_ARGS="-e TC_BUILD_BRANCH -e GITHUB_API_TOKEN -e GOOGLE_EPHEMERAL_CREDENTIALS -e BUILD_VCS_NUMBER -e TC_BUILD_ID -e TC_SERVER_URL -e TC_BUILDTYPE_ID -e GITHUB_REPO" \
   run_bazel build/teamcity/cockroach/nightlies/sqllogic_corpus_nightly_impl.sh
 tc_end_block "Run SQL Logic Test High VModule"

pkg/ccl/backupccl/backup_job.go

@@ -13,6 +13,8 @@ import (
 	"fmt"
 	"net/url"
 	"path"
+	"reflect"
+	"sort"
 	"strings"
 	"time"
 
@@ -790,6 +792,122 @@ func (b *backupResumer) ReportResults(ctx context.Context, resultsCh chan<- tree
 	}
 }
 
+func getBackupDetailAndManifest(
+	ctx context.Context,
+	execCfg *sql.ExecutorConfig,
+	txn *kv.Txn,
+	initialDetails jobspb.BackupDetails,
+	user username.SQLUsername,
+	backupDestination backupdest.ResolvedDestination,
+) (jobspb.BackupDetails, backuppb.BackupManifest, error) {
+	makeCloudStorage := execCfg.DistSQLSrv.ExternalStorageFromURI
+
+	kmsEnv := backupencryption.MakeBackupKMSEnv(execCfg.Settings, &execCfg.ExternalIODirConfig,
+		execCfg.DB, user, execCfg.InternalExecutor)
+
+	mem := execCfg.RootMemoryMonitor.MakeBoundAccount()
+	defer mem.Close(ctx)
+
+	prevBackups, encryptionOptions, memSize, err := backupinfo.FetchPreviousBackups(ctx, &mem, user,
+		makeCloudStorage, backupDestination.PrevBackupURIs, *initialDetails.EncryptionOptions, &kmsEnv)
+
+	if err != nil {
+		return jobspb.BackupDetails{}, backuppb.BackupManifest{}, err
+	}
+	defer func() {
+		mem.Shrink(ctx, memSize)
+	}()
+
+	if len(prevBackups) > 0 {
+		baseManifest := prevBackups[0]
+		if baseManifest.DescriptorCoverage == tree.AllDescriptors &&
+			!initialDetails.FullCluster {
+			return jobspb.BackupDetails{}, backuppb.BackupManifest{}, errors.Errorf("cannot append a backup of specific tables or databases to a cluster backup")
+		}
+
+		if err := requireEnterprise(execCfg, "incremental"); err != nil {
+			return jobspb.BackupDetails{}, backuppb.BackupManifest{}, err
+		}
+		lastEndTime := prevBackups[len(prevBackups)-1].EndTime
+		if lastEndTime.Compare(initialDetails.EndTime) > 0 {
+			return jobspb.BackupDetails{}, backuppb.BackupManifest{},
+				errors.Newf("`AS OF SYSTEM TIME` %s must be greater than "+
+					"the previous backup's end time of %s.",
+					initialDetails.EndTime.GoTime(), lastEndTime.GoTime())
+		}
+	}
+
+	localityKVs := make([]string, len(backupDestination.URIsByLocalityKV))
+	i := 0
+	for k := range backupDestination.URIsByLocalityKV {
+		localityKVs[i] = k
+		i++
+	}
+
+	for i := range prevBackups {
+		prevBackup := prevBackups[i]
+		// IDs are how we identify tables, and those are only meaningful in the
+		// context of their own cluster, so we need to ensure we only allow
+		// incremental previous backups that we created.
+		if fromCluster := prevBackup.ClusterID; !fromCluster.Equal(execCfg.NodeInfo.LogicalClusterID()) {
+			return jobspb.BackupDetails{}, backuppb.BackupManifest{}, errors.Newf("previous BACKUP belongs to cluster %s", fromCluster.String())
+		}
+
+		prevLocalityKVs := prevBackup.LocalityKVs
+
+		// Checks that each layer in the backup uses the same localities
+		// Does NOT check that each locality/layer combination is actually at the
+		// expected locations.
+		// This is complex right now, but should be easier shortly.
+		// TODO(benbardin): Support verifying actual existence of localities for
+		// each layer after deprecating TO-syntax in 22.2
+		sort.Strings(localityKVs)
+		sort.Strings(prevLocalityKVs)
+		if !(len(localityKVs) == 0 && len(prevLocalityKVs) == 0) && !reflect.DeepEqual(localityKVs,
+			prevLocalityKVs) {
+			// Note that this won't verify the default locality. That's not
+			// necessary, because the default locality defines the backup manifest
+			// location. If that URI isn't right, the backup chain will fail to
+			// load.
+			return jobspb.BackupDetails{}, backuppb.BackupManifest{}, errors.Newf(
+				"Requested backup has localities %s, but a previous backup layer in this collection has localities %s. "+
+					"Mismatched backup layers are not supported. Please take a new full backup with the new localities, or an "+
+					"incremental backup with matching localities.",
+				localityKVs, prevLocalityKVs,
+			)
+		}
+	}
+
+	// updatedDetails and backupManifest should be treated as read-only after
+	// they're returned from their respective functions. Future changes to those
+	// objects should be made within those functions.
+	updatedDetails, err := updateBackupDetails(
+		ctx,
+		initialDetails,
+		backupDestination.CollectionURI,
+		backupDestination.DefaultURI,
+		backupDestination.ChosenSubdir,
+		backupDestination.URIsByLocalityKV,
+		prevBackups,
+		encryptionOptions,
+		&kmsEnv)
+	if err != nil {
+		return jobspb.BackupDetails{}, backuppb.BackupManifest{}, err
+	}
+
+	backupManifest, err := createBackupManifest(
+		ctx,
+		execCfg,
+		txn,
+		updatedDetails,
+		prevBackups)
+	if err != nil {
+		return jobspb.BackupDetails{}, backuppb.BackupManifest{}, err
+	}
+
+	return updatedDetails, backupManifest, nil
+}
+
 func (b *backupResumer) readManifestOnResume(
 	ctx context.Context,
 	mem *mon.BoundAccount,

pkg/ccl/backupccl/backup_planning.go

@@ -11,15 +11,12 @@ package backupccl
 import (
 	"context"
 	"fmt"
-	"reflect"
-	"sort"
 	"strconv"
 	"strings"
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/build"
 	"github.com/cockroachdb/cockroach/pkg/ccl/backupccl/backupbase"
-	"github.com/cockroachdb/cockroach/pkg/ccl/backupccl/backupdest"
 	"github.com/cockroachdb/cockroach/pkg/ccl/backupccl/backupencryption"
 	"github.com/cockroachdb/cockroach/pkg/ccl/backupccl/backupinfo"
 	"github.com/cockroachdb/cockroach/pkg/ccl/backupccl/backuppb"
@@ -37,7 +34,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/protectedts/ptpb"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/scheduledjobs"
-	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/sql"
@@ -1217,122 +1213,6 @@ func checkForNewTables(
 	return nil
 }
 
-func getBackupDetailAndManifest(
-	ctx context.Context,
-	execCfg *sql.ExecutorConfig,
-	txn *kv.Txn,
-	initialDetails jobspb.BackupDetails,
-	user username.SQLUsername,
-	backupDestination backupdest.ResolvedDestination,
-) (jobspb.BackupDetails, backuppb.BackupManifest, error) {
-	makeCloudStorage := execCfg.DistSQLSrv.ExternalStorageFromURI
-
-	kmsEnv := backupencryption.MakeBackupKMSEnv(execCfg.Settings, &execCfg.ExternalIODirConfig,
-		execCfg.DB, user, execCfg.InternalExecutor)
-
-	mem := execCfg.RootMemoryMonitor.MakeBoundAccount()
-	defer mem.Close(ctx)
-
-	prevBackups, encryptionOptions, memSize, err := backupinfo.FetchPreviousBackups(ctx, &mem, user,
-		makeCloudStorage, backupDestination.PrevBackupURIs, *initialDetails.EncryptionOptions, &kmsEnv)
-
-	if err != nil {
-		return jobspb.BackupDetails{}, backuppb.BackupManifest{}, err
-	}
-	defer func() {
-		mem.Shrink(ctx, memSize)
-	}()
-
-	if len(prevBackups) > 0 {
-		baseManifest := prevBackups[0]
-		if baseManifest.DescriptorCoverage == tree.AllDescriptors &&
-			!initialDetails.FullCluster {
-			return jobspb.BackupDetails{}, backuppb.BackupManifest{}, errors.Errorf("cannot append a backup of specific tables or databases to a cluster backup")
-		}
-
-		if err := requireEnterprise(execCfg, "incremental"); err != nil {
-			return jobspb.BackupDetails{}, backuppb.BackupManifest{}, err
-		}
-		lastEndTime := prevBackups[len(prevBackups)-1].EndTime
-		if lastEndTime.Compare(initialDetails.EndTime) > 0 {
-			return jobspb.BackupDetails{}, backuppb.BackupManifest{},
-				errors.Newf("`AS OF SYSTEM TIME` %s must be greater than "+
-					"the previous backup's end time of %s.",
-					initialDetails.EndTime.GoTime(), lastEndTime.GoTime())
-		}
-	}
-
-	localityKVs := make([]string, len(backupDestination.URIsByLocalityKV))
-	i := 0
-	for k := range backupDestination.URIsByLocalityKV {
-		localityKVs[i] = k
-		i++
-	}
-
-	for i := range prevBackups {
-		prevBackup := prevBackups[i]
-		// IDs are how we identify tables, and those are only meaningful in the
-		// context of their own cluster, so we need to ensure we only allow
-		// incremental previous backups that we created.
-		if fromCluster := prevBackup.ClusterID; !fromCluster.Equal(execCfg.NodeInfo.LogicalClusterID()) {
-			return jobspb.BackupDetails{}, backuppb.BackupManifest{}, errors.Newf("previous BACKUP belongs to cluster %s", fromCluster.String())
-		}
-
-		prevLocalityKVs := prevBackup.LocalityKVs
-
-		// Checks that each layer in the backup uses the same localities
-		// Does NOT check that each locality/layer combination is actually at the
-		// expected locations.
-		// This is complex right now, but should be easier shortly.
-		// TODO(benbardin): Support verifying actual existence of localities for
-		// each layer after deprecating TO-syntax in 22.2
-		sort.Strings(localityKVs)
-		sort.Strings(prevLocalityKVs)
-		if !(len(localityKVs) == 0 && len(prevLocalityKVs) == 0) && !reflect.DeepEqual(localityKVs,
-			prevLocalityKVs) {
-			// Note that this won't verify the default locality. That's not
-			// necessary, because the default locality defines the backup manifest
-			// location. If that URI isn't right, the backup chain will fail to
-			// load.
-			return jobspb.BackupDetails{}, backuppb.BackupManifest{}, errors.Newf(
-				"Requested backup has localities %s, but a previous backup layer in this collection has localities %s. "+
-					"Mismatched backup layers are not supported. Please take a new full backup with the new localities, or an "+
-					"incremental backup with matching localities.",
-				localityKVs, prevLocalityKVs,
-			)
-		}
-	}
-
-	// updatedDetails and backupManifest should be treated as read-only after
-	// they're returned from their respective functions. Future changes to those
-	// objects should be made within those functions.
-	updatedDetails, err := updateBackupDetails(
-		ctx,
-		initialDetails,
-		backupDestination.CollectionURI,
-		backupDestination.DefaultURI,
-		backupDestination.ChosenSubdir,
-		backupDestination.URIsByLocalityKV,
-		prevBackups,
-		encryptionOptions,
-		&kmsEnv)
-	if err != nil {
-		return jobspb.BackupDetails{}, backuppb.BackupManifest{}, err
-	}
-
-	backupManifest, err := createBackupManifest(
-		ctx,
-		execCfg,
-		txn,
-		updatedDetails,
-		prevBackups)
-	if err != nil {
-		return jobspb.BackupDetails{}, backuppb.BackupManifest{}, err
-	}
-
-	return updatedDetails, backupManifest, nil
-}
-
 func getTenantInfo(
 	ctx context.Context, execCfg *sql.ExecutorConfig, txn *kv.Txn, jobDetails jobspb.BackupDetails,
 ) ([]roachpb.Span, []descpb.TenantInfoWithUsage, error) {

pkg/ccl/backupccl/backupdest/BUILD.bazel

@@ -29,6 +29,7 @@ go_library(
         "//pkg/util/ioctx",
         "//pkg/util/mon",
         "//pkg/util/timeutil",
+        "//pkg/util/tracing",
         "@com_github_cockroachdb_errors//:errors",
     ],
 )