Merge #153628 #153668 #153755

153628: backup: remove bulkio.backup.deprecated_full_backup_with_subdir.enabled setting r=jeffswenson a=msbutler

This patch removes the bulkio.backup.deprecated_full_backup_with_subdir.enabled
cluster setting, since when set to true, the backups will now fail.

Informs #139159

Release note (ops change): this patch removes the
bulkio.backup.deprecated_full_backup_with_subdir.enabled cluster setting, since
backups will now fail if it is set to true.

153668: external connection: fix sql injection vuln in ALTER EXTERNAL CONNECTION r=jeffswenson a=msbutler

Previously, the sql command which ran directly on system.external_connections passed the external connection name via string parsing, which makes the query vulnerable to sql injection. This patch fixes this vulnerability by passing the name as a parameter.

Epic: none

Release note: none

153755: roachprod: fix NewPromClient creating IAP token source when disabled r=golgeek a=ajstorm

## Summary

Fixes a bug introduced in the IAP authentication refactor (ba62711) where `NewPromClient()` would always attempt to create an IAP token source, even when Prometheus integration was disabled.

## Problem

After the recent IAP authentication refactor, users running `roachprod start` outside of Google Cloud environments would encounter this error:

```
failed to create IAP token source: failed to get default credentials: glcoud not on path
```

This happens even when `ROACHPROD_PROM_HOST_URL=""` is set to disable Prometheus integration.

## Root Cause

The condition in `NewPromClient()` only checked `if c.httpClient == nil` but didn't verify whether the client was disabled. When `ROACHPROD_PROM_HOST_URL=""` is set, `promRegistrationUrl` becomes `""` which correctly sets `disabled: true`, but the IAP token source creation was still being attempted.

## Solution

Modified the condition to also check `!c.disabled`, ensuring that when Prometheus integration is disabled, no IAP authentication is attempted.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-authored-by: Michael Butler <[email protected]>
Co-authored-by: Adam Storm <[email protected]>

Author: 3 people

docs/generated/settings/settings-for-tenants.txt

@@ -5,7 +5,6 @@ admission.epoch_lifo.epoch_duration	duration	100ms	the duration of an epoch, for
 admission.epoch_lifo.queue_delay_threshold_to_switch_to_lifo	duration	105ms	the queue delay encountered by a (tenant,priority) for switching to epoch-LIFO ordering	application
 admission.sql_kv_response.enabled	boolean	true	when true, work performed by the SQL layer when receiving a KV response is subject to admission control	application
 admission.sql_sql_response.enabled	boolean	true	when true, work performed by the SQL layer when receiving a DistSQL response is subject to admission control	application
-bulkio.backup.deprecated_full_backup_with_subdir.enabled	boolean	false	when true, a backup command with a user specified subdirectory will create a full backup at the subdirectory if no backup already exists at that subdirectory	application
 bulkio.backup.file_size	byte size	128 MiB	target size for individual data files produced during BACKUP	application
 bulkio.backup.read_timeout	duration	5m0s	amount of time after which a read attempt is considered timed out, which causes the backup to fail	application
 bulkio.backup.read_with_priority_after	duration	1m0s	amount of time since the read-as-of time above which a BACKUP should use priority when retrying reads	application

docs/generated/settings/settings.html

@@ -9,7 +9,6 @@
 <tr><td><div id="setting-admission-kv-enabled" class="anchored"><code>admission.kv.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>when true, work performed by the KV layer is subject to admission control</td><td>Advanced/Self-Hosted</td></tr>
 <tr><td><div id="setting-admission-sql-kv-response-enabled" class="anchored"><code>admission.sql_kv_response.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>when true, work performed by the SQL layer when receiving a KV response is subject to admission control</td><td>Basic/Standard/Advanced/Self-Hosted</td></tr>
 <tr><td><div id="setting-admission-sql-sql-response-enabled" class="anchored"><code>admission.sql_sql_response.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>when true, work performed by the SQL layer when receiving a DistSQL response is subject to admission control</td><td>Basic/Standard/Advanced/Self-Hosted</td></tr>
-<tr><td><div id="setting-bulkio-backup-deprecated-full-backup-with-subdir-enabled" class="anchored"><code>bulkio.backup.deprecated_full_backup_with_subdir.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>when true, a backup command with a user specified subdirectory will create a full backup at the subdirectory if no backup already exists at that subdirectory</td><td>Basic/Standard/Advanced/Self-Hosted</td></tr>
 <tr><td><div id="setting-bulkio-backup-file-size" class="anchored"><code>bulkio.backup.file_size</code></div></td><td>byte size</td><td><code>128 MiB</code></td><td>target size for individual data files produced during BACKUP</td><td>Basic/Standard/Advanced/Self-Hosted</td></tr>
 <tr><td><div id="setting-bulkio-backup-read-timeout" class="anchored"><code>bulkio.backup.read_timeout</code></div></td><td>duration</td><td><code>5m0s</code></td><td>amount of time after which a read attempt is considered timed out, which causes the backup to fail</td><td>Basic/Standard/Advanced/Self-Hosted</td></tr>
 <tr><td><div id="setting-bulkio-backup-read-with-priority-after" class="anchored"><code>bulkio.backup.read_with_priority_after</code></div></td><td>duration</td><td><code>1m0s</code></td><td>amount of time since the read-as-of time above which a BACKUP should use priority when retrying reads</td><td>Basic/Standard/Advanced/Self-Hosted</td></tr>

pkg/backup/backup_test.go

@@ -730,7 +730,7 @@ func TestBackupAndRestoreJobDescription(t *testing.T) {
 	sqlDB.ExpectErr(
 		t,
 		fmt.Sprintf(
-			`No full backup exists in "%s" to append an incremental backup to. To take a full backup, remove the subdirectory from the backup command`,
+			`No full backup exists in "%s" to append an incremental backup to`,
 			nonExistentFullDir,
 		),
 		"BACKUP INTO $4 IN ($1, $2, $3)", append(collections, nonExistentFullDir)...,
@@ -3518,8 +3518,7 @@ func TestBackupAsOfSystemTime(t *testing.T) {
 				equalDir,
 				beforeTs))
 
-		sqlDB.ExpectErr(t, "A full backup already exists in .* Consider running an incremental backup"+
-			" to this full backup via `BACKUP INTO ",
+		sqlDB.ExpectErr(t, "pq: a full backup already exists in .*",
 			fmt.Sprintf(`BACKUP DATABASE data INTO '%s' AS OF SYSTEM TIME %s`, equalDir,
 				equalTs))
 	}

pkg/backup/backupdest/backup_destination.go

@@ -24,7 +24,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
-	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
@@ -55,17 +54,6 @@ const (
 // will contain one.
 var backupPathRE = regexp.MustCompile("^/?[^\\/]+/[^\\/]+/[^\\/]+/" + backupbase.DeprecatedBackupManifestName + "$")
 
-// featureFullBackupUserSubdir, when true, will create a full backup at a user
-// specified subdirectory if no backup already exists at that subdirectory. As
-// of 22.1, this feature is default disabled, and will be totally disabled by 22.2.
-var featureFullBackupUserSubdir = settings.RegisterBoolSetting(
-	settings.ApplicationLevel,
-	"bulkio.backup.deprecated_full_backup_with_subdir.enabled",
-	"when true, a backup command with a user specified subdirectory will create a full backup at"+
-		" the subdirectory if no backup already exists at that subdirectory",
-	false,
-	settings.WithPublic)
-
 // TODO(adityamaru): Move this to the soon to be `backupinfo` package.
 func containsManifest(ctx context.Context, exportStore cloud.ExternalStorage) (bool, error) {
 	r, _, err := exportStore.ReadFile(ctx, backupbase.DeprecatedBackupManifestName, cloud.ReadOptions{NoFileSize: true})
@@ -165,30 +153,13 @@ func ResolveDest(
 		return ResolvedDestination{}, err
 	}
 	if exists && !dest.Exists {
-		// We disallow a user from writing a full backup to a path in a collection containing an
-		// existing backup iff we're 99.9% confident this backup was planned on a 22.1 node.
 		return ResolvedDestination{},
-			errors.Newf("A full backup already exists in %s. "+
-				"Consider running an incremental backup to this full backup via `BACKUP INTO '%s' IN '%s'`",
-				plannedBackupDefaultURI, chosenSuffix, dest.To[0])
+			errors.Newf("a full backup already exists in %s", plannedBackupDefaultURI)
 
 	} else if !exists {
 		if dest.Exists {
-			// Implies the user passed a subdirectory in their backup command, either
-			// explicitly or using LATEST; however, we could not find an existing
-			// backup in that subdirectory.
-			// - Pre 22.1: this was fine. we created a full backup in their specified subdirectory.
-			// - 22.1: throw an error: full backups with an explicit subdirectory are deprecated.
-			// User can use old behavior by switching the 'bulkio.backup.full_backup_with_subdir.
-			// enabled' to true.
-			// - 22.2+: the backup will fail unconditionally.
-			// TODO (msbutler): throw error in 22.2
-			if !featureFullBackupUserSubdir.Get(execCfg.SV()) {
-				return ResolvedDestination{},
-					errors.Errorf("No full backup exists in %q to append an incremental backup to. "+
-						"To take a full backup, remove the subdirectory from the backup command "+
-						"(i.e. run 'BACKUP ... INTO <collectionURI>'). ", chosenSuffix)
-			}
+			return ResolvedDestination{},
+				errors.Errorf("No full backup exists in %q to append an incremental backup to", chosenSuffix)
 		}
 		// There's no full backup in the resolved subdirectory; therefore, we're conducting a full backup.
 		return ResolvedDestination{

pkg/cloud/externalconn/record.go

@@ -355,20 +355,26 @@ func (e *MutableExternalConnection) Create(ctx context.Context, txn isql.Txn) er
 }
 
 func (e *MutableExternalConnection) Update(ctx context.Context, txn isql.Txn) error {
-	cols, qargs, err := e.marshalChanges()
+	cols, qargsForChanges, err := e.marshalChanges()
 	if err != nil {
 		return err
 	}
 
+	qargs := make([]interface{}, 0, 1+len(qargsForChanges))
+	qargs = append(qargs, e.ConnectionName())
+	qargs = append(qargs, qargsForChanges...)
+
 	var setClauses []string
-	for i, col := range cols {
-		setClauses = append(setClauses, fmt.Sprintf("%s = $%d", col, i+1))
+	placeHolder := 2
+	for _, col := range cols {
+		setClauses = append(setClauses, fmt.Sprintf("%s = $%d", col, placeHolder))
+		placeHolder++
 	}
 
 	updateQuery := fmt.Sprintf(`UPDATE system.external_connections 
 															SET %s, updated = now() 
-		                          WHERE connection_name = '%s'`,
-		strings.Join(setClauses, ", "), e.ConnectionName())
+		                          WHERE connection_name = $1`,
+		strings.Join(setClauses, ", "))
 
 	_, err = txn.ExecEx(ctx, "ExternalConnection.Update", txn.KV(), sessiondata.NodeUserSessionDataOverride, updateQuery, qargs...)
 	return err

pkg/roachprod/promhelperclient/client.go

@@ -113,8 +113,8 @@ func NewPromClient(options ...Option) (*PromClient, error) {
 		option.apply(c)
 	}
 
-	// If the client is not set, create a new client
-	if c.httpClient == nil {
+	// If the client is not set and not disabled, create a new client
+	if c.httpClient == nil && !c.disabled {
 		iapTokenSource, err := roachprodutil.NewIAPTokenSource(roachprodutil.IAPTokenSourceOptions{
 			OAuthClientID:       OAuthClientID,
 			ServiceAccountEmail: ServiceAccountEmail,