HTTP/2 implementation: HTTP/2-cache (#309). #1383

aleksostapenko · 2020-02-18T18:03:34Z

No description provided.

nmmmnu · 2020-02-18T18:11:38Z

tempesta_fw/http.h

+static inline unsigned short
+tfw_h2_pseudo_index(unsigned short status)
+{
+	switch (status) {


Wouldn't this be faster wit array?
Of course will use minimum 512 bytes.

The compiler will do binary search over the labels which is much faster than accessing 4 cache line array. The compiler also generates lookup table (exactly the array you proposed), but for more localized values.

krizhanovsky

I did only quick and highlevel review, so I left only several comments and can not say whether it's LGTM or not. A more solid review from @ikoveshnikov is required.

krizhanovsky · 2020-02-20T17:12:47Z

tempesta_fw/cache.c

+		r = write_actor(db, trec, resp, p, s->len, &dc_iter);
+		if (likely(!r))
+			*acc_len += dc_iter.acc_len;
+		goto out;


Useless goto instead of simple return r

krizhanovsky · 2020-02-20T17:24:42Z

tempesta_fw/cache.c

+		 * version of this header.
+		 */
+		if ((field->flags & TFW_STR_HBH_HDR)
+		    || hid == TFW_HTTP_HDR_SERVER


Shouldn't we skip Via as well if the response already contains the header?

We must preserve all the already existing Via headers in the message while forwarding it (https://tools.ietf.org/html/rfc7230#section-5.7.1). The resulting order of the Via headers remains correct, as we add our Via header in the end - after all the response headers are already copied.

krizhanovsky · 2020-02-20T17:35:25Z

tempesta_fw/http.c

+	TfwH2TransOp op = cache ? TFW_H2_TRANS_EXPAND : TFW_H2_TRANS_ADD;
+
+	if (!h_mods)
+		return 0;


Double condition check

krizhanovsky · 2020-02-20T17:36:38Z

tempesta_fw/cache.c

@@ -1529,44 +1978,33 @@ tfw_cache_build_resp_body(TDB *db, TfwHttpResp *resp, TdbVRec *trec,
 * TODO use iterator and passed skbs to be called from net_tx_action.
 */
 static TfwHttpResp *
-tfw_cache_build_resp(TfwHttpReq *req, TfwCacheEntry *ce)
+tfw_cache_h2_build_resp(TfwHttpReq *req, TfwCacheEntry *ce, time_t lifetime,


The function builds HTTP/1 responses as well as HTTP/2, so why to rename it?

Corrected (reverted names).

krizhanovsky · 2020-02-20T17:38:51Z

tempesta_fw/cache.c

+
+static inline int
+tfw_cache_h2_set_status(TDB *db, TfwCacheEntry *ce, TfwHttpResp *resp,
+			TdbVRec **trec,	char **p, unsigned long *acc_len)


The function works for both HTTP versions, so I propose not to use h2 in names of such functions.

krizhanovsky · 2020-02-20T18:16:08Z

tempesta_fw/cache.c

+		if (st_index) {
+			if (tfw_cache_h2_copy_int(&ce->hdr_len, st_index, 0xF,
+						  p, trec, tot_len))
+				return -ENOMEM;


That's cool: now we store all static-table-indexed headers by indexes only in the cache!

It seems if we send a response from the cache, we don't need and should skip the while (!hdrs_end) loop calling tfw_hpack_encode() for all the headers in tfw_h2_resp_adjust_fwd().

Yes - it is exactly how it works now: the response from cache is served through tfw_http_req_cache_service() -> tfw_h2_resp_fwd() flow, bypassing the tfw_h2_resp_adjust_fwd() procedure.

krizhanovsky · 2020-02-20T22:42:49Z

tempesta_fw/http_msg.c

+ * likely - aggregated into one general-purpose procedure, after HTTP/1.1-parser
+ * extending (see also TODO-comment for @tfw_http_hdr_split_cp() procedure); for
+ * now this procedure leaves RWS in the resulting @val and this is not correct
+ * in context of HTTP/2<=>HTTP/1.1 comparisons, transformations etc.


Just leave a comment to remember that the TODO must be fixed in upcoming PR.

TODO is fixed.

krizhanovsky · 2020-02-20T22:43:06Z

tempesta_fw/http.c

+ * TODO: this procedure should be evicted after HTTP/1.1-parser extending
+ * (and @tfw_http_hdr_split()/@tfw_h2_msg_hdr_length procedures upgrading) in
+ * order to split headers into separate chunks for name, ':', LWS, value, and
+ * RWS - during the HTTP-message parsing stage.


Just leave a comment to remember that the TODO must be fixed in upcoming PR.

TODO is fixed.

Parse name, colon, LWS, value and RWS of HTTP/1.1-response headers into separate chunks to facilitate the name/value splitting and colon/OWS eviction during HTTP/1.1=>HTTP/2 response transformation.

krizhanovsky

LGTM and needs an accurate review from @ikoveshnikov

krizhanovsky · 2020-02-27T19:45:22Z

tempesta_fw/http.c

-			if (unlikely(!idx))
-				break;
-			--idx;
-		}


The function is used instead of tfw_h2_msg_hdr_length() (e.g. in __set_etag()) and now the loop body w/o this while looks much better and acceptable.

Yeah - there is no need to iterate characters inside the chunks any more.

…309).

vankoven · 2020-03-01T14:51:53Z

tempesta_fw/http.h

@@ -302,6 +300,9 @@ enum {
 	((!hmmsg->conn || TFW_CONN_TYPE(hmmsg->conn) & Conn_Srv) &&	\
 	 hmmsg->pair && TFW_MSG_H2(hmmsg->pair))

+#define H2_STAT_VAL_LEN			3


Either move the definition close to existing macros in the beginning of the http_msg.c or add a comment here. http.h is too overloaded and messy, additions of a new undocumented macro definitions is not a good choice.

Done: moved to http_msg.h with comment.

vankoven · 2020-03-01T15:14:34Z

tempesta_fw/http.h

@@ -302,6 +300,9 @@ enum {
 	((!hmmsg->conn || TFW_CONN_TYPE(hmmsg->conn) & Conn_Srv) &&	\
 	 hmmsg->pair && TFW_MSG_H2(hmmsg->pair))

+#define H2_STAT_VAL_LEN			3
+#define RESP_BUF_LEN			128


The macro defines size of buffer for some temporal operations, it's not related to http itself and it's better to define it right above buffer definitions (which are static and never exposed outside their translation unit).

vankoven · 2020-03-01T15:34:48Z

tempesta_fw/http_sess.c

+		struct sk_buff **skb_head = &resp->msg.skb_head;
+		TfwStr crlf = { .data = S_CRLF, .len = SLEN(S_CRLF) };
+
+		r = tfw_http_msg_expand_data(&mit->iter, skb_head,


Hm, probably we can avoid skb expand operations during serving from cache. We allocate skb big enough to fit all headers stored in cache, but we can add some extra space to put a set-cookie header: we know everything about it: header name size, cookie name, and length of Tempesta sticky cookie. If you're agree, just drop a TODO here.

The problem here is that along with copying headers from cache and addition the known in advance headers (e.g. set-cookie, date etc.) we must also process configured headers corrections (which can be addition/substitution/deletion/appending) and we cannot predict here the resulting size of memory for the skbs to be allocated in the target response.

vankoven · 2020-03-01T16:48:07Z

tempesta_fw/cache.c

-		resp = NULL;
-		goto out;
+	if (TFW_MSG_H2(req)) {
+		id = tfw_h2_stream_id(req);


tfw_h2_stream_id() gets stream id under h2 context lock. Why? The request can't be reassigned to other stream and stream can't be destroyed until request is alive, aren't they?

stream can't be destroyed until request is alive

Actually can: during processing of request on http layer, the framing layer can receive the RST_STREAM from the client and stream will be moved to the final closed state (and consequently - will be removed).

Ok, i will check that, but it seems to be resource management question then. Stream can be removed, but stream object must exist until referencing request exists. Same as http1.1 requests keep connection object alive.

Anyway. no need to change this right now, i was curious for locking reason.

…309).

vankoven

Looks good to me. Great job!

vankoven · 2020-03-02T13:52:02Z

tempesta_fw/cache.c

+		st_index = hdr->hpack_idx;
+		h_len = tfw_h2_hdr_size(s_nm.len, s_val.len, st_index);
+
+		/* Don't split short stprings. */


Suggested change

/* Don't split short stprings. */

/* Don't split short strings. */

vankoven · 2020-03-02T13:56:13Z

tempesta_fw/cache.c

+		 * version of this header.
+		 */
+		if ((field->flags & TFW_STR_HBH_HDR)
+		    || hid == TFW_HTTP_HDR_SERVER


Let's skip Set-Cookie header. Actually backend should protect responses with private cookies from caching, but I'll be good not to spread the same cookie among all clients if it mistakenly leaks. What do you think?

As a result of discussion in the chat, it was decided to postpone this change and additionally investigate it.

Made some research and moved the requirement to #1075 which is all about cookies and security.

…om cache (#309).

HTTP/2 Parser implementation: HTTP/2-cache (#309).

c721aaa

aleksostapenko changed the title ~~HTTP/2 Parser implementation: HTTP/2-cache (#309).~~ HTTP/2 implementation: HTTP/2-cache (#309). Feb 18, 2020

aleksostapenko added the ready for review label Feb 18, 2020

nmmmnu reviewed Feb 18, 2020

View reviewed changes

krizhanovsky requested review from krizhanovsky and vankoven February 20, 2020 14:55

krizhanovsky assigned krizhanovsky and vankoven Feb 20, 2020

krizhanovsky reviewed Feb 20, 2020

View reviewed changes

HTTP/2: changes in processing of headers strings (#309).

4dec0e8

Parse name, colon, LWS, value and RWS of HTTP/1.1-response headers into separate chunks to facilitate the name/value splitting and colon/OWS eviction during HTTP/1.1=>HTTP/2 response transformation.

krizhanovsky approved these changes Feb 27, 2020

View reviewed changes

HTTP/2: fix duplicate headers processing during response forwarding (#…

2d7bb75

…309).

vankoven reviewed Mar 1, 2020

View reviewed changes

HTTP/2: add header name tracking into the headers' compare procedure (#…

b5e71ac

…309).

vankoven approved these changes Mar 2, 2020

View reviewed changes

HTTP/2: corrections according review comments (#309).

4f0cdd1

vankoven reviewed Mar 2, 2020

View reviewed changes

aleksostapenko added 2 commits March 4, 2020 10:21

HTTP/2: evict redundant space during response status-line creation fr…

4f43032

…om cache (#309).

HTTP/2: corrections according additional review comments (#309).

6164968

aleksostapenko merged commit 7b4fc72 into ao-309-parser-transformation Mar 4, 2020

aleksostapenko deleted the ao-309-parser-transformation-h2-cache branch March 4, 2020 11:05

vankoven mentioned this pull request Mar 4, 2020

Hardening the sticky cookie module #1075

Open

	/* Don't split short stprings. */
	/* Don't split short strings. */

HTTP/2 implementation: HTTP/2-cache (#309). #1383

HTTP/2 implementation: HTTP/2-cache (#309). #1383

Conversation

aleksostapenko commented Feb 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krizhanovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krizhanovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksostapenko Mar 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vankoven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksostapenko Mar 2, 2020 •

edited

Loading