Skip to content

Commit b7504f9

Browse files
kaovilaiGitHub Co-pilot
kaovilai
and
GitHub Co-pilot
committed
Replace carriage return when getting provider secrets for registry + E2E changes (openshift#652)
* Replace carriage return when getting provider secrets for registry Co-authored-by: GitHub Co-pilot <[email protected]> * add tests * replaceCarriageReturn unit test * Registry credentials can use other secret names Co-authored-by: GitHub Co-pilot <[email protected]>
1 parent 17fbc2a commit b7504f9

File tree

6 files changed

+214
-35
lines changed

6 files changed

+214
-35
lines changed

‎controllers/registry.go

Lines changed: 30 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -285,14 +285,14 @@ func (r *DPAReconciler) buildRegistryDeployment(registryDeployment *appsv1.Deplo
285285

286286
// attach secret volume for cloud providers
287287
if _, ok := bsl.Spec.Config["credentialsFile"]; ok {
288-
if cloudProviderMap, bslCredOk := credentials.PluginSpecificFields[oadpv1alpha1.DefaultPlugin(bsl.Spec.Provider)]; bslCredOk {
288+
if secretName, err := credentials.GetSecretNameFromCredentialsFileConfigString(bsl.Spec.Config["credentialsFile"]); err == nil {
289289
registryDeployment.Spec.Template.Spec.Volumes = append(
290290
registryDeployment.Spec.Template.Spec.Volumes,
291291
corev1.Volume{
292-
Name: cloudProviderMap.BslSecretName,
292+
Name: secretName,
293293
VolumeSource: corev1.VolumeSource{
294294
Secret: &corev1.SecretVolumeSource{
295-
SecretName: cloudProviderMap.BslSecretName,
295+
SecretName: secretName,
296296
},
297297
},
298298
},
@@ -387,12 +387,14 @@ func (r *DPAReconciler) buildRegistryContainer(bsl *velerov1.BackupStorageLocati
387387

388388
// check for secret name
389389
if _, ok := bsl.Spec.Config["credentialsFile"]; ok { // If credentialsFile config is used, then mount the bsl secret
390-
if _, bslCredOk := credentials.PluginSpecificFields[oadpv1alpha1.DefaultPlugin(bsl.Spec.Provider)]; bslCredOk {
391-
containers[0].VolumeMounts = []corev1.VolumeMount{
392-
{
393-
Name: credentials.PluginSpecificFields[oadpv1alpha1.DefaultPlugin(bsl.Spec.Provider)].BslSecretName,
394-
MountPath: credentials.PluginSpecificFields[oadpv1alpha1.DefaultPlugin(bsl.Spec.Provider)].BslMountPath,
395-
},
390+
if secretName, err := credentials.GetSecretNameFromCredentialsFileConfigString(bsl.Spec.Config["credentialsFile"]); err == nil {
391+
if _, bslCredOk := credentials.PluginSpecificFields[oadpv1alpha1.DefaultPlugin(bsl.Spec.Provider)]; bslCredOk {
392+
containers[0].VolumeMounts = []corev1.VolumeMount{
393+
{
394+
Name: secretName,
395+
MountPath: credentials.PluginSpecificFields[oadpv1alpha1.DefaultPlugin(bsl.Spec.Provider)].BslMountPath,
396+
},
397+
}
396398
}
397399
}
398400
} else if bsl.Spec.Provider == GCPProvider { // append secret volumes if the BSL provider is GCP
@@ -557,8 +559,22 @@ func (r *DPAReconciler) getProviderSecret(secretName string) (corev1.Secret, err
557559
return secret, err
558560
}
559561
r.Log.Info(fmt.Sprintf("got provider secret name: %s", secret.Name))
562+
// replace carriage return with new line
563+
secret.Data = replaceCarriageReturn(secret.Data, r.Log)
560564
return secret, nil
561565
}
566+
567+
func replaceCarriageReturn(data map[string][]byte, logger logr.Logger) map[string][]byte {
568+
for k, v := range data {
569+
// report if carriage return is found
570+
if strings.Contains(string(v), "\r\n") {
571+
logger.Info("carriage return replaced")
572+
data[k] = []byte(strings.ReplaceAll(string(v), "\r\n", "\n"))
573+
}
574+
}
575+
return data
576+
}
577+
562578
func (r *DPAReconciler) getSecretNameAndKeyforBackupLocation(bslspec oadpv1alpha1.BackupLocation) (string, string) {
563579

564580
if bslspec.CloudStorage != nil {
@@ -578,11 +594,12 @@ func (r *DPAReconciler) getSecretNameAndKey(bslSpec *velerov1.BackupStorageLocat
578594
secretName := credentials.PluginSpecificFields[plugin].SecretName
579595
secretKey := credentials.PluginSpecificFields[plugin].PluginSecretKey
580596
if _, ok := bslSpec.Config["credentialsFile"]; ok {
581-
secretName = credentials.PluginSpecificFields[plugin].BslSecretName
582-
secretKey = credentials.PluginSpecificFields[plugin].PluginSecretKey
597+
if secretName, secretKey, err :=
598+
credentials.GetSecretNameKeyFromCredentialsFileConfigString(bslSpec.Config["credentialsFile"]); err == nil {
599+
r.Log.Info(fmt.Sprintf("credentialsFile secret: %s, key: %s", secretName, secretKey))
600+
return secretName, secretKey
601+
}
583602
}
584-
r.Log.Info(fmt.Sprintf("secret: %s", secretName))
585-
r.Log.Info(fmt.Sprintf("key: %s", secretKey))
586603
// check if user specified the Credential Name and Key
587604
credential := bslSpec.Credential
588605
if credential != nil {

‎controllers/registry_test.go

Lines changed: 112 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,26 @@
11
package controllers
22

33
import (
4-
"github.com/go-logr/logr"
5-
routev1 "github.com/openshift/api/route/v1"
6-
oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1"
7-
"k8s.io/apimachinery/pkg/runtime"
8-
"k8s.io/apimachinery/pkg/types"
9-
10-
//"k8s.io/apimachinery/pkg/types"
4+
"context"
115
"reflect"
126
"testing"
137

14-
"k8s.io/client-go/tools/record"
15-
"sigs.k8s.io/controller-runtime/pkg/client"
16-
"sigs.k8s.io/controller-runtime/pkg/client/fake"
17-
8+
"github.com/go-logr/logr"
9+
routev1 "github.com/openshift/api/route/v1"
10+
oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1"
1811
"github.com/openshift/oadp-operator/pkg/common"
1912
velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
2013
appsv1 "k8s.io/api/apps/v1"
2114
corev1 "k8s.io/api/core/v1"
2215
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
16+
"k8s.io/apimachinery/pkg/runtime"
17+
"k8s.io/apimachinery/pkg/types"
2318
"k8s.io/apimachinery/pkg/util/intstr"
2419
"k8s.io/client-go/kubernetes/scheme"
20+
"k8s.io/client-go/tools/record"
2521
"k8s.io/utils/pointer"
22+
"sigs.k8s.io/controller-runtime/pkg/client"
23+
"sigs.k8s.io/controller-runtime/pkg/client/fake"
2624
)
2725

2826
func getSchemeForFakeClientForRegistry() (*runtime.Scheme, error) {
@@ -91,6 +89,19 @@ var (
9189
"aws_secret_access_key=" + testSecretAccessKey + "=" + testSecretAccessKey,
9290
),
9391
}
92+
secretDataWithCarriageReturnInSecret = map[string][]byte{
93+
"cloud": []byte(
94+
"\n[" + testBslProfile + "]\r\n" +
95+
"aws_access_key_id=" + testBslAccessKey + "\n" +
96+
"aws_secret_access_key=" + testBslSecretAccessKey + "=" + testBslSecretAccessKey +
97+
"\n[default]" + "\n" +
98+
"aws_access_key_id=" + testAccessKey + "\n" +
99+
"aws_secret_access_key=" + testSecretAccessKey + "=" + testSecretAccessKey +
100+
"\r\n[test-profile]\n" +
101+
"aws_access_key_id=" + testAccessKey + "\r\n" +
102+
"aws_secret_access_key=" + testSecretAccessKey + "=" + testSecretAccessKey,
103+
),
104+
}
94105
secretDataWithMixedQuotesAndSpacesInSecret = map[string][]byte{
95106
"cloud": []byte(
96107
"\n[" + testBslProfile + "]\n" +
@@ -261,6 +272,56 @@ func TestDPAReconciler_buildRegistryDeployment(t *testing.T) {
261272
},
262273
dpa: &oadpv1alpha1.DataProtectionApplication{},
263274
},
275+
{
276+
name: "given a valid bsl with carriageReturn in secret val get appropriate registry deployment",
277+
registryDeployment: &appsv1.Deployment{
278+
ObjectMeta: metav1.ObjectMeta{
279+
Name: "test-registry",
280+
Namespace: "test-ns",
281+
},
282+
Spec: appsv1.DeploymentSpec{
283+
Selector: &metav1.LabelSelector{
284+
MatchLabels: map[string]string{
285+
"component": "oadp-" + "test-bsl" + "-" + "aws" + "-registry",
286+
},
287+
},
288+
},
289+
},
290+
bsl: &velerov1.BackupStorageLocation{
291+
ObjectMeta: metav1.ObjectMeta{
292+
Name: "test-bsl",
293+
Namespace: "test-ns",
294+
},
295+
Spec: velerov1.BackupStorageLocationSpec{
296+
Provider: AWSProvider,
297+
StorageType: velerov1.StorageType{
298+
ObjectStorage: &velerov1.ObjectStorageLocation{
299+
Bucket: "aws-bucket",
300+
},
301+
},
302+
Config: map[string]string{
303+
Region: "aws-region",
304+
S3URL: "https://sr-url-aws-domain.com",
305+
InsecureSkipTLSVerify: "false",
306+
},
307+
},
308+
},
309+
secret: &corev1.Secret{
310+
ObjectMeta: metav1.ObjectMeta{
311+
Name: "cloud-credentials",
312+
Namespace: "test-ns",
313+
},
314+
Data: secretDataWithCarriageReturnInSecret,
315+
},
316+
registrySecret: &corev1.Secret{
317+
ObjectMeta: metav1.ObjectMeta{
318+
Name: "oadp-test-bsl-aws-registry-secret",
319+
Namespace: "test-ns",
320+
},
321+
Data: secretDataWithCarriageReturnInSecret,
322+
},
323+
dpa: &oadpv1alpha1.DataProtectionApplication{},
324+
},
264325
{
265326
name: "given a valid bsl with use of quotes in secret val get appropriate registry deployment",
266327
registryDeployment: &appsv1.Deployment{
@@ -1851,3 +1912,43 @@ func TestDPAReconciler_populateAzureRegistrySecret(t *testing.T) {
18511912
})
18521913
}
18531914
}
1915+
1916+
func Test_replaceCarriageReturn(t *testing.T) {
1917+
type args struct {
1918+
data map[string][]byte
1919+
logger logr.Logger
1920+
}
1921+
tests := []struct {
1922+
name string
1923+
args args
1924+
want map[string][]byte
1925+
}{
1926+
{
1927+
name: "Given a map with carriage return, carriage return is replaced with new line",
1928+
args: args{
1929+
data: map[string][]byte{
1930+
"test": []byte("test\r\n"),
1931+
},
1932+
logger: logr.FromContextOrDiscard(context.TODO()),
1933+
},
1934+
want: map[string][]byte{
1935+
"test": []byte("test\n"),
1936+
},
1937+
},
1938+
{
1939+
name: "Given secret data with carriage return, carriage return is replaced with new line",
1940+
args: args{
1941+
data: secretDataWithCarriageReturnInSecret,
1942+
logger: logr.FromContextOrDiscard(context.TODO()),
1943+
},
1944+
want: secretDataWithEqualInSecret,
1945+
},
1946+
}
1947+
for _, tt := range tests {
1948+
t.Run(tt.name, func(t *testing.T) {
1949+
if got := replaceCarriageReturn(tt.args.data, tt.args.logger); !reflect.DeepEqual(got, tt.want) {
1950+
t.Errorf("replaceCarriageReturn() = %v, want %v", got, tt.want)
1951+
}
1952+
})
1953+
}
1954+
}

‎pkg/credentials/credentials.go

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"errors"
55
"fmt"
66
"os"
7+
"strings"
78

89
oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1"
910
"github.com/openshift/oadp-operator/pkg/common"
@@ -15,7 +16,6 @@ type DefaultPluginFields struct {
1516
IsCloudProvider bool
1617
SecretName string
1718
MountPath string
18-
BslSecretName string
1919
BslMountPath string
2020
EnvCredentialsFile string
2121
PluginImage string
@@ -33,7 +33,6 @@ var (
3333
IsCloudProvider: true,
3434
SecretName: "cloud-credentials",
3535
MountPath: "/credentials",
36-
BslSecretName: "bsl-cloud-credentials-aws",
3736
BslMountPath: "/bsl-cloud-credentials-aws",
3837
EnvCredentialsFile: common.AWSSharedCredentialsFileEnvKey,
3938
PluginName: common.VeleroPluginForAWS,
@@ -43,7 +42,6 @@ var (
4342
IsCloudProvider: true,
4443
SecretName: "cloud-credentials-gcp",
4544
MountPath: "/credentials-gcp",
46-
BslSecretName: "bsl-cloud-credentials-gcp",
4745
BslMountPath: "/bsl-cloud-credentials-gcp",
4846
EnvCredentialsFile: common.GCPCredentialsEnvKey,
4947
PluginName: common.VeleroPluginForGCP,
@@ -53,7 +51,6 @@ var (
5351
IsCloudProvider: true,
5452
SecretName: "cloud-credentials-azure",
5553
MountPath: "/credentials-azure",
56-
BslSecretName: "bsl-cloud-credentials-azure",
5754
BslMountPath: "/bsl-cloud-credentials-azure",
5855
EnvCredentialsFile: common.AzureCredentialsFileEnvKey,
5956
PluginName: common.VeleroPluginForAzure,
@@ -75,6 +72,24 @@ var (
7572
}
7673
)
7774

75+
// Get secretName and secretKey from "secretName/secretKey"
76+
func GetSecretNameKeyFromCredentialsFileConfigString(credentialsFile string) (string, string, error) {
77+
credentialsFile = strings.TrimSpace(credentialsFile)
78+
if credentialsFile == "" {
79+
return "", "", nil
80+
}
81+
nameKeyArray := strings.Split(credentialsFile, "/")
82+
if len(nameKeyArray) != 2 {
83+
return "", "", errors.New("credentials file is not supported")
84+
}
85+
return nameKeyArray[0], nameKeyArray[1], nil
86+
}
87+
88+
func GetSecretNameFromCredentialsFileConfigString(credentialsFile string) (string, error) {
89+
name, _, err := GetSecretNameKeyFromCredentialsFileConfigString(credentialsFile)
90+
return name, err
91+
}
92+
7893
func getAWSPluginImage(dpa *oadpv1alpha1.DataProtectionApplication) string {
7994
if dpa.Spec.UnsupportedOverrides[oadpv1alpha1.AWSPluginImageKey] != "" {
8095
return dpa.Spec.UnsupportedOverrides[oadpv1alpha1.AWSPluginImageKey]
@@ -225,14 +240,14 @@ func AppendCloudProviderVolumes(dpa *oadpv1alpha1.DataProtectionApplication, ds
225240
}
226241
for _, bslSpec := range dpa.Spec.BackupLocations {
227242
if _, ok := bslSpec.Velero.Config["credentialsFile"]; ok {
228-
if cloudProviderMap, bslCredOk := PluginSpecificFields[oadpv1alpha1.DefaultPlugin(bslSpec.Velero.Provider)]; bslCredOk {
243+
if secretName, err := GetSecretNameFromCredentialsFileConfigString(bslSpec.Velero.Config["credentialsFile"]); err == nil {
229244
ds.Spec.Template.Spec.Volumes = append(
230245
ds.Spec.Template.Spec.Volumes,
231246
corev1.Volume{
232-
Name: cloudProviderMap.BslSecretName,
247+
Name: secretName,
233248
VolumeSource: corev1.VolumeSource{
234249
Secret: &corev1.SecretVolumeSource{
235-
SecretName: cloudProviderMap.BslSecretName,
250+
SecretName: secretName,
236251
},
237252
},
238253
},
@@ -308,20 +323,20 @@ func AppendPluginSpecificSpecs(dpa *oadpv1alpha1.DataProtectionApplication, vele
308323
// append bsl volume secret
309324
for _, bslSpec := range dpa.Spec.BackupLocations {
310325
if _, ok := bslSpec.Velero.Config["credentialsFile"]; ok {
311-
if cloudProviderMap, bslCredOk := PluginSpecificFields[oadpv1alpha1.DefaultPlugin(bslSpec.Velero.Provider)]; bslCredOk {
326+
if secretName, err := GetSecretNameFromCredentialsFileConfigString(bslSpec.Velero.Config["credentialsFile"]); err == nil {
312327
veleroContainer.VolumeMounts = append(
313328
veleroContainer.VolumeMounts,
314329
corev1.VolumeMount{
315-
Name: cloudProviderMap.BslSecretName,
330+
Name: secretName,
316331
MountPath: pluginSpecificMap.BslMountPath,
317332
})
318333
veleroDeployment.Spec.Template.Spec.Volumes = append(
319334
veleroDeployment.Spec.Template.Spec.Volumes,
320335
corev1.Volume{
321-
Name: cloudProviderMap.BslSecretName,
336+
Name: secretName,
322337
VolumeSource: corev1.VolumeSource{
323338
Secret: &corev1.SecretVolumeSource{
324-
SecretName: cloudProviderMap.BslSecretName,
339+
SecretName: secretName,
325340
},
326341
},
327342
},

0 commit comments

Comments
 (0)