Skip to content

Make the path of web endpoints configurable #10181

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

Closed
wilkinsona opened this issue Sep 6, 2017 · 15 comments
Closed

Make the path of web endpoints configurable #10181

wilkinsona opened this issue Sep 6, 2017 · 15 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@wilkinsona
Copy link
Member

Many gateways and load balancers ping a URL and just need a 200 back to consider a server to be a candidate for handling requests. We've also heard of teams that "require" this URL to be /health. Given that all they need is a 200 response, the status endpoint is currently the best fit but it's mapped to /status. We should consider making /health provide the rolled up information about the health of the application and making /status provide detailed information.

@wilkinsona wilkinsona added for: team-attention An issue we'd like other members of the team to review priority: normal type: enhancement A general enhancement labels Sep 6, 2017
@snicoll
Copy link
Member

snicoll commented Sep 6, 2017

So the argument is about the size of the body? IMO it feels weird that something named status returns the full information while health returns only something named "status".

@wilkinsona
Copy link
Member Author

No, I think it's more an argument about security. If we leave things as they are and a load balancer requires the use of /health then the balancer either has to authenticate or you have to open up all of the information exposed by /health to everyone.

@snicoll
Copy link
Member

snicoll commented Sep 11, 2017

OK but let's keep #8685 on the radar. If we do this, we'd offer /status/{name} rather than /health/{name}. Given the structure of the reply and what a "status" is vs. what "health" means, it looks completely backwards to me.

@philwebb
Copy link
Member

I think /health is the more common URL so perhaps that should be the one reveals the least. I'd also be quite keen on bringing the names closer together. How about /heath and /healthdetails ?

@snicoll
Copy link
Member

snicoll commented Sep 19, 2017

If we want to do that, we have no choice IMO, status should go away and should be renamed to something that states it provides additional details. It is a shame because I think the current names are just about right (I am not a huge fan of /healthdetails and an HealhDetailsEndpoint), this
constraint of having the existing endpoint showing only the status is messing things up :(

Can we reconsider perhaps? We want to use 2.0 to fix things and I believe this is one of them.

@wilkinsona
Copy link
Member Author

@snicoll What do you want to reconsider?

@bclozel
Copy link
Member

bclozel commented Sep 22, 2017

Agree, both /status and /health names are right and logically we should just swap them. But this will a real problem for environments running both Spring Boot 1.x and 2.x.

So my vote goes to /status going away and keeping /health for the basic information. As for the additional endpoint, /healthdetails is not perfect, but at least it gives you the impression that you'll get "more of the same"; which is exactly what we're trying to do.

I'm still nervous about moving things there; we're basically making a backwards incompatible change not for the load-balancers, but for the APMs using that endpoint to get the details.

@snicoll
Copy link
Member

snicoll commented Sep 22, 2017

Reconsider swapping them. The naming is right now IMO so I'd really like to keep things as they are now.

@philwebb
Copy link
Member

Since we're pretty much all agreed that /application/status and /application/health are logical paths, perhaps we should fix the "problem" a different way? Can we re-instate the ability to change the paths that endpoints are mapped to? It doesn't need to be in properties, perhaps a strategy bean that the user can register?

@wilkinsona
Copy link
Member Author

wilkinsona commented Sep 23, 2017

I don't even think it would be so bad if we supported it via the properties now, particularly if we moved them beneath web to make it clear that JMX was unaffected: endpoints.<id>.web.path.

We definitely don't want to reintroduce support for changing the ID which had the side-effect of changing the path.

@snicoll
Copy link
Member

snicoll commented Sep 23, 2017

This isn't my preferred option as well. Hopefully systems can adapt over time to /status.

@wilkinsona
Copy link
Member Author

wilkinsona commented Sep 23, 2017

@snicoll Did you mean this is my preferred option?

@snicoll
Copy link
Member

snicoll commented Sep 23, 2017

Yes sorry. The auto-correct on my phone decided to write isn't 😕

@wilkinsona wilkinsona changed the title Consider swapping the behaviour of the status and health endpoints Make the path of web endpoints configurable Sep 23, 2017
@wilkinsona wilkinsona removed the for: team-attention An issue we'd like other members of the team to review label Sep 23, 2017
@wilkinsona
Copy link
Member Author

I've retitled the issue for the preferred approach which is to allow a web endpoint's path to be configured

@wilkinsona wilkinsona added this to the 2.0.0.M6 milestone Sep 23, 2017
@snicoll
Copy link
Member

snicoll commented Oct 25, 2017

We need to be careful no to change the mapping for CloudFoundry as that support expects the standard paths. We have a determinePath in WebAnnotationEndpointDiscoverer that will need to change to use the configured option and it sounds like the good place to override the behaviour in that case to always use the id (Right now we need to create a discoverer to avoid filtering out the endpoints the user may have disabled on the regular paths so an override should work there).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants