Skip to content
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

fix: return provider response during fnc listing errors #1763

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions gateway/metrics/add_metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I am OK with this change, let's hope nobody is looking for that specific message because I think it's been there a long time.

return
}

var functions []types.FunctionStatus

err := json.Unmarshal(upstreamBody, &functions)

if err != nil {
log.Printf("Metrics upstream error: %s, value: %s", err, string(upstreamBody))

Expand All @@ -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)
}
Expand Down
29 changes: 29 additions & 0 deletions gateway/metrics/add_metrics_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"log"
"net/http"
"net/http/httptest"
"strings"
"testing"

types "github.com/openfaas/faas-provider/types"
Expand Down Expand Up @@ -55,6 +56,34 @@ func Test_PrometheusMetrics_MixedInto_Services(t *testing.T) {
}
}

func Test_MetricHandler_ForwardsErrors(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add one more test to show the positive case, if we have that already, just let me know and I'll go ahead and approve.

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()

Expand Down