-
Notifications
You must be signed in to change notification settings - Fork 377
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
Varnisncsa: Change matching rules to reflect reality #4213
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be better organized.
b6c21ef
to
cbc5843
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can improve this change a bit more if we actually follow the proposal for new matching rules more closely.
cbc5843
to
4d66ff0
Compare
Discussed this offline with @dridi. Force pushed a (hopefully) much cleaner version now. All review items should be addressed/obsolete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pass my vote on to @dridi . If he is happy with it now, I am too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something went terribly wrong with this new iteration. I probably didn't express myself correctly so I tried to be a lot more specific in this review.
4d66ff0
to
5f6107c
Compare
This is needed in order to differentiate between what was sent/received to/from the client/server and the changes made to [be]req/resp in VCL. It should be noted that this is the best approximation we can get as of today, as there is no reliable vsl tag to tell exactly when we have finished receiving a req/beresp, therefore we have to use vcl_recv and vcl_backend_response for now.
With this change, [Be]resp/[Be]req formats should reflect better what was received/sent from/to the backend/client without taking irrelevant VCL changes into account. There is however still an exception for the changes performed by the core code before vcl_recv for req and before vcl_backend_response for beresp that cannot be distinguished from the original headers. Refs: varnishcache#3528
5f6107c
to
7342d70
Compare
Sorry for the mix-up, I have updated the PR now, let me know if it's still missing something. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One polish suggestion.
7342d70
to
de4b66c
Compare
This PR introduces the changes discussed in #3528
The relevant parts of [BE]requests/responses are now shown as they were received/sent from/to the peer, without taking irrelevant VCL changes into account. This PR also introduces the handling of Unset headers in varnishncsa.
Note: this breaks the default behavior of varnishncsa.
Fixes #3528.