From 955eea2af736f785cf8e8f92ecd693f68ace945d Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 28 Apr 2022 16:15:38 -0400 Subject: [PATCH 1/3] Replace carriage return when getting provider secrets for registry Co-authored-by: GitHub Co-pilot --- controllers/registry.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/controllers/registry.go b/controllers/registry.go index 59132cc7d2..daf129ff85 100644 --- a/controllers/registry.go +++ b/controllers/registry.go @@ -558,8 +558,18 @@ 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) return secret, nil } + +func replaceCarriageReturn(data map[string][]byte) map[string][]byte { + for k, v := range data { + data[k] = []byte(strings.Replace(string(v), "\r\n", "\n", -1)) + } + return data +} + func (r *DPAReconciler) getSecretNameAndKeyforBackupLocation(bslspec oadpv1alpha1.BackupLocation) (string, string) { if bslspec.CloudStorage != nil { From 29e3eec3a020fa5d00cc683c0f505876dec9338c Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 28 Apr 2022 16:50:45 -0400 Subject: [PATCH 2/3] add tests --- controllers/registry.go | 10 ++-- controllers/registry_test.go | 63 ++++++++++++++++++++++++++ tests/e2e/dpa_deployment_suite_test.go | 37 +++++++++++++++ tests/e2e/e2e_suite_test.go | 2 + tests/e2e/utils/common_utils.go | 7 +++ 5 files changed, 116 insertions(+), 3 deletions(-) diff --git a/controllers/registry.go b/controllers/registry.go index daf129ff85..cf065aa042 100644 --- a/controllers/registry.go +++ b/controllers/registry.go @@ -559,13 +559,17 @@ func (r *DPAReconciler) getProviderSecret(secretName string) (corev1.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) + secret.Data = replaceCarriageReturn(secret.Data, r.Log) return secret, nil } -func replaceCarriageReturn(data map[string][]byte) map[string][]byte { +func replaceCarriageReturn(data map[string][]byte, logger logr.Logger) map[string][]byte { for k, v := range data { - data[k] = []byte(strings.Replace(string(v), "\r\n", "\n", -1)) + // 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 } diff --git a/controllers/registry_test.go b/controllers/registry_test.go index cc5b8bed26..f6e67fb26e 100644 --- a/controllers/registry_test.go +++ b/controllers/registry_test.go @@ -91,6 +91,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" + @@ -261,6 +274,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{ diff --git a/tests/e2e/dpa_deployment_suite_test.go b/tests/e2e/dpa_deployment_suite_test.go index 4c7a88f9a5..6cba61ed53 100644 --- a/tests/e2e/dpa_deployment_suite_test.go +++ b/tests/e2e/dpa_deployment_suite_test.go @@ -26,6 +26,7 @@ var _ = Describe("Configuration testing for DPA Custom Resource", func() { Name string BRestoreType BackupRestoreType DpaSpec *oadpv1alpha1.DataProtectionApplicationSpec + TestCarriageReturn bool WantError bool } @@ -62,6 +63,39 @@ var _ = Describe("Configuration testing for DPA Custom Resource", func() { }, WantError: false, }, nil), + Entry("Default velero CR, test carriage return", InstallCase{ + Name: "default-cr", + BRestoreType: RESTIC, + TestCarriageReturn: true, + DpaSpec: &oadpv1alpha1.DataProtectionApplicationSpec{ + Configuration: &oadpv1alpha1.ApplicationConfig{ + Velero: &oadpv1alpha1.VeleroConfig{ + DefaultPlugins: Dpa.Spec.Configuration.Velero.DefaultPlugins, + PodConfig: &oadpv1alpha1.PodConfig{}, + }, + Restic: &oadpv1alpha1.ResticConfig{ + PodConfig: &oadpv1alpha1.PodConfig{}, + Enable: pointer.Bool(true), + }, + }, + BackupLocations: []oadpv1alpha1.BackupLocation{ + { + Velero: &velero.BackupStorageLocationSpec{ + Provider: provider, + Config: bslConfig, + Default: true, + StorageType: velero.StorageType{ + ObjectStorage: &velero.ObjectStorageLocation{ + Bucket: bucket, + Prefix: VeleroPrefix, + }, + }, + }, + }, + }, + }, + WantError: false, + }, nil), Entry("Adding Velero custom plugin", InstallCase{ Name: "default-cr-velero-custom-plugin", BRestoreType: RESTIC, @@ -496,6 +530,9 @@ var _ = Describe("Configuration testing for DPA Custom Resource", func() { if len(installCase.DpaSpec.BackupLocations) > 0 { if installCase.DpaSpec.BackupLocations[0].Velero.Config != nil { installCase.DpaSpec.BackupLocations[0].Velero.Config["credentialsFile"] = "bsl-cloud-credentials-" + dpaCR.Provider + "/cloud" + if installCase.TestCarriageReturn { + installCase.DpaSpec.BackupLocations[0].Velero.Config["credentialsFile"] = "bsl-cloud-credentials-" + dpaCR.Provider + "-with-carriage-return/cloud" + } } } err = dpaCR.CreateOrUpdate(installCase.DpaSpec) diff --git a/tests/e2e/e2e_suite_test.go b/tests/e2e/e2e_suite_test.go index 6aadd715ca..673a1376bc 100755 --- a/tests/e2e/e2e_suite_test.go +++ b/tests/e2e/e2e_suite_test.go @@ -70,6 +70,8 @@ var _ = BeforeSuite(func() { Expect(err).NotTo(HaveOccurred()) err = CreateCredentialsSecret(cloudCredData, namespace, "bsl-cloud-credentials-"+provider) Expect(err).NotTo(HaveOccurred()) + err = CreateCredentialsSecret(utils.ReplaceSecretDataNewLineWithCarriageReturn(cloudCredData), namespace, "bsl-cloud-credentials-"+provider+"-with-carriage-return") + Expect(err).NotTo(HaveOccurred()) dpaCR.Credentials = ci_cred_file credData, err := utils.ReadFile(dpaCR.Credentials) diff --git a/tests/e2e/utils/common_utils.go b/tests/e2e/utils/common_utils.go index d60e1f9ba0..281194d882 100644 --- a/tests/e2e/utils/common_utils.go +++ b/tests/e2e/utils/common_utils.go @@ -2,6 +2,7 @@ package utils import ( "io/ioutil" + "strings" ) func ReadFile(path string) ([]byte, error) { @@ -14,3 +15,9 @@ func ReadFile(path string) ([]byte, error) { file, err := ioutil.ReadFile(path) return file, err } + +func ReplaceSecretDataNewLineWithCarriageReturn(data []byte) []byte { + // Replace new line with carriage return + data = []byte(strings.ReplaceAll(string(data), "\n", "\r\n")) + return data +} From dce9b9984bafd6d7f24c134efeaa5bef8bcd3f67 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 28 Apr 2022 19:43:10 -0400 Subject: [PATCH 3/3] replaceCarriageReturn unit test --- controllers/registry_test.go | 60 +++++++++++++++++++++++++++++------- 1 file changed, 49 insertions(+), 11 deletions(-) diff --git a/controllers/registry_test.go b/controllers/registry_test.go index f6e67fb26e..9b06ea36cf 100644 --- a/controllers/registry_test.go +++ b/controllers/registry_test.go @@ -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) { @@ -1914,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) + } + }) + } +}