Skip to content

Commit 293aa89

Browse files
kannanjgithubkannanjgithub
authored
xds: ignore keep_empty_value and discard header keys on empty mutations (#12852)
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), we proceed with the empty value.
1 parent c7a946b commit 293aa89

6 files changed

Lines changed: 28 additions & 45 deletions

File tree

‎xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutator.java‎

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -67,25 +67,18 @@ private void applyHeaderUpdates(final Iterable<HeaderValueOption> headerOptions,
6767
private void updateHeader(final HeaderValueOption option, Metadata mutableHeaders) {
6868
HeaderValue header = option.header();
6969
HeaderAppendAction action = option.appendAction();
70-
boolean keepEmptyValue = option.keepEmptyValue();
7170

7271
if (header.key().endsWith(Metadata.BINARY_HEADER_SUFFIX)) {
7372
if (header.rawValue().isPresent()) {
74-
byte[] value = header.rawValue().get().toByteArray();
75-
if (value.length > 0 || keepEmptyValue) {
76-
updateHeader(action, Metadata.Key.of(header.key(), Metadata.BINARY_BYTE_MARSHALLER),
77-
value, mutableHeaders);
78-
}
73+
Metadata.Key<byte[]> key = Metadata.Key.of(header.key(), Metadata.BINARY_BYTE_MARSHALLER);
74+
updateHeader(action, key, header.rawValue().get().toByteArray(), mutableHeaders);
7975
} else {
8076
logger.fine("Missing binary rawValue for header: " + header.key());
8177
}
8278
} else {
8379
if (header.value().isPresent()) {
84-
String value = header.value().get();
85-
if (!value.isEmpty() || keepEmptyValue) {
86-
updateHeader(action, Metadata.Key.of(header.key(), Metadata.ASCII_STRING_MARSHALLER),
87-
value, mutableHeaders);
88-
}
80+
Metadata.Key<String> key = Metadata.Key.of(header.key(), Metadata.ASCII_STRING_MARSHALLER);
81+
updateHeader(action, key, header.value().get(), mutableHeaders);
8982
} else {
9083
logger.fine("Missing value for header: " + header.key());
9184
}

‎xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderValueOption.java‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,16 +27,14 @@
2727
public abstract class HeaderValueOption {
2828

2929
public static HeaderValueOption create(
30-
HeaderValue header, HeaderAppendAction appendAction, boolean keepEmptyValue) {
31-
return new AutoValue_HeaderValueOption(header, appendAction, keepEmptyValue);
30+
HeaderValue header, HeaderAppendAction appendAction) {
31+
return new AutoValue_HeaderValueOption(header, appendAction);
3232
}
3333

3434
public abstract HeaderValue header();
3535

3636
public abstract HeaderAppendAction appendAction();
3737

38-
public abstract boolean keepEmptyValue();
39-
4038
/**
4139
* Defines the action to take when appending headers.
4240
* Mirrors io.envoyproxy.envoy.config.core.v3.HeaderValueOption.HeaderAppendAction.

‎xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutationFilterTest.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,12 +36,12 @@ public class HeaderMutationFilterTest {
3636

3737
private static HeaderValueOption header(String key, ByteString value) {
3838
return HeaderValueOption.create(io.grpc.xds.internal.grpcservice.HeaderValue.create(key, value),
39-
HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, false);
39+
HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD);
4040
}
4141

4242
private static HeaderValueOption header(String key, String value) {
4343
return HeaderValueOption.create(io.grpc.xds.internal.grpcservice.HeaderValue.create(key, value),
44-
HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, false);
44+
HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD);
4545
}
4646

4747
@Test

‎xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutationsTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ public class HeaderMutationsTest {
3131
public void testCreate() {
3232
HeaderValueOption header = HeaderValueOption.create(
3333
HeaderValue.create("key", "value"),
34-
HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, false);
34+
HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD);
3535
HeaderMutations mutations = HeaderMutations.create(
3636
ImmutableList.of(header), ImmutableList.of("remove-key"));
3737
assertThat(mutations.headers()).containsExactly(header);

‎xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutatorTest.java‎

Lines changed: 18 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ public void tearDown() {
7373
}
7474

7575
private static HeaderValueOption header(String key, String value, HeaderAppendAction action) {
76-
return HeaderValueOption.create(HeaderValue.create(key, value), action, false);
76+
return HeaderValueOption.create(HeaderValue.create(key, value), action);
7777
}
7878

7979
@Test
@@ -147,8 +147,7 @@ public void applyMutations_binary() {
147147
HeaderValueOption option =
148148
HeaderValueOption.create(
149149
HeaderValue.create(BINARY_KEY.name(), ByteString.copyFrom(value)),
150-
HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD,
151-
false);
150+
HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD);
152151
headerMutator.applyMutations(
153152
HeaderMutations.create(ImmutableList.of(option), ImmutableList.of()), headers);
154153
assertThat(headers.get(BINARY_KEY)).isEqualTo(value);
@@ -195,15 +194,14 @@ public void applyResponseMutations_binary() {
195194
HeaderValueOption option =
196195
HeaderValueOption.create(
197196
HeaderValue.create(BINARY_KEY.name(), ByteString.copyFrom(value)),
198-
HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD,
199-
false);
197+
HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD);
200198
headerMutator.applyMutations(
201199
HeaderMutations.create(ImmutableList.of(option), ImmutableList.of()), headers);
202200
assertThat(headers.get(BINARY_KEY)).isEqualTo(value);
203201
}
204202

205203
@Test
206-
public void applyMutations_keepEmptyValue() {
204+
public void applyMutations_emptyValuesAreKept() {
207205
Metadata headers = new Metadata();
208206
headers.put(APPEND_KEY, "existing-value");
209207
headers.put(OVERWRITE_KEY, "existing-value");
@@ -219,21 +217,19 @@ public void applyMutations_keepEmptyValue() {
219217
header(OVERWRITE_IF_EXISTS_KEY.name(), "", HeaderAppendAction.OVERWRITE_IF_EXISTS),
220218
HeaderValueOption.create(
221219
HeaderValue.create("keep-empty-key", ""),
222-
HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD,
223-
true),
220+
HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD),
224221
HeaderValueOption.create(
225222
HeaderValue.create("keep-empty-overwrite-key", ""),
226-
HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD,
227-
true),
223+
HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD),
228224
HeaderValueOption.create(
229225
HeaderValue.create("keep-empty-bin-key-bin", ByteString.EMPTY),
230-
HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, true),
226+
HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD),
231227
HeaderValueOption.create(
232228
HeaderValue.create("ignore-empty-bin-key-bin", ByteString.EMPTY),
233-
HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, false),
229+
HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD),
234230
HeaderValueOption.create(
235231
HeaderValue.create("overwrite-empty-bin-key-bin", ByteString.EMPTY),
236-
HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD, false)),
232+
HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD)),
237233
ImmutableList.of());
238234

239235
headers.put(
@@ -246,31 +242,28 @@ public void applyMutations_keepEmptyValue() {
246242

247243
headerMutator.applyMutations(mutations, headers);
248244

249-
assertThat(headers.containsKey(NEW_ADD_KEY)).isFalse();
250-
assertThat(headers.getAll(APPEND_KEY)).containsExactly("existing-value");
251-
assertThat(headers.get(OVERWRITE_KEY)).isEqualTo("existing-value");
252-
assertThat(headers.containsKey(ADD_KEY)).isFalse();
253-
assertThat(headers.get(OVERWRITE_IF_EXISTS_KEY)).isEqualTo("existing-value");
245+
assertThat(headers.get(NEW_ADD_KEY)).isEqualTo("");
246+
assertThat(headers.getAll(APPEND_KEY)).containsExactly("existing-value", "");
247+
assertThat(headers.get(OVERWRITE_KEY)).isEqualTo("");
248+
assertThat(headers.get(ADD_KEY)).isEqualTo("");
249+
assertThat(headers.get(OVERWRITE_IF_EXISTS_KEY)).isEqualTo("");
254250

255251
Metadata.Key<String> keepEmptyKey =
256252
Metadata.Key.of("keep-empty-key", Metadata.ASCII_STRING_MARSHALLER);
257253
Metadata.Key<String> keepEmptyOverwriteKey =
258254
Metadata.Key.of("keep-empty-overwrite-key", Metadata.ASCII_STRING_MARSHALLER);
259255

260-
assertThat(headers.containsKey(keepEmptyKey)).isTrue();
261256
assertThat(headers.get(keepEmptyKey)).isEqualTo("");
262-
assertThat(headers.containsKey(keepEmptyOverwriteKey)).isTrue();
263257
assertThat(headers.get(keepEmptyOverwriteKey)).isEqualTo("");
264258

265259
Metadata.Key<byte[]> keepEmptyBinKey =
266260
Metadata.Key.of("keep-empty-bin-key-bin", Metadata.BINARY_BYTE_MARSHALLER);
267261
Metadata.Key<byte[]> ignoreEmptyBinKey =
268262
Metadata.Key.of("ignore-empty-bin-key-bin", Metadata.BINARY_BYTE_MARSHALLER);
269263

270-
assertThat(headers.containsKey(keepEmptyBinKey)).isTrue();
271264
assertThat(headers.get(keepEmptyBinKey)).isEqualTo(new byte[0]);
272-
assertThat(headers.containsKey(ignoreEmptyBinKey)).isFalse();
273-
assertThat(headers.get(overwriteEmptyBinKey)).isEqualTo(originalBinValue);
265+
assertThat(headers.get(ignoreEmptyBinKey)).isEqualTo(new byte[0]);
266+
assertThat(headers.get(overwriteEmptyBinKey)).isEqualTo(new byte[0]);
274267
}
275268

276269
@Test
@@ -290,7 +283,7 @@ public void applyMutations_binaryRemoval() {
290283
public void applyMutations_stringValueWithBinaryKey_ignored() {
291284
Metadata headers = new Metadata();
292285
HeaderValueOption option = HeaderValueOption.create(HeaderValue.create("some-key-bin", "value"),
293-
HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, false);
286+
HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD);
294287

295288
headerMutator.applyMutations(
296289
HeaderMutations.create(ImmutableList.of(option), ImmutableList.of()), headers);
@@ -304,7 +297,7 @@ public void applyMutations_binaryValueWithAsciiKey_ignored() {
304297
Metadata headers = new Metadata();
305298
HeaderValueOption option = HeaderValueOption.create(
306299
HeaderValue.create("some-key", ByteString.copyFrom(new byte[] {1})),
307-
HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, false);
300+
HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD);
308301

309302
headerMutator.applyMutations(
310303
HeaderMutations.create(ImmutableList.of(option), ImmutableList.of()), headers);

‎xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderValueOptionTest.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,10 +31,9 @@ public class HeaderValueOptionTest {
3131
public void create_withAllFields_success() {
3232
HeaderValue header = HeaderValue.create("key1", "value1");
3333
HeaderValueOption option = HeaderValueOption.create(
34-
header, HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, true);
34+
header, HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD);
3535

3636
assertThat(option.header()).isEqualTo(header);
3737
assertThat(option.appendAction()).isEqualTo(HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD);
38-
assertThat(option.keepEmptyValue()).isTrue();
3938
}
4039
}

0 commit comments

Comments
 (0)