Fix 1757 #1914

wind57 · 2025-04-08T08:05:45Z

No description provided.

Signed-off-by: wind57 <[email protected]>

wind57 · 2025-05-05T13:50:25Z

...gframework/cloud/kubernetes/fabric8/config/labeled_secret_with_profile/properties/Green.java

@@ -18,17 +18,11 @@

 import org.springframework.boot.context.properties.ConfigurationProperties;

-@ConfigurationProperties("green-purple-secret.green-purple-secret-k8s.green-secret.green-secret-k8s.green-secret-prod")


here is an example of a test that fixes the problem

wind57 · 2025-05-05T13:56:30Z

...s-commons/src/main/java/org/springframework/cloud/kubernetes/commons/config/ConfigUtils.java

@@ -390,7 +399,7 @@ public static final class Prefix {
 		/**
 		 * prefix is known at the callsite.
 		 */
-		public static Prefix KNOWN;
+		public static Prefix KNOWN = new Prefix(() -> "", "KNOWN");


this is a minor bug fix, that someone was not found until now, not really related to the fix itself

wind57 · 2025-05-05T14:12:16Z

The entire fix of the issue is based on the fact that we can (ab)use MultipleSourcesContainer. Its definition is:

public record MultipleSourcesContainer(LinkedHashSet<String> names, Map<String, Object> data) {

What we used to do, is keep inside names things like : my-configmap, my-configmap-k8s and in the data the actual data from both configmap. We would later use this info to compute a single property source and this is what the issue is really complaining about (and for good reason).

What I am proposing to do (and it would not be a breaking change, since public APIs don't break), is to still capture in names : my-configmap, my-configmap-k8s. But, in data we capture:

{
   my-configmap : [DATA_FROM_MY_CONFIGMAP],
   my-configmap-k8s : [DATA_FROM_MY_CONFIGMAP_K8S]
}

and yes, later in the code have a few casts. The idea is that is not going to be a breaking change anymore, granted, it is a bit ugly. But this "ugliness" can be re-factored away (which I will do of course) and be merged in the next major, without the casts and to slightly adjust the code.

Everything in this PR comes from this idea, all changes are reflecting it.

I also took a chance to refactor some of the code. Over the years (I can't believe its been years tbh!), we have fixed a few issues around NamedSourceData and LabeledSourceData and things got messy there. IMHO, this:

https://github.com/spring-cloud/spring-cloud-kubernetes/blob/cf8b2d54b112658895f4cae3d7fec7eb5b480380/spring-cloud-kubernetes-commons/src/main/java/org/springframework/cloud/kubernetes/commons/config/NamedSourceData.java

Now looks a lot easier to maintain.

I did the same clean-up for LabeledSourceData, just a simple flow to be able to understand the code faster.

I do understand that there are a lot of files to look at, but the vast majority are tests and if you start from the idea above, it might be not that complicated to grok them.

@ryanjbaxter

wind57 · 2025-05-05T14:12:49Z

btw, there is no documentation for this PR, which I wanted to make a PR immediately after this one

Signed-off-by: wind57 <[email protected]>

ryanjbaxter · 2025-05-05T15:28:55Z

...ons/src/main/java/org/springframework/cloud/kubernetes/commons/config/LabeledSourceData.java

+	 * When there is no data, the name of the property source is made from provided
+	 * labels, unlike when the data is present: when we use secret names.
+	 */
+	private SourceData emptySourceData(Map<String, String> labels, String target, String namespace) {


Wouldn't the secret still have a name in this case?

imho, in this case it does not matter, as there is nothing to wire from such sources, they are empty, so this bug does not affect these cases

But did the name of the empty property source also have the same problem as non-empty property sources?

not really. The name matters only when there is data also, but in this case data does not exist. Also, imo, this clearly denotes that : "we searched by these labels" and did not find anything... I hope I understood your question correctly.

ryanjbaxter · 2025-05-05T15:34:40Z

...mmons/src/main/java/org/springframework/cloud/kubernetes/commons/config/NamedSourceData.java

+						sourceDataForSourceName, prefix.prefixProvider().get()));
+			}
+
+			if (prefix.getName().equals(Prefix.DELAYED.getName())) {


What is a delayed prefix?

we had this for a while, I am not introducing it.

It's for the cases when you enabled "use-name-as-prefix", but the search happens based on labels, so the actual prefix is "delayed". Meaning we do not know it upfront, we will later, when we actually read by labels and find those names

ok i just needed a refresher because its been a while

jnodorp-jaconi

I reviewed this PR with respect to wether it fixes the issue we face (#1757), which it does.

wind57 · 2025-05-06T14:44:27Z

thank you for looking at it @jnodorp-jaconi

wind57 · 2025-05-06T15:36:27Z

I am aware that PR failed, I am looking into why and update

wind57 · 2025-05-06T15:40:29Z

@ryanjbaxter all PRs failed (3.1.x , 3.2.x and main) because of some github internal issue. You might want to re-trigger builds

ryanjbaxter · 2025-05-06T15:41:14Z

@wind57 yes I will try in a bit

wind57 added 2 commits April 1, 2025 22:46

started some basic work

c697df4

Signed-off-by: wind57 <[email protected]>

Merge branch '3.1.x' into fix-1757

c2647ca

spring-cloud-issues added the waiting-for-triage label Apr 8, 2025

wind57 added 8 commits April 8, 2025 23:42

dirty

8a43f03

Signed-off-by: wind57 <[email protected]>

first test

65b7011

Signed-off-by: wind57 <[email protected]>

assertions

36710f9

Signed-off-by: wind57 <[email protected]>

checkstyle

51c5144

Signed-off-by: wind57 <[email protected]>

Merge branch '3.1.x' into fix-1757

81600ba

checkstyle

abd7d46

Signed-off-by: wind57 <[email protected]>

Merge branch '3.1.x' into fix-1757

d735a3a

fix tests

cf8b2d5

Signed-off-by: wind57 <[email protected]>

wind57 commented May 5, 2025

View reviewed changes

wind57 changed the title ~~Fix 1757 a dummy try~~ Fix 1757 May 5, 2025

wind57 marked this pull request as ready for review May 5, 2025 14:12

wind57 mentioned this pull request May 5, 2025

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

Closed

checkstyle

f32f29b

Signed-off-by: wind57 <[email protected]>

ryanjbaxter reviewed May 5, 2025

View reviewed changes

jnodorp-jaconi approved these changes May 6, 2025

View reviewed changes

wind57 requested a review from ryanjbaxter May 6, 2025 14:43

ryanjbaxter linked an issue May 6, 2025 that may be closed by this pull request

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

Closed

ryanjbaxter merged commit e34c892 into spring-cloud:3.1.x May 6, 2025
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 1757 #1914

Fix 1757 #1914

wind57 commented Apr 8, 2025

wind57 May 5, 2025

wind57 May 5, 2025

wind57 commented May 5, 2025

wind57 commented May 5, 2025

ryanjbaxter May 5, 2025

wind57 May 5, 2025

ryanjbaxter May 5, 2025

wind57 May 6, 2025

ryanjbaxter May 5, 2025

wind57 May 5, 2025

ryanjbaxter May 5, 2025

jnodorp-jaconi left a comment •

edited

Loading

wind57 commented May 6, 2025

wind57 commented May 6, 2025

wind57 commented May 6, 2025

ryanjbaxter commented May 6, 2025

		@@ -18,17 +18,11 @@

		import org.springframework.boot.context.properties.ConfigurationProperties;

		@ConfigurationProperties("green-purple-secret.green-purple-secret-k8s.green-secret.green-secret-k8s.green-secret-prod")

Fix 1757 #1914

Fix 1757 #1914

Conversation

wind57 commented Apr 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wind57 commented May 5, 2025

wind57 commented May 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnodorp-jaconi left a comment • edited Loading

Choose a reason for hiding this comment

wind57 commented May 6, 2025

wind57 commented May 6, 2025

wind57 commented May 6, 2025

ryanjbaxter commented May 6, 2025

jnodorp-jaconi left a comment •

edited

Loading