http: split strict_1xx_and_204_response_headers into two settings

[tbarrella]

Commit Message:

http: split strict_1xx_and_204_response_headers into two settings

Signed-off-by: Taylor Barrella [email protected]

Additional Description:
Risk Level: Low
Testing: unit
Docs Changes: N/A
Release Notes: added minor behavior change
Runtime guard: replaced strict_1xx_and_204_response_headers with require_strict_1xx_and_204_response_headers and send_strict_1xx_and_204_response_headers
Fixes #13868

[tbarrella]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15880 (comment) was created by @tbarrella.

see: more, trace.

[lambdai]

Generally LGTM. Thanks!

Question: If accept_strict_1xx_and_204_response_headers == false and send_strict_1xx_and_204_response_headers == true
Is TE stripped in downstream response when the upstream response contains TE?

It's not clear to me whether the downstream TE is always decided by send_strict_1xx_and_204_response_headers regardless the upstream response contains TE or not.

[alyssawilk]

LG overall, thanks so much for picking this one up!

Two thoughts. Do you think it should be strict_accept since we're making accepting strict, it's not that we're accepting strict headers (which I think we would either way?)

I think it's worth an integration test making sure that new-accept works with old-send, to make sure this actually addresses the problem.

[tbarrella]

Do you think it should be strict_accept since we're making accepting strict, it's not that we're accepting strict headers (which I think we would either way?)

That's more grammatical yeah, although I think the flags should be consistent. So I can switch that as long as it's ok to switch the other to strict_send. Also, feel like I might as well use strict_allow because that seems a little better...

[alyssawilk]

Hm, I feel like send_strict and strict_accept makes sense but I hear you about wanting to keep them parallel. How about send_strict and require_strict so you can keep the ordering the same but the meaning clear?

[tbarrella]

Question: If accept_strict_1xx_and_204_response_headers == false and send_strict_1xx_and_204_response_headers == true
Is TE stripped in downstream response when the upstream response contains TE?

I think so based on this comment.

I think it's worth an integration test making sure that new-accept works with old-send, to make sure this actually addresses the problem.

Do you mean require_strict_1xx_and_204_response_headers = false and send_strict_1xx_and_204_response_headers = true (what's desired in istio/istio#28450 (comment))? Looking more, I'm not sure how valuable it would be to add an integration test for this. For example, I could duplicate the BidirectionalChunkedData test with require_strict_1xx_and_204_response_headers = false (it passes, and also fails with send_strict_1xx_and_204_response_headers = false instead) but the only real place this is used is here. I think these paths are effectively covered by the tests in codec_impl_test.cc.

[tbarrella]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15880 (comment) was created by @tbarrella.

see: more, trace.

[alyssawilk]

Ah, is what you're saying that the codec tests, while looking the same, are actually testing the case which failed before, of a mix of old and new style headers, so are validating the bug fix despite not being end to end? If so I think I'm good :-)

[tbarrella]

Yeah, specifically the second half of 204ResponseWithContentLength0 is a situation that could result from this combination of flags. Thank you!

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15880 (comment) was created by @tbarrella.

see: more, trace.

[tbarrella]

Hi @snowp, @alyssawilk said this is

going to need a main merge around 12PDT when the new release is created and 1.19 notes are created. ... I've tagged snowp for second pass, and to do the merge while I'm out so you can ping him once it's re-ready.

Did I tag you too late? Which release will this be going into?

[tbarrella]

Hm, the format check failure looks unrelated to my change

[snowp]

This will go out with 1.19 as is. Can you try merging main and see it that helps resolve the format issue? It does seem unrelated to this code as you say

[tbarrella]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15880 (comment) was created by @tbarrella.

see: more, trace.

[snowp]

Thanks!

[jojofeng]

Hi, I'm new to envoy so I apologize for the naive question, but how do I actually configure these flags? I would like to set require_strict_1xx_and_204_headers to False in my configuration. Do I pass it from the command line? Or do I need to add something to my YAML config?

EDIT: I believe I can add it by configuring a static_layer

[rbalaraman-c]

Same as @jojofeng (above), I would also like to know how to configure these flags.
I believe I can do it by calling "POST /runtime_modify" on the envoy sidecar admin interface. But what is a permanent way of configuring them?

[tbarrella]

@rbalaraman-c You can add a static_layer layer to layered_runtime.layers in bootstrap config. The static layer should be a {runtime flag: value} map.

[jojofeng]

@rbalaraman-c this is the exact configuration block i use 😄 i hope it helps!

layered_runtime:
  layers:
    - name: base
      static_layer:
        envoy.reloadable_features.require_strict_1xx_and_204_response_headers: false