Fix tests

Note that this fixes a bug in CommonTlsContextUtil where
CombinedValidationContext was not checked. I believe this was the only
location with such a bug as I audited all non-test usages of
has/getValidationContext() and confirmed they have have a cooresponding
has/getCombinedValidationContext().

Author: ejona86

xds/src/main/java/io/grpc/xds/internal/security/CommonTlsContextUtil.java

@@ -28,21 +28,18 @@ public static boolean hasCertProviderInstance(CommonTlsContext commonTlsContext)
     if (commonTlsContext == null) {
       return false;
     }
-    return hasIdentityCertificateProviderInstance(commonTlsContext)
-        || hasCertProviderValidationContext(commonTlsContext);
-  }
-
-  private static boolean hasCertProviderValidationContext(CommonTlsContext commonTlsContext) {
-    return hasValidationProviderInstance(commonTlsContext);
-  }
-
-  private static boolean hasIdentityCertificateProviderInstance(CommonTlsContext commonTlsContext) {
-    return commonTlsContext.hasTlsCertificateProviderInstance();
+    return commonTlsContext.hasTlsCertificateProviderInstance()
+        || hasValidationProviderInstance(commonTlsContext);
   }
 
   private static boolean hasValidationProviderInstance(CommonTlsContext commonTlsContext) {
-    return commonTlsContext.hasValidationContext() && commonTlsContext.getValidationContext()
-        .hasCaCertificateProviderInstance();
+    if (commonTlsContext.hasValidationContext() && commonTlsContext.getValidationContext()
+        .hasCaCertificateProviderInstance()) {
+      return true;
+    }
+    return commonTlsContext.hasCombinedValidationContext()
+        && commonTlsContext.getCombinedValidationContext().getDefaultValidationContext()
+          .hasCaCertificateProviderInstance();
   }
 
   /**

xds/src/test/java/io/grpc/xds/FilterChainMatchingProtocolNegotiatorsTest.java

@@ -1125,7 +1125,6 @@ public void filterChain_5stepMatch() throws Exception {
   }
 
   @Test
-  @SuppressWarnings("deprecation")
   public void filterChainMatch_unsupportedMatchers() throws Exception {
     EnvoyServerProtoData.DownstreamTlsContext tlsContext1 =
             CommonTlsContextTestsUtil.buildTestInternalDownstreamTlsContext("CERT1", "ROOTCA");
@@ -1194,7 +1193,7 @@ public void filterChainMatch_unsupportedMatchers() throws Exception {
     assertThat(sslSet.get()).isEqualTo(defaultFilterChain.sslContextProviderSupplier());
     assertThat(routingSettable.get()).isEqualTo(noopConfig);
     assertThat(sslSet.get().getTlsContext().getCommonTlsContext()
-            .getTlsCertificateCertificateProviderInstance()
+            .getTlsCertificateProviderInstance()
             .getCertificateName()).isEqualTo("CERT3");
   }
 

xds/src/test/java/io/grpc/xds/GrpcXdsClientImplDataTest.java

@@ -47,7 +47,6 @@
 import io.envoyproxy.envoy.config.route.v3.WeightedCluster;
 import io.envoyproxy.envoy.extensions.filters.http.router.v3.Router;
 import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CertificateProviderPluginInstance;
-import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CommonTlsContext;
 import io.grpc.BindableService;
 import io.grpc.ChannelCredentials;
 import io.grpc.Context;
@@ -2245,7 +2244,6 @@ public void cdsResponseWithCircuitBreakers() {
    * CDS response containing UpstreamTlsContext for a cluster.
    */
   @Test
-  @SuppressWarnings("deprecation")
   public void cdsResponseWithUpstreamTlsContext() {
     DiscoveryRpcCall call = startResourceWatcher(XdsClusterResource.getInstance(), CDS_RESOURCE,
         cdsResourceWatcher);
@@ -2269,9 +2267,9 @@ public void cdsResponseWithUpstreamTlsContext() {
     verify(cdsResourceWatcher, times(1))
         .onChanged(cdsUpdateCaptor.capture());
     CdsUpdate cdsUpdate = cdsUpdateCaptor.getValue();
-    CommonTlsContext.CertificateProviderInstance certificateProviderInstance =
+    CertificateProviderPluginInstance certificateProviderInstance =
         cdsUpdate.upstreamTlsContext().getCommonTlsContext().getCombinedValidationContext()
-            .getValidationContextCertificateProviderInstance();
+            .getDefaultValidationContext().getCaCertificateProviderInstance();
     assertThat(certificateProviderInstance.getInstanceName()).isEqualTo("cert-instance-name");
     assertThat(certificateProviderInstance.getCertificateName()).isEqualTo("cert1");
     verifyResourceMetadataAcked(CDS, CDS_RESOURCE, clusterEds, VERSION_1, TIME_INCREMENT);
@@ -2282,7 +2280,6 @@ public void cdsResponseWithUpstreamTlsContext() {
    * CDS response containing new UpstreamTlsContext for a cluster.
    */
   @Test
-  @SuppressWarnings("deprecation")
   public void cdsResponseWithNewUpstreamTlsContext() {
     DiscoveryRpcCall call = startResourceWatcher(XdsClusterResource.getInstance(), CDS_RESOURCE,
         cdsResourceWatcher);
@@ -2344,7 +2341,6 @@ public void cdsResponseErrorHandling_badUpstreamTlsContext() {
    * CDS response containing OutlierDetection for a cluster.
    */
   @Test
-  @SuppressWarnings("deprecation")
   public void cdsResponseWithOutlierDetection() {
     DiscoveryRpcCall call = startResourceWatcher(XdsClusterResource.getInstance(), CDS_RESOURCE,
         cdsResourceWatcher);
@@ -2413,7 +2409,6 @@ public void cdsResponseWithOutlierDetection() {
    * CDS response containing OutlierDetection for a cluster.
    */
   @Test
-  @SuppressWarnings("deprecation")
   public void cdsResponseWithInvalidOutlierDetectionNacks() {
 
     DiscoveryRpcCall call = startResourceWatcher(XdsClusterResource.getInstance(), CDS_RESOURCE,

xds/src/test/java/io/grpc/xds/GrpcXdsClientImplTestBase.java

@@ -613,18 +613,15 @@ protected Message buildLeastRequestLbConfig(int choiceCount) {
     }
 
     @Override
-    @SuppressWarnings("deprecation")
     protected Message buildUpstreamTlsContext(String instanceName, String certName) {
       CommonTlsContext.Builder commonTlsContextBuilder = CommonTlsContext.newBuilder();
       if (instanceName != null && certName != null) {
-        CommonTlsContext.CertificateProviderInstance providerInstance =
-            CommonTlsContext.CertificateProviderInstance.newBuilder()
-                .setInstanceName(instanceName)
-                .setCertificateName(certName)
-                .build();
         CommonTlsContext.CombinedCertificateValidationContext combined =
             CommonTlsContext.CombinedCertificateValidationContext.newBuilder()
-                .setValidationContextCertificateProviderInstance(providerInstance)
+                .setDefaultValidationContext(CertificateValidationContext.newBuilder()
+                  .setCaCertificateProviderInstance(CertificateProviderPluginInstance.newBuilder()
+                    .setInstanceName(instanceName)
+                    .setCertificateName(certName)))
                 .build();
         commonTlsContextBuilder.setCombinedValidationContext(combined);
       }
@@ -751,7 +748,6 @@ protected Message buildDropOverload(String category, int dropPerMillion) {
           .build();
     }
 
-    @SuppressWarnings("deprecation")
     @Override
     protected FilterChain buildFilterChain(
         List<String> alpn, Message tlsContext, String transportSocketName,

xds/src/test/java/io/grpc/xds/GrpcXdsClientImplV3Test.java

@@ -317,7 +317,6 @@ static void verifyWatcher(
         .isSameInstanceAs(sslContextProvider);
   }
 
-  @SuppressWarnings("deprecation")
   static CommonTlsContext.Builder addFilenames(
       CommonTlsContext.Builder builder, String certChain, String privateKey, String trustCa) {
     TlsCertificate tlsCert =
@@ -329,13 +328,10 @@ static CommonTlsContext.Builder addFilenames(
         CertificateValidationContext.newBuilder()
             .setTrustedCa(DataSource.newBuilder().setFilename(trustCa))
             .build();
-    CommonTlsContext.CertificateProviderInstance certificateProviderInstance =
-        builder.getValidationContextCertificateProviderInstance();
     CommonTlsContext.CombinedCertificateValidationContext.Builder combinedBuilder =
         CommonTlsContext.CombinedCertificateValidationContext.newBuilder();
     combinedBuilder
-        .setDefaultValidationContext(certContext)
-        .setValidationContextCertificateProviderInstance(certificateProviderInstance);
+        .setDefaultValidationContext(certContext);
     return builder
         .addTlsCertificates(tlsCert)
         .setCombinedValidationContext(combinedBuilder.build());

xds/src/test/java/io/grpc/xds/internal/security/ClientSslContextProviderFactoryTest.java

@@ -23,7 +23,6 @@
 import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CertificateProviderPluginInstance;
 import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CertificateValidationContext;
 import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CommonTlsContext;
-import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CommonTlsContext.CertificateProviderInstance;
 import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CommonTlsContext.CombinedCertificateValidationContext;
 import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.DownstreamTlsContext;
 import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.UpstreamTlsContext;
@@ -63,48 +62,26 @@ public class CommonTlsContextTestsUtil {
   public static final String BAD_CLIENT_KEY_FILE = "badclient.key";
 
   /** takes additional values and creates CombinedCertificateValidationContext as needed. */
-  @SuppressWarnings("deprecation")
-  static CommonTlsContext buildCommonTlsContextWithAdditionalValues(
+  private static CommonTlsContext buildCommonTlsContextWithAdditionalValues(
       String certInstanceName, String certName,
       String validationContextCertInstanceName, String validationContextCertName,
       Iterable<StringMatcher> matchSubjectAltNames,
       Iterable<String> alpnNames) {
-
-    CommonTlsContext.Builder builder = CommonTlsContext.newBuilder();
-
-    CertificateProviderInstance certificateProviderInstance = CertificateProviderInstance
-        .newBuilder().setInstanceName(certInstanceName).setCertificateName(certName).build();
-    if (certificateProviderInstance != null) {
-      builder.setTlsCertificateCertificateProviderInstance(certificateProviderInstance);
-    }
-    CertificateProviderInstance validationCertificateProviderInstance =
-        CertificateProviderInstance.newBuilder().setInstanceName(validationContextCertInstanceName)
-            .setCertificateName(validationContextCertName).build();
-    CertificateValidationContext certValidationContext =
-        matchSubjectAltNames == null
-            ? null
-            : CertificateValidationContext.newBuilder()
-                .addAllMatchSubjectAltNames(matchSubjectAltNames)
-                .build();
-    if (validationCertificateProviderInstance != null) {
-      CombinedCertificateValidationContext.Builder combinedBuilder =
-          CombinedCertificateValidationContext.newBuilder()
-              .setValidationContextCertificateProviderInstance(
-                  validationCertificateProviderInstance);
-      if (certValidationContext != null) {
-        combinedBuilder = combinedBuilder.setDefaultValidationContext(certValidationContext);
-      }
-      builder.setCombinedValidationContext(combinedBuilder);
-    } else if (validationCertificateProviderInstance != null) {
-      builder
-          .setValidationContextCertificateProviderInstance(validationCertificateProviderInstance);
-    } else if (certValidationContext != null) {
-      builder.setValidationContext(certValidationContext);
-    }
-    if (alpnNames != null) {
-      builder.addAllAlpnProtocols(alpnNames);
-    }
-    return builder.build();
+    @SuppressWarnings("deprecation") // gRFC A29 predates match_typed_subject_alt_names
+    CertificateValidationContext.Builder certificateValidationContextBuilder
+        = CertificateValidationContext.newBuilder()
+        .addAllMatchSubjectAltNames(matchSubjectAltNames);
+    return CommonTlsContext.newBuilder()
+        .setTlsCertificateProviderInstance(CertificateProviderPluginInstance.newBuilder()
+          .setInstanceName(certInstanceName)
+          .setCertificateName(certName))
+        .setCombinedValidationContext(CombinedCertificateValidationContext.newBuilder()
+          .setDefaultValidationContext(certificateValidationContextBuilder
+            .setCaCertificateProviderInstance(CertificateProviderPluginInstance.newBuilder()
+              .setInstanceName(validationContextCertInstanceName)
+              .setCertificateName(validationContextCertName))))
+        .addAllAlpnProtocols(alpnNames)
+        .build();
   }
 
   /** Helper method to build DownstreamTlsContext for multiple test classes. */
@@ -152,7 +129,7 @@ public static DownstreamTlsContext buildTestDownstreamTlsContext(
           useSans ? Arrays.asList(
               StringMatcher.newBuilder()
                   .setExact("spiffe://grpc-sds-testing.svc.id.goog/ns/default/sa/bob")
-                  .build()) : null,
+                  .build()) : Arrays.asList(),
           Arrays.asList("managed-tls"));
     }
     return buildDownstreamTlsContext(commonTlsContext, /* requireClientCert= */ false);
@@ -199,7 +176,6 @@ public static X509Certificate getCertFromResourceName(String resourceName)
     }
   }
 
-  @SuppressWarnings("deprecation")
   private static CommonTlsContext buildCommonTlsContextForCertProviderInstance(
       String certInstanceName,
       String certName,
@@ -210,10 +186,10 @@ private static CommonTlsContext buildCommonTlsContextForCertProviderInstance(
     CommonTlsContext.Builder builder = CommonTlsContext.newBuilder();
     if (certInstanceName != null) {
       builder =
-          builder.setTlsCertificateCertificateProviderInstance(
-              CommonTlsContext.CertificateProviderInstance.newBuilder()
-                  .setInstanceName(certInstanceName)
-                  .setCertificateName(certName));
+              builder.setTlsCertificateProviderInstance(
+                      CertificateProviderPluginInstance.newBuilder()
+                              .setInstanceName(certInstanceName)
+                              .setCertificateName(certName));
     }
     builder =
         addCertificateValidationContext(
@@ -248,35 +224,28 @@ private static CommonTlsContext buildNewCommonTlsContextForCertProviderInstance(
     return builder.build();
   }
 
-  @SuppressWarnings("deprecation")
   private static CommonTlsContext.Builder addCertificateValidationContext(
       CommonTlsContext.Builder builder,
       String rootInstanceName,
       String rootCertName,
       CertificateValidationContext staticCertValidationContext) {
-    CertificateProviderInstance providerInstance = null;
-    if (rootInstanceName != null) {
-      providerInstance = CertificateProviderInstance.newBuilder()
-          .setInstanceName(rootInstanceName)
-          .setCertificateName(rootCertName)
-          .build();
-    }
-    if (providerInstance != null) {
-      builder = builder.setValidationContextCertificateProviderInstance(providerInstance);
+    if (staticCertValidationContext == null && rootInstanceName == null) {
+      return builder;
     }
-    CombinedCertificateValidationContext.Builder combined =
-        CombinedCertificateValidationContext.newBuilder();
-    if (providerInstance != null) {
-      combined = combined.setValidationContextCertificateProviderInstance(providerInstance);
-    }
-    if (staticCertValidationContext != null) {
-      combined = combined.setDefaultValidationContext(staticCertValidationContext);
+    CertificateValidationContext.Builder contextBuilder;
+    if (staticCertValidationContext == null) {
+      contextBuilder = CertificateValidationContext.newBuilder();
+    } else {
+      contextBuilder = staticCertValidationContext.toBuilder();
     }
-    if (combined.hasValidationContextCertificateProviderInstance()
-        || combined.hasDefaultValidationContext()) {
-      builder = builder.setCombinedValidationContext(combined.build());
+    if (rootInstanceName != null) {
+      contextBuilder.setCaCertificateProviderInstance(CertificateProviderPluginInstance.newBuilder()
+          .setInstanceName(rootInstanceName)
+          .setCertificateName(rootCertName));
+      builder.setValidationContext(contextBuilder.build());
     }
-    return builder;
+    return builder.setCombinedValidationContext(CombinedCertificateValidationContext.newBuilder()
+        .setDefaultValidationContext(contextBuilder));
   }
 
   private static CommonTlsContext.Builder addNewCertificateValidationContext(

xds/src/test/java/io/grpc/xds/internal/security/CommonTlsContextTestsUtil.java

@@ -123,7 +123,6 @@ public void notifyCertUpdatesNotSupported_expectExceptionOnSecondCall() {
   }
 
   @Test
-  @SuppressWarnings("deprecation")
   public void onePluginSameConfig_sameInstance() {
     registerPlugin("plugin1");
     CertificateProvider.Watcher mockWatcher1 = mock(CertificateProvider.Watcher.class);
@@ -167,7 +166,6 @@ public void onePluginSameConfig_sameInstance() {
   }
 
   @Test
-  @SuppressWarnings("deprecation")
   public void onePluginSameConfig_secondWatcherAfterFirstNotify() {
     registerPlugin("plugin1");
     CertificateProvider.Watcher mockWatcher1 = mock(CertificateProvider.Watcher.class);
@@ -275,7 +273,6 @@ public void twoPlugins_differentInstance() {
         mockWatcher1, handle1, certProviderProvider1, mockWatcher2, handle2, certProviderProvider2);
   }
 
-  @SuppressWarnings("deprecation")
   private static void checkDifferentInstances(
       CertificateProvider.Watcher mockWatcher1,
       CertificateProviderStore.Handle handle1,