Skip to content

Unrestricted Health check should be able to be cached #2630

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
ericdahl opened this issue Mar 10, 2015 · 3 comments
Closed

Unrestricted Health check should be able to be cached #2630

ericdahl opened this issue Mar 10, 2015 · 3 comments
Assignees
Milestone

Comments

@ericdahl
Copy link
Contributor

If endpoints.health.sensitive is set to false, then the caching feature of the Health endpoint is bypassed due to this check in HealthMvcEndpoint:

if (isCacheStale(accessTime) || isSecure(principal) || isUnrestricted()) {
    this.lastAccess = accessTime;
    this.cached = this.delegate.invoke();
}

It would be nice to still be able to have the caching functionality even if the health endpoint is unrestricted. For example, if I have an intensive health check which is unrestricted, but running on a local IP or otherwise protected, I may still want to have the value be cached so that an overly zealous health check doesn't cause the service to degrade.

@philwebb philwebb added this to the 1.2.3 milestone Mar 10, 2015
@philwebb philwebb added type: enhancement A general enhancement and removed type: enhancement A general enhancement labels Mar 10, 2015
@wilkinsona wilkinsona self-assigned this Mar 16, 2015
@wilkinsona
Copy link
Member

I have a fix for this issue here.

@philwebb I'd welcome your thoughts on the fix. Specifically, I'm wondering if we might also want a property to force caching for secure access. If we do, or we think we might in the future, then a single, more generally named property could be preferable to one that controls caching for unrestricted access and one that controls caching for secure access. endpoints.health.always-cache, perhaps?

@philwebb
Copy link
Member

@wilkinsona We could consider just always caching results. The current logic is a little complex and I'm wondering if the time-to-live property alone is enough for most people. The docs already imply that caching occurs regardless of how the endpoint is accessed or if it is secured.

@wilkinsona
Copy link
Member

@philwebb Sold. The current logic made my head hurt a bit when I was working on this fix; simplifying it definitely sounds good to me.

snicoll pushed a commit to snicoll/spring-boot that referenced this issue Mar 20, 2015
Previously, the response from /health was not cached if the request
was secure, i.e. the user has authenticated, or the endpoint was
configured as not being sensitive. 

The commit updates HealthMvcEndpoint to apply the caching logic
all the time. Users that do not want caching can disable it by 
configuring the TTL with a value of zero.

Closes spring-projectsgh-2630
snicoll pushed a commit to snicoll/spring-boot that referenced this issue Mar 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants