Fixup 12492: Address copilot comments

Author: sauravzg

xds/src/main/java/io/grpc/xds/internal/extauthz/ExtAuthzConfigParser.java

@@ -24,6 +24,7 @@
 import io.grpc.xds.internal.grpcservice.GrpcServiceConfigParser;
 import io.grpc.xds.internal.grpcservice.GrpcServiceParseException;
 import io.grpc.xds.internal.grpcservice.GrpcServiceXdsContextProvider;
+import io.grpc.xds.internal.headermutations.HeaderMutationRulesParseException;
 import io.grpc.xds.internal.headermutations.HeaderMutationRulesParser;
 
 
@@ -87,8 +88,12 @@ public static ExtAuthzConfig parse(
     }
 
     if (extAuthzProto.hasDecoderHeaderMutationRules()) {
-      builder.decoderHeaderMutationRules(
-          HeaderMutationRulesParser.parse(extAuthzProto.getDecoderHeaderMutationRules()));
+      try {
+        builder.decoderHeaderMutationRules(
+            HeaderMutationRulesParser.parse(extAuthzProto.getDecoderHeaderMutationRules()));
+      } catch (HeaderMutationRulesParseException e) {
+        throw new ExtAuthzParseException(e.getMessage(), e);
+      }
     }
 
     return builder.build();

xds/src/main/java/io/grpc/xds/internal/grpcservice/GrpcServiceConfigParser.java

@@ -21,6 +21,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.protobuf.Any;
 import com.google.protobuf.InvalidProtocolBufferException;
+import com.google.protobuf.util.Durations;
 import io.envoyproxy.envoy.config.core.v3.GrpcService;
 import io.envoyproxy.envoy.extensions.grpc_service.call_credentials.access_token.v3.AccessTokenCredentials;
 import io.envoyproxy.envoy.extensions.grpc_service.channel_credentials.xds.v3.XdsCredentials;
@@ -92,7 +93,7 @@ public static GrpcServiceConfig parse(GrpcService grpcServiceProto,
       } else {
         headerValue = HeaderValue.create(key, header.getValue());
       }
-      if (HeaderValueValidationUtils.shouldIgnore(headerValue)) {
+      if (HeaderValueValidationUtils.isDisallowed(headerValue)) {
         throw new GrpcServiceParseException("Invalid initial metadata header: " + key);
       }
       initialMetadata.add(headerValue);
@@ -101,9 +102,8 @@ public static GrpcServiceConfig parse(GrpcService grpcServiceProto,
 
     if (grpcServiceProto.hasTimeout()) {
       com.google.protobuf.Duration timeout = grpcServiceProto.getTimeout();
-      if (timeout.getSeconds() < 0 || timeout.getNanos() < 0
-          || (timeout.getSeconds() == 0 && timeout.getNanos() == 0)) {
-        throw new GrpcServiceParseException("Timeout must be strictly positive");
+      if (!Durations.isValid(timeout) || Durations.compare(timeout, Durations.ZERO) <= 0) {
+        throw new GrpcServiceParseException("Timeout must be strictly positive and valid");
       }
       builder.timeout(Duration.ofSeconds(timeout.getSeconds(), timeout.getNanos()));
     }

xds/src/main/java/io/grpc/xds/internal/grpcservice/HeaderValueValidationUtils.java

@@ -27,11 +27,11 @@ public final class HeaderValueValidationUtils {
   private HeaderValueValidationUtils() {}
 
   /**
-   * Returns true if the header key should be ignored for mutations or validation.
+   * Returns true if the header key is disallowed for mutations or validation.
    *
    * @param key The header key (e.g., "content-type")
    */
-  public static boolean shouldIgnore(String key) {
+  public static boolean isDisallowed(String key) {
     if (key.isEmpty() || key.length() > MAX_HEADER_LENGTH) {
       return true;
     }
@@ -48,12 +48,12 @@ public static boolean shouldIgnore(String key) {
   }
 
   /**
-   * Returns true if the header value should be ignored.
+   * Returns true if the header value is disallowed.
    *
    * @param header The HeaderValue containing key and values
    */
-  public static boolean shouldIgnore(HeaderValue header) {
-    if (shouldIgnore(header.key())) {
+  public static boolean isDisallowed(HeaderValue header) {
+    if (isDisallowed(header.key())) {
       return true;
     }
     if (header.value().isPresent() && header.value().get().length() > MAX_HEADER_LENGTH) {

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

@@ -55,7 +55,7 @@ public static Builder builder() {
   public abstract boolean disallowAll();
 
   /**
-   * If true, disallows any header mutation that would result in an invalid header value.
+   * If true, a disallowed header mutation will result in an error instead of being ignored.
    *
    * @see HeaderMutationRules#getDisallowIsError()
    */

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

@@ -0,0 +1,32 @@
+/*
+ * 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;
+
+/**
+ * Exception thrown when parsing header mutation rules fails.
+ */
+public final class HeaderMutationRulesParseException extends Exception {
+  private static final long serialVersionUID = 1L;
+
+  public HeaderMutationRulesParseException(String message) {
+    super(message);
+  }
+
+  public HeaderMutationRulesParseException(String message, Throwable cause) {
+    super(message, cause);
+  }
+}

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

@@ -19,7 +19,6 @@
 import com.google.re2j.Pattern;
 import com.google.re2j.PatternSyntaxException;
 import io.envoyproxy.envoy.config.common.mutation_rules.v3.HeaderMutationRules;
-import io.grpc.xds.internal.extauthz.ExtAuthzParseException;
 
 /**
  * Parser for {@link io.envoyproxy.envoy.config.common.mutation_rules.v3.HeaderMutationRules}.
@@ -29,7 +28,7 @@ public final class HeaderMutationRulesParser {
   private HeaderMutationRulesParser() {}
 
   public static HeaderMutationRulesConfig parse(HeaderMutationRules proto)
-      throws ExtAuthzParseException {
+      throws HeaderMutationRulesParseException {
     HeaderMutationRulesConfig.Builder builder = HeaderMutationRulesConfig.builder();
     builder.disallowAll(proto.getDisallowAll().getValue());
     builder.disallowIsError(proto.getDisallowIsError().getValue());
@@ -44,11 +43,12 @@ public static HeaderMutationRulesConfig parse(HeaderMutationRules proto)
     return builder.build();
   }
 
-  private static Pattern parseRegex(String regex, String fieldName) throws ExtAuthzParseException {
+  private static Pattern parseRegex(String regex, String fieldName)
+      throws HeaderMutationRulesParseException {
     try {
       return Pattern.compile(regex);
     } catch (PatternSyntaxException e) {
-      throw new ExtAuthzParseException(
+      throw new HeaderMutationRulesParseException(
           "Invalid regex pattern for " + fieldName + ": " + e.getMessage(), e);
     }
   }

xds/src/test/java/io/grpc/xds/internal/grpcservice/HeaderValueValidationUtilsTest.java

@@ -30,58 +30,58 @@
 public class HeaderValueValidationUtilsTest {
 
   @Test
-  public void shouldIgnore_string_emptyKey() {
-    assertThat(HeaderValueValidationUtils.shouldIgnore("")).isTrue();
+  public void isDisallowed_string_emptyKey() {
+    assertThat(HeaderValueValidationUtils.isDisallowed("")).isTrue();
   }
 
   @Test
-  public void shouldIgnore_string_tooLongKey() {
+  public void isDisallowed_string_tooLongKey() {
     String longKey = new String(new char[16385]).replace('\0', 'a');
-    assertThat(HeaderValueValidationUtils.shouldIgnore(longKey)).isTrue();
+    assertThat(HeaderValueValidationUtils.isDisallowed(longKey)).isTrue();
   }
 
   @Test
-  public void shouldIgnore_string_notLowercase() {
-    assertThat(HeaderValueValidationUtils.shouldIgnore("Content-Type")).isTrue();
+  public void isDisallowed_string_notLowercase() {
+    assertThat(HeaderValueValidationUtils.isDisallowed("Content-Type")).isTrue();
   }
 
   @Test
-  public void shouldIgnore_string_grpcPrefix() {
-    assertThat(HeaderValueValidationUtils.shouldIgnore("grpc-timeout")).isTrue();
+  public void isDisallowed_string_grpcPrefix() {
+    assertThat(HeaderValueValidationUtils.isDisallowed("grpc-timeout")).isTrue();
   }
 
   @Test
-  public void shouldIgnore_string_systemHeader_colon() {
-    assertThat(HeaderValueValidationUtils.shouldIgnore(":authority")).isTrue();
+  public void isDisallowed_string_systemHeader_colon() {
+    assertThat(HeaderValueValidationUtils.isDisallowed(":authority")).isTrue();
   }
 
   @Test
-  public void shouldIgnore_string_systemHeader_host() {
-    assertThat(HeaderValueValidationUtils.shouldIgnore("host")).isTrue();
+  public void isDisallowed_string_systemHeader_host() {
+    assertThat(HeaderValueValidationUtils.isDisallowed("host")).isTrue();
   }
 
   @Test
-  public void shouldIgnore_string_valid() {
-    assertThat(HeaderValueValidationUtils.shouldIgnore("content-type")).isFalse();
+  public void isDisallowed_string_valid() {
+    assertThat(HeaderValueValidationUtils.isDisallowed("content-type")).isFalse();
   }
 
   @Test
-  public void shouldIgnore_headerValue_tooLongValue() {
+  public void isDisallowed_headerValue_tooLongValue() {
     String longValue = new String(new char[16385]).replace('\0', 'v');
     HeaderValue header = HeaderValue.create("content-type", longValue);
-    assertThat(HeaderValueValidationUtils.shouldIgnore(header)).isTrue();
+    assertThat(HeaderValueValidationUtils.isDisallowed(header)).isTrue();
   }
 
   @Test
-  public void shouldIgnore_headerValue_tooLongRawValue() {
+  public void isDisallowed_headerValue_tooLongRawValue() {
     ByteString longRawValue = ByteString.copyFrom(new byte[16385]);
     HeaderValue header = HeaderValue.create("content-type", longRawValue);
-    assertThat(HeaderValueValidationUtils.shouldIgnore(header)).isTrue();
+    assertThat(HeaderValueValidationUtils.isDisallowed(header)).isTrue();
   }
 
   @Test
-  public void shouldIgnore_headerValue_valid() {
+  public void isDisallowed_headerValue_valid() {
     HeaderValue header = HeaderValue.create("content-type", "application/grpc");
-    assertThat(HeaderValueValidationUtils.shouldIgnore(header)).isFalse();
+    assertThat(HeaderValueValidationUtils.isDisallowed(header)).isFalse();
   }
 }

xds/src/test/java/io/grpc/xds/internal/headermutations/HeaderMutationRulesParserTest.java

@@ -22,7 +22,7 @@
 import com.google.protobuf.BoolValue;
 import io.envoyproxy.envoy.config.common.mutation_rules.v3.HeaderMutationRules;
 import io.envoyproxy.envoy.type.matcher.v3.RegexMatcher;
-import io.grpc.xds.internal.extauthz.ExtAuthzParseException;
+import io.grpc.xds.internal.headermutations.HeaderMutationRulesParseException;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -64,25 +64,25 @@ public void parse_protoWithNoExpressions_success() throws Exception {
   }
 
   @Test
-  public void parse_invalidRegexAllowExpression_throwsExtAuthzParseException() {
+  public void parse_invalidRegexAllowExpression_throwsHeaderMutationRulesParseException() {
     HeaderMutationRules proto = HeaderMutationRules.newBuilder()
         .setAllowExpression(RegexMatcher.newBuilder().setRegex("allow-["))
         .build();
 
-    ExtAuthzParseException exception = assertThrows(
-        ExtAuthzParseException.class, () -> HeaderMutationRulesParser.parse(proto));
+    HeaderMutationRulesParseException exception = assertThrows(
+        HeaderMutationRulesParseException.class, () -> HeaderMutationRulesParser.parse(proto));
 
     assertThat(exception).hasMessageThat().contains("Invalid regex pattern for allow_expression");
   }
 
   @Test
-  public void parse_invalidRegexDisallowExpression_throwsExtAuthzParseException() {
+  public void parse_invalidRegexDisallowExpression_throwsHeaderMutationRulesParseException() {
     HeaderMutationRules proto = HeaderMutationRules.newBuilder()
         .setDisallowExpression(RegexMatcher.newBuilder().setRegex("disallow-["))
         .build();
 
-    ExtAuthzParseException exception = assertThrows(
-        ExtAuthzParseException.class, () -> HeaderMutationRulesParser.parse(proto));
+    HeaderMutationRulesParseException exception = assertThrows(
+        HeaderMutationRulesParseException.class, () -> HeaderMutationRulesParser.parse(proto));
 
     assertThat(exception).hasMessageThat()
         .contains("Invalid regex pattern for disallow_expression");