-
Notifications
You must be signed in to change notification settings - Fork 41.1k
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
Fail fast if management base path and an endpoint mapping are both '/' on the main server port #43885
Comments
Thanks for the report. Unfortunately, I don't think I understand the problem as currently described. Your snippet shows the use of Also, please note that Spring Boot 3.2.x is no longer supported. Any sample that you share with us should use 3.3.7 or 3.4.1 to demonstrate the problem. |
this, precisely.
Sure thing. Gimme a sec.
Sure, no problemo, as this problem also affects 3.4.1; basically, everything higher than 3.2.1 or 3.1.7 (including >=3.1.8+, >=3.2.2, >=3.3.0 and >= 3.4.0) is affected. |
spring-boot-actuator-vs-restcontroller-problem-43885-main.zip Here you go mate :) Ping me if anything else is missing, but I guess this is as MCVE as it gets :) |
Thank you. The problem's occurring because there's a clash at the path level between the mappings for the health endpoint and the mapping for your
The combination of a patch match and a method mismatch is detected in I'd like to discuss this with the rest of the team as web request matching is a complex area. I may well be overlooking a risk in overriding |
We discussed this today and concluded that it wasn't a good idea. Returning The root of the problem here is that there are multiple mappings capable of handling the path If we do anything here it will be to try to detect overlapping mappings and to fail fast at startup but will need to be careful not to fail to eagerly and make things unnecessarily brittle. To that end, @FyiurAmron, can you please describe what your end goal was when mapping actuator and health to |
I think this would be completely acceptable, I agree that this would be a reasonable solution here. To reiterate, my point then would be that
To reiterate
, the end goal here was: have
I.e. it says that Frankly, current implementation prevents having anything actuator-related to be exposed on That aside, a question: should an actuator path mapping consume all the requests that are underneath it (via |
We can't easily do something like this with our current design since we let Spring MVC decide which mapping to invoke. This makes more sense when you consider content negotiation. I'm still not sure I've completely understood why you'd want to have health at the root of your application in addition to other |
I understand. I still find the current behaviour surprising, to say the least.
Most (ca. 95%) of apps in our domain have no good
In my defense - I didn't anticipate this being an unanticipated code setup either, I wouldn't have gone with it if I expected it to break in a year or two when it worked perfectly for many years back :)
Yeah, TBH I'm okay with just a redirect or transclusion of the healthcheck result, so this doesn't really have a direct business impact ATM (we already have a root cause, we already handled the mitigation of impact by changing the paths to defaults etc.). Like I said, having a fail-fast here is completely OK with me - if this config is considered an abuse of the system and thus doesn't work and won't work in the future (regardless if it's the quirk of particular config, quirks of Spring MVC or whatnot), I strongly believe it should just explicitly fail during actuator init, instead of suppressing the endpoints. Simple check along the lines of
(please excuse the pseudo-code :) would really do the trick here IMVHO. Detecting more complex cases might be brittle, as @wilkinsona pointed out - however just checking for this particular case in the actuator init would save some people some sleepless nights later on :) |
MVC's decision can be influenced by changing the ordering of the handler mappings. At the moment the various @FyiurAmron you can change this order using a
This will order the Actuator endpoints towards the end, immediately before the mapping for static resource handling. Note that your mapping of Actuator and its health endpoint to |
@wilkinsona a really good idea for a low-level workaround. TBH though, I think that taking everything mentioned here into account, going for the |
Yeah, I think you're right. I agree that a redirect is the best way to handle this as it avoids a variety of problems with potential clashes between various mappings. As it sounds like you have a way forward that we think should prove to be robust in the longer-term, I'll close this one. |
@wilkinsona would it be possible to have a fail-fast, or at least a warning, for this particular setting? I believe it's very unlikely that anyone wants to have RestController functionality broken intentionally, and debugging this case fully is really problematic due to all the complexities that were mentioned here :) |
I've repurposed this issue, but I'd like some discussion with the team to make sure there aren't unexpected side effects and to decide when we should do it. |
So, I faced this issue in a rather different way. Hoping to add to the discussion. So I have an application that runs spring cloud gateway. It's deployed with a custom helm chart, but because of some issues with the cloud environment (potentially a bug), the cloud provider doesn't respect my custom health point configured on the helm. As a result I am forced to use their default health endpoint which is /. After recently upgrading Spring Boot 3.3.7, the cloud gateway simply stopped matching all my predicates. Given my limited knowledge of reactive programming, it was difficult for me to figure this out. But this seems like the same issue. The issue with our gateway setup was WebFluxEndpointHandlerMapping is (as before) being executed before RoutePredicateHandlerMapping. So effectively all my calls were being mapped to the health endpoint with all the route configurations on cloud gateway being completely ignored. I was simply being served 405s. I am still working on a solution, which will either involve using a different port for hosting the management endpoints or as suggested here reordering the mapping order using a "BeanPostProcessor". I thought I'll add this comment because it's another failure mode. Also, I am also limited by my cloud setup to use the / endpoint as health check. (At least until that issue is fixed). Not sure if failing fast is the way to go, but a warning is certainly in order. |
Well, you can't access any RestControllers if you configure the healthcheck this way and that is completely opaque case ATM (you know neither that it will fail, when it fails or why it fails until you read this thread :D), that's why I think
for just |
Yeah. So what I meant is, with the current config, with reordering of the marchers we can get it to work. But if we start failing such configurations the work around (with the reorder) probably won't work either. But I can completely appreciate that this is a very specific issue with me where I am tied down to the / endpoint for health check at the moment. I have no qualms with that. BTW I actually figured it out by debugging spring cloud gateway for hours actually. I am embarrassed to admit how many hours. :D Then I checked the commit history on WebFluxEndpointHandlerMapping, then through to discussions I got here. |
true. The ordering can be detected by the failfast code IIUC so that the fail only happens if actuator has higher ordering than user-defined endpoints. I agree that an unconditional fail is problematic, as you pointed out.
Don't be. It has taken me a lot of time - and a production cluster**** in my company alongside that due to inaccessible customer endpoints - to figure this one too... and that is probably why I'm so eager to have this handled by the lib somehow, with exception/warning paths I mean. |
Impossible for me to say if this is a bug or a feature/documented change, hence the ticket. Facts:
test snippets:
results for 3.2.1:
results for 3.2.2:
Further details:
health
from/
to e.g./foo
"fixes" the behaviour (i.e. the endpoints start to work again)base-path
to anything else than/
(e.g./foo
) in this case triggers:Ambiguous mapping. Cannot map 'Actuator root web endpoint' method Actuator root web endpoint to {GET [/foo], produces [application/vnd.spring-boot.actuator.v3+json || application/vnd.spring-boot.actuator.v2+json || application/json]}: There is already 'Actuator web endpoint 'health'' bean method
My take on it is:
base-path
and at least one of the mappings equal to/
) is deemed invalid, it should throw an exception during bean init for this particular case, like the case with non-/
base and/
endpoint. However, the valid use case for this was hiding the "root" actuator endpoint AFAIK.With all honesty, I'd say this is a bug, mainly due to backwards-compatibility reasons. The code in question did work in previous patches in the same major/minor release (AFAIK both 3.2 and 3.1 are affected in this way), and a patch shouldn't introduce breaking changes that ain't backwards-compatible even if they are just fixing some abuses or corner cases (and the code shown isn't even really one of those I'd argue). If it would introduced purely with a minor increase and clearly documented, only the lack of exception thrown would be a problem here.
The text was updated successfully, but these errors were encountered: