[PATCH 0/4] proxy_pass updates

[PATCH 0/4] proxy_pass updates

[Eric Wong]

Test coverage for common cases is pretty good by now,
one major bugfix for giant uploads.

We still need to handle HTTP trailers properly in both directions.

 lib/yahns/proxy_http_response.rb |   9 +-
 lib/yahns/proxy_pass.rb          |   5 +-
 test/test_proxy_pass.rb          | 190 +++++++++++++++++++++++++++++++++++----
 3 files changed, 182 insertions(+), 22 deletions(-)

[PATCH 1/4] proxy_pass: test and fix larger uploads

[Eric Wong]

We were incorrectly stashing the return value of detach_rbuf!
into the inter-thread buffer buffer which is bound to the client.
---
 lib/yahns/proxy_pass.rb |  5 +++--
 test/test_proxy_pass.rb | 22 +++++++++++++++++++++-
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/lib/yahns/proxy_pass.rb b/lib/yahns/proxy_pass.rb
index 09fb884..ec1fbcf 100644
--- a/lib/yahns/proxy_pass.rb
+++ b/lib/yahns/proxy_pass.rb
@@ -108,7 +108,7 @@ def send_req_body(req)
           when Array
             vec = rv # partial write, retry in case loop
           when :wait_writable
-            buf = detach_rbuf!
+            detach_rbuf!
             req[0] = vec
             return :wait_writable
           when nil
@@ -120,13 +120,14 @@ def send_req_body(req)
         # note: we do not send any trailer, they are folded into the header
         # because this relies on full request buffering
         send_req_buf("0\r\n\r\n".freeze)
+        # prepare_wait_readable already called by send_req_buf
       else # identity request, easy:
         while input.read(0x2000, buf)
           case rv = kgio_trywrite(buf)
           when String
             buf = rv # partial write, retry in case loop
           when :wait_writable
-            buf = detach_rbuf!
+            detach_rbuf!
             req[0] = buf
             return :wait_writable
           when nil
diff --git a/test/test_proxy_pass.rb b/test/test_proxy_pass.rb
index 5bf8722..53542ac 100644
--- a/test/test_proxy_pass.rb
+++ b/test/test_proxy_pass.rb
@@ -144,6 +144,7 @@ def test_proxy_pass
       cfg.instance_eval do
         app(:rack, Yahns::ProxyPass.new("http://#{host2}:#{port2}")) do
           listen "#{host}:#{port}"
+          client_max_body_size nil
         end
         stderr_path err.path
       end
@@ -154,6 +155,7 @@ def test_proxy_pass
       cfg.instance_eval do
         app(:rack, ProxiedApp.new) do
           listen "#{host2}:#{port2}"
+          client_max_body_size nil
         end
         stderr_path err.path
       end
@@ -195,7 +197,7 @@ def test_proxy_pass
       assert_equal 'text/PAIN', res['Content-Type']
       assert_equal 'HIHIHI!', res.body
 
-      # normal content-length
+      # normal content-length (PUT)
       gplv3.rewind
       req = Net::HTTP::Put.new('/')
       req.body_stream = gplv3
@@ -210,6 +212,24 @@ def test_proxy_pass
       res = http.request(Net::HTTP::Get.new('/giant-body'))
       assert_equal 200, res.code.to_i
       assert_equal OMFG, res.body
+
+      # giant PUT content-length
+      req = Net::HTTP::Put.new('/')
+      req.body_stream = StringIO.new(OMFG)
+      req.content_type = 'application/octet-stream'
+      req.content_length = OMFG.size
+      res = http.request(req)
+      assert_equal OMFG, res.body
+      assert_equal 201, res.code.to_i
+
+      # giant PUT chunked encoding
+      req = Net::HTTP::Put.new('/')
+      req.body_stream = StringIO.new(OMFG)
+      req.content_type = 'application/octet-stream'
+      req['Transfer-Encoding'] = 'chunked'
+      res = http.request(req)
+      assert_equal OMFG, res.body
+      assert_equal 201, res.code.to_i
     end
 
     # ensure we do not chunk responses back to an HTTP/1.0 client even if
-- 
EW

[PATCH 2/4] proxy_pass: test for truncated response behavior

[Eric Wong]

Even if a response is screwed, we need to ensure breakage is
predictable.
---
 test/test_proxy_pass.rb | 55 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/test/test_proxy_pass.rb b/test/test_proxy_pass.rb
index 53542ac..cebd004 100644
--- a/test/test_proxy_pass.rb
+++ b/test/test_proxy_pass.rb
@@ -7,6 +7,11 @@ class TestProxyPass < Testcase
   ENV["N"].to_i > 1 and parallelize_me!
   include ServerHelper
   OMFG = 'a' * (1024 * 1024 * 32)
+  TRUNCATE_BODY = "HTTP/1.1 200 OK\r\n" \
+                  "Content-Length: 7\r\n" \
+                  "Content-Type: text/PAIN\r\n\r\nshort".freeze
+  TRUNCATE_HEAD = "HTTP/1.1 200 OK\r\n" \
+                  "Content-Length: 666\r\n".freeze
 
   class ProxiedApp
     def call(env)
@@ -30,6 +35,16 @@ def call(env)
             sleep delay
           end
           io.close
+        when '/truncate-body'
+          io = env['rack.hijack'].call
+          io.write(TRUNCATE_BODY)
+          io.close
+        when '/truncate-head'
+          io = env['rack.hijack'].call
+          io.write(TRUNCATE_HEAD)
+          io.close
+        when '/immediate-EOF'
+          env['rack.hijack'].call.close
         when %r{\A/chunky-slow-(\d+(?:\.\d+)?)\z}
           delay = $1.to_f
           chunky = Object.new
@@ -248,6 +263,7 @@ def test_proxy_pass
         h10.close
       end
     end
+    check_truncated_upstream(host, port)
   ensure
     gplv3.close if gplv3
     quit_wait pid
@@ -298,4 +314,43 @@ def check_pipelining(host, port)
   ensure
     pl.close
   end
+
+  def check_truncated_upstream(host, port)
+    # we want to make sure we show the truncated response without extra headers
+    s = TCPSocket.new(host, port)
+    check_err
+    res = Timeout.timeout(60) do
+      s.write "GET /truncate-body HTTP/1.1\r\nHost: example.com\r\n\r\n"
+      s.read
+    end
+    s.close
+
+    exp = "HTTP/1.1 200 OK\r\n" \
+          "Content-Length: 7\r\n" \
+          "Content-Type: text/PAIN\r\n" \
+          "Connection: keep-alive\r\n" \
+          "\r\nshort"
+    assert_equal exp, res
+    errs = File.readlines(@err.path).grep(/\bERROR\b/)
+    assert_equal 1, errs.size
+    assert_match(/premature upstream EOF/, errs[0])
+    @err.truncate(0)
+
+    # truncated headers or no response at all...
+    # Send a 502 error
+    %w(immediate-EOF truncate-head).each do |path|
+      s = TCPSocket.new(host, port)
+      check_err
+      res = Timeout.timeout(60) do
+        s.write "GET /#{path} HTTP/1.1\r\nHost: example.com\r\n\r\n"
+        s.read(1024)
+      end
+      assert_match %r{\AHTTP/1.1 502\s+}, res
+      s.close
+      errs = File.readlines(@err.path).grep(/\bERROR\b/)
+      assert_equal 1, errs.size
+      assert_match(/premature upstream EOF/, errs[0])
+      @err.truncate(0)
+    end
+  end
 end
-- 
EW

[PATCH 3/4] proxy_pass: expand pipelining tests for after-upload behavior

[Eric Wong]

Pipelining uploads is rare in practice, but they must behave
properly in case some brave soul wants to start doing it.
---
 test/test_proxy_pass.rb | 51 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/test/test_proxy_pass.rb b/test/test_proxy_pass.rb
index cebd004..163a0f7 100644
--- a/test/test_proxy_pass.rb
+++ b/test/test_proxy_pass.rb
@@ -286,21 +286,44 @@ def check_pipelining(host, port)
         r2 << pl.readpartial(666)
       end
 
-      if false
-        pl.write "ET / HTTP/1.1\r\nHost: example.com\r\n\r\n"
-        until r3 =~ /hi\n/
-          r3 << pl.readpartial(666)
-        end
-      else
-        pl.write "UT / HTTP/1.1\r\nHost: example.com\r\n"
-        pl.write "Transfer-Encoding: chunked\r\n\r\n"
-        pl.write "6\r\nchunky\r\n"
-        pl.write "0\r\n\r\n"
-
-        until r3 =~ /chunky/
-          r3 << pl.readpartial(666)
-        end
+      pl.write "UT / HTTP/1.1\r\nHost: example.com\r\n"
+      pl.write "Transfer-Encoding: chunked\r\n\r\n"
+      pl.write "6\r\nchunky\r\n"
+      pl.write "0\r\n\r\n"
+
+      until r3 =~ /chunky/
+        r3 << pl.readpartial(666)
+      end
+
+      # ensure stuff still works after a chunked upload:
+      pl.write "GET / HTTP/1.1\r\nHost: example.com\r\n\r\nP"
+      after_up = ''
+      until after_up =~ /hi\n/
+        after_up << pl.readpartial(666)
+      end
+      re = /^Date:[^\r\n]+/
+      assert_equal after_up.sub(re, ''), r1.sub(re, '')
+
+      # another upload, this time without chunking
+      pl.write "UT / HTTP/1.1\r\nHost: example.com\r\n"
+      pl.write "Content-Length: 8\r\n\r\n"
+      pl.write "identity"
+      identity = ''
+
+      until identity =~ /identity/
+        identity << pl.readpartial(666)
+      end
+      assert_match %r{identity\z}, identity
+      assert_match %r{\AHTTP/1\.1 201\b}, identity
+
+      # ensure stuff still works after an identity upload:
+      pl.write "GET / HTTP/1.1\r\nHost: example.com\r\n\r\n"
+      after_up = ''
+      until after_up =~ /hi\n/
+        after_up << pl.readpartial(666)
       end
+      re = /^Date:[^\r\n]+/
+      assert_equal after_up.sub(re, ''), r1.sub(re, '')
     end
     r1 = r1.split("\r\n").reject { |x| x =~ /^Date: / }
     r2 = r2.split("\r\n").reject { |x| x =~ /^Date: / }
-- 
EW

[PATCH 4/4] proxy_pass: more tests for giant headers and truncations

[Eric Wong]

We need to ensure more uncommon cases such as gigantic upstream
headers and truncated upstream responses are handled properly
and predictably.
---
 lib/yahns/proxy_http_response.rb |  9 ++++--
 test/test_proxy_pass.rb          | 62 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 66 insertions(+), 5 deletions(-)

diff --git a/lib/yahns/proxy_http_response.rb b/lib/yahns/proxy_http_response.rb
index 31505d3..8f4790e 100644
--- a/lib/yahns/proxy_http_response.rb
+++ b/lib/yahns/proxy_http_response.rb
@@ -28,10 +28,13 @@ def proxy_write(wbuf, buf, alive)
 
   def proxy_err_response(code, req_res, exc, wbuf)
     logger = @hs.env['rack.logger']
-    if exc
-      Yahns::Log.exception(logger, 'upstream error', exc)
-    else
+    case exc
+    when nil
       logger.error('premature upstream EOF')
+    when Kcar::ParserError
+      logger.error("upstream response error: #{exc.message}")
+    else
+      Yahns::Log.exception(logger, 'upstream error', exc)
     end
     # try to write something, but don't care if we fail
     Integer === code and
diff --git a/test/test_proxy_pass.rb b/test/test_proxy_pass.rb
index 163a0f7..47fc231 100644
--- a/test/test_proxy_pass.rb
+++ b/test/test_proxy_pass.rb
@@ -13,6 +13,11 @@ class TestProxyPass < Testcase
   TRUNCATE_HEAD = "HTTP/1.1 200 OK\r\n" \
                   "Content-Length: 666\r\n".freeze
 
+  # not too big, or kcar will reject
+  BIG_HEADER = [%w(Content-Type text/plain), %W(Content-Length #{OMFG.size})]
+  3000.times { |i| BIG_HEADER << %W(X-#{i} BIG-HEADER!!!!!!!!!!!!!!) }
+  BIG_HEADER.freeze
+
   class ProxiedApp
     def call(env)
       h = [ %w(Content-Length 3), %w(Content-Type text/plain) ]
@@ -22,6 +27,11 @@ def call(env)
         when '/giant-body'
           h = [ %W(Content-Length #{OMFG.size}), %w(Content-Type text/plain) ]
           [ 200, h, [ OMFG ] ]
+        when '/big-headers'
+          [ 200, BIG_HEADER, [ OMFG ] ]
+        when '/oversize-headers'
+          100000.times { |x| h << %W(X-TOOBIG-#{x} #{x}) }
+          [ 200, h, [ "big" ] ]
         when %r{\A/slow-headers-(\d+(?:\.\d+)?)\z}
           delay = $1.to_f
           io = env['rack.hijack'].call
@@ -61,7 +71,12 @@ def chunky.each
           [ 200, h, [ "hi\n"] ]
         end
       when 'HEAD'
-        [ 200, h, [] ]
+        case env['PATH_INFO']
+        when '/big-headers'
+          [ 200, BIG_HEADER, [] ]
+        else
+          [ 200, h, [] ]
+        end
       when 'PUT'
         buf = env['rack.input'].read
         [ 201, {
@@ -158,7 +173,7 @@ def test_proxy_pass
       @srv2.close
       cfg.instance_eval do
         app(:rack, Yahns::ProxyPass.new("http://#{host2}:#{port2}")) do
-          listen "#{host}:#{port}"
+          listen "#{host}:#{port}", sndbuf: 16384
           client_max_body_size nil
         end
         stderr_path err.path
@@ -245,6 +260,14 @@ def test_proxy_pass
       res = http.request(req)
       assert_equal OMFG, res.body
       assert_equal 201, res.code.to_i
+
+      # sometimes upstream feeds kcar too much
+      req = Net::HTTP::Get.new('/oversize-headers')
+      res = http.request(req)
+      errs = File.readlines(@err.path).grep(/ERROR/)
+      assert_equal 1, errs.size
+      assert_match %r{upstream response error:}, errs[0]
+      @err.truncate(0)
     end
 
     # ensure we do not chunk responses back to an HTTP/1.0 client even if
@@ -264,6 +287,8 @@ def test_proxy_pass
       end
     end
     check_truncated_upstream(host, port)
+    check_slow_giant_body(host, port)
+    check_slow_read_headers(host, port)
   ensure
     gplv3.close if gplv3
     quit_wait pid
@@ -376,4 +401,37 @@ def check_truncated_upstream(host, port)
       @err.truncate(0)
     end
   end
+
+  def check_slow_giant_body(host, port)
+    s = TCPSocket.new(host, port)
+    s.write "GET /giant-body HTTP/1.0\r\n\r\n"
+    sleep 0.1
+    str = ''
+    buf = ''
+    assert_raises(EOFError) { loop { str << s.readpartial(400, buf) } }
+    h, b = str.split(/\r\n\r\n/, 2)
+    assert_equal OMFG, b
+    assert_match %r{\AHTTP/1\.1 200\b}, h
+  ensure
+    s.close if s
+  end
+
+  def check_slow_read_headers(host, port)
+    s = TCPSocket.new(host, port)
+    s.write "GET /big-headers HTTP/1.1\r\nHost: example.com\r\n\r\n"
+    s.write "HEAD /big-headers HTTP/1.0\r\n\r\n"
+    buf = ''
+    res = ''
+    sleep 0.1
+    begin
+      res << s.readpartial(32786, buf)
+    rescue EOFError
+      break
+    end while true
+    # res = Timeout.timeout(60) { s.read }
+    assert_match %r{\r\n\r\n\z}, res
+    assert_match %r{\AHTTP/1\.1 200 OK}, res
+  ensure
+    s.close if s
+  end
 end
-- 
EW