From ff00d5b6ed1ac5b37926b8735a11d0bc6c548bfc Mon Sep 17 00:00:00 2001 From: Kannan J Date: Wed, 10 Jun 2026 05:14:25 +0000 Subject: [PATCH 1/3] xds: ignore keep_empty_value and discard header keys on empty mutations In GrpcService/ext_proc header mutations, we do not support the `keep_empty_value` field (https://github.com/grpc/proposal/pull/510/changes/f6d42d67dd7b80304eee1b29084d9b9af0b2feae). 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. --- .../headermutations/HeaderMutator.java | 36 ++++++++++++++----- .../headermutations/HeaderMutatorTest.java | 17 ++++----- 2 files changed, 34 insertions(+), 19 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutator.java b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutator.java index e6cdc126f22..3edbb6287d1 100644 --- a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutator.java +++ b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutator.java @@ -67,24 +67,23 @@ private void applyHeaderUpdates(final Iterable headerOptions, private void updateHeader(final HeaderValueOption option, Metadata mutableHeaders) { HeaderValue header = option.header(); HeaderAppendAction action = option.appendAction(); - boolean keepEmptyValue = option.keepEmptyValue(); if (header.key().endsWith(Metadata.BINARY_HEADER_SUFFIX)) { if (header.rawValue().isPresent()) { - byte[] value = header.rawValue().get().toByteArray(); - if (value.length > 0 || keepEmptyValue) { - updateHeader(action, Metadata.Key.of(header.key(), Metadata.BINARY_BYTE_MARSHALLER), - value, mutableHeaders); + Metadata.Key key = Metadata.Key.of(header.key(), Metadata.BINARY_BYTE_MARSHALLER); + updateHeader(action, key, header.rawValue().get().toByteArray(), mutableHeaders); + if (containsEmpty(key, mutableHeaders)) { + mutableHeaders.discardAll(key); } } else { logger.fine("Missing binary rawValue for header: " + header.key()); } } else { if (header.value().isPresent()) { - String value = header.value().get(); - if (!value.isEmpty() || keepEmptyValue) { - updateHeader(action, Metadata.Key.of(header.key(), Metadata.ASCII_STRING_MARSHALLER), - value, mutableHeaders); + Metadata.Key key = Metadata.Key.of(header.key(), Metadata.ASCII_STRING_MARSHALLER); + updateHeader(action, key, header.value().get(), mutableHeaders); + if (containsEmpty(key, mutableHeaders)) { + mutableHeaders.discardAll(key); } } else { logger.fine("Missing value for header: " + header.key()); @@ -119,5 +118,24 @@ private void updateHeader(final HeaderAppendAction action, final Metadata.Ke logger.fine("Unknown HeaderAppendAction: " + action); } } + + private boolean containsEmpty(Metadata.Key key, Metadata headers) { + Iterable 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; + } } diff --git a/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutatorTest.java b/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutatorTest.java index b6806760f9b..0eddb9ec9d3 100644 --- a/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutatorTest.java +++ b/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutatorTest.java @@ -247,30 +247,27 @@ public void applyMutations_keepEmptyValue() { headerMutator.applyMutations(mutations, headers); assertThat(headers.containsKey(NEW_ADD_KEY)).isFalse(); - assertThat(headers.getAll(APPEND_KEY)).containsExactly("existing-value"); - assertThat(headers.get(OVERWRITE_KEY)).isEqualTo("existing-value"); + assertThat(headers.containsKey(APPEND_KEY)).isFalse(); + assertThat(headers.containsKey(OVERWRITE_KEY)).isFalse(); assertThat(headers.containsKey(ADD_KEY)).isFalse(); - assertThat(headers.get(OVERWRITE_IF_EXISTS_KEY)).isEqualTo("existing-value"); + assertThat(headers.containsKey(OVERWRITE_IF_EXISTS_KEY)).isFalse(); Metadata.Key keepEmptyKey = Metadata.Key.of("keep-empty-key", Metadata.ASCII_STRING_MARSHALLER); Metadata.Key keepEmptyOverwriteKey = Metadata.Key.of("keep-empty-overwrite-key", Metadata.ASCII_STRING_MARSHALLER); - assertThat(headers.containsKey(keepEmptyKey)).isTrue(); - assertThat(headers.get(keepEmptyKey)).isEqualTo(""); - assertThat(headers.containsKey(keepEmptyOverwriteKey)).isTrue(); - assertThat(headers.get(keepEmptyOverwriteKey)).isEqualTo(""); + assertThat(headers.containsKey(keepEmptyKey)).isFalse(); + assertThat(headers.containsKey(keepEmptyOverwriteKey)).isFalse(); Metadata.Key keepEmptyBinKey = Metadata.Key.of("keep-empty-bin-key-bin", Metadata.BINARY_BYTE_MARSHALLER); Metadata.Key ignoreEmptyBinKey = Metadata.Key.of("ignore-empty-bin-key-bin", Metadata.BINARY_BYTE_MARSHALLER); - assertThat(headers.containsKey(keepEmptyBinKey)).isTrue(); - assertThat(headers.get(keepEmptyBinKey)).isEqualTo(new byte[0]); + assertThat(headers.containsKey(keepEmptyBinKey)).isFalse(); assertThat(headers.containsKey(ignoreEmptyBinKey)).isFalse(); - assertThat(headers.get(overwriteEmptyBinKey)).isEqualTo(originalBinValue); + assertThat(headers.containsKey(overwriteEmptyBinKey)).isFalse(); } @Test From fa8c7353164fb39e6c74247e68bde6ddc383b46d Mon Sep 17 00:00:00 2001 From: Kannan J Date: Thu, 11 Jun 2026 05:04:14 +0000 Subject: [PATCH 2/3] . --- .../headermutations/HeaderValueOption.java | 6 ++--- .../HeaderMutationFilterTest.java | 4 ++-- .../headermutations/HeaderMutationsTest.java | 2 +- .../headermutations/HeaderMutatorTest.java | 24 ++++++++----------- .../HeaderValueOptionTest.java | 3 +-- 5 files changed, 16 insertions(+), 23 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderValueOption.java b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderValueOption.java index 6cb96da864d..e9b58338972 100644 --- a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderValueOption.java +++ b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderValueOption.java @@ -27,16 +27,14 @@ public abstract class HeaderValueOption { public static HeaderValueOption create( - HeaderValue header, HeaderAppendAction appendAction, boolean keepEmptyValue) { - return new AutoValue_HeaderValueOption(header, appendAction, keepEmptyValue); + HeaderValue header, HeaderAppendAction appendAction) { + return new AutoValue_HeaderValueOption(header, appendAction); } public abstract HeaderValue header(); public abstract HeaderAppendAction appendAction(); - public abstract boolean keepEmptyValue(); - /** * Defines the action to take when appending headers. * Mirrors io.envoyproxy.envoy.config.core.v3.HeaderValueOption.HeaderAppendAction. diff --git a/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutationFilterTest.java b/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutationFilterTest.java index f07997a6244..99f3c8e9967 100644 --- a/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutationFilterTest.java +++ b/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutationFilterTest.java @@ -36,12 +36,12 @@ public class HeaderMutationFilterTest { private static HeaderValueOption header(String key, ByteString value) { return HeaderValueOption.create(io.grpc.xds.internal.grpcservice.HeaderValue.create(key, value), - HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, false); + HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD); } private static HeaderValueOption header(String key, String value) { return HeaderValueOption.create(io.grpc.xds.internal.grpcservice.HeaderValue.create(key, value), - HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, false); + HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD); } @Test diff --git a/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutationsTest.java b/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutationsTest.java index ef7f22b7ac8..af4849a126c 100644 --- a/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutationsTest.java +++ b/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutationsTest.java @@ -31,7 +31,7 @@ public class HeaderMutationsTest { public void testCreate() { HeaderValueOption header = HeaderValueOption.create( HeaderValue.create("key", "value"), - HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, false); + HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD); HeaderMutations mutations = HeaderMutations.create( ImmutableList.of(header), ImmutableList.of("remove-key")); assertThat(mutations.headers()).containsExactly(header); diff --git a/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutatorTest.java b/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutatorTest.java index 0eddb9ec9d3..b6cf90712c3 100644 --- a/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutatorTest.java +++ b/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutatorTest.java @@ -73,7 +73,7 @@ public void tearDown() { } private static HeaderValueOption header(String key, String value, HeaderAppendAction action) { - return HeaderValueOption.create(HeaderValue.create(key, value), action, false); + return HeaderValueOption.create(HeaderValue.create(key, value), action); } @Test @@ -147,8 +147,7 @@ public void applyMutations_binary() { HeaderValueOption option = HeaderValueOption.create( HeaderValue.create(BINARY_KEY.name(), ByteString.copyFrom(value)), - HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, - false); + HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD); headerMutator.applyMutations( HeaderMutations.create(ImmutableList.of(option), ImmutableList.of()), headers); assertThat(headers.get(BINARY_KEY)).isEqualTo(value); @@ -195,8 +194,7 @@ public void applyResponseMutations_binary() { HeaderValueOption option = HeaderValueOption.create( HeaderValue.create(BINARY_KEY.name(), ByteString.copyFrom(value)), - HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, - false); + HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD); headerMutator.applyMutations( HeaderMutations.create(ImmutableList.of(option), ImmutableList.of()), headers); assertThat(headers.get(BINARY_KEY)).isEqualTo(value); @@ -219,21 +217,19 @@ public void applyMutations_keepEmptyValue() { header(OVERWRITE_IF_EXISTS_KEY.name(), "", HeaderAppendAction.OVERWRITE_IF_EXISTS), HeaderValueOption.create( HeaderValue.create("keep-empty-key", ""), - HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, - true), + HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), HeaderValueOption.create( HeaderValue.create("keep-empty-overwrite-key", ""), - HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD, - true), + HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD), HeaderValueOption.create( HeaderValue.create("keep-empty-bin-key-bin", ByteString.EMPTY), - HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, true), + HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), HeaderValueOption.create( HeaderValue.create("ignore-empty-bin-key-bin", ByteString.EMPTY), - HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, false), + HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), HeaderValueOption.create( HeaderValue.create("overwrite-empty-bin-key-bin", ByteString.EMPTY), - HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD, false)), + HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD)), ImmutableList.of()); headers.put( @@ -287,7 +283,7 @@ public void applyMutations_binaryRemoval() { public void applyMutations_stringValueWithBinaryKey_ignored() { Metadata headers = new Metadata(); HeaderValueOption option = HeaderValueOption.create(HeaderValue.create("some-key-bin", "value"), - HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, false); + HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD); headerMutator.applyMutations( HeaderMutations.create(ImmutableList.of(option), ImmutableList.of()), headers); @@ -301,7 +297,7 @@ public void applyMutations_binaryValueWithAsciiKey_ignored() { Metadata headers = new Metadata(); HeaderValueOption option = HeaderValueOption.create( HeaderValue.create("some-key", ByteString.copyFrom(new byte[] {1})), - HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, false); + HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD); headerMutator.applyMutations( HeaderMutations.create(ImmutableList.of(option), ImmutableList.of()), headers); diff --git a/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderValueOptionTest.java b/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderValueOptionTest.java index 49c43749135..c2ee4b747ba 100644 --- a/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderValueOptionTest.java +++ b/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderValueOptionTest.java @@ -31,10 +31,9 @@ public class HeaderValueOptionTest { public void create_withAllFields_success() { HeaderValue header = HeaderValue.create("key1", "value1"); HeaderValueOption option = HeaderValueOption.create( - header, HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, true); + header, HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD); assertThat(option.header()).isEqualTo(header); assertThat(option.appendAction()).isEqualTo(HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD); - assertThat(option.keepEmptyValue()).isTrue(); } } From 0ac325d000aa34630c1c672baa7af8fa8564a12d Mon Sep 17 00:00:00 2001 From: Kannan J Date: Mon, 22 Jun 2026 08:37:12 +0000 Subject: [PATCH 3/3] xds: always keep empty mutated headers and ignore keep_empty_value Updates the HeaderMutator library to always apply mutations even if the resulting value is empty, ignoring the keep_empty_value setting altogether. Empty mutated values are now kept in the metadata and the header key is no longer discarded. - Removes containsEmpty check and helper from HeaderMutator. - Updates HeaderMutatorTest to assert that empty mutations are kept. --- .../headermutations/HeaderMutator.java | 25 ------------------- .../headermutations/HeaderMutatorTest.java | 22 ++++++++-------- 2 files changed, 11 insertions(+), 36 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutator.java b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutator.java index 3edbb6287d1..ef13590e2c0 100644 --- a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutator.java +++ b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutator.java @@ -72,9 +72,6 @@ private void updateHeader(final HeaderValueOption option, Metadata mutableHeader if (header.rawValue().isPresent()) { Metadata.Key key = Metadata.Key.of(header.key(), Metadata.BINARY_BYTE_MARSHALLER); updateHeader(action, key, header.rawValue().get().toByteArray(), mutableHeaders); - if (containsEmpty(key, mutableHeaders)) { - mutableHeaders.discardAll(key); - } } else { logger.fine("Missing binary rawValue for header: " + header.key()); } @@ -82,9 +79,6 @@ private void updateHeader(final HeaderValueOption option, Metadata mutableHeader if (header.value().isPresent()) { Metadata.Key key = Metadata.Key.of(header.key(), Metadata.ASCII_STRING_MARSHALLER); updateHeader(action, key, header.value().get(), mutableHeaders); - if (containsEmpty(key, mutableHeaders)) { - mutableHeaders.discardAll(key); - } } else { logger.fine("Missing value for header: " + header.key()); } @@ -118,24 +112,5 @@ private void updateHeader(final HeaderAppendAction action, final Metadata.Ke logger.fine("Unknown HeaderAppendAction: " + action); } } - - private boolean containsEmpty(Metadata.Key key, Metadata headers) { - Iterable 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; - } } diff --git a/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutatorTest.java b/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutatorTest.java index b6cf90712c3..f6f750b0dfc 100644 --- a/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutatorTest.java +++ b/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutatorTest.java @@ -201,7 +201,7 @@ public void applyResponseMutations_binary() { } @Test - public void applyMutations_keepEmptyValue() { + public void applyMutations_emptyValuesAreKept() { Metadata headers = new Metadata(); headers.put(APPEND_KEY, "existing-value"); headers.put(OVERWRITE_KEY, "existing-value"); @@ -242,28 +242,28 @@ public void applyMutations_keepEmptyValue() { headerMutator.applyMutations(mutations, headers); - assertThat(headers.containsKey(NEW_ADD_KEY)).isFalse(); - assertThat(headers.containsKey(APPEND_KEY)).isFalse(); - assertThat(headers.containsKey(OVERWRITE_KEY)).isFalse(); - assertThat(headers.containsKey(ADD_KEY)).isFalse(); - assertThat(headers.containsKey(OVERWRITE_IF_EXISTS_KEY)).isFalse(); + assertThat(headers.get(NEW_ADD_KEY)).isEqualTo(""); + assertThat(headers.getAll(APPEND_KEY)).containsExactly("existing-value", ""); + assertThat(headers.get(OVERWRITE_KEY)).isEqualTo(""); + assertThat(headers.get(ADD_KEY)).isEqualTo(""); + assertThat(headers.get(OVERWRITE_IF_EXISTS_KEY)).isEqualTo(""); Metadata.Key keepEmptyKey = Metadata.Key.of("keep-empty-key", Metadata.ASCII_STRING_MARSHALLER); Metadata.Key keepEmptyOverwriteKey = Metadata.Key.of("keep-empty-overwrite-key", Metadata.ASCII_STRING_MARSHALLER); - assertThat(headers.containsKey(keepEmptyKey)).isFalse(); - assertThat(headers.containsKey(keepEmptyOverwriteKey)).isFalse(); + assertThat(headers.get(keepEmptyKey)).isEqualTo(""); + assertThat(headers.get(keepEmptyOverwriteKey)).isEqualTo(""); Metadata.Key keepEmptyBinKey = Metadata.Key.of("keep-empty-bin-key-bin", Metadata.BINARY_BYTE_MARSHALLER); Metadata.Key ignoreEmptyBinKey = Metadata.Key.of("ignore-empty-bin-key-bin", Metadata.BINARY_BYTE_MARSHALLER); - assertThat(headers.containsKey(keepEmptyBinKey)).isFalse(); - assertThat(headers.containsKey(ignoreEmptyBinKey)).isFalse(); - assertThat(headers.containsKey(overwriteEmptyBinKey)).isFalse(); + assertThat(headers.get(keepEmptyBinKey)).isEqualTo(new byte[0]); + assertThat(headers.get(ignoreEmptyBinKey)).isEqualTo(new byte[0]); + assertThat(headers.get(overwriteEmptyBinKey)).isEqualTo(new byte[0]); } @Test