fix 1051: reset backup target to default

pkg/api/vm/handler.go

@@ -380,9 +380,16 @@ func (h *vmActionHandler) restoreBackup(vmName, vmNamespace string, input Restor
 }
 
 func (h *vmActionHandler) checkBackupTargetConfigured() error {
-	target, err := h.settingCache.Get(settings.BackupTargetSettingName)
-	if err == nil && harvesterv1.SettingConfigured.IsTrue(target) {
-		return nil
+	targetSetting, err := h.settingCache.Get(settings.BackupTargetSettingName)
+	if err == nil && harvesterv1.SettingConfigured.IsTrue(targetSetting) {
+		// backup target may be reset to initial/default, the SettingConfigured.IsTrue meets
+		target, err := settings.DecodeBackupTarget(targetSetting.Value)
+		if err != nil {
+			return err
+		}
+		if !target.IsDefaultBackupTarget() {
+			return nil
+		}
 	}
 	return fmt.Errorf("backup target is invalid")
 }

pkg/controller/master/backup/backup.go

@@ -119,6 +119,8 @@ func (h *Handler) OnBackupChange(key string, vmBackup *harvesterv1.VirtualMachin
 		return nil, err
 	}
 
+	logrus.Debugf("OnBackupChange: vmBackup name:%s, target:%s:%s", vmBackup.Name, target.Type, target.Endpoint)
+
 	if isBackupReady(vmBackup) {
 		// We've changed backup target information to status since v1.0.0.
 		// For backport to v0.3.0, we move backup target information from annotation to status.
@@ -537,6 +539,12 @@ func (h *Handler) deleteVMBackupMetadata(vmBackup *harvesterv1.VirtualMachineBac
 		}
 	}
 
+	// when backup target has been reset to default, skip following
+	if target.IsDefaultBackupTarget() {
+		logrus.Debugf("vmBackup delete:%s, backup target is default, skip", vmBackup.Name)
+		return nil
+	}
+
 	if !IsBackupTargetSame(vmBackup.Status.BackupTarget, target) {
 		return nil
 	}
@@ -579,6 +587,12 @@ func (h *Handler) uploadVMBackupMetadata(vmBackup *harvesterv1.VirtualMachineBac
 		}
 	}
 
+	// when current backup target is default, skip following steps
+	// if backup target is default, IsBackupTargetSame is true when vmBackup.Status.BackupTarget is also default value
+	if target.IsDefaultBackupTarget() {
+		return nil
+	}
+
 	if !IsBackupTargetSame(vmBackup.Status.BackupTarget, target) {
 		return nil
 	}
@@ -618,13 +632,14 @@ func (h *Handler) uploadVMBackupMetadata(vmBackup *harvesterv1.VirtualMachineBac
 
 	shouldUpload := true
 	destURL := filepath.Join(metadataFolderPath, getVMBackupMetadataFileName(vmBackup.Namespace, vmBackup.Name))
-	if exist := bsDriver.FileExists(destURL); exist {
+	if bsDriver.FileExists(destURL) {
 		if remoteVMBackupMetadata, err := loadBackupMetadataInBackupTarget(destURL, bsDriver); err != nil {
 			return err
 		} else if reflect.DeepEqual(vmBackupMetadata, remoteVMBackupMetadata) {
 			shouldUpload = false
 		}
 	}
+
 	if shouldUpload {
 		logrus.Debugf("upload vm backup metadata %s/%s to backup target %s", vmBackup.Namespace, vmBackup.Name, target.Type)
 		if err := bsDriver.Write(destURL, bytes.NewReader(j)); err != nil {

pkg/controller/master/backup/backup_metadata.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"time"
 
 	"github.com/longhorn/backupstore"
 	_ "github.com/longhorn/backupstore/nfs"
@@ -84,14 +85,21 @@ func (h *MetadataHandler) OnBackupTargetChange(key string, setting *harvesterv1.
 		return nil, nil
 	}
 
-	target, err := settings.DecodeBackupTarget(settings.BackupTargetSet.Get())
+	target, err := settings.DecodeBackupTarget(setting.Value)
 	if err != nil {
 		return setting, err
 	}
 
+	logrus.Debugf("backup target change, sync vm backup:%s:%s", target.Type, target.Endpoint)
+
+	// when backup target is reset to default, do not trig sync
+	if target.IsDefaultBackupTarget() {
+		return nil, nil
+	}
+
 	if err = h.syncVMBackup(target); err != nil {
-		logrus.Errorf("can't sync vm backup metadata, err: %v", err)
-		h.settings.Enqueue(setting.Name)
+		logrus.Errorf("can't sync vm backup metadata, target:%s:%s, err: %v", target.Type, target.Endpoint, err)
+		h.settings.EnqueueAfter(setting.Name, 5*time.Second)
 		return nil, nil
 	}
 

pkg/controller/master/backup/backup_target.go

@@ -10,6 +10,7 @@ import (
 
 	longhornv1 "github.com/longhorn/longhorn-manager/k8s/pkg/apis/longhorn/v1beta1"
 	ctlcorev1 "github.com/rancher/wrangler/pkg/generated/controllers/core/v1"
+	"github.com/sirupsen/logrus"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -79,32 +80,64 @@ func (h *TargetHandler) OnBackupTargetChange(key string, setting *harvesterv1.Se
 		return setting, err
 	}
 
-	// Since we remove S3 access key id and secret access key after S3 backup target has been verified,
-	// we stop the controller to reconcile it again.
-	if target.Type == settings.S3BackupType && (target.SecretAccessKey == "" || target.AccessKeyID == "") {
-		return nil, nil
-	}
+	logrus.Debugf("backup target change:%s:%s", target.Type, target.Endpoint)
 
-	if err = h.updateLonghornTarget(target); err != nil {
-		return nil, err
-	}
+	switch target.Type {
+	case settings.S3BackupType:
+		// Since S3 access key id and secret access key are stripped after S3 backup target has been verified
+		// in reUpdateBackupTargetSettingSecret
+		// stop the controller to reconcile it
+		if target.SecretAccessKey == "" && target.AccessKeyID == "" {
+			return nil, nil
+		}
+
+		if err = h.updateLonghornTarget(target); err != nil {
+			return nil, err
+		}
 
-	if target.Type == settings.S3BackupType {
 		if err = h.updateBackupTargetSecret(target); err != nil {
 			return nil, err
 		}
-	}
 
-	return h.updateBackupTargetSetting(setting.DeepCopy(), target, err)
-}
+		return h.reUpdateBackupTargetSettingSecret(setting, target)
+
+	case settings.NFSBackupType:
+		if err = h.updateLonghornTarget(target); err != nil {
+			return nil, err
+		}
 
-func (h *TargetHandler) updateBackupTargetSetting(setting *harvesterv1.Setting, target *settings.BackupTarget, err error) (*harvesterv1.Setting, error) {
-	harvesterv1.SettingConfigured.SetError(setting, "", err)
+		// delete the may existing previous secret of S3
+		if err = h.deleteBackupTargetSecret(target); err != nil {
+			return nil, err
+		}
 
-	if target.Type == settings.NFSBackupType {
-		target.BucketName = ""
-		target.BucketRegion = ""
+		return nil, nil
+
+	default:
+		// reset backup target to default, then delete/update related settings
+		if target.IsDefaultBackupTarget() {
+			if err = h.updateLonghornTarget(target); err != nil {
+				return nil, err
+			}
+
+			// delete the may existing previous secret of S3
+			if err = h.deleteBackupTargetSecret(target); err != nil {
+				return nil, err
+			}
+
+			return nil, nil
+		}
+
+		return nil, fmt.Errorf("Invalid backup target type:%s or parameter", target.Type)
+	}
+}
+
+func (h *TargetHandler) reUpdateBackupTargetSettingSecret(setting *harvesterv1.Setting, target *settings.BackupTarget) (*harvesterv1.Setting, error) {
+	// only do a second update when s3 with credentials
+	if target.Type != settings.S3BackupType {
+		return nil, nil
 	}
+
 	// reset the s3 credentials to prevent controller reconcile and not to expose secret key
 	target.SecretAccessKey = ""
 	target.AccessKeyID = ""
@@ -113,9 +146,10 @@ func (h *TargetHandler) updateBackupTargetSetting(setting *harvesterv1.Setting,
 		return nil, err
 	}
 
-	setting.Value = string(targetBytes)
+	settingCpy := setting.DeepCopy()
+	settingCpy.Value = string(targetBytes)
 
-	return h.settings.Update(setting)
+	return h.settings.Update(settingCpy)
 }
 
 func (h *TargetHandler) updateLonghornTarget(backupTarget *settings.BackupTarget) error {
@@ -204,6 +238,18 @@ func (h *TargetHandler) updateBackupTargetSecret(target *settings.BackupTarget)
 	return h.updateLonghornBackupTargetSecretSetting(target)
 }
 
+func (h *TargetHandler) deleteBackupTargetSecret(target *settings.BackupTarget) error {
+	if err := h.secrets.Delete(util.LonghornSystemNamespaceName, util.BackupTargetSecretName, nil); err != nil && !apierrors.IsNotFound(err) {
+		return err
+	}
+
+	if err := h.longhornSettings.Delete(util.LonghornSystemNamespaceName, longhornBackupTargetSecretSettingName, nil); err != nil && !apierrors.IsNotFound(err) {
+		return err
+	}
+
+	return nil
+}
+
 func (h *TargetHandler) updateLonghornBackupTargetSecretSetting(target *settings.BackupTarget) error {
 	targetSecret, err := h.longhornSettingCache.Get(util.LonghornSystemNamespaceName, longhornBackupTargetSecretSettingName)
 	if err != nil {

pkg/settings/settings.go

@@ -3,6 +3,7 @@ package settings
 import (
 	"encoding/json"
 	"fmt"
+	"reflect"
 	"regexp"
 	"strconv"
 	"strings"
@@ -196,6 +197,15 @@ func DecodeBackupTarget(value string) (*BackupTarget, error) {
 	return target, nil
 }
 
+func (target *BackupTarget) IsDefaultBackupTarget() bool {
+	if target == nil || target.Type != "" {
+		return false
+	}
+
+	defaultTarget := &BackupTarget{}
+	return reflect.DeepEqual(target, defaultTarget)
+}
+
 func InitVMForceResetPolicy() string {
 	policy := &VMForceResetPolicy{
 		Enable: true,

pkg/webhook/resources/restore/validator.go

@@ -1,7 +1,6 @@
 package restore
 
 import (
-	"encoding/json"
 	"errors"
 	"fmt"
 
@@ -102,11 +101,16 @@ func (v *restoreValidator) checkBackupTarget(vmRestore *v1beta1.VirtualMachineRe
 	if err != nil {
 		return fmt.Errorf("can't get backup target setting, err: %w", err)
 	}
-	backupTarget := &settings.BackupTarget{}
-	if err := json.Unmarshal([]byte(backupTargetSetting.Value), backupTarget); err != nil {
+
+	backupTarget, err := settings.DecodeBackupTarget(backupTargetSetting.Value)
+	if err != nil {
 		return fmt.Errorf("unmarshal backup target failed, value: %s, err: %w", backupTargetSetting.Value, err)
 	}
 
+	if backupTarget.IsDefaultBackupTarget() {
+		return fmt.Errorf("backup target is invalid")
+	}
+
 	// get vmbackup
 	vmBackup, err := v.vmBackup.Get(vmRestore.Spec.VirtualMachineBackupNamespace, vmRestore.Spec.VirtualMachineBackupName)
 	if err != nil {

pkg/webhook/resources/setting/validator.go

@@ -15,6 +15,7 @@ import (
 	_ "github.com/longhorn/backupstore/nfs"
 	_ "github.com/longhorn/backupstore/s3"
 	"github.com/rancher/wrangler/pkg/slice"
+	"github.com/sirupsen/logrus"
 	"golang.org/x/net/http/httpproxy"
 	admissionregv1 "k8s.io/api/admissionregistration/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -146,6 +147,59 @@ func validateVMForceResetPolicy(setting *v1beta1.Setting) error {
 	return nil
 }
 
+// chech if this backup target is updated again by controller to strip secret information
+func (v *settingValidator) isUpdatedS3BackupTarget(target *settings.BackupTarget) bool {
+	if target.Type != settings.S3BackupType || target.SecretAccessKey != "" || target.AccessKeyID != "" {
+		return false
+	}
+
+	if savedSetting, err := v.settingCache.Get(settings.BackupTargetSettingName); err != nil {
+		return false
+	} else if savedTarget, err := settings.DecodeBackupTarget(savedSetting.Value); err != nil {
+		return false
+	} else {
+		// when any of those fields is different, then it is from user input, not from internal re-config
+		if savedTarget.Type != target.Type || savedTarget.BucketName != target.BucketName || savedTarget.BucketRegion != target.BucketRegion || savedTarget.Endpoint != target.Endpoint || savedTarget.VirtualHostedStyle != target.VirtualHostedStyle {
+			return false
+		}
+	}
+
+	return true
+}
+
+// for each type of backup target, a well defined field composition is checked here
+func (v *settingValidator) validateBackupTargetFields(target *settings.BackupTarget) error {
+	switch target.Type {
+	case settings.S3BackupType:
+		if target.SecretAccessKey == "" || target.AccessKeyID == "" {
+			return werror.NewInvalidError("S3 backup target should have access key and access key id", "value")
+		}
+
+		if target.BucketName == "" || target.BucketRegion == "" {
+			return werror.NewInvalidError("S3 backup target should have bucket name and region ", "value")
+		}
+
+	case settings.NFSBackupType:
+		if target.Endpoint == "" {
+			return werror.NewInvalidError("NFS backup target should have endpoint", "value")
+		}
+
+		if target.SecretAccessKey != "" || target.AccessKeyID != "" {
+			return werror.NewInvalidError("NFS backup target should not have access key or access key id", "value")
+		}
+
+		if target.BucketName != "" || target.BucketRegion != "" {
+			return werror.NewInvalidError("NFS backup target should not have bucket name or region", "value")
+		}
+
+	default:
+		// do not check target.IsDefaultBackupTarget again, direct return error
+		return werror.NewInvalidError("Invalid backup target type", "value")
+	}
+
+	return nil
+}
+
 func (v *settingValidator) validateBackupTarget(setting *v1beta1.Setting) error {
 	if setting.Value == "" {
 		return nil
@@ -156,15 +210,12 @@ func (v *settingValidator) validateBackupTarget(setting *v1beta1.Setting) error
 		return werror.NewInvalidError(err.Error(), "value")
 	}
 
-	// Since we remove S3 access key id and secret access key after S3 backup target has been verified,
-	// we stop the controller to reconcile it again.
-	if target.Type == settings.S3BackupType && (target.SecretAccessKey == "" || target.AccessKeyID == "") {
+	// when target is from internal re-update, allow fast pass
+	if v.isUpdatedS3BackupTarget(target) {
 		return nil
 	}
 
-	if target.Type == settings.NFSBackupType && (target.BucketName != "" || target.BucketRegion != "") {
-		return werror.NewInvalidError("NFS backup target should not have bucket name or region ", "value")
-	}
+	logrus.Debugf("validate backup target:%s:%s", target.Type, target.Endpoint)
 
 	vmBackups, err := v.vmBackupCache.List(metav1.NamespaceAll, labels.Everything())
 	if err != nil {
@@ -174,8 +225,18 @@ func (v *settingValidator) validateBackupTarget(setting *v1beta1.Setting) error
 		return werror.NewBadRequest("There is VMBackup in creating or deleting progress")
 	}
 
-	// Set OS environment variables for S3
+	// It is allowed to reset the current backup target setting to the default value
+	// when it is default, the validator will skip all remaining checks
+	if target.IsDefaultBackupTarget() {
+		return nil
+	}
+
+	if err = v.validateBackupTargetFields(target); err != nil {
+		return err
+	}
+
 	if target.Type == settings.S3BackupType {
+		// Set OS environment variables for S3
 		os.Setenv(backup.AWSAccessKey, target.AccessKeyID)
 		os.Setenv(backup.AWSSecretKey, target.SecretAccessKey)
 		os.Setenv(backup.AWSEndpoints, target.Endpoint)

pkg/webhook/types/admission.go

@@ -15,7 +15,7 @@ import (
 
 const (
 	AdmissionTypeMutation   = "mutation"
-	AdmissionTypeValidation = "validaton"
+	AdmissionTypeValidation = "validation"
 )
 
 // JSON Patch operations to mutate input data. See https://jsonpatch.com/ for more information.