Added missing filter parameters

Closes #18

filters/flowid/filter.go

@@ -1,6 +1,7 @@
 package flowid
 
 import (
+	"errors"
 	"github.com/zalando/skipper/skipper"
 )
 
@@ -12,10 +13,15 @@ const (
 type flowId struct {
 	id            string
 	reuseExisting bool
+	flowIdLength  uint8
 }
 
-func New(id string, allowOverride bool) skipper.Filter {
-	return &flowId{id, allowOverride}
+var (
+	ErrInvalidFilterParameters = errors.New("Invalid filter parameters")
+)
+
+func New(id string, allowOverride bool, len uint8) skipper.Filter {
+	return &flowId{id, allowOverride, len}
 }
 
 func (this *flowId) Id() string { return this.id }
@@ -28,13 +34,12 @@ func (this *flowId) Request(fc skipper.FilterContext) {
 
 	if this.reuseExisting {
 		flowId = r.Header.Get(flowIdHeaderName)
+		if isValid(flowId) {
+			return
+		}
 	}
 
-	var err error
-	if !isValid(flowId) {
-		flowId, err = newFlowId(defaultLen)
-	}
-
+	flowId, err := newFlowId(this.flowIdLength)
 	if err == nil {
 		fc.Request().Header.Set(flowIdHeaderName, flowId)
 	}
@@ -43,6 +48,21 @@ func (this *flowId) Request(fc skipper.FilterContext) {
 func (this *flowId) Response(skipper.FilterContext) {}
 
 func (this *flowId) MakeFilter(id string, fc skipper.FilterConfig) (skipper.Filter, error) {
-	reuseExisting, _ := fc[0].(bool)
-	return New(id, reuseExisting), nil
+	var reuseExisting bool
+	if len(fc) > 0 {
+		if r, ok := fc[0].(bool); ok {
+			reuseExisting = r
+		} else {
+			return nil, ErrInvalidFilterParameters
+		}
+	}
+	var flowIdLength uint8 = defaultLen
+	if len(fc) > 1 {
+		if l, ok := fc[1].(float64); ok && l >= minLength && l <= maxLength {
+			flowIdLength = uint8(l)
+		} else {
+			return nil, ErrInvalidFilterParameters
+		}
+	}
+	return New(id, reuseExisting, flowIdLength), nil
 }

filters/flowid/filter_test.go

@@ -2,16 +2,25 @@ package flowid
 
 import (
 	"github.com/zalando/skipper/mock"
+	"github.com/zalando/skipper/skipper"
 	"net/http"
 	"testing"
 )
 
-const testFlowId = "FLOW-ID-FOR-TESTING"
+const (
+	testFlowId    = "FLOW-ID-FOR-TESTING"
+	invalidFlowId = "[<>] (o) [<>]"
+)
+
+var (
+	testFlowIdSpec           = &flowId{}
+	filterConfigWithReuse    = skipper.FilterConfig{true}
+	filterConfigWithoutReuse = skipper.FilterConfig{false}
+)
 
 func TestNewFlowIdGeneration(t *testing.T) {
-	r, _ := http.NewRequest("GET", "http://example.org", nil)
-	f := New(filterName, true)
-	fc := &mock.FilterContext{FRequest: r}
+	f, _ := testFlowIdSpec.MakeFilter(filterName, filterConfigWithReuse)
+	fc := buildfilterContext()
 	f.Request(fc)
 
 	flowId := fc.Request().Header.Get(flowIdHeaderName)
@@ -21,10 +30,8 @@ func TestNewFlowIdGeneration(t *testing.T) {
 }
 
 func TestFlowIdReuseExisting(t *testing.T) {
-	r, _ := http.NewRequest("GET", "http://example.org", nil)
-	f := New(filterName, true)
-	r.Header.Set(flowIdHeaderName, testFlowId)
-	fc := &mock.FilterContext{FRequest: r}
+	f, _ := testFlowIdSpec.MakeFilter(filterName, filterConfigWithReuse)
+	fc := buildfilterContext(flowIdHeaderName, testFlowId)
 	f.Request(fc)
 
 	flowId := fc.Request().Header.Get(flowIdHeaderName)
@@ -34,10 +41,8 @@ func TestFlowIdReuseExisting(t *testing.T) {
 }
 
 func TestFlowIdIgnoreReuseExisting(t *testing.T) {
-	r, _ := http.NewRequest("GET", "http://example.org", nil)
-	f := New(filterName, false)
-	r.Header.Set(flowIdHeaderName, testFlowId)
-	fc := &mock.FilterContext{FRequest: r}
+	f, _ := testFlowIdSpec.MakeFilter(filterName, filterConfigWithoutReuse)
+	fc := buildfilterContext(flowIdHeaderName, testFlowId)
 	f.Request(fc)
 
 	flowId := fc.Request().Header.Get(flowIdHeaderName)
@@ -46,15 +51,49 @@ func TestFlowIdIgnoreReuseExisting(t *testing.T) {
 	}
 }
 
-func TestFlowIdRejectInvalidFlowId(t *testing.T) {
-	r, _ := http.NewRequest("GET", "http://example.org", nil)
-	f := New(filterName, true)
-	r.Header.Set(flowIdHeaderName, "[<>] (o) [<>]")
-	fc := &mock.FilterContext{FRequest: r}
+func TestFlowIdRejectInvalidReusedFlowId(t *testing.T) {
+	f, _ := testFlowIdSpec.MakeFilter(filterName, filterConfigWithReuse)
+	fc := buildfilterContext(flowIdHeaderName, invalidFlowId)
 	f.Request(fc)
 
 	flowId := fc.Request().Header.Get(flowIdHeaderName)
-	if flowId == "[<>] (o) [<>]" {
+	if flowId == invalidFlowId {
 		t.Errorf("Got wrong flow id. Expected a newly generated flowid but got the test flow id '%s'", flowId)
 	}
 }
+
+func TestFlowIdWithSpecificLen(t *testing.T) {
+	fc := skipper.FilterConfig{true, float64(42.0)}
+	f, _ := testFlowIdSpec.MakeFilter(filterName, fc)
+	fctx := buildfilterContext()
+	f.Request(fctx)
+
+	flowId := fctx.Request().Header.Get(flowIdHeaderName)
+
+	l := len(flowId)
+	if l != 42 {
+		t.Errorf("Wrong flowId len. Expected %d, got %d", 42, l)
+	}
+}
+
+func TestFlowIdWithInvalidParameters(t *testing.T) {
+	fc := skipper.FilterConfig{"wrong-parameter-type"}
+	_, err := testFlowIdSpec.MakeFilter(filterName, fc)
+	if err != ErrInvalidFilterParameters {
+		t.Errorf("Expected an invalid parameters error, got %s", err)
+	}
+
+	fc = skipper.FilterConfig{true, float64(minLength - 1)}
+	_, err = testFlowIdSpec.MakeFilter(filterName, fc)
+	if err != ErrInvalidFilterParameters {
+		t.Errorf("Expected an invalid parameters error, got %s", err)
+	}
+}
+
+func buildfilterContext(headers ...string) skipper.FilterContext {
+	r, _ := http.NewRequest("GET", "http://example.org", nil)
+	for i := 0; i < len(headers); i += 2 {
+		r.Header.Set(headers[i], headers[i+1])
+	}
+	return &mock.FilterContext{FRequest: r}
+}

filters/flowid/hashing.go

@@ -4,13 +4,13 @@ import (
 	"crypto/rand"
 	"encoding/hex"
 	"errors"
-	"regexp"
 	"fmt"
+	"regexp"
 )
 
 const (
 	defaultLen = 16
-	maxLength  = 255
+	maxLength  = 254
 	minLength  = 8
 )
 
@@ -19,9 +19,8 @@ var (
 	flowIdRegex, _ = regexp.Compile(`^[\w+/=\-]+$`)
 )
 
-
 func newFlowId(len uint8) (string, error) {
-	if len < minLength || len%2 != 0 {
+	if len < minLength || len > maxLength || len%2 != 0 {
 		return "", ErrInvalidLen
 	}
 

filters/flowid/hashing_test.go

@@ -9,7 +9,7 @@ func TestFlowIdInvalidLength(t *testing.T) {
 	}
 
 	_, err = newFlowId(15)
-	if err == nil {
+	if err != ErrInvalidLen {
 		t.Errorf("Request for an invalid flow id length (odd number) succeeded and it shouldn't")
 	}
 }

filters/flowid/readme.md

@@ -1,12 +1,43 @@
 # Flow ID Filter
 
-Flow IDs let you correlate router logs for a given request against the upstream application logs for that same request.
-If your upstream application makes other requests to other services it can provide the same Flow ID value so that all
+Flow Ids let you correlate router logs for a given request against the upstream application logs for that same request.
+If your upstream application makes other requests to other services it can provide the same Flow Id value so that all
 of those logs can be correlated.
 
 ## How it works
+
 Skipper generates a unique Flow ID for every HTTP request that it receives. The Flow ID is then passed to your 
-upstream application as an HTTP header called X-Flow-Id.
+upstream application as an HTTP header called `X-Flow-Id`.
+
+The filter takes 2 optional parameters:
+
+1. Accept existing `X-Flow-Id` header
+2. Flow Id length
+
+The first parameter is a boolean parameter that, when set to true, will make the filter skip the generation of
+a new flow id. If the existing header value is not a valid flow id it is ignored and a new flow id is also generated.
+
+The second parameter is a number that defines the length of the generated flow ids. Valid options are any even number
+between 8 and 254.
+
+## Usage
+
+The filter can be used with many different combinations of parameters. It can also be used without any parameter, since 
+both are options.
+
+### Default parameters
+	FlowId()
+Without any parameters, the filter doesn't reuse existing `X-Flow-Id` headers and generates new ones with 16 bytes.
+
+### Reuse existing flow id
+	FlowId(true)
+With only the first parameter with the boolean value `true` the filter will accept existing `X-Flow-Id` headers, if 
+they're present in the request.
+
+### Generate bigger flow ids
+	FlowId(false, 64)
+This example doesn't accept existing `X-Flow-Id` headers and will always generate new flow ids with 64 bytes.
+
 
 ## Some benchmarks