Compare 'bereq.backend' / 'req.backend_hint' myDirector.backend() does not work

[ronaldploeger]

"if ( req.backend_hint == d1.backend() )" and also "if ( bereq.backend == d1.backend() )" does not compile in Varnish 4.1.4. Please see the following Varnish Test Case:

varnishtest "Test backends and directors"

server s1 {
  rxreq
  txresp -status 200
} -start


varnish v1 -vcl {
  import std;
  import directors;
  backend b1 {
    .host = "${s1_addr}";
    .port = "${s1_port}";
  }

  sub vcl_init {
    new d1 = directors.round_robin();
    d1.add_backend(b1);
  }

  sub vcl_recv {
    set req.backend_hint = d1.backend();
  }

  sub vcl_backend_response {
    std.log("xxxxxxxx-vcl_backend_response-bereq: " + bereq.backend);
    std.log("xxxxxxxx-vcl_backend_response-beresp: " + beresp.backend);
    if ( bereq.backend == d1.backend() ) {
      set beresp.http.X-Backend-Response = "d1";
    }
  }

  sub vcl_deliver {
    std.log("xxxxxxxx-vcl_deliver: " + req.backend_hint);
    if ( req.backend_hint == d1.backend() ) {
      set resp.http.X-Backend-Deliver = "d1";
    }
  }

} -start

client c1 {
  txreq
  rxresp
  expect resp.status == 200
  expect resp.http.X-Backend-Response == "d1"
  expect resp.http.X-Backend-Deliver == "d1"
} -run

Please also see the following discussion:
https://www.varnish-cache.org/lists/pipermail/varnish-misc/2016-December/025500.html
https://www.varnish-cache.org/lists/pipermail/varnish-misc/2016-December/025502.html
https://www.varnish-cache.org/lists/pipermail/varnish-misc/2016-December/025504.html
https://www.varnish-cache.org/lists/pipermail/varnish-misc/2016-December/025505.html

@Dridi says: "mention that it does work on the master branch"

Current Behavior

**** v1    0.4 CLI RX| Message from VCC-compiler:\n
**** v1    0.4 CLI RX| Backend not found: 'd1.backend'\n
**** v1    0.4 CLI RX| ('<vcl.inline>' Line 22 Pos 27)\n
**** v1    0.4 CLI RX|     if ( bereq.backend == d1.backend() ) {\n
**** v1    0.4 CLI RX| --------------------------##########------\n

Your Environment

• varnish-4.1.4 revision 4529ff7
• Ubuntu trusty

[bsdphk]

This works in -trunk, maybe as a result of the sym/var type changes I made to VCC some days ago (9d81a71)

[Dridi]

Assigning to @hermunn (you can ping me if you need help on this one)

[hermunn]

In vcl_backend_response, a work-around (assuming you know the backend of d1 is b1), is comparing with b1 instead of d1

	if ( bereq.backend == b1 ) {
		set beresp.http.X-Backend-Response = "d1";
	}

A proper work-around is

sub vcl_backend_response {
	set bereq.http.x-d1-backend = d1.backend();
	set bereq.http.x-req-backend = bereq.backend;
	if ( bereq.http.x-d1-backend == bereq.http.x-req-backend ) {
		set beresp.http.X-Backend-Response = "d1";
	}
}

In vcl_deliver, the following works:

	set req.http.xxxx = d1.backend();
	set req.http.xxxxhint = req.backend_hint;
	if ( req.http.xxxxhint == req.http.xxxx ) {
		set resp.http.X-Backend-Deliver = "d1";
	}

The type system in vcl has changed a lot i master since 4.1, and I would say that comparing backend expressions is a feature not present in 4.1, and that we should leave it that way.

However, trying to make a shorter, similar workaround for vcl_backend_response,

sub vcl_backend_response {
	set bereq.http.x-d1-backend = d1.backend();
	if ( bereq.backend == bereq.http.x-d1-backend ) {
		set beresp.http.X-Backend-Response = "d1";
	} else {
		set beresp.http.X-Backend-Response = bereq.backend;
	}
}

turns out to crash VCC with the message:

**** v1    0.3 CLI RX| Message from VCC-compiler:\n
**** v1    0.3 CLI RX| Assert error in vcc_Var_Wildcard(), vcc_var.c line 85:\n
**** v1    0.3 CLI RX|   Condition((sym) != 0) not true.\n
**** v1    0.3 CLI RX| Running VCC-compiler failed, signal 6\n
**** v1    0.3 CLI RX| VCL compilation failed

I will try to fix the crash, but in my opinion nothing else should be done for 4.1.

[hermunn]

The crash in the previous comment happens because vcc_Var_Wildcard only produces symbols of type SYM_VAR. I will explain:

The expression bereq.backend == bereq.http.x-d1-backend causes VCC_FindSymbol to be called with "bereq.http.x-d1-backend" as one of the parameters. Another parameter representins SYM_BACKEND, since the left hand side is a backend. This call to VCC_FindSymbol happens in vcc_expr_cmp.

In VCC_FindSymbol, the string matches the "wildcard" symbol bereq.http., and this causes a new symbol with the name "bereq.http.x-d1-backend" of type SYM_VAR to be generated by vcc_Var_Wildcard and returned from VCC_FindSymbol.

The next time VCC_FindSymbol is called (this time not from vcc_expr_cmp) with the same parameters, the now existing symbol "bereq.http.x-d1-backend" of type SYM_VAR is not matched since VCC_FindSymbol searches for a symbol "bereq.http.x-d1-backend" of type SYM_BACKEND. This time around a new symbol cannot be created because of "Name Collision" in vcc_AddSymbol. The name collision causes NULL to be returned to vcc_Var_Wildcard, and we run into the assert.

One solution would be, in vcc_expr_cmp, to call a different version of VCC_FindSymbol that does not allow matching with wildcards. Another solution would be to change the prototype of "wildcard" functions to include a symbol type, so that the wildcard function can return NULL if a symbol of the wanted type cannot be produced. There are probably many other ways of fixing this, too. However, I prefer to just turn the crash into a helpful error message, not to add features or change the behavior of the code.

[hermunn]

I wrote a patch, hermunn@2db2901 , that should fix the problem with the crash, and maybe be helpful. Comments are welcome.

[Dridi]

@hermunn what would error messages look like?

[hermunn]

The test case checks the error messages to some degree, but I should give you the full info:

In the first -errvcl, the complete error is

Not a backend: 'bereq.http.a' (Right hand side must be a backend, but saw a var)

In the second -errvcl, you get

Backend not found: 'foo' This must be a backend identifier.

The point here is that people might privoed an expression. Maybe I should use the word "expected" in this message?

In the last -errvcl, the error is

Expected ID got '"foo"'

which is unchanged from before the patch.

[Dridi]

No it's fine, it's just that since it's not a pull request I won't git fetch it out of the box, so I got extra lazy and didn't push the review beyond reading the patch :)

[hermunn]

I think I will just change the error message slightly and push this. If anyone feels the need to fix this further, that's ok, but right now we should be happy to get rid of the crash and know what the work-around is.

[hermunn]

Since this issue is not in master and 18c9983 fixes this in 4.1, I am closing this issue.

[hermunn]

Backport review: This patch was to 4.1, and the problem does not exist in 5.0.