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

Enable Spring Config Server #226

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

rrileyca
Copy link

@rrileyca rrileyca commented Jun 27, 2022

Current Situation

Promregator does not support the Spring Cloud Config Server, which allows externalized configuration to be fetched from a service using HTTP requests.

Problem Statement

This PR simply adds an additional way to configure Promregator using a well known and supported method from the Spring project.

Solution Approach

Add the dependencies for the Spring Cloud Config Server to the pom.xml.

To make use of the Spring Cloud Config Server in Promregator, simply add the application property:

spring:
  cloud:
    config:
      uri: https://username:password@hostname.domain.com

If you are deploying Promregator to Cloud Foundry, you can simply add the environment variable:

applications:
  # omitted some elements for brevity...
  env:
    SPRING_CLOUD_CONFIG_URI: "https://username:password@config-server.domain.com"

@rrileyca
Copy link
Author

Hi @eaglerainbow, I see you're an active contributor to this project. Is it possible to merge this? It's a fairly small commit, and just adds another piece of the Spring project. Thanks!

@eaglerainbow
Copy link
Contributor

eaglerainbow commented Mar 18, 2023

Thanks for the contribution! I don't see anything speaking against your PR in principle. The point is that it requires some sort of integration testing as part of the delivery pipeline. Background to this is that in the past, I have made bad experience on this w.r.t. encrypted passwords and the spring cloud CLI.
Moreover, you also could consider this to be a feature that should be listed on the feature list in our main README.md. Perhaps I'll find some time later to tackle all this...
BTW: Spending a brief documentation page on this (already copying what is part of the description in this PR could be enough) would also be helpful.

@eaglerainbow eaglerainbow added the enhancement New feature or request (of code/functionality/quality - no documentation-only!) label Mar 18, 2023
@eaglerainbow
Copy link
Contributor

Would you also need this in the 0.x.y series?

@eaglerainbow
Copy link
Contributor

I am currently reading https://docs.spring.io/spring-cloud-config/docs/current/reference/html/#config-first-bootstrap. Looks like that they suggest a newer approach of inclusion:

spring.config.import=optional:configserver:https://username:password@hostname.domain.com

Is the difference intentional?

Comment on lines +86 to +89
<dependency>
<groupId>org.springframework.cloud</groupId>
<artifactId>spring-cloud-starter-bootstrap</artifactId>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

I am currently trying to wrap my head around this dependency. I think it refers to https://docs.spring.io/spring-cloud-config/docs/current/reference/html/#config-first-bootstrap.

If I get things right, then with this dependency, we would automatically being connecting to a localhost:8888 trying to fetch there. This is a somewhat incompatible behavior, because we don't know in which environment Promregator is running: It may be that other spring-boot applications are using a local config server, but because Promregator did not in the past, we would be suddenly taking configuration values from there, potentially causing confusion. If with bad luck, this could even end up in a security flaw being introduced (hopefully, this wouldn't happen, though).

Essentially, what is currently on my mind is to facilitate this feature, but to not have it enabled by default. I am currently asking myself, if the option of using Spring Boot Config Data Import isn't the better way.

@eaglerainbow
Copy link
Contributor

eaglerainbow commented Mar 18, 2023

Just found out by trying: Adding spring-cloud-starter-config as dependency leads to that spring-boot expects to have something set for spring.config.import. This also causes unit tests fail, if not disabled using spring.cloud.config.enabled=false. See also https://stackoverflow.com/a/72973229.

fwiw: I am currently playing around at https://github.com/promregator/promregator/tree/cloudConfigServer (cf https://github.com/promregator/promregator/compare/cloudConfigServer )

@eaglerainbow
Copy link
Contributor

eaglerainbow commented Mar 18, 2023

It's 4 lines to add the dependency, but it's another 360 lines to test/ensure that you don't break delivery (with config support enabled and disabled). See also https://github.com/promregator/promregator/tree/68acae0f7e06110211c85c0a54efc74a4362d9c7 .

If we can agree on this newer spring cloud config server configuration approach, I think "only" documentation and mentioning in the feature list would be missing.

@rrileyca
Copy link
Author

Hi @eaglerainbow and thanks for your reply. I must admit I hadn't noticed the new Spring Boot property for Config Server. I will test this shortly. The JAR has been in my applications since before Spring 2.4.

I am happy to add documentation to the PR, though if the new method of just adding a property to a Spring Boot app works without adding a maven dependency, I am also happy to close this PR. Let me know if you'd still like the feature mentioned in docs, I can do that.

@eaglerainbow
Copy link
Contributor

though if the new method of just adding a property to a Spring Boot app works without adding a maven dependency, I am also happy to close this PR.

I think you are getting a little ahead of yourself 😁
You still will need spring-cloud-starter-config in your dependencies, but (at least as far as I get things) spring-cloud-starter-bootstrap should no longer be used. If you removed spring-cloud-starter-config from the pom.xml, then you run into a

java.lang.IllegalStateException: Unable to load config data from 'configserver:'
...
Caused by: java.lang.IllegalStateException: File extension is not known to any PropertySourceLoader. If the location is meant to reference a directory, it must end in '/' or File.separator

which I take that the HTTP interface for accessing the spring cloud config server in the client isn't available.

Let me know if you'd still like the feature mentioned in docs, I can do that.

I am currently for doing it. However, please note that this PR here is deprecated. I think, we should continue on my branch where also integration tests are included (which just helped perfectly fine to cross-check whether spring-cloud-starter-config was really needed or not).

Do you want to take the documentation stuff up from there?
If yes, I'd recommend to have a look at

java -Dspring.cloud.config.enabled=true -Dspring.config.import=configserver: -jar target/promregator-${params.currentVersion}.jar &
which demos how I got this thing up flying from a user's perspective.

@eaglerainbow
Copy link
Contributor

eaglerainbow commented Mar 18, 2023

Eventually, my plan would be to merge my branch into master (either directly or via PR, I don't know yet).

Ah, and one more thing:

Would you also need this in the 0.x.y series?

Do you have an opinion about that?

@rrileyca
Copy link
Author

If you're referring to the 0.x.y release version of Promregator then no, I personally use latest. I don't have an opinion either way.

@eaglerainbow
Copy link
Contributor

If you're referring to the 0.x.y release version of Promregator then no, I personally use latest. I don't have an opinion either way.

Okay, then let's keep this on 1.0.x for the time being.

@eaglerainbow eaglerainbow added author action Further information is requested awaiting feedback We are waiting for one of the participants to provide feedback (on the solution) labels Apr 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author action Further information is requested awaiting feedback We are waiting for one of the participants to provide feedback (on the solution) enhancement New feature or request (of code/functionality/quality - no documentation-only!)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants