mosn / pkg

Common packages used by other repos.
Apache License 2.0
26 stars 28 forks source link

Add support for http trailers and full range keys #66

Open codefromthecrypt opened 2 years ago

codefromthecrypt commented 2 years ago

fasthttp implements trailing headers as a part of the http request and response headers objects. To ensure we can use these without relying on fasthttp API, we need a few more functions, which I'm happy to implement after #65 is in. I need this to help support the new http-wasm host, which has glitches both in trailers and also multiple header values.

cc @taoyuanyuan I'll backfill unit tests and polish up stuff, but probably this should be done separate from the version bump

codefromthecrypt commented 2 years ago

I referenced some upstream issues on fasthttp as at some point it would be nice for these to be in the standard library underneath as well. Regardless, we'll need apis here as otherwise when people access code, they are reliant on the implementation of fasthttp, not the types and functions defined explicitly here:

https://github.com/valyala/fasthttp/issues/1400 https://github.com/valyala/fasthttp/issues/1401

cc @li-jin-gou

antJack commented 2 years ago

mosn filter's API already provide accessibility to http header/body/trailer

func (f *filter) OnReceive(ctx context.Context, headers HeaderMap, body IoBuffer, trailers HeaderMap) StreamFilterStatus
                                                                                    ^^^
func (f *Filter) Append(ctx context.Context, headers api.HeaderMap, body buffer.IoBuffer, trailers api.HeaderMap) api.StreamFilterStatus
                                                                                             ^^^

you can simple put the header/body/trailer in the filter(ctx) and get them whenever you need

codefromthecrypt commented 2 years ago

@antJack ok I think there are a couple things then. one is that fasthttp doesn't work with trailers, yet. https://github.com/mosn/mosn/issues/2145#issuecomment-1282305959

When we update here, we need a different type for trailers because in fasthttp trailers are in the same requestheaders type as headers. If we don't handle that here, then mosn will need similar logic to do it. https://github.com/valyala/fasthttp/pull/1165

There's a separate topic about access for GetAllKeys and GetAll, but let's talk about ^^ first.

antJack commented 2 years ago

ok, after reading those materials, I probably understand what you want to do.

If there is any idea about where to add those trailers-related api? Another problem is that whether mosn should also "implements trailing headers as a part of the http request and response headers", since mosn's api is not just for http.

codefromthecrypt commented 2 years ago

My first thought was to add the extra apis to the existing http request/response types here https://github.com/mosn/pkg/blob/master/protocol/http/types.go

another way could be to define new types RequestTrailers and ResponseTrailers, if that's easier to integrate with the existing mosn callbacks. either way, it would be http only. wdyt?

codefromthecrypt commented 2 years ago

I will work on an implementation, as after we should cut a version of api, pkg so that https://github.com/mosn/holmes/pull/129 and then eventually dapr can have api/pkg version alignment

antJack commented 2 years ago

We may prefer not changing the top-level stream filter api, such as:

func (f *filter) OnReceive(ctx context.Context, headers HeaderMap, body IoBuffer, trailers HeaderMap) StreamFilterStatus
func (f *Filter) Append(ctx context.Context, headers api.HeaderMap, body buffer.IoBuffer, trailers api.HeaderMap) api.StreamFilterStatus

just keeps the separation of headers/trailers, they were designed not only for http, and not even for fasthttp.

and it's ok to define new types RequestTrailers and ResponseTrailers in pkg/protocol/http, and finally integrate with fasthttp in pkg/stream/http

codefromthecrypt commented 2 years ago

sounds good. My plan was that even if the ResponseTrailers type is backed by fasthttp RequestHeaders one, the call sites don't change. I think we are eye-to-eye!

codefromthecrypt commented 2 years ago

@antJack there are a few glitches we'll run into with fasthttp because the trailers are comingled with the headers.

For example, logic like this either needs to be inaccurate or filter out the trailer keys

func (h ResponseHeader) ByteSize() (size uint64) {
    h.VisitAll(func(key, value []byte) {
        size += uint64(len(key) + len(value))
    })
    return size
}

Same issue in range really. I have no issue doing the filtering meanwhile (Ex. inspect to see if a header key is a trailing one and filter it out plus visa versa).

p.s. I'm not sure this is the right byte size? Is it supposed to be the text encoded size? if so, I think we need colons at least if not colon space? Maybe we can go back and document ByteSize more specifically? or should we just drop it.. wdyt?

antJack commented 2 years ago

just keep it simple, the most common usage scenarios of ByteSize() is to report size to logger or other data sinks. In these scenes, inaccuracy (not filtering out trailers) is better than missing them.

seems that byte size should account for colons, ref: https://github.com/envoyproxy/envoy/blob/main/source/common/http/header_map_impl.cc#L250

codefromthecrypt commented 2 years ago

thanks for the advice @antJack!

antJack commented 2 years ago

sorry i said it wrong yesterday

byte size should not account for colons

codefromthecrypt commented 2 years ago

@antJack I think maybe it is a good idea to release api and pkg just the version updates before I do this change. Then after we can follow-up and make improvements indepedent of go+fasthttp. wdyt?

codefromthecrypt commented 2 years ago

cc @taoyuanyuan on ^^ basically I think safest way is to tag api/pkg before I change any logic in it, mainly to adjust for vendoring go 1.18 and fasthttp version update. then roll that through, and after this go back and change logic.

codefromthecrypt commented 1 year ago

when next fasthttp comes out (v1.42.0), we can revisit this https://github.com/valyala/fasthttp/pull/1405

codefromthecrypt commented 1 year ago

1.42 is out