Skip to content

Commit

Permalink
Prefer using Data over StringData in secrets
Browse files Browse the repository at this point in the history
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 cloudfoundry#3125

Co-authored-by: Danail Branekov <danailster@gmail.com>
Co-authored-by: Georgi Sabev <georgethebeatle@gmail.com>
  • Loading branch information
2 people authored and marsteg committed Mar 19, 2024
1 parent 7816e0d commit be2b51e
Show file tree
Hide file tree
Showing 8 changed files with 21 additions and 22 deletions.
7 changes: 4 additions & 3 deletions api/repositories/app_repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions controllers/controllers/workloads/cfapp_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
})
Expand Down
6 changes: 3 additions & 3 deletions controllers/controllers/workloads/env/vcap_app_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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{}
Expand All @@ -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
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ var _ = Describe("Builder", func() {

Describe("BuildVCAPServicesEnvValue", func() {
var (
vcapServices map[string]string
vcapServices map[string][]byte
buildVCAPServicesEnvValueErr error
)

Expand Down Expand Up @@ -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("{}")))
})
})

Expand All @@ -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)
}
Expand Down
1 change: 0 additions & 1 deletion controllers/controllers/workloads/k8sns/reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
})
Expand Down
1 change: 0 additions & 1 deletion controllers/controllers/workloads/k8sns/reconciler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down

0 comments on commit be2b51e

Please sign in to comment.