[jira] [Commented] (TS-4092) Decouple HPACK from HTTP/2

["ASF GitHub Bot (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/TS-4092?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15205290#comment-15205290 ] 

ASF GitHub Bot commented on TS-4092:
------------------------------------

Github user bryancall commented on a diff in the pull request:

    https://github.com/apache/trafficserver/pull/460#discussion_r56908946
  
    --- Diff: proxy/http2/HPACK.cc ---
    @@ -828,9 +828,111 @@ update_dynamic_table_size(const uint8_t *buf_start, const uint8_t *buf_end, Http
       if (len == HPACK_ERROR_COMPRESSION_ERROR)
         return HPACK_ERROR_COMPRESSION_ERROR;
     
    -  if (indexing_table.set_dynamic_table_size(size) == false) {
    +  if (indexing_table.update_maximum_size(size) == false) {
         return HPACK_ERROR_COMPRESSION_ERROR;
       }
     
       return len;
     }
    +
    +int64_t
    +hpack_decode_header_block(HpackIndexingTable &indexing_table, HTTPHdr *hdr, const uint8_t *in_buf, const size_t in_buf_len)
    +{
    +  const uint8_t *cursor = in_buf;
    +  const uint8_t *const in_buf_end = in_buf + in_buf_len;
    +  HdrHeap *heap = hdr->m_heap;
    +  HTTPHdrImpl *hh = hdr->m_http;
    +  bool header_field_started = false;
    +  bool has_http2_violation = false;
    +
    +  while (cursor < in_buf_end) {
    +    int64_t read_bytes = 0;
    +
    +    // decode a header field encoded by HPACK
    +    MIMEField *field = mime_field_create(heap, hh->m_fields_impl);
    +    MIMEFieldWrapper header(field, heap, hh->m_fields_impl);
    +    HpackFieldType ftype = hpack_parse_field_type(*cursor);
    +
    +    switch (ftype) {
    +    case HPACK_FIELD_INDEX:
    +      read_bytes = decode_indexed_header_field(header, cursor, in_buf_end, indexing_table);
    +      if (read_bytes == HPACK_ERROR_COMPRESSION_ERROR) {
    +        return HPACK_ERROR_COMPRESSION_ERROR;
    +      }
    +      cursor += read_bytes;
    +      header_field_started = true;
    +      break;
    +    case HPACK_FIELD_INDEXED_LITERAL:
    +    case HPACK_FIELD_NOINDEX_LITERAL:
    +    case HPACK_FIELD_NEVERINDEX_LITERAL:
    +      read_bytes = decode_literal_header_field(header, cursor, in_buf_end, indexing_table);
    +      if (read_bytes == HPACK_ERROR_COMPRESSION_ERROR) {
    +        return HPACK_ERROR_COMPRESSION_ERROR;
    +      }
    +      if (read_bytes < 0) {
    +        has_http2_violation = true;
    +        read_bytes = -read_bytes;
    +      }
    +      cursor += read_bytes;
    +      header_field_started = true;
    +      break;
    +    case HPACK_FIELD_TABLESIZE_UPDATE:
    +      if (header_field_started) {
    +        return HPACK_ERROR_COMPRESSION_ERROR;
    +      }
    +      read_bytes = update_dynamic_table_size(cursor, in_buf_end, indexing_table);
    +      if (read_bytes == HPACK_ERROR_COMPRESSION_ERROR) {
    +        return HPACK_ERROR_COMPRESSION_ERROR;
    +      }
    +      cursor += read_bytes;
    +      continue;
    +    }
    +    // Store to HdrHeap
    +    mime_hdr_field_attach(hh->m_fields_impl, field, 1, NULL);
    +  }
    +  // Parsing all headers is done
    +  if (has_http2_violation) {
    +    return -(cursor - in_buf);
    +  } else {
    +    return cursor - in_buf;
    +  }
    +}
    +
    +int64_t
    +hpack_encode_header_block(HpackIndexingTable &indexing_table, uint8_t *out_buf, const size_t out_buf_len, HTTPHdr *hdr)
    +{
    +  uint8_t *cursor = out_buf;
    +  const uint8_t *const out_buf_end = out_buf + out_buf_len;
    +  int64_t written;
    +
    +  ink_assert(http_hdr_type_get(hdr->m_http) != HTTP_TYPE_UNKNOWN);
    +
    +  MIMEFieldIter field_iter;
    +  for (MIMEField *field = hdr->iter_get_first(&field_iter); field != NULL; field = hdr->iter_get_next(&field_iter)) {
    +    HpackFieldType field_type;
    +    MIMEFieldWrapper header(field, hdr->m_heap, hdr->m_http->m_fields_impl);
    +    if (header.is_sensitive()) {
    --- End diff --
    
    Can't we just do the Cookie and Authentication header check here instead of adding another mime filed flag?  I would rather have HPACK know about Cookie and Authentication headers instead of adding the sensitive flag to MIME.


> Decouple HPACK from HTTP/2
> --------------------------
>
>                 Key: TS-4092
>                 URL: https://issues.apache.org/jira/browse/TS-4092
>             Project: Traffic Server
>          Issue Type: Improvement
>          Components: HTTP/2
>            Reporter: Masakazu Kitajo
>            Assignee: Masakazu Kitajo
>             Fix For: 6.2.0
>
>
> Our HTTP/2 implementation and HPACK implementation are coupled tightly. This is bad. It makes things complicated.
> I tried to write a test runner which uses [hpack-test-case |https://github.com/http2jp/hpack-test-case], however, I had to call functions in HTTP2.cc. Because HPACK.cc has only primitive encoder and decoder, and doesn't handle header blocks. HTTP2.cc covers not only RFC7540 but also some part of RFC7541.
> On the other hand, HPACK.h exports pseudo header names as constants. They should be in HTTP2.h or MIME.h as WKS. We don't need them in HPACK implementation.
> Also, HPACK is used with QUIC (at least in current draft). We should decouple HPACK from HTTP/2 so that we can use the module with QUIC in the future.
> Once we have done this, we can write tests for these improvements more easily.
> TS-4002, TS-4061, TS-4014 and TS-3478



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)