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

Feature #18 add x flow #23

Merged
merged 7 commits into from
Oct 19, 2015
Merged
Show file tree
Hide file tree
Changes from 3 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
5 changes: 5 additions & 0 deletions filters/filters.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package filters
import (
// import filter packages here:

"errors"
"github.com/zalando/skipper/filters/healthcheck"
"github.com/zalando/skipper/filters/humanstxt"
"github.com/zalando/skipper/filters/pathrewrite"
Expand All @@ -14,6 +15,10 @@ import (
"github.com/zalando/skipper/skipper"
)

var (
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for the braces when there is only a single var, but of course fine like this, too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe we can add some more global errors soon :)

ErrInvalidFilterParameters = errors.New("Invalid filter parameters")
)

// takes a registry object and registers the filter spec in the package
func Register(registry skipper.FilterRegistry) {
registry.Add(
Expand Down
64 changes: 64 additions & 0 deletions filters/flowid/filter.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
package flowid

import (
"github.com/zalando/skipper/filters"
"github.com/zalando/skipper/skipper"
)

const (
filterName = "FlowId"
flowIdHeaderName = "X-Flow-Id"
)

type flowId struct {
id string
reuseExisting bool
flowIdLength uint8
}

func New(id string, allowOverride bool, len uint8) skipper.Filter {
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't see where the filter spec is exported

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I believe I got it now how this should work out...

return &flowId{id, allowOverride, len}
Copy link
Contributor

Choose a reason for hiding this comment

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

reuse existing or allow override? which one is it?

}

func (this *flowId) Id() string { return this.id }

func (this *flowId) Name() string { return filterName }

func (this *flowId) Request(fc skipper.FilterContext) {
r := fc.Request()
var flowId string

if this.reuseExisting {
flowId = r.Header.Get(flowIdHeaderName)
if isValid(flowId) {
return
}
}

flowId, err := newFlowId(this.flowIdLength)
if err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

swallowing an error here. If this goes wrong, it should be logged, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We haven't exactly established this logging strategy yet...

Copy link
Contributor

Choose a reason for hiding this comment

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

true, but all errors that couldn't be handled were printed to stderr as a basic approach. i think we should do it here, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay

fc.Request().Header.Set(flowIdHeaderName, flowId)
}
}

func (this *flowId) Response(skipper.FilterContext) {}

func (this *flowId) MakeFilter(id string, fc skipper.FilterConfig) (skipper.Filter, error) {
var reuseExisting bool
if len(fc) > 0 {
if r, ok := fc[0].(bool); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't have bool :(
the doc is not yet in the master, but you can have a look at here:
https://github.com/zalando/skipper/blob/release-cleanup-doc/eskip/doc.go

reuseExisting = r
} else {
return nil, filters.ErrInvalidFilterParameters
}
}
var flowIdLength uint8 = defaultLen
Copy link
Contributor

Choose a reason for hiding this comment

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

where does the defaultLen come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, it's used only here but it is in the hashing file. Will move the const here

if len(fc) > 1 {
if l, ok := fc[1].(float64); ok && l >= minLength && l <= maxLength {
flowIdLength = uint8(l)
} else {
return nil, filters.ErrInvalidFilterParameters
}
}
return New(id, reuseExisting, flowIdLength), nil
}
99 changes: 99 additions & 0 deletions filters/flowid/filter_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
package flowid

import (
"github.com/zalando/skipper/mock"
"github.com/zalando/skipper/skipper"
"net/http"
"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) {
f, _ := testFlowIdSpec.MakeFilter(filterName, filterConfigWithReuse)
fc := buildfilterContext()
f.Request(fc)

flowId := fc.Request().Header.Get(flowIdHeaderName)
if !isValid(flowId) {
t.Errorf("'%s' is not a valid flow id", flowId)
}
}

func TestFlowIdReuseExisting(t *testing.T) {
f, _ := testFlowIdSpec.MakeFilter(filterName, filterConfigWithReuse)
fc := buildfilterContext(flowIdHeaderName, testFlowId)
f.Request(fc)

flowId := fc.Request().Header.Get(flowIdHeaderName)
if flowId != testFlowId {
t.Errorf("Got wrong flow id. Expected '%s' got '%s'", testFlowId, flowId)
}
}

func TestFlowIdIgnoreReuseExisting(t *testing.T) {
f, _ := testFlowIdSpec.MakeFilter(filterName, filterConfigWithoutReuse)
fc := buildfilterContext(flowIdHeaderName, testFlowId)
f.Request(fc)

flowId := fc.Request().Header.Get(flowIdHeaderName)
if flowId == testFlowId {
t.Errorf("Got wrong flow id. Expected a newly generated flowid but got the test flow id '%s'", flowId)
}
}

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 == 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}
}
37 changes: 37 additions & 0 deletions filters/flowid/hashing.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package flowid

import (
"crypto/rand"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually need cryptographic randomness here? It seems like regular, pseudo randomness should be enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't

"encoding/hex"
"errors"
"fmt"
"regexp"
)

const (
defaultLen = 16
maxLength = 254
minLength = 8
)

var (
ErrInvalidLen = errors.New(fmt.Sprintf("Invalid length. len must be >= %d and < %d", minLength, maxLength))
flowIdRegex, _ = regexp.Compile(`^[\w+/=\-]+$`)
Copy link
Contributor

Choose a reason for hiding this comment

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

in these cases, you can use MustCompile. it doesn't return an error, but panics immediately when running the tests. The idea is the same, the expression is not coming from outside, but you're one step safer by a panic during the tests.

)

func newFlowId(len uint8) (string, error) {
if len < minLength || len > maxLength || len%2 != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

if it works, it works. just a warning that i managed once to trigger very strange behavior from the compiler by overdefining 'make' and it resulted in a relatively hard-to-figure compilation error. (talking about overdefining the 'len' builtin here)

Copy link
Contributor

Choose a reason for hiding this comment

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

if len%2 is a requirement, the ErrInvalidLen should point that out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good hint

return "", ErrInvalidLen
}

u := make([]byte, hex.DecodedLen(int(len)))
buf := make([]byte, len)

rand.Read(u)
hex.Encode(buf, u)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hex is cool and all... I get that, but couldn't you just draw len times a random char and concatenate them?

return string(buf), nil
}

func isValid(flowId string) bool {
return len(flowId) >= minLength && len(flowId) <= maxLength && flowIdRegex.MatchString(flowId)
Copy link
Contributor

Choose a reason for hiding this comment

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

Reggie could also check for the length. not sure if this makes sense though...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would be more expensive to delegate that to the regex. I expect the short-circuit of the boolean evaluation to beat that

}
64 changes: 64 additions & 0 deletions filters/flowid/hashing_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
package flowid

import "testing"

func TestFlowIdInvalidLength(t *testing.T) {
_, err := newFlowId(0)
if err == nil {
t.Errorf("Request for an invalid flow id length (0) succeeded and it shouldn't")
}

_, err = newFlowId(15)
if err != ErrInvalidLen {
t.Errorf("Request for an invalid flow id length (odd number) succeeded and it shouldn't")
}
}

func TestFlowIdLength(t *testing.T) {
for expected := minLength; expected <= maxLength; expected += 2 {
flowId, err := newFlowId(uint8(expected))
if err != nil {
t.Errorf("Failed to generate flowId with len %d", expected)
}

l := len(flowId)
if l != expected {
t.Errorf("Got wrong flowId len. Requested %d, got %d (%s)", expected, l, flowId)
}
}
}

func BenchmarkFlowIdLen8(b *testing.B) {
testFlowIdWithLen(b.N, 8)
}

func BenchmarkFlowIdLen10(b *testing.B) {
testFlowIdWithLen(b.N, 10)
}

func BenchmarkFlowIdLen12(b *testing.B) {
testFlowIdWithLen(b.N, 12)
}

func BenchmarkFlowIdLen14(b *testing.B) {
testFlowIdWithLen(b.N, 14)
}

func BenchmarkFlowIdLen16(b *testing.B) {
testFlowIdWithLen(b.N, 16)
}

func BenchmarkFlowIdLen32(b *testing.B) {
testFlowIdWithLen(b.N, 32)
}

func BenchmarkFlowIdLen64(b *testing.B) {
testFlowIdWithLen(b.N, 64)
}

func testFlowIdWithLen(times int, len uint8) {
for i := 0; i < times; i++ {
newFlowId(len)
}

}
67 changes: 67 additions & 0 deletions filters/flowid/readme.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
# Flow ID Filter
Copy link
Contributor

Choose a reason for hiding this comment

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

Flow Id or Flow ID - please be consistent


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`.

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Forcing an even number seems strangely arbitrary without knowing the implementation


## Usage

The filter can be used with many different combinations of parameters. It can also be used without any parameter, since
both are options.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/options./optional./


### Default parameters
FlowId()
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use camel casing for the filters, and Pascal casing for the matcher conditions? it would be nice to have a convention here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your call. I haven't noticed, until now, which is out standard

Copy link
Contributor

Choose a reason for hiding this comment

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

there wasn't :) you are the first person with whom i can agree on it

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

in these types of filter arguments, we could use enum strings. FlowId(true) would result in a parsing error.
I recommend: FlowId("reuse")

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

To decide upon which hashing mechanism to use we tested some versions of UUID v1 - v4 and some other implementations.
The results are as follow:

Benchmark_uuidv1-4 5000000 281 ns/op
Benchmark_uuidv2-4 5000000 284 ns/op
Benchmark_uuidv3-4 2000000 605 ns/op
Benchmark_uuidv4-4 1000000 1903 ns/op
BenchmarkRndAndSprintf-4 500000 3312 ns/op
BenchmarkSha1-4 1000000 2188 ns/op
BenchmarkMd5-4 1000000 2076 ns/op
BenchmarkFnv-4 500000 2223 ns/op

The current implementation just gets len / 2 (hex.DecodedLen) bytes from the PRNG and hex encodes them.
Its performance is only dependent on the length of the generated FlowId, according to the following benchmarks:

BenchmarkFlowIdLen8-4 1000000 1157 ns/op
BenchmarkFlowIdLen10-4 1000000 1162 ns/op
BenchmarkFlowIdLen12-4 1000000 1163 ns/op
BenchmarkFlowIdLen14-4 1000000 1171 ns/op
BenchmarkFlowIdLen16-4 1000000 1180 ns/op
BenchmarkFlowIdLen32-4 1000000 1957 ns/op
BenchmarkFlowIdLen64-4 300000 3520 ns/op

As you can see, starting at len = 32 (16 random bytes) the performance starts dropping dramatically.
Copy link
Contributor

Choose a reason for hiding this comment

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

did you figure the reason? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went all the way to the getrandom(2) syscall. Without wasting too much time there, I'll blame this on the drainage of entropy pool

2 changes: 1 addition & 1 deletion proxy/proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -272,4 +272,4 @@ func (p *proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) {
func addBranding(rs *http.Response) {
rs.Header.Set("X-Powered-By", "Skipper")
rs.Header.Set("Server", "Skipper")
}
}