Would this cause a behavioral change for proxied 101 responses? Also I don't think 304 responses can have a body. I wonder if it's worth refactoring the status code logic in the HTTP/1.1 codec to have a shared utility function.

Either way I think it's worth runtime guarding this feature as it affects the data plane.

thanks for the review @alyssawilk . i've added new config option which enables this new behavior (i'm bad with naming though, so more than ready to change it if needed). as for "refactoring the status code logic" could you please elaborate on this? are you talking about some kind of utility like hasContent(status_code) bool which checks internally is it "204/304/something else or not"?

Is this function needed? Isn't end_stream in encodeHeaders() an indication of a header-only response?

Yep, that's exactly what I meant with regards to a utility, sorry if that was not clear. end_stream may also work as a first pass, though I'm not sure if for HTTP/1 it's legal to send a header frame and separate end stream bit for responses without a body.

with regards to guarding, I think rather than having permanent config it'd be fine to just runtime guard - there are instructions in CONTRIBUTING.md What do you think?

sure. will rework w/ runtime guard.

This can be abbreviated to just proto_config.response_direction_config().always_compress_content_if_exists(), since proto has matching default semantics.

This is a response-specific message. Can this field be applicable also to requests?

i can add it there as well. sure

Is this function needed? Isn't end_stream in encodeHeaders() an indication of a header-only response?

This runtime flag is enabled by default which changes the result of this function for existing installations working with HTTP/1.1. Not sure this is a good idea.

If we aim to fix H2 streaming then a better approach would be to narrow the Transfer-Encoding check below to apply to HTTP/1.1 only IMHO. Transfer-Encoding was introduced in HTTP/1.1 and its chunked value was dropped in HTTP2. WDYT?

i will rework so that default behavior wont be changed and if set to false, it would ignore transfer-encoding chunked and return true unconditionally. how thats sounds @rojkov ?

Is this defined anywhere?

forget to remove. will do

Does it have to part of the permanent config? I think @alyssawilk suggested moving it to the https://github.com/envoyproxy/envoy/blob/master/source/common/runtime/runtime_features.cc

mb i missread her, but my understanding was that it should be protected by RuntimeFlag. as defined here, and by @rojkov comment - it should be disabled by default (so no need to add it into https://github.com/envoyproxy/envoy/blob/master/source/common/runtime/runtime_features.cc#L54). could you folks (@rojkov @alyssawilk) shed a light here please?

My understanding is that runtime guards are defined here https://github.com/envoyproxy/envoy/blob/master/source/common/runtime/runtime_features.cc I do not know use cases for defining them in the config proto.

Nit: It is a bit weird to use the :method header in response headers, I guess this is was copied from other tests. If it is not too much trouble can it be changes to :status please? Just to reduce confusion a bit.

sure. as i will bake another diff w/ removing isResponseWithContent anyway.

The test creates 5 byte request body when there is no content-length header and the min_content_length needed for compression is 30 bytes by default. So the compression should not have been applied, unless I'm missing something.

thats the whole point of the diff. new runtime flag would allow to unconditionally compress data if no content-length header and transfer-coding exists. in our deployment we found some h2 servers which do send replies w/o such headers (this is acceptable by h2 rfc). unfortunately there is no visible (for me at least) way to pass that info from h2 frame (where length is specified in h2 world). as a side effect yes, it would compress data even if total lengths is less that min_content_length (but only in scenarios when content-length header is missing)

Ok, I see. Conceptually you could check the minimum length in the encode/decodeData, right? If however min_content_length is ignored at least it should be documented in the config proto. Or even the field entirely deprecated.

min_content_length is used for http/1.1 usecases (at least from my understanding @rojkov wanted for it to continue to do so) so there is no point to depreciate it. it is documented that by default we compress only if transfer-encoding exists and set to chunked and/or content-length is set. anyway is it ok w/ @rojkov i can rework this to be runtime guard. let me know

min_content_length is a setting related to performance: compressing too small payloads doesn't make sense performance-wise. That's why I think deprecating this field would be too early. Checking it in encode/decodeData is too late, because we need to advertise content-encoding before that.

I've just checked the http/1.1 RFC and it says that

     If a Content-Length header field (section 14.13) is present, its
     decimal value in OCTETs represents both the entity-length and the
     transfer-length. The Content-Length header field MUST NOT be sent
     if these two lengths are different (i.e., if a Transfer-Encoding
     header field is present). If a message is received with both a
     Transfer-Encoding header field and a Content-Length header field,
     the latter MUST be ignored.

There's only one mention that content-length is a must in requests with bodies if the server is not known to support http/1.1.

So, basically we should first check transfer-encoding containing chunked and return true from isMinimumContentLength() if that's the case. Then check if content-length is present. If it is not then return true. Otherwise return the result of content-length >= min_content_length. Something like this

bool CompressorFilterConfig::DirectionConfig::isMinimumContentLength(
    const Http::RequestOrResponseHeaderMap& headers) const {
  if (StringUtil::caseFindToken(headers.getTransferEncodingValue(), ",",
                                Http::Headers::get().TransferEncodingValues.Chunked)) {
    return true;
  }

  const Http::HeaderEntry* content_length = headers.ContentLength();
  if (content_length == nullptr) {
    // FIXME: in case of HTTP/1.0 requests we should probably return false here, because the request is invalid
    //        and should not be touched by the compressor. A request's protocol can be consulted with
    //        decoder_callbacks_->streamInfo().protocol().
    return true;
  }

  uint64_t length;
  const bool is_minimum_content_length =
      absl::SimpleAtoi(content_length->value().getStringView(), &length) &&
      length >= min_content_length_;
  if (!is_minimum_content_length) {
    stats_.content_length_too_small_.inc();
  }

  return is_minimum_content_length;
}

Since it changes the current logic it's better to preserve the old code of isMinimumContentLength() and continue to use it unless the runtime guard allows the fix.

No need to explore other headers for detecting body-less responses because it will corrupt Vary headers.

Ok makes sense. I have just reread the comment for the min_content_length and does say that it is applicable to the content-length value. Thanks for clarification, please disregard my first comment in this thread.

My understanding is that runtime guards are defined here https://github.com/envoyproxy/envoy/blob/master/source/common/runtime/runtime_features.cc I do not know use cases for defining them in the config proto.

Ok, I see. Conceptually you could check the minimum length in the encode/decodeData, right? If however min_content_length is ignored at least it should be documented in the config proto. Or even the field entirely deprecated.

Is it worth only doing this in the case of HTTP/2? Or should we do it for HTTP/1 also? I don't feel strongly about it but was wondering if the RFC says anything about this one way or the other?