custom_response: allow LocalResponsePolicy to preserve or set response_code_details

Project: envoyproxy/envoy | Language: C++ | Concepts: api-design, backwards-compatibility, testing-strategy | Difficulty: mid
Link: https://github.com/envoyproxy/envoy/pull/46600

The problem

When Envoy’s LocalResponsePolicy triggered a local reply, it always cleared the response_code_details field on the stream, overwriting any existing value (such as csrf_origin_mismatch). This made it harder to debug or log the original reason for a local response, since that information was lost after the reply.

Why it's non-trivial

A naive fix might just always preserve the existing details, or always set a new value. However, this would silently change behavior for existing configurations, potentially breaking compatibility or expectations. The solution needed to be opt-in, with clear, explicit configuration, and maintain the legacy behavior by default.

The approach

1. API Extension: Add a oneof to control response_code_details behavior

+  oneof response_code_details_action {
+    // If set, keep the existing ``response_code_details`` already present on StreamInfo.
+    bool preserve_response_code_details = 5 [(validate.rules).bool = {const: true}];
+
+    // Replace ``response_code_details`` with this value.
+    string response_code_details = 6 [(validate.rules).string = {min_len: 1}];
+  }

• This adds a mutually exclusive option: either preserve the existing details, or set a new string, or (by default) clear the field. The PGV (Protobuf Validation) rule ensures the boolean can only be set to true (never false).

2. Implementation: Respect the new config in the local response logic

Helper functions to check which option is set:

+bool preserveResponseCodeDetails(const LocalResponsePolicyProto& config) {
+  return config.response_code_details_action_case() ==
+             LocalResponsePolicyProto::kPreserveResponseCodeDetails &&
+         config.preserve_response_code_details();
+}
+
+std::optional<std::string> overrideResponseCodeDetails(const LocalResponsePolicyProto& config) {
+  if (config.response_code_details_action_case() ==
+      LocalResponsePolicyProto::kResponseCodeDetails) {
+    return config.response_code_details();
+  }
+  return std::nullopt;
+}

• These helpers encapsulate the logic for reading which field (if any) is set in the oneof.

Store the config in the object:

+      preserve_response_code_details_(preserveResponseCodeDetails(config)),
+      response_code_details_(overrideResponseCodeDetails(config)) {

• The policy object now remembers which option (if any) was chosen.

Set the details at the point of sending the local reply:

+  absl::string_view details;
+  if (response_code_details_.has_value()) {
+    details = *response_code_details_;
+  } else if (preserve_response_code_details_) {
+    const auto& stream_details = encoder_callbacks->streamInfo().responseCodeDetails();
+    details = stream_details.has_value() ? *stream_details : EMPTY_STRING;
+  }
...
-  encoder_callbacks->sendLocalReply(code, body, mutate_headers, std::nullopt, "");
+  encoder_callbacks->sendLocalReply(code, body, mutate_headers, std::nullopt, details);

• If a new value is set, use it. If preserve is set, copy the existing value. Otherwise, pass the empty string (legacy behavior).

3. Testing: Add comprehensive tests for all behaviors

Tests cover:

• Legacy (unset) clears details
• Preserve copies existing details
• Preserve with no existing details results in empty
• Explicit override sets the provided string

Example:

+TEST_F(CustomResponseFilterTest, PreserveResponseCodeDetails) {
+  ...
+  encoder_callbacks_.stream_info_.response_code_details_ = "csrf_origin_mismatch";
+  ...
+  EXPECT_CALL(encoder_callbacks_, sendLocalReply(static_cast<::Envoy::Http::Code>(403), "forbidden",
+                                                 _, _, "csrf_origin_mismatch"));
+  ...
+}

• This verifies that the preserve option correctly carries forward the original detail.

From the review thread

wbpcode: Please add a PGV to ensure the bool only could be set be true?

wbpcode: prefer absl::string_view here because we needn't ownership here.

Takeaways

• When changing default behaviors in widely used APIs, provide explicit, opt-in configuration to avoid breaking existing users.
• Use oneof in protobuf to make mutually exclusive options clear and enforceable at the API level.
• Validate configuration strictly (e.g., via PGV rules) to prevent ambiguous or invalid states.
• Comprehensive testing of all configuration permutations ensures both legacy and new behaviors are correct and maintainable.