Merge https://github.com/vmware-tanzu/velero:main (a1026cb) into oadp-dev

.github/workflows/stale-issues.yml

@@ -7,7 +7,7 @@ jobs:
   stale:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/[email protected].0
+      - uses: actions/[email protected].1
         with:
           repo-token: ${{ secrets.GITHUB_TOKEN }}
           stale-issue-message: "This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. If a Velero team member has requested log or more information, please provide the output of the shared commands."

changelogs/unreleased/9132-mjnagel

@@ -0,0 +1 @@
+Add `--apply` flag to `install` command, allowing usage of Kubernetes apply to make changes to existing installs

changelogs/unreleased/9321-shubham-pampattiwar

@@ -0,0 +1 @@
+Sanitize Azure HTTP responses in BSL status messages

changelogs/unreleased/9431-blackpiglet

@@ -0,0 +1 @@
+Remove VolumeSnapshotClass from CSI B/R process.

design/Implemented/apply-flag.md

@@ -0,0 +1,70 @@
+# Apply flag for install command
+
+## Abstract
+Add an `--apply` flag to the install command that enables applying existing resources rather than creating them. This can be useful as part of the upgrade process for existing installations.
+
+## Background
+The current Velero install command creates resources but doesn't provide a direct way to apply updates to an existing installation.
+Users attempting to run the install command on an existing installation receive "already exists" messages.
+Upgrade steps for existing installs typically involve a three (or more) step process to apply updated CRDs (using `--dry-run` and piping to `kubectl apply`) and then updating/setting images on the Velero deployment and node-agent.
+
+## Goals
+- Provide a simple flag to enable applying resources on an existing Velero installation.
+- Use server-side apply to update existing resources rather than attempting to create them.
+- Maintain consistency with the regular install flow.
+
+## Non Goals
+- Implement special logic for specific version-to-version upgrades (i.e. resource deletion, etc).
+- Add complex upgrade validation or pre/post-upgrade hooks.
+- Provide rollback capabilities.
+
+## High-Level Design
+The `--apply` flag will be added to the Velero install command.
+When this flag is set, the installation process will use server-side apply to update existing resources instead of using create on new resources.
+This flag can be used as _part_ of the upgrade process, but will not always fully handle an upgrade.
+
+## Detailed Design
+The implementation adds a new boolean flag `--apply` to the install command.
+This flag will be passed through to the underlying install functions where the resource creation logic resides.
+
+When the flag is set to true:
+- The `createOrApplyResource` function will use server-side apply with field manager "velero-cli" and `force=true` to update resources.
+- Resources will be applied in the same order as they would be created during installation.
+- Custom Resource Definitions will still be processed first, and the system will wait for them to be established before continuing.
+
+The server-side apply approach with `force=true` ensures that resources are updated even if there are conflicts with the last applied state.
+This provides a best-effort mechanism to apply resources that follows the same flow as installation but updates resources instead of creating them.
+
+No special handling is added for specific versions or resource structures, making this a general-purpose mechanism for applying resources.
+
+## Alternatives Considered
+1. Creating a separate `upgrade` command that would duplicate much of the install command logic.
+   - Rejected due to code duplication and maintenance overhead.
+
+2. Implementing version-specific upgrade logic to handle breaking changes between versions.
+   - Rejected as overly complex and difficult to maintain across multiple version paths.
+   - This could be considered again in the future, but is not in the scope of the current design.
+
+3. Adding automatic detection of existing resources and switching to apply mode.
+   - Rejected as it could lead to unexpected behavior and confusion if users unintentionally apply changes to existing resources.
+
+## Security Considerations
+The apply flag maintains the same security profile as the install command.
+No additional permissions are required beyond what is needed for resource creation.
+The use of `force=true` with server-side apply could potentially override manual changes made to resources, but this is a necessary trade-off to ensure apply is successful.
+
+## Compatibility
+This enhancement is compatible with all existing Velero installations as it is a new opt-in flag.
+It does not change any resource formats or API contracts.
+The apply process is best-effort and does not guarantee compatibility between arbitrary versions of Velero.
+Users should still consult release notes for any breaking changes that may require manual intervention.
+This flag could be adopted by the helm chart, specifically for CRD updates, to simplify the CRD update job.
+
+## Implementation
+The implementation involves:
+1. Adding support for `Apply` to the existing Kubernetes client code.
+1. Adding the `--apply` flag to the install command options.
+1. Changing `createResource` to `createOrApplyResource` and updating it to use server-side apply when the `apply` boolean is set.
+
+The implementation is straightforward and follows existing code patterns.
+No migration of state or special handling of specific resources is required.

hack/build-image/Dockerfile

@@ -94,7 +94,7 @@ RUN ARCH=$(go env GOARCH) && \
     chmod +x /usr/bin/goreleaser
 
 # get golangci-lint
-RUN curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v2.5.0
+RUN curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh | sh -s -- -b $(go env GOPATH)/bin v2.5.0
 
 # install kubectl
 RUN curl -LO https://storage.googleapis.com/kubernetes-release/release/$(curl -s https://storage.googleapis.com/kubernetes-release/release/stable.txt)/bin/linux/$(go env GOARCH)/kubectl

internal/delete/actions/csi/volumesnapshotcontent_action.go

@@ -103,6 +103,14 @@ func (p *volumeSnapshotContentDeleteItemAction) Execute(
 
 	snapCont.ResourceVersion = ""
 
+	if snapCont.Spec.VolumeSnapshotClassName != nil {
+		// Delete VolumeSnapshotClass from the VolumeSnapshotContent.
+		// This is necessary to make the deletion independent of the VolumeSnapshotClass.
+		snapCont.Spec.VolumeSnapshotClassName = nil
+		p.log.Debugf("Deleted VolumeSnapshotClassName from VolumeSnapshotContent %s to make deletion independent of VolumeSnapshotClass",
+			snapCont.Name)
+	}
+
 	if err := p.crClient.Create(context.TODO(), &snapCont); err != nil {
 		return errors.Wrapf(err, "fail to create VolumeSnapshotContent %s", snapCont.Name)
 	}

internal/delete/actions/csi/volumesnapshotcontent_action_test.go

@@ -70,7 +70,7 @@ func TestVSCExecute(t *testing.T) {
 		},
 		{
 			name:      "Normal case, VolumeSnapshot should be deleted",
-			vsc:       builder.ForVolumeSnapshotContent("bar").ObjectMeta(builder.WithLabelsMap(map[string]string{velerov1api.BackupNameLabel: "backup"})).Status(&snapshotv1api.VolumeSnapshotContentStatus{SnapshotHandle: &snapshotHandleStr}).Result(),
+			vsc:       builder.ForVolumeSnapshotContent("bar").ObjectMeta(builder.WithLabelsMap(map[string]string{velerov1api.BackupNameLabel: "backup"})).VolumeSnapshotClassName("volumesnapshotclass").Status(&snapshotv1api.VolumeSnapshotContentStatus{SnapshotHandle: &snapshotHandleStr}).Result(),
 			backup:    builder.ForBackup("velero", "backup").ObjectMeta(builder.WithAnnotationsMap(map[string]string{velerov1api.ResourceTimeoutAnnotation: "5s"})).Result(),
 			expectErr: false,
 			function: func(
@@ -82,7 +82,7 @@ func TestVSCExecute(t *testing.T) {
 			},
 		},
 		{
-			name:      "Normal case, VolumeSnapshot should be deleted",
+			name:      "Error case, deletion fails",
 			vsc:       builder.ForVolumeSnapshotContent("bar").ObjectMeta(builder.WithLabelsMap(map[string]string{velerov1api.BackupNameLabel: "backup"})).Status(&snapshotv1api.VolumeSnapshotContentStatus{SnapshotHandle: &snapshotHandleStr}).Result(),
 			backup:    builder.ForBackup("velero", "backup").ObjectMeta(builder.WithAnnotationsMap(map[string]string{velerov1api.ResourceTimeoutAnnotation: "5s"})).Result(),
 			expectErr: true,

pkg/backup/actions/csi/volumesnapshot_action.go

@@ -84,25 +84,32 @@ func (p *volumeSnapshotBackupItemAction) Execute(
 		return nil, nil, "", nil, errors.WithStack(err)
 	}
 
+	if backup.Status.Phase == velerov1api.BackupPhaseFinalizing ||
+		backup.Status.Phase == velerov1api.BackupPhaseFinalizingPartiallyFailed {
+		p.log.
+			WithField("Backup", fmt.Sprintf("%s/%s", backup.Namespace, backup.Name)).
+			WithField("BackupPhase", backup.Status.Phase).Debugf("Cleaning VolumeSnapshots.")
+
+		csi.DeleteReadyVolumeSnapshot(*vs, p.crClient, p.log)
+		return item, nil, "", nil, nil
+	}
+
 	additionalItems := make([]velero.ResourceIdentifier, 0)
+
 	if vs.Spec.VolumeSnapshotClassName != nil {
+		// This is still needed to add the VolumeSnapshotClass to the backup.
+		// The secret with VolumeSnapshotClass is still relevant to backup.
 		additionalItems = append(
 			additionalItems,
 			velero.ResourceIdentifier{
 				GroupResource: kuberesource.VolumeSnapshotClasses,
 				Name:          *vs.Spec.VolumeSnapshotClassName,
 			},
 		)
-	}
-
-	if backup.Status.Phase == velerov1api.BackupPhaseFinalizing ||
-		backup.Status.Phase == velerov1api.BackupPhaseFinalizingPartiallyFailed {
-		p.log.
-			WithField("Backup", fmt.Sprintf("%s/%s", backup.Namespace, backup.Name)).
-			WithField("BackupPhase", backup.Status.Phase).Debugf("Cleaning VolumeSnapshots.")
 
-		csi.DeleteReadyVolumeSnapshot(*vs, p.crClient, p.log)
-		return item, nil, "", nil, nil
+		// Because async operation will update VolumeSnapshot during finalizing phase.
+		// No matter what we do, VolumeSnapshotClass cannot be deleted. So skip it.
+		// Just deleting VolumeSnapshotClass during restore and delete is enough.
 	}
 
 	p.log.Infof("Getting VolumesnapshotContent for Volumesnapshot %s/%s",

pkg/backup/actions/csi/volumesnapshotcontent_action.go

@@ -97,6 +97,10 @@ func (p *volumeSnapshotContentBackupItemAction) Execute(
 		})
 	}
 
+	// Because async operation will update VolumeSnapshotContent during finalizing phase.
+	// No matter what we do, VolumeSnapshotClass cannot be deleted. So skip it.
+	// Just deleting VolumeSnapshotClass during restore and delete is enough.
+
 	snapContMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&snapCont)
 	if err != nil {
 		return nil, nil, "", nil, errors.WithStack(err)

pkg/backup/actions/csi/volumesnapshotcontent_action_test.go

@@ -42,7 +42,7 @@ func TestVSCExecute(t *testing.T) {
 		expectedItems []velero.ResourceIdentifier
 	}{
 		{
-			name: "Invalid VolumeSnapshotClass",
+			name: "Invalid VolumeSnapshotContent",
 			item: velerotest.UnstructuredOrDie(
 				`
 				{

pkg/client/dynamic.go

@@ -102,6 +102,11 @@ type StatusUpdater interface {
 	UpdateStatus(obj *unstructured.Unstructured, opts metav1.UpdateOptions) (*unstructured.Unstructured, error)
 }
 
+// Applier applies changes to an object using server-side apply
+type Applier interface {
+	Apply(name string, obj *unstructured.Unstructured, opts metav1.ApplyOptions) (*unstructured.Unstructured, error)
+}
+
 // Dynamic contains client methods that Velero needs for backing up and restoring resources.
 type Dynamic interface {
 	Creator
@@ -111,6 +116,7 @@ type Dynamic interface {
 	Patcher
 	Deletor
 	StatusUpdater
+	Applier
 }
 
 // dynamicResourceClient implements Dynamic.
@@ -136,6 +142,10 @@ func (d *dynamicResourceClient) Get(name string, opts metav1.GetOptions) (*unstr
 	return d.resourceClient.Get(context.TODO(), name, opts)
 }
 
+func (d *dynamicResourceClient) Apply(name string, obj *unstructured.Unstructured, opts metav1.ApplyOptions) (*unstructured.Unstructured, error) {
+	return d.resourceClient.Apply(context.TODO(), name, obj, opts)
+}
+
 func (d *dynamicResourceClient) Patch(name string, data []byte) (*unstructured.Unstructured, error) {
 	return d.resourceClient.Patch(context.TODO(), name, types.MergePatchType, data, metav1.PatchOptions{})
 }

pkg/cmd/cli/install/install.go

@@ -92,6 +92,7 @@ type Options struct {
 	ConcurrentBackups               int
 	NodeAgentDisableHostPath        bool
 	kubeletRootDir                  string
+	Apply                           bool
 	ServerPriorityClassName         string
 	NodeAgentPriorityClassName      string
 }
@@ -102,6 +103,7 @@ func (o *Options) BindFlags(flags *pflag.FlagSet) {
 	flags.StringVar(&o.BucketName, "bucket", o.BucketName, "Name of the object storage bucket where backups should be stored")
 	flags.StringVar(&o.SecretFile, "secret-file", o.SecretFile, "File containing credentials for backup and volume provider. If not specified, --no-secret must be used for confirmation. Optional.")
 	flags.BoolVar(&o.NoSecret, "no-secret", o.NoSecret, "Flag indicating if a secret should be created. Must be used as confirmation if --secret-file is not provided. Optional.")
+	flags.BoolVar(&o.Apply, "apply", o.Apply, "Flag indicating if resources should be applied instead of created. This can be used for updating existing resources.")
 	flags.BoolVar(&o.NoDefaultBackupLocation, "no-default-backup-location", o.NoDefaultBackupLocation, "Flag indicating if a default backup location should be created. Must be used as confirmation if --bucket or --provider are not provided. Optional.")
 	flags.StringVar(&o.Image, "image", o.Image, "Image to use for the Velero and node agent pods. Optional.")
 	flags.StringVar(&o.Prefix, "prefix", o.Prefix, "Prefix under which all Velero data should be stored within the bucket. Optional.")
@@ -416,7 +418,7 @@ func (o *Options) Run(c *cobra.Command, f client.Factory) error {
 
 	errorMsg := fmt.Sprintf("\n\nError installing Velero. Use `kubectl logs deploy/velero -n %s` to check the deploy logs", o.Namespace)
 
-	err = install.Install(dynamicFactory, kbClient, resources, os.Stdout)
+	err = install.Install(dynamicFactory, kbClient, resources, os.Stdout, o.Apply)
 	if err != nil {
 		return errors.Wrap(err, errorMsg)
 	}

pkg/controller/backup_storage_location_controller.go

@@ -18,6 +18,7 @@ package controller
 
 import (
 	"context"
+	"regexp"
 	"strings"
 	"time"
 
@@ -46,6 +47,104 @@ const (
 	bslValidationEnqueuePeriod = 10 * time.Second
 )
 
+// sanitizeStorageError cleans up verbose HTTP responses from cloud provider errors,
+// particularly Azure which includes full HTTP response details and XML in error messages.
+// It extracts the error code and message while removing HTTP headers and response bodies.
+// It also scrubs sensitive information like SAS tokens from URLs.
+func sanitizeStorageError(err error) string {
+	if err == nil {
+		return ""
+	}
+
+	errMsg := err.Error()
+
+	// Scrub sensitive information from URLs (SAS tokens, credentials, etc.)
+	// Azure SAS token parameters: sig, se, st, sp, spr, sv, sr, sip, srt, ss
+	// These appear as query parameters in URLs like: ?sig=value&se=value
+	sasParamsRegex := regexp.MustCompile(`([?&])(sig|se|st|sp|spr|sv|sr|sip|srt|ss)=([^&\s<>\n]+)`)
+	errMsg = sasParamsRegex.ReplaceAllString(errMsg, `${1}${2}=***REDACTED***`)
+
+	// Check if this looks like an Azure HTTP response error
+	// Azure errors contain patterns like "RESPONSE 404:" and "ERROR CODE:"
+	if !strings.Contains(errMsg, "RESPONSE") || !strings.Contains(errMsg, "ERROR CODE:") {
+		// Not an Azure-style error, return as-is
+		return errMsg
+	}
+
+	// Extract the error code (e.g., "ContainerNotFound", "BlobNotFound")
+	errorCodeRegex := regexp.MustCompile(`ERROR CODE:\s*(\w+)`)
+	errorCodeMatch := errorCodeRegex.FindStringSubmatch(errMsg)
+	var errorCode string
+	if len(errorCodeMatch) > 1 {
+		errorCode = errorCodeMatch[1]
+	}
+
+	// Extract the error message from the XML or plain text
+	// Look for message between <Message> tags or after "RESPONSE XXX:"
+	var errorMessage string
+
+	// Try to extract from XML first
+	messageRegex := regexp.MustCompile(`<Message>(.*?)</Message>`)
+	messageMatch := messageRegex.FindStringSubmatch(errMsg)
+	if len(messageMatch) > 1 {
+		errorMessage = messageMatch[1]
+		// Remove RequestId and Time from the message
+		if idx := strings.Index(errorMessage, "\nRequestId:"); idx != -1 {
+			errorMessage = errorMessage[:idx]
+		}
+	} else {
+		// Try to extract from plain text response (e.g., "RESPONSE 404: 404 The specified container does not exist.")
+		responseRegex := regexp.MustCompile(`RESPONSE\s+\d+:\s+\d+\s+([^\n]+)`)
+		responseMatch := responseRegex.FindStringSubmatch(errMsg)
+		if len(responseMatch) > 1 {
+			errorMessage = strings.TrimSpace(responseMatch[1])
+		}
+	}
+
+	// Build a clean error message
+	var cleanMsg string
+	if errorCode != "" && errorMessage != "" {
+		cleanMsg = errorCode + ": " + errorMessage
+	} else if errorCode != "" {
+		cleanMsg = errorCode
+	} else if errorMessage != "" {
+		cleanMsg = errorMessage
+	} else {
+		// Fallback: try to extract the desc part from gRPC error
+		descRegex := regexp.MustCompile(`desc\s*=\s*(.+)`)
+		descMatch := descRegex.FindStringSubmatch(errMsg)
+		if len(descMatch) > 1 {
+			// Take everything up to the first newline or "RESPONSE" marker
+			desc := descMatch[1]
+			if idx := strings.Index(desc, "\n"); idx != -1 {
+				desc = desc[:idx]
+			}
+			if idx := strings.Index(desc, "RESPONSE"); idx != -1 {
+				desc = strings.TrimSpace(desc[:idx])
+			}
+			cleanMsg = desc
+		} else {
+			// Last resort: return first line
+			if idx := strings.Index(errMsg, "\n"); idx != -1 {
+				cleanMsg = errMsg[:idx]
+			} else {
+				cleanMsg = errMsg
+			}
+		}
+	}
+
+	// Preserve the prefix part of the error (e.g., "rpc error: code = Unknown desc = ")
+	// but replace the verbose description with our clean message
+	if strings.Contains(errMsg, "desc = ") {
+		parts := strings.SplitN(errMsg, "desc = ", 2)
+		if len(parts) == 2 {
+			return parts[0] + "desc = " + cleanMsg
+		}
+	}
+
+	return cleanMsg
+}
+
 // BackupStorageLocationReconciler reconciles a BackupStorageLocation object
 type backupStorageLocationReconciler struct {
 	ctx                       context.Context
@@ -125,9 +224,9 @@ func (r *backupStorageLocationReconciler) Reconcile(ctx context.Context, req ctr
 			if err != nil {
 				log.Info("BackupStorageLocation is invalid, marking as unavailable")
 				err = errors.Wrapf(err, "BackupStorageLocation %q is unavailable", location.Name)
-				unavailableErrors = append(unavailableErrors, err.Error())
+				unavailableErrors = append(unavailableErrors, sanitizeStorageError(err))
 				location.Status.Phase = velerov1api.BackupStorageLocationPhaseUnavailable
-				location.Status.Message = err.Error()
+				location.Status.Message = sanitizeStorageError(err)
 			} else {
 				log.Info("BackupStorageLocations is valid, marking as available")
 				location.Status.Phase = velerov1api.BackupStorageLocationPhaseAvailable