Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use-name-as-prefix does not work as expected #1757

Open
jnodorp-jaconi opened this issue Oct 7, 2024 · 10 comments
Open

use-name-as-prefix does not work as expected #1757

jnodorp-jaconi opened this issue Oct 7, 2024 · 10 comments
Labels

Comments

@jnodorp-jaconi
Copy link

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
Copy link

jekkel commented Oct 7, 2024

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
Copy link
Contributor

wind57 commented Oct 7, 2024

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
Copy link
Contributor

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

@wind57
Copy link
Contributor

wind57 commented Oct 16, 2024

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
Copy link
Contributor

I also agree with the OP

@wind57
Copy link
Contributor

wind57 commented Oct 16, 2024

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

@ryanjbaxter
Copy link
Contributor

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
Copy link
Contributor

wind57 commented Oct 16, 2024

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
Copy link
Contributor

No in that case we need to wait

@wind57
Copy link
Contributor

wind57 commented Oct 16, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants