issue 8072: restic deprecation - warning messages

Signed-off-by: Lyndon-Li <[email protected]>

changelogs/unreleased/8096-Lyndon-Li

@@ -0,0 +1 @@
+Fix issue #8072, add the warning messages for restic deprecation

pkg/cmd/cli/install/install.go

@@ -364,8 +364,10 @@ func (o *Options) Validate(c *cobra.Command, args []string, f client.Factory) er
 		return err
 	}
 
-	if err := uploader.ValidateUploaderType(o.UploaderType); err != nil {
+	if msg, err := uploader.ValidateUploaderType(o.UploaderType); err != nil {
 		return err
+	} else if msg != "" {
+		fmt.Printf("⚠️  %s\n", msg)
 	}
 
 	// If we're only installing CRDs, we can skip the rest of the validation.

pkg/cmd/server/server.go

@@ -288,8 +288,10 @@ type server struct {
 }
 
 func newServer(f client.Factory, config serverConfig, logger *logrus.Logger) (*server, error) {
-	if err := uploader.ValidateUploaderType(config.uploaderType); err != nil {
+	if msg, err := uploader.ValidateUploaderType(config.uploaderType); err != nil {
 		return nil, err
+	} else if msg != "" {
+		logger.Warn(msg)
 	}
 
 	if config.clientQPS < 0.0 {

pkg/cmd/server/server_test.go

@@ -203,6 +203,13 @@ func Test_newServer(t *testing.T) {
 	}, logger)
 	assert.Error(t, err)
 
+	// invalid clientQPS Restic uploader
+	_, err = newServer(factory, serverConfig{
+		uploaderType: uploader.ResticType,
+		clientQPS:    -1,
+	}, logger)
+	assert.Error(t, err)
+
 	// invalid clientBurst
 	factory.On("SetClientQPS", mock.Anything).Return()
 	_, err = newServer(factory, serverConfig{

pkg/podvolume/backupper.go

@@ -36,6 +36,7 @@ import (
 	"github.com/vmware-tanzu/velero/pkg/label"
 	"github.com/vmware-tanzu/velero/pkg/nodeagent"
 	"github.com/vmware-tanzu/velero/pkg/repository"
+	"github.com/vmware-tanzu/velero/pkg/uploader"
 	uploaderutil "github.com/vmware-tanzu/velero/pkg/uploader/util"
 	"github.com/vmware-tanzu/velero/pkg/util/boolptr"
 	"github.com/vmware-tanzu/velero/pkg/util/kube"
@@ -163,10 +164,13 @@ func (b *backupper) getMatchAction(resPolicies *resourcepolicies.Policies, pvc *
 	return nil, errors.Errorf("failed to check resource policies for empty volume")
 }
 
+var funcGetRepositoryType = getRepositoryType
+
 func (b *backupper) BackupPodVolumes(backup *velerov1api.Backup, pod *corev1api.Pod, volumesToBackup []string, resPolicies *resourcepolicies.Policies, log logrus.FieldLogger) ([]*velerov1api.PodVolumeBackup, *PVCBackupSummary, []error) {
 	if len(volumesToBackup) == 0 {
 		return nil, nil, nil
 	}
+
 	log.Infof("pod %s/%s has volumes to backup: %v", pod.Namespace, pod.Name, volumesToBackup)
 
 	var (
@@ -189,25 +193,35 @@ func (b *backupper) BackupPodVolumes(backup *velerov1api.Backup, pod *corev1api.
 		}
 	}
 
+	if msg, err := uploader.ValidateUploaderType(b.uploaderType); err != nil {
+		skipAllPodVolumes(pod, volumesToBackup, err, pvcSummary, log)
+		return nil, pvcSummary, []error{err}
+	} else if msg != "" {
+		log.Warn(msg)
+	}
+
 	if err := kube.IsPodRunning(pod); err != nil {
 		skipAllPodVolumes(pod, volumesToBackup, err, pvcSummary, log)
 		return nil, pvcSummary, nil
 	}
 
 	err := nodeagent.IsRunningInNode(b.ctx, backup.Namespace, pod.Spec.NodeName, b.crClient)
 	if err != nil {
-		return nil, nil, []error{err}
+		skipAllPodVolumes(pod, volumesToBackup, err, pvcSummary, log)
+		return nil, pvcSummary, []error{err}
 	}
 
-	repositoryType := getRepositoryType(b.uploaderType)
+	repositoryType := funcGetRepositoryType(b.uploaderType)
 	if repositoryType == "" {
 		err := errors.Errorf("empty repository type, uploader %s", b.uploaderType)
-		return nil, nil, []error{err}
+		skipAllPodVolumes(pod, volumesToBackup, err, pvcSummary, log)
+		return nil, pvcSummary, []error{err}
 	}
 
 	repo, err := b.repoEnsurer.EnsureRepo(b.ctx, backup.Namespace, pod.Namespace, backup.Spec.StorageLocation, repositoryType)
 	if err != nil {
-		return nil, nil, []error{err}
+		skipAllPodVolumes(pod, volumesToBackup, err, pvcSummary, log)
+		return nil, pvcSummary, []error{err}
 	}
 
 	// get a single non-exclusive lock since we'll wait for all individual

pkg/podvolume/backupper_test.go

@@ -309,22 +309,38 @@ func TestBackupPodVolumes(t *testing.T) {
 	corev1api.AddToScheme(scheme)
 
 	tests := []struct {
-		name            string
-		bsl             string
-		uploaderType    string
-		volumes         []string
-		sourcePod       *corev1api.Pod
-		kubeClientObj   []runtime.Object
-		ctlClientObj    []runtime.Object
-		veleroClientObj []runtime.Object
-		veleroReactors  []reactor
-		runtimeScheme   *runtime.Scheme
-		pvbs            int
-		errs            []string
+		name                  string
+		bsl                   string
+		uploaderType          string
+		volumes               []string
+		sourcePod             *corev1api.Pod
+		kubeClientObj         []runtime.Object
+		ctlClientObj          []runtime.Object
+		veleroClientObj       []runtime.Object
+		veleroReactors        []reactor
+		runtimeScheme         *runtime.Scheme
+		pvbs                  int
+		mockGetRepositoryType bool
+		errs                  []string
 	}{
 		{
 			name: "empty volume list",
 		},
+		{
+			name: "wrong uploader type",
+			volumes: []string{
+				"fake-volume-1",
+				"fake-volume-2",
+			},
+			sourcePod: createPodObj(true, false, false, 2),
+			kubeClientObj: []runtime.Object{
+				createNodeAgentPodObj(true),
+			},
+			uploaderType: "fake-uploader-type",
+			errs: []string{
+				"invalid uploader type 'fake-uploader-type', valid upload types are: 'restic', 'kopia'",
+			},
+		},
 		{
 			name: "pod is not running",
 			volumes: []string{
@@ -348,7 +364,8 @@ func TestBackupPodVolumes(t *testing.T) {
 				"fake-volume-1",
 				"fake-volume-2",
 			},
-			sourcePod: createPodObj(true, false, false, 2),
+			sourcePod:    createPodObj(true, false, false, 2),
+			uploaderType: "kopia",
 			errs: []string{
 				"daemonset pod not found in running state in node fake-node-name",
 			},
@@ -363,9 +380,10 @@ func TestBackupPodVolumes(t *testing.T) {
 			kubeClientObj: []runtime.Object{
 				createNodeAgentPodObj(true),
 			},
-			uploaderType: "fake-uploader-type",
+			uploaderType:          "kopia",
+			mockGetRepositoryType: true,
 			errs: []string{
-				"empty repository type, uploader fake-uploader-type",
+				"empty repository type, uploader kopia",
 			},
 		},
 		{
@@ -542,6 +560,12 @@ func TestBackupPodVolumes(t *testing.T) {
 
 			require.NoError(t, err)
 
+			if test.mockGetRepositoryType {
+				funcGetRepositoryType = func(string) string { return "" }
+			} else {
+				funcGetRepositoryType = getRepositoryType
+			}
+
 			pvbs, _, errs := bp.BackupPodVolumes(backupObj, test.sourcePod, test.volumes, nil, velerotest.NewLogger())
 
 			if errs == nil {

pkg/uploader/types.go

@@ -39,12 +39,17 @@ const (
 
 // ValidateUploaderType validates if the input param is a valid uploader type.
 // It will return an error if it's invalid.
-func ValidateUploaderType(t string) error {
+func ValidateUploaderType(t string) (string, error) {
 	t = strings.TrimSpace(t)
 	if t != ResticType && t != KopiaType {
-		return fmt.Errorf("invalid uploader type '%s', valid upload types are: '%s', '%s'", t, ResticType, KopiaType)
+		return "", fmt.Errorf("invalid uploader type '%s', valid upload types are: '%s', '%s'", t, ResticType, KopiaType)
 	}
-	return nil
+
+	if t == ResticType {
+		return fmt.Sprintf("Uploader '%s' is deprecated, don't use it for new backups, otherwise, the backed up data may not be accessed by new versions of Velero", t), nil
+	}
+
+	return "", nil
 }
 
 type SnapshotInfo struct {

pkg/uploader/types_test.go

@@ -1,34 +1,47 @@
 package uploader
 
-import "testing"
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
 
 func TestValidateUploaderType(t *testing.T) {
 	tests := []struct {
 		name    string
 		input   string
-		wantErr bool
+		wantErr string
+		wantMsg string
 	}{
 		{
 			"'restic' is a valid type",
 			"restic",
-			false,
+			"",
+			"Uploader 'restic' is deprecated, don't use it for new backups, otherwise, the backed up data may not be accessed by new versions of Velero",
 		},
 		{
 			"'   kopia  ' is a valid type (space will be trimmed)",
 			"   kopia  ",
-			false,
+			"",
+			"",
 		},
 		{
 			"'anything_else' is invalid",
 			"anything_else",
-			true,
+			"invalid uploader type 'anything_else', valid upload types are: 'restic', 'kopia'",
+			"",
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if err := ValidateUploaderType(tt.input); (err != nil) != tt.wantErr {
-				t.Errorf("ValidateUploaderType(), input = '%s' error = %v, wantErr %v", tt.input, err, tt.wantErr)
+			msg, err := ValidateUploaderType(tt.input)
+			if tt.wantErr != "" {
+				assert.EqualError(t, err, tt.wantErr)
+			} else {
+				assert.NoError(t, err)
 			}
+
+			assert.Equal(t, tt.wantMsg, msg)
 		})
 	}
 }