Skip to content

Commit 5794cc4

Browse files
malpoumalpou
committed
refactor: improve placeholder handler code quality
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]>
1 parent b9d5586 commit 5794cc4

File tree

4 files changed

+108
-103
lines changed

4 files changed

+108
-103
lines changed

‎interceptor/handler/placeholder.go‎

Lines changed: 53 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -14,23 +14,27 @@ import (
1414
"github.com/kedacore/http-add-on/operator/apis/http/v1alpha1"
1515
)
1616

17+
const (
18+
headerPlaceholderServed = "X-KEDA-HTTP-Placeholder-Served"
19+
headerCacheControl = "Cache-Control"
20+
headerContentType = "Content-Type"
21+
cacheControlValue = "no-cache, no-store, must-revalidate"
22+
fallbackContentType = "text/plain; charset=utf-8"
23+
fallbackMessageFormat = "%s is starting up...\n"
24+
)
1725

18-
// cacheEntry stores a template along with resource generation info for cache invalidation
1926
type cacheEntry struct {
20-
template *template.Template
21-
hsoGeneration int64
22-
configMapVersion string
27+
template *template.Template
28+
hsoGeneration int64
2329
}
2430

25-
// PlaceholderHandler handles serving placeholder pages during scale-from-zero
2631
type PlaceholderHandler struct {
2732
templateCache map[string]*cacheEntry
2833
cacheMutex sync.RWMutex
2934
defaultTmpl *template.Template
3035
servingCfg *config.Serving
3136
}
3237

33-
// PlaceholderData contains data for rendering placeholder templates
3438
type PlaceholderData struct {
3539
ServiceName string
3640
Namespace string
@@ -39,11 +43,9 @@ type PlaceholderData struct {
3943
Timestamp string
4044
}
4145

42-
// NewPlaceholderHandler creates a new placeholder handler
4346
func NewPlaceholderHandler(servingCfg *config.Serving) (*PlaceholderHandler, error) {
4447
var defaultTmpl *template.Template
4548

46-
// Try to load template from configured path if provided
4749
if servingCfg.PlaceholderDefaultTemplatePath != "" {
4850
content, err := os.ReadFile(servingCfg.PlaceholderDefaultTemplatePath)
4951
if err != nil {
@@ -66,9 +68,6 @@ func NewPlaceholderHandler(servingCfg *config.Serving) (*PlaceholderHandler, err
6668
}, nil
6769
}
6870

69-
70-
71-
// ServePlaceholder serves a placeholder page based on the HTTPScaledObject configuration
7271
func (h *PlaceholderHandler) ServePlaceholder(w http.ResponseWriter, r *http.Request, hso *v1alpha1.HTTPScaledObject) error {
7372
if hso.Spec.PlaceholderConfig == nil || !hso.Spec.PlaceholderConfig.Enabled {
7473
http.Error(w, "Service temporarily unavailable", http.StatusServiceUnavailable)
@@ -82,20 +81,13 @@ func (h *PlaceholderHandler) ServePlaceholder(w http.ResponseWriter, r *http.Req
8281
statusCode = http.StatusServiceUnavailable
8382
}
8483

85-
// Set custom headers first
8684
for k, v := range config.Headers {
8785
w.Header().Set(k, v)
8886
}
8987

90-
// Get template and render content
91-
tmpl, err := h.getTemplate(r.Context(), hso)
88+
tmpl, err := h.resolveTemplate(r.Context(), hso)
9289
if err != nil {
93-
w.Header().Set("Content-Type", "text/plain; charset=utf-8")
94-
w.Header().Set("X-KEDA-HTTP-Placeholder-Served", "true")
95-
w.Header().Set("Cache-Control", "no-cache, no-store, must-revalidate")
96-
w.WriteHeader(statusCode)
97-
fmt.Fprintf(w, "%s is starting up...\n", hso.Spec.ScaleTargetRef.Service)
98-
return nil
90+
return h.serveFallbackPlaceholder(w, hso.Spec.ScaleTargetRef.Service, statusCode)
9991
}
10092

10193
data := PlaceholderData{
@@ -108,52 +100,30 @@ func (h *PlaceholderHandler) ServePlaceholder(w http.ResponseWriter, r *http.Req
108100

109101
var buf bytes.Buffer
110102
if err := tmpl.Execute(&buf, data); err != nil {
111-
w.Header().Set("Content-Type", "text/plain; charset=utf-8")
112-
w.Header().Set("X-KEDA-HTTP-Placeholder-Served", "true")
113-
w.Header().Set("Cache-Control", "no-cache, no-store, must-revalidate")
114-
w.WriteHeader(statusCode)
115-
fmt.Fprintf(w, "%s is starting up...\n", hso.Spec.ScaleTargetRef.Service)
116-
return nil
103+
return h.serveFallbackPlaceholder(w, hso.Spec.ScaleTargetRef.Service, statusCode)
117104
}
118105

119-
content := buf.String()
120-
121-
// Set standard headers
122-
w.Header().Set("X-KEDA-HTTP-Placeholder-Served", "true")
123-
w.Header().Set("Cache-Control", "no-cache, no-store, must-revalidate")
106+
w.Header().Set(headerPlaceholderServed, "true")
107+
w.Header().Set(headerCacheControl, cacheControlValue)
108+
w.WriteHeader(statusCode)
109+
_, err = w.Write(buf.Bytes())
110+
return err
111+
}
124112

113+
func (h *PlaceholderHandler) serveFallbackPlaceholder(w http.ResponseWriter, serviceName string, statusCode int) error {
114+
w.Header().Set(headerContentType, fallbackContentType)
115+
w.Header().Set(headerPlaceholderServed, "true")
116+
w.Header().Set(headerCacheControl, cacheControlValue)
125117
w.WriteHeader(statusCode)
126-
_, err = w.Write([]byte(content))
118+
_, err := fmt.Fprintf(w, fallbackMessageFormat, serviceName)
127119
return err
128120
}
129121

130-
// getTemplate retrieves the template for the given HTTPScaledObject
131-
func (h *PlaceholderHandler) getTemplate(ctx context.Context, hso *v1alpha1.HTTPScaledObject) (*template.Template, error) {
122+
func (h *PlaceholderHandler) resolveTemplate(_ context.Context, hso *v1alpha1.HTTPScaledObject) (*template.Template, error) {
132123
config := hso.Spec.PlaceholderConfig
133124

134125
if config.Content != "" {
135-
cacheKey := fmt.Sprintf("%s/%s/inline", hso.Namespace, hso.Name)
136-
137-
h.cacheMutex.RLock()
138-
entry, ok := h.templateCache[cacheKey]
139-
if ok && entry.hsoGeneration == hso.Generation {
140-
h.cacheMutex.RUnlock()
141-
return entry.template, nil
142-
}
143-
h.cacheMutex.RUnlock()
144-
145-
h.cacheMutex.Lock()
146-
tmpl, err := template.New("inline").Parse(config.Content)
147-
if err != nil {
148-
h.cacheMutex.Unlock()
149-
return nil, err
150-
}
151-
h.templateCache[cacheKey] = &cacheEntry{
152-
template: tmpl,
153-
hsoGeneration: hso.Generation,
154-
}
155-
h.cacheMutex.Unlock()
156-
return tmpl, nil
126+
return h.getCachedInlineTemplate(hso, config.Content)
157127
}
158128

159129
if h.defaultTmpl != nil {
@@ -162,3 +132,30 @@ func (h *PlaceholderHandler) getTemplate(ctx context.Context, hso *v1alpha1.HTTP
162132

163133
return nil, fmt.Errorf("no placeholder template configured")
164134
}
135+
136+
func (h *PlaceholderHandler) getCachedInlineTemplate(hso *v1alpha1.HTTPScaledObject, content string) (*template.Template, error) {
137+
cacheKey := fmt.Sprintf("%s/%s/inline", hso.Namespace, hso.Name)
138+
139+
h.cacheMutex.RLock()
140+
entry, ok := h.templateCache[cacheKey]
141+
if ok && entry.hsoGeneration == hso.Generation {
142+
h.cacheMutex.RUnlock()
143+
return entry.template, nil
144+
}
145+
h.cacheMutex.RUnlock()
146+
147+
h.cacheMutex.Lock()
148+
defer h.cacheMutex.Unlock()
149+
150+
tmpl, err := template.New("inline").Parse(content)
151+
if err != nil {
152+
return nil, err
153+
}
154+
155+
h.templateCache[cacheKey] = &cacheEntry{
156+
template: tmpl,
157+
hsoGeneration: hso.Generation,
158+
}
159+
160+
return tmpl, nil
161+
}

‎interceptor/handler/placeholder_test.go‎

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -250,11 +250,11 @@ func TestGetTemplate_Caching(t *testing.T) {
250250

251251
ctx := context.Background()
252252

253-
tmpl1, err := handler.getTemplate(ctx, hso)
253+
tmpl1, err := handler.resolveTemplate(ctx, hso)
254254
require.NoError(t, err)
255255
assert.NotNil(t, tmpl1)
256256

257-
tmpl2, err := handler.getTemplate(ctx, hso)
257+
tmpl2, err := handler.resolveTemplate(ctx, hso)
258258
require.NoError(t, err)
259259
assert.NotNil(t, tmpl2)
260260

@@ -283,14 +283,14 @@ func TestGetTemplate_CacheInvalidation_Generation(t *testing.T) {
283283

284284
ctx := context.Background()
285285

286-
tmpl1, err := handler.getTemplate(ctx, hso)
286+
tmpl1, err := handler.resolveTemplate(ctx, hso)
287287
require.NoError(t, err)
288288
assert.NotNil(t, tmpl1)
289289

290290
hso.Generation = 2
291291
hso.Spec.PlaceholderConfig.Content = customContent2
292292

293-
tmpl2, err := handler.getTemplate(ctx, hso)
293+
tmpl2, err := handler.resolveTemplate(ctx, hso)
294294
require.NoError(t, err)
295295
assert.NotNil(t, tmpl2)
296296

@@ -299,7 +299,6 @@ func TestGetTemplate_CacheInvalidation_Generation(t *testing.T) {
299299

300300
func TestGetTemplate_CacheInvalidation_ConfigMapVersion_REMOVED(t *testing.T) {
301301
t.Skip("ConfigMap support removed per maintainer feedback")
302-
return
303302
// The code below is kept for reference but won't be executed
304303
/*
305304
cm := &v1.ConfigMap{
@@ -331,7 +330,7 @@ func TestGetTemplate_CacheInvalidation_ConfigMapVersion_REMOVED(t *testing.T) {
331330
332331
ctx := context.Background()
333332
334-
tmpl1, err := handler.getTemplate(ctx, hso)
333+
tmpl1, err := handler.resolveTemplate(ctx, hso)
335334
require.NoError(t, err)
336335
assert.NotNil(t, tmpl1)
337336
@@ -340,7 +339,7 @@ func TestGetTemplate_CacheInvalidation_ConfigMapVersion_REMOVED(t *testing.T) {
340339
_, err = k8sClient.CoreV1().ConfigMaps("default").Update(ctx, cm, metav1.UpdateOptions{})
341340
require.NoError(t, err)
342341
343-
tmpl2, err := handler.getTemplate(ctx, hso)
342+
tmpl2, err := handler.resolveTemplate(ctx, hso)
344343
require.NoError(t, err)
345344
assert.NotNil(t, tmpl2)
346345
*/
@@ -365,7 +364,7 @@ func TestGetTemplate_ConcurrentAccess(t *testing.T) {
365364

366365
ctx := context.Background()
367366

368-
_, err := handler.getTemplate(ctx, hso)
367+
_, err := handler.resolveTemplate(ctx, hso)
369368
require.NoError(t, err)
370369

371370
var wg sync.WaitGroup
@@ -376,7 +375,7 @@ func TestGetTemplate_ConcurrentAccess(t *testing.T) {
376375
wg.Add(1)
377376
go func() {
378377
defer wg.Done()
379-
tmpl, err := handler.getTemplate(ctx, hso)
378+
tmpl, err := handler.resolveTemplate(ctx, hso)
380379
if err != nil {
381380
errors <- err
382381
} else {
@@ -436,7 +435,7 @@ func TestGetTemplate_ConcurrentFirstAccess(t *testing.T) {
436435
wg.Add(1)
437436
go func() {
438437
defer wg.Done()
439-
_, err := handler.getTemplate(ctx, hso)
438+
_, err := handler.resolveTemplate(ctx, hso)
440439
if err != nil {
441440
errors <- err
442441
}
@@ -488,7 +487,7 @@ func TestGetTemplate_ConcurrentCacheUpdates(t *testing.T) {
488487
},
489488
}
490489

491-
_, err := handler.getTemplate(ctx, hso)
490+
_, err := handler.resolveTemplate(ctx, hso)
492491
if err != nil {
493492
errors <- err
494493
}

‎interceptor/proxy_handlers.go‎

Lines changed: 43 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313

1414
"github.com/kedacore/http-add-on/interceptor/config"
1515
"github.com/kedacore/http-add-on/interceptor/handler"
16+
"github.com/kedacore/http-add-on/operator/apis/http/v1alpha1"
1617
kedahttp "github.com/kedacore/http-add-on/pkg/http"
1718
"github.com/kedacore/http-add-on/pkg/k8s"
1819
kedanet "github.com/kedacore/http-add-on/pkg/net"
@@ -45,9 +46,6 @@ func newForwardingConfigFromTimeouts(t *config.Timeouts, s *config.Serving) forw
4546
}
4647
}
4748

48-
// newForwardingHandler takes in the service URL for the app backend
49-
// and forwards incoming requests to it. Note that it isn't multitenant.
50-
// It's intended to be deployed and scaled alongside the application itself.
5149
func newForwardingHandler(
5250
lggr logr.Logger,
5351
dialCtxFunc kedanet.DialContextFunc,
@@ -76,30 +74,8 @@ func newForwardingHandler(
7674
httpso := util.HTTPSOFromContext(ctx)
7775
hasFailover := httpso.Spec.ColdStartTimeoutFailoverRef != nil
7876

79-
// Check if we should serve a placeholder page
80-
// Ensure placeholderHandler is not nil to prevent panics even if placeholder is enabled in spec
81-
if httpso.Spec.PlaceholderConfig != nil && httpso.Spec.PlaceholderConfig.Enabled && placeholderHandler != nil {
82-
endpoints, err := endpointsCache.Get(httpso.GetNamespace(), httpso.Spec.ScaleTargetRef.Service)
83-
if err != nil {
84-
// Error getting endpoints cache - return 503 Service Unavailable
85-
lggr.Error(err, "failed to get endpoints from cache while placeholder is configured",
86-
"namespace", httpso.GetNamespace(),
87-
"service", httpso.Spec.ScaleTargetRef.Service)
88-
w.WriteHeader(http.StatusServiceUnavailable)
89-
if _, writeErr := w.Write([]byte("Service temporarily unavailable - unable to check service status")); writeErr != nil {
90-
lggr.Error(writeErr, "could not write error response to client")
91-
}
92-
return
93-
}
94-
95-
if workloadActiveEndpoints(endpoints) == 0 {
96-
if placeholderErr := placeholderHandler.ServePlaceholder(w, r, httpso); placeholderErr != nil {
97-
lggr.Error(placeholderErr, "failed to serve placeholder page")
98-
w.WriteHeader(http.StatusBadGateway)
99-
if _, err := w.Write([]byte("error serving placeholder page")); err != nil {
100-
lggr.Error(err, "could not write error response to client")
101-
}
102-
}
77+
if shouldServePlaceholder(httpso, placeholderHandler) {
78+
if err := servePlaceholderIfNoEndpoints(lggr, w, r, httpso, placeholderHandler, endpointsCache); err != nil {
10379
return
10480
}
10581
}
@@ -159,3 +135,43 @@ func newForwardingHandler(
159135
uh.ServeHTTP(w, r)
160136
})
161137
}
138+
139+
func shouldServePlaceholder(httpso *v1alpha1.HTTPScaledObject, placeholderHandler *handler.PlaceholderHandler) bool {
140+
return httpso.Spec.PlaceholderConfig != nil &&
141+
httpso.Spec.PlaceholderConfig.Enabled &&
142+
placeholderHandler != nil
143+
}
144+
145+
func servePlaceholderIfNoEndpoints(
146+
lggr logr.Logger,
147+
w http.ResponseWriter,
148+
r *http.Request,
149+
httpso *v1alpha1.HTTPScaledObject,
150+
placeholderHandler *handler.PlaceholderHandler,
151+
endpointsCache k8s.EndpointsCache,
152+
) error {
153+
endpoints, err := endpointsCache.Get(httpso.GetNamespace(), httpso.Spec.ScaleTargetRef.Service)
154+
if err != nil {
155+
lggr.Error(err, "failed to get endpoints from cache while placeholder is configured",
156+
"namespace", httpso.GetNamespace(),
157+
"service", httpso.Spec.ScaleTargetRef.Service)
158+
w.WriteHeader(http.StatusServiceUnavailable)
159+
if _, writeErr := w.Write([]byte("Service temporarily unavailable - unable to check service status")); writeErr != nil {
160+
lggr.Error(writeErr, "could not write error response to client")
161+
}
162+
return err
163+
}
164+
165+
if workloadActiveEndpoints(endpoints) == 0 {
166+
if placeholderErr := placeholderHandler.ServePlaceholder(w, r, httpso); placeholderErr != nil {
167+
lggr.Error(placeholderErr, "failed to serve placeholder page")
168+
w.WriteHeader(http.StatusBadGateway)
169+
if _, err := w.Write([]byte("error serving placeholder page")); err != nil {
170+
lggr.Error(err, "could not write error response to client")
171+
}
172+
}
173+
return fmt.Errorf("placeholder served")
174+
}
175+
176+
return nil
177+
}

‎operator/apis/http/v1alpha1/httpscaledobject_types.go‎

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -141,30 +141,23 @@ type Header struct {
141141

142142
// PlaceholderConfig defines the configuration for serving placeholder responses during scale-from-zero
143143
type PlaceholderConfig struct {
144-
// Enable placeholder response when replicas are scaled to zero
145144
// +kubebuilder:default=false
146145
// +optional
147146
Enabled bool `json:"enabled" description:"Enable placeholder response when replicas are scaled to zero"`
148-
// Inline content for placeholder response. Supports any format (HTML, JSON, XML, plain text, etc.)
149-
// Content is processed as a Go template with variables: ServiceName, Namespace, RefreshInterval, RequestID, Timestamp
150-
// Content-Type should be set via the Headers field to match your content format
147+
// Supports Go template variables: ServiceName, Namespace, RefreshInterval, RequestID, Timestamp
151148
// +optional
152149
Content string `json:"content,omitempty" description:"Inline content for placeholder response (any format supported)"`
153-
// HTTP status code to return with placeholder response
154150
// +kubebuilder:default=503
155151
// +kubebuilder:validation:Minimum=100
156152
// +kubebuilder:validation:Maximum=599
157153
// +optional
158154
StatusCode int32 `json:"statusCode,omitempty" description:"HTTP status code to return with placeholder response (Default 503)"`
159-
// RefreshInterval is a template variable available in content (in seconds)
160-
// This is just data passed to the template, not used by the interceptor for automatic refresh
155+
// Template variable only - not used by interceptor for automatic refresh
161156
// +kubebuilder:default=5
162157
// +kubebuilder:validation:Minimum=1
163158
// +kubebuilder:validation:Maximum=60
164159
// +optional
165160
RefreshInterval int32 `json:"refreshInterval,omitempty" description:"Template variable for refresh interval in seconds (Default 5)"`
166-
// Additional HTTP headers to include with placeholder response
167-
// Use this to set Content-Type matching your content format (e.g., 'Content-Type: application/json')
168161
// +optional
169162
Headers map[string]string `json:"headers,omitempty" description:"Additional HTTP headers to include with placeholder response"`
170163
}

0 commit comments

Comments
 (0)