openshift/oadp-operator

Clean up credentials related code

Opened this issue · 3 comments

mateusoliveira43 commented

Credentials are used by BackupStorageLocation (BSL) and VolumeSnapshotLocation (VSL)

  • confirm that no other part of OADP uses credentials code
  • should controllers/registry.go be deleted/moved to pkg/credentials/credentials.go?

in controllers/bsl.go

  • oadp-operator/controllers/bsl.go

    Lines 72 to 74 in 91e1aac

    if bslSpec.CloudStorage.Credential.LocalObjectReference.Name == "" {
    return false, fmt.Errorf("must provide a valid credential secret name")
    }
    is duplication of

    oadp-operator/controllers/bsl.go

    Line 415 in 91e1aac

    if bsl.Velero.Credential.Name == "" {

  • oadp-operator/controllers/bsl.go

    Lines 81 to 83 in 91e1aac

    if bslSpec.CloudStorage != nil && bslSpec.Velero != nil {
    return false, fmt.Errorf("must choose one of bucket or velero")
    }
    is duplication of

    oadp-operator/controllers/bsl.go

    Line 386 in 91e1aac

    if bsl.CloudStorage != nil && bsl.Velero != nil {

  • these functions all have duplication

    oadp-operator/controllers/bsl.go

    Line 268 in 91e1aac

    func (r *DPAReconciler) validateAWSBackupStorageLocation(bslSpec velerov1.BackupStorageLocationSpec, dpa *oadpv1alpha1.DataProtectionApplication) error {

    oadp-operator/controllers/bsl.go

    Line 299 in 91e1aac

    func (r *DPAReconciler) validateAzureBackupStorageLocation(bslSpec velerov1.BackupStorageLocationSpec, dpa *oadpv1alpha1.DataProtectionApplication) error {

    oadp-operator/controllers/bsl.go

    Line 330 in 91e1aac

    func (r *DPAReconciler) validateGCPBackupStorageLocation(bslSpec velerov1.BackupStorageLocationSpec, dpa *oadpv1alpha1.DataProtectionApplication) error {

    move it to this function

    oadp-operator/controllers/bsl.go

    Line 361 in 91e1aac

    func (r *DPAReconciler) validateProviderPluginAndSecret(bslSpec velerov1.BackupStorageLocationSpec, dpa *oadpv1alpha1.DataProtectionApplication) error {
    (but remove secret validation from it, it already done by other part of the code)

  • should this only be called if !(dpa.Spec.Configuration.Velero.HasFeatureFlag("no-secret"))?

    oadp-operator/controllers/bsl.go

    Lines 123 to 127 in 91e1aac

    secretName, _ := r.getSecretNameAndKeyforBackupLocation(bslSpec)
    _, err := r.UpdateCredentialsSecretLabels(secretName, dpa.Namespace, dpa.Name)
    if err != nil {
    return false, err
    }

  • remove validation from this function, it was done previously

    oadp-operator/controllers/bsl.go

    Line 194 in 91e1aac

    func (r *DPAReconciler) UpdateCredentialsSecretLabels(secretName string, namespace string, dpaName string) (bool, error) {

in controllers/registry.go

  • are not these duplication from api/v1alpha1/oadp_types.go?

    oadp-operator/controllers/registry.go

    Line 55 in 91e1aac

    const (
  • this function should not patch secret every time

    oadp-operator/controllers/registry.go

    Line 216 in 91e1aac

    func (r *DPAReconciler) getProviderSecret(secretName string) (corev1.Secret, error) {
    move patching code to validation part
  • delete this code

    oadp-operator/controllers/registry.go

    Lines 264 to 270 in 91e1aac

    if _, ok := bslSpec.Config["credentialsFile"]; ok {
    if secretName, secretKey, err :=
    credentials.GetSecretNameKeyFromCredentialsFileConfigString(bslSpec.Config["credentialsFile"]); err == nil {
    r.Log.Info(fmt.Sprintf("credentialsFile secret: %s, key: %s", secretName, secretKey))
    return secretName, secretKey
    }
    }
    and docs/developer/testing/ MULTI_CLOUD_TESTING_UPDATES.md file
  • verify credential function should check all cases
// add doc comments!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
func (r *DPAReconciler) verifyCredential(credential *corev1.SecretKeySelector, provider oadpv1alpha1.DefaultPlugin, location string) error {
	var credentialName string
	var credentialKey string

	if credential != nil {
		// Check if user specified empty credential name
		if credential.Name == "" {
			return fmt.Errorf("credential name specified in %s cannot be empty", location)
		} else {
			credentialName = credential.Name
		}
		// Check if user specified empty credential key
		if credential.Key == "" {
			return fmt.Errorf("credential key specified in %s cannot be empty", location)
		} else {
			credentialKey = credential.Key
		}
	} else {
		if provider != "" {
			// Assume default values
			credentialName = credentials.PluginSpecificFields[provider].SecretName
			credentialKey = credentials.PluginSpecificFields[provider].PluginSecretKey
		} else {
			// cloud storage case
			return fmt.Errorf("must provide a valid credential secret")
		}
	}

	secret, err := r.getProviderSecret(credentialName)
	if err != nil {
		return err
	}
	// need???
	// if secret.Name == "" {
	// 	return false, errors.New("secret not found")
	// }
	data, foundKey := secret.Data[credentialKey]
	if !foundKey || len(data) == 0 {
		return fmt.Errorf("Secret name %s is missing data for key %s", credentialName, credentialKey)
	}
	return nil
}

in controllers/validator.go

in controllers/vsl.go

in pkg/credentials/credentials.go

kaovilai commented

related issue: #921

openshift-bot commented

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

mateusoliveira43 commented

/lifecycle frozen