xds: ignore keep_empty_value and discard header keys on empty mutations#12852
xds: ignore keep_empty_value and discard header keys on empty mutations#12852kannanjgithub wants to merge 1 commit into
Conversation
In GrpcService/ext_proc header mutations, we do not support the `keep_empty_value` field (grpc/proposal@f6d42d6). If any header mutation results in a header containing an empty value (either string or binary), the entire header key is discarded/removed from the metadata. This chang modifies HeaderMutator to apply mutations and discard keys containing empty values.
|
/gemini review |
|
/gemini-review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the header mutation logic in HeaderMutator.java to discard headers containing empty values, updating the corresponding test cases in HeaderMutatorTest.java to reflect this change. A review comment suggests that the newly added containsEmpty helper method is redundant and can be safely removed.
| private <T> boolean containsEmpty(Metadata.Key<T> key, Metadata headers) { | ||
| Iterable<T> values = headers.getAll(key); | ||
| if (values == null) { | ||
| return false; | ||
| } | ||
| for (T val : values) { | ||
| if (val instanceof String) { | ||
| if (((String) val).isEmpty()) { | ||
| return true; | ||
| } | ||
| } else if (val instanceof byte[]) { | ||
| if (((byte[]) val).length == 0) { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Since empty mutations are now handled directly in updateHeader without needing to scan the metadata, the containsEmpty helper method is no longer needed and can be safely removed.
There was a problem hiding this comment.
We might want to investigate the need for this function. I've added a comment here previously grpc/proposal#481 (comment)
about compliance with envoy behavior.
| private void updateHeader(final HeaderValueOption option, Metadata mutableHeaders) { | ||
| HeaderValue header = option.header(); | ||
| HeaderAppendAction action = option.appendAction(); | ||
| boolean keepEmptyValue = option.keepEmptyValue(); |
There was a problem hiding this comment.
We should probably delete the config field as well if unused.
2 participants
In GrpcService/ext_proc header mutations, we do not support the
keep_empty_valuefield (grpc/proposal@f6d42d6). If any header mutation results in a header containing an empty value (either string or binary), the entire header key is discarded/removed from the metadata.This change modifies HeaderMutator to apply mutations and discard keys containing empty values.