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 <richard@technologee.co.uk>

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)
+	}
+}

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 {

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

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