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

Turn a POST req into two HEAD then POST bereqs #1948

Closed
dridi opened this issue May 12, 2016 · 4 comments
Closed

Turn a POST req into two HEAD then POST bereqs #1948

dridi opened this issue May 12, 2016 · 4 comments

Comments

@dridi
Copy link
Member

dridi commented May 12, 2016

This is a followup of #1943 after the question was raised on IRC whether sending the first request as HEAD would leave the body untouched and allow a POST with a body to be carried after a restart (without using the std module).

Making sure to remove the Content-Length header removed the initial error (Uncached req.body can only be consumed once) but it still doesn't work.

Expected Behavior

I don't know what to expect. Ideally the test should pass.

Current Behavior

Currently the test times out on 4.1 and master branches, s1 receives both HEAD and POST bereq and waits for the body. It fails on 4.0: varnish closes the backend connection after sending the POST bereq headers.

Enabling the send command doesn't change anything on 4.1 and master.

I haven't looked hard at what's happening, I'm reporting it before I lose the test case.

Steps to Reproduce

varnishtest "Turn a POST req into two HEAD then POST bereqs"

server s1 {
    rxreq
    txresp -hdr X-Logged-In:yes
    expect req.method == HEAD
    expect req.url == "/"
    expect req.http.Host == session.example.com
    expect req.http.Content-Length == <undef>
    expect req.bodylen == 0

    rxreq
    txresp
    expect req.method == POST
    expect req.url == "/post"
    expect req.http.Host == service.example.com
    expect req.http.Content-Length == 5
    expect req.bodylen == 5
} -start

varnish v1 -vcl+backend {
    sub vcl_recv {
        if (req.restarts == 0){
            set req.http.X-Logged-In = "check";
            return (hash);
        }
    }

    sub vcl_backend_fetch {
        if (bereq.http.X-Logged-In == "check") {
            set bereq.method = "HEAD";
            set bereq.url = "/";
            set bereq.http.Host = "session.example.com";
            unset bereq.http.Content-Type;
        }
    }

    sub vcl_deliver {
        if (req.http.X-Logged-In == "check") {
            set req.http.X-Logged-In = resp.http.X-Logged-In;
            return (restart);
        }
    }
} -start

client c1 {
    txreq -req POST -url "/post" -hdr Host:service.example.com -body hello
    #send world
    rxresp
    expect resp.status == 200
} -run

Context

I believe the goal in the original request is to check in Varnish that a user is logged in before and then handle the original request using return (restart). This is something I hold dear (vanilla VCL without VMODs) but I always limited this varnish-level check for cacheable GET requests for obvious reasons, letting pass'ed requests deal with that at the backend level.

However I don't see any reason not to generalize this kind of checks for all requests. I tend to make GET backend requests (instead of HEAD) and cache them, but it doesn't change anything wrt the body.

Your Environment

  • Version used: 4.0.2 (not tested myself) 4.1.2 and master (376bc27)
  • Operating System and version: Up-to-date Fedora 23
@fgsch
Copy link
Member

fgsch commented May 14, 2016

This sounds like #1936

@fgsch
Copy link
Member

fgsch commented May 14, 2016

Also #1927.

In any case, you still need to cache the request body if you are going to restart.
From the docs:

Normally the request body is not available after sending it to the backend.  
By caching it is possible to retry pass operations, e.g. POST and PUT.

Regardless of being around pass the former still applies.

@dridi
Copy link
Member Author

dridi commented May 16, 2016

I'm well aware that this is not supposed to work. With Varnish 4 it's an immediate fail, which is better than 4.1+ where the test hangs until it reaches the test timeout. And because the test doesn't fail instantly I have this tiny hope that it could become legit :)

Anyway, there's at least something to fix/improve.

@lkarsten
Copy link
Contributor

Backport update: OP confirms that this fails instantly on 4.1. No fix, nothing to backport.

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

No branches or pull requests

3 participants