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..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 @@ -67,25 +67,18 @@ 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); } 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); } else { logger.fine("Missing value for header: " + header.key()); } 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 b6806760f9b..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 @@ -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,15 +194,14 @@ 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); } @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"); @@ -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( @@ -246,20 +242,18 @@ 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(ADD_KEY)).isFalse(); - assertThat(headers.get(OVERWRITE_IF_EXISTS_KEY)).isEqualTo("existing-value"); + 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)).isTrue(); assertThat(headers.get(keepEmptyKey)).isEqualTo(""); - assertThat(headers.containsKey(keepEmptyOverwriteKey)).isTrue(); assertThat(headers.get(keepEmptyOverwriteKey)).isEqualTo(""); Metadata.Key keepEmptyBinKey = @@ -267,10 +261,9 @@ public void applyMutations_keepEmptyValue() { 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(ignoreEmptyBinKey)).isFalse(); - assertThat(headers.get(overwriteEmptyBinKey)).isEqualTo(originalBinValue); + assertThat(headers.get(ignoreEmptyBinKey)).isEqualTo(new byte[0]); + assertThat(headers.get(overwriteEmptyBinKey)).isEqualTo(new byte[0]); } @Test @@ -290,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); @@ -304,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(); } }