From cc415e4d8a17d1ab7c11bed4682e68685a336987 Mon Sep 17 00:00:00 2001
From: Lucas Roesler <roesler.lucas@gmail.com>
Date: Fri, 7 Oct 2022 17:14:33 +0200
Subject: [PATCH] fix: return provider response during fnc listing errors

Return the original upstream response body when the the list request
returns an error. In general, the provider is returning useful and
actionable error messages for the user, the previous code hid this in
the logs and this is easy for user to overlook.

Additionally, remove an early return from error case after fetching
metrics. This looked like a bug and could result in empty api responses
if there was a prometheus error.

Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>
---
 gateway/metrics/add_metrics.go      |  6 ++----
 gateway/metrics/add_metrics_test.go | 29 +++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/gateway/metrics/add_metrics.go b/gateway/metrics/add_metrics.go
index 548bb8a72..3c35652fa 100644
--- a/gateway/metrics/add_metrics.go
+++ b/gateway/metrics/add_metrics.go
@@ -34,15 +34,13 @@ func AddMetricsHandler(handler http.HandlerFunc, prometheusQuery PrometheusQuery
 			log.Printf("List functions responded with code %d, body: %s",
 				recorder.Code,
 				string(upstreamBody))
-
-			http.Error(w, "Metrics handler: unexpected status code from provider listing functions", http.StatusInternalServerError)
+			http.Error(w, string(upstreamBody), recorder.Code)
 			return
 		}
 
 		var functions []types.FunctionStatus
 
 		err := json.Unmarshal(upstreamBody, &functions)
-
 		if err != nil {
 			log.Printf("Metrics upstream error: %s, value: %s", err, string(upstreamBody))
 
@@ -63,8 +61,8 @@ func AddMetricsHandler(handler http.HandlerFunc, prometheusQuery PrometheusQuery
 
 			results, err := prometheusQuery.Fetch(url.QueryEscape(q))
 			if err != nil {
+				// log the error but continue, the mixIn will correctly handle the empty results.
 				log.Printf("Error querying Prometheus: %s\n", err.Error())
-				return
 			}
 			mixIn(&functions, results)
 		}
diff --git a/gateway/metrics/add_metrics_test.go b/gateway/metrics/add_metrics_test.go
index 5636f4a09..cc6a64f85 100644
--- a/gateway/metrics/add_metrics_test.go
+++ b/gateway/metrics/add_metrics_test.go
@@ -5,6 +5,7 @@ import (
 	"log"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 
 	types "github.com/openfaas/faas-provider/types"
@@ -55,6 +56,34 @@ func Test_PrometheusMetrics_MixedInto_Services(t *testing.T) {
 	}
 }
 
+func Test_MetricHandler_ForwardsErrors(t *testing.T) {
+	functionsHandler := func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusConflict)
+		w.Write([]byte("test error case"))
+	}
+	// explicitly set the query fetcher to nil because it should
+	// not be called when a non-200 response is returned from the
+	// functions handler, if it is called then the test will panic
+	handler := AddMetricsHandler(functionsHandler, nil)
+
+	rr := httptest.NewRecorder()
+	request, _ := http.NewRequest(http.MethodGet, "/system/functions", nil)
+	handler.ServeHTTP(rr, request)
+
+	if status := rr.Code; status != http.StatusConflict {
+		t.Errorf("handler returned wrong status code: got %v want %v", status, http.StatusConflict)
+	}
+
+	if rr.Header().Get("Content-Type") != "text/plain; charset=utf-8" {
+		t.Errorf("Want 'text/plain; charset=utf-8' content-type, got: %s", rr.Header().Get("Content-Type"))
+	}
+	body := strings.TrimSpace(rr.Body.String())
+	if body != "test error case" {
+		t.Errorf("Want 'test error case', got: %q", body)
+	}
+
+}
+
 func Test_FunctionsHandler_ReturnsJSONAndOneFunction(t *testing.T) {
 	functionsHandler := makeFunctionsHandler()