cli: fix usage of gzip.Reader to better detect corrupt snapshots during save/restore #7697
Conversation
|
I think given how we nest a required |
| // StartTestServer fires up a web server on a random unused port to serve the | ||
| // given handler body. The address it is listening on is returned. When the | ||
| // test case terminates the server will be stopped via cleanup functions. | ||
| func StartTestServer(t testing.T, handler http.Handler) string { |
There was a problem hiding this comment.
This looks very similar to https://golang.org/pkg/net/http/httptest/#NewServer.
Would httptest.Server work here? If not I think the reason would be helpful to include in the docstring.
There was a problem hiding this comment.
Mostly httptest.Server only thinks a port is free if it's not bound. Consul tests frequently reserve ports via sdk/freeport so you can have one part of the test try to use a port and know nothing is listening. If you simply assumed unbound ports were free you'd end up with test cross-talk and weirdness.
There was a problem hiding this comment.
updating comment when i do the merge
|
|
||
| if err := concludeGzipRead(decomp); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
very minor style note, you could remove the if and return err in both cases:
err := concludeGzipRead(decomp)
return &metdata, errI believe as a convention non-err return values are considered to be not usable if the err return is non-nil, unless explicitly documented.
There was a problem hiding this comment.
In general I agree with what you're saying, but I try to avoid leaving loaded footguns lying around if it's fairly cheap to do so, so prefer to just not hand back non-zero return values when an error occurs to prevent being misleading.
When generating a snapshot using
consul snapshot savethe server prepares the snapshot as a gzip compressed tar archive which is streamed over an HTTP response straight to disk on the CLI side. Immediately after it receives this data it runs an internal verification to sanity check that the snapshot that was produced is parseable. It does this by:gzip.ReaderSometimes "network things happen" and you get your responses from the server unexpectedly truncated early (like #6893). This should be expected when streaming a large snapshot, and the CLI should detect it as #6893 does ... sometimes.
Unfortunately if you do targeted removal of a few bytes from the end of the snapshot files they still validate.
There's a small catch to how to safely use a
gzip.Readerand we aren't doing that today:We don't explicitly attempt to drain the
gzip.Readerafter walking the tar archive portion to see if the rest of the gzip envelope is present in its entirety. If you do actually read that stream until EOF you can properly fail when even a single byte is lost from the end of the gzip envelope.This PR fixes that, and adds tests to cover early truncation situations like this.