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

Fail fast if management base path and an endpoint mapping are both '/' on the main server port #43885

Open
FyiurAmron opened this issue Jan 20, 2025 · 13 comments
Labels
for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged

Comments

@FyiurAmron
Copy link

FyiurAmron commented Jan 20, 2025

Impossible for me to say if this is a bug or a feature/documented change, hence the ticket. Facts:

test snippets:

@RestController
@RequestMapping("/api")
class TestController {

    @PostMapping("endpoint")
    fun endpoint(): HttpEntity<*> {
        return ResponseEntity.EMPTY
    }

}
management:
  endpoints.web:
    base-path: "/"
    path-mapping:
      health: "/"

results for 3.2.1:

GET -> 405
POST -> 200
PUT -> 405

results for 3.2.2:

GET -> 404
POST -> 405
PUT -> 405

Further details:

  • The config shown doesn't throw any exception and never did.
  • I also didn't notice any kind of warnings emitted with it (went through logs with DEBUG log level)
  • Changing health from / to e.g. /foo "fixes" the behaviour (i.e. the endpoints start to work again)
  • Changing 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:

  • If this config (i.e. both 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.
  • If this config is deemed valid, the RestController should still work when that config is used.

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.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 20, 2025
@FyiurAmron FyiurAmron changed the title possible bug with accessing RestController endpoints due to #35426 bug with accessing RestController endpoints due to #35426 ? Jan 20, 2025
@wilkinsona
Copy link
Member

wilkinsona commented Jan 20, 2025

Thanks for the report. Unfortunately, I don't think I understand the problem as currently described. Your snippet shows the use of @RestController which is separate from the Actuator endpoint infrastructure. Perhaps you meant @RestControllerEndpoint or perhaps you are saying that the Actuator and its health endpoint being mapped to / prevents access to a standard @RestController endpoint on /api? To help to remove this confusion, please provide a complete yet minimal sample that reproduces the problem. You can share it with us by pushing it to a separate repository on GitHub or by zipping it up and attaching it to this issue.

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.

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Jan 20, 2025
@FyiurAmron
Copy link
Author

FyiurAmron commented Jan 20, 2025

perhaps you are saying that the Actuator and its health endpoint being mapped to / prevents access to a standard @RestController endpoint on /api?

this, precisely.

To help to remove this confusion, please provide a complete yet minimal sample that reproduces the problem. You can share it with us by pushing it to a separate repository on GitHub or by zipping it up and attaching it to this issue.

Sure thing. Gimme a sec.

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.

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-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 20, 2025
@FyiurAmron
Copy link
Author

FyiurAmron commented Jan 20, 2025

@wilkinsona

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 :)

@wilkinsona
Copy link
Member

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 @RestController:

2025-01-21T10:13:12.592Z TRACE 61191 --- [           main] s.w.s.m.m.a.RequestMappingHandlerMapping : 
	t.v.b.Controller:
	{POST [/api/endpoint]}: endpoint()
…
2025-01-21T10:13:12.732Z TRACE 61191 --- [           main] s.b.a.e.w.s.WebMvcEndpointHandlerMapping : Register "{GET [/**], produces [application/vnd.spring-boot.actuator.v3+json || application/vnd.spring-boot.actuator.v2+json || application/json]}" to java.lang.Object org.springframework.boot.actuate.endpoint.web.servlet.AbstractWebMvcEndpointHandlerMapping$OperationHandler.handle(jakarta.servlet.http.HttpServletRequest,java.util.Map<java.lang.String, java.lang.String>)
2025-01-21T10:13:12.733Z TRACE 61191 --- [           main] s.b.a.e.w.s.WebMvcEndpointHandlerMapping : Register "{GET [/ || ], produces [application/vnd.spring-boot.actuator.v3+json || application/vnd.spring-boot.actuator.v2+json || application/json]}" to java.lang.Object org.springframework.boot.actuate.endpoint.web.servlet.AbstractWebMvcEndpointHandlerMapping$OperationHandler.handle(jakarta.servlet.http.HttpServletRequest,java.util.Map<java.lang.String, java.lang.String>)

WebMvcEndpointHandlerMapping is called first by the dispatcher servlet. The path match combined with the HTTP method mismatch results in a HttpRequestMethodNotSupportedException being thrown and a 405 response. This doesn't occur in 3.2.1 because of a bug in how to Actuator created its mappings with the health endpoint being mapped to //** instead of /**. This causes the path match to be missed.

The combination of a patch match and a method mismatch is detected in handleNoMatch in RequestMappingInfoHandlerMapping from where the HttpRequestMethodNotSupportedException is thrown. We may be able to accommodate mappings for both POST [/api/endpoint] and GET [/**] by overriding handleNoMatch in AbstractWebMvcEndpointHandlerMapping and returning null. This would then give subsequent handler mappings an opportunity to provide a match. Spring Data REST's RepositoryRestHandlerMapping does exactly this, for similar reasons I would guess.

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 handleNoMatch to return null.

@wilkinsona wilkinsona added for: team-attention An issue we'd like other members of the team to review for: team-meeting An issue we'd like to discuss as a team to make progress and removed status: feedback-provided Feedback has been provided for: team-attention An issue we'd like other members of the team to review labels Jan 21, 2025
@wilkinsona
Copy link
Member

We may be able to accommodate mappings for both POST [/api/endpoint] and GET [/**] by overriding handleNoMatch in AbstractWebMvcEndpointHandlerMapping and returning null

We discussed this today and concluded that it wasn't a good idea. Returning null would mean that a request to a path that maps to an Actuator endpoint with an HTTP method that isn't supported will respond with a 404 rather than the more helpful and more correct 405.

The root of the problem here is that there are multiple mappings capable of handling the path /api/endpoint. Those are the health endpoint's /** mapping and the REST controller's /api/endpoint mapping. There is also ambiguity within the Actuator itself if other endpoints are exposed as well as the health endpoint because the health endpoint's /** mapping will overlap with their mappings. With the current implementation using Spring MVC, this overlap will prevent access to a specific health component when its name clashes with an endpoint's ID. For example, if there's a health component called flyway and the flyway endpoint is exposed, the flyway health component would be hidden and only the endpoint would be accessible.

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 /?

@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed for: team-meeting An issue we'd like to discuss as a team to make progress labels Jan 22, 2025
@FyiurAmron
Copy link
Author

FyiurAmron commented Jan 22, 2025

@wilkinsona

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.

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

[if this config] is deemed invalid, it should throw an exception during bean init for this particular case [and be documented as invalid - see below]

can you please describe what your end goal was when mapping actuator and health to /?

To reiterate

the valid use case for this was hiding the "root" actuator endpoint AFAIK.

, the end goal here was: have / being mapped to health endpoint, no other endpoint exposed, actuator root (discovery page aka endpoint list) not exposed, other regular RestControllers etc. endpoints accessible. AFAIK, this required the actuator base path to be / and the health actuator endpoint path to be / as well (and required disabling other actuator endpoints for this case precisely), and the only way of handling that pre-3.2.2 known to me was this one shown here. Someone requested this function on SO even before me:
https://stackoverflow.com/questions/72939126/how-to-point-a-single-spring-actuator-endpoint-to-the-root
- and the docs hinted that this is actually supported, see https://docs.spring.io/spring-boot/reference/actuator/endpoints.html#actuator.endpoints.hypermedia (docs for 3.4.1) e.g.

When the management context path is set to /, the discovery page is disabled to prevent the possibility of a clash with other mappings.

I.e. it says that base-path: / is a valid and expected way to disable discovery page. Then, there is no discovery page, but the endpoints are still accessible as /fooendpointname. Thus, the only way to have no discovery page and healthcheck accessible at / would be / + / config, like shown. Arguable whether it's an abuse of the feature or a valid use case - but it did work for many years before, and the use case (have healthcheck at /) was actually quite simple and reasonable IMVHO - e.g. many gRPC apps don't have a "real" HTTP interface, and the only HTTP thing they require is the healthcheck for k8s etc. - but having a 404 on / is problematic for many reasons (path semantics mostly, but also observability, POLA, you name it). Having just a healthcheck on / is then IMVHO the most natural way of handling things if one doesn't want to expose discovery (e.g. for security or simplicity).

Frankly, current implementation prevents having anything actuator-related to be exposed on / (path root, single slash) - because setting base path as / hides the discovery - and #35426 prevents exposing anything else from actuator on it. Previously, it was the (documented) behaviour was to hide discovery on /, but to allow to map a single endpoint there instead.

That aside, a question: should an actuator path mapping consume all the requests that are underneath it (via /**) before considering e.g. normal RestControllers? IMVHO, actuator should do matching after all the user-defined controllers are handled, not before - as explicit manual config should, as a rule, override implicit defaults?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 22, 2025
@philwebb
Copy link
Member

That aside, a question: should an actuator path mapping consume all the requests that are underneath it (via /**) before considering e.g. normal RestControllers? IMVHO, actuator should do matching after all the user-defined controllers are handled, not before - as explicit manual config should, as a rule, override implicit defaults?

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 @RestControllers that are presumably mapped to sub-paths. This is certainly a setup that we didn't anticipate. I'm wondering if there's another way to support this. Perhaps your own @RestController for / that invokes the health endpoint directly.

@FyiurAmron
Copy link
Author

FyiurAmron commented Jan 22, 2025

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 understand. I still find the current behaviour surprising, to say the least.

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 @RestControllers that are presumably mapped to sub-paths.

Most (ca. 95%) of apps in our domain have no good / HTTP mapping because they are either gRPC-only services which require no business HTTP endpoints by themselves, or GraphQL apps which only expose /graphql path for business access. As such, 404 on / were the "usual" response, but caused disturbances for those unfamiliar with the setup (and I believe that was to be expected). I proposed moving the healthcheck to /, because that's the only actuator endpoint we use, and because it's basically all we needed those 95% of the time. They still work properly, although arguably they shouldn't, if you take into account what's been discussed in this ticket.
However, a single app required a RestController and used it - which wasn't a problem until SB v3.2.2; starting with the patch, it broke. That's the use case.

This is certainly a setup that we didn't anticipate.

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 :)

I'm wondering if there's another way to support this. Perhaps your own @RestController for / that invokes the health endpoint directly.

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

if ( base-path == "\" && path-mappings.any{ it == "\" } ) throw NotSupportedEx("Ambiguous mapping. Please don't :)");

(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 :)

@wilkinsona
Copy link
Member

wilkinsona commented Jan 23, 2025

That aside, a question: should an actuator path mapping consume all the requests that are underneath it (via /**) before considering e.g. normal RestControllers? IMVHO, actuator should do matching after all the user-defined controllers are handled, not before - as explicit manual config should, as a rule, override implicit defaults?

We can't easily do something like this with our current design since we let Spring MVC decide which mapping to invoke.

MVC's decision can be influenced by changing the ordering of the handler mappings. At the moment the various AbstractWebMvcEndpointHandlerMapping sub-classes are ordered at -100. This results in them being called quite early and before the handler mapping that deals with @RequestMapping.

@FyiurAmron you can change this order using a BeanPostProcessor:

@Bean
static BeanPostProcessor actuatorEndpointMappingOrderer() {
	return new BeanPostProcessor() {
		@Override
		public Object postProcessAfterInitialization(Object bean, String beanName) throws BeansException {
			if (bean instanceof AbstractWebMvcEndpointHandlerMapping endpointHandlerMapping) {
				endpointHandlerMapping.setOrder(Ordered.LOWEST_PRECEDENCE - 2);
			}
			return bean;
		}
	};
}

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 / will inhibit access to static resources (or anything else that's ordered after it). I'm not sure that this is something that we'd want to officially support as it's really relying on an implementation detail of the MVC-based endpoint infrastructure, but it may be sufficient to meet your needs.

@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jan 23, 2025
@FyiurAmron
Copy link
Author

FyiurAmron commented Jan 23, 2025

@wilkinsona a really good idea for a low-level workaround. TBH though, I think that taking everything mentioned here into account, going for the base-path:"/" with default endpoint settings and providing @GetMapping("/") RedirectView handleRoot() { return new RedirectView("/health"); } etc. might be more self-explanatory and fool-proof. Pity if there's no way to handle it otherwise, but at least it works and probably won't break on some random SB patch release later on :D

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 23, 2025
@wilkinsona
Copy link
Member

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 wilkinsona closed this as not planned Won't fix, can't repro, duplicate, stale Jan 24, 2025
@wilkinsona wilkinsona removed the status: waiting-for-triage An issue we've not yet triaged label Jan 24, 2025
@wilkinsona wilkinsona added status: declined A suggestion or change that we don't feel we should currently apply and removed status: feedback-provided Feedback has been provided labels Jan 24, 2025
@FyiurAmron
Copy link
Author

@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 :)

@philwebb philwebb reopened this Jan 24, 2025
@philwebb philwebb changed the title bug with accessing RestController endpoints due to #35426 ? Fail fast if management base path and an endpoint mapping are both '/' on the main server port Jan 24, 2025
@philwebb philwebb added status: waiting-for-triage An issue we've not yet triaged for: team-attention An issue we'd like other members of the team to review and removed status: declined A suggestion or change that we don't feel we should currently apply labels Jan 24, 2025
@philwebb
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

No branches or pull requests

4 participants