From df6f4c49f2552dc747e18d04575f7b4d1ab9e70a Mon Sep 17 00:00:00 2001 From: Richard Gee Date: Fri, 21 Sep 2018 23:16:31 +0100 Subject: [PATCH] Add checking for existent function in GetReplicas Within MakeScalingHandler() there is a call to GetReplicas() which was not returning an error when a non-200 http response was received from /system/function/. The call would also return a populated struct, so the perception was that a function existed an had been scaled to zero. This meant that the function would be added to the function cache and the code would continue into SetReplicas() where an attempt would be made to scale up a non-existent function. This change amends GetReplicas() so that it will return an error if the gateway returns anything other than a 200 reponse code from the /system/function/ endpoint. This causes MakeScalingHandler() to return earlier with an error indicating that the function could not be found. The cache.Set call is also moved to after the error check so that the cache is only updated to include existent functions. During investigations as to the cause of #876 tests were added to function_cache to check that Get() is behaving as intended when function exists and when not. Tests are also added to plugin/external to test that GetReplicas() and SetReplicas() are following their intended modes of operation when 200 and non-200 responses are received from the gateway. Signed-off-by: Richard Gee --- gateway/handlers/function_cache_test.go | 40 +++++++++ gateway/handlers/scaling.go | 3 +- gateway/plugin/external.go | 3 + gateway/plugin/external_test.go | 109 +++++++++++++++++++++++- 4 files changed, 153 insertions(+), 2 deletions(-) diff --git a/gateway/handlers/function_cache_test.go b/gateway/handlers/function_cache_test.go index 8d0cb064..edee342c 100644 --- a/gateway/handlers/function_cache_test.go +++ b/gateway/handlers/function_cache_test.go @@ -73,3 +73,43 @@ func Test_CacheGivesHitWithLongExpiry(t *testing.T) { t.Errorf("hit, want: %v, got %v", wantHit, hit) } } + +func Test_CacheFunctionExists(t *testing.T) { + fnName := "echo" + + cache := FunctionCache{ + Cache: make(map[string]*FunctionMeta), + Expiry: time.Millisecond * 10, + } + + cache.Set(fnName, ServiceQueryResponse{AvailableReplicas: 1}) + time.Sleep(time.Millisecond * 2) + + _, hit := cache.Get(fnName) + + wantHit := true + + if hit != wantHit { + t.Errorf("hit, want: %v, got %v", wantHit, hit) + } +} +func Test_CacheFunctionNotExist(t *testing.T) { + fnName := "echo" + testName := "burt" + + cache := FunctionCache{ + Cache: make(map[string]*FunctionMeta), + Expiry: time.Millisecond * 10, + } + + cache.Set(fnName, ServiceQueryResponse{AvailableReplicas: 1}) + time.Sleep(time.Millisecond * 2) + + _, hit := cache.Get(testName) + + wantHit := false + + if hit != wantHit { + t.Errorf("hit, want: %v, got %v", wantHit, hit) + } +} diff --git a/gateway/handlers/scaling.go b/gateway/handlers/scaling.go index 5827078d..67f45ef1 100644 --- a/gateway/handlers/scaling.go +++ b/gateway/handlers/scaling.go @@ -36,7 +36,6 @@ func MakeScalingHandler(next http.HandlerFunc, upstream http.HandlerFunc, config } queryResponse, err := config.ServiceQuery.GetReplicas(functionName) - cache.Set(functionName, queryResponse) if err != nil { var errStr string @@ -48,6 +47,8 @@ func MakeScalingHandler(next http.HandlerFunc, upstream http.HandlerFunc, config return } + cache.Set(functionName, queryResponse) + if queryResponse.AvailableReplicas == 0 { minReplicas := uint64(1) if queryResponse.MinReplicas > 0 { diff --git a/gateway/plugin/external.go b/gateway/plugin/external.go index 4a9bfd3f..b8fcd6ae 100644 --- a/gateway/plugin/external.go +++ b/gateway/plugin/external.go @@ -61,6 +61,7 @@ type ScaleServiceRequest struct { // GetReplicas replica count for function func (s ExternalServiceQuery) GetReplicas(serviceName string) (handlers.ServiceQueryResponse, error) { var err error + var emptyServiceQueryResponse handlers.ServiceQueryResponse function := requests.Function{} @@ -88,6 +89,8 @@ func (s ExternalServiceQuery) GetReplicas(serviceName string) (handlers.ServiceQ if err != nil { log.Println(urlPath, err) } + } else { + return emptyServiceQueryResponse, fmt.Errorf("server returned non-200 status code (%d) for function, %s", res.StatusCode, serviceName) } } diff --git a/gateway/plugin/external_test.go b/gateway/plugin/external_test.go index c2698ab3..1d14ab00 100644 --- a/gateway/plugin/external_test.go +++ b/gateway/plugin/external_test.go @@ -1,6 +1,15 @@ package plugin -import "testing" +import ( + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/openfaas/faas-provider/auth" + "github.com/openfaas/faas/gateway/handlers" +) const fallbackValue = 120 @@ -30,3 +39,101 @@ func TestLabelValueWasInValid(t *testing.T) { t.Fail() } } +func TestGetReplicasNonExistentFn(t *testing.T) { + + testServer := httptest.NewServer( + http.HandlerFunc(func(res http.ResponseWriter, req *http.Request) { + res.WriteHeader(http.StatusNotFound) + })) + defer testServer.Close() + + var creds auth.BasicAuthCredentials + + url, _ := url.Parse(testServer.URL + "/") + + esq := NewExternalServiceQuery(*url, &creds) + + svcQryResp, err := esq.GetReplicas("burt") + + if err == nil { + t.Logf("Error was nil, expected non-nil - the service query response value was %+v ", svcQryResp) + t.Fail() + } +} + +func TestGetReplicasExistentFn(t *testing.T) { + + testServer := httptest.NewServer( + http.HandlerFunc(func(res http.ResponseWriter, req *http.Request) { + res.WriteHeader(http.StatusOK) + res.Write([]byte(`{"json":"body"}`)) + })) + defer testServer.Close() + + expectedSvcQryResp := handlers.ServiceQueryResponse{ + Replicas: 0, + MaxReplicas: uint64(handlers.DefaultMaxReplicas), + MinReplicas: uint64(handlers.DefaultMinReplicas), + ScalingFactor: uint64(handlers.DefaultScalingFactor), + AvailableReplicas: 0, + } + + var creds auth.BasicAuthCredentials + + url, _ := url.Parse(testServer.URL + "/") + + esq := NewExternalServiceQuery(*url, &creds) + + svcQryResp, err := esq.GetReplicas("burt") + + if err != nil { + t.Logf("Expected err to be nil got: %s ", err.Error()) + t.Fail() + } + if svcQryResp != expectedSvcQryResp { + t.Logf("Unexpected return values - wanted %+v, got: %+v ", expectedSvcQryResp, svcQryResp) + t.Fail() + } +} + +func TestSetReplicasNonExistentFn(t *testing.T) { + + testServer := httptest.NewServer( + http.HandlerFunc(func(res http.ResponseWriter, req *http.Request) { + res.WriteHeader(http.StatusInternalServerError) + })) + defer testServer.Close() + + var creds auth.BasicAuthCredentials + url, _ := url.Parse(testServer.URL + "/") + esq := NewExternalServiceQuery(*url, &creds) + + err := esq.SetReplicas("burt", 1) + + expectedErrStr := "error scaling HTTP code 500" + + if !strings.Contains(err.Error(), expectedErrStr) { + t.Logf("Wanted string containing %s, got %s", expectedErrStr, err.Error()) + t.Fail() + } +} + +func TestSetReplicasExistentFn(t *testing.T) { + + testServer := httptest.NewServer( + http.HandlerFunc(func(res http.ResponseWriter, req *http.Request) { + res.WriteHeader(http.StatusOK) + })) + defer testServer.Close() + + var creds auth.BasicAuthCredentials + url, _ := url.Parse(testServer.URL + "/") + esq := NewExternalServiceQuery(*url, &creds) + + err := esq.SetReplicas("burt", 1) + + if err != nil { + t.Logf("Expected err to be nil got: %s ", err.Error()) + t.Fail() + } +}