Is the documentation in the BigPipe README correct?

It advises:

vcl_fetch {
  if (beresp.Surrogate-Control ~ "BigPipe/1.0") {
    set beresp.do_stream = true;
    set beresp.ttl = 0;
  }
}

I am uncertain under what circumstances BigPipe might switch from using BigPipe/1.0 to BigPipe/1.1 or BigPipe/2.0. It is also unclear whether the 1.0 here is the exact version of the BigPipe module, or, what seems more likely, it might be a form of protocol version that changes less frequently. That is, might BigPipe version 1.3 continue to use "BigPipe/1.0", or would it report "BigPipe/1.3"? If BigPipe is adopted in Drupal 8 core, would it report "BigPipe/1.0", or "BigPipe/8.0"?

Clearly, it is possible that future versions might require different varnish settings; also, other clients might in the future use the Surrogate-Control for some purpose other than Varnish cache control, which might require updating the reported version number for other unrelated purposes. I don't really know; this is a question.

If the reported Surrogate-Control value does get bumped up to BigPipe/1.1, would it be better if clients that are not yet savvy to the change stop using the modified varnish settings (which definitely breaks), or continue using the old settings (might break)? If the former, it might be better to recommend testing for "BigPipe/1" or "BigPipe/", as a contingency. Individual users who build their own servers can be expected to update their configuration before they upgrade their BigPipe implementation, but service providers that set up varnish for clients will want to use a strategy that is best suited to long-term stability. Also, even for individual users, it would be best to minimize the need to update varnish configuration.

It would be great if the README could be updated to explain what the plan / policy for changing the Surrogate-Control value is, and what the best possible configuration tests are.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greg.1.anderson created an issue. See original summary.

Wim Leers’s picture

Title: BigPipe Varnish documentation » BigPipe Varnish documentation: Surrogate-Control header value's future evolution?
Category: Support request » Task

These are excellent questions! Let me think about that some time. Perhaps @Fabianx also has thoughts on this.

Fabianx’s picture

I don't think there will ever be a 1.1 atm. ESI is also still at ESI/1.0 since ages.

So I would say:

BigPipe/1.0 just means BigPipe as streaming technology, not as advertised version.

Thoughts?

Wim Leers’s picture

Status: Active » Needs review
FileSize
1.06 KB

I think I agree with @Fabianx. Even more so because we do not expect anything special of proxies: the only thing we expect is that they don't perform buffering. That's it. We can change implementation details of BigPipe, but the key requirement of BigPipe is just that: no buffering.

Consequently, I also can't see any need to update this version number in the future.

Finally, note that ESI has far more requirements for proxies claiming to support it. They haven't had to (or simply chose not to) increase their version number.


Patch attached that adds this information to the README.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Sounds perfect!

Wim Leers’s picture

Status: Reviewed & tested by the community » Fixed

Per #5, committed & pushed #4.

Wim Leers’s picture

Adjusting issue credit.

  • Wim Leers committed 9fecb38 on 8.x-1.x
    Issue #2644146 by Wim Leers, Fabianx, greg.1.anderson: BigPipe Varnish...
Anonymous’s picture

fyi,

for current Varnish v4, vcl_fetch() has been replaced by vcl_backend_response(), cref: https://www.varnish-cache.org/docs/4.0/whats-new/upgrading.html#vcl-fetc...

and,

    set beresp.ttl = 0;

throws a VCL compile error

Jan 18 11:18:04 test.loc varnishd[11250]: Error:
Jan 18 11:18:04 test.loc varnishd[11250]: Message from VCC-compiler:
Jan 18 11:18:04 test.loc varnishd[11250]: Expected ID got ';'
Jan 18 11:18:04 test.loc varnishd[11250]: (program line 166), at
Jan 18 11:18:04 test.loc varnishd[11250]: ('input' Line 437 Pos 35)
Jan 18 11:18:04 test.loc varnishd[11250]: set beresp.ttl = 0;
Jan 18 11:18:04 test.loc varnishd[11250]: ----------------------------------#
Jan 18 11:18:04 test.loc varnishd[11250]: Running VCC-compiler failed, exited with 2
Jan 18 11:18:04 test.loc varnishd[11250]: VCL compilation failed

should be, instead,

    set beresp.ttl = 0s;
Wim Leers’s picture

Status: Fixed » Active

Thanks, will fix.

Wim Leers’s picture

Created a separate issue + patch for that: #2652692: BigPipe's Varnish documentation does not apply to Varnish 4. Please review.

Wim Leers’s picture

Giving @bugsonly issue credit here because his comment is not on #2652692: BigPipe's Varnish documentation does not apply to Varnish 4, which means I can't give him issue credit there.

Thanks again!

Wim Leers’s picture

Which means… congrats with your first credit on your d.o profile: https://www.drupal.org/u/bugsonly :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

x610’s picture

@Wim

We were trying to apply VCL changes on acquia, and found out, that for Varnish 3 `beresp.Surrogate-Control` is not correct, instead it should be `beresp.http.Surrogate-Control`.

README.md still says `beresp.Surrogate-Control`.

kostajh’s picture

Status: Closed (fixed) » Needs review
FileSize
399 bytes

@x610 is right. Here's a patch.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Indeed.

The last submitted patch, 4: 2644146-4.patch, failed testing.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review

Heh, https://www.drupal.org/documentation/modules/big_pipe/environment is already correct.

I think it may be better to just remove this from the README and point to the online docs.

kostajh’s picture

Up to ya'll. I personally find README docs more reliable/trustworthy than the d.o. documentation pages, but so long as it's clear where to look, then removing from the README is fine.

  • Wim Leers committed c5574e8 on 8.x-1.x
    Follow-up for issue #2644146 by Wim Leers: strip the README, refer to...
Wim Leers’s picture

Status: Needs review » Fixed

I personally find README docs more reliable/trustworthy than the d.o. documentation pages

Hah! You're not alone!

However, since this module now also lives in Drupal core, it just makes more sense to let that be the sole point of documentation.

Did exactly that.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.