Closed marnee01 closed 4 years ago
My half-baked proposed workaround does not appear as if it will work. I can't envision any workaround for this.
The reason is that the lookupHttpClientBuilder
method does not track which repo entry in the bootstrap was matched. It just maintains a list of all the repos that were configured and tries to do a string match against all of them. So, no matter where I specify the uri with the {application} placeholder, it seems lookupHttpClientBuilder
will always match against that and find multiple URIs for any application that has its own git repo definition.
Shouldn't the lookupHttpClientBuilder
method honor the repo configuration in the bootstrap? It seems like a design flaw to me.
A workaround will probably involve creating your own implementation.
Then you need to create a bean that looks like
@Bean
@Primary
public ConfigurableHttpConnectionFactory myHttpClientConnectionFactory() {
return new MyCustomHttpClientConfigurableHttpConnectionFactory();
}
Thanks for the quick response! Yes, it's kind of looking like that is the only option.
Is it feasible to override with this custom bean? I don't see how to inject my own bean when EnvironmentRepositoryConfiguration creates MultipleJGitEnvironmentRepositoryFactory.
It's just a bean AFAICT
Got it. Thought there was something special about that one for some reason.
Workaround Info
As suggested, I was able to work around this by injecting my own implementation of the ConfigurableHttpConnectionFactory
interface.
For anyone else looking for a workaround, here are some details:
ConfigurableHttpConnectionFactory
is a copy of the HttpClientConfigurableHttpConnectionFactory
with lookupHttpClientBuilder
modified. Instead of trying to match against any URL, including those with placeholders, it first searches for an exact match. Only if no exact match is found does it then consider urls with placeholders.EnvironmentRepositoryConfiguration.JGitHttpClientConfig
. @Primary
@Configuration
@Import({EnvironmentRepositoryConfiguration.class})
spring.main.allow-bean-definition-overriding: true
I don't know if my updated HttpClientConfigurableHttpConnectionFactory
could be considered for a permanent fix. It may be that for some teams, finding both an exact match and a placeholder match should actually be treated as an error instead of ignoring the placeholder as my implementation is doing.
Note in case it helps: I found a pull request which describes why this class is treating multiple matches as an error, but it looks like the intention was to catch the case where there were matches against multiple URLs that had placeholders (not one exact match and one placeholder match).
Should I attach my implementation of the connection factory? (It hasn't gone through code review or thorough testing yet, though all my local testing passed.)
I forgot to list this step, which is needed for the Workaround:
spring.factories
file.I created a sample project demonstrating the issue: https://github.com/marnee01/vts-spring-config-server-1480
Instructions are in the README file at the root of the project.
Note that although the effect of the error is that proxy settings are not being read from the bootstrap file, I can’t demonstrate that, since putting our proxy settings in that sample app won’t work of course. But this project does replicate the underlying problem.
(I also have submitted a support ticket and send the link to the project on that ticket.)
@marnee01 since your workaround above fixes the issue would you be able to submit a PR with the workaround so we can incorporate the fix?
The workaround is not a fix, it's just a workaround. It wouldn't fix https://github.com/spring-cloud/spring-cloud-config/issues/1482, which is closely related.
@marnee01
You stated that
My implementation of the ConfigurableHttpConnectionFactory is a copy of the HttpClientConfigurableHttpConnectionFactory with lookupHttpClientBuilder modified. Instead of trying to match against any URL, including those with placeholders, it first searches for an exact match. Only if no exact match is found does it then consider urls with placeholders.
What I am suggesting is that the PR basically modify HttpClientConfigurableHttpConnectionFactory.lookupHttpClientBuilder
with the logic from your workaround. Would that not fix this issue?
1482 is a separate issue so lets not combine the two
They have the same underlying cause, though.
It would fix this issue for me. As I said above:
I don't know if my updated HttpClientConfigurableHttpConnectionFactory could be considered for a permanent fix. It may be that for some teams, finding both an exact match and a placeholder match should actually be treated as an error instead of ignoring the placeholder as my implementation is doing.
And I wrote the above before I realized there was a more fundamental issue with the design of the class, which is why this issue is related to the one documented in #1482.
I won't send a pull request, because I'm not going to get set up with the project, figure out maven, figure out how to test the changes that are made directly to the HttpClientConfigurableHttpConnectionFactory class, etc. for something that appears to me to be a band-aid. I don't want to be responsible for breaking someone else's software if they consider this "fix" to instead be a "regression".
I've attached my file. You can do a "diff" against HttpClientConfigurableHttpConnectionFactory to see what I changed.
Closed via c7ab0f3
We recently tried to upgrade from Edgware.SR5 to Greenwich SR3. ConfigServer reports the following error for certain requests:
The error is coming from the
HttpClientConfigurableHttpConnectionFactory.lookupHttpClientBuilder
method. With Greenwich SR3, a bugfix was implemented in that class based on a support ticket my team opened. However, it appears it was only a partial fix.We have a bootstrap similar to this:
The problem is that if you call configserver for application "webapp", the
lookupHttpClientBuilder
method finds both uris shown in the sample bootstrap above. ThelookupHttpClientBuilder
expected to find one and only one. The service will continue on to process the request, but logs error:We need to use the proxy, so this won't work for us.
This did not happen with Finchley. (The
HttpClientConfigurableHttpConnectionFactory
class did not exist in that release.)1) I wanted to first relay the issue and request that it be fixed. 2) Short-term, I am trying to find a workaround. I am hoping you can help. We have a strong need to upgrade to the latest release very soon.
Workaround: For a workaround, I tried adding a default repo entry to the end of the bootstrap and removing the default git entry, as shown below. Unsurprisingly, the service won't even start if you don't have
spring.cloud.config.server.git.uri
defined. So this did not work:We could add the
spring.cloud.config.server.git.uri
back in to point to just one repo instead of using {application} placeholder and have theDefault
for everything else. But for that one repo specified onspring.cloud.config.server.git.uri
, we'll get the error, so that won't work. We could put a dummy repo in forspring.cloud.config.server.git.uri
, but that is too much of a hack for a long-term solution.Can you think of a way for us to configure repos so that only one repo url is ever found? Is there a way to specify the
Default
repo as in my example, but in addition topattern
, add an exclusion to exclude the repo present as the default? Something like the example below. In that example, note:The
spring.cloud.config.server.git.uri
is set to vvv_configs-authenticationAccounts instead of vvv_configs-{application}The
Default
repo has apatternExclude
(which isn't a real parameter).Can
pattern
be regex, or is asterisk for wildcard the only matching that is supported?Note that eliminating having a token in the uri (vvv_configs-{application}) and explicitly defining a repo entry for every service and app is not a feasible workaround for us. We have many dozens (100s?) of apps and services. It's not feasible to add a repo entry into the bootstrap for each of them and also have to continue adding for every new service we create.