zalando / skipper

An HTTP router and reverse proxy for service composition, including use cases like Kubernetes Ingress
https://opensource.zalando.com/skipper/
Other
3.07k stars 345 forks source link

explicit types for different request/response objects during filtering and maybe predicates #110

Open aryszka opened 8 years ago

aryszka commented 8 years ago

Pasting on behalf of @mo-gr :

The incoming req/resp and the outgoing req/resp in some filters can be confusing. I believe, the likelyhood of such issues can be greatly reduced by some type-trickery. package main

import (
"fmt"
"net/http"
)

type IncomingRequest http.Request
type OutgoingRequest http.Request

func foo(r http.Request) int {
    return 1
}

func Filter(r IncomingRequest) int {
    return 2
}

func main() {
    fmt.Println(Filter(IncomingRequest{}))
    // fmt.Println(Filter(OutgoingRequest{})) // fails
    fmt.Println(foo(http.Request(IncomingRequest{})))
}

This would make it clear, which is which and you could write functions, that are only callable with inbound requests (or outbound responses or whatever) I am aware that this is a rather huge refactoring. And I'm not 100% sure if the benefits are worth it Most annoyingly, it would completely break the Filter/Predicate API Maybe something to consider for a version 2?

aryszka commented 8 years ago

Good point. The type names can be fine tuned yet. I would start looking at the filters.FilterContext around the OriginalRequest and OriginalResponse properties, in the first round. The actual outgoing request object is never exposed to the filters, so maybe we can call the original as IncomingRequest, but the other one as ForwardedRequest.

Issue open for discussions.

lmineiro commented 8 years ago

I was trying to figure out where a Filter could access the request created to access the backend but I don't think it is ever exposed.

I can understand that with the Response this is not that clear. We already tried to improve it a little bit getting rid of the ResponseWriter/Response ambiguity. Is there such a thing as an IncomingResponse?

aryszka commented 8 years ago

first part: true

second: it is a very similar case, almost symmetric. Actually, that's kind of cool, I think. My first approach would be in this scenario to call it ForwardedResponse, too, but that's not how most of us think about the http request flow. The downstream/upstream is also constantly confusing, because we always mix which is which.

What we have:

  1. OriginalRequest
  2. Request
  3. OriginalResponse
  4. Response
  5. Serve(Response)

I admit that the OriginalRequest/OriginalResponse properties on the filter context are a very weak kind of a naming.

So, I'm really not sure at the moment, but there is something to this. The question in my view is, if one writing a filter, needs to understand too much about the proxying mechanism to be sure about what they're doing, or just can go ahead confidently with some overall view and with the help of the filter context. Some neat type trickery can be useful if it makes it easier to use the API for newcomers, filter editors, or occasional contributors. Open for suggestions.

For the fun, some very descriptive, enterprise level names/types could be:

OriginalRequest: TheRequestAsWeGotItFromTheClientWhichIfYouChangeYouAreOnlyFoolingOtherFiltersAndBtwWatchOutWithTheBodyBecauseOfStreaming
Request: TheRequestObjectThatWeUseOnlyToCollectYourShitAtYourPreferenceAndItWillBeMappedButWithSlightChangesAndBtwWatchOutWithTheBodyBecauseOfStreaming
OriginalResponse: TheResponseObjectAsWeGotItFromTheBackendWhichIfYouChangeYouAreOnlyFoolingOtherFiltersAndBtwWatchOutWithTheBodyBecauseOfStreaming
Response: TheResponseObjectThatWeUseOnlyToCollectYourShitAtYourPreferenceAndItWillBeMappedAndBtwWatchOutWithTheBodyBecauseOfStreaming
Serve(Response): TheResponseObjectThatYouUseToDiscardWhateverHappenedOrDidNotOnTheBackendButTheOthersWillNotKnowItAnywayAndBtwHopeThatTheOthersWillWatchOutWithTheBodyBecauseOfStreaming
LappleApple commented 8 years ago

Can we label this one as enhancement, @aryszka?

lmineiro commented 8 years ago

Worth waiting for Go 1.7 and the new Context to consider anything that might break the existing API