router: Add do_formatting arg to responseHeaderTransforms

[ahedberg]

Commit Message: Add do_formatting arg to responseHeaderTransforms
Additional Description: This change adds an additional argument, bool do_formatting, to ResponseEntry::responseHeaderTransforms. This allows callers to retrieve the original value instead of the evaluated/formatted value. The default value is true to match existing behavior and prevents changes to existing callsites.
Risk Level: low
Testing: unit tests
Docs Changes: doc updated
Release Notes: n/a
Platform Specific Features: n/a

[snowp]

Thanks, this seems reasonable to just a few nits.

Out of curiosity what is the use case for this?

[ahedberg]

Thanks, this seems reasonable to just a few nits.

Out of curiosity what is the use case for this?

We have an Envoy deployment where, at request time, we need to send the configured response header transformations to another (non-Envoy) service, which will apply them at response time. We don't want to evaluate the response header transformations at request time, as we need to convert any with the Envoy-style %PER_REQUEST_FORMAT% markers to the format the other service understands.

My initial PR adding this API should have done this, but I didn't realize the interaction at the time. My bad.

[ahedberg]

Ping @snowp ? Windows presubmit errors look unrelated to this change.

[snowp]

LGTM!

/retest

[repokitteh-read-only]

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

🐱

Caused by: a #16529 (review) was submitted by @snowp.

see: more, trace.

[snowp]

The Windows CI failures seem related:

external/com_google_googletest/googlemock/include\gmock/gmock-actions.h(776,60): error: returning reference to local temporary object [-Werror,-Wreturn-stack-address]
    Result Perform(const ArgumentTuple&) override { return ref_; }
                                                           ^~~~
external/com_google_googletest/googlemock/include\gmock/gmock-actions.h(774,14): note: in instantiation of member function 'testing::internal::ReturnRefAction<std::shared_ptr<Envoy::StreamInfo::FilterStateImpl>>::Impl<const std::shared_ptr<Envoy::StreamInfo::FilterState> &()>::Perform' requested here
    explicit Impl(T& ref) : ref_(ref) {}  // NOLINT
             ^
external/com_google_googletest/googlemock/include\gmock/gmock-actions.h(763,26): note: in instantiation of member function 'testing::internal::ReturnRefAction<std::shared_ptr<Envoy::StreamInfo::FilterStateImpl>>::Impl<const std::shared_ptr<Envoy::StreamInfo::FilterState> &()>::Impl' requested here
    return Action<F>(new Impl<F>(ref_));
                         ^
test/common/router/header_formatter_test.cc(1472,53): note: in instantiation of function template specialization 'testing::internal::ReturnRefAction<std::shared_ptr<Envoy::StreamInfo::FilterStateImpl>>::operator Action<const std::shared_ptr<Envoy::StreamInfo::FilterState> &()>' requested here
  ON_CALL(stream_info, filterState()).WillByDefault(ReturnRef(filter_state));

[ahedberg]

The Windows CI failures seem related:

external/com_google_googletest/googlemock/include\gmock/gmock-actions.h(776,60): error: returning reference to local temporary object [-Werror,-Wreturn-stack-address]
    Result Perform(const ArgumentTuple&) override { return ref_; }
                                                           ^~~~
external/com_google_googletest/googlemock/include\gmock/gmock-actions.h(774,14): note: in instantiation of member function 'testing::internal::ReturnRefAction<std::shared_ptr<Envoy::StreamInfo::FilterStateImpl>>::Impl<const std::shared_ptr<Envoy::StreamInfo::FilterState> &()>::Perform' requested here
    explicit Impl(T& ref) : ref_(ref) {}  // NOLINT
             ^
external/com_google_googletest/googlemock/include\gmock/gmock-actions.h(763,26): note: in instantiation of member function 'testing::internal::ReturnRefAction<std::shared_ptr<Envoy::StreamInfo::FilterStateImpl>>::Impl<const std::shared_ptr<Envoy::StreamInfo::FilterState> &()>::Impl' requested here
    return Action<F>(new Impl<F>(ref_));
                         ^
test/common/router/header_formatter_test.cc(1472,53): note: in instantiation of function template specialization 'testing::internal::ReturnRefAction<std::shared_ptr<Envoy::StreamInfo::FilterStateImpl>>::operator Action<const std::shared_ptr<Envoy::StreamInfo::FilterState> &()>' requested here
  ON_CALL(stream_info, filterState()).WillByDefault(ReturnRef(filter_state));

Hmm, that was not the error I saw the first time.

I think Windows is doing something weird now that I've switched this to auto - there are existing uses of ON_CALL(stream_info, filterState()).WillByDefault(ReturnRef(filter_state)); in this file that compile just fine, but those don't use auto. Switching back to spelling out the type to see if that fixes CI...

[ahedberg]

Dropping auto has indeed resolved the Windows CI problems. @snowp PTAL.

[snowp]

Thanks!

/retest

[repokitteh-read-only]

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

🐱

Caused by: a #16529 (review) was submitted by @snowp.

see: more, trace.