From aaf9f18d933ca515cef32c7825f42ce3da19c268 Mon Sep 17 00:00:00 2001 From: Gaurav Gahlot Date: Tue, 3 Nov 2020 16:15:50 +0530 Subject: [PATCH] Fixed golangci-lint issues: /fission/pkg/router (#1831) --- go.mod | 1 - pkg/router/analytics.go | 9 +++-- pkg/router/functionHandler.go | 54 +++++++++++++++---------- pkg/router/functionReferenceResolver.go | 10 ++--- pkg/router/functionServiceMap.go | 6 +-- pkg/router/httpTriggers.go | 1 + pkg/router/mutablemux.go | 2 +- pkg/router/mutablemux_test.go | 21 +++++++--- pkg/router/requesthHeader.go | 1 + pkg/router/router.go | 17 +++++--- pkg/router/util_test.go | 4 +- 11 files changed, 77 insertions(+), 49 deletions(-) diff --git a/go.mod b/go.mod index c326b52a..999ae51e 100644 --- a/go.mod +++ b/go.mod @@ -53,7 +53,6 @@ require ( github.com/opencontainers/image-spec v1.0.1 // indirect github.com/opencontainers/runc v0.1.1 // indirect github.com/ory/dockertest v3.3.5+incompatible - github.com/pierrec/lz4 v2.0.5+incompatible // indirect github.com/pkg/errors v0.9.1 github.com/prometheus/client_golang v1.0.0 github.com/prometheus/common v0.4.1 diff --git a/pkg/router/analytics.go b/pkg/router/analytics.go index dc4985e4..b69aefae 100644 --- a/pkg/router/analytics.go +++ b/pkg/router/analytics.go @@ -12,21 +12,22 @@ import ( ) type ( - // Analytics struct + // Analytics helps exporting usage metrics Analytics struct { id string url string } - // AnalyticsData struct + + // AnalyticsData is the data exported as usage metrics AnalyticsData struct { ID string `json:"ID"` FunctionCallCount uint64 `json:"FunctionCallCount"` } ) -// MakeAnalytics returns Analytics if url is not empty +// MakeAnalytics returns a new instance of Analytics if url or +// 'ANALYTICS_URL' environment variable is set; nil otherwise func MakeAnalytics(url string) *Analytics { - if len(url) == 0 { url = os.Getenv("ANALYTICS_URL") if len(url) == 0 { diff --git a/pkg/router/functionHandler.go b/pkg/router/functionHandler.go index 08ced339..71f5ac7a 100644 --- a/pkg/router/functionHandler.go +++ b/pkg/router/functionHandler.go @@ -42,7 +42,10 @@ import ( ) const ( - FORWARDED = "Forwarded" + // FORWARDED represents the 'Forwarded' request header + FORWARDED = "Forwarded" + + // X_FORWARDED_HOST represents the 'X_FORWARDED_HOST' request header X_FORWARDED_HOST = "X-Forwarded-Host" ) @@ -54,7 +57,7 @@ type ( function *fv1.Function httpTrigger *fv1.HTTPTrigger functionMap map[string]*fv1.Function - fnWeightDistributionList []FunctionWeightDistribution + fnWeightDistributionList []functionWeightDistribution tsRoundTripperParams *tsRoundTripperParams isDebugEnv bool svcAddrUpdateThrottler *throttler.Throttler @@ -82,13 +85,13 @@ type ( svcAddrRetryCount int } - // A layer on top of http.DefaultTransport, with retries. + // RetryingRoundTripper is a layer on top of http.DefaultTransport, with retries. RetryingRoundTripper struct { logger *zap.Logger funcHandler *functionHandler funcTimeout time.Duration closeContextFunc *context.CancelFunc - serviceUrl *url.URL + serviceURL *url.URL urlFromCache bool totalRetry int } @@ -184,7 +187,7 @@ func (roundTripper *RetryingRoundTripper) RoundTrip(req *http.Request) (*http.Re // trying to get new service url from cache/executor. if retryCounter == 0 { // get function service url from cache or executor - roundTripper.serviceUrl, err = roundTripper.funcHandler.getServiceEntryFromExecutor() + roundTripper.serviceURL, err = roundTripper.funcHandler.getServiceEntryFromExecutor() if err != nil { // We might want a specific error code or header for fission failures as opposed to // user function bugs. @@ -210,15 +213,15 @@ func (roundTripper *RetryingRoundTripper) RoundTrip(req *http.Request) (*http.Re // } } if roundTripper.funcHandler.function.Spec.InvokeStrategy.ExecutionStrategy.ExecutorType == fv1.ExecutorTypePoolmgr { - defer func(fn *fv1.Function, serviceUrl *url.URL) { - go roundTripper.funcHandler.unTapService(fn, serviceUrl) - }(roundTripper.funcHandler.function, roundTripper.serviceUrl) + defer func(fn *fv1.Function, serviceURL *url.URL) { + go roundTripper.funcHandler.unTapService(fn, serviceURL) //nolint errcheck + }(roundTripper.funcHandler.function, roundTripper.serviceURL) } // modify the request to reflect the service url // this service url comes from executor response - req.URL.Scheme = roundTripper.serviceUrl.Scheme - req.URL.Host = roundTripper.serviceUrl.Host + req.URL.Scheme = roundTripper.serviceURL.Scheme + req.URL.Host = roundTripper.serviceURL.Host // To keep the function run container simple, it // doesn't do any routing. In the future if we have @@ -230,7 +233,7 @@ func (roundTripper *RetryingRoundTripper) RoundTrip(req *http.Request) (*http.Re // Overwrite request host with internal host, // or request will be blocked in some situations // (e.g. istio-proxy) - req.Host = roundTripper.serviceUrl.Host + req.Host = roundTripper.serviceURL.Host } // over-riding default settings. @@ -350,11 +353,11 @@ func (roundTripper *RetryingRoundTripper) closeContext() { } } -func (fh *functionHandler) tapService(fn *fv1.Function, serviceUrl *url.URL) { +func (fh *functionHandler) tapService(fn *fv1.Function, serviceURL *url.URL) { if fh.executor == nil { return } - fh.executor.TapService(fn.ObjectMeta, fn.Spec.InvokeStrategy.ExecutionStrategy.ExecutorType, serviceUrl) + fh.executor.TapService(fn.ObjectMeta, fn.Spec.InvokeStrategy.ExecutionStrategy.ExecutorType, serviceURL) } func (fh functionHandler) handler(responseWriter http.ResponseWriter, request *http.Request) { @@ -426,7 +429,7 @@ func (fh functionHandler) handler(responseWriter http.ResponseWriter, request *h // findCeil picks a function from the functionWeightDistribution list based on the // random number generated. It uses the prefix calculated for the function weights. -func findCeil(randomNumber int, wtDistrList []FunctionWeightDistribution) string { +func findCeil(randomNumber int, wtDistrList []functionWeightDistribution) string { low := 0 high := len(wtDistrList) - 1 @@ -445,13 +448,12 @@ func findCeil(randomNumber int, wtDistrList []FunctionWeightDistribution) string if wtDistrList[low].sumPrefix >= randomNumber { return wtDistrList[low].name - } else { - return "" } + return "" } // picks a function to route to based on a random number generated -func getCanaryBackend(fnMap map[string]*fv1.Function, fnWtDistributionList []FunctionWeightDistribution) *fv1.Function { +func getCanaryBackend(fnMap map[string]*fv1.Function, fnWtDistributionList []functionWeightDistribution) *fv1.Function { randomNumber := rand.Intn(fnWtDistributionList[len(fnWtDistributionList)-1].sumPrefix + 1) fnName := findCeil(randomNumber, fnWtDistributionList) return fnMap[fnName] @@ -541,15 +543,15 @@ func (fh functionHandler) getServiceEntryFromExecutor() (*url.URL, error) { } // parse the address into url - serviceUrl, err := url.Parse(fmt.Sprintf("http://%v", service)) + serviceURL, err := url.Parse(fmt.Sprintf("http://%v", service)) if err != nil { fh.logger.Error("error parsing service url", zap.Error(err), - zap.String("service_url", serviceUrl.String())) + zap.String("service_url", serviceURL.String())) return nil, err } - return serviceUrl, nil + return serviceURL, nil } // getProxyErrorHandler returns a reverse proxy error handler @@ -584,7 +586,15 @@ func (fh functionHandler) getProxyErrorHandler(start time.Time, rrt *RetryingRou // TODO: return error message that contains traceable UUID back to user. Issue #693 rw.WriteHeader(status) - rw.Write([]byte(msg)) + _, err = rw.Write([]byte(msg)) + if err != nil { + fh.logger.Error( + "error writing HTTP response", + zap.Error(err), + zap.Any("function", fh.function), + zap.Any("request_header", req.Header), + ) + } } } @@ -613,7 +623,7 @@ func (fh functionHandler) collectFunctionMetric(start time.Time, rrt *RetryingRo // tapService before invoking roundTrip for the serviceUrl if rrt.urlFromCache { - fh.tapService(fh.function, rrt.serviceUrl) + fh.tapService(fh.function, rrt.serviceURL) } fh.logger.Debug("Request complete", zap.String("function", fh.function.ObjectMeta.Name), diff --git a/pkg/router/functionReferenceResolver.go b/pkg/router/functionReferenceResolver.go index 87af231c..d13f228c 100644 --- a/pkg/router/functionReferenceResolver.go +++ b/pkg/router/functionReferenceResolver.go @@ -39,7 +39,7 @@ type ( resolveResultType int - FunctionWeightDistribution struct { + functionWeightDistribution struct { name string weight int sumPrefix int @@ -51,7 +51,7 @@ type ( resolveResult struct { resolveResultType functionMap map[string]*fv1.Function - functionWtDistributionList []FunctionWeightDistribution + functionWtDistributionList []functionWeightDistribution } // namespacedTriggerReference is just a trigger reference plus a @@ -112,7 +112,7 @@ func (frr *functionReferenceResolver) resolve(trigger fv1.HTTPTrigger) (*resolve } // cache resolve result - frr.refCache.Set(nfr, *rr) + frr.refCache.Set(nfr, *rr) //nolint: errcheck return rr, nil } @@ -149,7 +149,7 @@ func (frr *functionReferenceResolver) resolveByName(namespace, name string) (*re func (frr *functionReferenceResolver) resolveByFunctionWeights(namespace string, fr *fv1.FunctionReference) (*resolveResult, error) { functionMap := make(map[string]*fv1.Function) - fnWtDistrList := make([]FunctionWeightDistribution, 0) + fnWtDistrList := make([]functionWeightDistribution, 0) sumPrefix := 0 for functionName, functionWeight := range fr.FunctionWeights { @@ -170,7 +170,7 @@ func (frr *functionReferenceResolver) resolveByFunctionWeights(namespace string, f := obj.(*fv1.Function) functionMap[f.ObjectMeta.Name] = f sumPrefix = sumPrefix + functionWeight - fnWtDistrList = append(fnWtDistrList, FunctionWeightDistribution{ + fnWtDistrList = append(fnWtDistrList, functionWeightDistribution{ name: functionName, weight: functionWeight, sumPrefix: sumPrefix, diff --git a/pkg/router/functionServiceMap.go b/pkg/router/functionServiceMap.go index 7a11578f..0fed6a70 100644 --- a/pkg/router/functionServiceMap.go +++ b/pkg/router/functionServiceMap.go @@ -66,11 +66,11 @@ func (fmap *functionServiceMap) lookup(f *metav1.ObjectMeta) (*url.URL, error) { return u, nil } -func (fmap *functionServiceMap) assign(f *metav1.ObjectMeta, serviceUrl *url.URL) { +func (fmap *functionServiceMap) assign(f *metav1.ObjectMeta, serviceURL *url.URL) { mk := keyFromMetadata(f) - old, err := fmap.cache.Set(*mk, serviceUrl) + old, err := fmap.cache.Set(*mk, serviceURL) if err != nil { - if *serviceUrl == *(old.(*url.URL)) { + if *serviceURL == *(old.(*url.URL)) { return } fmap.logger.Error("error caching service url for function with a different value", zap.Error(err)) diff --git a/pkg/router/httpTriggers.go b/pkg/router/httpTriggers.go index 37ddf2ce..4e821861 100644 --- a/pkg/router/httpTriggers.go +++ b/pkg/router/httpTriggers.go @@ -37,6 +37,7 @@ import ( "github.com/fission/fission/pkg/utils" ) +// HTTPTriggerSet represents an HTTP trigger set type HTTPTriggerSet struct { *functionServiceMap *mutableRouter diff --git a/pkg/router/mutablemux.go b/pkg/router/mutablemux.go index ee35cc7b..b98c0f9f 100644 --- a/pkg/router/mutablemux.go +++ b/pkg/router/mutablemux.go @@ -34,7 +34,7 @@ type mutableRouter struct { router atomic.Value // mux.Router } -func NewMutableRouter(logger *zap.Logger, handler *mux.Router) *mutableRouter { +func newMutableRouter(logger *zap.Logger, handler *mux.Router) *mutableRouter { mr := mutableRouter{ logger: logger.Named("mutable_router"), } diff --git a/pkg/router/mutablemux_test.go b/pkg/router/mutablemux_test.go index 34551d3b..ca951f16 100644 --- a/pkg/router/mutablemux_test.go +++ b/pkg/router/mutablemux_test.go @@ -28,19 +28,28 @@ import ( ) func OldHandler(responseWriter http.ResponseWriter, request *http.Request) { - responseWriter.Write([]byte("old handler")) + _, err := responseWriter.Write([]byte("old handler")) + if err != nil { + log.Fatal(err) + } } func NewHandler(responseWriter http.ResponseWriter, request *http.Request) { - responseWriter.Write([]byte("new handler")) + _, err := responseWriter.Write([]byte("new handler")) + if err != nil { + log.Fatal(err) + } } func verifyRequest(expectedResponse string) { - targetUrl := "http://localhost:3333" - testRequest(targetUrl, expectedResponse) + targetURL := "http://localhost:3333" + testRequest(targetURL, expectedResponse) } func startServer(mr *mutableRouter) { - http.ListenAndServe(":3333", mr) + err := http.ListenAndServe(":3333", mr) + if err != nil { + log.Fatal(err) + } } func spamServer(quit chan bool) { @@ -70,7 +79,7 @@ func TestMutableMux(t *testing.T) { logger, err := config.Build() panicIf(err) - mr := NewMutableRouter(logger, muxRouter) + mr := newMutableRouter(logger, muxRouter) // start http server log.Print("Start http server") diff --git a/pkg/router/requesthHeader.go b/pkg/router/requesthHeader.go index 9388fe4f..f6994226 100644 --- a/pkg/router/requesthHeader.go +++ b/pkg/router/requesthHeader.go @@ -26,6 +26,7 @@ import ( ) const ( + // HEADERS_FISSION_FUNCTION_PREFIX represents a function prefix request header HEADERS_FISSION_FUNCTION_PREFIX = "Fission-Function" ) diff --git a/pkg/router/router.go b/pkg/router/router.go index 1684a9ce..0fbf8af9 100644 --- a/pkg/router/router.go +++ b/pkg/router/router.go @@ -69,9 +69,9 @@ func router(ctx context.Context, logger *zap.Logger, httpTriggerSet *HTTPTrigger // see issue https://github.com/fission/fission/issues/1317 useEncodedPath, _ := strconv.ParseBool(os.Getenv("USE_ENCODED_PATH")) if useEncodedPath { - mr = NewMutableRouter(logger, mux.NewRouter().UseEncodedPath()) + mr = newMutableRouter(logger, mux.NewRouter().UseEncodedPath()) } else { - mr = NewMutableRouter(logger, mux.NewRouter()) + mr = newMutableRouter(logger, mux.NewRouter()) } httpTriggerSet.subscribeRouter(ctx, mr, resolver) @@ -83,7 +83,7 @@ func serve(ctx context.Context, logger *zap.Logger, port int, tracingSamplingRat mr := router(ctx, logger, httpTriggerSet, resolver) url := fmt.Sprintf(":%v", port) - http.ListenAndServe(url, &ochttp.Handler{ + err := http.ListenAndServe(url, &ochttp.Handler{ Handler: mr, GetStartOptions: func(r *http.Request) trace.StartOptions { // do not trace router healthz endpoint @@ -101,6 +101,12 @@ func serve(ctx context.Context, logger *zap.Logger, port int, tracingSamplingRat } }, }) + if err != nil { + logger.Error( + "HTTP server error", + zap.Error(err), + ) + } } func serveMetric(logger *zap.Logger) { @@ -111,7 +117,8 @@ func serveMetric(logger *zap.Logger) { logger.Fatal("done listening on metrics endpoint", zap.Error(err)) } -func Start(logger *zap.Logger, port int, executorUrl string) { +// Start starts a router +func Start(logger *zap.Logger, port int, executorURL string) { _ = MakeAnalytics("") fmap := makeFunctionServiceMap(logger, time.Minute) @@ -126,7 +133,7 @@ func Start(logger *zap.Logger, port int, executorUrl string) { logger.Fatal("error waiting for CRDs", zap.Error(err)) } - executor := executorClient.MakeClient(logger, executorUrl) + executor := executorClient.MakeClient(logger, executorURL) timeoutStr := os.Getenv("ROUTER_ROUND_TRIP_TIMEOUT") timeout, err := time.ParseDuration(timeoutStr) diff --git a/pkg/router/util_test.go b/pkg/router/util_test.go index f3e3ce0f..be0e315a 100644 --- a/pkg/router/util_test.go +++ b/pkg/router/util_test.go @@ -6,8 +6,8 @@ import ( "net/http" ) -func testRequest(targetUrl string, expectedResponse string) { - resp, err := http.Get(targetUrl) +func testRequest(targetURL string, expectedResponse string) { + resp, err := http.Get(targetURL) if err != nil { log.Panicf("failed to make get request: %v", err) }