Repository navigation
Try the next config server URI after an unreadable response - #3346
sharanggupta wants to merge 2 commits into
Conversation
When a config server is secured with form login and the config client
sends missing or wrong credentials, the server redirects the request to
its HTML login page. The client then fails with an
UnknownContentTypeException ("no suitable HttpMessageConverter found
... content type [text/html]"), which does not say what is wrong.
Both the ConfigData loader and the bootstrap property source locator now
turn that exception into an IllegalStateException whose message names the
config server URI, the status and the content type. For an HTML response
it says that a secured config server answers with its login page when the
credentials are missing or wrong and names spring.cloud.config.username
and spring.cloud.config.password. For any other content type it asks to
check that spring.cloud.config.uri points at a config server.
Fixes spring-cloudgh-1723
Signed-off-by: Sharang Gupta <sharang@sharanggupta.dev>
| static String unreadableResponseMessage(String uri, UnknownContentTypeException ex) { | ||
| String message = "Could not read the response of the config server at " + uri + " (status " + ex.getStatusCode() | ||
| + ", content type " + ex.getContentType() + "). "; | ||
| if (MediaType.TEXT_HTML.includes(ex.getContentType())) { |
There was a problem hiding this comment.
This assumption may not be valid. We could get an HTML response for other reasons. We should remove this
There was a problem hiding this comment.
Removed in 44a0fb5. The client no longer guesses why a config server answered with HTML.
| * @return the message | ||
| */ | ||
| static String unreadableResponseMessage(String uri, UnknownContentTypeException ex) { | ||
| String message = "Could not read the response of the config server at " + uri + " (status " + ex.getStatusCode() |
There was a problem hiding this comment.
Do not print the URL in a message that can be logged
There was a problem hiding this comment.
Removed in 44a0fb5. None of the new messages include the URI.
| } | ||
| } | ||
| catch (UnknownContentTypeException e) { | ||
| throw new IllegalStateException(unreadableResponseMessage(uri, e), e); |
There was a problem hiding this comment.
This new exception type is not handled in multi-uri configurations, we would want to try the next uri in the list if this is thrown and not stop at the first exception
There was a problem hiding this comment.
I am also not sure IllegalStateException makes sense, I think we could just throw a new UnknownContentTypeException since that is really the case here
There was a problem hiding this comment.
Done in 44a0fb5. With multiple-uri-strategy: always, an unreadable response now moves on to the next URI, like a 4xx or 5xx response, in both the ConfigServerConfigDataLoader and the bootstrap ConfigServicePropertySourceLocator, and the tests cover both strategies.
There was a problem hiding this comment.
Agreed. The last URI now throws the UnknownContentTypeException itself. Its constructors do not take a message, so a new instance would have the same message as the original, and rethrowing the original also keeps its stack trace and response body.
Following the review, drop the message that pointed at the credentials for an HTML response, since a config server can send HTML for other reasons, and that put the URI of the config server in a message that can be logged. Instead, with the ALWAYS multiple URI strategy, a response that is not an Environment now moves on to the next URI, like a 4xx or 5xx response, in both the ConfigData loader and the bootstrap locator. The last URI rethrows the UnknownContentTypeException as before. Signed-off-by: Sharang Gupta <sharang@sharanggupta.dev>
sharanggupta
changed the title
This branch has not been deployed
3 participants
See #1723
When a config server answers with a body that the client cannot read as an
Environment, such as an HTML page,RestTemplatethrows anUnknownContentTypeException. Before this change, that exception stopped the client at the first URI, even withmultiple-uri-strategy: always, while a 4xx or 5xx response moved on to the next URI.With this change,
ConfigServerConfigDataLoaderand the bootstrapConfigServicePropertySourceLocatortry the next URI after an unreadable response when the strategy isalways, and log it at INFO like the other failures. The last URI, or any URI withconnection-timeout-only, rethrows theUnknownContentTypeExceptionas before, so fail fast and optional imports behave as they did.Following the review, the first version's message, which pointed at the credentials for an HTML response and included the config server URI, is gone.
Testing
Test first: the
alwaystests failed onmainwithConfigClientFailFastException, because the client stopped at the first URI.ConfigServerConfigDataLoaderTestsandConfigServicePropertySourceLocatorTests: the first URI answers200withtext/html. Withalways, the second URI is used. Withconnection-timeout-only, fail fast reports theUnknownContentTypeExceptionas the cause../mvnw -pl spring-cloud-config-client verifypasses, including Checkstyle and the Spring Java Format check.