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

SB-10000: Adding app- and pipeline-ids to server #290

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 9 additions & 9 deletions examples/go.mod
Expand Up @@ -3,22 +3,22 @@ module github.com/NYTimes/gizmo/examples
replace github.com/NYTimes/gizmo => ../

require (
cloud.google.com/go v0.38.0
cloud.google.com/go v0.56.0
github.com/NYTimes/gizmo v1.2.1
github.com/NYTimes/gziphandler v1.1.0
github.com/NYTimes/logrotate v1.0.0
github.com/NYTimes/sqliface v0.0.0-20180310195202-f8e6c8b78d37
github.com/go-kit/kit v0.9.0
github.com/go-sql-driver/mysql v1.4.1
github.com/golang/protobuf v1.3.2
github.com/go-sql-driver/mysql v1.5.0
github.com/golang/protobuf v1.3.5
github.com/gorilla/context v1.1.1
github.com/gorilla/websocket v1.4.0
github.com/kelseyhightower/envconfig v1.3.0
github.com/pkg/errors v0.8.1
github.com/sirupsen/logrus v1.3.0
golang.org/x/net v0.0.0-20190628185345-da137c7871d7
google.golang.org/genproto v0.0.0-20190716160619-c506a9f90610
google.golang.org/grpc v1.22.0
github.com/kelseyhightower/envconfig v1.4.0
github.com/pkg/errors v0.9.1
github.com/sirupsen/logrus v1.5.0
golang.org/x/net v0.0.0-20200324143707-d3edc9973b7e
google.golang.org/genproto v0.0.0-20200331122359-1ee6d9798940
google.golang.org/grpc v1.28.0
)

go 1.13
264 changes: 264 additions & 0 deletions examples/go.sum

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions go.mod
Expand Up @@ -24,9 +24,9 @@ require (
github.com/prometheus/client_golang v0.9.4
github.com/sirupsen/logrus v1.5.0
go.opencensus.io v0.22.3
golang.org/x/net v0.0.0-20200301022130-244492dfa37a
golang.org/x/net v0.0.0-20200324143707-d3edc9973b7e
golang.org/x/oauth2 v0.0.0-20200107190931-bf48bf16ab8d
google.golang.org/api v0.20.0
google.golang.org/genproto v0.0.0-20200305110556-506484158171
google.golang.org/genproto v0.0.0-20200331122359-1ee6d9798940
google.golang.org/grpc v1.28.0
)
1 change: 1 addition & 0 deletions go.sum
Expand Up @@ -263,6 +263,7 @@ github.com/stretchr/testify v1.3.0 h1:TivCn/peBQ7UY8ooIcPgZFpTNSz0Q2U6UrFlUfqbe0
github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
github.com/stretchr/testify v1.4.0 h1:2E4SXV/wtOkTonXsotYi4li6zVWxYlZuYNCXe9XRJyk=
github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
github.com/stretchr/testify v1.5.1 h1:nOGnQDM7FYENwehXlg/kFVnos3rEvtKTjRvOWSzb6H4=
github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA=
github.com/xdg/scram v0.0.0-20180814205039-7eeb5667e42c/go.mod h1:lB8K/P019DLNhemzwFU4jHLhdvlE6uDZjXFejJXr49I=
github.com/xdg/stringprep v1.0.0/go.mod h1:Jhud4/sHMO4oL310DaZAKk9ZaJ08SJfe+sJh0HrGL1Y=
Expand Down
71 changes: 71 additions & 0 deletions server/ids.go
@@ -0,0 +1,71 @@
package server

import (
"fmt"
"strings"

uuid "github.com/nu7hatch/gouuid"
)

// IDer generates a new ID
type IDer interface {
ID() (string, error)
}

// AppUUID generates IDs based on v4 UUIDs
type AppUUID struct {
formatter func(string) string
}

// NewAppUUID creates a new AppUUID generator
func NewAppUUID(appname string) *AppUUID {
uuider := &AppUUID{
formatter: func(u string) string {
// default formatter does no formatting at all, simply
// returning the generated UUID as a string
return u
},
}

if len(appname) > 0 {
// ...if an appname is supplied, use that instead of the default
format := appname + "-%s"
uuider.formatter = func(u string) string {
return fmt.Sprintf(format, u)
}
}

return uuider
}

// ID returns a random (v4) UUID using the prefix supplied to NewAppUUID()
func (i *AppUUID) ID() (string, error) {
u, err := uuid.NewV4()
if err != nil {
return "", err
}

return i.formatter(u.String()), nil
}

// A PipelineID identifies requests all the way through some system by concatenating the
// IDs generated by each individual app, each separated by a separator string, "|"
type PipelineID struct {
AppIDer IDer
}

const fullIDerSep = "|"

// ID generates a new pipeline ID based on the current ID and its app IDer
func (f *PipelineID) ID(current string) (string, error) {
next, err := f.AppIDer.ID()
if err != nil {
return "", err
}

if len(current) == 0 {
return next, nil
}

return strings.Join([]string{current, next}, fullIDerSep), nil
}
88 changes: 88 additions & 0 deletions server/ids_test.go
@@ -0,0 +1,88 @@
package server

import (
"regexp"
"testing"
)

func TestAppUUID_ID(t *testing.T) {
// thanks to https://adamscheller.com/regular-expressions/uuid-regex/ for saving me from writing this
uuidRegex := "([a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}){1}"
t.Run("NoAppName", func(t *testing.T) {
ider := NewAppUUID("")
id, err := ider.ID()
if err != nil {
t.Error("failed to get mock app ID", "err", err)
}

match, err := regexp.MatchString(uuidRegex, id)
if err != nil {
t.Error("err matching generated ID", "err", err)
}
if !match {
t.Error("ID did not match", "got", id)
}
})

t.Run("WithAppName", func(t *testing.T) {
ider := NewAppUUID("blapp")
id, err := ider.ID()
if err != nil {
t.Error("failed to get mock app ID", "err", err)
}

match, err := regexp.MatchString("blapp-"+uuidRegex, id)
if err != nil {
t.Error("err matching generated ID", "err", err)
}
if !match {
t.Error("ID did not match", "got", id)
}
})
}

type MockIDer struct {
sendThis string
}

func (m *MockIDer) ID() (string, error) {
return m.sendThis, nil
}

func TestPipelineID_ID(t *testing.T) {
first := "antleeb"
second := "babananab"
third := "canola"

mockIDer := &MockIDer{}
pipeIDer := &PipelineID{AppIDer: mockIDer}

mockIDer.sendThis = first
id, err := pipeIDer.ID("")
if err != nil {
t.Error("failed to get pipeline ID", "err", err)
}
if first != id {
t.Error("frist ID call did not match", "got", id, "expected", first)
}

mockIDer.sendThis = second
id, err = pipeIDer.ID(id)
exp := first + fullIDerSep + second
if err != nil {
t.Error("failed to get pipeline ID", "err", err)
}
if exp != id {
t.Error("second ID call did not match", "got", id, "expected", exp)
}

mockIDer.sendThis = third
id, err = pipeIDer.ID(id)
exp = first + fullIDerSep + second + fullIDerSep + third
if err != nil {
t.Error("failed to get pipeline ID", "err", err)
}
if exp != id {
t.Error("third ID call did not match", "got", id, "expected", exp)
}
}
2 changes: 1 addition & 1 deletion server/kit/kitserver_test.go
Expand Up @@ -234,5 +234,5 @@ func (s *server) getCatByName(ctx context.Context, _ interface{}) (interface{},
}

func (s *server) error(ctx context.Context, _ interface{}) (interface{}, error) {
return nil, errors.New("doh!")
return nil, errors.New("doh")
}
69 changes: 69 additions & 0 deletions server/middleware.go
Expand Up @@ -193,3 +193,72 @@ func WithCloseHandler(h ContextHandler) ContextHandler {
h.ServeHTTPContext(ctx, w, r)
})
}

// Key to use when setting the request ID to a context
type ctxRequestIDKey int

const (
// RequestIDHeader is the request ID key set to http headers
RequestIDHeader = "X-Request-Id"
// RequestIDKey is the key used to stash the request ID into the context
RequestIDKey ctxRequestIDKey = 0
)

// AppIDHandler is a middleware that, if the $X-Request-ID$ header is not already
// set on the request, generates an ID using the provided IDer then sets it on the
// request and response. In both cases the value of the $X-Request-ID$ header is
// set to the context
func AppIDHandler(next http.Handler, idGen IDer) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
var err error
var requestID = r.Header.Get(RequestIDHeader)
if requestID == "" {
requestID, err = idGen.ID()
if err != nil {
// TODO gah, what then? I have good mind to eliminate this err altogether
Copy link

Choose a reason for hiding this comment

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

I think this is fine, we don't want to set the request ID to empty string, better to not set it at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, but a) I shouldn't have let this TODO slip into the commit, and b) I think of this code as being important but not critical, and the idea that something that isn't critical blitzing stuff that is annoys me. In this case, this err is here solely because the UUID generator.

If you think this is fine I'll remove the TODO and leave the return.

return
}
r.Header.Set(RequestIDHeader, requestID)
w.Header().Set(RequestIDHeader, requestID)
}

ctx := r.Context()
ctx = context.WithValue(ctx, RequestIDKey, requestID)
next.ServeHTTP(w, r.WithContext(ctx))
})
}

// PipelineIDHandler is a middleware that tracks all the IDs generated by the many apps
// in a system. For each app, it concatenates an app-specific ID to the "pipeline ID",
// which is then set to the context and the request & response $X-Request-ID$
func PipelineIDHandler(next http.Handler, pipeGen *PipelineID) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
var err error
var requestID = r.Header.Get(RequestIDHeader)
requestID, err = pipeGen.ID(requestID)
if err != nil {
// TODO gah, what then? I have good mind to eliminate this err altogether
return
}
r.Header.Set(RequestIDHeader, requestID)
w.Header().Set(RequestIDHeader, requestID)

ctx := r.Context()
ctx = context.WithValue(ctx, RequestIDKey, requestID)
next.ServeHTTP(w, r.WithContext(ctx))
})
}

// GetRequestID returns the value of the $X-Request-ID$ header as it was written
// to the context
func GetRequestID(ctx context.Context) string {
if ctx == nil {
return ""
}

if reqID, ok := ctx.Value(RequestIDKey).(string); ok {
return reqID
}

return ""
}
74 changes: 74 additions & 0 deletions server/middleware_test.go
Expand Up @@ -177,3 +177,77 @@ func TestNoCacheHandler(t *testing.T) {
t.Errorf("expected no-cache Expires header to be '%#v', got '%#v'", want, got)
}
}

func TestAppIDHandler(t *testing.T) {
r, err := http.NewRequest("GET", "", nil)
if err != nil {
t.Error("failed to create mock request", "err", err)
}
w := httptest.NewRecorder()

id := "flambe"
AppIDHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
requestID := GetRequestID(r.Context())
// write the ID to the response body so we can test it on the recorder
_, _ = w.Write([]byte(requestID))
w.WriteHeader(http.StatusOK)
}), &MockIDer{sendThis: id}).ServeHTTP(w, r)

headVal, ok := w.Result().Header[RequestIDHeader]
if !ok {
t.Error("header value was not found")
}
if len(headVal) != 1 {
t.Error("expected one value in request ID header", "got", len(headVal))
}
if headVal[0] != id {
t.Error("unexpected value in request ID header", "got", headVal[0], "expected", id)
}
if w.Body.String() != id {
t.Error("unexpected value in body", "got", w.Body.String(), "expected", id)
}
}

func TestPipelineIDHandler(t *testing.T) {
tests := []struct {
prev, next, expected string
}{
{"", "roger", "roger"},
{"roger", "roderick", "roger|roderick"},
{"roger|roderick", "brian", "roger|roderick|brian"},
}

for _, test := range tests {
r, err := http.NewRequest("GET", "", nil)
Copy link

@jonsabados jonsabados Apr 9, 2020

Choose a reason for hiding this comment

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

for table driven test stuff each test case should be in a t.run, ie:

func TestPipelineIDHandler(t *testing.T) {
   tests := []struct {
   	desc, prev, next, expected string
   }{
   	{"new", "", "roger", "roger"},
   	{"existing", "roger", "roderick", "roger|roderick"},
   	{"chained", "roger|roderick", "brian", "roger|roderick|brian"},
   }

   for _, test := range tests {
   	t.Run(test.desc, func(t *testing.T) {
   		r, err := http.NewRequest("GET", "", nil)
   		...
   	})
   }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks! fixed.

if err != nil {
t.Error("failed to create mock request", "err", err)
}
r.Header.Set(RequestIDHeader, test.prev)
w := httptest.NewRecorder()

pipeIDer := &PipelineID{
AppIDer: &MockIDer{sendThis: test.next},
}

PipelineIDHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
requestID := GetRequestID(r.Context())
// write the ID to the response body so we can test it on the recorder
_, _ = w.Write([]byte(requestID))
w.WriteHeader(http.StatusOK)
}), pipeIDer).ServeHTTP(w, r)

headVal, ok := w.Result().Header[RequestIDHeader]
if !ok {
t.Error("header value was not found")
}
if len(headVal) != 1 {
t.Error("expected one value in request ID header", "got", len(headVal))
}
if headVal[0] != test.expected {
t.Error("unexpected value in request ID header", "got", headVal[0], "expected", test.expected)
}
if w.Body.String() != test.expected {
t.Error("unexpected value in body", "got", w.Body.String(), "expected", test.expected)
}
}
}