Support NotSet string behavior in falco #248
Conversation
…n-for-header-value
|
Definitely a lot to look through, will start reviewing the changes in the next day or two when I have a bigger block of free time. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- on
log,synthetic, andsynthetic_base64statements, message string includes NotSet will evaluate as "(null)" - on making actual HTTP headers that send to the origin, the header value string includes NotSet will evaluate as "(null)"
- on assigning into
STRINGlocal variable likevar.SomeStringwill evaluate asempty string - In other cases, as you pointed out if expression,
NotSetstring will be evaluated asfalsyand any comparison operation should be treated like NULL. - 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.
There was a problem hiding this comment.
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.
|
Put together this fiddle / falco test file to evaluate as many not set conditions I could come up with in a reasonable amount of time. https://fiddle.fastly.dev/fiddle/3f8ea76a Running these tests against this PR branch shows a few discrepancies from what occurs in Fastly. It seems the I have not done an exhaustive set of testing of the built-in functions that accept strings but the ones I have tested all seem to handle not set STRING/IP values a empty strings. Back to the code review aspect of this I have some reservations about the LenientString solution that I will try and write up tonight or tomorrow morning. Not having as much time to work on this as I typically would so please bear with me and I'll get through the review as soon as I can. |
|
The Multiple valuesSince the only use case for the multiple values is the
|
interpreter/http/director.go
Outdated
| } | ||
|
|
||
| for retry := 0; retry < maxRetry; retry++ { | ||
| // Check backens are enough healthy to determine |
| ) | ||
| } | ||
| if v, ok := prop.Value.(*ast.Ident); !ok { | ||
| return nil, exception.Runtime(&prop.GetMeta().Token, ".key value must be integer") |
There was a problem hiding this comment.
Typo integer should be ident.
interpreter/http/director.go
Outdated
| } | ||
| } | ||
|
|
||
| rand.New(rand.NewSource(time.Now().Unix())) |
There was a problem hiding this comment.
The generator created by this isn't stored, but for this use case a local generator isn't really needed so this line can just be removed.
There was a problem hiding this comment.
Should be stored to variable and call r.Intn to be sure, thanks.
There was a problem hiding this comment.
Ah you're right, we should remove this line to correct choosing backend by weight.
interpreter/transport/transport.go
Outdated
| "github.com/ysugimoto/falco/interpreter/limitations" | ||
| ) | ||
|
|
||
| // Send proxy request to spefic origin |
interpreter/transport/backend.go
Outdated
| if v, ok := prop.(T); ok { | ||
| return v, nil | ||
| } | ||
| return nil, errors.New("Type assertion failed") |
There was a problem hiding this comment.
While the linter will catch an invalid property value type it would be good to give the user a more helpful error message here so they know what backend property is faulty in the test and simulator output.
There was a problem hiding this comment.
T is just generics type info so now I improved the message that the user needs to set the correct type for the property name of key.
| func backendHost(backend *value.Backend, override *config.OverrideBackend) (string, error) { | ||
| // host may be overrided by config | ||
| if override != nil { | ||
| return override.Host, nil |
There was a problem hiding this comment.
Shouldn't this have a check for override.Host not being an empty string?
There was a problem hiding this comment.
Yes, but I think it should be validated on config initialization. I'll do it on another issue.
|
|
||
| func backendScheme(backend *value.Backend, override *config.OverrideBackend) (string, error) { | ||
| // scheme may be overrided by config | ||
| if override != nil { |
There was a problem hiding this comment.
Since this pair of if statements are logically one thing it could be simplified into a single condition. override != nil && override.SSL. There are a couple similar pairings in this file as well, lines 56/57 and 131/132.
Largely a style preference nit though.
There was a problem hiding this comment.
Yes, but we could make it clear the override.SSL would be compared with a bool type.
|
@richardmarshall richardmarshall@3212a48 is a great approach to consider the (null) string! could you send me a PR for this branch of |
Sure will do, but probably won't have time for a few days. |
|
I got started on rebasing my experimental changes onto this branch when I got derailed by discovering what seems to be a bug in Fastly's not_set handling for header fields. Realized I hadn't tested how header fields behave when assigned not_set values or are included in concatenation, etc. Through that I found that the length of the field name has an impact on the behavior. set req.http.test1 = req.http.unset; // test1 header is a not_set string
set req.http.test2:a = req.http.unset; // field a of the test2 header is a not_set string
set req.http.test3:ab = req.http.unset; // field ab of the test3 header is an empty stringHeader fields with a key length of 1 behave differently than keys with length > 1 when the header is unset. Which doesn't make any logical sense so seems like a bug. Fiddle showing this odd behavior: Anyway will get back to the rebase work soon. |
|
Ended up running into another issue that opened up a whole new problem area for our compatibility with Fastly's header field implementation. Currently in our implementation setting the main header value to an encoded set of fields like Fastly parses the full set of fields and makes them accessible. I initially though supporting this seemed like it would be doable with some tweaks to the implementation in this PR until I found another weird case. set req.http.foo = "a=1,b=2";
log req.http.foo:a; // `1` as expected
log req.http.foo:b; // `2` as expected
set req.http.foo:c = ",d=3";
log req.http.foo:c; // `,d=3` as expected
log req.http.foo:d; // `3"` unexpectedSetting a field value that contains a field separator At this point I started putting together a set of validation tests to figure out as much about how fastly is dealing with these header fields. Found that the RFC-8941 specification for structured header values contained a number of test cases and used that as a starting point. RFC - https://www.rfc-editor.org/rfc/rfc8941.html I don't have a fiddle prepared confirming the Fastly results of all the tests but I can put that together soon. Results for main and this PRs branch: main: This PR: Took a quick stab at trying to handle field access in a way that didn't rely on storing the fields independently from the main header value. Started with an approach based on simple string splits on the field separator and was able to significantly improve the compatibility test results. But ran into challenges with properly dealing with field values that contain a The regex pattern is hard to interpret and could likely be optimized for clarity but it handles the idiosyncrasies of the Fastly behavior. main...richardmarshall:falco:header_field_experiment Additionally I did open a PR against this branch for my not_set handling #265. |
|
Oh really, I also did not know such behavior in dealing with comma-separated header values. |
Yeah it was quite a surprise when I first saw it. Further experimentation found a number of additional unexpected or currently unaccounted for behaviors. I believe all of the ones I found are handled properly in the proof of concept implementation in #267
Don't follow, what would be confusing? |
Fixes #235 #236 #237
I apologize this PR includes massive changes.
This PR implements
NotSetstring support which is described here. To support this behavior, I had to make fundamental changes to HTTP header manipulation:Implement original HTTP-related struct
To simulate Fastly HTTP-related behavior conveniently, I implemented the following structs originally:
http.Requesthttp.Responsehttp.HeaderThese are equivalent to Golang's
net/httppackage and can convert each other through the utility function.You can see the above structs as
flchttpaliased import.Particularly,
http.Headerimplements Fastly header manipulation feature, colon-separated syntax, multiple header behavior, and adding(null)string to the actual HTTP request when NotSet string is assigned.Implement more string value type
I implemented
LenientString, which keeps assigned/concatenated values inside and can support to output(null)string if the value is entirely NotSet.According to declare the new value type, I needed to change the entire expression evaluation stuff, assign, operator, built-in function, etc.
After this implementation, we need to consider whether the string type is either
*value.Stringor*value.LenientiString, then we can usevalue.GetString(value.Value)as wrap them transparently.Implement new transport package
This is not related to NotSet-related things but the actual origin requests feature handles this package.
This package includes creating Golang's
http.Requestpointer fromflchttp.Request, making director, and so on.Please look at the implementation.
This change should bump the major version I think.
Again, sorry for the huge changes, please take a look at the changesets with having the above perspective and perhaps you have unclear points, then feel free to put comments to diff, and let's discuss it, thanks.