`use-name-as-prefix` does not work as expected

[jnodorp-jaconi]

Describe the bug

When importing Kubernetes secrets with use-name-as-prefix, I would expect the prefix to be the name of the secret. This does not work when using a label selector with duplicate keys:

application.yaml

spring:
  config:
    import: "kubernetes:"

  cloud:
    kubernetes:
      secrets:
        enable-api: true
        sources:
          - labels:
              load: "true"
        use-name-as-prefix: true

k8s-secrets.yaml

---
apiVersion: v1
kind: Secret
metadata:
  name: client-1
  labels:
    load: "true"
stringData:
  client-id: client-1
  client-secret: S3cr371!
---
apiVersion: v1
kind: Secret
metadata:
  name: client-2
  labels:
    load: "true"
stringData:
  client-id: client-2
  client-secret: S3cr372!

will result in these properties:

client-1.client-2.client-secret: S3cr372!
client-1.client-2.client-id: client-2

Expected result:

client-1.client-secret: S3cr371!
client-1.client-id: client-1
client-2.client-secret: S3cr372!
client-2.client-id: client-2

Spring Cloud: 2023.0.3

Sample

I built an example application here: https://github.com/jnodorp-jaconi/spring-cloud-kubernetes-reproducer

[jekkel]

Mmh, seems this is expected behavior:

One more thing to bear in mind is that we support prefix per source, not per secret. The easiest way to explain this is via an example:

[...]

Suppose that a query matching such a label will provide two secrets as a result: secret-a and secret-b. Both of these secrets have the same property name: color=sea-blue and color=ocean-blue. It is undefined which color will end-up as part of property sources, but the prefix for it will be secret-a.secret-b (concatenated sorted naturally, names of the secrets).

But IMO the behavior OP describes makes more sense to me, too.

[wind57]

First of all, @jekkel is correct, the behavior you are seeing is expected, in the sense that we did document it to be like this. Now, if that makes sense or not, is debatable. For example, this is the first time we receive such an issue...

I took a pass of the code for about 1h today and it seems that this is indeed doable, but:

• only in the next major release, since we can't break compatibility.
• someone from Spring team has to agree that this is a "bug" and we should do things differently.

If @ryanjbaxter says this is a bug and it's OK to change things in the next major, I might work on this one in the future.

[ryanjbaxter]

@wind57 why did we originally decide on the existing behavior?

[wind57]

I honestly do not recall :( I searched through the PRs history and it seems we did not pay enough attention to this. Now that I look back and think a little, I agree with OP here...

[ryanjbaxter]

I also agree with the OP

[wind57]

If this is a bug, means we can break compatibility in order to fix it?

[ryanjbaxter]

Well it doesn't seem like we intended this to work this way right? So in my mind it is a bug.

And if by breaking compatibility you mean that we would change the property name then yes.

[wind57]

the fix could mean altering some public records or methods that we have in place, that is what I meant: if it is OK to "break" those for the sake of fixing this issue

[ryanjbaxter]

No in that case we need to wait

[wind57]

ok, I'll give this one a spin over the weekend to see where it takes me... thx for the feedback.

[jnodorp-jaconi]

I am still interested in getting this issue resolved, as the feature would be really useful for handling multi-tenancy without requiring static configuration.

I updated the MRE repo to the latest Spring Boot and Spring Cloud versions.

I briefly looked at the code and think I could give this a shot. However, resolving this issue would be a slightly bigger change, because we loose the association between the ConfigMap / Secret and its content very early in the chain. Specifically when converting from List<StrippedSourceContainer> to MultipleSourcesContainer in the implementations of KubernetesClientContextToSourceData.

If you're willing to take a contribution despite that, I'll give it a try.

[wind57]

if you want, of course, give it a go! I planned to look at this one after all the changes I am currently working on around integration tests (which is almost done). So if you want this resolved, it will happen, very soon, imho. If you want to work on it, I will not touch it :)

[ryanjbaxter]

Agree, always happy to have contributions!

[jnodorp-jaconi]

I created #1911 as a draft. I adjusted the behavior the way I would expect it to work. I also adjusted all affected test cases. I did not yet update the documentation. If you're happy with the general direction I'll update the docs as well.

[wind57]

I've been looking at this for the past few weeks and the code there got very messy over time... it's not only this issue that you report here present, but there are some more.

I am working on something that should fix this issue and have some code refactoring too, without any breaking changes. I hope to have something ready in a couple of days.

[wind57]

@jnodorp-jaconi I am proposing a fix too, if you want to take a look at : https://github.com/spring-cloud/spring-cloud-kubernetes/pull/1914 thank you

[jnodorp-jaconi]

@jnodorp-jaconi I am proposing a fix too, if you want to take a look at : #1914 thank you

Thank you! I reviewed the PR and it should fix the issue we face.

[wind57]

@jnodorp-jaconi I am also proposing this PR: https://github.com/spring-cloud/spring-cloud-kubernetes/pull/1919 to update the documentation. Can you please take a look also?