Refactor v1 HTTP to use middlewares and http.HandlerFuncs #325

metalmatze · 2020-04-03T14:25:15Z

Now that Telemeter doesn't have the hashring anymore, we can go ahead and get rid of the store.Store interface, allowing us to move to simpler architecture.
With this PR we're moving to a pkg/server that contains multiple HTTP middlewares and http.HandlerFuncs.

/cc @squat @kakkoyun @krasi-georgiev @bwplotka @brancz

…middleware

It already passes e2e with forward.Handler. I guess we can get rid of the store.Store interface really soon

squat · 2020-04-03T15:29:59Z

cmd/telemeter-server/main.go

@@ -325,7 +322,12 @@ func (o *Options) Run() error {
 		})
 	}
 	{
-		external := http.NewServeMux()
+		external := chi.NewRouter()


nit: above we call the router r and the mux internal and here we are following a different pattern. let's keep it consistent.

The problem is that the above still needs the internal to begin with. We need to refactor telemeter_http.DebugRoutes etc in another PR and then can have consistency.

no I simply mean we should call the router r in both cases

squat · 2020-04-03T15:30:43Z

cmd/telemeter-server/main.go

@@ -403,17 +405,12 @@ func (o *Options) Run() error {
 			}

 			auth := jwt.NewAuthorizeClusterHandler(o.Logger, o.PartitionKey, o.TokenExpireSeconds, signer, o.RequiredLabels, clusterAuth)
-			validator := validate.New(o.PartitionKey, o.LimitBytes, 24*time.Hour, time.Now)
+			//validator := validate.New(o.PartitionKey, o.LimitBytes, 24*time.Hour, time.Now)


are we intentionally ignoring this? do we just want to delete the line?

commented out during refactoring for reference. The validate.New func doesn't even exist anymore. Will remove.

squat · 2020-04-03T15:34:10Z

pkg/server/forward.go

-	}
-}
+	return func(w http.ResponseWriter, r *http.Request) {
+		partitionKey, ok := PartitionFromContext(r.Context())


let's rename this partitionKey thing. It's a relic from the partitioned metrics type but it's a weird name IMO. I think something like ID, key, tenant would be much more clear

Yeah, I had the same thought during refactoring. I would have gone for clusterID. That's what it's ultimately is, right?

yes exactly

squat · 2020-04-03T15:34:55Z

pkg/server/forward.go

 		}

 		if len(timeseries) == 0 {
-			level.Info(s.logger).Log("msg", "no time series to forward to receive endpoint")
-			return nil
+			level.Info(logger).Log("msg", "no time series to forward to receive endpoint")


Should we also respond with an HTTP error here?

It was a transient error before. We simply ignored those requests and returned 200. I don't really care, honestly.

squat · 2020-04-03T15:35:59Z

pkg/server/ratelimited.go

+	"golang.org/x/time/rate"
+)
+
+// ErrWriteLimitReached is an error that is returned when a cluster has send too many requests.


nit: s/send/sent/

squat · 2020-04-03T16:07:59Z

pkg/server/snappy.go

+			}
+			defer r.Body.Close()
+
+			payload, _ := snappy.Decode(nil, body)


I suggest we handle this error just in case

Seems like I've forgotten 🙃

squat · 2020-04-03T16:09:15Z

pkg/server/snappy.go

+			reader = ioutil.NopCloser(bytes.NewBuffer(payload))
+		}
+
+		r.Body = reader


I think we can put this line and line 14 inside the if block, no? otherwise it's funny following why we are changing the body when the encoding is not snappy

squat · 2020-04-03T16:11:21Z

test/e2e/forward_test.go

 		labels := map[string]string{"cluster": "test"}
-		validator := validate.New("cluster", 0, 0, time.Now)
+		//validator := validate.New("cluster", 0, 0, time.Now)


can we simply delete this line?

squat

Looks great overall @metalmatze good work so far!

Not sure it ever got in there, as it's wrong.

squat · 2020-04-17T10:06:35Z

/retest

squat · 2020-04-17T10:17:21Z

pkg/server/forward.go

 		}

 		if resp.StatusCode/100 != 2 {
-			return fmt.Errorf("response status code is %s", resp.Status)
+			// surfacing upstreams error to our users too
+			http.Error(w, fmt.Errorf("response status code is %s", resp.Status).Error(), resp.StatusCode)


This isn't correct IMO. If telemeter-server gets a 400 from the upstream then it is not the user's fault that there is an auth error, but rather an internal server error. The user should not receive a 400 for an improperly configured server. Instead the user should get a 500.

Right. That makes sense. Keep in mind that this is the request send from Telemeter to Thanos Receivers. I guess it still holds true then too? (There's not auth in this step)

pkg/server/forward.go

squat · 2020-04-17T10:19:42Z

pkg/server/instrument.go

@@ -36,7 +36,8 @@ func init() {
 	prometheus.MustRegister(requestDuration, requestSize, requestsTotal)
 }

-func NewInstrumentedHandler(handlerName string, next http.Handler) http.Handler {
+// InstrumentedHandler is a HTTP middleware that monitors HTTP requests and responses.


nit: ... is an HTTP ...

squat · 2020-04-17T10:20:39Z

pkg/server/ratelimited.go

+type ErrWriteLimitReached string
+
+func (e ErrWriteLimitReached) Error() string {
+	return fmt.Sprintf("write limit reached for key %q", string(e))


maybe we should rename key to id or cluster now

write limit reached for cluster with id? 😊

squat · 2020-04-17T10:22:38Z

pkg/server/snappy_test.go

+			t.Errorf("expected %d and got %d", http.StatusOK, resp.StatusCode)
+		}
+	}
+	//{


do we want to re-enable this test or should we remove it simply?

squat · 2020-04-17T10:23:47Z

pkg/server/validator.go

+	return p, ok
+}
+
+type clusterIDCtxType int


let's put these definitions at the top of the file to follow convention

squat · 2020-04-17T10:26:00Z

pkg/server/validator.go

+		transforms.With(metricfamily.NewRequiredLabels(client.Labels))
+		transforms.With(metricfamily.TransformerFunc(metricfamily.DropEmptyFamilies))
+
+		if limitBytes > 0 {


hmm this is funny, shouldn't this come before the ioutil.ReadAll on line 66?

squat · 2020-04-17T10:26:36Z

pkg/server/validator.go

+		transforms.With(metricfamily.TransformerFunc(metricfamily.DropEmptyFamilies))
+
+		if limitBytes > 0 {
+			r.Body = reader.NewLimitReadCloser(r.Body, limitBytes)


what effect does this have? it seems like the entire body was already read into a new buffer called body on line 66

squat · 2020-04-17T10:27:26Z

pkg/server/validator_test.go

+		fakeAuthorizeHandler(
+			Validate(metricfamily.MultiTransformer{}, time.Hour, 512*1024, time.Now,
+				func(w http.ResponseWriter, r *http.Request) {
+					// TODO: Make the check proper to changing timestamps?


Should we delete or re-enable this?

metalmatze · 2020-04-20T08:35:40Z

/retest

squat · 2020-04-21T08:01:14Z

/retest

squat

/lgtm

openshift-ci-robot · 2020-04-21T08:06:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: metalmatze, squat

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [metalmatze,squat]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

brancz · 2020-04-21T10:03:44Z

This is entirely code that is not shipped in OpenShift, but needed for the telemetry server infrastructure. All CI green. Merging.

metalmatze added 14 commits March 30, 2020 17:49

pkg/store/ratelimited: Create HTTP middleware

cdcc68d

pkg/http/server: Refactor to be Post to be HandleFunc, create Snappy …

b237e41

…middleware

pkg/validate: Create PartitionKey middleware

75c27ea

Use new middlewares and use chi Router as HTTP mux

30f6c1a

Replace server.Post handler with foward.Handler entirely

ad1a1ee

It already passes e2e with forward.Handler. I guess we can get rid of the store.Store interface really soon

Fix some staticcheck issues

1924379

pkg/http/server: Delete old and now unused Post handler

5bf077f

Delete unused store.Store interface

1d08da2

pkg/store/memstore: Delete unused memstore

b42fb6a

Start moving HTTP API v1 files into pkg/server

7885e6d

Consolidate Telemeter server middlewares and handlers into pkg/server

2e1f41a

Refactor Validator to HTTP middleware

0f78d48

pkg/server: Add GoDoc comments to public functions

fa52d2a

Use server.Validate middleware in Telemeter server

a53a7ad

openshift-ci-robot requested review from brancz, bwplotka, kakkoyun, krasi-georgiev and squat April 3, 2020 14:25

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 3, 2020

squat reviewed Apr 3, 2020

View reviewed changes

Address comments about refactoring clean ups

4db0156

metalmatze added 4 commits April 15, 2020 19:20

Improve validator middleware to correctly forward http body

a9d3ab5

Do not use server.ClusterID middleware for v2 receive endpoint

32e6e15

Not sure it ever got in there, as it's wrong.

Pass transforms into validate middleware from main.go

220a33c

Fix unit tests for http middlewares

2154269

squat reviewed Apr 17, 2020

View reviewed changes

pkg/server/forward.go Outdated Show resolved Hide resolved

squat reviewed Apr 17, 2020

View reviewed changes

metalmatze added 7 commits April 20, 2020 13:05

Improve GoDoc comment

69ea0e0

Don't set a timeout on the HTTP client

eba3200

Improve error for rate limited cluster

a072437

Improve and add back all test cases for snappy_test.go

28b3273

Move clusterIDcontext to the top of the validator file

3f24e56

Limit the body size before reading it in the validator

b6fd58d

Fix vlidator_test by passing known now func

b516d66

squat approved these changes Apr 21, 2020

View reviewed changes

openshift-ci-robot assigned squat Apr 21, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 21, 2020

brancz merged commit 958dc61 into openshift:master Apr 21, 2020

metalmatze deleted the http-refactorings branch April 21, 2020 10:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor v1 HTTP to use middlewares and http.HandlerFuncs #325

Refactor v1 HTTP to use middlewares and http.HandlerFuncs #325

metalmatze commented Apr 3, 2020

squat Apr 3, 2020

metalmatze Apr 6, 2020

squat Apr 17, 2020

squat Apr 3, 2020

metalmatze Apr 3, 2020

squat Apr 3, 2020 •

edited

metalmatze Apr 3, 2020

squat Apr 3, 2020

squat Apr 3, 2020

metalmatze Apr 6, 2020

squat Apr 3, 2020

squat Apr 3, 2020

metalmatze Apr 3, 2020

squat Apr 3, 2020

squat Apr 3, 2020

squat left a comment

squat commented Apr 17, 2020

squat Apr 17, 2020

metalmatze Apr 20, 2020 •

edited

squat Apr 17, 2020

squat Apr 17, 2020

metalmatze Apr 20, 2020

squat Apr 17, 2020

squat Apr 17, 2020

squat Apr 17, 2020

squat Apr 17, 2020

squat Apr 17, 2020

metalmatze commented Apr 20, 2020

squat commented Apr 21, 2020

squat left a comment

openshift-ci-robot commented Apr 21, 2020

brancz commented Apr 21, 2020

Refactor v1 HTTP to use middlewares and http.HandlerFuncs #325

Refactor v1 HTTP to use middlewares and http.HandlerFuncs #325

Conversation

metalmatze commented Apr 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squat Apr 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squat left a comment

Choose a reason for hiding this comment

squat commented Apr 17, 2020

Choose a reason for hiding this comment

metalmatze Apr 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metalmatze commented Apr 20, 2020

squat commented Apr 21, 2020

squat left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Apr 21, 2020

brancz commented Apr 21, 2020

squat Apr 3, 2020 •

edited

metalmatze Apr 20, 2020 •

edited