Merge #57352

57352: backupccl: make backup (mostly) work in tenants r=dt a=dt

These changes conditionally remove the dependencies on gossip/protected ts/etc that would prevent a BACKUP from being planned and run inside a tenant. They also allow tenants to run ExportRequest so long as they are asking for the exported data to be returned, rather than sent directly from the kv node handling the request to a third party destination. This addition of this last limitation however does means that BACKUP will not work in tenants until #57317 lands as well.

Co-authored-by: David Taylor <[email protected]>

pkg/ccl/backupccl/backup_job.go

@@ -203,15 +203,22 @@ func backup(
 	var lastCheckpoint time.Time
 
 	var ranges []roachpb.RangeDescriptor
-	if err := db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
-		var err error
-		// TODO(benesch): limit the range descriptors we fetch to the ranges that
-		// are actually relevant in the backup to speed up small backups on large
-		// clusters.
-		ranges, err = allRangeDescriptors(ctx, txn)
-		return err
-	}); err != nil {
-		return RowCount{}, err
+
+	// TODO(dt): we should really get a RangeDescriptor iterator and do this the
+	// right way, just iterate the spans we need, in a tenant or not. For now
+	// though tenants aren't allowed to just go read all ranges so we'll skip this
+	// and let distsender just split our ExportRequest for us.
+	if execCtx.ExecCfg().Codec.ForSystemTenant() {
+		if err := db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
+			var err error
+			// TODO(benesch): limit the range descriptors we fetch to the ranges that
+			// are actually relevant in the backup to speed up small backups on large
+			// clusters.
+			ranges, err = allRangeDescriptors(ctx, txn)
+			return err
+		}); err != nil {
+			return RowCount{}, err
+		}
 	}
 
 	var completedSpans, completedIntroducedSpans []roachpb.Span
@@ -499,7 +506,7 @@ func (b *backupResumer) Resume(
 
 	numClusterNodes, err := clusterNodeCount(p.ExecCfg().Gossip)
 	if err != nil {
-		if !build.IsRelease() {
+		if !build.IsRelease() && p.ExecCfg().Codec.ForSystemTenant() {
 			return err
 		}
 		log.Warningf(ctx, "unable to determine cluster node count: %v", err)

pkg/ccl/backupccl/backup_planning.go

@@ -1122,7 +1122,7 @@ func backupPlanHook(
 			EncryptionInfo:    encryptionInfo,
 			CollectionURI:     collectionURI,
 		}
-		if len(spans) > 0 {
+		if len(spans) > 0 && p.ExecCfg().Codec.ForSystemTenant() {
 			protectedtsID := uuid.MakeV4()
 			backupDetails.ProtectedTimestampRecord = &protectedtsID
 		}
@@ -1310,6 +1310,9 @@ func protectTimestampForBackup(
 	startTime, endTime hlc.Timestamp,
 	backupDetails jobspb.BackupDetails,
 ) error {
+	if backupDetails.ProtectedTimestampRecord == nil {
+		return nil
+	}
 	if len(spans) > 0 {
 		tsToProtect := endTime
 		if !startTime.IsEmpty() {

pkg/ccl/backupccl/backup_test.go

@@ -6124,6 +6124,11 @@ func TestBackupRestoreTenant(t *testing.T) {
 	systemDB.Exec(t, `BACKUP TENANT 11 TO 'nodelocal://1/t11'`)
 	systemDB.Exec(t, `BACKUP TENANT 20 TO 'nodelocal://1/t20'`)
 
+	t.Run("inside-tenant", func(t *testing.T) {
+		skip.WithIssue(t, 57317)
+		tenant10.Exec(t, `BACKUP DATABASE foo TO 'userfile://defaultdb.myfililes/test'`)
+	})
+
 	t.Run("non-existent", func(t *testing.T) {
 		systemDB.ExpectErr(t, "tenant 123 does not exist", `BACKUP TENANT 123 TO 'nodelocal://1/t1'`)
 		systemDB.ExpectErr(t, "tenant 21 does not exist", `BACKUP TENANT 21 TO 'nodelocal://1/t20'`)
@@ -7087,7 +7092,7 @@ schema_name`
 	sqlDBRestore.CheckQueryResults(t, checkSchemasQuery,
 		[][]string{{"pg_temp_0_0"}, {"pg_temp_0_1"}, {"pg_temp_0_2"}, {"pg_temp_0_3"}})
 
-	checkTempTablesQuery := `SELECT schema_name, 
+	checkTempTablesQuery := `SELECT schema_name,
 table_name FROM [SHOW TABLES] ORDER BY schema_name, table_name`
 	sqlDBRestore.CheckQueryResults(t, checkTempTablesQuery, [][]string{{"pg_temp_0_0", "t"},
 		{"pg_temp_0_1", "t"}, {"pg_temp_0_2", "t"}, {"pg_temp_0_3", "t"}})

pkg/ccl/storageccl/export.go

@@ -100,6 +100,17 @@ func evalExport(
 		log.Eventf(ctx, "export [%s,%s)", args.Key, args.EndKey)
 	}
 
+	if makeExternalStorage {
+		// TODO(dt): this blanket ban means we must do all uploads from the caller
+		// which is nice and simple but imposes extra copies/overhead/cost. We might
+		// want to instead allow *some* forms of external storage for *some* tenants
+		// e.g. allow some tenants to dial out to s3 directly -- if we do though we
+		// would need to continue to restrict unsafe ones like userfile here.
+		if _, ok := roachpb.TenantFromContext(ctx); ok {
+			return result.Result{}, errors.Errorf("requests on behalf of tenants are not allowed to contact external storage")
+		}
+	}
+
 	// To get the store to export to, first try to match the locality of this node
 	// to the locality KVs in args.StorageByLocalityKV (used for partitioned
 	// backups). If that map isn't set or there's no match, fall back to

pkg/rpc/auth_tenant.go

@@ -99,6 +99,7 @@ var reqMethodAllowlist = [...]bool{
 	roachpb.QueryIntent:    true,
 	roachpb.InitPut:        true,
 	roachpb.AddSSTable:     true,
+	roachpb.Export:         true,
 	roachpb.Refresh:        true,
 	roachpb.RefreshRange:   true,
 }