http: fix datadog and squash handling of responses without body

[alyssawilk]

Commit Message: Fixing bugs in datadog and sqaush where unexpected bodyless responses would crash Envoy
Risk Level: low
Testing: new unit tests
Docs Changes: n/a
Release Notes: inline

[alyssawilk]

per API discussion I've got
https://github.com/envoyproxy/envoy/compare/master...alyssawilk:datadog2?expand=1
which simply allocates a buffer. I don't think there's any perf hit so I mildly prefer the simplicity to optional reference wrapper.
The down side is there could be slight behavioral changes because one then can't differentiate no body from empty body, so if we go that route I'd be inclined to land it in a separate PR and only backport the bugfix.
Actually either way I think for backporting it'd probably be worth landing the minimal change on its own. WDYT?

[alyssawilk]

cc @envoyproxy/stable-maintainers for assessment

[mattklein123]

Yeah I agree on the further fixups. I like I mentioned in the other discussion, you could keep the reference return, and just return a static const buffer if the actual buffer is empty. This would probably perform slightly better but it's probably not worth it. Up to you.

/wait

[alyssawilk]

I don't think we could do static const, because it's mutable (and often mutated). I think we either stack allocate individual buffers, or do reference wrappers

[mattklein123]

I don't think we could do static const, because it's mutable (and often mutated). I think we either stack allocate individual buffers, or do reference wrappers

Ah OK. I'm fine either way. IIRC the buffer impl doesn't allocate any slices until data is added, so probably minimal perf hit.

[alyssawilk]

cool.
As said due to minor behavioral differences (code paths for H2 [headers only] vs [headers with delayed 0 body fin] I'd prefer to land the API change as a follow up, especially as if this gets backported (hoping it makes the bar for the reporter's sake) it's easier to do the smaller fix.
Anything else you'd like to see here?

[mattklein123]

I'd prefer to land the API change as a follow up, especially as if this gets backported (hoping it makes the bar for the reporter's sake) it's easier to do the smaller fix.

Ah OK sounds good!

[PiotrSikora]

/backport

[PiotrSikora]

@cpakulski anything with backport/review label needs to be evaluated for backporting and if approved, that label should be changed to backport/approved and PRs sent to stable releases.

[cpakulski]

Backported to releases 1.13-1.15. Removing backport/approved label.