pr feedback adjustments [not tested yet]

Signed-off-by: malpou <malthe@grundtvigsvej.dk>

Author: malpou

interceptor/handler/placeholder.go

@@ -108,11 +108,11 @@ type cacheEntry struct {
 
 // PlaceholderHandler handles serving placeholder pages during scale-from-zero
 type PlaceholderHandler struct {
-	templateCache  map[string]*cacheEntry
-	cacheMutex     sync.RWMutex
-	defaultTmpl    *template.Template
-	servingCfg     *config.Serving
-	enableScript   bool
+	templateCache map[string]*cacheEntry
+	cacheMutex    sync.RWMutex
+	defaultTmpl   *template.Template
+	servingCfg    *config.Serving
+	enableScript  bool
 }
 
 // PlaceholderData contains data for rendering placeholder templates
@@ -127,27 +127,27 @@ type PlaceholderData struct {
 // NewPlaceholderHandler creates a new placeholder handler
 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", 
+			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)
@@ -201,19 +201,19 @@ func detectContentType(acceptHeader string, content string) string {
 	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, "]")) {
+		(strings.HasPrefix(trimmed, "[") && strings.HasSuffix(trimmed, "]")) {
 		return "application/json"
 	}
 	if strings.HasPrefix(trimmed, "<") {
@@ -222,7 +222,7 @@ func detectContentType(acceptHeader string, content string) string {
 		}
 		return "text/html; charset=utf-8"
 	}
-	
+
 	return "text/plain; charset=utf-8"
 }
 
@@ -275,17 +275,17 @@ func (h *PlaceholderHandler) ServePlaceholder(w http.ResponseWriter, r *http.Req
 	}
 
 	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")

interceptor/handler/placeholder_test.go

@@ -329,47 +329,47 @@ func TestGetTemplate_CacheInvalidation_ConfigMapVersion_REMOVED(t *testing.T) {
 	return
 	// The code below is kept for reference but won't be executed
 	/*
-	cm := &v1.ConfigMap{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:            "placeholder-cm",
-			Namespace:       "default",
-			ResourceVersion: "1",
-		},
-		Data: map[string]string{
-			"template.html": `<html><body>Version 1: {{.ServiceName}}</body></html>`,
-		},
-	}
-	k8sClient := fake.NewSimpleClientset(cm)
-	routingTable := test.NewTable()
-	handler, _ := NewPlaceholderHandler(k8sClient, routingTable)
-
-	hso := &v1alpha1.HTTPScaledObject{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:       "test-app",
-			Namespace:  "default",
-			Generation: 1,
-		},
-		Spec: v1alpha1.HTTPScaledObjectSpec{
-			PlaceholderConfig: &v1alpha1.PlaceholderConfig{
-				ContentConfigMap: "placeholder-cm",
+		cm := &v1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:            "placeholder-cm",
+				Namespace:       "default",
+				ResourceVersion: "1",
 			},
-		},
-	}
+			Data: map[string]string{
+				"template.html": `<html><body>Version 1: {{.ServiceName}}</body></html>`,
+			},
+		}
+		k8sClient := fake.NewSimpleClientset(cm)
+		routingTable := test.NewTable()
+		handler, _ := NewPlaceholderHandler(k8sClient, routingTable)
+
+		hso := &v1alpha1.HTTPScaledObject{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:       "test-app",
+				Namespace:  "default",
+				Generation: 1,
+			},
+			Spec: v1alpha1.HTTPScaledObjectSpec{
+				PlaceholderConfig: &v1alpha1.PlaceholderConfig{
+					ContentConfigMap: "placeholder-cm",
+				},
+			},
+		}
 
-	ctx := context.Background()
+		ctx := context.Background()
 
-	tmpl1, err := handler.getTemplate(ctx, hso)
-	require.NoError(t, err)
-	assert.NotNil(t, tmpl1)
+		tmpl1, err := handler.getTemplate(ctx, hso)
+		require.NoError(t, err)
+		assert.NotNil(t, tmpl1)
 
-	cm.ResourceVersion = "2"
-	cm.Data["template.html"] = `<html><body>Version 2: {{.ServiceName}}</body></html>`
-	_, err = k8sClient.CoreV1().ConfigMaps("default").Update(ctx, cm, metav1.UpdateOptions{})
-	require.NoError(t, err)
+		cm.ResourceVersion = "2"
+		cm.Data["template.html"] = `<html><body>Version 2: {{.ServiceName}}</body></html>`
+		_, err = k8sClient.CoreV1().ConfigMaps("default").Update(ctx, cm, metav1.UpdateOptions{})
+		require.NoError(t, err)
 
-	tmpl2, err := handler.getTemplate(ctx, hso)
-	require.NoError(t, err)
-	assert.NotNil(t, tmpl2)
+		tmpl2, err := handler.getTemplate(ctx, hso)
+		require.NoError(t, err)
+		assert.NotNil(t, tmpl2)
 	*/
 }
 
@@ -646,52 +646,52 @@ func TestServePlaceholder_ConfigMapContentWithScriptInjection_REMOVED(t *testing
 	return
 	// The code below is kept for reference but won't be executed
 	/*
-	configMapContent := `<html><body><h1>ConfigMap placeholder for {{.ServiceName}}</h1></body></html>`
-	cm := &v1.ConfigMap{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "placeholder-cm",
-			Namespace: "default",
-		},
-		Data: map[string]string{
-			"template.html": configMapContent,
-		},
-	}
-	k8sClient := fake.NewSimpleClientset(cm)
-	routingTable := test.NewTable()
-	handler, _ := NewPlaceholderHandler(k8sClient, routingTable)
-
-	hso := &v1alpha1.HTTPScaledObject{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "test-app",
-			Namespace: "default",
-		},
-		Spec: v1alpha1.HTTPScaledObjectSpec{
-			ScaleTargetRef: v1alpha1.ScaleTargetRef{
-				Service: "test-service",
+		configMapContent := `<html><body><h1>ConfigMap placeholder for {{.ServiceName}}</h1></body></html>`
+		cm := &v1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "placeholder-cm",
+				Namespace: "default",
 			},
-			PlaceholderConfig: &v1alpha1.PlaceholderConfig{
-				Enabled:          true,
-				StatusCode:       503,
-				RefreshInterval:  15,
-				ContentConfigMap: "placeholder-cm",
+			Data: map[string]string{
+				"template.html": configMapContent,
 			},
-		},
-	}
+		}
+		k8sClient := fake.NewSimpleClientset(cm)
+		routingTable := test.NewTable()
+		handler, _ := NewPlaceholderHandler(k8sClient, routingTable)
+
+		hso := &v1alpha1.HTTPScaledObject{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "test-app",
+				Namespace: "default",
+			},
+			Spec: v1alpha1.HTTPScaledObjectSpec{
+				ScaleTargetRef: v1alpha1.ScaleTargetRef{
+					Service: "test-service",
+				},
+				PlaceholderConfig: &v1alpha1.PlaceholderConfig{
+					Enabled:          true,
+					StatusCode:       503,
+					RefreshInterval:  15,
+					ContentConfigMap: "placeholder-cm",
+				},
+			},
+		}
 
-	req := httptest.NewRequest("GET", "http://example.com", nil)
-	w := httptest.NewRecorder()
+		req := httptest.NewRequest("GET", "http://example.com", nil)
+		w := httptest.NewRecorder()
 
-	err := handler.ServePlaceholder(w, req, hso)
-	assert.NoError(t, err)
-	assert.Equal(t, http.StatusServiceUnavailable, w.Code)
+		err := handler.ServePlaceholder(w, req, hso)
+		assert.NoError(t, err)
+		assert.Equal(t, http.StatusServiceUnavailable, w.Code)
 
-	// Check that custom content is there
-	assert.Contains(t, w.Body.String(), "ConfigMap placeholder for test-service")
+		// Check that custom content is there
+		assert.Contains(t, w.Body.String(), "ConfigMap placeholder for test-service")
 
-	// Check that script was injected
-	assert.Contains(t, w.Body.String(), "checkServiceStatus")
-	assert.Contains(t, w.Body.String(), "X-KEDA-HTTP-Placeholder-Served")
-	assert.Contains(t, w.Body.String(), "checkInterval =  15  * 1000")
+		// Check that script was injected
+		assert.Contains(t, w.Body.String(), "checkServiceStatus")
+		assert.Contains(t, w.Body.String(), "X-KEDA-HTTP-Placeholder-Served")
+		assert.Contains(t, w.Body.String(), "checkInterval =  15  * 1000")
 	*/
 }
 
@@ -777,14 +777,14 @@ Please wait a moment...`
 	assert.NotContains(t, body, "<!DOCTYPE html>")
 	assert.NotContains(t, body, "<html>")
 	assert.NotContains(t, body, "<body>")
-	
+
 	// Check that original content is preserved as-is
 	assert.Contains(t, body, "Welcome! Your service is starting up.")
 	assert.Contains(t, body, "Please wait a moment...")
 
 	// Check that script was NOT injected into plain text
 	assert.NotContains(t, body, "checkServiceStatus")
-	
+
 	// Check content type is plain text
 	assert.Equal(t, "text/plain; charset=utf-8", w.Header().Get("Content-Type"))
 }

interceptor/main_test.go

@@ -81,6 +81,9 @@ func TestRunProxyServerCountMiddleware(t *testing.T) {
 		fmt.Println(err, "Error setting up tracer")
 	}
 
+	servingCfg := &config.Serving{
+		PlaceholderEnableScript: true,
+	}
 	g.Go(func() error {
 		return runProxyServer(
 			ctx,
@@ -90,6 +93,7 @@ func TestRunProxyServerCountMiddleware(t *testing.T) {
 			routingTable,
 			svcCache,
 			timeouts,
+			servingCfg,
 			port,
 			false,
 			map[string]interface{}{},
@@ -222,6 +226,9 @@ func TestRunProxyServerWithTLSCountMiddleware(t *testing.T) {
 		return false, nil
 	}
 	tracingCfg := config.Tracing{Enabled: true, Exporter: "otlphttp"}
+	servingCfg := &config.Serving{
+		PlaceholderEnableScript: true,
+	}
 
 	g.Go(func() error {
 		return runProxyServer(
@@ -232,6 +239,7 @@ func TestRunProxyServerWithTLSCountMiddleware(t *testing.T) {
 			routingTable,
 			svcCache,
 			timeouts,
+			servingCfg,
 			port,
 			true,
 			map[string]interface{}{"certificatePath": "../certs/tls.crt", "keyPath": "../certs/tls.key", "skipVerify": true},
@@ -374,6 +382,9 @@ func TestRunProxyServerWithMultipleCertsTLSCountMiddleware(t *testing.T) {
 	}
 
 	tracingCfg := config.Tracing{Enabled: true, Exporter: "otlphttp"}
+	servingCfg := &config.Serving{
+		PlaceholderEnableScript: true,
+	}
 
 	g.Go(func() error {
 		return runProxyServer(
@@ -384,6 +395,7 @@ func TestRunProxyServerWithMultipleCertsTLSCountMiddleware(t *testing.T) {
 			routingTable,
 			svcCache,
 			timeouts,
+			servingCfg,
 			port,
 			true,
 			map[string]interface{}{"certstorePaths": "../certs"},

interceptor/proxy_handlers.go

@@ -76,7 +76,7 @@ func newForwardingHandler(
 				}
 				return
 			}
-			
+
 			if workloadActiveEndpoints(endpoints) == 0 {
 				if placeholderErr := placeholderHandler.ServePlaceholder(w, r, httpso); placeholderErr != nil {
 					lggr.Error(placeholderErr, "failed to serve placeholder page")

interceptor/proxy_handlers_test.go

@@ -192,6 +192,8 @@ func TestImmediatelySuccessfulFailoverProxy(t *testing.T) {
 		},
 		&tls.Config{},
 		&config.Tracing{},
+		nil, // placeholderHandler
+		nil, // endpointsCache
 	)
 	const path = "/testfwd"
 	res, req, err := reqAndRes(path)