mattrobenolt
Repos
345
Followers
470

Extra security for your sensitive pages

276
22

email me when a thing is done

53
3

Opinionated uWSGI setup

15
0

CLI for Jinja2

387
82

Events

Change api to use connect option conventions (#1)

Hey! Just opened up this pr with some changed that I think will bring the api a little closer to what connect-go uses.

Changes two things:

  • brotli.New and brotli.NewLevel are renamed to brotli.WithCompression and brotli.WithCompressionLevel respectively to come closer to terminology used in connect-go around option naming.
  • brotli.WithCompression and brotli.WithCompressionLevel returns a single connect.Option that can be passed into handler or client constructors.

Let me know what you think

Co-authored-by: Matt Robenolt matt@ydekproductions.com

Created at 4 hours ago
pull request closed
Change api to use connect option conventions

Hey! Just opened up this pr with some changed that I think will bring the api a little closer to what connect-go uses.

Changes two things:

  • brotli.New and brotli.NewLevel are renamed to brotli.WithCompression and brotli.WithCompressionLevel respectively to come closer to terminology used in connect-go around option naming.
  • brotli.WithCompression and brotli.WithCompressionLevel returns a single connect.Option that can be passed into handler or client constructors.

Let me know what you think

Created at 4 hours ago
issue comment
http3: ContentLength must be -1 for unknown body length

@marten-seemann :wave: let me know if you have any other suggestions or opinions here.

Created at 7 hours ago
issue comment
Feature: Return error if ResponseWriter does not implement Flush() for streaming RPCs.

Not a solution necessarily, but go1.20 is attempting to address this with their new http.ResponseController. https://pkg.go.dev/net/http@master#ResponseController

Created at 1 week ago
issue comment
implement Server.RequestContext

At this point, why not just use a middleware pattern and wrap the server handler?

Still going to strongly suggest instead of any of this, we just implement ConnContext correctly.

Created at 1 week ago
issue comment
implement Server.ConnContext (#3603)

To be clear, to do this correctly, this needs to be hooked much much sooner in the flow and ensure it's passed all the way through. This should be hooked as close to the Accept() call as is realistic.

Created at 1 week ago
issue comment
implement Server.ConnContext (#3603)

Just to chime in since I've been looking for this to. This is entirely not the equivalent to stdlib and it'd be very disingenuous to commit to this feature and it behave this way.

In this implementation, this is happening per-request, not per-connection. The entire point of the ConnContext is to be one per connection.

Created at 1 week ago
issue comment
Error: no Grpc-Status trailer for wrapped http.ResponseWriters

Playing around a tad more since I was curious, this might be a decent option:

type responseWriterFlusher interface {
	http.ResponseWriter
	http.Flusher
}

type RWWrapper struct {
	responseWriterFlusher
}

// assure our wrapper satisfies both http.ResponseWriter and http.Flusher
var _ responseWriterFlusher = (*RWWrapper)(nil)

func main() {
	mux := http.NewServeMux()
	mux.Handle("/", WrapperMiddleware(http.NewServeMux()))

}

func WrapperMiddleware(h http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		if rwf, ok := w.(responseWriterFlusher); ok {
			h.ServeHTTP(&RWWrapper{rwf}, r)
		} else {
			// we simply require both interfaces to be satisfied, so there's nothing
			// more reasonable we can do, and your server is implemented incorrectly.
			panic("uh oh")
		}

	})
}
Created at 1 week ago
issue comment
Error: no Grpc-Status trailer for wrapped http.ResponseWriters

I'm speculating without investigating a ton, but http.ResponseWriter isn't fully enough to be compliant, it also needs to implement http.Flusher.

This is definitely a wart about the stdlib net/http module, but it is what it is.

connect-go (any other implementation) need to explicitly test against this interface: https://github.com/bufbuild/connect-go/blob/main/protocol.go#L300-L304

And this is documented in net/http docs: https://pkg.go.dev/net/http#Flusher

The default HTTP/1.x and HTTP/2 ResponseWriter implementations support Flusher, but ResponseWriter wrappers may not. Handlers should always test for this ability at runtime.

I would suggest doing something like this: https://go.dev/play/p/geC2Ns2zotI

type RWWrapper struct {
	http.ResponseWriter
}

func (w *RWWrapper) Flush() {
	if f, ok := w.ResponseWriter.(http.Flusher); ok {
		f.Flush()
	}
}

// assure our wrapper satisfies both http.ResponseWriter and http.Flusher
var (
	_ http.ResponseWriter = (*RWWrapper)(nil)
	_ http.Flusher        = (*RWWrapper)(nil)
)
Created at 1 week ago
create branch
mattrobenolt create branch main
Created at 1 week ago
delete branch
mattrobenolt delete branch master
Created at 1 week ago

Modernize and add uhh, fix decimal vs binary units

  • Adopt go modules, moved to go.withmatt.com/size
  • Added all binary and decimal units
Created at 1 week ago

http3: ContentLength must be -1 for unknown body length

According to the http.Request docs, on a server request, a ContentLength of -1 indicates an unknown length.

So in the case of a request without an explicit Content-Length header, the ContentLength was being set to 0, which explicitly indicates there's no body to read. Whereas -1 would indicate to read until EOF.

When no body is expected, let ContentLength be 0.

Created at 1 week ago

http3: ContentLength must be -1 for unknown body length

According to the http.Request docs, on a server request, a ContentLength of -1 indicates an unknown length.

So in the case of a request without an explicit Content-Length header, the ContentLength was being set to 0, which explicitly indicates there's no body to read. Whereas -1 would indicate to read until EOF.

When no body is expected, let ContentLength be 0.

Created at 1 week ago

http3: ContentLength must be -1 for unknown body length

According to the http.Request docs, on a server request, a ContentLength of -1 indicates an unknown length.

So in the case of a request without an explicit Content-Length header, the ContentLength was being set to 0, which explicitly indicates there's no body to read. Whereas -1 would indicate to read until EOF.

When no body is expected, let ContentLength be 0.

Created at 1 week ago
issue comment
http3: ContentLength must be -1 for unknown body length

@marten-seemann I have updated this with more tests, and to add a set of methods with expected bodies.

This gets us I think to where we wanted, where a GET request ends up with a ContentLength=0, and a POST without a Content-Length header, gets us to ContentLength=-1.

Created at 1 week ago

http3: ContentLength must be -1 for unknown body length

According to the http.Request docs, on a server request, a ContentLength of -1 indicates an unknown length.

So in the case of a request without an explicit Content-Length header, the ContentLength was being set to 0, which explicitly indicates there's no body to read. Whereas -1 would indicate to read until EOF.

When no body is expected, let ContentLength be 0.

Created at 1 week ago
issue comment
http3: ContentLength must be -1 for unknown body length

Personally I wouldn't do that since the specs allow bodies on other methods, even if they're not typically supported. That's just a choice your implementation can make, say, not supporting a body on a GET. I haven't checked this against what stdlib stuff does since it's not very applicable to me.

I'd say if anything, it's an easy iteration and should be sufficient for a lot of cases.

I wonder if the inverse would be problematic tho? Since you could POST/PUT whatever without bodies at all. So in theory if I POST with no body, I'd expect ContentLength to be 0, not -1. But I think we're splitting hairs here.

Created at 1 week ago
issue comment
http3: ContentLength must be -1 for unknown body length

Ah, that makes more sense. I got the Transfer-Encoding because I was using the httputil.DumpRequest, which apparently isn't fully accurate. :) This has been a bit tricky to debug I'll admit.

But it seems that logic is correct to me. I'm not sure off hand how to know in http3 if there is a body or not. Happy to look into it later, or you can commandeer this PR since I'm sure you'll be faster with it.

Created at 1 week ago
issue comment
http3: ContentLength must be -1 for unknown body length

Ok, so I really think this boils down to simply being Transfer-Encoding chunked. It seems to be correct in other cases.

Created at 1 week ago
issue comment
http3: ContentLength must be -1 for unknown body length

In stdlib, I guess the logic here is much more in depth: https://cs.opensource.google/go/go/+/refs/tags/go1.19.3:src/net/http/transfer.go;l=669-737;drc=8c17505da792755ea59711fc8349547a4f24b5c5

I'm still inclined to say, sending back -1 when unknown is better than 0. I think the difference is an efficiency gain for clients when they know the length, they can allocate buffers and whatnot to read it efficiently.

Created at 1 week ago
issue comment
http3: ContentLength must be -1 for unknown body length

Oh, and I guess to clarify my current findings, I am sending the request as Transfer-Encoding: chunked, which is why through normal http.Server, I get a ContentLength of -1, whereas through http3.Server, I get 0. So maybe we can also check the Transfer-Encoding header.

Created at 1 week ago
issue comment
http3: ContentLength must be -1 for unknown body length

I'm just saying that we should figure out how to handle the case where there's no body.

Yeah, I just think that's entirely tangential.

Poking around at what http stdlib does, I think there's a bit of magic happening, since it seems to fill a correct ContentLength in normal cases, even when there's no Content-Length header, so my assumption is it's reading the body into a buffer and then it knows the actual length.

The only case req.ContentLength is coming through as -1 is when Transfer-Encoding: chunked is set on the request headers.

So to be fair, there could be a bug elsewhere, since I'm also using the http3.RoundTripper, and when using that, somehow a Content-Length isn't being set while using an http2.RoundTripper, it is.

I'm going to continue investigating, but I still believe defaulting here to -1 is more correct than 0.

Created at 1 week ago
issue comment
http3: ContentLength must be -1 for unknown body length

I guess what I'm saying is this isn't about the 0 length case, it's about an unknown length explicitly. If I'm a client, and I send a request without a Content-Length header, but I'm streaming in a long body, saying req.ContentLength of 0, to me as a server handler, indicates the body is a length of 0, rather than an unknown length, as -1 would indicate.

Created at 1 week ago
issue comment
http3: ContentLength must be -1 for unknown body length

Sure it does, according to the ContentLength spec:

The value -1 indicates that the length is unknown.

If I'm implementing a server, and I want to read a body, I would use req.ContentLength to say, allocate a fixed []byte to read my body into. If it's 0, I can't exactly differentiate between "unknown" and "0" without also double checking against the Content-Length header.

But either way, this behavior differs from the stdlib http server, and caused an issue on my end that I needed to work around. I'm fine keeping the workaround, but it feels incorrect on http3.Server's side, according to their docs.

Created at 1 week ago