Skip to content

Replace carriage return when getting provider secrets for registry + E2E changes #652

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
May 3, 2022
Merged

Replace carriage return when getting provider secrets for registry + E2E changes #652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
955eea2
Replace carriage return when getting provider secrets for registry
kaovilai Apr 28, 2022
29e3eec
add tests
kaovilai Apr 28, 2022
dce9b99
replaceCarriageReturn unit test
kaovilai Apr 28, 2022
5a2bf0b
Registry credentials can use other secret names
kaovilai Apr 29, 2022
7ea4fa4
Merge branch 'just_registryCanUseOtherSecretNameFor_credentialsFile' …
kaovilai Apr 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 30 additions & 13 deletions controllers/registry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,14 +285,14 @@ func (r *DPAReconciler) buildRegistryDeployment(registryDeployment *appsv1.Deplo

// attach secret volume for cloud providers
if _, ok := bsl.Spec.Config["credentialsFile"]; ok {
if cloudProviderMap, bslCredOk := credentials.PluginSpecificFields[oadpv1alpha1.DefaultPlugin(bsl.Spec.Provider)]; bslCredOk {
if secretName, err := credentials.GetSecretNameFromCredentialsFileConfigString(bsl.Spec.Config["credentialsFile"]); err == nil {
registryDeployment.Spec.Template.Spec.Volumes = append(
registryDeployment.Spec.Template.Spec.Volumes,
corev1.Volume{
Name: cloudProviderMap.BslSecretName,
Name: secretName,
VolumeSource: corev1.VolumeSource{
Secret: &corev1.SecretVolumeSource{
SecretName: cloudProviderMap.BslSecretName,
SecretName: secretName,
},
},
},
Expand Down Expand Up @@ -388,12 +388,14 @@ func (r *DPAReconciler) buildRegistryContainer(bsl *velerov1.BackupStorageLocati

// check for secret name
if _, ok := bsl.Spec.Config["credentialsFile"]; ok { // If credentialsFile config is used, then mount the bsl secret
if _, bslCredOk := credentials.PluginSpecificFields[oadpv1alpha1.DefaultPlugin(bsl.Spec.Provider)]; bslCredOk {
containers[0].VolumeMounts = []corev1.VolumeMount{
{
Name: credentials.PluginSpecificFields[oadpv1alpha1.DefaultPlugin(bsl.Spec.Provider)].BslSecretName,
MountPath: credentials.PluginSpecificFields[oadpv1alpha1.DefaultPlugin(bsl.Spec.Provider)].BslMountPath,
},
if secretName, err := credentials.GetSecretNameFromCredentialsFileConfigString(bsl.Spec.Config["credentialsFile"]); err == nil {
if _, bslCredOk := credentials.PluginSpecificFields[oadpv1alpha1.DefaultPlugin(bsl.Spec.Provider)]; bslCredOk {
containers[0].VolumeMounts = []corev1.VolumeMount{
{
Name: secretName,
MountPath: credentials.PluginSpecificFields[oadpv1alpha1.DefaultPlugin(bsl.Spec.Provider)].BslMountPath,
},
}
}
}
} else if bsl.Spec.Provider == GCPProvider { // append secret volumes if the BSL provider is GCP
Expand Down Expand Up @@ -558,8 +560,22 @@ func (r *DPAReconciler) getProviderSecret(secretName string) (corev1.Secret, err
return secret, err
}
r.Log.Info(fmt.Sprintf("got provider secret name: %s", secret.Name))
// replace carriage return with new line
secret.Data = replaceCarriageReturn(secret.Data, r.Log)
return secret, nil
}

func replaceCarriageReturn(data map[string][]byte, logger logr.Logger) map[string][]byte {
for k, v := range data {
// report if carriage return is found
if strings.Contains(string(v), "\r\n") {
logger.Info("carriage return replaced")
data[k] = []byte(strings.ReplaceAll(string(v), "\r\n", "\n"))
}
}
return data
}

func (r *DPAReconciler) getSecretNameAndKeyforBackupLocation(bslspec oadpv1alpha1.BackupLocation) (string, string) {

if bslspec.CloudStorage != nil {
Expand All @@ -579,11 +595,12 @@ func (r *DPAReconciler) getSecretNameAndKey(bslSpec *velerov1.BackupStorageLocat
secretName := credentials.PluginSpecificFields[plugin].SecretName
secretKey := credentials.PluginSpecificFields[plugin].PluginSecretKey
if _, ok := bslSpec.Config["credentialsFile"]; ok {
secretName = credentials.PluginSpecificFields[plugin].BslSecretName
secretKey = credentials.PluginSpecificFields[plugin].PluginSecretKey
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
}
}
r.Log.Info(fmt.Sprintf("secret: %s", secretName))
r.Log.Info(fmt.Sprintf("key: %s", secretKey))
// check if user specified the Credential Name and Key
credential := bslSpec.Credential
if credential != nil {
Expand Down
123 changes: 112 additions & 11 deletions controllers/registry_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,26 @@
package controllers

import (
"github.com/go-logr/logr"
routev1 "github.com/openshift/api/route/v1"
oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"

//"k8s.io/apimachinery/pkg/types"
"context"
"reflect"
"testing"

"k8s.io/client-go/tools/record"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"

"github.com/go-logr/logr"
routev1 "github.com/openshift/api/route/v1"
oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1"
"github.com/openshift/oadp-operator/pkg/common"
velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/tools/record"
"k8s.io/utils/pointer"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
)

func getSchemeForFakeClientForRegistry() (*runtime.Scheme, error) {
Expand Down Expand Up @@ -91,6 +89,19 @@ var (
"aws_secret_access_key=" + testSecretAccessKey + "=" + testSecretAccessKey,
),
}
secretDataWithCarriageReturnInSecret = map[string][]byte{
"cloud": []byte(
"\n[" + testBslProfile + "]\r\n" +
"aws_access_key_id=" + testBslAccessKey + "\n" +
"aws_secret_access_key=" + testBslSecretAccessKey + "=" + testBslSecretAccessKey +
"\n[default]" + "\n" +
"aws_access_key_id=" + testAccessKey + "\n" +
"aws_secret_access_key=" + testSecretAccessKey + "=" + testSecretAccessKey +
"\r\n[test-profile]\n" +
"aws_access_key_id=" + testAccessKey + "\r\n" +
"aws_secret_access_key=" + testSecretAccessKey + "=" + testSecretAccessKey,
),
}
secretDataWithMixedQuotesAndSpacesInSecret = map[string][]byte{
"cloud": []byte(
"\n[" + testBslProfile + "]\n" +
Expand Down Expand Up @@ -261,6 +272,56 @@ func TestDPAReconciler_buildRegistryDeployment(t *testing.T) {
},
dpa: &oadpv1alpha1.DataProtectionApplication{},
},
{
name: "given a valid bsl with carriageReturn in secret val get appropriate registry deployment",
registryDeployment: &appsv1.Deployment{
ObjectMeta: metav1.ObjectMeta{
Name: "test-registry",
Namespace: "test-ns",
},
Spec: appsv1.DeploymentSpec{
Selector: &metav1.LabelSelector{
MatchLabels: map[string]string{
"component": "oadp-" + "test-bsl" + "-" + "aws" + "-registry",
},
},
},
},
bsl: &velerov1.BackupStorageLocation{
ObjectMeta: metav1.ObjectMeta{
Name: "test-bsl",
Namespace: "test-ns",
},
Spec: velerov1.BackupStorageLocationSpec{
Provider: AWSProvider,
StorageType: velerov1.StorageType{
ObjectStorage: &velerov1.ObjectStorageLocation{
Bucket: "aws-bucket",
},
},
Config: map[string]string{
Region: "aws-region",
S3URL: "https://sr-url-aws-domain.com",
InsecureSkipTLSVerify: "false",
},
},
},
secret: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "cloud-credentials",
Namespace: "test-ns",
},
Data: secretDataWithCarriageReturnInSecret,
},
registrySecret: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "oadp-test-bsl-aws-registry-secret",
Namespace: "test-ns",
},
Data: secretDataWithCarriageReturnInSecret,
},
dpa: &oadpv1alpha1.DataProtectionApplication{},
},
{
name: "given a valid bsl with use of quotes in secret val get appropriate registry deployment",
registryDeployment: &appsv1.Deployment{
Expand Down Expand Up @@ -1851,3 +1912,43 @@ func TestDPAReconciler_populateAzureRegistrySecret(t *testing.T) {
})
}
}

func Test_replaceCarriageReturn(t *testing.T) {
type args struct {
data map[string][]byte
logger logr.Logger
}
tests := []struct {
name string
args args
want map[string][]byte
}{
{
name: "Given a map with carriage return, carriage return is replaced with new line",
args: args{
data: map[string][]byte{
"test": []byte("test\r\n"),
},
logger: logr.FromContextOrDiscard(context.TODO()),
},
want: map[string][]byte{
"test": []byte("test\n"),
},
},
{
name: "Given secret data with carriage return, carriage return is replaced with new line",
args: args{
data: secretDataWithCarriageReturnInSecret,
logger: logr.FromContextOrDiscard(context.TODO()),
},
want: secretDataWithEqualInSecret,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := replaceCarriageReturn(tt.args.data, tt.args.logger); !reflect.DeepEqual(got, tt.want) {
t.Errorf("replaceCarriageReturn() = %v, want %v", got, tt.want)
}
})
}
}
37 changes: 26 additions & 11 deletions pkg/credentials/credentials.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"errors"
"fmt"
"os"
"strings"

oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1"
"github.com/openshift/oadp-operator/pkg/common"
Expand All @@ -15,7 +16,6 @@ type DefaultPluginFields struct {
IsCloudProvider bool
SecretName string
MountPath string
BslSecretName string
BslMountPath string
EnvCredentialsFile string
PluginImage string
Expand All @@ -33,7 +33,6 @@ var (
IsCloudProvider: true,
SecretName: "cloud-credentials",
MountPath: "/credentials",
BslSecretName: "bsl-cloud-credentials-aws",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kinda followed the logic we had for the previous ones as I included this for the case of CI, now that we are pushing this for new feature, we expect the user to have created the secret name with key given in the credentialsFile right ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think user should be able to pick which secret is used. My change didn't break functionality so I think it's okay.

Copy link
Member Author

@kaovilai kaovilai May 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior to this PR, user was expected to have created a secret anyway but the name that they can use when specifying credentialsFile was hardcoded. This meant I couldn't use a different secret for the carriage return test case unless I overwrite the current secret with the fixed name.

This only affected the credentialFile config case which most customers won't be using. With this PR the secret name for registry is now flexible if they specified bsl secret with credentialsFile config.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we expect the user to have created the secret name with key given in the credentialsFile right?

Yes.

BslMountPath: "/bsl-cloud-credentials-aws",
EnvCredentialsFile: common.AWSSharedCredentialsFileEnvKey,
PluginName: common.VeleroPluginForAWS,
Expand All @@ -43,7 +42,6 @@ var (
IsCloudProvider: true,
SecretName: "cloud-credentials-gcp",
MountPath: "/credentials-gcp",
BslSecretName: "bsl-cloud-credentials-gcp",
BslMountPath: "/bsl-cloud-credentials-gcp",
EnvCredentialsFile: common.GCPCredentialsEnvKey,
PluginName: common.VeleroPluginForGCP,
Expand All @@ -53,7 +51,6 @@ var (
IsCloudProvider: true,
SecretName: "cloud-credentials-azure",
MountPath: "/credentials-azure",
BslSecretName: "bsl-cloud-credentials-azure",
BslMountPath: "/bsl-cloud-credentials-azure",
EnvCredentialsFile: common.AzureCredentialsFileEnvKey,
PluginName: common.VeleroPluginForAzure,
Expand All @@ -75,6 +72,24 @@ var (
}
)

// Get secretName and secretKey from "secretName/secretKey"
func GetSecretNameKeyFromCredentialsFileConfigString(credentialsFile string) (string, string, error) {
credentialsFile = strings.TrimSpace(credentialsFile)
if credentialsFile == "" {
return "", "", nil
}
nameKeyArray := strings.Split(credentialsFile, "/")
if len(nameKeyArray) != 2 {
return "", "", errors.New("credentials file is not supported")
}
return nameKeyArray[0], nameKeyArray[1], nil
}

func GetSecretNameFromCredentialsFileConfigString(credentialsFile string) (string, error) {
name, _, err := GetSecretNameKeyFromCredentialsFileConfigString(credentialsFile)
return name, err
}

func getAWSPluginImage(dpa *oadpv1alpha1.DataProtectionApplication) string {
if dpa.Spec.UnsupportedOverrides[oadpv1alpha1.AWSPluginImageKey] != "" {
return dpa.Spec.UnsupportedOverrides[oadpv1alpha1.AWSPluginImageKey]
Expand Down Expand Up @@ -225,14 +240,14 @@ func AppendCloudProviderVolumes(dpa *oadpv1alpha1.DataProtectionApplication, ds
}
for _, bslSpec := range dpa.Spec.BackupLocations {
if _, ok := bslSpec.Velero.Config["credentialsFile"]; ok {
if cloudProviderMap, bslCredOk := PluginSpecificFields[oadpv1alpha1.DefaultPlugin(bslSpec.Velero.Provider)]; bslCredOk {
if secretName, err := GetSecretNameFromCredentialsFileConfigString(bslSpec.Velero.Config["credentialsFile"]); err == nil {
ds.Spec.Template.Spec.Volumes = append(
ds.Spec.Template.Spec.Volumes,
corev1.Volume{
Name: cloudProviderMap.BslSecretName,
Name: secretName,
VolumeSource: corev1.VolumeSource{
Secret: &corev1.SecretVolumeSource{
SecretName: cloudProviderMap.BslSecretName,
SecretName: secretName,
},
},
},
Expand Down Expand Up @@ -308,20 +323,20 @@ func AppendPluginSpecificSpecs(dpa *oadpv1alpha1.DataProtectionApplication, vele
// append bsl volume secret
for _, bslSpec := range dpa.Spec.BackupLocations {
if _, ok := bslSpec.Velero.Config["credentialsFile"]; ok {
if cloudProviderMap, bslCredOk := PluginSpecificFields[oadpv1alpha1.DefaultPlugin(bslSpec.Velero.Provider)]; bslCredOk {
if secretName, err := GetSecretNameFromCredentialsFileConfigString(bslSpec.Velero.Config["credentialsFile"]); err == nil {
veleroContainer.VolumeMounts = append(
veleroContainer.VolumeMounts,
corev1.VolumeMount{
Name: cloudProviderMap.BslSecretName,
Name: secretName,
MountPath: pluginSpecificMap.BslMountPath,
})
veleroDeployment.Spec.Template.Spec.Volumes = append(
veleroDeployment.Spec.Template.Spec.Volumes,
corev1.Volume{
Name: cloudProviderMap.BslSecretName,
Name: secretName,
VolumeSource: corev1.VolumeSource{
Secret: &corev1.SecretVolumeSource{
SecretName: cloudProviderMap.BslSecretName,
SecretName: secretName,
},
},
},
Expand Down
Loading