Merge #141745 #143709

141745: pkg/cli: allow system.zones in debug zip r=annrpom a=annrpom

This patch adds `system.zones` into debug zip collection.

Epic: none

Release note: None

143709: backup: fix up encryption handling in compacted backups r=kev-cao a=msbutler

Fixes #143725

Release note: none

Co-authored-by: Annie Pompa <[email protected]>
Co-authored-by: Michael Butler <[email protected]>

Author: 3 people

pkg/backup/backup_job.go

@@ -1663,8 +1663,11 @@ func updateBackupDetails(
 		startTime = prevBackups[len(prevBackups)-1].EndTime
 	}
 
-	// If we didn't load any prior backups from which get encryption info, we
-	// need to generate encryption specific data.
+	// If we didn't load any prior backups with EncryptionInfo, we need to
+	// generate encryption specific data.
+	//
+	// TODO(msbutler): consider using prev backups instead of nil encryptiuon
+	// info, if possible.
 	var encryptionInfo *jobspb.EncryptionInfo
 	if encryptionOptions == nil {
 		encryptionOptions, encryptionInfo, err = backupencryption.MakeNewEncryptionOptions(ctx, details.EncryptionOptions, kmsEnv)

pkg/backup/backupdest/backup_destination.go

@@ -109,14 +109,17 @@ type ResolvedDestination struct {
 // In addition, in this case that this backup is an incremental backup (either
 // explicitly, or due to the auto-append feature), it will resolve the
 // encryption options based on the base backup, as well as find all previous
-// backup manifests in the backup chain. Additionally, if a startTime is provided,
-// it will be used to determine its destination. If one is not provided, we assume
-// that the incremental is chained off of the most recent backup in the chain
-// and that backup's manifest will be fetched to determine the start time.
+// backup manifests in the backup chain. Additionally, if a startTime is
+// provided, it will be used to determine its destination. If one is not
+// provided, we assume that the incremental is chained off of the most recent
+// backup in the chain and that backup's manifest will be fetched to determine
+// the start time.
 //
 // The encryptions passed to the encryption options should include the raw
-// encryption options. TODO (kev-cao): Once we have completed the backup
-// directory index work, we can remove the need for encryption and KMS.
+// encryption options, not the resolved key.
+//
+// TODO (kev-cao): Once we have completed the backup directory index work, we
+// can remove the need for encryption and KMS.
 func ResolveDest(
 	ctx context.Context,
 	user username.SQLUsername,

pkg/backup/backupencryption/encryption.go

@@ -204,7 +204,7 @@ func ValidateKMSURIsAgainstFullBackup(
 func MakeNewEncryptionOptions(
 	ctx context.Context, encryptionParams *jobspb.BackupEncryptionOptions, kmsEnv cloud.KMSEnv,
 ) (*jobspb.BackupEncryptionOptions, *jobspb.EncryptionInfo, error) {
-	if encryptionParams == nil || encryptionParams.Mode == jobspb.EncryptionMode_None {
+	if !encryptionParams.IsEncrypted() {
 		return nil, nil, nil
 	}
 	var encryptionOptions *jobspb.BackupEncryptionOptions
@@ -332,7 +332,9 @@ func WriteNewEncryptionInfoToBackup(
 
 // GetEncryptionFromBase retrieves the encryption options of the base backup. It
 // is expected that incremental backups use the same encryption options as the
-// base backups.
+// base backups. The encryptionParams input is expected not to have a key set
+// and to simply have the user supplied fields. The output will only have the
+// key set.
 func GetEncryptionFromBase(
 	ctx context.Context,
 	user username.SQLUsername,
@@ -341,7 +343,7 @@ func GetEncryptionFromBase(
 	encryptionParams *jobspb.BackupEncryptionOptions,
 	kmsEnv cloud.KMSEnv,
 ) (*jobspb.BackupEncryptionOptions, error) {
-	if encryptionParams == nil || encryptionParams.Mode == jobspb.EncryptionMode_None {
+	if !encryptionParams.IsEncrypted() {
 		return nil, nil
 	}
 	exportStore, err := makeCloudStorage(ctx, baseBackupURI, user)
@@ -359,9 +361,14 @@ func GetEncryptionFromBaseStore(
 	encryptionParams *jobspb.BackupEncryptionOptions,
 	kmsEnv cloud.KMSEnv,
 ) (*jobspb.BackupEncryptionOptions, error) {
-	if encryptionParams == nil || encryptionParams.Mode == jobspb.EncryptionMode_None {
+	if !encryptionParams.IsEncrypted() {
 		return nil, nil
 	}
+
+	if encryptionParams.HasKey() {
+		return nil, errors.New("encryption options already have a key")
+	}
+
 	opts, err := ReadEncryptionOptions(ctx, baseStore)
 	if err != nil {
 		return nil, err

pkg/backup/compaction_job.go

@@ -211,7 +211,7 @@ func (b *backupResumer) ResumeCompaction(
 	// We interleave the computation of the compaction chain between the destination
 	// resolution and writing of backup lock due to the need to verify that the
 	// compaction chain is a valid chain.
-	prevManifests, localityInfo, encryption, allIters, err := getBackupChain(
+	prevManifests, localityInfo, baseEncryptionOpts, allIters, err := getBackupChain(
 		ctx, execCtx.ExecCfg(), execCtx.User(), initialDetails.Destination,
 		initialDetails.EncryptionOptions, initialDetails.EndTime, kmsEnv,
 	)
@@ -253,7 +253,7 @@ func (b *backupResumer) ResumeCompaction(
 			return err
 		}
 		updatedDetails, err = updateCompactionBackupDetails(
-			ctx, compactChain, initialDetails, backupDest, encryption, kmsEnv,
+			ctx, compactChain, initialDetails, backupDest, baseEncryptionOpts, kmsEnv,
 		)
 		if err != nil {
 			return err
@@ -492,22 +492,12 @@ func updateCompactionBackupDetails(
 	compactionChain compactionChain,
 	initialDetails jobspb.BackupDetails,
 	resolvedDest backupdest.ResolvedDestination,
-	encryption *jobspb.BackupEncryptionOptions,
+	baseEncryptOpts *jobspb.BackupEncryptionOptions,
 	kmsEnv cloud.KMSEnv,
 ) (jobspb.BackupDetails, error) {
 	if len(compactionChain.chainToCompact) == 0 {
 		return jobspb.BackupDetails{}, errors.New("no backup manifests to compact")
 	}
-	var encryptionInfo *jobspb.EncryptionInfo
-	if encryption != nil {
-		var err error
-		_, encryptionInfo, err = backupencryption.MakeNewEncryptionOptions(
-			ctx, encryption, kmsEnv,
-		)
-		if err != nil {
-			return jobspb.BackupDetails{}, err
-		}
-	}
 	lastBackup := compactionChain.lastBackup()
 	allDescs, _, err := backupinfo.LoadSQLDescsFromBackupsAtTime(
 		ctx,
@@ -524,13 +514,16 @@ func updateCompactionBackupDetails(
 	destination := initialDetails.Destination
 	destination.Subdir = resolvedDest.ChosenSubdir
 	compactedDetails := jobspb.BackupDetails{
-		Destination:         destination,
-		StartTime:           compactionChain.start,
-		EndTime:             compactionChain.end,
-		URI:                 resolvedDest.DefaultURI,
-		URIsByLocalityKV:    resolvedDest.URIsByLocalityKV,
-		EncryptionOptions:   encryption,
-		EncryptionInfo:      encryptionInfo,
+		Destination:      destination,
+		StartTime:        compactionChain.start,
+		EndTime:          compactionChain.end,
+		URI:              resolvedDest.DefaultURI,
+		URIsByLocalityKV: resolvedDest.URIsByLocalityKV,
+		// Because we cannot have nice things, we persist a semi hydrated version of
+		// EncryptionOptions at planning, and then add the key at the beginning of
+		// resume using the same data structure in details. NB: EncryptionInfo is
+		// left blank and only added on the full backup (i think).
+		EncryptionOptions:   baseEncryptOpts,
 		CollectionURI:       resolvedDest.CollectionURI,
 		ResolvedTargets:     allDescsPb,
 		ResolvedCompleteDbs: lastBackup.CompleteDbs,
@@ -692,28 +685,22 @@ func getBackupChain(
 			log.Warningf(ctx, "failed to cleanup incremental backup stores: %+v", err)
 		}
 	}()
-	// If encryption keys have not already been computed, then we will compute
-	// it using the base store.
-	// TODO (kev-cao): Once we resolve #143725, we can remove this check and
-	// expect `GetEncryptionFromBaseStore` to be idempotent.
-	var encryption *jobspb.BackupEncryptionOptions
-	if encryptionOpts != nil && encryptionOpts.Mode != jobspb.EncryptionMode_None &&
-		(encryptionOpts.Key != nil || encryptionOpts.KMSInfo != nil) {
-		encryption = encryptionOpts
-	} else {
-		encryption, err = backupencryption.GetEncryptionFromBaseStore(
+	baseEncryptionInfo := encryptionOpts
+	if encryptionOpts != nil && !encryptionOpts.HasKey() {
+		baseEncryptionInfo, err = backupencryption.GetEncryptionFromBaseStore(
 			ctx, baseStores[0], encryptionOpts, kmsEnv,
 		)
 		if err != nil {
 			return nil, nil, nil, nil, err
 		}
 	}
+
 	mem := execCfg.RootMemoryMonitor.MakeBoundAccount()
 	defer mem.Close(ctx)
 
 	_, manifests, localityInfo, memReserved, err := backupdest.ResolveBackupManifests(
 		ctx, &mem, baseStores, incStores, mkStore, resolvedBaseDirs,
-		resolvedIncDirs, endTime, encryption, kmsEnv,
+		resolvedIncDirs, endTime, baseEncryptionInfo, kmsEnv,
 		user, false,
 	)
 	if err != nil {
@@ -723,12 +710,12 @@ func getBackupChain(
 		mem.Shrink(ctx, memReserved)
 	}()
 	allIters, err := backupinfo.GetBackupManifestIterFactories(
-		ctx, execCfg.DistSQLSrv.ExternalStorage, manifests, encryption, kmsEnv,
+		ctx, execCfg.DistSQLSrv.ExternalStorage, manifests, baseEncryptionInfo, kmsEnv,
 	)
 	if err != nil {
 		return nil, nil, nil, nil, err
 	}
-	return manifests, localityInfo, encryption, allIters, nil
+	return manifests, localityInfo, baseEncryptionInfo, allIters, nil
 }
 
 // concludeBackupCompaction completes the backup compaction process after the backup has been

pkg/backup/compaction_test.go

@@ -256,6 +256,7 @@ ARRAY['nodelocal://1/backup/%d'], '%s', ''::BYTES, %d::DECIMAL, %d::DECIMAL
 		db.Exec(t, "CREATE TABLE foo (a INT, b INT)")
 		defer func() {
 			db.Exec(t, "DROP TABLE foo")
+			db.Exec(t, "SET CLUSTER SETTING jobs.debug.pausepoints = ''")
 		}()
 		db.Exec(t, "INSERT INTO foo VALUES (1, 1)")
 		opts := "encryption_passphrase = 'correct-horse-battery-staple'"
@@ -277,6 +278,10 @@ ARRAY['nodelocal://1/backup/%d'], '%s', ''::BYTES, %d::DECIMAL, %d::DECIMAL
 		var backupPath string
 		db.QueryRow(t, "SHOW BACKUPS IN 'nodelocal://1/backup/6'").Scan(&backupPath)
 		var jobID jobspb.JobID
+		pause := rand.Intn(2) == 0
+		if pause {
+			db.Exec(t, "SET CLUSTER SETTING jobs.debug.pausepoints = 'backup.after.details_has_checkpoint'")
+		}
 		db.QueryRow(
 			t,
 			fmt.Sprintf(
@@ -290,6 +295,10 @@ crdb_internal.json_to_pb(
 				backupPath, start, end,
 			),
 		).Scan(&jobID)
+		if pause {
+			jobutils.WaitForJobToPause(t, db, jobID)
+			db.Exec(t, "RESUME JOB $1", jobID)
+		}
 		waitForSuccessfulJob(t, tc, jobID)
 		validateCompactedBackupForTablesWithOpts(
 			t, db, []string{"foo"}, "'nodelocal://1/backup/6'", start, end, opts,

pkg/cli/testdata/zip/file-filters/testzip_file_filters

@@ -117,6 +117,7 @@ debug/system.tenant_settings.txt
 debug/system.tenant_tasks.txt
 debug/system.tenant_usage.txt
 debug/system.tenants.txt
+debug/system.zones.txt
 debug/tenant_ranges.err.txt
 
 
@@ -246,6 +247,7 @@ debug/system.tenant_settings.txt
 debug/system.tenant_tasks.txt
 debug/system.tenant_usage.txt
 debug/system.tenants.txt
+debug/system.zones.txt
 debug/tenant_ranges.err.txt
 
 
@@ -406,6 +408,7 @@ debug/system.tenant_settings.txt
 debug/system.tenant_tasks.txt
 debug/system.tenant_usage.txt
 debug/system.tenants.txt
+debug/system.zones.txt
 debug/tenant_ranges.err.txt
 
 
@@ -541,6 +544,7 @@ debug/system.tenant_settings.txt
 debug/system.tenant_tasks.txt
 debug/system.tenant_usage.txt
 debug/system.tenants.txt
+debug/system.zones.txt
 debug/tenant_ranges.err.txt
 
 #include only txt files with system table file exclustion

pkg/cli/zip_table_registry.go

@@ -1125,7 +1125,6 @@ var zipInternalTablesPerNode = DebugZipTableRegistry{
 //   - system.join_tokens: avoid downloading secret join keys.
 //   - system.comments: avoid downloading noise from SQL schema.
 //   - system.ui: avoid downloading noise from UI customizations.
-//   - system.zones: the contents of crdb_internal.zones is easier to use.
 //   - system.statement_bundle_chunks: avoid downloading a large table that's
 //     hard to interpret currently.
 //   - system.statement_statistics: historical data, usually too much to
@@ -1604,4 +1603,11 @@ limit 5000;`,
 			"info",
 		},
 	},
+	"system.zones": {
+		customQueryRedacted: `SELECT "id" FROM system.zones;`,
+		customQueryUnredacted: `SELECT 
+			"id", 
+			crdb_internal.pb_to_json('cockroach.config.zonepb.ZoneConfig', config)
+			FROM system.zones;`,
+	},
 }

pkg/cli/zip_table_registry_test.go

@@ -128,7 +128,6 @@ func TestNoForbiddenSystemTablesInDebugZip(t *testing.T) {
 		"system.join_tokens",
 		"system.comments",
 		"system.ui",
-		"system.zones",
 		"system.statement_bundle_chunks",
 		"system.statement_statistics",
 		"system.transaction_statistics",

pkg/jobs/jobspb/jobs.go

@@ -138,3 +138,17 @@ func (m *ChangefeedProgress_Checkpoint) IsEmpty() bool {
 func (r RestoreDetails) OnlineImpl() bool {
 	return r.ExperimentalCopy || r.ExperimentalOnline
 }
+
+func (b *BackupEncryptionOptions) HasKey() bool {
+
+	if b.KMSInfo != nil {
+		return len(b.KMSInfo.EncryptedDataKey) > 0
+	}
+	// Used for encryption passphrases.
+	return len(b.Key) > 0
+}
+
+func (b *BackupEncryptionOptions) IsEncrypted() bool {
+	// For dumb reasons, there are two ways to represent no encryption.
+	return !(b == nil || b.Mode == EncryptionMode_None)
+}

pkg/jobs/jobspb/jobs.proto

@@ -396,7 +396,21 @@ message BackupDetails {
   // partitioned backups. The map does not include the default locality.
   map<string, string> uris_by_locality_kv = 5 [(gogoproto.customname) = "URIsByLocalityKV"];
   bytes deprecated_backup_manifest = 4;
+
+  // EncryptionOptions contains user inputed data initially, but once
+  // ResolveDest is called at the beginning of resumption, this field will
+  // contain the encryption key. 
+  //
+  // TODO(msbutler): use seperate job info key to persist hydrated encryption
+  // keys so encrypted backup logic becomes easier to reason about. This
+  // shouldn't be as complicated as rocket science.
   BackupEncryptionOptions encryption_options = 6;
+
+  // EncryptionInfo should only ever get set on the first encrypted backup in
+  // the chain because it persists the salt that each key in the chain must
+  // have. 
+  //
+  // TODO(msbutler): understand if only the full ever sets EncryptionInfo.
   EncryptionInfo encryption_info = 9;
 
   // ProtectedTimestampRecord is the ID of the protected timestamp record