-
Notifications
You must be signed in to change notification settings - Fork 132
feat: Add custom placeholder responses for scale-from-zero scenarios #1303
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great feature! Thanks for the contribution 🙇
interceptor/handler/placeholder.go
Outdated
<html> | ||
<head> | ||
<title>Service Starting</title> | ||
<meta http-equiv="refresh" content="{{.RefreshInterval}}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we inject this value instead of expect that users provide it? I mean, this looks as something that we want to enforce. @wozniakjan ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about injecting it but wanted some feedback first.
I think it also would be possible to add some JavaScript instead which calls the URL the browser is on and checks if the X-KEDA-HTTP-Placeholder-Served: true
header is there. If it's not we should do a full refresh.
This avoids having the browser do an actual refresh every X
seconds. This would mostly be an UX improvement.
Both approaches could be injected into whatever template the user provides, if that's the approach you would prefer.
Example:
<script>
(function() {
const checkInterval = {{.RefreshInterval}} * 1000; // Convert seconds to milliseconds
async function checkServiceStatus() {
try {
// Make a HEAD request to the current URL to check headers
const response = await fetch(window.location.href, {
method: 'HEAD',
cache: 'no-cache'
});
// Check if the placeholder header is present
const placeholderHeader = response.headers.get('X-KEDA-HTTP-Placeholder-Served');
if (placeholderHeader !== 'true') {
// Service is ready! Do a full page refresh
window.location.reload();
} else {
// Still showing placeholder, check again later
setTimeout(checkServiceStatus, checkInterval);
}
} catch (error) {
// On error, continue checking
console.error('Error checking service status:', error);
setTimeout(checkServiceStatus, checkInterval);
}
}
// Start checking after the interval
setTimeout(checkServiceStatus, checkInterval);
})();
</script>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this approach! it's quite elegant IMHO, the only thing is that JS needs to be injected as well, doesn't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wozniakjan @zroubalik WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JorTurFer I've tried to make a take on the injection with a <script>
tag and some JS to figure out when to refresh the browser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I like this as a default behavior, but I have mentioned a few additional points regarding this in my review - #1303 (review)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces configurable placeholder pages for scale-from-zero scenarios, enhancing the user experience during cold starts. Key changes include:
- Adding a new placeholder handler and supporting API changes in the HTTPScaledObject CRD.
- Adjusting proxy handler logic and test cases to incorporate placeholder page responses.
- Updating documentation, examples, and deepcopy functions to reflect the new placeholderConfig functionality.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
tests/checks/placeholder_pages/placeholder_pages_test.go | Added E2E tests for placeholder page responses and script injection. |
operator/apis/http/v1alpha1/zz_generated.deepcopy.go | Added DeepCopy functions for the new PlaceholderConfig type. |
operator/apis/http/v1alpha1/httpscaledobject_types.go | Introduced the PlaceholderConfig type in the CRD schema. |
interceptor/proxy_handlers_test.go, proxy_handlers_integration_test.go | Updated tests to pass new parameters for placeholder handling. |
interceptor/proxy_handlers.go | Integrated the placeholder handler into the forwarding logic. |
interceptor/main.go, main_test.go | Updated server initialization to include the placeholder handler. |
interceptor/handler/placeholder.go | Implemented the placeholder page rendering and caching logic. |
examples/vX.X.X/httpscaledobject.yaml | Provided example configuration for placeholder pages. |
docs/ref/vX.X.X/http_scaled_object.md | Documented the placeholderConfig section details. |
config/crd/bases/http.keda.sh_httpscaledobjects.yaml | Updated the CRD with placeholderConfig properties. |
CHANGELOG.md | Added an entry for the custom placeholder pages feature. |
Comments suppressed due to low confidence (1)
interceptor/handler/placeholder.go:206
- [nitpick] Consider logging the error returned from getTemplate before falling back to the inline response to improve debuggability of template parsing issues.
tmpl, err := h.getTemplate(r.Context(), hso)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome job! Only one small nit inline and it's okey to merge ❤️
Hello any news on this feature ? 🙏 |
Sorry, @wozniakjan told me that he wanted to review the feature and I just kept this to him. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks really great, thank you for contributing!
I do have a couple of discussion points before it gets merged
- usage of
ConfigMaps
for storing content for placeholder pages - imho defaults would be better configured globally as a statically mountedConfigMap
or env variable onDeployment
- simplifying content processing - the default as a nicely formatted HTML page makes great sense but then we should allow arbitrary payloads with arbitrary headers, not all usage of this tool is aimed at web browsers, sometimes it's microservices talking between each other
interceptor/handler/placeholder.go
Outdated
const placeholderScript = `<script> | ||
(function() { | ||
const checkInterval = {{.RefreshInterval}} * 1000; | ||
async function checkServiceStatus() { | ||
try { | ||
const response = await fetch(window.location.href, { | ||
method: 'HEAD', | ||
cache: 'no-cache' | ||
}); | ||
const placeholderHeader = response.headers.get('X-KEDA-HTTP-Placeholder-Served'); | ||
if (placeholderHeader !== 'true') { | ||
window.location.reload(); | ||
} else { | ||
setTimeout(checkServiceStatus, checkInterval); | ||
} | ||
} catch (error) { | ||
console.error('Error checking service status:', error); | ||
setTimeout(checkServiceStatus, checkInterval); | ||
} | ||
} | ||
setTimeout(checkServiceStatus, checkInterval); | ||
})(); | ||
</script>` | ||
|
||
const defaultPlaceholderTemplateWithoutScript = `<!DOCTYPE html> | ||
<html> | ||
<head> | ||
<title>Service Starting</title> | ||
<meta charset="utf-8"> | ||
<style> | ||
body { | ||
font-family: -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, sans-serif; | ||
display: flex; | ||
align-items: center; | ||
justify-content: center; | ||
min-height: 100vh; | ||
margin: 0; | ||
background: #f5f5f5; | ||
} | ||
.container { | ||
text-align: center; | ||
padding: 2rem; | ||
background: white; | ||
border-radius: 8px; | ||
box-shadow: 0 2px 10px rgba(0,0,0,0.1); | ||
max-width: 400px; | ||
} | ||
h1 { | ||
color: #333; | ||
margin-bottom: 1rem; | ||
font-size: 1.5rem; | ||
} | ||
.spinner { | ||
width: 40px; | ||
height: 40px; | ||
margin: 1.5rem auto; | ||
border: 4px solid #f3f3f3; | ||
border-top: 4px solid #3498db; | ||
border-radius: 50%; | ||
animation: spin 1s linear infinite; | ||
} | ||
@keyframes spin { | ||
0% { transform: rotate(0deg); } | ||
100% { transform: rotate(360deg); } | ||
} | ||
p { | ||
color: #666; | ||
margin: 0.5rem 0; | ||
} | ||
</style> | ||
</head> | ||
<body> | ||
<div class="container"> | ||
<h1>{{.ServiceName}} is starting up...</h1> | ||
<div class="spinner"></div> | ||
<p>Please wait while we prepare your service.</p> | ||
</div> | ||
</body> | ||
</html>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wdyt about moving these default templates to configuration instead of having them hardcoded in the binary? I can imagine setting it through environment variables in the Deployment
or alternatively, which might be even better, mounted as a ConfigMap
.
env: |
These default pages are pretty good examples, but I'd imagine people may want to tweak them a little bit or centrally replace with different defaults. As implemented, iiuc, users would have to copy the same ConfigMap
into each namespace and then reference it in the HTTPScaledObject
or inline the same string in each HTTPScaledObject
.
interceptor/handler/placeholder.go
Outdated
} | ||
|
||
// For non-HTML content, wrap it in a minimal HTML structure with the script | ||
return fmt.Sprintf(`<!DOCTYPE html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if users don't want HTML content? The client doesn't have to be web browser, it could be two microservices talking to each other and they may have other L7 protocols (json, protobuf, etc) with their own mechanisms on how to handle retries.
// Path to ConfigMap containing placeholder HTML content (in same namespace) | ||
// +optional | ||
ContentConfigMap string `json:"contentConfigMap,omitempty" description:"ConfigMap name containing placeholder HTML content"` | ||
// Key in ConfigMap containing the HTML template | ||
// +kubebuilder:default="template.html" | ||
// +optional | ||
ContentConfigMapKey string `json:"contentConfigMapKey,omitempty" description:"Key in ConfigMap containing the HTML template"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mind the fact interceptor doesn't have RBAC for ConfigMaps
. Is it worth adding clusterwide read access to ConfigMaps
so users can configure content of a placeholder page per namespace? Imho it's a bit too much and I would rather just add defaults as mounted ConfigMaps
to interceptor or env variables on the Deployment
as mentioned above.
interceptor/handler/placeholder.go
Outdated
if config.ContentConfigMap != "" { | ||
cacheKey := fmt.Sprintf("%s/%s/cm/%s", hso.Namespace, hso.Name, config.ContentConfigMap) | ||
|
||
cm, err := h.k8sClient.CoreV1().ConfigMaps(hso.Namespace).Get(ctx, config.ContentConfigMap, metav1.GetOptions{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather not use ConfigMaps
at the HTTPScaledObject
level but if we decide to go with them, please use cached client instead of the direct client. Fetching ConfigMap
for each request on zero-scaled apps might be tricky with kube-client QPS and could easily cause a lot of load for kube-apiserver if users increase the burst config to "deal with errors".
interceptor/proxy_handlers.go
Outdated
endpoints, err := endpointsCache.Get(httpso.GetNamespace(), httpso.Spec.ScaleTargetRef.Service) | ||
if err == nil && workloadActiveEndpoints(endpoints) == 0 { | ||
if placeholderErr := placeholderHandler.ServePlaceholder(w, r, httpso); placeholderErr != nil { | ||
lggr.Error(placeholderErr, "failed to serve placeholder page") | ||
w.WriteHeader(http.StatusBadGateway) | ||
if _, err := w.Write([]byte("error serving placeholder page")); err != nil { | ||
lggr.Error(err, "could not write error response to client") | ||
} | ||
} | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
situation when placeholder pages are configured and the endpoints cache returns error is not handled, which means the code will fall through to waiting on cold-start. Is that desired, or should the interceptor instead return a 503 error indicating an internal problem?
b15d40f
to
d95a60b
Compare
@wozniakjan I've looked at the feedback and tried to adjust it accordingly. Haven't had time to do the E2E tests, so put the PR in draft until it's ready for review. |
854c1ba
to
087532b
Compare
Allows HTTPScaledObjects to serve configurable HTML pages while workloads scale up from zero, with support for templates, custom headers, and automatic refresh. Signed-off-by: malpou <[email protected]>
Signed-off-by: malpou <[email protected]>
Signed-off-by: malpou <[email protected]>
Signed-off-by: malpou <[email protected]>
…itten to Signed-off-by: malpou <[email protected]>
…should be comptatible in the GHA environment Signed-off-by: malpou <[email protected]>
Signed-off-by: Malthe Poulsen <[email protected]> Signed-off-by: malpou <[email protected]>
Signed-off-by: malpou <[email protected]>
Signed-off-by: malpou <[email protected]>
Signed-off-by: Malthe Poulsen <[email protected]> Signed-off-by: malpou <[email protected]>
Signed-off-by: Malthe Poulsen <[email protected]> Signed-off-by: malpou <[email protected]>
Signed-off-by: Malthe Poulsen <[email protected]> Signed-off-by: malpou <[email protected]>
Signed-off-by: Malthe Poulsen <[email protected]> Signed-off-by: malpou <[email protected]>
Signed-off-by: Malthe Poulsen <[email protected]> Signed-off-by: malpou <[email protected]>
Co-authored-by: Jorge Turrado Ferrero <[email protected]> Signed-off-by: Malthe Poulsen <[email protected]> Signed-off-by: malpou <[email protected]>
Co-authored-by: Jorge Turrado Ferrero <[email protected]> Signed-off-by: Malthe Poulsen <[email protected]> Signed-off-by: malpou <[email protected]>
Signed-off-by: malpou <[email protected]>
Co-authored-by: Jan Wozniak <[email protected]> Signed-off-by: Malthe Poulsen <[email protected]> Signed-off-by: malpou <[email protected]>
- Move default templates to configuration via environment variables - Add ConfigMap for default placeholder templates - Support multiple content types (HTML, JSON, XML, plain text) - Remove ConfigMap fields from HTTPScaledObject CRD - Add error handling for endpoint cache failures - Update tests to reflect all changes This addresses all review comments: - Defaults configured globally via mounted ConfigMap - Arbitrary content types supported for microservices - No RBAC changes needed (removed ConfigMap access) - Proper error handling when cache fails Signed-off-by: malpou <[email protected]>
Signed-off-by: malpou <[email protected]>
Refactored placeholder pages to support any content type without automatic modifications. Users can now provide HTML, JSON, XML, or plain text content that will be served as-is during scale-from-zero. Changes: - Removed automatic script injection and default HTML template - Added Content-Type header support in placeholderConfig - Template variables (ServiceName, Namespace, RefreshInterval) work with any content format - Added examples for JSON, XML, and plain text responses - Fixed e2e tests and improved test helper reliability Signed-off-by: malpou <[email protected]>
Reorder condition checks to verify placeholder configuration before checking handler to prevent potential panics if placeholder is enabled in spec but handler is nil. Signed-off-by: malpou <[email protected]>
087532b
to
06322d2
Compare
Improve maintainability and readability of placeholder pages feature: - Add constants for headers and messages to eliminate magic strings - Remove self-explanatory comments that duplicate code intent - Extract helper methods to reduce duplication and improve separation of concerns - Rename getTemplate to resolveTemplate for clarity - Simplify template caching with cleaner double-check locking pattern - Extract placeholder serving logic into focused helper functions No functional changes - pure refactoring for long-term maintainability. Signed-off-by: malpou <[email protected]>
Remove left over stuff from testing
Signed-off-by: Malthe Poulsen <[email protected]>
Signed-off-by: Malthe Poulsen <[email protected]>
Signed-off-by: Malthe Poulsen <[email protected]>
Signed-off-by: Malthe Poulsen <[email protected]>
Signed-off-by: Malthe Poulsen <[email protected]>
Allows
HTTPScaledObjects
to serve configurable content (HTML, JSON, XML, plain text, etc.) while workloads scale up from zero, improving user experience during cold starts.Description
This PR implements customizable placeholder responses that are displayed to users while applications are scaling up from zero. This significantly improves the user experience during cold starts by providing immediate feedback instead of connection timeouts or errors.
The implementation is content-agnostic, allowing users to serve any type of response (HTML pages, JSON API responses, XML, plain text, etc.) without automatic modifications or assumptions about the content format.
Key features:
placeholderConfig
section toHTTPScaledObject
CRD for configuring placeholder responses{{.ServiceName}}
,{{.Namespace}}
, and{{.RefreshInterval}}
Content-Type
headers to match your API formatImplementation details:
Use cases:
{"status": "initializing", "message": "Service is starting..."}
responsesChecklist
README.md
docs/
directory