Configurable SSL Logback extensions can't be supported due to URL checks #22946
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
spring-projects-issues
added
the
status: waiting-for-triage
|
Thanks for the proposal. I'm a little wary of making of Log4j2-specific change in a class that is used with any With the change in place, it changes to the following: The former is arguably better as it gets straight to the root cause rather than it being buried a few causes down. Let's see what the rest of the team think. If we think the change in diagnostics is important we may be able to do something to improve them and implement the proposed change. |
|
Perhaps I shouldn't have put this in terms of being Log4j 2 specific as it is not. It is just the first to provide support for external configurations. If the Spring Cloud Config team wanted to provide a similar enhancement for Logback (or this team decided to do it) they would be unable as the line of code in question prevents anyone from accessing a url that requires authentication. In other words, this single line of code is preventing anyone from ever placing their logging configuration in SCC if it requires authentication. Why would you want that? |
|
See similar PR in spring cloud commons spring-cloud/spring-cloud-commons#809 |
|
The |
|
I think it could be argued that the If we're worried about the stack trace, we could search for |
philwebb
added
type: enhancement
|
Any further update on this? |
@rgoers as shown in the history, the issue has been triaged as an enhancement in the |
|
Oh, I'm not sure why it would be considered an enhancement but I guess I will take what I can get. The problem is that user's of Log4j 2 who want to use this feature (myself included) can't because of this and there is no way to workaround the problem. Even if you try to create your own version of the class Spring Cloud will create a new SpringApplication that uses the original version and it will fail there. I consider it a bug and would have liked it applied to 2.2 and 2.3. |
philwebb
added
the
for: team-attention
philwebb
added
type: bug
|
I can see the argument for making this a bug. We'll take a look at doing it for 2.2.x. |
|
Thanks, Please see #23053 for an alternate way this could be handled. |
philwebb
changed the title
philwebb
changed the title
philwebb
changed the title
philwebb
pushed a commit
that referenced
this pull request
Aug 28, 2020
Remove `ResourceUtils.getURL` checking from `LoggingApplicationListener` so that logging systems can implement custom location support. Prior to this commit, we checked in the listener if the specified config location could be opened as a URL. This unfortunately prevents Log4J extensions such as `log4j-spring-cloud-config-client` from implementing configurable SSL and credentials support. See gh-22946
philwebb
added a commit
that referenced
this pull request
Aug 28, 2020
Extend `initializeSystem` to search the exception stack for a FileNotFoundException before reporting the error. This allows us to provide a similar stack trace to the one that used to be thrown when we had the `ResourceUtils.getURL` check. See gh-22946
|
@rgoers I've gone with your first approach in the end since it seems like the easier one to apply. I've added some additional logic to try and find the Thanks for the PR! |
philwebb
added a commit
that referenced
this pull request
Aug 28, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Log4j 2 provides enhanced Spring Boot and Spring Cloud Config support with its log4j-spring-boot and log4j-spring-cloud-config-client modules. The Spring Cloud Config Client allows Log4j configuration files to be managed in Spring Cloud Config allowing for dynamic updating of the configuration, configurable SSL support, and support for credentials to access Spring Cloud Config server.
Unfortunately, Spring Boot's LoggingApplicationListener makes it impossible to provide the configurable SSL and credentials support since it needlessly tries to access the configured URL ostensibly to determine if the URL is valid. This check really provides no value since the underlying LoggingSystem will throw an exception if the url cannot be accessed, and because it does not include credentials it results in a 401 response that prevents the Spring Boot application from starting if credentials are required to access the Spring Cloud Configuration server.