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

Kubernetes as a Composite config source #1873

Merged
merged 21 commits into from
Feb 23, 2025

Conversation

arjavdongaonkar
Copy link
Contributor

Enables Kubernets as a type in composite config source

public EnvironmentRepository kubernetesEnvironmentRepository(CoreV1Api coreV1Api,
List<KubernetesPropertySourceSupplier> kubernetesPropertySourceSuppliers,
KubernetesNamespaceProvider kubernetesNamespaceProvider) {
List<KubernetesPropertySourceSupplier> kubernetesPropertySourceSuppliers,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will fail checkstyle... before pushing a PR, you can do a mvn clean install -Dspring-boot.build-image.skip=true -DskipITs -DskipTests locally, just to see that at least it compiles OK.

@wind57
Copy link
Contributor

wind57 commented Feb 11, 2025

I had an initial pass over it, but I do not know all the details of what is going on. I might look again towards the end of the week... and btw, you have no tests here :)

@arjavdongaonkar
Copy link
Contributor Author

I had an initial pass over it, but I do not know all the details of what is going on. I might look again towards the end of the week... and btw, you have no tests here :)

Thanks! I'll add the tests. For context, please check this issue.
tbh, This is my first open-source contribution, so I appreciate the feedback—I'll keep these points in mind.

@ryanjbaxter
Copy link
Contributor

It would be great to have some tests that actually test that the kubernetes environment repository works in a composite scenario.

@arjavdongaonkar
Copy link
Contributor Author

It would be great to have some tests that actually test that the kubernetes environment repository works in a composite scenario.

Hey @ryanjbaxter,
Just to confirm, when we say "kubernetes environment repository works in a composite scenario," do we mean writing Composite Integration Tests where kubernetes is one of the type in the composite setup?

@ryanjbaxter
Copy link
Contributor

ryanjbaxter commented Feb 14, 2025

I am rethinking this a bit....

There are two ways to configure a composite environment repository

The first way you would do

spring:
  profiles:
    active: composite
  cloud:
    config:
      server:
        composite:
        - type: git
           uri: file:///path/to/rex/git/repo
        - type: kubernetes

But I don't think this would work with this PR because there is not kubernetes profile active.

The second way

spring:
 profiles:
   active: git, kubernetes
 cloud:
   config:
     server:
       git:
         uri: file:///path/to/git/repo
         order: 2

This should work fine I think

@arjavdongaonkar
Copy link
Contributor Author

This should work fine I think

Sure, Will check this out

  • As far as I know, the kubernetes profile is automatically included when the config server runs in a Kubernetes cluster, so there’s no need to explicitly define it in the spring.profiles.include / active property. This beahviour is due to this code
  • The main advantage of using composite is the ability to define multiple instances of a profile.
  • For example, my use case requires multiple Vault instances (application and infra) with different keys, along with Kubernetes and Git sources:
spring:
  profiles:
    active: composite
  cloud:
    config:
      server:
        composite:
          - type: kubernetes
          - type: vault
            defaultKey: application
          - type: vault
            defaultKey: infra
          - type: git
    kubernetes:
      secrets:
        enableApi: true
  • Additionally, This configuration allows me to control the order of property sources.
  • The above setup is working as expected in my environment and use case with code of this PR.

Do we have a specific reason to prefer the second approach over the composite model?

@ryanjbaxter
Copy link
Contributor

Sure, Will check this out

Please add a test for this

As far as I know, the kubernetes profile is automatically included when the config server runs in a Kubernetes cluster, so there’s no need to explicitly define it in the spring.profiles.include / active property. This beahviour is due to this code

I missed this in your tests but now I see it, sorry.

Do we have a specific reason to prefer the second approach over the composite model?

No but both are valid and wanted to make sure it is tested.

@arjavdongaonkar
Copy link
Contributor Author

Hey @ryanjbaxter

While writing a test case for Scenario 2 of the composite configuration (comma-separated active profiles), I found that the composite environment repository sorts the repositories based on the order property. However, in the case of the Kubernetes environment repository, the ordering was not happening because we never defined an order property for it.

Taking reference of similar profile types in Spring Cloud Config Server (e.g., Vault, Native, Git ), I found that there, we pass the order property via properties when creating their environment repository beans. However, the Kubernetes environment repository bean was missing this argument.

I initially considered accepting order as an environment variable and injecting it using @Value, but this felt like a workaround.
For a consistent approach and proper ordering, I have:

  1. Introduced an KubernetesConfigServerProperties argument in the Kubernetes environment repository bean (following the pattern used by other profiles).
  2. Added relevant test cases to verify the expected ordering behaviour.

Let me know if you have any concerns or if you'd like to discuss alternative approaches!

Signed-off-by: Arjav <[email protected]>
Signed-off-by: Arjav <[email protected]>
Signed-off-by: Arjav <[email protected]>
Signed-off-by: Arjav <[email protected]>
Signed-off-by: Arjav <[email protected]>
Signed-off-by: Arjav <[email protected]>
Signed-off-by: Arjav <[email protected]>
…e composite setup

KubernetesEnvironmentRepository as condition for KubernetesEnvironmentRepositoryFactory bean creation

Signed-off-by: Arjav <[email protected]>
…e composite setup

KubernetesEnvironmentRepository as condition for KubernetesEnvironmentRepositoryFactory bean creation

Signed-off-by: Arjav <[email protected]>
Signed-off-by: Arjav <[email protected]>
@ryanjbaxter ryanjbaxter merged commit 2b9709b into spring-cloud:3.1.x Feb 23, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configuring Secrets Priority in propertySources
4 participants