MicroProfile Metrics, Health, OpenAPI URI changed

[AdamBien]

Quarkus introduced: /q/ prefix for all non-application endpoints (/health, /metrics,/openapi) which is a not a conventional behaviour. The other MicroProfile runtimes behave as expected.

"/health", "/metrics", (...) endpoints accessed by MP Rest Clients / JAX-RS are currently broken,
because 301 is not honoured per default.

Quarkus should at least ship with the setting:

quarkus.http.non-application-root-path=/

The non-application endpoints could be shifted to a different location on-demand.

Expected behavior

Metrics, Health, OpenAPI URIs are exposed as specified in: https://download.eclipse.org/microprofile/microprofile-metrics-3.0/microprofile-metrics-spec-3.0.html#rest-api / https://download.eclipse.org/microprofile/microprofile-health-3.0/microprofile-health-spec-3.0.html#_response_codes_and_status_mappings

Actual behavior

/q is prepended

See also: https://adambien.blog/roller/abien/entry/quarkus_health_metrics_openapi_moved

To Reproduce

curl -i localhost:8080/health

Expected: 200

Actual: 301

[ghost]

/cc @EricWittmann, @MikeEdgar, @jmartisk, @phillip-kruger, @xstefank

[phillip-kruger]

@kenfinnigan

[maxandersen]

Thanks @AdamBien - just to check the two end points I see in microprofile spec referenced is /health and /metrics for health and metrics ...openapi is in https://download.eclipse.org/microprofile/microprofile-open-api-1.0/microprofile-openapi-spec.html

Now on the issue, the reason for moving to a /q prefix was two-fold:

1. we wanted a place where we could "safely" add endpoints that would not conflict with users own endpoints. i.e. we got /q/dev and i'm sure there will be more like that in the future.

2. multiple asks on being able to easily and securely separate "application endpoints" from "infrastructure/internal endpoints", i.e. in nginx configs you now just secure /q/* and everything else you let through.

Thats the reasons and #1 prevents (at least in the current form) us from having / as default instead of /q.

That said - the health and metrics are in a grey area here. One is the microprofile spec the other is that many health check and metric systems most likely don't honor 30x's - most likely because of not having activated redirection (an oversight) or they actually deliberately did not do it for performance reasons.

We do have an option available to remove /q/ but that then in its current form will also move all the other endpoints which would go against #1 above.

If Quarkus from day one have had check points at /q/health just like i.e. spring has a /actuator/health/live and we had a property to put them into the microprofile spec location I don't think we would have a problem, would we?

I see a few options:

0. Keep as is where the spec compliant endpoints are present but do 30x and everything is actually served into /q/

1. we change the default behaviour to have /health, /metrics and /openapi in microprofile spec specified locations, but everything else defaults on /q/ with an option to easily move all the spec compliant locations under /q/ to force them under one place.

2. we keep /q/ as default prefix and have a "follow microprofile spec" option to turn on effectively option 1.

3. we serve out the 3 spec endpoints both at / and /q/ by default with option to turn off.

Considering the two reasons why we introduced /q/ and the now new realisation of what effect 30x have (or rather don't have) if I could go back in time I think Option #1 is the best option, Option #3 the second best as I believe there might be a performance hit to do #3.

And I want to say it is (to me) not about microprofile spec compliance having to be the default in Quarkus but that existing users deployments/setups in production will fail to expose metrics and health checkpoints because those systems does not honour 30x redirects.

@kenfinnigan and @stuartwdouglas wdyt?

[jmartisk]

I think I'd vote for option 3 (if we make sure the performance hit is not too large and that applications trying to serve something on those endpoints receive priority).
Doing option 1 now would be a breaking change, and I assume some users have already adapted after the original breaking change.
And I don't like number 2 because following the spec sounds like something that should be done by default rather than opt-in (I know we've been diverging from this principle, but still).

[phillip-kruger]

I would also prefer 3. But maybe do it for all current non application endpoints, not just the 3 spec related onces ?
If we take those 3 (health, metrics, openapi) what is left ? swagger-ui and graphql-ui (not in prod mode by default) and dev-ui (can not be used in prod)

[maxandersen]

I would also prefer 3. But maybe do it for all current non application endpoints, not just the 3 spec related onces ?

That is not an option IMO, we do not want to constantly have to consider namespace collisions. We need a place to (fairly) reliably add endpoints. That is why we added /q/ to begin with.

We can't have a clean add of new items if we default every endpoint to the root(/).

[maxandersen]

And I don't like number 2 because following the spec sounds like something that should be done by default rather than opt-in (I know we've been diverging from this principle, but still).

The spec is really secondary in this issue. Its about effect on existing production rollouts.

[maxandersen]

I think I'd vote for option 3 (if we make sure the performance hit is not too large and that applications trying to serve something on those endpoints receive priority).

Yes, if there is no performance hit for production i'm +3 for option 3 too. @kenfinnigan raised the perf concern so I'll have to await his comment on what it really was about.

[kenfinnigan]

We can do option 3, the issue is the behavior would need to live in each of those extensions instead of a central location, but the config to turn it off would need to be centralized in Vert.x HTTP extension.

We'd need to clearly document in the extensions why it's done this way to prevent other non-application extensions from doing the same as Health, Metrics.

Would we still provide a redirect from / for things other than Health? If so, I'm not sure how we'd know to not add a redirect for a specific set of endpoints when a redirect is enabled.

[ebullient]

We use a builder for non-application endpoints.. can we make it an option? (additional endpoint or "also serve from root").

Something that would allow the general "non-application endpoint" handling to register the additional route, too.

[kenfinnigan]

Thinking it through I see the following:

• A flag on Vertx HTTP config as to whether "MP endpoints", for want of a better term right now, should be exposed on the root path. Enabled by default
• Health/Metrics/OpenAPI extensions create their RouteBuildItems as before, but now also call serveFromRoot() on the builder as well if the "MP endpoints" flag is true. Builder defaults serveFromRoot to false.
• Vert.x HTTP processor adds the roots to non application router and the main router if serveFromRoot is true.

Does that align with what you were describing @ebullient?

For the redirect flag, do we skip creating a redirect when serveFromRoot is true? Or change redirect handling in some other way?

@maxandersen thoughts?

[ebullient]

I think the redirect was a bad call. Wherever we had a redirect, we should just serve the thing (additional route that could match an existing handler).

Would that mean existing configuration can stay as-is.. ?!

I think we still have the "does this endpoint nest under /q or should it be served from root" question, which is floating around in the OAuth/JWT space right now.

My original thought was that Vert.x HTTP could be stupider: if the "also serve from root" flag is present, it creates an additional route using the endpoint's configured API... but that does make it harder to manage health/metrics/openapi/oauth-jwt as a group..

[maxandersen]

For me to grok it best - can you link to where this logic is handled now ? sounds like it is in a central place - not per extension ?

[kenfinnigan]

Right now Health, as an example, does https://github.com/quarkusio/quarkus/blob/master/extensions/smallrye-health/deployment/src/main/java/io/quarkus/smallrye/health/deployment/SmallRyeHealthProcessor.java#L231 to register a non-application endpoint.

The processing of the routes is then handled here: https://github.com/quarkusio/quarkus/blob/master/extensions/vertx-http/deployment/src/main/java/io/quarkus/vertx/http/deployment/VertxHttpProcessor.java#L135

[maxandersen]

Thanks for the pointers @kenfinnigan - that helped.

So, as I grok it we introduced two settings:

quarkus.http.non-application-root-path which defaults to /q to specify the default root to namespace our non-app additions. Thats a good thing IMO.

Then quarkus.http.redirect-to-non-application-root-path was added to define wether redirects should be done...but do I read the docs and code right that this only applies to a specific list of endpoints ? (i.e. /health, /metrics and /openapi).

So this setting might better been named quarkus.http.keep-pre-1.11-non-application-root-path or simply quarkus.http.microprofile-endpoints which we due to product migration and spec compliance would have as true and we would not just redirect them we would serve them directly and they would be managed as a group (I think that is somewhat what you both suggests?)

I think that would make sense and simply just instantly deprecate/remove the redirect setting.

What is left is wether we can find a way to manage the other paths under /q/* individually - but I kinda feel that is a separate concern/issue.

thoughts?

[kenfinnigan]

quarkus.http.redirect-to-non-application-root-path was intended as a bridging mechanism to enable anyone relying on the previous '/*' routes to not be impacted. Clearly, that wasn't the case because not everything handles redirects.

This redirect is applied to all non-application endpoints, not just /health, /metrics, etc.

Given the experience, it probably makes sense to ditch the redirect and just serve the handler on the original endpoint as well, with a flag to disable.

The issue is trying to do this for MP specific endpoints, or a list of specific endpoints. It would need the extension to set a value with RouteBuildItem that the processor can then use to know whether to serve a handler from both locations. So it's a bit messy 1) as to which endpoints need to be handled in this special way and 2) is it ok for specific extensions to set this info. In some respects, it depends on what these endpoints mean, ie. is it all MP endpoints or some other grouping.

[maxandersen]

This redirect is applied to all non-application endpoints

Is that true ? like /health -> /q/health works but there is no /dev -> /q/dev or are you saying that the list is "the extensions that mounted into /<something> pre 1.11.0" now has the redirect ?

[kenfinnigan]

All non application endpoints are redirected.

In the VertxHttpProcessor, in the if statement a line below the link above, it checks if the route is a framework one (carryover of old name it seems) and does the redirect.

Any route that calls nonApplicationRoute() on the builder will have the redirect applied

[maxandersen]

...okey so /q/dev is not considered a non application route but something else.

#I'm trying to grok what decides which is a non app route and which is not since if everyone gets added in the / then what is the point of having g /q to avoid collisions? Is it then purely setup to have one sub tree to secure ?

[kenfinnigan]

It looks like /q/dev isn't a non-application endpoint, but it's because it defines its' own router: https://github.com/quarkusio/quarkus/blob/master/extensions/vertx-http/deployment/src/main/java/io/quarkus/vertx/http/deployment/devmode/console/DevConsoleProcessor.java#L186

Anything that calls https://github.com/quarkusio/quarkus/blob/master/extensions/vertx-http/deployment/src/main/java/io/quarkus/vertx/http/deployment/RouteBuildItem.java#L142 will be considered one.

Maybe what's needed is to change the default of calling nonApplicationRoute() to not set the redirect to true, and require health, metrics extensions to pass a true to specifically indicate it should be done. The flag indicating a redirect can be changed to a "serveFromRoot" as @ebullient suggested.

Extensions need to explicitly indicate they should also be served from the root, as opposed to it being the default it is now. We can also have another flag to disable "serveFromRoot" completely

[maxandersen]

so this "non-application-route" is not consistently applied I guess?

But lets say /q/dev is an odd-ball out and everything was proper recorded as "non-application-route" and the flag changed to "serveFromRoot" we still end up with a weird situation here.

Like, I thought that the reason for /q was to not pollute the root namespace and thus the only thing that made sense to serve from root was the endpoints that a) was specified in a spec b) possibly affecting product migrations.

And that isn't the case for everything "non-application-route" (i.e. /q/dev isn't a nor b, neither is /q/swagger-ui)

I wonder if we would do this knowing what we know now we would in stead of an "all or nothing" approach would just have some "on/off" properties for each or some grouping (like "microprofile-specs") to toggle would make more sense?

[kenfinnigan]

The original reasoning for serving anything from the root after the move to /q was purely as a backward-compatible "don't break" mode. Ignoring the fact we know now some things don't follow redirects.

I thought about on/off for each, but that gets really messy fast as we continue to add more endpoints.

We can have a flag to enable a grouping of endpoints, maybe MP ones, to be served from the root as well as /q. What would be the suggested default?

[dandreadis]

I think the default for the MP endpoints should be / (root) (or whatever the specs define), which is what everyone expected up to now. Then if users want, they could move those all under q/* using the provided flag. I wouldn't add any redirects and/or dual-serves, because it complicates things.

Whether we should re-brake the early movers, or not, while unfortunate, IMO it is better to admit and correct a mistake early, rather than prolonging it. Besides those affected are already aware of the problem. But we need to act fast and explain properly.

[maxandersen]

I would go for owning up to the issue and just do direct serve from both /q/* and /*

The whole intention was to get /q/ uniquely namespaced and add redirects for backwards compatiabilty - which turned out to not work. So doing direct serve seem to be the simplest - possibly only do it for the spec routes since we already only do it partially. (i.e. /q/dev is not served out)

@dandreadis btw. are you talking only about the spec locations or the additional paths that exist?

[dandreadis]

/metrics, /health/, /openapi by default (with the flag moving them to the /q/ namespace)
The others could stay there: /q/dev/, q/swagger-ui

[kenfinnigan]

The list of endpoints that were moved under /q and with redirects is:

• /health
• /metrics
• /openapi
• /health-ui
• /swagger-ui
• /graphql-ui

In terms of what we do with any endpoints that we want to be served under the root, I think they should also be served under /q.

So every non-application endpoint is present under /q, but then some subset can be under the root with a config flag set.

[knutwannheden]

In Quarkus 1.12.0 I could have quarkus.http.root-path=/foo and quarkus.http.non-application-root-path=/ and the effect would be that I would have the Swagger UI under /foo/swagger-ui. Now with Quarkus 1.12.1 the same config leads to the Swagger UI being served as /swagger-ui. I suppose that should be considered a regression. I can open a ticket for this, unless you consider the 1.12.0 behavior to be wrong.

[knutwannheden]

Possibly this is already addressed by #15458. I will take a look.