include additional volume policy actions in validation

Don't validate volume policies from plugin calls, since
those have alreay been validated for the backup.

Signed-off-by: Scott Seago <sseago@redhat.com>

Author: sseago

changelogs/unreleased/9506-sseago

@@ -0,0 +1 @@
+Add installer/server arg to allow custom volume policy actions

internal/resourcepolicies/resource_policies.go

@@ -101,9 +101,10 @@ type ResourcePolicies struct {
 }
 
 type Policies struct {
-	version              string
-	volumePolicies       []volPolicy
-	includeExcludePolicy *IncludeExcludePolicy
+	version                       string
+	volumePolicies                []volPolicy
+	additionalVolumePolicyActions []string
+	includeExcludePolicy          *IncludeExcludePolicy
 	// OtherPolicies
 }
 
@@ -210,7 +211,7 @@ func (p *Policies) Validate() error {
 	}
 
 	for _, policy := range p.volumePolicies {
-		if err := policy.action.validate(); err != nil {
+		if err := policy.action.validate(p.additionalVolumePolicyActions); err != nil {
 			return errors.WithStack(err)
 		}
 		for _, con := range policy.conditions {
@@ -237,6 +238,8 @@ func GetResourcePoliciesFromBackup(
 	backup velerov1api.Backup,
 	client crclient.Client,
 	logger logrus.FieldLogger,
+	additionalVolumePolicyActions []string,
+	validate bool,
 ) (resourcePolicies *Policies, err error) {
 	if backup.Spec.ResourcePolicy != nil &&
 		strings.EqualFold(backup.Spec.ResourcePolicy.Kind, ConfigmapRefType) {
@@ -258,11 +261,14 @@ func GetResourcePoliciesFromBackup(
 				backup.Namespace+"/"+backup.Name, err.Error())
 			return nil, fmt.Errorf("fail to read the ResourcePolicies from ConfigMap %s with error %s",
 				backup.Namespace+"/"+backup.Name, err.Error())
-		} else if err = resourcePolicies.Validate(); err != nil {
-			logger.Errorf("Fail to validate ResourcePolicies in ConfigMap %s with error %s.",
-				backup.Namespace+"/"+backup.Name, err.Error())
-			return nil, fmt.Errorf("fail to validate ResourcePolicies in ConfigMap %s with error %s",
-				backup.Namespace+"/"+backup.Name, err.Error())
+		} else if validate {
+			resourcePolicies.additionalVolumePolicyActions = additionalVolumePolicyActions
+			if err = resourcePolicies.Validate(); err != nil {
+				logger.Errorf("Fail to validate ResourcePolicies in ConfigMap %s with error %s.",
+					backup.Namespace+"/"+backup.Name, err.Error())
+				return nil, fmt.Errorf("fail to validate ResourcePolicies in ConfigMap %s with error %s",
+					backup.Namespace+"/"+backup.Name, err.Error())
+			}
 		}
 	}
 

internal/resourcepolicies/volume_resources_validator.go

@@ -18,6 +18,7 @@ package resourcepolicies
 import (
 	"fmt"
 	"io"
+	"slices"
 
 	"github.com/pkg/errors"
 	"gopkg.in/yaml.v3"
@@ -87,12 +88,15 @@ func decodeStruct(r io.Reader, s any) error {
 }
 
 // validate check action format
-func (a *Action) validate() error {
+func (a *Action) validate(additionalVolumePolicyActions []string) error {
 	// validate Type
 	valid := false
 	if a.Type == Skip || a.Type == Snapshot || a.Type == FSBackup {
 		valid = true
+	} else if slices.Contains(additionalVolumePolicyActions, string(a.Type)) {
+		valid = true
 	}
+
 	if !valid {
 		return fmt.Errorf("invalid action type %s", a.Type)
 	}

internal/resourcepolicies/volume_resources_validator_test.go

@@ -83,9 +83,10 @@ func TestCapacityConditionValidate(t *testing.T) {
 
 func TestValidate(t *testing.T) {
 	testCases := []struct {
-		name    string
-		res     *ResourcePolicies
-		wantErr bool
+		name              string
+		res               *ResourcePolicies
+		additionalActions []string
+		wantErr           bool
 	}{
 		{
 			name: "unknown key in yaml",
@@ -244,6 +245,66 @@ func TestValidate(t *testing.T) {
 			},
 			wantErr: true,
 		},
+		{
+			name: "supported action with additional actions defined",
+			res: &ResourcePolicies{
+				Version: "v1",
+				VolumePolicies: []VolumePolicy{
+					{
+						Action: Action{Type: "skip"},
+						Conditions: map[string]any{
+							"capacity": "0,10Gi",
+							"csi": any(
+								map[string]any{
+									"driver": "aws.efs.csi.driver",
+								}),
+						},
+					},
+				},
+			},
+			additionalActions: []string{"custom1", "custom2"},
+			wantErr:           false,
+		},
+		{
+			name: "custom action with additional actions defined",
+			res: &ResourcePolicies{
+				Version: "v1",
+				VolumePolicies: []VolumePolicy{
+					{
+						Action: Action{Type: "custom1"},
+						Conditions: map[string]any{
+							"capacity": "0,10Gi",
+							"csi": any(
+								map[string]any{
+									"driver": "aws.efs.csi.driver",
+								}),
+						},
+					},
+				},
+			},
+			additionalActions: []string{"custom1", "custom2"},
+			wantErr:           false,
+		},
+		{
+			name: "unsupported action with additional actions defined",
+			res: &ResourcePolicies{
+				Version: "v1",
+				VolumePolicies: []VolumePolicy{
+					{
+						Action: Action{Type: "unsupported"},
+						Conditions: map[string]any{
+							"capacity": "0,10Gi",
+							"csi": any(
+								map[string]any{
+									"driver": "aws.efs.csi.driver",
+								}),
+						},
+					},
+				},
+			},
+			additionalActions: []string{"custom1", "custom2"},
+			wantErr:           true,
+		},
 		{
 			name: "error format of nfs",
 			res: &ResourcePolicies{
@@ -554,6 +615,9 @@ func TestValidate(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			policies := &Policies{}
 			err1 := policies.BuildPolicy(tc.res)
+			if err1 == nil && tc.additionalActions != nil {
+				policies.additionalVolumePolicyActions = tc.additionalActions
+			}
 			err2 := policies.Validate()
 
 			if tc.wantErr {

internal/volumehelper/volume_policy_helper.go

@@ -111,7 +111,9 @@ func NewVolumeHelperImplWithCache(
 	logger logrus.FieldLogger,
 	pvcPodCache *podvolumeutil.PVCPodCache,
 ) (VolumeHelper, error) {
-	resourcePolicies, err := resourcepolicies.GetResourcePoliciesFromBackup(backup, client, logger)
+	// no need to validate resource policies here since this is called from the plugin. They're already
+	// validated at the backup level.
+	resourcePolicies, err := resourcepolicies.GetResourcePoliciesFromBackup(backup, client, logger, nil, false)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to get volume policies from backup")
 	}

pkg/cmd/cli/install/install.go

@@ -244,14 +244,14 @@ func NewInstallOptions() *Options {
 		NodeAgentPodCPULimit:      install.DefaultNodeAgentPodCPULimit,
 		NodeAgentPodMemLimit:      install.DefaultNodeAgentPodMemLimit,
 		// Default to creating a VSL unless we're told otherwise
-		UseVolumeSnapshots:       true,
-		NoDefaultBackupLocation:  false,
-		CRDsOnly:                 false,
-		DefaultVolumesToFsBackup: false,
-		UploaderType:             uploader.KopiaType,
-		DefaultSnapshotMoveData:  false,
-		DisableInformerCache:     false,
-		ScheduleSkipImmediately:  false,
+		UseVolumeSnapshots:            true,
+		NoDefaultBackupLocation:       false,
+		CRDsOnly:                      false,
+		DefaultVolumesToFsBackup:      false,
+		UploaderType:                  uploader.KopiaType,
+		DefaultSnapshotMoveData:       false,
+		DisableInformerCache:          false,
+		ScheduleSkipImmediately:       false,
 		kubeletRootDir:                install.DefaultKubeletRootDir,
 		NodeAgentDisableHostPath:      false,
 		AdditionalVolumePolicyActions: flag.NewStringArray(),

pkg/controller/backup_controller.go

@@ -84,32 +84,32 @@ var autoExcludeClusterScopedResources = []string{
 }
 
 type backupReconciler struct {
-	ctx                         context.Context
-	logger                      logrus.FieldLogger
-	discoveryHelper             discovery.Helper
-	backupper                   pkgbackup.Backupper
-	kbClient                    kbclient.Client
-	clock                       clock.WithTickerAndDelayedExecution
-	backupLogLevel              logrus.Level
-	newPluginManager            func(logrus.FieldLogger) clientmgmt.Manager
-	backupTracker               BackupTracker
-	defaultBackupLocation       string
-	defaultVolumesToFsBackup    bool
-	defaultBackupTTL            time.Duration
-	defaultVGSLabelKey          string
-	defaultCSISnapshotTimeout   time.Duration
-	resourceTimeout             time.Duration
-	defaultItemOperationTimeout time.Duration
-	defaultSnapshotLocations    map[string]string
-	metrics                     *metrics.ServerMetrics
-	backupStoreGetter           persistence.ObjectBackupStoreGetter
-	formatFlag                  logging.Format
-	credentialFileStore         credentials.FileStore
-	maxConcurrentK8SConnections int
-	defaultSnapshotMoveData     bool
-	globalCRClient              kbclient.Client
-	itemBlockWorkerCount        int
-	concurrentBackups           int
+	ctx                           context.Context
+	logger                        logrus.FieldLogger
+	discoveryHelper               discovery.Helper
+	backupper                     pkgbackup.Backupper
+	kbClient                      kbclient.Client
+	clock                         clock.WithTickerAndDelayedExecution
+	backupLogLevel                logrus.Level
+	newPluginManager              func(logrus.FieldLogger) clientmgmt.Manager
+	backupTracker                 BackupTracker
+	defaultBackupLocation         string
+	defaultVolumesToFsBackup      bool
+	defaultBackupTTL              time.Duration
+	defaultVGSLabelKey            string
+	defaultCSISnapshotTimeout     time.Duration
+	resourceTimeout               time.Duration
+	defaultItemOperationTimeout   time.Duration
+	defaultSnapshotLocations      map[string]string
+	metrics                       *metrics.ServerMetrics
+	backupStoreGetter             persistence.ObjectBackupStoreGetter
+	formatFlag                    logging.Format
+	credentialFileStore           credentials.FileStore
+	maxConcurrentK8SConnections   int
+	defaultSnapshotMoveData       bool
+	globalCRClient                kbclient.Client
+	itemBlockWorkerCount          int
+	concurrentBackups             int
 	additionalVolumePolicyActions []string
 }
 
@@ -142,32 +142,32 @@ func NewBackupReconciler(
 	additionalVolumePolicyActions []string,
 ) *backupReconciler {
 	b := &backupReconciler{
-		ctx:                         ctx,
-		discoveryHelper:             discoveryHelper,
-		backupper:                   backupper,
-		clock:                       &clock.RealClock{},
-		logger:                      logger,
-		backupLogLevel:              backupLogLevel,
-		newPluginManager:            newPluginManager,
-		backupTracker:               backupTracker,
-		kbClient:                    kbClient,
-		defaultBackupLocation:       defaultBackupLocation,
-		defaultVolumesToFsBackup:    defaultVolumesToFsBackup,
-		defaultBackupTTL:            defaultBackupTTL,
-		defaultVGSLabelKey:          defaultVGSLabelKey,
-		defaultCSISnapshotTimeout:   defaultCSISnapshotTimeout,
-		resourceTimeout:             resourceTimeout,
-		defaultItemOperationTimeout: defaultItemOperationTimeout,
-		defaultSnapshotLocations:    defaultSnapshotLocations,
-		metrics:                     metrics,
-		backupStoreGetter:           backupStoreGetter,
-		formatFlag:                  formatFlag,
-		credentialFileStore:         credentialStore,
-		maxConcurrentK8SConnections: maxConcurrentK8SConnections,
-		defaultSnapshotMoveData:     defaultSnapshotMoveData,
-		itemBlockWorkerCount:        itemBlockWorkerCount,
-		concurrentBackups:           max(concurrentBackups, 1),
-		globalCRClient:              globalCRClient,
+		ctx:                           ctx,
+		discoveryHelper:               discoveryHelper,
+		backupper:                     backupper,
+		clock:                         &clock.RealClock{},
+		logger:                        logger,
+		backupLogLevel:                backupLogLevel,
+		newPluginManager:              newPluginManager,
+		backupTracker:                 backupTracker,
+		kbClient:                      kbClient,
+		defaultBackupLocation:         defaultBackupLocation,
+		defaultVolumesToFsBackup:      defaultVolumesToFsBackup,
+		defaultBackupTTL:              defaultBackupTTL,
+		defaultVGSLabelKey:            defaultVGSLabelKey,
+		defaultCSISnapshotTimeout:     defaultCSISnapshotTimeout,
+		resourceTimeout:               resourceTimeout,
+		defaultItemOperationTimeout:   defaultItemOperationTimeout,
+		defaultSnapshotLocations:      defaultSnapshotLocations,
+		metrics:                       metrics,
+		backupStoreGetter:             backupStoreGetter,
+		formatFlag:                    formatFlag,
+		credentialFileStore:           credentialStore,
+		maxConcurrentK8SConnections:   maxConcurrentK8SConnections,
+		defaultSnapshotMoveData:       defaultSnapshotMoveData,
+		itemBlockWorkerCount:          itemBlockWorkerCount,
+		concurrentBackups:             max(concurrentBackups, 1),
+		globalCRClient:                globalCRClient,
 		additionalVolumePolicyActions: additionalVolumePolicyActions,
 	}
 	b.updateTotalBackupMetric()
@@ -583,7 +583,7 @@ func (b *backupReconciler) prepareBackupRequest(ctx context.Context, backup *vel
 		request.Status.ValidationErrors = append(request.Status.ValidationErrors, "encountered labelSelector as well as orLabelSelectors in backup spec, only one can be specified")
 	}
 
-	resourcePolicies, err := resourcepolicies.GetResourcePoliciesFromBackup(*request.Backup, b.kbClient, logger)
+	resourcePolicies, err := resourcepolicies.GetResourcePoliciesFromBackup(*request.Backup, b.kbClient, logger, b.additionalVolumePolicyActions, true)
 	if err != nil {
 		request.Status.ValidationErrors = append(request.Status.ValidationErrors, err.Error())
 	}

pkg/plugin/utils/volumehelper/volume_policy_helper.go

@@ -74,10 +74,13 @@ func ShouldPerformSnapshotWithVolumeHelper(
 	}
 
 	// Otherwise, create a new VolumeHelper (original behavior for third-party plugins)
+	// No need to validate policies here as this has already happened for the backup.
 	resourcePolicies, err := resourcepolicies.GetResourcePoliciesFromBackup(
 		backup,
 		crClient,
 		logger,
+		nil,
+		false,
 	)
 	if err != nil {
 		return false, err

site/content/docs/main/resource-filtering.md

@@ -363,6 +363,8 @@ There are three actions supported via the VolumePolicy feature:
 * snapshot: back up the action matching volume's data by the snapshot way.
 * fs-backup: back up the action matching volumes' data by the fs-backup way.
 
+Note that if you are using an external plugin which implements a volume policy that is not one of the above, you need to use the installer/server flag `--additional-volume-policy-actions=custom-action1,custom-action2` to enable validation to work, as Velero's backup validation also validates the volume policy actions.
+
 #### Supported conditions
 
 Currently, Velero supports the volume attributes listed below: