From 6561e32ddf32a5bfba497d8a951bd6aeedfdc037 Mon Sep 17 00:00:00 2001 From: Saurav Date: Fri, 24 Oct 2025 13:58:34 +0000 Subject: [PATCH 01/12] feat(xds): Add header mutations library This commit introduces a library for handling header mutations as specified by the xDS protocol. This library provides the core functionality for modifying request and response headers based on a set of rules. The main components of this library are: - `HeaderMutator`: Applies header mutations to `Metadata` objects. - `HeaderMutationFilter`: Filters header mutations based on a set of configurable rules, such as disallowing mutations of system headers. - `HeaderMutations`: A value class that represents the set of mutations to be applied to request and response headers. - `HeaderMutationDisallowedException`: An exception that is thrown when a disallowed header mutation is attempted. This commit also includes comprehensive unit tests for the new library. --- .../HeaderMutationDisallowedException.java | 32 ++ .../headermutations/HeaderMutationFilter.java | 172 ++++++++++ .../headermutations/HeaderMutations.java | 58 ++++ .../headermutations/HeaderMutator.java | 143 ++++++++ .../headermutations/HeaderValueOption.java | 50 +++ .../HeaderMutationFilterTest.java | 245 ++++++++++++++ .../headermutations/HeaderMutationsTest.java | 50 +++ .../headermutations/HeaderMutatorTest.java | 311 ++++++++++++++++++ .../HeaderValueOptionTest.java | 40 +++ 9 files changed, 1101 insertions(+) create mode 100644 xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationDisallowedException.java create mode 100644 xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java create mode 100644 xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutations.java create mode 100644 xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutator.java create mode 100644 xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderValueOption.java create mode 100644 xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutationFilterTest.java create mode 100644 xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutationsTest.java create mode 100644 xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutatorTest.java create mode 100644 xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderValueOptionTest.java diff --git a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationDisallowedException.java b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationDisallowedException.java new file mode 100644 index 00000000000..b8d4eb582fb --- /dev/null +++ b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationDisallowedException.java @@ -0,0 +1,32 @@ +/* + * Copyright 2024 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.xds.internal.headermutations; + +import io.grpc.Status; +import io.grpc.StatusException; + +/** + * Exception thrown when a header mutation is disallowed. + */ +public final class HeaderMutationDisallowedException extends StatusException { + + private static final long serialVersionUID = 1L; + + public HeaderMutationDisallowedException(String message) { + super(Status.INTERNAL.withDescription(message)); + } +} diff --git a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java new file mode 100644 index 00000000000..0452354d823 --- /dev/null +++ b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java @@ -0,0 +1,172 @@ +/* + * Copyright 2025 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.xds.internal.headermutations; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import io.envoyproxy.envoy.config.core.v3.HeaderValueOption; +import io.grpc.xds.internal.headermutations.HeaderMutations.RequestHeaderMutations; +import io.grpc.xds.internal.headermutations.HeaderMutations.ResponseHeaderMutations; +import java.util.Collection; +import java.util.Locale; +import java.util.Optional; +import java.util.function.Predicate; + +/** + * The HeaderMutationFilter class is responsible for filtering header mutations based on a given set + * of rules. + */ +public interface HeaderMutationFilter { + + /** + * A factory for creating {@link HeaderMutationFilter} instances. + */ + @FunctionalInterface + interface Factory { + /** + * Creates a new instance of {@code HeaderMutationFilter}. + * + * @param mutationRules The rules for header mutations. If an empty {@code Optional} is + * provided, all header mutations are allowed by default, except for certain system + * headers. If a {@link HeaderMutationRulesConfig} is provided, mutations will be + * filtered based on the specified rules. + */ + HeaderMutationFilter create(Optional mutationRules); + } + + /** + * The default factory for creating {@link HeaderMutationFilter} instances. + */ + Factory INSTANCE = HeaderMutationFilterImpl::new; + + /** + * Filters the given header mutations based on the configured rules and returns the allowed + * mutations. + * + * @param mutations The header mutations to filter + * @return The allowed header mutations. + * @throws HeaderMutationDisallowedException if a disallowed mutation is encountered and the rules + * specify that this should be an error. + */ + HeaderMutations filter(HeaderMutations mutations) throws HeaderMutationDisallowedException; + + /** Default implementation of {@link HeaderMutationFilter}. */ + final class HeaderMutationFilterImpl implements HeaderMutationFilter { + private final Optional mutationRules; + + /** + * Set of HTTP/2 pseudo-headers and the host header that are critical for routing and protocol + * correctness. These headers cannot be mutated by user configuration. + */ + private static final ImmutableSet IMMUTABLE_HEADERS = + ImmutableSet.of("host", ":authority", ":scheme", ":method"); + + private HeaderMutationFilterImpl(Optional mutationRules) { // NOPMD + this.mutationRules = mutationRules; + } + + @Override + public HeaderMutations filter(HeaderMutations mutations) + throws HeaderMutationDisallowedException { + ImmutableList allowedRequestHeaders = + filterCollection(mutations.requestMutations().headers(), + header -> isHeaderMutationAllowed(header.getHeader().getKey()) + && !appendsSystemHeader(header)); + ImmutableList allowedRequestHeadersToRemove = + filterCollection(mutations.requestMutations().headersToRemove(), + header -> isHeaderMutationAllowed(header) && isHeaderRemovalAllowed(header)); + ImmutableList allowedResponseHeaders = + filterCollection(mutations.responseMutations().headers(), + header -> isHeaderMutationAllowed(header.getHeader().getKey()) + && !appendsSystemHeader(header)); + return HeaderMutations.create( + RequestHeaderMutations.create(allowedRequestHeaders, allowedRequestHeadersToRemove), + ResponseHeaderMutations.create(allowedResponseHeaders)); + } + + /** + * A generic helper to filter a collection based on a predicate. + * + * @param items The collection of items to filter. + * @param isAllowedPredicate The predicate to apply to each item. + * @param The type of items in the collection. + * @return An immutable list of allowed items. + * @throws HeaderMutationDisallowedException if an item is disallowed and disallowIsError is + * true. + */ + private ImmutableList filterCollection(Collection items, + Predicate isAllowedPredicate) throws HeaderMutationDisallowedException { + ImmutableList.Builder allowed = ImmutableList.builder(); + for (T item : items) { + if (isAllowedPredicate.test(item)) { + allowed.add(item); + } else if (disallowIsError()) { + throw new HeaderMutationDisallowedException( + "Header mutation disallowed for header: " + item); + } + } + return allowed.build(); + } + + private boolean isHeaderRemovalAllowed(String headerKey) { + return !isSystemHeaderKey(headerKey); + } + + private boolean appendsSystemHeader(HeaderValueOption headerValueOption) { + String key = headerValueOption.getHeader().getKey(); + boolean isAppend = headerValueOption + .getAppendAction() == HeaderValueOption.HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD; + return isAppend && isSystemHeaderKey(key); + } + + private boolean isSystemHeaderKey(String key) { + return key.startsWith(":") || key.toLowerCase(Locale.ROOT).equals("host"); + } + + private boolean isHeaderMutationAllowed(String headerName) { + String lowerCaseHeaderName = headerName.toLowerCase(Locale.ROOT); + if (IMMUTABLE_HEADERS.contains(lowerCaseHeaderName)) { + return false; + } + return mutationRules.map(rules -> isHeaderMutationAllowed(lowerCaseHeaderName, rules)) + .orElse(true); + } + + private boolean isHeaderMutationAllowed(String lowerCaseHeaderName, + HeaderMutationRulesConfig rules) { + // TODO(sauravzg): The priority is slightly unclear in the spec. + // Both `disallowAll` and `disallow_expression` take precedence over `all other + // settings`. + // `allow_expression` takes precedence over everything except `disallow_expression`. + // This is a conflict between ordering for `allow_expression` and `disallowAll`. + // Choosing to proceed with current envoy implementation which favors `allow_expression` over + // `disallowAll`. + if (rules.disallowExpression().isPresent() + && rules.disallowExpression().get().matcher(lowerCaseHeaderName).matches()) { + return false; + } + if (rules.allowExpression().isPresent()) { + return rules.allowExpression().get().matcher(lowerCaseHeaderName).matches(); + } + return !rules.disallowAll(); + } + + private boolean disallowIsError() { + return mutationRules.map(HeaderMutationRulesConfig::disallowIsError).orElse(false); + } + } +} diff --git a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutations.java b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutations.java new file mode 100644 index 00000000000..e0cb3daede3 --- /dev/null +++ b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutations.java @@ -0,0 +1,58 @@ +/* + * Copyright 2025 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.xds.internal.headermutations; + +import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableList; +import io.envoyproxy.envoy.config.core.v3.HeaderValueOption; + +/** A collection of header mutations for both request and response headers. */ +@AutoValue +public abstract class HeaderMutations { + + public static HeaderMutations create(RequestHeaderMutations requestMutations, + ResponseHeaderMutations responseMutations) { + return new AutoValue_HeaderMutations(requestMutations, responseMutations); + } + + public abstract RequestHeaderMutations requestMutations(); + + public abstract ResponseHeaderMutations responseMutations(); + + /** Represents mutations for request headers. */ + @AutoValue + public abstract static class RequestHeaderMutations { + public static RequestHeaderMutations create(ImmutableList headers, + ImmutableList headersToRemove) { + return new AutoValue_HeaderMutations_RequestHeaderMutations(headers, headersToRemove); + } + + public abstract ImmutableList headers(); + + public abstract ImmutableList headersToRemove(); + } + + /** Represents mutations for response headers. */ + @AutoValue + public abstract static class ResponseHeaderMutations { + public static ResponseHeaderMutations create(ImmutableList headers) { + return new AutoValue_HeaderMutations_ResponseHeaderMutations(headers); + } + + public abstract ImmutableList headers(); + } +} 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 new file mode 100644 index 00000000000..de5b946bbc7 --- /dev/null +++ b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutator.java @@ -0,0 +1,143 @@ +/* + * Copyright 2025 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.xds.internal.headermutations; + +import com.google.common.io.BaseEncoding; +import io.envoyproxy.envoy.config.core.v3.HeaderValue; +import io.envoyproxy.envoy.config.core.v3.HeaderValueOption; +import io.envoyproxy.envoy.config.core.v3.HeaderValueOption.HeaderAppendAction; +import io.grpc.Metadata; +import io.grpc.xds.internal.headermutations.HeaderMutations.RequestHeaderMutations; +import io.grpc.xds.internal.headermutations.HeaderMutations.ResponseHeaderMutations; +import java.nio.charset.StandardCharsets; +import java.util.logging.Logger; + +/** + * The HeaderMutator class is an implementation of the HeaderMutator interface. It provides methods + * to apply header mutations to a given set of headers based on a given set of rules. + */ +public interface HeaderMutator { + /** + * Creates a new instance of {@code HeaderMutator}. + */ + static HeaderMutator create() { + return new HeaderMutatorImpl(); + } + + /** + * Applies the given header mutations to the provided metadata headers. + * + * @param mutations The header mutations to apply. + * @param headers The metadata headers to which the mutations will be applied. + */ + void applyRequestMutations(RequestHeaderMutations mutations, Metadata headers); + + + /** + * Applies the given header mutations to the provided metadata headers. + * + * @param mutations The header mutations to apply. + * @param headers The metadata headers to which the mutations will be applied. + */ + void applyResponseMutations(ResponseHeaderMutations mutations, Metadata headers); + + /** Default implementation of {@link HeaderMutator}. */ + final class HeaderMutatorImpl implements HeaderMutator { + + private static final Logger logger = Logger.getLogger(HeaderMutatorImpl.class.getName()); + + @Override + public void applyRequestMutations(final RequestHeaderMutations mutations, Metadata headers) { + // TODO(sauravzg): The specification is not clear on order of header removals and additions. + // in case of conflicts. Copying the order from Envoy here, which does removals at the end. + applyHeaderUpdates(mutations.headers(), headers); + for (String headerToRemove : mutations.headersToRemove()) { + headers.discardAll(Metadata.Key.of(headerToRemove, Metadata.ASCII_STRING_MARSHALLER)); + } + } + + @Override + public void applyResponseMutations(final ResponseHeaderMutations mutations, Metadata headers) { + applyHeaderUpdates(mutations.headers(), headers); + } + + private void applyHeaderUpdates(final Iterable headerOptions, + Metadata headers) { + for (HeaderValueOption headerOption : headerOptions) { + HeaderValue headerValue = headerOption.getHeader(); + updateHeader(headerValue, headerOption.getAppendAction(), headers); + } + } + + private void updateHeader(final HeaderValue header, final HeaderAppendAction action, + Metadata mutableHeaders) { + if (header.getKey().endsWith(Metadata.BINARY_HEADER_SUFFIX)) { + updateHeader(action, Metadata.Key.of(header.getKey(), Metadata.BINARY_BYTE_MARSHALLER), + getBinaryHeaderValue(header), mutableHeaders); + } else { + updateHeader(action, Metadata.Key.of(header.getKey(), Metadata.ASCII_STRING_MARSHALLER), + getAsciiValue(header), mutableHeaders); + } + } + + private void updateHeader(final HeaderAppendAction action, final Metadata.Key key, + final T value, Metadata mutableHeaders) { + switch (action) { + case APPEND_IF_EXISTS_OR_ADD: + mutableHeaders.put(key, value); + break; + case ADD_IF_ABSENT: + if (!mutableHeaders.containsKey(key)) { + mutableHeaders.put(key, value); + } + break; + case OVERWRITE_IF_EXISTS_OR_ADD: + mutableHeaders.discardAll(key); + mutableHeaders.put(key, value); + break; + case OVERWRITE_IF_EXISTS: + if (mutableHeaders.containsKey(key)) { + mutableHeaders.discardAll(key); + mutableHeaders.put(key, value); + } + break; + case UNRECOGNIZED: + // Ignore invalid value + logger.warning("Unrecognized HeaderAppendAction: " + action); + break; + default: + // Should be unreachable unless there's a proto schema mismatch. + logger.warning("Unknown HeaderAppendAction: " + action); + } + } + + private byte[] getBinaryHeaderValue(HeaderValue header) { + return BaseEncoding.base64().decode(getAsciiValue(header)); + } + + private String getAsciiValue(HeaderValue header) { + // TODO(sauravzg): GRPC only supports base64 encoded binary headers, so we decode bytes to + // String using `StandardCharsets.US_ASCII`. + // Envoy's spec `raw_value` specification can contain non UTF-8 bytes, so this may potentially + // cause an exception or corruption. + if (!header.getRawValue().isEmpty()) { + return header.getRawValue().toString(StandardCharsets.US_ASCII); + } + return header.getValue(); + } + } +} 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 new file mode 100644 index 00000000000..6cb96da864d --- /dev/null +++ b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderValueOption.java @@ -0,0 +1,50 @@ +/* + * Copyright 2026 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.xds.internal.headermutations; + +import com.google.auto.value.AutoValue; +import io.grpc.xds.internal.grpcservice.HeaderValue; + +/** + * Represents a header option to be appended or mutated as part of xDS configuration. + * Avoids direct dependency on Envoy's proto objects. + */ +@AutoValue +public abstract class HeaderValueOption { + + public static HeaderValueOption create( + HeaderValue header, HeaderAppendAction appendAction, boolean keepEmptyValue) { + return new AutoValue_HeaderValueOption(header, appendAction, keepEmptyValue); + } + + 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. + */ + public enum HeaderAppendAction { + APPEND_IF_EXISTS_OR_ADD, + ADD_IF_ABSENT, + OVERWRITE_IF_EXISTS_OR_ADD, + OVERWRITE_IF_EXISTS + } +} 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 new file mode 100644 index 00000000000..e73460924c7 --- /dev/null +++ b/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutationFilterTest.java @@ -0,0 +1,245 @@ +/* + * Copyright 2025 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.xds.internal.headermutations; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableList; +import io.envoyproxy.envoy.config.core.v3.HeaderValue; +import io.envoyproxy.envoy.config.core.v3.HeaderValueOption; +import io.envoyproxy.envoy.config.core.v3.HeaderValueOption.HeaderAppendAction; +import io.grpc.xds.internal.headermutations.HeaderMutations.RequestHeaderMutations; +import io.grpc.xds.internal.headermutations.HeaderMutations.ResponseHeaderMutations; +import java.util.Optional; +import java.util.regex.Pattern; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class HeaderMutationFilterTest { + + private static HeaderValueOption header(String key, String value) { + return HeaderValueOption.newBuilder() + .setHeader(HeaderValue.newBuilder().setKey(key).setValue(value)).build(); + } + + private static HeaderValueOption header(String key, String value, HeaderAppendAction action) { + return HeaderValueOption.newBuilder() + .setHeader(HeaderValue.newBuilder().setKey(key).setValue(value)).setAppendAction(action) + .build(); + } + + @Test + public void filter_removesImmutableHeaders() throws HeaderMutationDisallowedException { + HeaderMutationFilter filter = HeaderMutationFilter.INSTANCE.create(Optional.empty()); + HeaderMutations mutations = HeaderMutations.create( + RequestHeaderMutations.create( + ImmutableList.of(header("add-key", "add-value"), header(":authority", "new-authority"), + header("host", "new-host"), header(":scheme", "https"), header(":method", "PUT")), + ImmutableList.of("remove-key", "host", ":authority", ":scheme", ":method")), + ResponseHeaderMutations.create(ImmutableList.of(header("resp-add-key", "resp-add-value"), + header(":scheme", "https")))); + + HeaderMutations filtered = filter.filter(mutations); + + assertThat(filtered.requestMutations().headers()) + .containsExactly(header("add-key", "add-value")); + assertThat(filtered.requestMutations().headersToRemove()).containsExactly("remove-key"); + assertThat(filtered.responseMutations().headers()) + .containsExactly(header("resp-add-key", "resp-add-value")); + } + + @Test + public void filter_cannotAppendToSystemHeaders() throws HeaderMutationDisallowedException { + HeaderMutationFilter filter = HeaderMutationFilter.INSTANCE.create(Optional.empty()); + HeaderMutations mutations = + HeaderMutations.create( + RequestHeaderMutations.create( + ImmutableList.of( + header("add-key", "add-value", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), + header(":authority", "new-authority", + HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), + header("host", "new-host", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), + header(":path", "/new-path", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD)), + ImmutableList.of()), + ResponseHeaderMutations.create(ImmutableList + .of(header("host", "new-host", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD)))); + + HeaderMutations filtered = filter.filter(mutations); + + assertThat(filtered.requestMutations().headers()).containsExactly( + header("add-key", "add-value", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD)); + assertThat(filtered.responseMutations().headers()).isEmpty(); + } + + @Test + public void filter_cannotRemoveSystemHeaders() throws HeaderMutationDisallowedException { + HeaderMutationFilter filter = HeaderMutationFilter.INSTANCE.create(Optional.empty()); + HeaderMutations mutations = HeaderMutations.create( + RequestHeaderMutations.create(ImmutableList.of(), + ImmutableList.of("remove-key", "host", ":foo", ":bar")), + ResponseHeaderMutations.create(ImmutableList.of())); + + HeaderMutations filtered = filter.filter(mutations); + + assertThat(filtered.requestMutations().headersToRemove()).containsExactly("remove-key"); + } + + @Test + public void filter_canOverrideSystemHeadersNotInImmutableHeaders() + throws HeaderMutationDisallowedException { + HeaderMutationFilter filter = HeaderMutationFilter.INSTANCE.create(Optional.empty()); + HeaderMutations mutations = HeaderMutations.create( + RequestHeaderMutations.create( + ImmutableList.of(header("user-agent", "new-agent"), + header(":path", "/new/path", HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD), + header(":grpc-trace-bin", "binary-value", HeaderAppendAction.ADD_IF_ABSENT)), + ImmutableList.of()), + ResponseHeaderMutations.create(ImmutableList + .of(header(":alt-svc", "h3=:443", HeaderAppendAction.OVERWRITE_IF_EXISTS)))); + + HeaderMutations filtered = filter.filter(mutations); + + assertThat(filtered.requestMutations().headers()).containsExactly( + header("user-agent", "new-agent"), + header(":path", "/new/path", HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD), + header(":grpc-trace-bin", "binary-value", HeaderAppendAction.ADD_IF_ABSENT)); + assertThat(filtered.responseMutations().headers()) + .containsExactly(header(":alt-svc", "h3=:443", HeaderAppendAction.OVERWRITE_IF_EXISTS)); + } + + @Test + public void filter_disallowAll_disablesAllModifications() + throws HeaderMutationDisallowedException { + HeaderMutationRulesConfig rules = HeaderMutationRulesConfig.builder().disallowAll(true).build(); + HeaderMutationFilter filter = HeaderMutationFilter.INSTANCE.create(Optional.of(rules)); + HeaderMutations mutations = HeaderMutations.create( + RequestHeaderMutations.create(ImmutableList.of(header("add-key", "add-value")), + ImmutableList.of("remove-key")), + ResponseHeaderMutations.create(ImmutableList.of(header("resp-add-key", "resp-add-value")))); + + HeaderMutations filtered = filter.filter(mutations); + + assertThat(filtered.requestMutations().headers()).isEmpty(); + assertThat(filtered.requestMutations().headersToRemove()).isEmpty(); + assertThat(filtered.responseMutations().headers()).isEmpty(); + } + + @Test + public void filter_disallowExpression_filtersRelevantExpressions() + throws HeaderMutationDisallowedException { + HeaderMutationRulesConfig rules = HeaderMutationRulesConfig.builder() + .disallowExpression(Pattern.compile("^x-private-.*")).build(); + HeaderMutationFilter filter = HeaderMutationFilter.INSTANCE.create(Optional.of(rules)); + HeaderMutations mutations = HeaderMutations.create( + RequestHeaderMutations.create( + ImmutableList.of(header("x-public", "value"), header("x-private-key", "value")), + ImmutableList.of("x-public-remove", "x-private-remove")), + ResponseHeaderMutations.create( + ImmutableList.of(header("x-public-resp", "value"), header("x-private-resp", "value")))); + + HeaderMutations filtered = filter.filter(mutations); + + assertThat(filtered.requestMutations().headers()).containsExactly(header("x-public", "value")); + assertThat(filtered.requestMutations().headersToRemove()).containsExactly("x-public-remove"); + assertThat(filtered.responseMutations().headers()) + .containsExactly(header("x-public-resp", "value")); + } + + @Test + public void filter_allowExpression_onlyAllowsRelevantExpressions() + throws HeaderMutationDisallowedException { + HeaderMutationRulesConfig rules = HeaderMutationRulesConfig.builder() + .allowExpression(Pattern.compile("^x-allowed-.*")).build(); + HeaderMutationFilter filter = HeaderMutationFilter.INSTANCE.create(Optional.of(rules)); + HeaderMutations mutations = + HeaderMutations.create( + RequestHeaderMutations.create( + ImmutableList.of(header("x-allowed-key", "value"), + header("not-allowed-key", "value")), + ImmutableList.of("x-allowed-remove", "not-allowed-remove")), + ResponseHeaderMutations.create(ImmutableList.of(header("x-allowed-resp-key", "value"), + header("not-allowed-resp-key", "value")))); + + HeaderMutations filtered = filter.filter(mutations); + + assertThat(filtered.requestMutations().headers()) + .containsExactly(header("x-allowed-key", "value")); + assertThat(filtered.requestMutations().headersToRemove()).containsExactly("x-allowed-remove"); + assertThat(filtered.responseMutations().headers()) + .containsExactly(header("x-allowed-resp-key", "value")); + } + + @Test + public void filter_allowExpression_overridesDisallowAll() + throws HeaderMutationDisallowedException { + HeaderMutationRulesConfig rules = HeaderMutationRulesConfig.builder().disallowAll(true) + .allowExpression(Pattern.compile("^x-allowed-.*")).build(); + HeaderMutationFilter filter = HeaderMutationFilter.INSTANCE.create(Optional.of(rules)); + HeaderMutations mutations = HeaderMutations.create( + RequestHeaderMutations.create( + ImmutableList.of(header("x-allowed-key", "value"), header("not-allowed", "value")), + ImmutableList.of("x-allowed-remove", "not-allowed-remove")), + ResponseHeaderMutations.create(ImmutableList.of(header("x-allowed-resp-key", "value"), + header("not-allowed-resp-key", "value")))); + + HeaderMutations filtered = filter.filter(mutations); + + assertThat(filtered.requestMutations().headers()) + .containsExactly(header("x-allowed-key", "value")); + assertThat(filtered.requestMutations().headersToRemove()).containsExactly("x-allowed-remove"); + assertThat(filtered.responseMutations().headers()) + .containsExactly(header("x-allowed-resp-key", "value")); + } + + @Test(expected = HeaderMutationDisallowedException.class) + public void filter_disallowIsError_throwsExceptionOnDisallowed() + throws HeaderMutationDisallowedException { + HeaderMutationRulesConfig rules = + HeaderMutationRulesConfig.builder().disallowAll(true).disallowIsError(true).build(); + HeaderMutationFilter filter = HeaderMutationFilter.INSTANCE.create(Optional.of(rules)); + HeaderMutations mutations = HeaderMutations.create(RequestHeaderMutations + .create(ImmutableList.of(header("add-key", "add-value")), ImmutableList.of()), + ResponseHeaderMutations.create(ImmutableList.of())); + filter.filter(mutations); + } + + @Test(expected = HeaderMutationDisallowedException.class) + public void filter_disallowIsError_throwsExceptionOnDisallowedRemove() + throws HeaderMutationDisallowedException { + HeaderMutationRulesConfig rules = + HeaderMutationRulesConfig.builder().disallowAll(true).disallowIsError(true).build(); + HeaderMutationFilter filter = HeaderMutationFilter.INSTANCE.create(Optional.of(rules)); + HeaderMutations mutations = HeaderMutations.create( + RequestHeaderMutations.create(ImmutableList.of(), ImmutableList.of("remove-key")), + ResponseHeaderMutations.create(ImmutableList.of())); + filter.filter(mutations); + } + + @Test(expected = HeaderMutationDisallowedException.class) + public void filter_disallowIsError_throwsExceptionOnDisallowedResponseHeader() + throws HeaderMutationDisallowedException { + HeaderMutationRulesConfig rules = + HeaderMutationRulesConfig.builder().disallowAll(true).disallowIsError(true).build(); + HeaderMutationFilter filter = HeaderMutationFilter.INSTANCE.create(Optional.of(rules)); + HeaderMutations mutations = HeaderMutations.create( + RequestHeaderMutations.create(ImmutableList.of(), ImmutableList.of()), + ResponseHeaderMutations.create(ImmutableList.of(header("resp-add-key", "resp-add-value")))); + filter.filter(mutations); + } +} 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 new file mode 100644 index 00000000000..f1dc0561692 --- /dev/null +++ b/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutationsTest.java @@ -0,0 +1,50 @@ +/* + * Copyright 2025 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.xds.internal.headermutations; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableList; +import io.envoyproxy.envoy.config.core.v3.HeaderValue; +import io.envoyproxy.envoy.config.core.v3.HeaderValueOption; +import io.grpc.xds.internal.headermutations.HeaderMutations.RequestHeaderMutations; +import io.grpc.xds.internal.headermutations.HeaderMutations.ResponseHeaderMutations; +import org.junit.Test; + +public class HeaderMutationsTest { + @Test + public void testCreate() { + HeaderValueOption reqHeader = HeaderValueOption.newBuilder() + .setHeader(HeaderValue.newBuilder().setKey("req-key").setValue("req-value").build()) + .build(); + RequestHeaderMutations requestMutations = RequestHeaderMutations + .create(ImmutableList.of(reqHeader), ImmutableList.of("remove-req-key")); + assertThat(requestMutations.headers()).containsExactly(reqHeader); + assertThat(requestMutations.headersToRemove()).containsExactly("remove-req-key"); + + HeaderValueOption respHeader = HeaderValueOption.newBuilder() + .setHeader(HeaderValue.newBuilder().setKey("resp-key").setValue("resp-value").build()) + .build(); + ResponseHeaderMutations responseMutations = + ResponseHeaderMutations.create(ImmutableList.of(respHeader)); + assertThat(responseMutations.headers()).containsExactly(respHeader); + + HeaderMutations mutations = HeaderMutations.create(requestMutations, responseMutations); + assertThat(mutations.requestMutations()).isEqualTo(requestMutations); + assertThat(mutations.responseMutations()).isEqualTo(responseMutations); + } +} 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 new file mode 100644 index 00000000000..df6ce383d8c --- /dev/null +++ b/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutatorTest.java @@ -0,0 +1,311 @@ +/* + * Copyright 2025 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.xds.internal.headermutations; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableList; +import com.google.common.io.BaseEncoding; +import com.google.common.testing.TestLogHandler; +import com.google.protobuf.ByteString; +import io.envoyproxy.envoy.config.core.v3.HeaderValue; +import io.envoyproxy.envoy.config.core.v3.HeaderValueOption; +import io.envoyproxy.envoy.config.core.v3.HeaderValueOption.HeaderAppendAction; +import io.grpc.Metadata; +import io.grpc.xds.internal.headermutations.HeaderMutations.RequestHeaderMutations; +import io.grpc.xds.internal.headermutations.HeaderMutations.ResponseHeaderMutations; +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.logging.Level; +import java.util.logging.LogRecord; +import java.util.logging.Logger; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class HeaderMutatorTest { + + private static final Metadata.Key ASCII_KEY = + Metadata.Key.of("some-key", Metadata.ASCII_STRING_MARSHALLER); + private static final Metadata.Key BINARY_KEY = + Metadata.Key.of("some-key-bin", Metadata.BINARY_BYTE_MARSHALLER); + private static final Metadata.Key APPEND_KEY = + Metadata.Key.of("append-key", Metadata.ASCII_STRING_MARSHALLER); + private static final Metadata.Key ADD_KEY = + Metadata.Key.of("add-key", Metadata.ASCII_STRING_MARSHALLER); + private static final Metadata.Key OVERWRITE_KEY = + Metadata.Key.of("overwrite-key", Metadata.ASCII_STRING_MARSHALLER); + private static final Metadata.Key REMOVE_KEY = + Metadata.Key.of("remove-key", Metadata.ASCII_STRING_MARSHALLER); + private static final Metadata.Key NEW_ADD_KEY = + Metadata.Key.of("new-add-key", Metadata.ASCII_STRING_MARSHALLER); + private static final Metadata.Key NEW_OVERWRITE_KEY = + Metadata.Key.of("new-overwrite-key", Metadata.ASCII_STRING_MARSHALLER); + private static final Metadata.Key OVERWRITE_IF_EXISTS_KEY = + Metadata.Key.of("overwrite-if-exists-key", Metadata.ASCII_STRING_MARSHALLER); + private static final Metadata.Key OVERWRITE_IF_EXISTS_ABSENT_KEY = + Metadata.Key.of("overwrite-if-exists-absent-key", Metadata.ASCII_STRING_MARSHALLER); + + private final HeaderMutator headerMutator = HeaderMutator.create(); + + private static final TestLogHandler logHandler = new TestLogHandler(); + private static final Logger logger = + Logger.getLogger(HeaderMutator.HeaderMutatorImpl.class.getName()); + + @Before + public void setUp() { + logHandler.clear(); + logger.addHandler(logHandler); + logger.setLevel(Level.WARNING); + } + + @After + public void tearDown() { + logger.removeHandler(logHandler); + } + + private static HeaderValueOption header(String key, String value, HeaderAppendAction action) { + return HeaderValueOption.newBuilder() + .setHeader(HeaderValue.newBuilder().setKey(key).setValue(value)).setAppendAction(action) + .build(); + } + + @Test + public void applyRequestMutations_asciiHeaders() { + Metadata headers = new Metadata(); + headers.put(APPEND_KEY, "append-value-1"); + headers.put(ADD_KEY, "add-value-original"); + headers.put(OVERWRITE_KEY, "overwrite-value-original"); + headers.put(REMOVE_KEY, "remove-value-original"); + headers.put(OVERWRITE_IF_EXISTS_KEY, "original-value"); + + RequestHeaderMutations mutations = RequestHeaderMutations.create(ImmutableList.of( + // Append to existing header + header(APPEND_KEY.name(), "append-value-2", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), + // Try to add to an existing header (should be no-op) + header(ADD_KEY.name(), "add-value-new", HeaderAppendAction.ADD_IF_ABSENT), + // Add a new header + header(NEW_ADD_KEY.name(), "new-add-value", HeaderAppendAction.ADD_IF_ABSENT), + // Overwrite an existing header + header(OVERWRITE_KEY.name(), "overwrite-value-new", + HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD), + // Overwrite a new header + header(NEW_OVERWRITE_KEY.name(), "new-overwrite-value", + HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD), + // Overwrite an existing header if it exists + header(OVERWRITE_IF_EXISTS_KEY.name(), "new-value", HeaderAppendAction.OVERWRITE_IF_EXISTS), + // Try to overwrite a header that does not exist + header(OVERWRITE_IF_EXISTS_ABSENT_KEY.name(), "new-value", + HeaderAppendAction.OVERWRITE_IF_EXISTS)), + ImmutableList.of(REMOVE_KEY.name())); + + headerMutator.applyRequestMutations(mutations, headers); + + assertThat(headers.getAll(APPEND_KEY)).containsExactly("append-value-1", "append-value-2"); + assertThat(headers.get(ADD_KEY)).isEqualTo("add-value-original"); + assertThat(headers.get(NEW_ADD_KEY)).isEqualTo("new-add-value"); + assertThat(headers.get(OVERWRITE_KEY)).isEqualTo("overwrite-value-new"); + assertThat(headers.get(NEW_OVERWRITE_KEY)).isEqualTo("new-overwrite-value"); + assertThat(headers.containsKey(REMOVE_KEY)).isFalse(); + assertThat(headers.get(OVERWRITE_IF_EXISTS_KEY)).isEqualTo("new-value"); + assertThat(headers.containsKey(OVERWRITE_IF_EXISTS_ABSENT_KEY)).isFalse(); + } + + @Test + public void applyRequestMutations_InvalidAppendAction_isIgnored() { + Metadata headers = new Metadata(); + headers.put(ASCII_KEY, "value1"); + headerMutator + .applyRequestMutations( + RequestHeaderMutations + .create( + ImmutableList.of( + HeaderValueOption.newBuilder() + .setHeader(HeaderValue.newBuilder().setKey(ASCII_KEY.name()) + .setValue("value2")) + .setAppendActionValue(-1).build(), + HeaderValueOption.newBuilder() + .setHeader(HeaderValue.newBuilder().setKey(BINARY_KEY.name()) + .setValue("value2")) + .setAppendActionValue(-5).build()), + ImmutableList.of()), + headers); + assertThat(headers.getAll(ASCII_KEY)).containsExactly("value1"); + } + + @Test + public void applyRequestMutations_removalHasPriority() { + Metadata headers = new Metadata(); + headers.put(REMOVE_KEY, "value"); + RequestHeaderMutations mutations = RequestHeaderMutations.create( + ImmutableList.of( + header(REMOVE_KEY.name(), "new-value", HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD)), + ImmutableList.of(REMOVE_KEY.name())); + + headerMutator.applyRequestMutations(mutations, headers); + + assertThat(headers.containsKey(REMOVE_KEY)).isFalse(); + } + + @Test + public void applyRequestMutations_binary_withBase64RawValue() { + Metadata headers = new Metadata(); + byte[] value = new byte[] {1, 2, 3}; + HeaderValueOption option = HeaderValueOption.newBuilder() + .setHeader(HeaderValue.newBuilder().setKey(BINARY_KEY.name()).setRawValue( + ByteString.copyFrom(BaseEncoding.base64().encode(value), StandardCharsets.US_ASCII))) + .setAppendAction(HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD).build(); + headerMutator.applyRequestMutations( + RequestHeaderMutations.create(ImmutableList.of(option), ImmutableList.of()), headers); + assertThat(headers.get(BINARY_KEY)).isEqualTo(value); + } + + @Test + public void applyRequestMutations_binary_withBase64Value() { + Metadata headers = new Metadata(); + byte[] value = new byte[] {1, 2, 3}; + String base64Value = BaseEncoding.base64().encode(value); + HeaderValueOption option = HeaderValueOption.newBuilder() + .setHeader(HeaderValue.newBuilder().setKey(BINARY_KEY.name()).setValue(base64Value)) + .setAppendAction(HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD).build(); + + headerMutator.applyRequestMutations( + RequestHeaderMutations.create(ImmutableList.of(option), ImmutableList.of()), headers); + assertThat(headers.get(BINARY_KEY)).isEqualTo(value); + } + + @Test + public void applyRequestMutations_ascii_withRawValue() { + Metadata headers = new Metadata(); + byte[] value = "raw-value".getBytes(StandardCharsets.US_ASCII); + HeaderValueOption option = HeaderValueOption.newBuilder() + .setHeader(HeaderValue.newBuilder().setKey(ASCII_KEY.name()) + .setRawValue(ByteString.copyFrom(value))) + .setAppendAction(HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD).build(); + headerMutator.applyRequestMutations( + RequestHeaderMutations.create(ImmutableList.of(option), ImmutableList.of()), headers); + assertThat(headers.get(Metadata.Key.of(ASCII_KEY.name(), Metadata.ASCII_STRING_MARSHALLER))) + .isEqualTo("raw-value"); + } + + @Test + public void applyResponseMutations_asciiHeaders() { + Metadata headers = new Metadata(); + headers.put(APPEND_KEY, "append-value-1"); + headers.put(ADD_KEY, "add-value-original"); + headers.put(OVERWRITE_KEY, "overwrite-value-original"); + + ResponseHeaderMutations mutations = ResponseHeaderMutations.create(ImmutableList.of( + header(APPEND_KEY.name(), "append-value-2", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), + header(ADD_KEY.name(), "add-value-new", HeaderAppendAction.ADD_IF_ABSENT), + header(NEW_ADD_KEY.name(), "new-add-value", HeaderAppendAction.ADD_IF_ABSENT), + header(OVERWRITE_KEY.name(), "overwrite-value-new", + HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD), + header(NEW_OVERWRITE_KEY.name(), "new-overwrite-value", + HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD))); + + headerMutator.applyResponseMutations(mutations, headers); + + assertThat(headers.getAll(APPEND_KEY)).containsExactly("append-value-1", "append-value-2"); + assertThat(headers.get(ADD_KEY)).isEqualTo("add-value-original"); + assertThat(headers.get(NEW_ADD_KEY)).isEqualTo("new-add-value"); + assertThat(headers.get(OVERWRITE_KEY)).isEqualTo("overwrite-value-new"); + assertThat(headers.get(NEW_OVERWRITE_KEY)).isEqualTo("new-overwrite-value"); + } + + + @Test + public void applyResponseMutations_InvalidAppendAction_isIgnored() { + Metadata headers = new Metadata(); + headers.put(ASCII_KEY, "value1"); + headerMutator + .applyResponseMutations( + ResponseHeaderMutations + .create( + ImmutableList.of( + HeaderValueOption.newBuilder() + .setHeader(HeaderValue.newBuilder().setKey(ASCII_KEY.name()) + .setValue("value2")) + .setAppendActionValue(-1).build(), + HeaderValueOption + .newBuilder().setHeader(HeaderValue.newBuilder() + .setKey(BINARY_KEY.name()).setValue("value2")) + .setAppendActionValue(-5).build())), + headers); + assertThat(headers.getAll(ASCII_KEY)).containsExactly("value1"); + } + + @Test + public void applyResponseMutations_binary_withBase64RawValue() { + Metadata headers = new Metadata(); + byte[] value = new byte[] {1, 2, 3}; + HeaderValueOption option = HeaderValueOption.newBuilder() + .setHeader(HeaderValue.newBuilder().setKey(BINARY_KEY.name()).setRawValue( + ByteString.copyFrom(BaseEncoding.base64().encode(value), StandardCharsets.US_ASCII))) + .setAppendAction(HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD).build(); + headerMutator.applyResponseMutations(ResponseHeaderMutations.create(ImmutableList.of(option)), + headers); + assertThat(headers.get(BINARY_KEY)).isEqualTo(value); + } + + @Test + public void applyResponseMutations_binary_withBase64Value() { + Metadata headers = new Metadata(); + byte[] value = new byte[] {1, 2, 3}; + String base64Value = BaseEncoding.base64().encode(value); + HeaderValueOption option = HeaderValueOption.newBuilder() + .setHeader(HeaderValue.newBuilder().setKey(BINARY_KEY.name()).setValue(base64Value)) + .setAppendAction(HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD).build(); + + headerMutator.applyResponseMutations(ResponseHeaderMutations.create(ImmutableList.of(option)), + headers); + assertThat(headers.get(BINARY_KEY)).isEqualTo(value); + } + + @Test + public void applyResponseMutations_ascii_withRawValue() { + Metadata headers = new Metadata(); + byte[] value = "raw-value".getBytes(StandardCharsets.US_ASCII); + HeaderValueOption option = HeaderValueOption.newBuilder() + .setHeader(HeaderValue.newBuilder().setKey(ASCII_KEY.name()) + .setRawValue(ByteString.copyFrom(value))) + .setAppendAction(HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD).build(); + + headerMutator.applyResponseMutations(ResponseHeaderMutations.create(ImmutableList.of(option)), + headers); + assertThat(headers.get(Metadata.Key.of(ASCII_KEY.name(), Metadata.ASCII_STRING_MARSHALLER))) + .isEqualTo("raw-value"); + } + + @Test + public void applyRequestMutations_unrecognizedAction_logsWarning() { + Metadata headers = new Metadata(); + RequestHeaderMutations mutations = + RequestHeaderMutations.create(ImmutableList.of(HeaderValueOption.newBuilder() + .setHeader(HeaderValue.newBuilder().setKey("key").setValue("value")) + .setAppendActionValue(-1).build()), ImmutableList.of()); + headerMutator.applyRequestMutations(mutations, headers); + + List records = logHandler.getStoredLogRecords(); + assertThat(records).hasSize(1); + assertThat(records.get(0).getMessage()) + .contains("Unrecognized HeaderAppendAction: UNRECOGNIZED"); + } +} 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 new file mode 100644 index 00000000000..49c43749135 --- /dev/null +++ b/xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderValueOptionTest.java @@ -0,0 +1,40 @@ +/* + * Copyright 2025 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.xds.internal.headermutations; + +import static com.google.common.truth.Truth.assertThat; + +import io.grpc.xds.internal.grpcservice.HeaderValue; +import io.grpc.xds.internal.headermutations.HeaderValueOption.HeaderAppendAction; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class HeaderValueOptionTest { + + @Test + public void create_withAllFields_success() { + HeaderValue header = HeaderValue.create("key1", "value1"); + HeaderValueOption option = HeaderValueOption.create( + header, HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, true); + + assertThat(option.header()).isEqualTo(header); + assertThat(option.appendAction()).isEqualTo(HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD); + assertThat(option.keepEmptyValue()).isTrue(); + } +} From 24b736e817473af6eb6259bf422918dbd6b12166 Mon Sep 17 00:00:00 2001 From: Saurav Date: Thu, 12 Mar 2026 13:00:21 +0000 Subject: [PATCH 02/12] Fixup: 12494 address comments and bring back up to updated ext authz spec --- .../headermutations/HeaderMutationFilter.java | 184 +++++------- .../headermutations/HeaderMutations.java | 1 - .../headermutations/HeaderMutator.java | 164 +++++------ .../HeaderMutationFilterTest.java | 77 +++-- .../headermutations/HeaderMutationsTest.java | 16 +- .../headermutations/HeaderMutatorTest.java | 264 +++++++----------- 6 files changed, 307 insertions(+), 399 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java index 0452354d823..a2c6e6dc7eb 100644 --- a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java +++ b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java @@ -17,12 +17,10 @@ package io.grpc.xds.internal.headermutations; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; -import io.envoyproxy.envoy.config.core.v3.HeaderValueOption; +import io.grpc.xds.internal.grpcservice.HeaderValueValidationUtils; import io.grpc.xds.internal.headermutations.HeaderMutations.RequestHeaderMutations; import io.grpc.xds.internal.headermutations.HeaderMutations.ResponseHeaderMutations; import java.util.Collection; -import java.util.Locale; import java.util.Optional; import java.util.function.Predicate; @@ -30,28 +28,14 @@ * The HeaderMutationFilter class is responsible for filtering header mutations based on a given set * of rules. */ -public interface HeaderMutationFilter { +public class HeaderMutationFilter { + private final Optional mutationRules; - /** - * A factory for creating {@link HeaderMutationFilter} instances. - */ - @FunctionalInterface - interface Factory { - /** - * Creates a new instance of {@code HeaderMutationFilter}. - * - * @param mutationRules The rules for header mutations. If an empty {@code Optional} is - * provided, all header mutations are allowed by default, except for certain system - * headers. If a {@link HeaderMutationRulesConfig} is provided, mutations will be - * filtered based on the specified rules. - */ - HeaderMutationFilter create(Optional mutationRules); - } - /** - * The default factory for creating {@link HeaderMutationFilter} instances. - */ - Factory INSTANCE = HeaderMutationFilterImpl::new; + + public HeaderMutationFilter(Optional mutationRules) { // NOPMD + this.mutationRules = mutationRules; + } /** * Filters the given header mutations based on the configured rules and returns the allowed @@ -62,111 +46,73 @@ interface Factory { * @throws HeaderMutationDisallowedException if a disallowed mutation is encountered and the rules * specify that this should be an error. */ - HeaderMutations filter(HeaderMutations mutations) throws HeaderMutationDisallowedException; - - /** Default implementation of {@link HeaderMutationFilter}. */ - final class HeaderMutationFilterImpl implements HeaderMutationFilter { - private final Optional mutationRules; - - /** - * Set of HTTP/2 pseudo-headers and the host header that are critical for routing and protocol - * correctness. These headers cannot be mutated by user configuration. - */ - private static final ImmutableSet IMMUTABLE_HEADERS = - ImmutableSet.of("host", ":authority", ":scheme", ":method"); - - private HeaderMutationFilterImpl(Optional mutationRules) { // NOPMD - this.mutationRules = mutationRules; - } - - @Override - public HeaderMutations filter(HeaderMutations mutations) - throws HeaderMutationDisallowedException { - ImmutableList allowedRequestHeaders = - filterCollection(mutations.requestMutations().headers(), - header -> isHeaderMutationAllowed(header.getHeader().getKey()) - && !appendsSystemHeader(header)); - ImmutableList allowedRequestHeadersToRemove = - filterCollection(mutations.requestMutations().headersToRemove(), - header -> isHeaderMutationAllowed(header) && isHeaderRemovalAllowed(header)); - ImmutableList allowedResponseHeaders = - filterCollection(mutations.responseMutations().headers(), - header -> isHeaderMutationAllowed(header.getHeader().getKey()) - && !appendsSystemHeader(header)); - return HeaderMutations.create( - RequestHeaderMutations.create(allowedRequestHeaders, allowedRequestHeadersToRemove), - ResponseHeaderMutations.create(allowedResponseHeaders)); - } + public HeaderMutations filter(HeaderMutations mutations) + throws HeaderMutationDisallowedException { + ImmutableList allowedRequestHeaders = + filterCollection(mutations.requestMutations().headers(), + this::shouldIgnore, this::isHeaderMutationAllowed); + ImmutableList allowedRequestHeadersToRemove = + filterCollection(mutations.requestMutations().headersToRemove(), + this::shouldIgnore, this::isHeaderMutationAllowed); + ImmutableList allowedResponseHeaders = + filterCollection(mutations.responseMutations().headers(), + this::shouldIgnore, this::isHeaderMutationAllowed); + return HeaderMutations.create( + RequestHeaderMutations.create(allowedRequestHeaders, allowedRequestHeadersToRemove), + ResponseHeaderMutations.create(allowedResponseHeaders)); + } - /** - * A generic helper to filter a collection based on a predicate. - * - * @param items The collection of items to filter. - * @param isAllowedPredicate The predicate to apply to each item. - * @param The type of items in the collection. - * @return An immutable list of allowed items. - * @throws HeaderMutationDisallowedException if an item is disallowed and disallowIsError is - * true. - */ - private ImmutableList filterCollection(Collection items, - Predicate isAllowedPredicate) throws HeaderMutationDisallowedException { - ImmutableList.Builder allowed = ImmutableList.builder(); - for (T item : items) { - if (isAllowedPredicate.test(item)) { - allowed.add(item); - } else if (disallowIsError()) { - throw new HeaderMutationDisallowedException( - "Header mutation disallowed for header: " + item); - } + /** + * A generic helper to filter a collection based on a predicate. + */ + private ImmutableList filterCollection(Collection items, + Predicate isIgnoredPredicate, Predicate isAllowedPredicate) + throws HeaderMutationDisallowedException { + ImmutableList.Builder allowed = ImmutableList.builder(); + for (T item : items) { + if (isIgnoredPredicate.test(item)) { + continue; + } + if (isAllowedPredicate.test(item)) { + allowed.add(item); + } else if (disallowIsError()) { + throw new HeaderMutationDisallowedException( + "Header mutation disallowed for header: " + item); } - return allowed.build(); } + return allowed.build(); + } - private boolean isHeaderRemovalAllowed(String headerKey) { - return !isSystemHeaderKey(headerKey); - } + private boolean shouldIgnore(String key) { + return HeaderValueValidationUtils.shouldIgnore(key); + } - private boolean appendsSystemHeader(HeaderValueOption headerValueOption) { - String key = headerValueOption.getHeader().getKey(); - boolean isAppend = headerValueOption - .getAppendAction() == HeaderValueOption.HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD; - return isAppend && isSystemHeaderKey(key); - } + private boolean shouldIgnore(HeaderValueOption option) { + return HeaderValueValidationUtils.shouldIgnore(option.header()); + } - private boolean isSystemHeaderKey(String key) { - return key.startsWith(":") || key.toLowerCase(Locale.ROOT).equals("host"); - } + private boolean isHeaderMutationAllowed(HeaderValueOption option) { + return isHeaderMutationAllowed(option.header().key()); + } - private boolean isHeaderMutationAllowed(String headerName) { - String lowerCaseHeaderName = headerName.toLowerCase(Locale.ROOT); - if (IMMUTABLE_HEADERS.contains(lowerCaseHeaderName)) { - return false; - } - return mutationRules.map(rules -> isHeaderMutationAllowed(lowerCaseHeaderName, rules)) - .orElse(true); - } + private boolean isHeaderMutationAllowed(String headerName) { + return mutationRules.map(rules -> isHeaderMutationAllowed(headerName, rules)) + .orElse(true); + } - private boolean isHeaderMutationAllowed(String lowerCaseHeaderName, - HeaderMutationRulesConfig rules) { - // TODO(sauravzg): The priority is slightly unclear in the spec. - // Both `disallowAll` and `disallow_expression` take precedence over `all other - // settings`. - // `allow_expression` takes precedence over everything except `disallow_expression`. - // This is a conflict between ordering for `allow_expression` and `disallowAll`. - // Choosing to proceed with current envoy implementation which favors `allow_expression` over - // `disallowAll`. - if (rules.disallowExpression().isPresent() - && rules.disallowExpression().get().matcher(lowerCaseHeaderName).matches()) { - return false; - } - if (rules.allowExpression().isPresent()) { - return rules.allowExpression().get().matcher(lowerCaseHeaderName).matches(); - } - return !rules.disallowAll(); + private boolean isHeaderMutationAllowed(String lowerCaseHeaderName, + HeaderMutationRulesConfig rules) { + if (rules.disallowExpression().isPresent() + && rules.disallowExpression().get().matcher(lowerCaseHeaderName).matches()) { + return false; } - - private boolean disallowIsError() { - return mutationRules.map(HeaderMutationRulesConfig::disallowIsError).orElse(false); + if (rules.allowExpression().isPresent()) { + return rules.allowExpression().get().matcher(lowerCaseHeaderName).matches(); } + return !rules.disallowAll(); + } + + private boolean disallowIsError() { + return mutationRules.map(HeaderMutationRulesConfig::disallowIsError).orElse(false); } } diff --git a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutations.java b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutations.java index e0cb3daede3..911d798d483 100644 --- a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutations.java +++ b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutations.java @@ -18,7 +18,6 @@ import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; -import io.envoyproxy.envoy.config.core.v3.HeaderValueOption; /** A collection of header mutations for both request and response headers. */ @AutoValue 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 de5b946bbc7..a0ca2f6b76c 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 @@ -16,36 +16,45 @@ package io.grpc.xds.internal.headermutations; -import com.google.common.io.BaseEncoding; -import io.envoyproxy.envoy.config.core.v3.HeaderValue; -import io.envoyproxy.envoy.config.core.v3.HeaderValueOption; -import io.envoyproxy.envoy.config.core.v3.HeaderValueOption.HeaderAppendAction; + import io.grpc.Metadata; +import io.grpc.xds.internal.grpcservice.HeaderValue; import io.grpc.xds.internal.headermutations.HeaderMutations.RequestHeaderMutations; import io.grpc.xds.internal.headermutations.HeaderMutations.ResponseHeaderMutations; -import java.nio.charset.StandardCharsets; +import io.grpc.xds.internal.headermutations.HeaderValueOption.HeaderAppendAction; import java.util.logging.Logger; /** * The HeaderMutator class is an implementation of the HeaderMutator interface. It provides methods * to apply header mutations to a given set of headers based on a given set of rules. */ -public interface HeaderMutator { +public class HeaderMutator { + + private static final Logger logger = Logger.getLogger(HeaderMutator.class.getName()); + /** * Creates a new instance of {@code HeaderMutator}. */ - static HeaderMutator create() { - return new HeaderMutatorImpl(); + public static HeaderMutator create() { + return new HeaderMutator(); } + HeaderMutator() {} + /** * Applies the given header mutations to the provided metadata headers. * * @param mutations The header mutations to apply. * @param headers The metadata headers to which the mutations will be applied. */ - void applyRequestMutations(RequestHeaderMutations mutations, Metadata headers); - + public void applyRequestMutations(final RequestHeaderMutations mutations, Metadata headers) { + // TODO(sauravzg): The specification is not clear on order of header removals and additions. + // in case of conflicts. Copying the order from Envoy here, which does removals at the end. + applyHeaderUpdates(mutations.headers(), headers); + for (String headerToRemove : mutations.headersToRemove()) { + headers.discardAll(Metadata.Key.of(headerToRemove, Metadata.ASCII_STRING_MARSHALLER)); + } + } /** * Applies the given header mutations to the provided metadata headers. @@ -53,91 +62,84 @@ static HeaderMutator create() { * @param mutations The header mutations to apply. * @param headers The metadata headers to which the mutations will be applied. */ - void applyResponseMutations(ResponseHeaderMutations mutations, Metadata headers); - - /** Default implementation of {@link HeaderMutator}. */ - final class HeaderMutatorImpl implements HeaderMutator { - - private static final Logger logger = Logger.getLogger(HeaderMutatorImpl.class.getName()); - - @Override - public void applyRequestMutations(final RequestHeaderMutations mutations, Metadata headers) { - // TODO(sauravzg): The specification is not clear on order of header removals and additions. - // in case of conflicts. Copying the order from Envoy here, which does removals at the end. - applyHeaderUpdates(mutations.headers(), headers); - for (String headerToRemove : mutations.headersToRemove()) { - headers.discardAll(Metadata.Key.of(headerToRemove, Metadata.ASCII_STRING_MARSHALLER)); - } - } + public void applyResponseMutations(final ResponseHeaderMutations mutations, Metadata headers) { + applyHeaderUpdates(mutations.headers(), headers); + } - @Override - public void applyResponseMutations(final ResponseHeaderMutations mutations, Metadata headers) { - applyHeaderUpdates(mutations.headers(), headers); + private void applyHeaderUpdates(final Iterable headerOptions, + Metadata headers) { + for (HeaderValueOption headerOption : headerOptions) { + updateHeader(headerOption, headers); } + } - private void applyHeaderUpdates(final Iterable headerOptions, - Metadata headers) { - for (HeaderValueOption headerOption : headerOptions) { - HeaderValue headerValue = headerOption.getHeader(); - updateHeader(headerValue, headerOption.getAppendAction(), headers); - } - } + private void updateHeader(final HeaderValueOption option, Metadata mutableHeaders) { + HeaderValue header = option.header(); + HeaderAppendAction action = option.appendAction(); + boolean keepEmptyValue = option.keepEmptyValue(); - private void updateHeader(final HeaderValue header, final HeaderAppendAction action, - Metadata mutableHeaders) { - if (header.getKey().endsWith(Metadata.BINARY_HEADER_SUFFIX)) { - updateHeader(action, Metadata.Key.of(header.getKey(), Metadata.BINARY_BYTE_MARSHALLER), - getBinaryHeaderValue(header), mutableHeaders); - } else { - updateHeader(action, Metadata.Key.of(header.getKey(), Metadata.ASCII_STRING_MARSHALLER), - getAsciiValue(header), mutableHeaders); - } + if (header.key().endsWith(Metadata.BINARY_HEADER_SUFFIX)) { + updateHeader(action, Metadata.Key.of(header.key(), Metadata.BINARY_BYTE_MARSHALLER), + header.rawValue().get().toByteArray(), mutableHeaders, keepEmptyValue); + } else { + updateHeader(action, Metadata.Key.of(header.key(), Metadata.ASCII_STRING_MARSHALLER), + header.value().get(), mutableHeaders, keepEmptyValue); } + } - private void updateHeader(final HeaderAppendAction action, final Metadata.Key key, - final T value, Metadata mutableHeaders) { - switch (action) { - case APPEND_IF_EXISTS_OR_ADD: + private void updateHeader(final HeaderAppendAction action, final Metadata.Key key, + final T value, Metadata mutableHeaders, boolean keepEmptyValue) { + switch (action) { + case APPEND_IF_EXISTS_OR_ADD: + mutableHeaders.put(key, value); + break; + case ADD_IF_ABSENT: + if (!mutableHeaders.containsKey(key)) { mutableHeaders.put(key, value); - break; - case ADD_IF_ABSENT: - if (!mutableHeaders.containsKey(key)) { - mutableHeaders.put(key, value); - } - break; - case OVERWRITE_IF_EXISTS_OR_ADD: + } + break; + case OVERWRITE_IF_EXISTS_OR_ADD: + mutableHeaders.discardAll(key); + mutableHeaders.put(key, value); + break; + case OVERWRITE_IF_EXISTS: + if (mutableHeaders.containsKey(key)) { mutableHeaders.discardAll(key); mutableHeaders.put(key, value); - break; - case OVERWRITE_IF_EXISTS: - if (mutableHeaders.containsKey(key)) { - mutableHeaders.discardAll(key); - mutableHeaders.put(key, value); - } - break; - case UNRECOGNIZED: - // Ignore invalid value - logger.warning("Unrecognized HeaderAppendAction: " + action); - break; - default: - // Should be unreachable unless there's a proto schema mismatch. - logger.warning("Unknown HeaderAppendAction: " + action); - } + } + break; + + default: + // Should be unreachable unless there's a proto schema mismatch. + logger.warning("Unknown HeaderAppendAction: " + action); } - private byte[] getBinaryHeaderValue(HeaderValue header) { - return BaseEncoding.base64().decode(getAsciiValue(header)); + if (!keepEmptyValue) { + checkAndRemoveEmpty(key, mutableHeaders); } + } - private String getAsciiValue(HeaderValue header) { - // TODO(sauravzg): GRPC only supports base64 encoded binary headers, so we decode bytes to - // String using `StandardCharsets.US_ASCII`. - // Envoy's spec `raw_value` specification can contain non UTF-8 bytes, so this may potentially - // cause an exception or corruption. - if (!header.getRawValue().isEmpty()) { - return header.getRawValue().toString(StandardCharsets.US_ASCII); + private void checkAndRemoveEmpty(Metadata.Key key, Metadata headers) { + Iterable values = headers.getAll(key); + if (values == null) { + return; + } + boolean allEmpty = true; + for (T val : values) { + if (val instanceof String) { + if (!((String) val).isEmpty()) { + allEmpty = false; + break; + } + } else if (val instanceof byte[]) { + if (((byte[]) val).length > 0) { + allEmpty = false; + break; + } } - return header.getValue(); + } + if (allEmpty) { + headers.discardAll(key); } } } 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 e73460924c7..41ce2245211 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 @@ -19,13 +19,11 @@ import static com.google.common.truth.Truth.assertThat; import com.google.common.collect.ImmutableList; -import io.envoyproxy.envoy.config.core.v3.HeaderValue; -import io.envoyproxy.envoy.config.core.v3.HeaderValueOption; -import io.envoyproxy.envoy.config.core.v3.HeaderValueOption.HeaderAppendAction; +import com.google.re2j.Pattern; import io.grpc.xds.internal.headermutations.HeaderMutations.RequestHeaderMutations; import io.grpc.xds.internal.headermutations.HeaderMutations.ResponseHeaderMutations; +import io.grpc.xds.internal.headermutations.HeaderValueOption.HeaderAppendAction; import java.util.Optional; -import java.util.regex.Pattern; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -34,19 +32,19 @@ public class HeaderMutationFilterTest { private static HeaderValueOption header(String key, String value) { - return HeaderValueOption.newBuilder() - .setHeader(HeaderValue.newBuilder().setKey(key).setValue(value)).build(); + return HeaderValueOption.create(io.grpc.xds.internal.grpcservice.HeaderValue.create(key, value), + HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, false); } private static HeaderValueOption header(String key, String value, HeaderAppendAction action) { - return HeaderValueOption.newBuilder() - .setHeader(HeaderValue.newBuilder().setKey(key).setValue(value)).setAppendAction(action) - .build(); + return HeaderValueOption.create(io.grpc.xds.internal.grpcservice.HeaderValue.create(key, value), + action, + false); } @Test public void filter_removesImmutableHeaders() throws HeaderMutationDisallowedException { - HeaderMutationFilter filter = HeaderMutationFilter.INSTANCE.create(Optional.empty()); + HeaderMutationFilter filter = new HeaderMutationFilter(Optional.empty()); HeaderMutations mutations = HeaderMutations.create( RequestHeaderMutations.create( ImmutableList.of(header("add-key", "add-value"), header(":authority", "new-authority"), @@ -66,7 +64,7 @@ public void filter_removesImmutableHeaders() throws HeaderMutationDisallowedExce @Test public void filter_cannotAppendToSystemHeaders() throws HeaderMutationDisallowedException { - HeaderMutationFilter filter = HeaderMutationFilter.INSTANCE.create(Optional.empty()); + HeaderMutationFilter filter = new HeaderMutationFilter(Optional.empty()); HeaderMutations mutations = HeaderMutations.create( RequestHeaderMutations.create( @@ -89,7 +87,7 @@ public void filter_cannotAppendToSystemHeaders() throws HeaderMutationDisallowed @Test public void filter_cannotRemoveSystemHeaders() throws HeaderMutationDisallowedException { - HeaderMutationFilter filter = HeaderMutationFilter.INSTANCE.create(Optional.empty()); + HeaderMutationFilter filter = new HeaderMutationFilter(Optional.empty()); HeaderMutations mutations = HeaderMutations.create( RequestHeaderMutations.create(ImmutableList.of(), ImmutableList.of("remove-key", "host", ":foo", ":bar")), @@ -101,9 +99,9 @@ public void filter_cannotRemoveSystemHeaders() throws HeaderMutationDisallowedEx } @Test - public void filter_canOverrideSystemHeadersNotInImmutableHeaders() + public void filter_cannotOverrideSystemHeaders() throws HeaderMutationDisallowedException { - HeaderMutationFilter filter = HeaderMutationFilter.INSTANCE.create(Optional.empty()); + HeaderMutationFilter filter = new HeaderMutationFilter(Optional.empty()); HeaderMutations mutations = HeaderMutations.create( RequestHeaderMutations.create( ImmutableList.of(header("user-agent", "new-agent"), @@ -115,19 +113,17 @@ public void filter_canOverrideSystemHeadersNotInImmutableHeaders() HeaderMutations filtered = filter.filter(mutations); + // System headers should be filtered out assertThat(filtered.requestMutations().headers()).containsExactly( - header("user-agent", "new-agent"), - header(":path", "/new/path", HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD), - header(":grpc-trace-bin", "binary-value", HeaderAppendAction.ADD_IF_ABSENT)); - assertThat(filtered.responseMutations().headers()) - .containsExactly(header(":alt-svc", "h3=:443", HeaderAppendAction.OVERWRITE_IF_EXISTS)); + header("user-agent", "new-agent")); + assertThat(filtered.responseMutations().headers()).isEmpty(); } @Test public void filter_disallowAll_disablesAllModifications() throws HeaderMutationDisallowedException { HeaderMutationRulesConfig rules = HeaderMutationRulesConfig.builder().disallowAll(true).build(); - HeaderMutationFilter filter = HeaderMutationFilter.INSTANCE.create(Optional.of(rules)); + HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); HeaderMutations mutations = HeaderMutations.create( RequestHeaderMutations.create(ImmutableList.of(header("add-key", "add-value")), ImmutableList.of("remove-key")), @@ -145,7 +141,7 @@ public void filter_disallowExpression_filtersRelevantExpressions() throws HeaderMutationDisallowedException { HeaderMutationRulesConfig rules = HeaderMutationRulesConfig.builder() .disallowExpression(Pattern.compile("^x-private-.*")).build(); - HeaderMutationFilter filter = HeaderMutationFilter.INSTANCE.create(Optional.of(rules)); + HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); HeaderMutations mutations = HeaderMutations.create( RequestHeaderMutations.create( ImmutableList.of(header("x-public", "value"), header("x-private-key", "value")), @@ -166,7 +162,7 @@ public void filter_allowExpression_onlyAllowsRelevantExpressions() throws HeaderMutationDisallowedException { HeaderMutationRulesConfig rules = HeaderMutationRulesConfig.builder() .allowExpression(Pattern.compile("^x-allowed-.*")).build(); - HeaderMutationFilter filter = HeaderMutationFilter.INSTANCE.create(Optional.of(rules)); + HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); HeaderMutations mutations = HeaderMutations.create( RequestHeaderMutations.create( @@ -190,7 +186,7 @@ public void filter_allowExpression_overridesDisallowAll() throws HeaderMutationDisallowedException { HeaderMutationRulesConfig rules = HeaderMutationRulesConfig.builder().disallowAll(true) .allowExpression(Pattern.compile("^x-allowed-.*")).build(); - HeaderMutationFilter filter = HeaderMutationFilter.INSTANCE.create(Optional.of(rules)); + HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); HeaderMutations mutations = HeaderMutations.create( RequestHeaderMutations.create( ImmutableList.of(header("x-allowed-key", "value"), header("not-allowed", "value")), @@ -212,7 +208,7 @@ public void filter_disallowIsError_throwsExceptionOnDisallowed() throws HeaderMutationDisallowedException { HeaderMutationRulesConfig rules = HeaderMutationRulesConfig.builder().disallowAll(true).disallowIsError(true).build(); - HeaderMutationFilter filter = HeaderMutationFilter.INSTANCE.create(Optional.of(rules)); + HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); HeaderMutations mutations = HeaderMutations.create(RequestHeaderMutations .create(ImmutableList.of(header("add-key", "add-value")), ImmutableList.of()), ResponseHeaderMutations.create(ImmutableList.of())); @@ -224,7 +220,7 @@ public void filter_disallowIsError_throwsExceptionOnDisallowedRemove() throws HeaderMutationDisallowedException { HeaderMutationRulesConfig rules = HeaderMutationRulesConfig.builder().disallowAll(true).disallowIsError(true).build(); - HeaderMutationFilter filter = HeaderMutationFilter.INSTANCE.create(Optional.of(rules)); + HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); HeaderMutations mutations = HeaderMutations.create( RequestHeaderMutations.create(ImmutableList.of(), ImmutableList.of("remove-key")), ResponseHeaderMutations.create(ImmutableList.of())); @@ -236,10 +232,39 @@ public void filter_disallowIsError_throwsExceptionOnDisallowedResponseHeader() throws HeaderMutationDisallowedException { HeaderMutationRulesConfig rules = HeaderMutationRulesConfig.builder().disallowAll(true).disallowIsError(true).build(); - HeaderMutationFilter filter = HeaderMutationFilter.INSTANCE.create(Optional.of(rules)); + HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); HeaderMutations mutations = HeaderMutations.create( RequestHeaderMutations.create(ImmutableList.of(), ImmutableList.of()), ResponseHeaderMutations.create(ImmutableList.of(header("resp-add-key", "resp-add-value")))); filter.filter(mutations); } + + @Test + public void filter_ignoresUppercaseHeaders() throws HeaderMutationDisallowedException { + HeaderMutationFilter filter = new HeaderMutationFilter(Optional.empty()); + HeaderMutations mutations = HeaderMutations.create( + RequestHeaderMutations.create( + ImmutableList.of(header("Valid-Key", "value"), header("valid-key", "value")), + ImmutableList.of("UPPER-REMOVE", "lower-remove")), + ResponseHeaderMutations.create(ImmutableList.of())); + + HeaderMutations filtered = filter.filter(mutations); + + assertThat(filtered.requestMutations().headers()).containsExactly(header("valid-key", "value")); + assertThat(filtered.requestMutations().headersToRemove()).containsExactly("lower-remove"); + } + + @Test + public void filter_ignoresGrpcHeadersInRemoval() throws HeaderMutationDisallowedException { + HeaderMutationFilter filter = new HeaderMutationFilter(Optional.empty()); + HeaderMutations mutations = HeaderMutations.create( + RequestHeaderMutations.create( + ImmutableList.of(), + ImmutableList.of("grpc-timeout", "valid-remove")), + ResponseHeaderMutations.create(ImmutableList.of())); + + HeaderMutations filtered = filter.filter(mutations); + + assertThat(filtered.requestMutations().headersToRemove()).containsExactly("valid-remove"); + } } 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 f1dc0561692..7cd22c5eef6 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 @@ -19,26 +19,26 @@ import static com.google.common.truth.Truth.assertThat; import com.google.common.collect.ImmutableList; -import io.envoyproxy.envoy.config.core.v3.HeaderValue; -import io.envoyproxy.envoy.config.core.v3.HeaderValueOption; +import io.grpc.xds.internal.grpcservice.HeaderValue; import io.grpc.xds.internal.headermutations.HeaderMutations.RequestHeaderMutations; import io.grpc.xds.internal.headermutations.HeaderMutations.ResponseHeaderMutations; +import io.grpc.xds.internal.headermutations.HeaderValueOption.HeaderAppendAction; import org.junit.Test; public class HeaderMutationsTest { @Test public void testCreate() { - HeaderValueOption reqHeader = HeaderValueOption.newBuilder() - .setHeader(HeaderValue.newBuilder().setKey("req-key").setValue("req-value").build()) - .build(); + HeaderValueOption reqHeader = HeaderValueOption.create( + HeaderValue.create("req-key", "req-value"), + HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, false); RequestHeaderMutations requestMutations = RequestHeaderMutations .create(ImmutableList.of(reqHeader), ImmutableList.of("remove-req-key")); assertThat(requestMutations.headers()).containsExactly(reqHeader); assertThat(requestMutations.headersToRemove()).containsExactly("remove-req-key"); - HeaderValueOption respHeader = HeaderValueOption.newBuilder() - .setHeader(HeaderValue.newBuilder().setKey("resp-key").setValue("resp-value").build()) - .build(); + HeaderValueOption respHeader = HeaderValueOption.create( + HeaderValue.create("resp-key", "resp-value"), + HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, false); ResponseHeaderMutations responseMutations = ResponseHeaderMutations.create(ImmutableList.of(respHeader)); assertThat(responseMutations.headers()).containsExactly(respHeader); 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 df6ce383d8c..ede842d782e 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 @@ -19,19 +19,14 @@ import static com.google.common.truth.Truth.assertThat; import com.google.common.collect.ImmutableList; -import com.google.common.io.BaseEncoding; import com.google.common.testing.TestLogHandler; import com.google.protobuf.ByteString; -import io.envoyproxy.envoy.config.core.v3.HeaderValue; -import io.envoyproxy.envoy.config.core.v3.HeaderValueOption; -import io.envoyproxy.envoy.config.core.v3.HeaderValueOption.HeaderAppendAction; import io.grpc.Metadata; +import io.grpc.xds.internal.grpcservice.HeaderValue; import io.grpc.xds.internal.headermutations.HeaderMutations.RequestHeaderMutations; import io.grpc.xds.internal.headermutations.HeaderMutations.ResponseHeaderMutations; -import java.nio.charset.StandardCharsets; -import java.util.List; +import io.grpc.xds.internal.headermutations.HeaderValueOption.HeaderAppendAction; import java.util.logging.Level; -import java.util.logging.LogRecord; import java.util.logging.Logger; import org.junit.After; import org.junit.Before; @@ -42,8 +37,6 @@ @RunWith(JUnit4.class) public class HeaderMutatorTest { - private static final Metadata.Key ASCII_KEY = - Metadata.Key.of("some-key", Metadata.ASCII_STRING_MARSHALLER); private static final Metadata.Key BINARY_KEY = Metadata.Key.of("some-key-bin", Metadata.BINARY_BYTE_MARSHALLER); private static final Metadata.Key APPEND_KEY = @@ -66,8 +59,7 @@ public class HeaderMutatorTest { private final HeaderMutator headerMutator = HeaderMutator.create(); private static final TestLogHandler logHandler = new TestLogHandler(); - private static final Logger logger = - Logger.getLogger(HeaderMutator.HeaderMutatorImpl.class.getName()); + private static final Logger logger = Logger.getLogger(HeaderMutator.class.getName()); @Before public void setUp() { @@ -82,9 +74,7 @@ public void tearDown() { } private static HeaderValueOption header(String key, String value, HeaderAppendAction action) { - return HeaderValueOption.newBuilder() - .setHeader(HeaderValue.newBuilder().setKey(key).setValue(value)).setAppendAction(action) - .build(); + return HeaderValueOption.create(HeaderValue.create(key, value), action, false); } @Test @@ -96,25 +86,32 @@ public void applyRequestMutations_asciiHeaders() { headers.put(REMOVE_KEY, "remove-value-original"); headers.put(OVERWRITE_IF_EXISTS_KEY, "original-value"); - RequestHeaderMutations mutations = RequestHeaderMutations.create(ImmutableList.of( - // Append to existing header - header(APPEND_KEY.name(), "append-value-2", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), - // Try to add to an existing header (should be no-op) - header(ADD_KEY.name(), "add-value-new", HeaderAppendAction.ADD_IF_ABSENT), - // Add a new header - header(NEW_ADD_KEY.name(), "new-add-value", HeaderAppendAction.ADD_IF_ABSENT), - // Overwrite an existing header - header(OVERWRITE_KEY.name(), "overwrite-value-new", - HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD), - // Overwrite a new header - header(NEW_OVERWRITE_KEY.name(), "new-overwrite-value", - HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD), - // Overwrite an existing header if it exists - header(OVERWRITE_IF_EXISTS_KEY.name(), "new-value", HeaderAppendAction.OVERWRITE_IF_EXISTS), - // Try to overwrite a header that does not exist - header(OVERWRITE_IF_EXISTS_ABSENT_KEY.name(), "new-value", - HeaderAppendAction.OVERWRITE_IF_EXISTS)), - ImmutableList.of(REMOVE_KEY.name())); + RequestHeaderMutations mutations = + RequestHeaderMutations.create( + ImmutableList.of( + header( + APPEND_KEY.name(), + "append-value-2", + HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), + header(ADD_KEY.name(), "add-value-new", HeaderAppendAction.ADD_IF_ABSENT), + header(NEW_ADD_KEY.name(), "new-add-value", HeaderAppendAction.ADD_IF_ABSENT), + header( + OVERWRITE_KEY.name(), + "overwrite-value-new", + HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD), + header( + NEW_OVERWRITE_KEY.name(), + "new-overwrite-value", + HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD), + header( + OVERWRITE_IF_EXISTS_KEY.name(), + "new-value", + HeaderAppendAction.OVERWRITE_IF_EXISTS), + header( + OVERWRITE_IF_EXISTS_ABSENT_KEY.name(), + "new-value", + HeaderAppendAction.OVERWRITE_IF_EXISTS)), + ImmutableList.of(REMOVE_KEY.name())); headerMutator.applyRequestMutations(mutations, headers); @@ -128,36 +125,16 @@ public void applyRequestMutations_asciiHeaders() { assertThat(headers.containsKey(OVERWRITE_IF_EXISTS_ABSENT_KEY)).isFalse(); } - @Test - public void applyRequestMutations_InvalidAppendAction_isIgnored() { - Metadata headers = new Metadata(); - headers.put(ASCII_KEY, "value1"); - headerMutator - .applyRequestMutations( - RequestHeaderMutations - .create( - ImmutableList.of( - HeaderValueOption.newBuilder() - .setHeader(HeaderValue.newBuilder().setKey(ASCII_KEY.name()) - .setValue("value2")) - .setAppendActionValue(-1).build(), - HeaderValueOption.newBuilder() - .setHeader(HeaderValue.newBuilder().setKey(BINARY_KEY.name()) - .setValue("value2")) - .setAppendActionValue(-5).build()), - ImmutableList.of()), - headers); - assertThat(headers.getAll(ASCII_KEY)).containsExactly("value1"); - } - @Test public void applyRequestMutations_removalHasPriority() { Metadata headers = new Metadata(); headers.put(REMOVE_KEY, "value"); - RequestHeaderMutations mutations = RequestHeaderMutations.create( - ImmutableList.of( - header(REMOVE_KEY.name(), "new-value", HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD)), - ImmutableList.of(REMOVE_KEY.name())); + RequestHeaderMutations mutations = + RequestHeaderMutations.create( + ImmutableList.of( + header( + REMOVE_KEY.name(), "new-value", HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD)), + ImmutableList.of(REMOVE_KEY.name())); headerMutator.applyRequestMutations(mutations, headers); @@ -165,46 +142,19 @@ public void applyRequestMutations_removalHasPriority() { } @Test - public void applyRequestMutations_binary_withBase64RawValue() { + public void applyRequestMutations_binary() { Metadata headers = new Metadata(); byte[] value = new byte[] {1, 2, 3}; - HeaderValueOption option = HeaderValueOption.newBuilder() - .setHeader(HeaderValue.newBuilder().setKey(BINARY_KEY.name()).setRawValue( - ByteString.copyFrom(BaseEncoding.base64().encode(value), StandardCharsets.US_ASCII))) - .setAppendAction(HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD).build(); + HeaderValueOption option = + HeaderValueOption.create( + HeaderValue.create(BINARY_KEY.name(), ByteString.copyFrom(value)), + HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, + false); headerMutator.applyRequestMutations( RequestHeaderMutations.create(ImmutableList.of(option), ImmutableList.of()), headers); assertThat(headers.get(BINARY_KEY)).isEqualTo(value); } - @Test - public void applyRequestMutations_binary_withBase64Value() { - Metadata headers = new Metadata(); - byte[] value = new byte[] {1, 2, 3}; - String base64Value = BaseEncoding.base64().encode(value); - HeaderValueOption option = HeaderValueOption.newBuilder() - .setHeader(HeaderValue.newBuilder().setKey(BINARY_KEY.name()).setValue(base64Value)) - .setAppendAction(HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD).build(); - - headerMutator.applyRequestMutations( - RequestHeaderMutations.create(ImmutableList.of(option), ImmutableList.of()), headers); - assertThat(headers.get(BINARY_KEY)).isEqualTo(value); - } - - @Test - public void applyRequestMutations_ascii_withRawValue() { - Metadata headers = new Metadata(); - byte[] value = "raw-value".getBytes(StandardCharsets.US_ASCII); - HeaderValueOption option = HeaderValueOption.newBuilder() - .setHeader(HeaderValue.newBuilder().setKey(ASCII_KEY.name()) - .setRawValue(ByteString.copyFrom(value))) - .setAppendAction(HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD).build(); - headerMutator.applyRequestMutations( - RequestHeaderMutations.create(ImmutableList.of(option), ImmutableList.of()), headers); - assertThat(headers.get(Metadata.Key.of(ASCII_KEY.name(), Metadata.ASCII_STRING_MARSHALLER))) - .isEqualTo("raw-value"); - } - @Test public void applyResponseMutations_asciiHeaders() { Metadata headers = new Metadata(); @@ -212,14 +162,23 @@ public void applyResponseMutations_asciiHeaders() { headers.put(ADD_KEY, "add-value-original"); headers.put(OVERWRITE_KEY, "overwrite-value-original"); - ResponseHeaderMutations mutations = ResponseHeaderMutations.create(ImmutableList.of( - header(APPEND_KEY.name(), "append-value-2", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), - header(ADD_KEY.name(), "add-value-new", HeaderAppendAction.ADD_IF_ABSENT), - header(NEW_ADD_KEY.name(), "new-add-value", HeaderAppendAction.ADD_IF_ABSENT), - header(OVERWRITE_KEY.name(), "overwrite-value-new", - HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD), - header(NEW_OVERWRITE_KEY.name(), "new-overwrite-value", - HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD))); + ResponseHeaderMutations mutations = + ResponseHeaderMutations.create( + ImmutableList.of( + header( + APPEND_KEY.name(), + "append-value-2", + HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), + header(ADD_KEY.name(), "add-value-new", HeaderAppendAction.ADD_IF_ABSENT), + header(NEW_ADD_KEY.name(), "new-add-value", HeaderAppendAction.ADD_IF_ABSENT), + header( + OVERWRITE_KEY.name(), + "overwrite-value-new", + HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD), + header( + NEW_OVERWRITE_KEY.name(), + "new-overwrite-value", + HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD))); headerMutator.applyResponseMutations(mutations, headers); @@ -230,82 +189,59 @@ public void applyResponseMutations_asciiHeaders() { assertThat(headers.get(NEW_OVERWRITE_KEY)).isEqualTo("new-overwrite-value"); } - - @Test - public void applyResponseMutations_InvalidAppendAction_isIgnored() { - Metadata headers = new Metadata(); - headers.put(ASCII_KEY, "value1"); - headerMutator - .applyResponseMutations( - ResponseHeaderMutations - .create( - ImmutableList.of( - HeaderValueOption.newBuilder() - .setHeader(HeaderValue.newBuilder().setKey(ASCII_KEY.name()) - .setValue("value2")) - .setAppendActionValue(-1).build(), - HeaderValueOption - .newBuilder().setHeader(HeaderValue.newBuilder() - .setKey(BINARY_KEY.name()).setValue("value2")) - .setAppendActionValue(-5).build())), - headers); - assertThat(headers.getAll(ASCII_KEY)).containsExactly("value1"); - } - @Test - public void applyResponseMutations_binary_withBase64RawValue() { + public void applyResponseMutations_binary() { Metadata headers = new Metadata(); byte[] value = new byte[] {1, 2, 3}; - HeaderValueOption option = HeaderValueOption.newBuilder() - .setHeader(HeaderValue.newBuilder().setKey(BINARY_KEY.name()).setRawValue( - ByteString.copyFrom(BaseEncoding.base64().encode(value), StandardCharsets.US_ASCII))) - .setAppendAction(HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD).build(); - headerMutator.applyResponseMutations(ResponseHeaderMutations.create(ImmutableList.of(option)), - headers); + HeaderValueOption option = + HeaderValueOption.create( + HeaderValue.create(BINARY_KEY.name(), ByteString.copyFrom(value)), + HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, + false); + headerMutator.applyResponseMutations( + ResponseHeaderMutations.create(ImmutableList.of(option)), headers); assertThat(headers.get(BINARY_KEY)).isEqualTo(value); } @Test - public void applyResponseMutations_binary_withBase64Value() { + public void applyRequestMutations_keepEmptyValue() { Metadata headers = new Metadata(); - byte[] value = new byte[] {1, 2, 3}; - String base64Value = BaseEncoding.base64().encode(value); - HeaderValueOption option = HeaderValueOption.newBuilder() - .setHeader(HeaderValue.newBuilder().setKey(BINARY_KEY.name()).setValue(base64Value)) - .setAppendAction(HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD).build(); + headers.put(APPEND_KEY, "existing-value"); + headers.put(OVERWRITE_KEY, "existing-value"); - headerMutator.applyResponseMutations(ResponseHeaderMutations.create(ImmutableList.of(option)), - headers); - assertThat(headers.get(BINARY_KEY)).isEqualTo(value); - } + RequestHeaderMutations mutations = + RequestHeaderMutations.create( + ImmutableList.of( + header(NEW_ADD_KEY.name(), "", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), + header(APPEND_KEY.name(), "", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), + header(OVERWRITE_KEY.name(), "", HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD), + HeaderValueOption.create( + HeaderValue.create("keep-empty-key", ""), + HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, + true), + HeaderValueOption.create( + HeaderValue.create("keep-empty-overwrite-key", ""), + HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD, + true)), + ImmutableList.of()); + + headers.put( + Metadata.Key.of("keep-empty-overwrite-key", Metadata.ASCII_STRING_MARSHALLER), "old"); - @Test - public void applyResponseMutations_ascii_withRawValue() { - Metadata headers = new Metadata(); - byte[] value = "raw-value".getBytes(StandardCharsets.US_ASCII); - HeaderValueOption option = HeaderValueOption.newBuilder() - .setHeader(HeaderValue.newBuilder().setKey(ASCII_KEY.name()) - .setRawValue(ByteString.copyFrom(value))) - .setAppendAction(HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD).build(); + headerMutator.applyRequestMutations(mutations, headers); - headerMutator.applyResponseMutations(ResponseHeaderMutations.create(ImmutableList.of(option)), - headers); - assertThat(headers.get(Metadata.Key.of(ASCII_KEY.name(), Metadata.ASCII_STRING_MARSHALLER))) - .isEqualTo("raw-value"); - } + assertThat(headers.containsKey(NEW_ADD_KEY)).isFalse(); + assertThat(headers.getAll(APPEND_KEY)).containsExactly("existing-value", ""); + assertThat(headers.containsKey(OVERWRITE_KEY)).isFalse(); - @Test - public void applyRequestMutations_unrecognizedAction_logsWarning() { - Metadata headers = new Metadata(); - RequestHeaderMutations mutations = - RequestHeaderMutations.create(ImmutableList.of(HeaderValueOption.newBuilder() - .setHeader(HeaderValue.newBuilder().setKey("key").setValue("value")) - .setAppendActionValue(-1).build()), ImmutableList.of()); - headerMutator.applyRequestMutations(mutations, headers); + 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); - List records = logHandler.getStoredLogRecords(); - assertThat(records).hasSize(1); - assertThat(records.get(0).getMessage()) - .contains("Unrecognized HeaderAppendAction: UNRECOGNIZED"); + assertThat(headers.containsKey(keepEmptyKey)).isTrue(); + assertThat(headers.get(keepEmptyKey)).isEqualTo(""); + assertThat(headers.containsKey(keepEmptyOverwriteKey)).isTrue(); + assertThat(headers.get(keepEmptyOverwriteKey)).isEqualTo(""); } } From 65eb24f88307f9b0d29a7de2deb5de4a2b4ce7e2 Mon Sep 17 00:00:00 2001 From: Saurav Date: Sun, 15 Mar 2026 20:23:31 +0000 Subject: [PATCH 03/12] Fixup 12494: Fixes for logging and additional comment --- .../grpc/xds/internal/headermutations/HeaderMutationFilter.java | 2 +- .../io/grpc/xds/internal/headermutations/HeaderMutator.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java index a2c6e6dc7eb..3c5eef77894 100644 --- a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java +++ b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java @@ -33,7 +33,7 @@ public class HeaderMutationFilter { - public HeaderMutationFilter(Optional mutationRules) { // NOPMD + public HeaderMutationFilter(Optional mutationRules) { this.mutationRules = mutationRules; } 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 a0ca2f6b76c..f158a66123f 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 @@ -111,7 +111,7 @@ private void updateHeader(final HeaderAppendAction action, final Metadata.Ke default: // Should be unreachable unless there's a proto schema mismatch. - logger.warning("Unknown HeaderAppendAction: " + action); + logger.fine("Unknown HeaderAppendAction: " + action); } if (!keepEmptyValue) { From 6653d2ec80adc881aa303a91a270aed589b21dda Mon Sep 17 00:00:00 2001 From: Saurav Date: Mon, 16 Mar 2026 08:14:20 +0000 Subject: [PATCH 04/12] Fixup 12494: Remove Authz specific abstractions away from the generic headermutations libraries --- .../headermutations/HeaderMutationFilter.java | 20 +-- .../headermutations/HeaderMutations.java | 35 +--- .../headermutations/HeaderMutator.java | 14 +- .../HeaderMutationFilterTest.java | 162 +++++++----------- .../headermutations/HeaderMutationsTest.java | 25 +-- .../headermutations/HeaderMutatorTest.java | 37 ++-- 6 files changed, 103 insertions(+), 190 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java index 3c5eef77894..b0cba894f05 100644 --- a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java +++ b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java @@ -18,8 +18,6 @@ import com.google.common.collect.ImmutableList; import io.grpc.xds.internal.grpcservice.HeaderValueValidationUtils; -import io.grpc.xds.internal.headermutations.HeaderMutations.RequestHeaderMutations; -import io.grpc.xds.internal.headermutations.HeaderMutations.ResponseHeaderMutations; import java.util.Collection; import java.util.Optional; import java.util.function.Predicate; @@ -48,18 +46,12 @@ public HeaderMutationFilter(Optional mutationRules) { */ public HeaderMutations filter(HeaderMutations mutations) throws HeaderMutationDisallowedException { - ImmutableList allowedRequestHeaders = - filterCollection(mutations.requestMutations().headers(), - this::shouldIgnore, this::isHeaderMutationAllowed); - ImmutableList allowedRequestHeadersToRemove = - filterCollection(mutations.requestMutations().headersToRemove(), - this::shouldIgnore, this::isHeaderMutationAllowed); - ImmutableList allowedResponseHeaders = - filterCollection(mutations.responseMutations().headers(), - this::shouldIgnore, this::isHeaderMutationAllowed); - return HeaderMutations.create( - RequestHeaderMutations.create(allowedRequestHeaders, allowedRequestHeadersToRemove), - ResponseHeaderMutations.create(allowedResponseHeaders)); + ImmutableList allowedHeaders = + filterCollection(mutations.headers(), this::shouldIgnore, this::isHeaderMutationAllowed); + ImmutableList allowedHeadersToRemove = + filterCollection(mutations.headersToRemove(), this::shouldIgnore, + this::isHeaderMutationAllowed); + return HeaderMutations.create(allowedHeaders, allowedHeadersToRemove); } /** diff --git a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutations.java b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutations.java index 911d798d483..a456413c899 100644 --- a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutations.java +++ b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutations.java @@ -19,39 +19,16 @@ import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; -/** A collection of header mutations for both request and response headers. */ +/** A collection of header mutations. */ @AutoValue public abstract class HeaderMutations { - public static HeaderMutations create(RequestHeaderMutations requestMutations, - ResponseHeaderMutations responseMutations) { - return new AutoValue_HeaderMutations(requestMutations, responseMutations); + public static HeaderMutations create(ImmutableList headers, + ImmutableList headersToRemove) { + return new AutoValue_HeaderMutations(headers, headersToRemove); } - public abstract RequestHeaderMutations requestMutations(); + public abstract ImmutableList headers(); - public abstract ResponseHeaderMutations responseMutations(); - - /** Represents mutations for request headers. */ - @AutoValue - public abstract static class RequestHeaderMutations { - public static RequestHeaderMutations create(ImmutableList headers, - ImmutableList headersToRemove) { - return new AutoValue_HeaderMutations_RequestHeaderMutations(headers, headersToRemove); - } - - public abstract ImmutableList headers(); - - public abstract ImmutableList headersToRemove(); - } - - /** Represents mutations for response headers. */ - @AutoValue - public abstract static class ResponseHeaderMutations { - public static ResponseHeaderMutations create(ImmutableList headers) { - return new AutoValue_HeaderMutations_ResponseHeaderMutations(headers); - } - - public abstract ImmutableList headers(); - } + public abstract ImmutableList headersToRemove(); } 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 f158a66123f..0b2e234e1f7 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 @@ -19,8 +19,6 @@ import io.grpc.Metadata; import io.grpc.xds.internal.grpcservice.HeaderValue; -import io.grpc.xds.internal.headermutations.HeaderMutations.RequestHeaderMutations; -import io.grpc.xds.internal.headermutations.HeaderMutations.ResponseHeaderMutations; import io.grpc.xds.internal.headermutations.HeaderValueOption.HeaderAppendAction; import java.util.logging.Logger; @@ -47,7 +45,7 @@ public static HeaderMutator create() { * @param mutations The header mutations to apply. * @param headers The metadata headers to which the mutations will be applied. */ - public void applyRequestMutations(final RequestHeaderMutations mutations, Metadata headers) { + public void applyMutations(final HeaderMutations mutations, Metadata headers) { // TODO(sauravzg): The specification is not clear on order of header removals and additions. // in case of conflicts. Copying the order from Envoy here, which does removals at the end. applyHeaderUpdates(mutations.headers(), headers); @@ -56,16 +54,6 @@ public void applyRequestMutations(final RequestHeaderMutations mutations, Metada } } - /** - * Applies the given header mutations to the provided metadata headers. - * - * @param mutations The header mutations to apply. - * @param headers The metadata headers to which the mutations will be applied. - */ - public void applyResponseMutations(final ResponseHeaderMutations mutations, Metadata headers) { - applyHeaderUpdates(mutations.headers(), headers); - } - private void applyHeaderUpdates(final Iterable headerOptions, Metadata headers) { for (HeaderValueOption headerOption : headerOptions) { 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 41ce2245211..2331bdb2a46 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 @@ -20,8 +20,7 @@ import com.google.common.collect.ImmutableList; import com.google.re2j.Pattern; -import io.grpc.xds.internal.headermutations.HeaderMutations.RequestHeaderMutations; -import io.grpc.xds.internal.headermutations.HeaderMutations.ResponseHeaderMutations; +import io.grpc.xds.internal.headermutations.HeaderMutations; import io.grpc.xds.internal.headermutations.HeaderValueOption.HeaderAppendAction; import java.util.Optional; import org.junit.Test; @@ -46,56 +45,47 @@ private static HeaderValueOption header(String key, String value, HeaderAppendAc public void filter_removesImmutableHeaders() throws HeaderMutationDisallowedException { HeaderMutationFilter filter = new HeaderMutationFilter(Optional.empty()); HeaderMutations mutations = HeaderMutations.create( - RequestHeaderMutations.create( - ImmutableList.of(header("add-key", "add-value"), header(":authority", "new-authority"), - header("host", "new-host"), header(":scheme", "https"), header(":method", "PUT")), - ImmutableList.of("remove-key", "host", ":authority", ":scheme", ":method")), - ResponseHeaderMutations.create(ImmutableList.of(header("resp-add-key", "resp-add-value"), - header(":scheme", "https")))); + ImmutableList.of(header("add-key", "add-value"), header(":authority", "new-authority"), + header("host", "new-host"), header(":scheme", "https"), header(":method", "PUT"), + header("resp-add-key", "resp-add-value"), header(":scheme", "https")), + ImmutableList.of("remove-key", "host", ":authority", ":scheme", ":method")); HeaderMutations filtered = filter.filter(mutations); - assertThat(filtered.requestMutations().headers()) - .containsExactly(header("add-key", "add-value")); - assertThat(filtered.requestMutations().headersToRemove()).containsExactly("remove-key"); - assertThat(filtered.responseMutations().headers()) - .containsExactly(header("resp-add-key", "resp-add-value")); + + assertThat(filtered.headersToRemove()).containsExactly("remove-key"); + assertThat(filtered.headers()).containsExactly(header("add-key", "add-value"), + header("resp-add-key", "resp-add-value")); } @Test public void filter_cannotAppendToSystemHeaders() throws HeaderMutationDisallowedException { HeaderMutationFilter filter = new HeaderMutationFilter(Optional.empty()); - HeaderMutations mutations = - HeaderMutations.create( - RequestHeaderMutations.create( - ImmutableList.of( - header("add-key", "add-value", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), - header(":authority", "new-authority", - HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), - header("host", "new-host", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), - header(":path", "/new-path", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD)), - ImmutableList.of()), - ResponseHeaderMutations.create(ImmutableList - .of(header("host", "new-host", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD)))); + HeaderMutations mutations = HeaderMutations.create( + ImmutableList.of( + header("add-key", "add-value", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), + header(":authority", "new-authority", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), + header("host", "new-host", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), + header(":path", "/new-path", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), + header("host", "new-host", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD)), + ImmutableList.of()); HeaderMutations filtered = filter.filter(mutations); - assertThat(filtered.requestMutations().headers()).containsExactly( + assertThat(filtered.headers()).containsExactly( header("add-key", "add-value", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD)); - assertThat(filtered.responseMutations().headers()).isEmpty(); } @Test public void filter_cannotRemoveSystemHeaders() throws HeaderMutationDisallowedException { HeaderMutationFilter filter = new HeaderMutationFilter(Optional.empty()); HeaderMutations mutations = HeaderMutations.create( - RequestHeaderMutations.create(ImmutableList.of(), - ImmutableList.of("remove-key", "host", ":foo", ":bar")), - ResponseHeaderMutations.create(ImmutableList.of())); + ImmutableList.of(), + ImmutableList.of("remove-key", "host", ":foo", ":bar")); HeaderMutations filtered = filter.filter(mutations); - assertThat(filtered.requestMutations().headersToRemove()).containsExactly("remove-key"); + assertThat(filtered.headersToRemove()).containsExactly("remove-key"); } @Test @@ -103,20 +93,17 @@ public void filter_cannotOverrideSystemHeaders() throws HeaderMutationDisallowedException { HeaderMutationFilter filter = new HeaderMutationFilter(Optional.empty()); HeaderMutations mutations = HeaderMutations.create( - RequestHeaderMutations.create( - ImmutableList.of(header("user-agent", "new-agent"), - header(":path", "/new/path", HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD), - header(":grpc-trace-bin", "binary-value", HeaderAppendAction.ADD_IF_ABSENT)), - ImmutableList.of()), - ResponseHeaderMutations.create(ImmutableList - .of(header(":alt-svc", "h3=:443", HeaderAppendAction.OVERWRITE_IF_EXISTS)))); + ImmutableList.of(header("user-agent", "new-agent"), + header(":path", "/new/path", HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD), + header(":grpc-trace-bin", "binary-value", HeaderAppendAction.ADD_IF_ABSENT), + header(":alt-svc", "h3=:443", HeaderAppendAction.OVERWRITE_IF_EXISTS)), + ImmutableList.of()); HeaderMutations filtered = filter.filter(mutations); // System headers should be filtered out - assertThat(filtered.requestMutations().headers()).containsExactly( + assertThat(filtered.headers()).containsExactly( header("user-agent", "new-agent")); - assertThat(filtered.responseMutations().headers()).isEmpty(); } @Test @@ -125,15 +112,13 @@ public void filter_disallowAll_disablesAllModifications() HeaderMutationRulesConfig rules = HeaderMutationRulesConfig.builder().disallowAll(true).build(); HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); HeaderMutations mutations = HeaderMutations.create( - RequestHeaderMutations.create(ImmutableList.of(header("add-key", "add-value")), - ImmutableList.of("remove-key")), - ResponseHeaderMutations.create(ImmutableList.of(header("resp-add-key", "resp-add-value")))); + ImmutableList.of(header("add-key", "add-value"), header("resp-add-key", "resp-add-value")), + ImmutableList.of("remove-key")); HeaderMutations filtered = filter.filter(mutations); - assertThat(filtered.requestMutations().headers()).isEmpty(); - assertThat(filtered.requestMutations().headersToRemove()).isEmpty(); - assertThat(filtered.responseMutations().headers()).isEmpty(); + assertThat(filtered.headers()).isEmpty(); + assertThat(filtered.headersToRemove()).isEmpty(); } @Test @@ -143,18 +128,16 @@ public void filter_disallowExpression_filtersRelevantExpressions() .disallowExpression(Pattern.compile("^x-private-.*")).build(); HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); HeaderMutations mutations = HeaderMutations.create( - RequestHeaderMutations.create( - ImmutableList.of(header("x-public", "value"), header("x-private-key", "value")), - ImmutableList.of("x-public-remove", "x-private-remove")), - ResponseHeaderMutations.create( - ImmutableList.of(header("x-public-resp", "value"), header("x-private-resp", "value")))); + ImmutableList.of(header("x-public", "value"), header("x-private-key", "value"), + header("x-public-resp", "value"), header("x-private-resp", "value")), + ImmutableList.of("x-public-remove", "x-private-remove")); HeaderMutations filtered = filter.filter(mutations); - assertThat(filtered.requestMutations().headers()).containsExactly(header("x-public", "value")); - assertThat(filtered.requestMutations().headersToRemove()).containsExactly("x-public-remove"); - assertThat(filtered.responseMutations().headers()) - .containsExactly(header("x-public-resp", "value")); + + assertThat(filtered.headersToRemove()).containsExactly("x-public-remove"); + assertThat(filtered.headers()).containsExactly(header("x-public", "value"), + header("x-public-resp", "value")); } @Test @@ -163,22 +146,17 @@ public void filter_allowExpression_onlyAllowsRelevantExpressions() HeaderMutationRulesConfig rules = HeaderMutationRulesConfig.builder() .allowExpression(Pattern.compile("^x-allowed-.*")).build(); HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); - HeaderMutations mutations = - HeaderMutations.create( - RequestHeaderMutations.create( - ImmutableList.of(header("x-allowed-key", "value"), - header("not-allowed-key", "value")), - ImmutableList.of("x-allowed-remove", "not-allowed-remove")), - ResponseHeaderMutations.create(ImmutableList.of(header("x-allowed-resp-key", "value"), - header("not-allowed-resp-key", "value")))); + HeaderMutations mutations = HeaderMutations.create( + ImmutableList.of(header("x-allowed-key", "value"), header("not-allowed-key", "value"), + header("x-allowed-resp-key", "value"), header("not-allowed-resp-key", "value")), + ImmutableList.of("x-allowed-remove", "not-allowed-remove")); HeaderMutations filtered = filter.filter(mutations); - assertThat(filtered.requestMutations().headers()) - .containsExactly(header("x-allowed-key", "value")); - assertThat(filtered.requestMutations().headersToRemove()).containsExactly("x-allowed-remove"); - assertThat(filtered.responseMutations().headers()) - .containsExactly(header("x-allowed-resp-key", "value")); + + assertThat(filtered.headersToRemove()).containsExactly("x-allowed-remove"); + assertThat(filtered.headers()).containsExactly(header("x-allowed-key", "value"), + header("x-allowed-resp-key", "value")); } @Test @@ -188,19 +166,15 @@ public void filter_allowExpression_overridesDisallowAll() .allowExpression(Pattern.compile("^x-allowed-.*")).build(); HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); HeaderMutations mutations = HeaderMutations.create( - RequestHeaderMutations.create( - ImmutableList.of(header("x-allowed-key", "value"), header("not-allowed", "value")), - ImmutableList.of("x-allowed-remove", "not-allowed-remove")), - ResponseHeaderMutations.create(ImmutableList.of(header("x-allowed-resp-key", "value"), - header("not-allowed-resp-key", "value")))); + ImmutableList.of(header("x-allowed-key", "value"), header("not-allowed", "value"), + header("x-allowed-resp-key", "value"), header("not-allowed-resp-key", "value")), + ImmutableList.of("x-allowed-remove", "not-allowed-remove")); HeaderMutations filtered = filter.filter(mutations); - assertThat(filtered.requestMutations().headers()) - .containsExactly(header("x-allowed-key", "value")); - assertThat(filtered.requestMutations().headersToRemove()).containsExactly("x-allowed-remove"); - assertThat(filtered.responseMutations().headers()) - .containsExactly(header("x-allowed-resp-key", "value")); + assertThat(filtered.headersToRemove()).containsExactly("x-allowed-remove"); + assertThat(filtered.headers()).containsExactly(header("x-allowed-key", "value"), + header("x-allowed-resp-key", "value")); } @Test(expected = HeaderMutationDisallowedException.class) @@ -209,9 +183,9 @@ public void filter_disallowIsError_throwsExceptionOnDisallowed() HeaderMutationRulesConfig rules = HeaderMutationRulesConfig.builder().disallowAll(true).disallowIsError(true).build(); HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); - HeaderMutations mutations = HeaderMutations.create(RequestHeaderMutations - .create(ImmutableList.of(header("add-key", "add-value")), ImmutableList.of()), - ResponseHeaderMutations.create(ImmutableList.of())); + HeaderMutations mutations = HeaderMutations.create( + ImmutableList.of(header("add-key", "add-value")), + ImmutableList.of()); filter.filter(mutations); } @@ -222,8 +196,8 @@ public void filter_disallowIsError_throwsExceptionOnDisallowedRemove() HeaderMutationRulesConfig.builder().disallowAll(true).disallowIsError(true).build(); HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); HeaderMutations mutations = HeaderMutations.create( - RequestHeaderMutations.create(ImmutableList.of(), ImmutableList.of("remove-key")), - ResponseHeaderMutations.create(ImmutableList.of())); + ImmutableList.of(), + ImmutableList.of("remove-key")); filter.filter(mutations); } @@ -234,8 +208,8 @@ public void filter_disallowIsError_throwsExceptionOnDisallowedResponseHeader() HeaderMutationRulesConfig.builder().disallowAll(true).disallowIsError(true).build(); HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); HeaderMutations mutations = HeaderMutations.create( - RequestHeaderMutations.create(ImmutableList.of(), ImmutableList.of()), - ResponseHeaderMutations.create(ImmutableList.of(header("resp-add-key", "resp-add-value")))); + ImmutableList.of(header("resp-add-key", "resp-add-value")), + ImmutableList.of()); filter.filter(mutations); } @@ -243,28 +217,24 @@ public void filter_disallowIsError_throwsExceptionOnDisallowedResponseHeader() public void filter_ignoresUppercaseHeaders() throws HeaderMutationDisallowedException { HeaderMutationFilter filter = new HeaderMutationFilter(Optional.empty()); HeaderMutations mutations = HeaderMutations.create( - RequestHeaderMutations.create( - ImmutableList.of(header("Valid-Key", "value"), header("valid-key", "value")), - ImmutableList.of("UPPER-REMOVE", "lower-remove")), - ResponseHeaderMutations.create(ImmutableList.of())); + ImmutableList.of(header("Valid-Key", "value"), header("valid-key", "value")), + ImmutableList.of("UPPER-REMOVE", "lower-remove")); HeaderMutations filtered = filter.filter(mutations); - assertThat(filtered.requestMutations().headers()).containsExactly(header("valid-key", "value")); - assertThat(filtered.requestMutations().headersToRemove()).containsExactly("lower-remove"); + assertThat(filtered.headers()).containsExactly(header("valid-key", "value")); + assertThat(filtered.headersToRemove()).containsExactly("lower-remove"); } @Test public void filter_ignoresGrpcHeadersInRemoval() throws HeaderMutationDisallowedException { HeaderMutationFilter filter = new HeaderMutationFilter(Optional.empty()); HeaderMutations mutations = HeaderMutations.create( - RequestHeaderMutations.create( - ImmutableList.of(), - ImmutableList.of("grpc-timeout", "valid-remove")), - ResponseHeaderMutations.create(ImmutableList.of())); + ImmutableList.of(), + ImmutableList.of("grpc-timeout", "valid-remove")); HeaderMutations filtered = filter.filter(mutations); - assertThat(filtered.requestMutations().headersToRemove()).containsExactly("valid-remove"); + assertThat(filtered.headersToRemove()).containsExactly("valid-remove"); } } 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 7cd22c5eef6..5f820b62306 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 @@ -20,31 +20,18 @@ import com.google.common.collect.ImmutableList; import io.grpc.xds.internal.grpcservice.HeaderValue; -import io.grpc.xds.internal.headermutations.HeaderMutations.RequestHeaderMutations; -import io.grpc.xds.internal.headermutations.HeaderMutations.ResponseHeaderMutations; import io.grpc.xds.internal.headermutations.HeaderValueOption.HeaderAppendAction; import org.junit.Test; public class HeaderMutationsTest { @Test public void testCreate() { - HeaderValueOption reqHeader = HeaderValueOption.create( - HeaderValue.create("req-key", "req-value"), + HeaderValueOption header = HeaderValueOption.create( + HeaderValue.create("key", "value"), HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, false); - RequestHeaderMutations requestMutations = RequestHeaderMutations - .create(ImmutableList.of(reqHeader), ImmutableList.of("remove-req-key")); - assertThat(requestMutations.headers()).containsExactly(reqHeader); - assertThat(requestMutations.headersToRemove()).containsExactly("remove-req-key"); - - HeaderValueOption respHeader = HeaderValueOption.create( - HeaderValue.create("resp-key", "resp-value"), - HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, false); - ResponseHeaderMutations responseMutations = - ResponseHeaderMutations.create(ImmutableList.of(respHeader)); - assertThat(responseMutations.headers()).containsExactly(respHeader); - - HeaderMutations mutations = HeaderMutations.create(requestMutations, responseMutations); - assertThat(mutations.requestMutations()).isEqualTo(requestMutations); - assertThat(mutations.responseMutations()).isEqualTo(responseMutations); + HeaderMutations mutations = HeaderMutations.create( + ImmutableList.of(header), ImmutableList.of("remove-key")); + assertThat(mutations.headers()).containsExactly(header); + assertThat(mutations.headersToRemove()).containsExactly("remove-key"); } } 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 ede842d782e..a0059b14afc 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 @@ -23,8 +23,7 @@ import com.google.protobuf.ByteString; import io.grpc.Metadata; import io.grpc.xds.internal.grpcservice.HeaderValue; -import io.grpc.xds.internal.headermutations.HeaderMutations.RequestHeaderMutations; -import io.grpc.xds.internal.headermutations.HeaderMutations.ResponseHeaderMutations; +import io.grpc.xds.internal.headermutations.HeaderMutations; import io.grpc.xds.internal.headermutations.HeaderValueOption.HeaderAppendAction; import java.util.logging.Level; import java.util.logging.Logger; @@ -86,8 +85,8 @@ public void applyRequestMutations_asciiHeaders() { headers.put(REMOVE_KEY, "remove-value-original"); headers.put(OVERWRITE_IF_EXISTS_KEY, "original-value"); - RequestHeaderMutations mutations = - RequestHeaderMutations.create( + HeaderMutations mutations = + HeaderMutations.create( ImmutableList.of( header( APPEND_KEY.name(), @@ -113,7 +112,7 @@ public void applyRequestMutations_asciiHeaders() { HeaderAppendAction.OVERWRITE_IF_EXISTS)), ImmutableList.of(REMOVE_KEY.name())); - headerMutator.applyRequestMutations(mutations, headers); + headerMutator.applyMutations(mutations, headers); assertThat(headers.getAll(APPEND_KEY)).containsExactly("append-value-1", "append-value-2"); assertThat(headers.get(ADD_KEY)).isEqualTo("add-value-original"); @@ -129,14 +128,14 @@ public void applyRequestMutations_asciiHeaders() { public void applyRequestMutations_removalHasPriority() { Metadata headers = new Metadata(); headers.put(REMOVE_KEY, "value"); - RequestHeaderMutations mutations = - RequestHeaderMutations.create( + HeaderMutations mutations = + HeaderMutations.create( ImmutableList.of( header( REMOVE_KEY.name(), "new-value", HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD)), ImmutableList.of(REMOVE_KEY.name())); - headerMutator.applyRequestMutations(mutations, headers); + headerMutator.applyMutations(mutations, headers); assertThat(headers.containsKey(REMOVE_KEY)).isFalse(); } @@ -150,8 +149,8 @@ public void applyRequestMutations_binary() { HeaderValue.create(BINARY_KEY.name(), ByteString.copyFrom(value)), HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, false); - headerMutator.applyRequestMutations( - RequestHeaderMutations.create(ImmutableList.of(option), ImmutableList.of()), headers); + headerMutator.applyMutations( + HeaderMutations.create(ImmutableList.of(option), ImmutableList.of()), headers); assertThat(headers.get(BINARY_KEY)).isEqualTo(value); } @@ -162,8 +161,8 @@ public void applyResponseMutations_asciiHeaders() { headers.put(ADD_KEY, "add-value-original"); headers.put(OVERWRITE_KEY, "overwrite-value-original"); - ResponseHeaderMutations mutations = - ResponseHeaderMutations.create( + HeaderMutations mutations = + HeaderMutations.create( ImmutableList.of( header( APPEND_KEY.name(), @@ -178,9 +177,9 @@ public void applyResponseMutations_asciiHeaders() { header( NEW_OVERWRITE_KEY.name(), "new-overwrite-value", - HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD))); + HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD)), ImmutableList.of()); - headerMutator.applyResponseMutations(mutations, headers); + headerMutator.applyMutations(mutations, headers); assertThat(headers.getAll(APPEND_KEY)).containsExactly("append-value-1", "append-value-2"); assertThat(headers.get(ADD_KEY)).isEqualTo("add-value-original"); @@ -198,8 +197,8 @@ public void applyResponseMutations_binary() { HeaderValue.create(BINARY_KEY.name(), ByteString.copyFrom(value)), HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, false); - headerMutator.applyResponseMutations( - ResponseHeaderMutations.create(ImmutableList.of(option)), headers); + headerMutator.applyMutations( + HeaderMutations.create(ImmutableList.of(option), ImmutableList.of()), headers); assertThat(headers.get(BINARY_KEY)).isEqualTo(value); } @@ -209,8 +208,8 @@ public void applyRequestMutations_keepEmptyValue() { headers.put(APPEND_KEY, "existing-value"); headers.put(OVERWRITE_KEY, "existing-value"); - RequestHeaderMutations mutations = - RequestHeaderMutations.create( + HeaderMutations mutations = + HeaderMutations.create( ImmutableList.of( header(NEW_ADD_KEY.name(), "", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), header(APPEND_KEY.name(), "", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), @@ -228,7 +227,7 @@ public void applyRequestMutations_keepEmptyValue() { headers.put( Metadata.Key.of("keep-empty-overwrite-key", Metadata.ASCII_STRING_MARSHALLER), "old"); - headerMutator.applyRequestMutations(mutations, headers); + headerMutator.applyMutations(mutations, headers); assertThat(headers.containsKey(NEW_ADD_KEY)).isFalse(); assertThat(headers.getAll(APPEND_KEY)).containsExactly("existing-value", ""); From b96db79713865ed77258c3b5eb3c1ea7a52a7b8b Mon Sep 17 00:00:00 2001 From: Saurav Date: Mon, 23 Mar 2026 17:02:06 +0000 Subject: [PATCH 05/12] Fixup 12494: Rename variable --- .../internal/headermutations/HeaderMutationFilter.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java index b0cba894f05..381f59f7db3 100644 --- a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java +++ b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java @@ -92,14 +92,14 @@ private boolean isHeaderMutationAllowed(String headerName) { .orElse(true); } - private boolean isHeaderMutationAllowed(String lowerCaseHeaderName, - HeaderMutationRulesConfig rules) { + private boolean isHeaderMutationAllowed(String headerName, + HeaderMutationRulesConfig rules) { if (rules.disallowExpression().isPresent() - && rules.disallowExpression().get().matcher(lowerCaseHeaderName).matches()) { + && rules.disallowExpression().get().matcher(headerName).matches()) { return false; } if (rules.allowExpression().isPresent()) { - return rules.allowExpression().get().matcher(lowerCaseHeaderName).matches(); + return rules.allowExpression().get().matcher(headerName).matches(); } return !rules.disallowAll(); } From d866186c6e284269c2d0ebff0849bc34b2deecb0 Mon Sep 17 00:00:00 2001 From: Saurav Date: Wed, 25 Mar 2026 06:16:24 +0000 Subject: [PATCH 06/12] Fixup 12494: Address copilot comments --- .../headermutations/HeaderMutationFilter.java | 17 +++++++-------- .../headermutations/HeaderMutator.java | 21 ++++++++++++++----- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java index 381f59f7db3..4c6f90b54fa 100644 --- a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java +++ b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java @@ -47,10 +47,10 @@ public HeaderMutationFilter(Optional mutationRules) { public HeaderMutations filter(HeaderMutations mutations) throws HeaderMutationDisallowedException { ImmutableList allowedHeaders = - filterCollection(mutations.headers(), this::shouldIgnore, this::isHeaderMutationAllowed); + filterCollection(mutations.headers(), this::isDisallowed, this::isHeaderMutationAllowed); ImmutableList allowedHeadersToRemove = - filterCollection(mutations.headersToRemove(), this::shouldIgnore, - this::isHeaderMutationAllowed); + filterCollection(mutations.headersToRemove(), this::isDisallowed, + this::isHeaderMutationAllowed); return HeaderMutations.create(allowedHeaders, allowedHeadersToRemove); } @@ -68,19 +68,18 @@ private ImmutableList filterCollection(Collection items, if (isAllowedPredicate.test(item)) { allowed.add(item); } else if (disallowIsError()) { - throw new HeaderMutationDisallowedException( - "Header mutation disallowed for header: " + item); + throw new HeaderMutationDisallowedException("Header mutation disallowed"); } } return allowed.build(); } - private boolean shouldIgnore(String key) { - return HeaderValueValidationUtils.shouldIgnore(key); + private boolean isDisallowed(String key) { + return HeaderValueValidationUtils.isDisallowed(key); } - private boolean shouldIgnore(HeaderValueOption option) { - return HeaderValueValidationUtils.shouldIgnore(option.header()); + private boolean isDisallowed(HeaderValueOption option) { + return HeaderValueValidationUtils.isDisallowed(option.header()); } private boolean isHeaderMutationAllowed(HeaderValueOption option) { 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 0b2e234e1f7..6e80389a50c 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 @@ -50,7 +50,10 @@ public void applyMutations(final HeaderMutations mutations, Metadata headers) { // in case of conflicts. Copying the order from Envoy here, which does removals at the end. applyHeaderUpdates(mutations.headers(), headers); for (String headerToRemove : mutations.headersToRemove()) { - headers.discardAll(Metadata.Key.of(headerToRemove, Metadata.ASCII_STRING_MARSHALLER)); + Metadata.Key key = headerToRemove.endsWith(Metadata.BINARY_HEADER_SUFFIX) + ? Metadata.Key.of(headerToRemove, Metadata.BINARY_BYTE_MARSHALLER) + : Metadata.Key.of(headerToRemove, Metadata.ASCII_STRING_MARSHALLER); + headers.discardAll(key); } } @@ -67,11 +70,19 @@ private void updateHeader(final HeaderValueOption option, Metadata mutableHeader boolean keepEmptyValue = option.keepEmptyValue(); if (header.key().endsWith(Metadata.BINARY_HEADER_SUFFIX)) { - updateHeader(action, Metadata.Key.of(header.key(), Metadata.BINARY_BYTE_MARSHALLER), - header.rawValue().get().toByteArray(), mutableHeaders, keepEmptyValue); + if (header.rawValue().isPresent()) { + updateHeader(action, Metadata.Key.of(header.key(), Metadata.BINARY_BYTE_MARSHALLER), + header.rawValue().get().toByteArray(), mutableHeaders, keepEmptyValue); + } else { + logger.fine("Missing binary rawValue for header: " + header.key()); + } } else { - updateHeader(action, Metadata.Key.of(header.key(), Metadata.ASCII_STRING_MARSHALLER), - header.value().get(), mutableHeaders, keepEmptyValue); + if (header.value().isPresent()) { + updateHeader(action, Metadata.Key.of(header.key(), Metadata.ASCII_STRING_MARSHALLER), + header.value().get(), mutableHeaders, keepEmptyValue); + } else { + logger.fine("Missing value for header: " + header.key()); + } } } From c17c86abb888e014d0a036798c8a46b2e11b2199 Mon Sep 17 00:00:00 2001 From: Saurav Date: Fri, 27 Mar 2026 20:21:06 +0000 Subject: [PATCH 07/12] Fixup 12494: Improve test coverage for headermutations --- .../headermutations/HeaderMutatorTest.java | 48 +++++++++++++++++-- 1 file changed, 44 insertions(+), 4 deletions(-) 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 a0059b14afc..fd2b00284a4 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 @@ -77,7 +77,7 @@ private static HeaderValueOption header(String key, String value, HeaderAppendAc } @Test - public void applyRequestMutations_asciiHeaders() { + public void applyMutations_asciiHeaders() { Metadata headers = new Metadata(); headers.put(APPEND_KEY, "append-value-1"); headers.put(ADD_KEY, "add-value-original"); @@ -125,7 +125,7 @@ public void applyRequestMutations_asciiHeaders() { } @Test - public void applyRequestMutations_removalHasPriority() { + public void applyMutations_removalHasPriority() { Metadata headers = new Metadata(); headers.put(REMOVE_KEY, "value"); HeaderMutations mutations = @@ -141,7 +141,7 @@ public void applyRequestMutations_removalHasPriority() { } @Test - public void applyRequestMutations_binary() { + public void applyMutations_binary() { Metadata headers = new Metadata(); byte[] value = new byte[] {1, 2, 3}; HeaderValueOption option = @@ -203,7 +203,7 @@ public void applyResponseMutations_binary() { } @Test - public void applyRequestMutations_keepEmptyValue() { + public void applyMutations_keepEmptyValue() { Metadata headers = new Metadata(); headers.put(APPEND_KEY, "existing-value"); headers.put(OVERWRITE_KEY, "existing-value"); @@ -243,4 +243,44 @@ public void applyRequestMutations_keepEmptyValue() { assertThat(headers.containsKey(keepEmptyOverwriteKey)).isTrue(); assertThat(headers.get(keepEmptyOverwriteKey)).isEqualTo(""); } + + @Test + public void applyMutations_binaryRemoval() { + Metadata headers = new Metadata(); + byte[] value = new byte[] {1, 2, 3}; + headers.put(BINARY_KEY, value); + HeaderMutations mutations = + HeaderMutations.create(ImmutableList.of(), ImmutableList.of(BINARY_KEY.name())); + + headerMutator.applyMutations(mutations, headers); + + assertThat(headers.containsKey(BINARY_KEY)).isFalse(); + } + + @Test + 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); + + headerMutator.applyMutations( + HeaderMutations.create(ImmutableList.of(option), ImmutableList.of()), headers); + + Metadata.Key key = Metadata.Key.of("some-key-bin", Metadata.BINARY_BYTE_MARSHALLER); + assertThat(headers.containsKey(key)).isFalse(); + } + + @Test + 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); + + headerMutator.applyMutations( + HeaderMutations.create(ImmutableList.of(option), ImmutableList.of()), headers); + + Metadata.Key key = Metadata.Key.of("some-key", Metadata.ASCII_STRING_MARSHALLER); + assertThat(headers.containsKey(key)).isFalse(); + } } From e046a491ac049f57ae605b471b6888540e62096b Mon Sep 17 00:00:00 2001 From: Saurav Date: Tue, 31 Mar 2026 13:01:38 +0000 Subject: [PATCH 08/12] Fixup #12494: Fix documentation --- .../io/grpc/xds/internal/headermutations/HeaderMutator.java | 4 ++-- 1 file changed, 2 insertions(+), 2 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 6e80389a50c..cac236a50e7 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 @@ -23,8 +23,8 @@ import java.util.logging.Logger; /** - * The HeaderMutator class is an implementation of the HeaderMutator interface. It provides methods - * to apply header mutations to a given set of headers based on a given set of rules. + * The HeaderMutator provides methods to apply header mutations to a given set of headers based on a + * given set of rules. */ public class HeaderMutator { From b9caa4743b07667fb901e926ee506f52f88b63dd Mon Sep 17 00:00:00 2001 From: Saurav Date: Wed, 22 Apr 2026 11:41:07 +0000 Subject: [PATCH 09/12] Make `keepEmptyvalue` consistent with envoy impl https://github.com/grpc/proposal/pull/481/changes#r3022089452 --- .../headermutations/HeaderMutator.java | 45 +++++-------------- .../headermutations/HeaderMutatorTest.java | 5 ++- 2 files changed, 15 insertions(+), 35 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 cac236a50e7..e6cdc126f22 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 @@ -71,15 +71,21 @@ private void updateHeader(final HeaderValueOption option, Metadata mutableHeader if (header.key().endsWith(Metadata.BINARY_HEADER_SUFFIX)) { if (header.rawValue().isPresent()) { - updateHeader(action, Metadata.Key.of(header.key(), Metadata.BINARY_BYTE_MARSHALLER), - header.rawValue().get().toByteArray(), mutableHeaders, keepEmptyValue); + byte[] value = header.rawValue().get().toByteArray(); + if (value.length > 0 || keepEmptyValue) { + updateHeader(action, Metadata.Key.of(header.key(), Metadata.BINARY_BYTE_MARSHALLER), + value, mutableHeaders); + } } else { logger.fine("Missing binary rawValue for header: " + header.key()); } } else { if (header.value().isPresent()) { - updateHeader(action, Metadata.Key.of(header.key(), Metadata.ASCII_STRING_MARSHALLER), - header.value().get(), mutableHeaders, keepEmptyValue); + String value = header.value().get(); + if (!value.isEmpty() || keepEmptyValue) { + updateHeader(action, Metadata.Key.of(header.key(), Metadata.ASCII_STRING_MARSHALLER), + value, mutableHeaders); + } } else { logger.fine("Missing value for header: " + header.key()); } @@ -87,7 +93,7 @@ private void updateHeader(final HeaderValueOption option, Metadata mutableHeader } private void updateHeader(final HeaderAppendAction action, final Metadata.Key key, - final T value, Metadata mutableHeaders, boolean keepEmptyValue) { + final T value, Metadata mutableHeaders) { switch (action) { case APPEND_IF_EXISTS_OR_ADD: mutableHeaders.put(key, value); @@ -112,33 +118,6 @@ private void updateHeader(final HeaderAppendAction action, final Metadata.Ke // Should be unreachable unless there's a proto schema mismatch. logger.fine("Unknown HeaderAppendAction: " + action); } - - if (!keepEmptyValue) { - checkAndRemoveEmpty(key, mutableHeaders); - } - } - - private void checkAndRemoveEmpty(Metadata.Key key, Metadata headers) { - Iterable values = headers.getAll(key); - if (values == null) { - return; - } - boolean allEmpty = true; - for (T val : values) { - if (val instanceof String) { - if (!((String) val).isEmpty()) { - allEmpty = false; - break; - } - } else if (val instanceof byte[]) { - if (((byte[]) val).length > 0) { - allEmpty = false; - break; - } - } - } - if (allEmpty) { - headers.discardAll(key); - } } } + 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 fd2b00284a4..bc6734ff6fb 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 @@ -230,8 +230,8 @@ 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.containsKey(OVERWRITE_KEY)).isFalse(); + assertThat(headers.getAll(APPEND_KEY)).containsExactly("existing-value"); + assertThat(headers.get(OVERWRITE_KEY)).isEqualTo("existing-value"); Metadata.Key keepEmptyKey = Metadata.Key.of("keep-empty-key", Metadata.ASCII_STRING_MARSHALLER); @@ -242,6 +242,7 @@ public void applyMutations_keepEmptyValue() { assertThat(headers.get(keepEmptyKey)).isEqualTo(""); assertThat(headers.containsKey(keepEmptyOverwriteKey)).isTrue(); assertThat(headers.get(keepEmptyOverwriteKey)).isEqualTo(""); + } @Test From d2757fb29451d45e7b658da0bb0272f0887ffe7e Mon Sep 17 00:00:00 2001 From: Saurav Date: Wed, 22 Apr 2026 13:08:00 +0000 Subject: [PATCH 10/12] Handle gemini review comments. --- .../headermutations/HeaderMutationFilter.java | 5 ++-- .../HeaderMutationFilterTest.java | 23 +++++++++++++++++-- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java index 4c6f90b54fa..943f867fe4d 100644 --- a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java +++ b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java @@ -97,8 +97,9 @@ private boolean isHeaderMutationAllowed(String headerName, && rules.disallowExpression().get().matcher(headerName).matches()) { return false; } - if (rules.allowExpression().isPresent()) { - return rules.allowExpression().get().matcher(headerName).matches(); + if (rules.allowExpression().isPresent() + && rules.allowExpression().get().matcher(headerName).matches()) { + return true; } return !rules.disallowAll(); } 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 2331bdb2a46..10efae63842 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 @@ -154,11 +154,30 @@ public void filter_allowExpression_onlyAllowsRelevantExpressions() HeaderMutations filtered = filter.filter(mutations); - assertThat(filtered.headersToRemove()).containsExactly("x-allowed-remove"); + assertThat(filtered.headersToRemove()).containsExactly("x-allowed-remove", + "not-allowed-remove"); assertThat(filtered.headers()).containsExactly(header("x-allowed-key", "value"), - header("x-allowed-resp-key", "value")); + header("not-allowed-key", "value"), header("x-allowed-resp-key", "value"), + header("not-allowed-resp-key", "value")); + } + + @Test + public void filter_allowExpression_fallsThroughToDisallowAll() + throws HeaderMutationDisallowedException { + HeaderMutationRulesConfig rules = HeaderMutationRulesConfig.builder().disallowAll(true) + .allowExpression(Pattern.compile("^x-allowed-.*")).build(); + HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); + HeaderMutations mutations = HeaderMutations.create( + ImmutableList.of(header("x-allowed-key", "value"), header("not-allowed-key", "value")), + ImmutableList.of("x-allowed-remove", "not-allowed-remove")); + + HeaderMutations filtered = filter.filter(mutations); + + assertThat(filtered.headersToRemove()).containsExactly("x-allowed-remove"); + assertThat(filtered.headers()).containsExactly(header("x-allowed-key", "value")); } + @Test public void filter_allowExpression_overridesDisallowAll() throws HeaderMutationDisallowedException { From 9640f875e52aa175762412782c2ee75d8a83ee9f Mon Sep 17 00:00:00 2001 From: Saurav Date: Wed, 22 Apr 2026 14:00:42 +0000 Subject: [PATCH 11/12] Change HeaderMutationFilter to match envoy's implementation --- .../headermutations/HeaderMutationFilter.java | 12 ++++-- .../HeaderMutationFilterTest.java | 38 +++++++++++++++---- 2 files changed, 39 insertions(+), 11 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java index 943f867fe4d..35cab17d928 100644 --- a/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java +++ b/xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java @@ -62,10 +62,14 @@ private ImmutableList filterCollection(Collection items, throws HeaderMutationDisallowedException { ImmutableList.Builder allowed = ImmutableList.builder(); for (T item : items) { - if (isIgnoredPredicate.test(item)) { - continue; - } - if (isAllowedPredicate.test(item)) { + boolean isIgnored = isIgnoredPredicate.test(item); + boolean isAllowed = isAllowedPredicate.test(item); + + // TODO(sauravzg): The specification is ambiguous regarding whether system headers + // should be silently ignored or trigger an error when disallowIsError is enabled. + // We default to triggering errors matching Envoy's implementation. + // Ref: https://github.com/grpc/proposal/pull/481#discussion_r3124453674 + if (!isIgnored && isAllowed) { allowed.add(item); } else if (disallowIsError()) { throw new HeaderMutationDisallowedException("Header mutation disallowed"); 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 10efae63842..278e2f2160e 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 @@ -245,15 +245,39 @@ public void filter_ignoresUppercaseHeaders() throws HeaderMutationDisallowedExce assertThat(filtered.headersToRemove()).containsExactly("lower-remove"); } - @Test - public void filter_ignoresGrpcHeadersInRemoval() throws HeaderMutationDisallowedException { - HeaderMutationFilter filter = new HeaderMutationFilter(Optional.empty()); + @Test(expected = HeaderMutationDisallowedException.class) + public void filter_disallowIsError_throwsExceptionOnUppercaseHeaders() + throws HeaderMutationDisallowedException { + HeaderMutationRulesConfig rules = + HeaderMutationRulesConfig.builder().disallowIsError(true).build(); + HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); HeaderMutations mutations = HeaderMutations.create( - ImmutableList.of(), - ImmutableList.of("grpc-timeout", "valid-remove")); + ImmutableList.of(header("Valid-Key", "value")), + ImmutableList.of()); + filter.filter(mutations); + } - HeaderMutations filtered = filter.filter(mutations); + @Test(expected = HeaderMutationDisallowedException.class) + public void filter_disallowIsError_throwsExceptionOnSystemHeadersRemoval() + throws HeaderMutationDisallowedException { + HeaderMutationRulesConfig rules = + HeaderMutationRulesConfig.builder().disallowIsError(true).build(); + HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); + HeaderMutations mutations = HeaderMutations.create( + ImmutableList.of(), + ImmutableList.of(":path")); + filter.filter(mutations); + } - assertThat(filtered.headersToRemove()).containsExactly("valid-remove"); + @Test(expected = HeaderMutationDisallowedException.class) + public void filter_disallowIsError_throwsExceptionOnSystemHeadersModification() + throws HeaderMutationDisallowedException { + HeaderMutationRulesConfig rules = + HeaderMutationRulesConfig.builder().disallowIsError(true).build(); + HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); + HeaderMutations mutations = HeaderMutations.create( + ImmutableList.of(header(":path", "/new-path")), + ImmutableList.of()); + filter.filter(mutations); } } From 985f1c654907247831cf251a6374443c391b8eb7 Mon Sep 17 00:00:00 2001 From: Saurav Date: Thu, 23 Apr 2026 15:59:16 +0000 Subject: [PATCH 12/12] Fixup 12494: Improve test coverage --- .../HeaderMutationFilterTest.java | 321 ++++++++---------- .../headermutations/HeaderMutatorTest.java | 32 +- 2 files changed, 170 insertions(+), 183 deletions(-) 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 278e2f2160e..c0598590ebc 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 @@ -17,10 +17,12 @@ package io.grpc.xds.internal.headermutations; import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; +import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; +import com.google.protobuf.ByteString; import com.google.re2j.Pattern; -import io.grpc.xds.internal.headermutations.HeaderMutations; import io.grpc.xds.internal.headermutations.HeaderValueOption.HeaderAppendAction; import java.util.Optional; import org.junit.Test; @@ -30,85 +32,86 @@ @RunWith(JUnit4.class) public class HeaderMutationFilterTest { - private static HeaderValueOption header(String key, String value) { + private static final int MAX_HEADER_LENGTH = 16384; + + 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, false); } - private static HeaderValueOption header(String key, String value, HeaderAppendAction action) { + private static HeaderValueOption header(String key, String value) { return HeaderValueOption.create(io.grpc.xds.internal.grpcservice.HeaderValue.create(key, value), - action, - false); + HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, false); } @Test - public void filter_removesImmutableHeaders() throws HeaderMutationDisallowedException { + public void filter_validationRules_dropsInvalidHeaders() throws Exception { HeaderMutationFilter filter = new HeaderMutationFilter(Optional.empty()); - HeaderMutations mutations = HeaderMutations.create( - ImmutableList.of(header("add-key", "add-value"), header(":authority", "new-authority"), - header("host", "new-host"), header(":scheme", "https"), header(":method", "PUT"), - header("resp-add-key", "resp-add-value"), header(":scheme", "https")), - ImmutableList.of("remove-key", "host", ":authority", ":scheme", ":method")); - - HeaderMutations filtered = filter.filter(mutations); + String longString = Strings.repeat("a", MAX_HEADER_LENGTH + 1); + ByteString longBytes = ByteString.copyFrom(new byte[MAX_HEADER_LENGTH + 1]); - - assertThat(filtered.headersToRemove()).containsExactly("remove-key"); - assertThat(filtered.headers()).containsExactly(header("add-key", "add-value"), - header("resp-add-key", "resp-add-value")); - } - - @Test - public void filter_cannotAppendToSystemHeaders() throws HeaderMutationDisallowedException { - HeaderMutationFilter filter = new HeaderMutationFilter(Optional.empty()); HeaderMutations mutations = HeaderMutations.create( ImmutableList.of( - header("add-key", "add-value", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), - header(":authority", "new-authority", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), - header("host", "new-host", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), - header(":path", "/new-path", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), - header("host", "new-host", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD)), - ImmutableList.of()); + header("add-key", "add-value"), header(":authority", "new-authority"), + header("host", "new-host"), header(":scheme", "https"), header(":method", "PUT"), + header("resp-add-key", "resp-add-value"), header(":scheme", "https"), + header(":path", "/new-path"), header(":grpc-trace-bin", "binary-value"), + header(":alt-svc", "h3=:443"), header("user-agent", "new-agent"), + header("Valid-Key", "value"), header("", "value"), header(longString, "value"), + header("long-value-key", longString), header("long-bin-key-bin", longBytes), + header("grpc-timeout", "10S"), header("valid-key-lower", "value")), + ImmutableList.of("remove-key", "host", ":authority", ":scheme", ":method", ":foo", ":bar", + "Valid-Key", "", longString, "grpc-timeout", "UPPER-REMOVE", "lower-remove")); HeaderMutations filtered = filter.filter(mutations); + assertThat(filtered.headersToRemove()).containsExactly("remove-key", "lower-remove"); assertThat(filtered.headers()).containsExactly( - header("add-key", "add-value", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD)); + header("add-key", "add-value"), header("resp-add-key", "resp-add-value"), + header("user-agent", "new-agent"), header("valid-key-lower", "value")); } @Test - public void filter_cannotRemoveSystemHeaders() throws HeaderMutationDisallowedException { - HeaderMutationFilter filter = new HeaderMutationFilter(Optional.empty()); - HeaderMutations mutations = HeaderMutations.create( - ImmutableList.of(), - ImmutableList.of("remove-key", "host", ":foo", ":bar")); + public void filter_validationRules_throwsOnInvalidHeaders() throws Exception { + HeaderMutationRulesConfig rules = + HeaderMutationRulesConfig.builder().disallowIsError(true).build(); + HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); + String longString = Strings.repeat("a", MAX_HEADER_LENGTH + 1); - HeaderMutations filtered = filter.filter(mutations); + // Test system headers modification + assertThrows(HeaderMutationDisallowedException.class, () -> filter.filter(HeaderMutations + .create( + ImmutableList.of(header(":path", "/new-path")), ImmutableList.of()))); - assertThat(filtered.headersToRemove()).containsExactly("remove-key"); - } + // Test system headers removal + assertThrows(HeaderMutationDisallowedException.class, + () -> filter.filter(HeaderMutations.create( + ImmutableList.of(), ImmutableList.of(":path")))); - @Test - public void filter_cannotOverrideSystemHeaders() - throws HeaderMutationDisallowedException { - HeaderMutationFilter filter = new HeaderMutationFilter(Optional.empty()); - HeaderMutations mutations = HeaderMutations.create( - ImmutableList.of(header("user-agent", "new-agent"), - header(":path", "/new/path", HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD), - header(":grpc-trace-bin", "binary-value", HeaderAppendAction.ADD_IF_ABSENT), - header(":alt-svc", "h3=:443", HeaderAppendAction.OVERWRITE_IF_EXISTS)), - ImmutableList.of()); + // Test uppercase header modification + assertThrows(HeaderMutationDisallowedException.class, () -> filter.filter(HeaderMutations + .create( + ImmutableList.of(header("Valid-Key", "value")), ImmutableList.of()))); - HeaderMutations filtered = filter.filter(mutations); + // Test uppercase header removal + assertThrows(HeaderMutationDisallowedException.class, () -> filter + .filter(HeaderMutations.create( + ImmutableList.of(), ImmutableList.of("UPPER-REMOVE")))); - // System headers should be filtered out - assertThat(filtered.headers()).containsExactly( - header("user-agent", "new-agent")); + // Test empty header + assertThrows(HeaderMutationDisallowedException.class, () -> filter + .filter(HeaderMutations.create( + ImmutableList.of(header("", "value")), ImmutableList.of()))); + + // Test long header key + assertThrows(HeaderMutationDisallowedException.class, () -> filter + .filter(HeaderMutations.create( + ImmutableList.of(), ImmutableList.of(longString)))); } + @Test - public void filter_disallowAll_disablesAllModifications() - throws HeaderMutationDisallowedException { + public void filter_mutationRules_disallowAll_dropsAll() throws Exception { HeaderMutationRulesConfig rules = HeaderMutationRulesConfig.builder().disallowAll(true).build(); HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); HeaderMutations mutations = HeaderMutations.create( @@ -122,8 +125,30 @@ public void filter_disallowAll_disablesAllModifications() } @Test - public void filter_disallowExpression_filtersRelevantExpressions() - throws HeaderMutationDisallowedException { + public void filter_mutationRules_disallowAll_throws() throws Exception { + HeaderMutationRulesConfig rules = + HeaderMutationRulesConfig.builder().disallowAll(true).disallowIsError(true).build(); + HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); + + // Test add header + assertThrows(HeaderMutationDisallowedException.class, () -> filter.filter(HeaderMutations + .create( + ImmutableList.of(header("add-key", "add-value")), ImmutableList.of()))); + + // Test remove header + assertThrows(HeaderMutationDisallowedException.class, () -> filter + .filter(HeaderMutations.create( + ImmutableList.of(), ImmutableList.of("remove-key")))); + + // Test response header + assertThrows(HeaderMutationDisallowedException.class, () -> filter.filter(HeaderMutations + .create( + ImmutableList.of(header("resp-add-key", "resp-add-value")), ImmutableList.of()))); + } + + + @Test + public void filter_mutationRules_disallowExpression_dropsMatching() throws Exception { HeaderMutationRulesConfig rules = HeaderMutationRulesConfig.builder() .disallowExpression(Pattern.compile("^x-private-.*")).build(); HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); @@ -134,150 +159,84 @@ public void filter_disallowExpression_filtersRelevantExpressions() HeaderMutations filtered = filter.filter(mutations); - assertThat(filtered.headersToRemove()).containsExactly("x-public-remove"); assertThat(filtered.headers()).containsExactly(header("x-public", "value"), header("x-public-resp", "value")); } @Test - public void filter_allowExpression_onlyAllowsRelevantExpressions() - throws HeaderMutationDisallowedException { + public void filter_mutationRules_disallowExpression_throws() throws Exception { HeaderMutationRulesConfig rules = HeaderMutationRulesConfig.builder() - .allowExpression(Pattern.compile("^x-allowed-.*")).build(); - HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); - HeaderMutations mutations = HeaderMutations.create( - ImmutableList.of(header("x-allowed-key", "value"), header("not-allowed-key", "value"), - header("x-allowed-resp-key", "value"), header("not-allowed-resp-key", "value")), - ImmutableList.of("x-allowed-remove", "not-allowed-remove")); - - HeaderMutations filtered = filter.filter(mutations); - - - assertThat(filtered.headersToRemove()).containsExactly("x-allowed-remove", - "not-allowed-remove"); - assertThat(filtered.headers()).containsExactly(header("x-allowed-key", "value"), - header("not-allowed-key", "value"), header("x-allowed-resp-key", "value"), - header("not-allowed-resp-key", "value")); - } - - @Test - public void filter_allowExpression_fallsThroughToDisallowAll() - throws HeaderMutationDisallowedException { - HeaderMutationRulesConfig rules = HeaderMutationRulesConfig.builder().disallowAll(true) - .allowExpression(Pattern.compile("^x-allowed-.*")).build(); + .disallowExpression(Pattern.compile("^x-private-.*")).disallowIsError(true).build(); HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); - HeaderMutations mutations = HeaderMutations.create( - ImmutableList.of(header("x-allowed-key", "value"), header("not-allowed-key", "value")), - ImmutableList.of("x-allowed-remove", "not-allowed-remove")); - HeaderMutations filtered = filter.filter(mutations); + // Test disallowed key modification + assertThrows(HeaderMutationDisallowedException.class, () -> filter.filter(HeaderMutations + .create( + ImmutableList.of(header("x-private-key", "value")), ImmutableList.of()))); - assertThat(filtered.headersToRemove()).containsExactly("x-allowed-remove"); - assertThat(filtered.headers()).containsExactly(header("x-allowed-key", "value")); + // Test disallowed key removal + assertThrows(HeaderMutationDisallowedException.class, () -> filter + .filter(HeaderMutations.create( + ImmutableList.of(), ImmutableList.of("x-private-remove")))); } @Test - public void filter_allowExpression_overridesDisallowAll() - throws HeaderMutationDisallowedException { - HeaderMutationRulesConfig rules = HeaderMutationRulesConfig.builder().disallowAll(true) - .allowExpression(Pattern.compile("^x-allowed-.*")).build(); + public void filter_mutationRules_precedence() throws Exception { + HeaderMutationRulesConfig rules = HeaderMutationRulesConfig.builder() + .disallowAll(true) + .allowExpression(Pattern.compile("^x-allowed-.*")) + .disallowExpression(Pattern.compile("^x-allowed-but-disallowed-.*")) + .build(); HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); - HeaderMutations mutations = HeaderMutations.create( - ImmutableList.of(header("x-allowed-key", "value"), header("not-allowed", "value"), - header("x-allowed-resp-key", "value"), header("not-allowed-resp-key", "value")), - ImmutableList.of("x-allowed-remove", "not-allowed-remove")); - - HeaderMutations filtered = filter.filter(mutations); - assertThat(filtered.headersToRemove()).containsExactly("x-allowed-remove"); - assertThat(filtered.headers()).containsExactly(header("x-allowed-key", "value"), - header("x-allowed-resp-key", "value")); - } - - @Test(expected = HeaderMutationDisallowedException.class) - public void filter_disallowIsError_throwsExceptionOnDisallowed() - throws HeaderMutationDisallowedException { - HeaderMutationRulesConfig rules = - HeaderMutationRulesConfig.builder().disallowAll(true).disallowIsError(true).build(); - HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); - HeaderMutations mutations = HeaderMutations.create( - ImmutableList.of(header("add-key", "add-value")), - ImmutableList.of()); - filter.filter(mutations); - } + // Case 1: allowExpression overrides disallowAll + HeaderMutations mutations1 = HeaderMutations.create( + ImmutableList.of(header("x-allowed-key", "value"), header("not-allowed", "value")), + ImmutableList.of("x-allowed-remove", "not-allowed-remove")); + HeaderMutations filtered1 = filter.filter(mutations1); + assertThat(filtered1.headersToRemove()).containsExactly("x-allowed-remove"); + assertThat(filtered1.headers()).containsExactly(header("x-allowed-key", "value")); - @Test(expected = HeaderMutationDisallowedException.class) - public void filter_disallowIsError_throwsExceptionOnDisallowedRemove() - throws HeaderMutationDisallowedException { - HeaderMutationRulesConfig rules = - HeaderMutationRulesConfig.builder().disallowAll(true).disallowIsError(true).build(); - HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); - HeaderMutations mutations = HeaderMutations.create( - ImmutableList.of(), - ImmutableList.of("remove-key")); - filter.filter(mutations); - } - - @Test(expected = HeaderMutationDisallowedException.class) - public void filter_disallowIsError_throwsExceptionOnDisallowedResponseHeader() - throws HeaderMutationDisallowedException { - HeaderMutationRulesConfig rules = - HeaderMutationRulesConfig.builder().disallowAll(true).disallowIsError(true).build(); - HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); - HeaderMutations mutations = HeaderMutations.create( - ImmutableList.of(header("resp-add-key", "resp-add-value")), - ImmutableList.of()); - filter.filter(mutations); + // Case 2: disallowExpression overrides allowExpression + HeaderMutations mutations2 = HeaderMutations.create( + ImmutableList.of(header("x-allowed-but-disallowed-key", "value")), + ImmutableList.of("x-allowed-but-disallowed-remove")); + HeaderMutations filtered2 = filter.filter(mutations2); + assertThat(filtered2.headers()).isEmpty(); + assertThat(filtered2.headersToRemove()).isEmpty(); } @Test - public void filter_ignoresUppercaseHeaders() throws HeaderMutationDisallowedException { - HeaderMutationFilter filter = new HeaderMutationFilter(Optional.empty()); - HeaderMutations mutations = HeaderMutations.create( - ImmutableList.of(header("Valid-Key", "value"), header("valid-key", "value")), - ImmutableList.of("UPPER-REMOVE", "lower-remove")); - - HeaderMutations filtered = filter.filter(mutations); - - assertThat(filtered.headers()).containsExactly(header("valid-key", "value")); - assertThat(filtered.headersToRemove()).containsExactly("lower-remove"); - } - - @Test(expected = HeaderMutationDisallowedException.class) - public void filter_disallowIsError_throwsExceptionOnUppercaseHeaders() - throws HeaderMutationDisallowedException { - HeaderMutationRulesConfig rules = - HeaderMutationRulesConfig.builder().disallowIsError(true).build(); - HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); - HeaderMutations mutations = HeaderMutations.create( - ImmutableList.of(header("Valid-Key", "value")), - ImmutableList.of()); - filter.filter(mutations); - } - - @Test(expected = HeaderMutationDisallowedException.class) - public void filter_disallowIsError_throwsExceptionOnSystemHeadersRemoval() - throws HeaderMutationDisallowedException { - HeaderMutationRulesConfig rules = - HeaderMutationRulesConfig.builder().disallowIsError(true).build(); - HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); - HeaderMutations mutations = HeaderMutations.create( - ImmutableList.of(), - ImmutableList.of(":path")); - filter.filter(mutations); - } - - @Test(expected = HeaderMutationDisallowedException.class) - public void filter_disallowIsError_throwsExceptionOnSystemHeadersModification() - throws HeaderMutationDisallowedException { - HeaderMutationRulesConfig rules = - HeaderMutationRulesConfig.builder().disallowIsError(true).build(); - HeaderMutationFilter filter = new HeaderMutationFilter(Optional.of(rules)); - HeaderMutations mutations = HeaderMutations.create( - ImmutableList.of(header(":path", "/new-path")), - ImmutableList.of()); - filter.filter(mutations); + public void filter_mutationRules_precedence_throws() throws Exception { + // Case 1: allowExpression overrides disallowAll (does not throw) + HeaderMutationRulesConfig rules1 = HeaderMutationRulesConfig.builder() + .disallowAll(true) + .allowExpression(Pattern.compile("^x-allowed-.*")) + .disallowIsError(true) + .build(); + HeaderMutationFilter filter1 = new HeaderMutationFilter(Optional.of(rules1)); + HeaderMutations mutations1 = HeaderMutations.create( + ImmutableList.of(header("x-allowed-key", "value")), ImmutableList.of("x-allowed-remove")); + HeaderMutations filtered1 = filter1.filter(mutations1); + assertThat(filtered1.headersToRemove()).containsExactly("x-allowed-remove"); + assertThat(filtered1.headers()).containsExactly(header("x-allowed-key", "value")); + + // Case 2: disallowExpression overrides allowExpression (throws) + HeaderMutationRulesConfig rules2 = HeaderMutationRulesConfig.builder() + .allowExpression(Pattern.compile("^x-allowed-.*")) + .disallowExpression(Pattern.compile("^x-allowed-but-disallowed-.*")) + .disallowIsError(true) + .build(); + HeaderMutationFilter filter2 = new HeaderMutationFilter(Optional.of(rules2)); + assertThrows(HeaderMutationDisallowedException.class, + () -> filter2.filter(HeaderMutations.create( + ImmutableList.of(header("x-allowed-but-disallowed-key", "value")), + ImmutableList.of()))); + + assertThrows(HeaderMutationDisallowedException.class, () -> filter2.filter(HeaderMutations + .create( + ImmutableList.of(), ImmutableList.of("x-allowed-but-disallowed-remove")))); } } 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 bc6734ff6fb..b6806760f9b 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 @@ -207,6 +207,7 @@ public void applyMutations_keepEmptyValue() { Metadata headers = new Metadata(); headers.put(APPEND_KEY, "existing-value"); headers.put(OVERWRITE_KEY, "existing-value"); + headers.put(OVERWRITE_IF_EXISTS_KEY, "existing-value"); HeaderMutations mutations = HeaderMutations.create( @@ -214,24 +215,42 @@ public void applyMutations_keepEmptyValue() { header(NEW_ADD_KEY.name(), "", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), header(APPEND_KEY.name(), "", HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD), header(OVERWRITE_KEY.name(), "", HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD), - HeaderValueOption.create( + header(ADD_KEY.name(), "", HeaderAppendAction.ADD_IF_ABSENT), + header(OVERWRITE_IF_EXISTS_KEY.name(), "", HeaderAppendAction.OVERWRITE_IF_EXISTS), + HeaderValueOption.create( HeaderValue.create("keep-empty-key", ""), HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, true), HeaderValueOption.create( HeaderValue.create("keep-empty-overwrite-key", ""), HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD, - true)), + true), + HeaderValueOption.create( + HeaderValue.create("keep-empty-bin-key-bin", ByteString.EMPTY), + HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, true), + HeaderValueOption.create( + HeaderValue.create("ignore-empty-bin-key-bin", ByteString.EMPTY), + HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD, false), + HeaderValueOption.create( + HeaderValue.create("overwrite-empty-bin-key-bin", ByteString.EMPTY), + HeaderAppendAction.OVERWRITE_IF_EXISTS_OR_ADD, false)), ImmutableList.of()); headers.put( Metadata.Key.of("keep-empty-overwrite-key", Metadata.ASCII_STRING_MARSHALLER), "old"); + Metadata.Key overwriteEmptyBinKey = + Metadata.Key.of("overwrite-empty-bin-key-bin", Metadata.BINARY_BYTE_MARSHALLER); + byte[] originalBinValue = new byte[] {1, 2, 3}; + headers.put(overwriteEmptyBinKey, originalBinValue); + 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"); Metadata.Key keepEmptyKey = Metadata.Key.of("keep-empty-key", Metadata.ASCII_STRING_MARSHALLER); @@ -243,6 +262,15 @@ public void applyMutations_keepEmptyValue() { assertThat(headers.containsKey(keepEmptyOverwriteKey)).isTrue(); 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)).isTrue(); + assertThat(headers.get(keepEmptyBinKey)).isEqualTo(new byte[0]); + assertThat(headers.containsKey(ignoreEmptyBinKey)).isFalse(); + assertThat(headers.get(overwriteEmptyBinKey)).isEqualTo(originalBinValue); } @Test