Skip to content

Provide an endpoint that provides a detailed view of the application's health #11869

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
snicoll opened this issue Jan 31, 2018 · 18 comments
Closed
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@snicoll
Copy link
Member

snicoll commented Jan 31, 2018

We've been back and forth on this topic with the latest change being the drop of the /status endpoint in #11113

There were a number of complaints about the fact we can't change the level of details based on the fact the request is authorized or not (see #11113 (comment)).

We've removed this feature because it was very complex to describe, in particular due to the sensitive flag. This flag has been removed so perhaps we should revisit our decision and restore some form of support for this.

@snicoll snicoll added 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 labels Jan 31, 2018
@wilkinsona
Copy link
Member

The idea that we had when discussing this was to support injection of a java.security.Principal into an endpoint operation (probably only in web-specific extensions) and to offer an enum that allows the health endpoint's behaviour to be configured to:

  • Always show full details
  • Never show full details
  • Only show full details when authenticated (the injected principle is non-null)

@mbhave
Copy link
Contributor

mbhave commented Jan 31, 2018

The enum coupled with Spring Security access rules might be a bit confusing. Users could end up with a combination of enum value and Spring Security access rules that don't really make sense. Always show full details, indicates that accessing /health will not require a Principal and full details will still be shown. But with Spring Security on the classpath (and custom security configuration), users would still need to permitAll access to the health endpoint.

Another example where there might be a mismatch: the enum value is Only show full details when authenticated (the injected principle is non-null) and access rules on the health endpoint are mvcMatchers('/actuator/health').hasRole("ACTUATOR"). The enum value could be a cause for confusion in this case because unauthenticated access to health is not possible.

@wilkinsona
Copy link
Member

Thanks, Madhura. I agree, that does sound confusing. I think what we're trying to specify in the enum is what a 200 response will contain. The user then needs to configure things so that it's actually possible to get a 200 response… Right now, I can't see how we can do that without the possible confusion that you've described.

@philwebb philwebb added type: enhancement A general enhancement priority: normal and removed 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 labels Feb 2, 2018
@philwebb philwebb added this to the 2.0.0.RC2 milestone Feb 2, 2018
@philwebb
Copy link
Member

philwebb commented Feb 2, 2018

We'll add the two endpoints back, but keep /health for the basic summary.

@vpavic
Copy link
Contributor

vpavic commented Feb 2, 2018

We'll add the two endpoints back, but keep /health for the basic summary.

What are the implications of this on the addition of ping endpoint proposed in #11191?

@wilkinsona
Copy link
Member

I don't think there are any implications. The two endpoints have different purposes.

@wilkinsona wilkinsona self-assigned this Feb 5, 2018
@wilkinsona wilkinsona changed the title Consider revisiting the health endpoint details strategy Provide a status endpoint that provides a detailed view of the application's health Feb 5, 2018
@wilkinsona
Copy link
Member

Having started implementing this, I'm not sure that the name status really works. It makes enough sense from an external perspective where health can be seen as a high-level view of the application's health and status can be seen as a detailed status report about the application's health. However, it starts to break down at the implementation level. We end up with code like this on the health side of things:

@ReadOperation
public WebEndpointResponse<Health> getHealth() {
	Status status = this.delegate.health().getStatus();
	Integer httpStatus = this.statusHttpMapper.mapStatus(status);
	return new WebEndpointResponse<>(Health.status(status).build(), httpStatus);
}

It's getting the whole health, extracting just the status, and using that in the response. In other words, it looks to be doing exactly what you'd expect an endpoint named status to be doing. Perhaps we just need to live with it?

@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label Feb 5, 2018
@wilkinsona
Copy link
Member

It makes enough sense from an external perspective

On second thoughts, I take that back. With the currently proposed arrangement, we'd end up with status returning the following:

{
  "status" : "UP",
  "details" : {
    "diskSpaceHealthIndicator" : {
      "status" : "UP",
      "details" : {
        "total" : 94834171904,
        "free" : 72733929472,
        "threshold" : 10485760
      }
    },
    "dataSourceHealthIndicator" : {
      "status" : "UP",
      "details" : {
        "database" : "HSQL Database Engine",
        "hello" : 1
      }
    }
  }
}

And health returning the following:

{
  "status" : "UP"
}

Having a health endpoint and a status endpoint where the health endpoint just shows the status is counterintuitive.

@wilkinsona wilkinsona changed the title Provide a status endpoint that provides a detailed view of the application's health Provide an endpoint that provides a detailed view of the application's health Feb 5, 2018
@shakuzen
Copy link
Member

shakuzen commented Feb 5, 2018

Brainstorming ideas for the endpoint name, healthdetails seems to better describe things than status, but it is wordy. diagnostics while still long is a single word that perhaps captures the intended meaning of the endpoint.

@adrogon
Copy link

adrogon commented Feb 6, 2018

If the new endpoint's response is an extended view of health's, then having it called healthsomething sounds like a good idea.
healthreport, healthindicators, healthdetails, healthier...

@wilkinsona
Copy link
Member

wilkinsona commented Feb 6, 2018

Thanks for the suggestions.

Given the difficulty of naming the new endpoint and the smell of having two endpoints that do the same thing, we're going to take another look at the approach described above. Our hope is that some well-named enum values couple with some documentation will avoid any confusing.

The current thinking is to have a property for showing the details that is an enum with three values: NEVER, WHEN_AUTHENTICATED, and WHEN_ACCESSIBLE. WHEN_ACCESSIBLE is so named to hopefully capture the need for the endpoint to be accessible, i.e. for any security configuration to allow access to it.

@adrogon
Copy link

adrogon commented Feb 6, 2018

How about WHEN_AUTHORIZED then?
That reflects the difference between Authentication and Authorization, Spring Security made a similar choice with @isAuthenticated and @preAuthorize annotations.

For the rest of the post, I'd rather have this proposed solution than two endpoints too.

@wilkinsona
Copy link
Member

How about WHEN_AUTHORIZED then?

What you're proposing isn't entirely clear to me. Are you proposing WHEN_AUTHORIZED as an additional value or as an alternative to one of the already-proposed values?

@adrogon
Copy link

adrogon commented Feb 6, 2018

Sorry for not being clear. You seemed not to be sure with WHEN_ACCESSIBLE, so that was an alternative to it.

@wilkinsona
Copy link
Member

wilkinsona commented Feb 6, 2018

Thanks. I don't think WHEN_AUTHORIZED is quite right as a user may not be using any form of security. It's trying to say that details will be shown to everyone if you're not using security, and to anyone who can access the endpoint if you are (for example if you'd used permitAll() in your Spring Security config). This discussion is making me wonder if ALWAYS would actually be less confusing after all. It's saying the endpoint will always return details in the response. If your security configuration prevents the endpoint from being reached at all then that's a separate concern.

@adrogon
Copy link

adrogon commented Feb 6, 2018

ALWAYS would actually be less confusing after all

👍

@wilkinsona wilkinsona removed the for: team-attention An issue we'd like other members of the team to review label Feb 7, 2018
@wilkinsona
Copy link
Member

@mbhave has pointed out a flaw with the current design. As things stand, it's difficult to show an overview to anonymous users and show details to authorized users. The closest that you can get is to configure Spring Security to only allow access to users in the ANONYMOUS role and the role(s) required for authorised access. The principal check in the endpoint means the anonymous users and authorised users are shown the right thing, but authenticated unauthorised users are denied access entirely. It would be good if they could be shown an overview.

@philwebb
Copy link
Member

I think the default of ShowDetails.WHEN_AUTHORIZED combined with the empty roles default is a bit risky. I'd prefer ShowDetails.NEVER and make people opt-in.

snicoll added a commit that referenced this issue Feb 21, 2018
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

7 participants