diff --git a/pkg/api/vm/handler.go b/pkg/api/vm/handler.go index 801c36b37d2..e97d2f4155a 100644 --- a/pkg/api/vm/handler.go +++ b/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") } diff --git a/pkg/controller/master/backup/backup.go b/pkg/controller/master/backup/backup.go index 25c11bab1f5..60ec6a836b5 100644 --- a/pkg/controller/master/backup/backup.go +++ b/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 { diff --git a/pkg/controller/master/backup/backup_metadata.go b/pkg/controller/master/backup/backup_metadata.go index ed893ed242a..98a75d86c8a 100644 --- a/pkg/controller/master/backup/backup_metadata.go +++ b/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 } diff --git a/pkg/controller/master/backup/backup_target.go b/pkg/controller/master/backup/backup_target.go index dd8807add62..32842c54052 100644 --- a/pkg/controller/master/backup/backup_target.go +++ b/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 { diff --git a/pkg/settings/settings.go b/pkg/settings/settings.go index cc12eeaf20a..1fd5cb18ed4 100644 --- a/pkg/settings/settings.go +++ b/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, diff --git a/pkg/webhook/resources/restore/validator.go b/pkg/webhook/resources/restore/validator.go index 0b9eceada77..3108f078c05 100644 --- a/pkg/webhook/resources/restore/validator.go +++ b/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 { diff --git a/pkg/webhook/resources/setting/validator.go b/pkg/webhook/resources/setting/validator.go index bc00d1c05b5..361987c253b 100644 --- a/pkg/webhook/resources/setting/validator.go +++ b/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) diff --git a/pkg/webhook/types/admission.go b/pkg/webhook/types/admission.go index 7e6c42e2cc0..ae33a874de8 100644 --- a/pkg/webhook/types/admission.go +++ b/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.