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.

Sign up for GitHub

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

Jump to bottom

Support NotSet string behavior in falco #248

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Support NotSet string behavior in falco #248

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
6cb06ee
WIP: define LenientString, HttpHeader
ysugimoto Jan 9, 2024
f08521b
WIP: refactor transport and director, create falco http related struct
ysugimoto Jan 10, 2024
6efb4fb
fix variables
ysugimoto Jan 10, 2024
5501ff5
fix up value and variable considering lenient string
ysugimoto Jan 10, 2024
c1a7165
wip: builtin functions
ysugimoto Jan 21, 2024
88c1a81
Merge remote-tracking branch 'origin/main' into feat/re-implementatio…
ysugimoto Jan 26, 2024
94afef2
upstream main
ysugimoto Jan 26, 2024
97eea38
pass interpreter tests with Lenient String
ysugimoto Jan 26, 2024
74ba597
avoid nil pointer panic
ysugimoto Jan 26, 2024
7069e59
fix built-in functions for lenient string
ysugimoto Jan 27, 2024
9a24af4
change process pointer as Public
ysugimoto Jan 27, 2024
52ed72e
linting
ysugimoto Jan 27, 2024
33821d6
divide testing vcl files
ysugimoto Jan 27, 2024
2e79923
add comment for refered issue URL
ysugimoto Jan 27, 2024
bf747c7
tidy mod
ysugimoto Jan 27, 2024
3dcfbd2
remove unused function
ysugimoto Jan 27, 2024
1edace3
fix typo and reflect reviews
ysugimoto Feb 18, 2024
3d944df
resolve conflict
ysugimoto Feb 18, 2024
5e786ac
remove unnecessary rand initialization
ysugimoto Feb 18, 2024
0850dae
fix failing test after merged #254
ysugimoto Feb 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
dist/*
playground
tools
local

.vscode

Expand Down
27 changes: 19 additions & 8 deletions cmd/falco/runner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,27 +336,38 @@ func TestTester(t *testing.T) {
tests := []struct {
name string
main string
filter string
passes int
}{
{
name: "table manipulation test",
main: "../../examples/testing/table_manipulation.vcl",
filter: "*table_*.test.vcl",
main: "../../examples/testing/table_manipulation/main.vcl",
passes: 2,
},
{
name: "empty and notset value test",
main: "../../examples/testing/default_values.vcl",
filter: "*values.test.vcl",
main: "../../examples/testing/default_values/main.vcl",
passes: 16,
},
{
name: "assertions test",
main: "../../examples/testing/assertion.vcl",
filter: "*assertion.test.vcl",
main: "../../examples/testing/assertion/main.vcl",
passes: 5,
},
{
name: "notset string test",
main: "../../examples/testing/notset_string/main.vcl",
passes: 6,
},
{
name: "objective header manipulation test",
main: "../../examples/testing/objective_header/main.vcl",
passes: 9,
},
{
name: "multiple header manipulation test",
main: "../../examples/testing/multiple_header/main.vcl",
passes: 8,
},
}

for _, tt := range tests {
Expand All @@ -371,7 +382,7 @@ func TestTester(t *testing.T) {
VerboseWarning: true,
},
Testing: &config.TestConfig{
Filter: tt.filter,
Filter: "*.test.vcl",
},
Commands: config.Commands{"test", main},
}
Expand Down
0 examples/testing/assertion.test.vcl → ...ples/testing/assertion/assertion.test.vcl
View file
Open in desktop
File renamed without changes.
0 examples/testing/assertion.vcl → examples/testing/assertion/main.vcl
View file
Open in desktop
File renamed without changes.
0 examples/testing/default.test.vcl → examples/testing/default/default.test.vcl
View file
Open in desktop
File renamed without changes.
0 examples/testing/default.vcl → examples/testing/default/main.vcl
View file
Open in desktop
File renamed without changes.
0 examples/testing/default_values.test.vcl → ...ng/default_values/default_values.test.vcl
View file
Open in desktop
File renamed without changes.
0 examples/testing/default_values.vcl → examples/testing/default_values/main.vcl
View file
Open in desktop
File renamed without changes.
Empty file added examples/testing/multiple_header/main.vcl
View file
Open in desktop
Empty file.
49 changes: 49 additions & 0 deletions examples/testing/multiple_header/multiple_header.test.vcl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// see: https://github.com/ysugimoto/falco/issues/237

// @scope: recv
// @suite: ADD header(add-add-add) BUGGY
sub test_recv {
add req.http.VALUE = "V1";
add req.http.VALUE = "V2";
add req.http.VALUE = "V3";
assert.equal(req.http.VALUE, "V1"); # request upstream with 3 line headers

set req.http.MESSAGE = req.http.VALUE; # set first header value
assert.equal(req.http.MESSAGE, "V1");
}

// @scope: recv
// @suite: ADD header(set-add-add) BUGGY
sub test_recv {
set req.http.VALUE = "V1";
add req.http.VALUE = "V2";
add req.http.VALUE = "V3";
assert.equal(req.http.VALUE, "V1"); # request upstream with 3 headers

set req.http.MESSAGE = req.http.VALUE; # set first header value
assert.equal(req.http.MESSAGE, "V1");
}

// @scope: recv
// @suite: ADD header(add-add-set)
sub test_recv {
add req.http.VALUE = "V1";
add req.http.VALUE = "V2";
set req.http.VALUE = "V3";
assert.equal(req.http.VALUE, "V3"); # 1 header

set req.http.MESSAGE = req.http.VALUE;
assert.equal(req.http.MESSAGE, "V3");
}

// @scope: recv
// @suite: UNSET header(add-add-unset)
sub test_recv {
add req.http.VALUE = "V1";
add req.http.VALUE = "V2";
unset req.http.VALUE;
assert.is_notset(req.http.VALUE); # 0 header

set req.http.MESSAGE = req.http.VALUE;
assert.equal(req.http.MESSAGE, "(null)");
}
1 change: 1 addition & 0 deletions examples/testing/notset_string/main.vcl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
// empty VCL
34 changes: 34 additions & 0 deletions examples/testing/notset_string/notset_string.test.vcl
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Started looking through the PR this morning and the test assertions in this file stood out to me.

For example in this test:

// @scope: recv
// @suite: UNSET header(add-add-unset)
sub test_recv {
    set req.http.VALUE = req.http.NOTSET;
    assert.is_notset(req.http.VALUE);

    set req.http.MESSAGE = req.http.VALUE;
    assert.equal(req.http.MESSAGE, "(null)");
}

What seemed odd to me was the fact that assert.equal(req.http.MESSAGE, "(null)"); was passing when the header is unset. A condition like req.http.MESSAGE == "(null)" in an if statement for example should evaluate to false. So assert.equal should seemingly also reject this case.

Put together a fiddle that reproduces the sequence of sets from the test and inspecting the results.
https://fiddle.fastly.dev/fiddle/d1c67281

The log output of that fiddle is

req.http.VALUE
not set
logged value: (null)
req.http.MESSAGE
not set
logged value: (null)

Both the VALUE and MESSAGE headers are "not set" and don't equal "(null)" in the context of an if statement condition. But what I found interesting is that the logged value is "(null)". That fact started a whole other path of looking into the behavior of all this and I ran out of time to be able to get back to reviewing the PR.

I started putting together another fiddle with as many permutations of usage of unset header and local string variables I could think of to use as a basis for comparing the falco not set string behavior with what Fastly is doing. Will share that when I have some time to finish it and will get back to reading through more of the PR soon.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right - this looks strange to us but it simulates Fastly's behavior.
The NotSet string evaluation depends on the context, sometimes empty string, and sometimes "(null)", and unfortunately Fastly VCL developer also could not grab all cases that the NotSet string could evaluate as "(null)".

However, I'd understand the cases following:

  1. on log, synthetic, and synthetic_base64 statements, message string includes NotSet will evaluate as "(null)"
  2. on making actual HTTP headers that send to the origin, the header value string includes NotSet will evaluate as "(null)"
  3. on assigning into STRING local variable like var.SomeString will evaluate as empty string
  4. In other cases, as you pointed out if expression, NotSet string will be evaluated as falsy and any comparison operation should be treated like NULL.
  5. built-in function - some functions will be treated as "(null)" but nobody is figuring it out now.

Probably there are more cases where the NotSet string is evaluated as "(null)" hence this PR includes the features that I only understand.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't see this when writing up the comment with the test case fiddle/tests yesterday. I had missed the synthetic statements when thinking through various places where not set values would need to be considered.

You're right synthetic does behave like the log statement and always expands not set values to (null).

I'm not seeing the (null) behavior with synthetic.base64 though. The response body in a fiddle with synthetic.base64 req.http.unset seems to be empty. Likewise doing a string concat expression with that statement synthetic.base64 req.http.unset + "aGkK" results in the body hi without the (null) expansion for the unset header.

Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// see: https://github.com/ysugimoto/falco/issues/235

// @scope: recv
// @suite: UNSET header
sub test_recv {
set req.http.VALUE = "V";
unset req.http.VALUE;
assert.is_notset(req.http.VALUE);

set req.http.MESSAGE = req.http.VALUE;
assert.equal(req.http.MESSAGE, "(null)");
}

// @scope: recv
// @suite: EMPTY header
sub test_recv {
set req.http.VALUE = "V";
set req.http.VALUE = "";
assert.equal(req.http.VALUE, "");

set req.http.MESSAGE = req.http.VALUE;
assert.equal(req.http.MESSAGE, "");
}

// @scope: recv
// @suite: NOTSET header
sub test_recv {
set req.http.VALUE = "V";
set req.http.VALUE = req.http.NOTSET;
assert.is_notset(req.http.VALUE);

set req.http.MESSAGE = req.http.VALUE;
assert.equal(req.http.MESSAGE, "(null)");
}
1 change: 1 addition & 0 deletions examples/testing/objective_header/main.vcl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
// empty VCL
79 changes: 79 additions & 0 deletions examples/testing/objective_header/objective_header.test.vcl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
// see: https://github.com/ysugimoto/falco/issues/236

// @scope: recv
// @suite: SET VARS VALUE
sub test_recv {
set req.http.VARS = "";
set req.http.VARS:VALUE = "V";
assert.equal(req.http.VARS, "VALUE=V");
}

// @scope: recv
// @suite: SET NOT-INITIALIZED VARS VALUE
sub test_recv {
set req.http.VARS:VALUE = "V";
assert.equal(req.http.VARS, "VALUE=V");
}

// @scope: recv
// @suite: SET MULTIPLE VARS VALUE
sub test_recv {
set req.http.VARS = "";
set req.http.VARS:VALUE = "V";
set req.http.VARS:VALUE2 = "V2";
assert.equal(req.http.VARS, "VALUE=V, VALUE2=V2");
}

// @scope: recv
// @suite: SET EMPTY VARS VALUE
sub test_recv {
set req.http.VARS = "";
set req.http.VARS:VALUE = "";
assert.equal(req.http.VARS, "VALUE");
}

// @scope: recv
// @suite: SET MULTIPLE EMPTY VARS VALUE
sub test_recv {
set req.http.VARS = "";
set req.http.VARS:VALUE = "";
set req.http.VARS:VALUE2 = "";
assert.equal(req.http.VARS, "VALUE, VALUE2");
}

// @scope: recv
// @suite: UNSET VARS ALL VALUE
sub test_recv {
set req.http.VARS = "";
set req.http.VARS:VALUE = "V";
unset req.http.VARS:VALUE;
assert.is_notset(req.http.VARS);
}

// @scope: recv
// @suite: UNSET VARS VALUE
sub test_recv {
set req.http.VARS = "";
set req.http.VARS:VALUE = "V";
set req.http.VARS:VALUE2 = "V2";
unset req.http.VARS:VALUE;
assert.equal(req.http.VARS, "VALUE2=V2");
}

// @scope: recv
// @suite: OVERRIDE VARS VALUE
sub test_recv {
set req.http.VARS = "";
set req.http.VARS:VALUE = "V";
set req.http.VARS:VALUE = "O";
assert.equal(req.http.VARS, "VALUE=O");
}

// @scope: recv
// @suite: SET NULL VALUE
sub test_recv {
set req.http.VARS = "";
set req.http.VARS:VALUE = "V";
set req.http.VARS:VALUE = req.http.NULL;
assert.equal(req.http.VARS, "VALUE");
}
0 examples/testing/table_manipulation.vcl → examples/testing/table_manipulation/main.vcl
View file
Open in desktop
File renamed without changes.
0 examples/testing/table_manipulation.test.vcl → ..._manipulation/table_manipulation.test.vcl
View file
Open in desktop
File renamed without changes.
Loading
Loading