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.

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

Fixes on serving requests with errors #1141

Merged
merged 7 commits into from
Jan 24, 2019
Merged

Fixes on serving requests with errors #1141

merged 7 commits into from
Jan 24, 2019

Conversation

vankoven
Copy link
Contributor

Requires #1140 and built upon it. Required for #1043

Changes:

  • If error has happened during request processing, don't close client connection immediately, send all pending requests first. Don't silently drop request to avoid breaking of the request-response sequence of client. Drop client connections on response blocking #962
  • Replace TFW_HTTP_SUSPECTED flag with TFW_HTTP_CONN_CLOSE. Later is more generic and Connection: is not set correctly with TFW_HTTP_SUSPECTED flag
  • Check sticky session cookie before forward request to cache. Don't allow unauthorised client to stress cache and receive protected information. [Cache] Unathorized clients should be blocked before accessing cache #899

vankoven added a commit to tempesta-tech/tempesta-test that referenced this pull request Dec 22, 2018
…osed

In tempesta-tech/tempesta#1141 connection is correctly closed and
header connection is set to 'Connection: close'
@vankoven
Copy link
Contributor Author

Tests must be patched to pass the tests. Build 361 on CI passes for this branch and tempesta-tech/tempesta-test#70

Copy link
Contributor

@krizhanovsky krizhanovsky left a comment

Choose a reason for hiding this comment

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

I have only small objections and comments, but the change significantly affects code written by @aleksostapenko , so his review is required.

struct list_head *prev;

if (list_empty_careful(&cli_conn->seq_queue))
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

We add a new items to the queue only on a CPU handling the cli_conn, i.e. local CPU, so it seems safe against list_add(). But is list_empty_careful() safe against __list_cut_position() called from tfw_http_resp_fwd()?

req->location = tfw_location_match(req->vhost,
&req->uri_path);
else if (block) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If HTTPtables didn't return a vhost, why they can't block the request, i.e. why the else appeared?

It'd be better to use curly braces for the if hading else if with curly braces.

else if (block) {
TFW_INC_STAT_BH(clnt.msgs_filtout);
tfw_http_req_parse_block(
req, 403,
Copy link
Contributor

Choose a reason for hiding this comment

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

You can leave req, 403 on the same line with tfw_http_req_parse_block(. The same for the similar call on the above and below - there is no need to introduce additional line breaks

}

/**
* Unintentional error happen during request processing. Caller function is
Copy link
Contributor

Choose a reason for hiding this comment

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

Comments here (and below - for tfw_http_req_block) are somewhat misleading, since these two functions are used only in response processing paths currently. So, maybe request processing -> request or response processing would be better here.


spin_lock(&cli_conn->seq_qlock);

prev = (!list_empty(&req->msg.seq_list)) ? req->msg.seq_list.prev
Copy link
Contributor

Choose a reason for hiding this comment

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

This is rather non-trivial place. Since this code may be called from different request/response processing paths and considering specific purpose of this function - I would add a comment to describe the cases in which we can get list_empty(&req->msg.seq_list) here (as well as list_empty_careful(&cli_conn->seq_queue)); for now I see two cases: 1. Error during request parsing on alive client connection; 2. Error during response processing for request, which belongs to already closed client connection.
Also, as this code is not intended for requests which already removed from seq_queue (for response sending) - maybe it is worth to add warning into the the function:

WARN_ON(req->resp && test_bit(TFW_HTTP_RESP_READY, req->resp->flags));

@vankoven vankoven changed the base branch from ik-bitops to master January 17, 2019 12:44
Copy link
Contributor

@krizhanovsky krizhanovsky left a comment

Choose a reason for hiding this comment

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

Good to merge. Just please make couple cleanups and comments fixes, including proposed by @aleksostapenko

* Error was happened and request should be dropped or blocked,
* but other modules (e.g. sticky cookie module) may have a response
* prepared for this request. A new error response is to be generated
* for the request, drop any previous request paired with the request.
Copy link
Contributor

Choose a reason for hiding this comment

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

-> "...any previous response paired with..."

}
/*
* If !on_req_recv_event, then the request @req - some
* random request from the seq_queue, not the last one.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't fully understand the statement: if we have only one request from a client, then the request will the only one and the last one. Why do we care here whether the request is last one or there are some other requests from the client?

TFW_INC_STAT_BH(clnt.msgs_otherr);
tfw_http_req_parse_block(req, 500,
"request dropped: internal error in Sticky "
"module");
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for the line break, you can place "module" on previous line

Instead of distinct flag, TFW_HTTP_CONN_CLOSE flags is set
to close client connection. The reason for a change
is message adjusting on forwarding. The 'Connection: ' header
should be properly set.
ss_close_sync() was replaced by tfw_connection_close() in current master.
@vankoven vankoven merged commit fe8e37d into master Jan 24, 2019
@vankoven vankoven deleted the ik-error-req-fixes branch January 24, 2019 13:12
vankoven added a commit to tempesta-tech/tempesta-test that referenced this pull request Jan 24, 2019
…osed

In tempesta-tech/tempesta#1141 connection is correctly closed and
header connection is set to 'Connection: close'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants