refactor: Address maintainer feedback on placeholder pages

malpou · malpou · commit d95a60bf77f1 · 2025-08-16T10:45:21.000+02:00
- 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
diff --git a/config/crd/bases/http.keda.sh_httpscaledobjects.yaml b/config/crd/bases/http.keda.sh_httpscaledobjects.yaml
@@ -113,16 +113,8 @@ spec:
                   scale-from-zero
                 properties:
                   content:
-                    description: Inline HTML content for placeholder page (takes precedence
-                      over ContentConfigMap)
-                    type: string
-                  contentConfigMap:
-                    description: Path to ConfigMap containing placeholder HTML content
-                      (in same namespace)
-                    type: string
-                  contentConfigMapKey:
-                    default: template.html
-                    description: Key in ConfigMap containing the HTML template
+                    description: Inline content for placeholder page (can be HTML,
+                      JSON, plain text, etc.)
                     type: string
                   enabled:
                     default: false
diff --git a/config/interceptor/deployment.yaml b/config/interceptor/deployment.yaml
@@ -54,6 +54,10 @@ spec:
           value: "10s"
         - name: KEDA_HTTP_EXPECT_CONTINUE_TIMEOUT
           value: "1s"
+        - name: KEDA_HTTP_PLACEHOLDER_DEFAULT_TEMPLATE_PATH
+          value: "/placeholder-templates/default.html"
+        - name: KEDA_HTTP_PLACEHOLDER_ENABLE_SCRIPT
+          value: "true"
         ports:
         - name: admin
           containerPort: 9090
@@ -83,9 +87,17 @@ spec:
           capabilities:
             drop:
               - ALL
+        volumeMounts:
+        - name: placeholder-templates
+          mountPath: /placeholder-templates
+          readOnly: true
       securityContext:
         runAsNonRoot: true
         seccompProfile:
           type: RuntimeDefault
       serviceAccountName: interceptor
       terminationGracePeriodSeconds: 10
+      volumes:
+      - name: placeholder-templates
+        configMap:
+          name: interceptor-placeholder-templates
diff --git a/config/interceptor/kustomization.yaml b/config/interceptor/kustomization.yaml
@@ -9,6 +9,7 @@ resources:
 - metrics.service.yaml
 - service_account.yaml
 - scaledobject.yaml
+- placeholder-templates.yaml
 configurations:
 - transformerconfig.yaml
 labels:
diff --git a/config/interceptor/placeholder-templates.yaml b/config/interceptor/placeholder-templates.yaml
@@ -0,0 +1,61 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: interceptor-placeholder-templates
+data:
+  default.html: |
+    <!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>
diff --git a/interceptor/config/serving.go b/interceptor/config/serving.go
@@ -49,6 +49,10 @@ type Serving struct {
 	TLSPort int `envconfig:"KEDA_HTTP_PROXY_TLS_PORT" default:"8443"`
 	// ProfilingAddr if not empty, pprof will be available on this address, assuming host:port here
 	ProfilingAddr string `envconfig:"PROFILING_BIND_ADDRESS" default:""`
+	// PlaceholderDefaultTemplatePath is the path to the default placeholder template file
+	PlaceholderDefaultTemplatePath string `envconfig:"KEDA_HTTP_PLACEHOLDER_DEFAULT_TEMPLATE_PATH" default:""`
+	// PlaceholderEnableScript controls whether to inject auto-refresh script into HTML responses
+	PlaceholderEnableScript bool `envconfig:"KEDA_HTTP_PLACEHOLDER_ENABLE_SCRIPT" default:"true"`
 }
 
 // Parse parses standard configs using envconfig and returns a pointer to the
diff --git a/interceptor/handler/placeholder.go b/interceptor/handler/placeholder.go
@@ -6,15 +6,13 @@ import (
 	"fmt"
 	"html/template"
 	"net/http"
+	"os"
 	"strings"
 	"sync"
 	"time"
 
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/client-go/kubernetes"
-
+	"github.com/kedacore/http-add-on/interceptor/config"
 	"github.com/kedacore/http-add-on/operator/apis/http/v1alpha1"
-	"github.com/kedacore/http-add-on/pkg/routing"
 )
 
 const placeholderScript = `<script>
@@ -110,10 +108,11 @@ type cacheEntry struct {
 
 // PlaceholderHandler handles serving placeholder pages during scale-from-zero
 type PlaceholderHandler struct {
-	k8sClient     kubernetes.Interface
-	templateCache map[string]*cacheEntry
-	cacheMutex    sync.RWMutex
-	defaultTmpl   *template.Template
+	templateCache  map[string]*cacheEntry
+	cacheMutex     sync.RWMutex
+	defaultTmpl    *template.Template
+	servingCfg     *config.Serving
+	enableScript   bool
 }
 
 // PlaceholderData contains data for rendering placeholder templates
@@ -126,19 +125,39 @@ type PlaceholderData struct {
 }
 
 // NewPlaceholderHandler creates a new placeholder handler
-func NewPlaceholderHandler(k8sClient kubernetes.Interface, routingTable routing.Table) (*PlaceholderHandler, error) {
-	// Combine the default template with the script
-	defaultTemplateWithScript := injectPlaceholderScript(defaultPlaceholderTemplateWithoutScript)
-	defaultTmpl, err := template.New("default").Parse(defaultTemplateWithScript)
+func NewPlaceholderHandler(servingCfg *config.Serving) (*PlaceholderHandler, error) {
+	var defaultTemplate string
+	
+	// Try to load template from configured path
+	if servingCfg.PlaceholderDefaultTemplatePath != "" {
+		content, err := os.ReadFile(servingCfg.PlaceholderDefaultTemplatePath)
+		if err == nil {
+			defaultTemplate = string(content)
+		} else {
+			// Fall back to built-in template if file cannot be read
+			fmt.Printf("Warning: Could not read placeholder template from %s: %v. Using built-in template.\n", 
+				servingCfg.PlaceholderDefaultTemplatePath, err)
+			defaultTemplate = defaultPlaceholderTemplateWithoutScript
+		}
+	} else {
+		defaultTemplate = defaultPlaceholderTemplateWithoutScript
+	}
+	
+	// Inject script if enabled
+	if servingCfg.PlaceholderEnableScript {
+		defaultTemplate = injectPlaceholderScript(defaultTemplate)
+	}
+	
+	defaultTmpl, err := template.New("default").Parse(defaultTemplate)
 	if err != nil {
 		return nil, fmt.Errorf("failed to parse default template: %w", err)
 	}
 
 	return &PlaceholderHandler{
-		k8sClient:     k8sClient,
-		routingTable:  routingTable,
 		templateCache: make(map[string]*cacheEntry),
 		defaultTmpl:   defaultTmpl,
+		servingCfg:    servingCfg,
+		enableScript:  servingCfg.PlaceholderEnableScript,
 	}, nil
 }
 
@@ -166,18 +185,45 @@ func injectPlaceholderScript(templateContent string) string {
 		return templateContent + placeholderScript
 	}
 
-	// For non-HTML content, wrap it in a minimal HTML structure with the script
-	return fmt.Sprintf(`<!DOCTYPE html>
-<html>
-<head>
-    <meta charset="utf-8">
-    <title>Service Starting</title>
-</head>
-<body>
-%s
-%s
-</body>
-</html>`, templateContent, placeholderScript)
+	// Don't wrap non-HTML content - return as-is
+	return templateContent
+}
+
+// detectContentType determines the appropriate content type based on Accept header and content
+func detectContentType(acceptHeader string, content string) string {
+	// Check Accept header for specific content types
+	if strings.Contains(acceptHeader, "application/json") {
+		return "application/json"
+	}
+	if strings.Contains(acceptHeader, "application/xml") {
+		return "application/xml"
+	}
+	if strings.Contains(acceptHeader, "text/plain") {
+		return "text/plain"
+	}
+	
+	// Default to HTML for browser requests or when HTML is accepted
+	if strings.Contains(acceptHeader, "text/html") || strings.Contains(acceptHeader, "*/*") || acceptHeader == "" {
+		// Check if content looks like HTML
+		if strings.Contains(content, "<") && strings.Contains(content, ">") {
+			return "text/html; charset=utf-8"
+		}
+	}
+	
+	// Try to detect based on content
+	trimmed := strings.TrimSpace(content)
+	if (strings.HasPrefix(trimmed, "{") && strings.HasSuffix(trimmed, "}")) ||
+	   (strings.HasPrefix(trimmed, "[") && strings.HasSuffix(trimmed, "]")) {
+		return "application/json"
+	}
+	if strings.HasPrefix(trimmed, "<") {
+		if strings.HasPrefix(trimmed, "<?xml") {
+			return "application/xml"
+		}
+		return "text/html; charset=utf-8"
+	}
+	
+	return "text/plain; charset=utf-8"
 }
 
 // ServePlaceholder serves a placeholder page based on the HTTPScaledObject configuration
@@ -194,19 +240,19 @@ func (h *PlaceholderHandler) ServePlaceholder(w http.ResponseWriter, r *http.Req
 		statusCode = http.StatusServiceUnavailable
 	}
 
+	// Set custom headers first
 	for k, v := range config.Headers {
 		w.Header().Set(k, v)
 	}
 
-	w.Header().Set("Content-Type", "text/html; charset=utf-8")
-	w.Header().Set("X-KEDA-HTTP-Placeholder-Served", "true")
-	w.Header().Set("Cache-Control", "no-cache, no-store, must-revalidate")
-
+	// Get template and render content
 	tmpl, err := h.getTemplate(r.Context(), hso)
 	if err != nil {
+		w.Header().Set("Content-Type", "text/plain; charset=utf-8")
+		w.Header().Set("X-KEDA-HTTP-Placeholder-Served", "true")
+		w.Header().Set("Cache-Control", "no-cache, no-store, must-revalidate")
 		w.WriteHeader(statusCode)
-		fmt.Fprintf(w, "<h1>%s is starting up...</h1><meta http-equiv='refresh' content='%d'>",
-			hso.Spec.ScaleTargetRef.Service, config.RefreshInterval)
+		fmt.Fprintf(w, "%s is starting up...\n", hso.Spec.ScaleTargetRef.Service)
 		return nil
 	}
 
@@ -220,14 +266,32 @@ func (h *PlaceholderHandler) ServePlaceholder(w http.ResponseWriter, r *http.Req
 
 	var buf bytes.Buffer
 	if err := tmpl.Execute(&buf, data); err != nil {
+		w.Header().Set("Content-Type", "text/plain; charset=utf-8")
+		w.Header().Set("X-KEDA-HTTP-Placeholder-Served", "true")
+		w.Header().Set("Cache-Control", "no-cache, no-store, must-revalidate")
 		w.WriteHeader(statusCode)
-		fmt.Fprintf(w, "<h1>%s is starting up...</h1><meta http-equiv='refresh' content='%d'>",
-			hso.Spec.ScaleTargetRef.Service, config.RefreshInterval)
+		fmt.Fprintf(w, "%s is starting up...\n", hso.Spec.ScaleTargetRef.Service)
 		return nil
 	}
 
+	content := buf.String()
+	
+	// Detect and set content type based on Accept header and content
+	contentType := detectContentType(r.Header.Get("Accept"), content)
+	
+	// For non-HTML content, don't inject script even if enabled
+	isHTML := strings.Contains(contentType, "text/html")
+	if !isHTML && h.enableScript && strings.Contains(content, placeholderScript) {
+		// Remove script from non-HTML content
+		content = strings.ReplaceAll(content, placeholderScript, "")
+	}
+	
+	w.Header().Set("Content-Type", contentType)
+	w.Header().Set("X-KEDA-HTTP-Placeholder-Served", "true")
+	w.Header().Set("Cache-Control", "no-cache, no-store, must-revalidate")
+
 	w.WriteHeader(statusCode)
-	_, err = w.Write(buf.Bytes())
+	_, err = w.Write([]byte(content))
 	return err
 }
 
@@ -246,58 +310,20 @@ func (h *PlaceholderHandler) getTemplate(ctx context.Context, hso *v1alpha1.HTTP
 		}
 		h.cacheMutex.RUnlock()
 
-		injectedContent := injectPlaceholderScript(config.Content)
-		tmpl, err := template.New("inline").Parse(injectedContent)
-		if err != nil {
-			return nil, err
-		}
-
 		h.cacheMutex.Lock()
-		h.templateCache[cacheKey] = &cacheEntry{
-			template:      tmpl,
-			hsoGeneration: hso.Generation,
+		content := config.Content
+		// Only inject script for HTML-like content if enabled
+		if h.enableScript && (strings.Contains(content, "<") && strings.Contains(content, ">")) {
+			content = injectPlaceholderScript(content)
 		}
-		h.cacheMutex.Unlock()
-		return tmpl, nil
-	}
-
-	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{})
-		if err != nil {
-			return nil, fmt.Errorf("failed to get ConfigMap %s: %w", config.ContentConfigMap, err)
-		}
-
-		h.cacheMutex.RLock()
-		entry, ok := h.templateCache[cacheKey]
-		if ok && entry.hsoGeneration == hso.Generation && entry.configMapVersion == cm.ResourceVersion {
-			h.cacheMutex.RUnlock()
-			return entry.template, nil
-		}
-		h.cacheMutex.RUnlock()
-
-		key := config.ContentConfigMapKey
-		if key == "" {
-			key = "template.html"
-		}
-
-		content, ok := cm.Data[key]
-		if !ok {
-			return nil, fmt.Errorf("key %s not found in ConfigMap %s", key, config.ContentConfigMap)
-		}
-
-		injectedContent := injectPlaceholderScript(content)
-		tmpl, err := template.New("configmap").Parse(injectedContent)
+		tmpl, err := template.New("inline").Parse(content)
 		if err != nil {
+			h.cacheMutex.Unlock()
 			return nil, err
 		}
-
-		h.cacheMutex.Lock()
 		h.templateCache[cacheKey] = &cacheEntry{
-			template:         tmpl,
-			hsoGeneration:    hso.Generation,
-			configMapVersion: cm.ResourceVersion,
+			template:      tmpl,
+			hsoGeneration: hso.Generation,
 		}
 		h.cacheMutex.Unlock()
 		return tmpl, nil
diff --git a/interceptor/handler/placeholder_test.go b/interceptor/handler/placeholder_test.go
diff --git a/interceptor/main.go b/interceptor/main.go
