From be2b51e470b3de78eea56756a20b9a911a7abfd6 Mon Sep 17 00:00:00 2001 From: Danail Branekov Date: Mon, 26 Feb 2024 10:55:21 +0000 Subject: [PATCH] Prefer using Data over StringData in secrets According to the [docs](https://github.com/kubernetes/api/blob/48ed98046a81320c5ec6681f614e31892035edef/core/v1/types.go#L6841): ``` stringData allows specifying non-binary secret data in string form. It is provided as a write-only input field for convenience. All keys and values are merged into the data field on write, overwriting any existing values. ``` This means that it is not always safe to use StringData, since existing keys will never be removed fixes #3125 Co-authored-by: Danail Branekov Co-authored-by: Georgi Sabev --- api/repositories/app_repository.go | 7 ++++--- .../controllers/workloads/cfapp_controller.go | 4 ++-- .../controllers/workloads/env/vcap_app_builder.go | 6 +++--- .../workloads/env/vcap_app_builder_test.go | 2 +- .../workloads/env/vcap_services_builder.go | 8 ++++---- .../workloads/env/vcap_services_builder_test.go | 14 +++++++------- .../controllers/workloads/k8sns/reconciler.go | 1 - .../controllers/workloads/k8sns/reconciler_test.go | 1 - 8 files changed, 21 insertions(+), 22 deletions(-) diff --git a/api/repositories/app_repository.go b/api/repositories/app_repository.go index b5fdab17f..197a10c8a 100644 --- a/api/repositories/app_repository.go +++ b/api/repositories/app_repository.go @@ -374,12 +374,14 @@ func (f *AppRepo) PatchAppEnvVars(ctx context.Context, authInfo authorization.In } _, err = controllerutil.CreateOrPatch(ctx, userClient, &secretObj, func() error { - secretObj.StringData = map[string]string{} + if secretObj.Data == nil { + secretObj.Data = map[string][]byte{} + } for k, v := range message.EnvironmentVariables { if v == nil { delete(secretObj.Data, k) } else { - secretObj.StringData[k] = *v + secretObj.Data[k] = []byte(*v) } } return nil @@ -701,7 +703,6 @@ func appEnvVarsSecretToRecord(envVars corev1.Secret) AppEnvVarsRecord { } func convertByteSliceValuesToStrings(inputMap map[string][]byte) map[string]string { - // StringData is a write-only field of a corev1.Secret, the real data lives in .Data and is []byte & base64 encoded outputMap := make(map[string]string, len(inputMap)) for k, v := range inputMap { outputMap[k] = string(v) diff --git a/controllers/controllers/workloads/cfapp_controller.go b/controllers/controllers/workloads/cfapp_controller.go index bec2ad5ca..1fb61a5d8 100644 --- a/controllers/controllers/workloads/cfapp_controller.go +++ b/controllers/controllers/workloads/cfapp_controller.go @@ -26,7 +26,7 @@ import ( ) type EnvValueBuilder interface { - BuildEnvValue(context.Context, *korifiv1alpha1.CFApp) (map[string]string, error) + BuildEnvValue(context.Context, *korifiv1alpha1.CFApp) (map[string][]byte, error) } // CFAppReconciler reconciles a CFApp object @@ -416,7 +416,7 @@ func (r *CFAppReconciler) reconcileVCAPSecret( } _, err = controllerutil.CreateOrPatch(ctx, r.k8sClient, secret, func() error { - secret.StringData = envValue + secret.Data = envValue return controllerutil.SetControllerReference(cfApp, secret, r.scheme) }) diff --git a/controllers/controllers/workloads/env/vcap_app_builder.go b/controllers/controllers/workloads/env/vcap_app_builder.go index 778e96791..2097993ef 100644 --- a/controllers/controllers/workloads/env/vcap_app_builder.go +++ b/controllers/controllers/workloads/env/vcap_app_builder.go @@ -23,7 +23,7 @@ func NewVCAPApplicationEnvValueBuilder(k8sClient client.Client, extraValues map[ } } -func (b *VCAPApplicationEnvValueBuilder) BuildEnvValue(ctx context.Context, cfApp *korifiv1alpha1.CFApp) (map[string]string, error) { +func (b *VCAPApplicationEnvValueBuilder) BuildEnvValue(ctx context.Context, cfApp *korifiv1alpha1.CFApp) (map[string][]byte, error) { space, err := b.getSpaceFromNamespace(ctx, cfApp.Namespace) if err != nil { return nil, fmt.Errorf("failed retrieving space for CFApp: %w", err) @@ -54,8 +54,8 @@ func (b *VCAPApplicationEnvValueBuilder) BuildEnvValue(ctx context.Context, cfAp marshalledVars, _ := json.Marshal(vars) - return map[string]string{ - "VCAP_APPLICATION": string(marshalledVars), + return map[string][]byte{ + "VCAP_APPLICATION": marshalledVars, }, nil } diff --git a/controllers/controllers/workloads/env/vcap_app_builder_test.go b/controllers/controllers/workloads/env/vcap_app_builder_test.go index 50a19f103..26826f72d 100644 --- a/controllers/controllers/workloads/env/vcap_app_builder_test.go +++ b/controllers/controllers/workloads/env/vcap_app_builder_test.go @@ -21,7 +21,7 @@ var _ = Describe("VCAP_APPLICATION env value builder", func() { Describe("BuildEnvValue", func() { var ( - vcapApplication map[string]string + vcapApplication map[string][]byte buildVCAPApplicationEnvValueErr error ) diff --git a/controllers/controllers/workloads/env/vcap_services_builder.go b/controllers/controllers/workloads/env/vcap_services_builder.go index 3f8149623..58a58010b 100644 --- a/controllers/controllers/workloads/env/vcap_services_builder.go +++ b/controllers/controllers/workloads/env/vcap_services_builder.go @@ -23,7 +23,7 @@ func NewVCAPServicesEnvValueBuilder(k8sClient client.Client) *VCAPServicesEnvVal return &VCAPServicesEnvValueBuilder{k8sClient: k8sClient} } -func (b *VCAPServicesEnvValueBuilder) BuildEnvValue(ctx context.Context, cfApp *korifiv1alpha1.CFApp) (map[string]string, error) { +func (b *VCAPServicesEnvValueBuilder) BuildEnvValue(ctx context.Context, cfApp *korifiv1alpha1.CFApp) (map[string][]byte, error) { serviceBindings := &korifiv1alpha1.CFServiceBindingList{} err := b.k8sClient.List(ctx, serviceBindings, client.InNamespace(cfApp.Namespace), @@ -34,7 +34,7 @@ func (b *VCAPServicesEnvValueBuilder) BuildEnvValue(ctx context.Context, cfApp * } if len(serviceBindings.Items) == 0 { - return map[string]string{"VCAP_SERVICES": "{}"}, nil + return map[string][]byte{"VCAP_SERVICES": []byte("{}")}, nil } serviceEnvs := VCAPServices{} @@ -59,8 +59,8 @@ func (b *VCAPServicesEnvValueBuilder) BuildEnvValue(ctx context.Context, cfApp * return nil, err } - return map[string]string{ - "VCAP_SERVICES": string(jsonVal), + return map[string][]byte{ + "VCAP_SERVICES": jsonVal, }, nil } diff --git a/controllers/controllers/workloads/env/vcap_services_builder_test.go b/controllers/controllers/workloads/env/vcap_services_builder_test.go index 4d7f4de51..82710ca1e 100644 --- a/controllers/controllers/workloads/env/vcap_services_builder_test.go +++ b/controllers/controllers/workloads/env/vcap_services_builder_test.go @@ -135,7 +135,7 @@ var _ = Describe("Builder", func() { Describe("BuildVCAPServicesEnvValue", func() { var ( - vcapServices map[string]string + vcapServices map[string][]byte buildVCAPServicesEnvValueErr error ) @@ -241,7 +241,7 @@ var _ = Describe("Builder", func() { }) It("returns an empty JSON string", func() { - Expect(vcapServices).To(HaveKeyWithValue("VCAP_SERVICES", "{}")) + Expect(vcapServices).To(HaveKeyWithValue("VCAP_SERVICES", BeEquivalentTo("{}"))) }) }) @@ -257,20 +257,20 @@ var _ = Describe("Builder", func() { }) }) -func extractServiceInfo(vcapServicesData map[string]string, serviceKey string, expectedElements int) []map[string]interface{} { +func extractServiceInfo(vcapServicesData map[string][]byte, serviceKey string, expectedElements int) []map[string]any { Expect(vcapServicesData).To(HaveKey("VCAP_SERVICES")) - var vcapServices map[string]interface{} + var vcapServices map[string]any Expect(json.Unmarshal([]byte(vcapServicesData["VCAP_SERVICES"]), &vcapServices)).To(Succeed()) Expect(vcapServices).To(HaveKey(serviceKey)) - serviceInfos, ok := vcapServices[serviceKey].([]interface{}) + serviceInfos, ok := vcapServices[serviceKey].([]any) Expect(ok).To(BeTrue()) Expect(serviceInfos).To(HaveLen(expectedElements)) - infos := make([]map[string]interface{}, 0, expectedElements) + infos := make([]map[string]any, 0, expectedElements) for i := range serviceInfos { - info, ok := serviceInfos[i].(map[string]interface{}) + info, ok := serviceInfos[i].(map[string]any) Expect(ok).To(BeTrue()) infos = append(infos, info) } diff --git a/controllers/controllers/workloads/k8sns/reconciler.go b/controllers/controllers/workloads/k8sns/reconciler.go index d8a36810d..cd9003a4b 100644 --- a/controllers/controllers/workloads/k8sns/reconciler.go +++ b/controllers/controllers/workloads/k8sns/reconciler.go @@ -184,7 +184,6 @@ func (r *Reconciler[T, NS]) propagateSecrets(ctx context.Context, obj NS, secret newSecret.Labels = removePackageManagerKeys(secret.Labels, looplog) newSecret.Immutable = secret.Immutable newSecret.Data = secret.Data - newSecret.StringData = secret.StringData newSecret.Type = secret.Type return nil }) diff --git a/controllers/controllers/workloads/k8sns/reconciler_test.go b/controllers/controllers/workloads/k8sns/reconciler_test.go index 89335654e..9d433698a 100644 --- a/controllers/controllers/workloads/k8sns/reconciler_test.go +++ b/controllers/controllers/workloads/k8sns/reconciler_test.go @@ -163,7 +163,6 @@ var _ = Describe("K8S NS Reconciler Integration Tests", func() { Expect(createdSecret.Data).To(Equal(imageRegistrySecret.Data)) Expect(createdSecret.Immutable).To(Equal(imageRegistrySecret.Immutable)) - Expect(createdSecret.StringData).To(Equal(imageRegistrySecret.StringData)) Expect(createdSecret.Type).To(Equal(imageRegistrySecret.Type)) By("omitting annotations from deployment tools", func() {