Change HeaderMutationFilter to match envoy's implementation

Author: sauravzg

xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java

@@ -62,10 +62,14 @@ private <T> ImmutableList<T> filterCollection(Collection<T> items,
       throws HeaderMutationDisallowedException {
     ImmutableList.Builder<T> 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");

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);
   }
 }