Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ensure resp.Body is discarded to avoid broken tcp sockets #348

Merged
merged 1 commit into from
Aug 14, 2018
Merged

ensure resp.Body is discarded to avoid broken tcp sockets #348

merged 1 commit into from
Aug 14, 2018

Conversation

tmsmr
Copy link
Contributor

@tmsmr tmsmr commented Aug 14, 2018

Hi guys,

currently the Blackbox exporter produces server-side (Confirmed with Zuul, NGINX, Python 3 http.server, TCP-Resets in Wireshark) broken pipes when probing with a 'big' HTTP request.

Example:

  • Download and start the Blackbox exporter with default configuration
cd /tmp
wget https://github.com/prometheus/blackbox_exporter/releases/download/v0.12.0/blackbox_exporter-0.12.0.linux-amd64.tar.gz
tar xf blackbox_exporter-0.12.0.linux-amd64.tar.gz
(cd blackbox_exporter-0.12.0.linux-amd64 && ./blackbox_exporter &)
  • Create some random payload
dd if=/dev/urandom of=small-garbage bs=1k count=10
dd if=/dev/urandom of=big-garbage bs=1k count=1000
  • Serve the content
python3 -m http.server &
  • Request small-garbage
curl http://localhost:9115/probe\?target\=127.0.0.1:8000/small-garbage\&module\=http_2xx > /dev/null

...
127.0.0.1 - - [14/Aug/2018 15:17:06] "GET /small-garbage HTTP/1.1" 200 -
...
  • Request big-garbage
curl http://localhost:9115/probe\?target\=127.0.0.1:8000/big-garbage\&module\=http_2xx > /dev/null

...
Traceback (most recent call last):
  File "/usr/lib/python3.7/socketserver.py", line 647, in process_request_thread                                                                   
    self.finish_request(request, client_address)
  File "/usr/lib/python3.7/socketserver.py", line 357, in finish_request
    self.RequestHandlerClass(request, client_address, self)
  File "/usr/lib/python3.7/http/server.py", line 646, in __init__
    super().__init__(*args, **kwargs)
  File "/usr/lib/python3.7/socketserver.py", line 717, in __init__
    self.handle()
  File "/usr/lib/python3.7/http/server.py", line 426, in handle
    self.handle_one_request()
  File "/usr/lib/python3.7/http/server.py", line 414, in handle_one_request
    method()
  File "/usr/lib/python3.7/http/server.py", line 653, in do_GET
    self.copyfile(f, self.wfile)
  File "/usr/lib/python3.7/http/server.py", line 844, in copyfile
    shutil.copyfileobj(source, outputfile)
  File "/usr/lib/python3.7/shutil.py", line 82, in copyfileobj
    fdst.write(buf)
  File "/usr/lib/python3.7/socketserver.py", line 796, in write
    self._sock.sendall(b)
ConnectionResetError: [Errno 104] Connection reset by peer
----------------------------------------
....

In ProbeHTTP (https://github.com/prometheus/blackbox_exporter/blob/master/prober/http.go#L135) resp.Body is read/discarded only if the payload gets checked for certain contents in https://github.com/prometheus/blackbox_exporter/blob/master/prober/http.go#L42.

The resp.Body should always be read/discarded, no matter whether the body is needed or not.

@brian-brazil
Copy link
Contributor

Can you add the DCO please?

Signed-off-by: Thomas Maier <contact@thomas-maier.net>
@tmsmr
Copy link
Contributor Author

tmsmr commented Aug 14, 2018

Done. I was confused - thought this DCO thingy is for maintainers only...

@brian-brazil brian-brazil merged commit 9a02cbf into prometheus:master Aug 14, 2018
@brian-brazil
Copy link
Contributor

Thanks!

It's for everyone, it's a legal copyright thingy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants