Skip to content

Commit 87760b5

Browse files
ejona86ejona86
committed
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().
1 parent 6171a1a commit 87760b5

File tree

8 files changed

+97
-205
lines changed

8 files changed

+97
-205
lines changed

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

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -28,21 +28,18 @@ public static boolean hasCertProviderInstance(CommonTlsContext commonTlsContext)
2828
if (commonTlsContext == null) {
2929
return false;
3030
}
31-
return hasIdentityCertificateProviderInstance(commonTlsContext)
32-
|| hasCertProviderValidationContext(commonTlsContext);
33-
}
34-
35-
private static boolean hasCertProviderValidationContext(CommonTlsContext commonTlsContext) {
36-
return hasValidationProviderInstance(commonTlsContext);
37-
}
38-
39-
private static boolean hasIdentityCertificateProviderInstance(CommonTlsContext commonTlsContext) {
40-
return commonTlsContext.hasTlsCertificateProviderInstance();
31+
return commonTlsContext.hasTlsCertificateProviderInstance()
32+
|| hasValidationProviderInstance(commonTlsContext);
4133
}
4234

4335
private static boolean hasValidationProviderInstance(CommonTlsContext commonTlsContext) {
44-
return commonTlsContext.hasValidationContext() && commonTlsContext.getValidationContext()
45-
.hasCaCertificateProviderInstance();
36+
if (commonTlsContext.hasValidationContext() && commonTlsContext.getValidationContext()
37+
.hasCaCertificateProviderInstance()) {
38+
return true;
39+
}
40+
return commonTlsContext.hasCombinedValidationContext()
41+
&& commonTlsContext.getCombinedValidationContext().getDefaultValidationContext()
42+
.hasCaCertificateProviderInstance();
4643
}
4744

4845
/**

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1125,7 +1125,6 @@ public void filterChain_5stepMatch() throws Exception {
11251125
}
11261126

11271127
@Test
1128-
@SuppressWarnings("deprecation")
11291128
public void filterChainMatch_unsupportedMatchers() throws Exception {
11301129
EnvoyServerProtoData.DownstreamTlsContext tlsContext1 =
11311130
CommonTlsContextTestsUtil.buildTestInternalDownstreamTlsContext("CERT1", "ROOTCA");
@@ -1194,7 +1193,7 @@ public void filterChainMatch_unsupportedMatchers() throws Exception {
11941193
assertThat(sslSet.get()).isEqualTo(defaultFilterChain.sslContextProviderSupplier());
11951194
assertThat(routingSettable.get()).isEqualTo(noopConfig);
11961195
assertThat(sslSet.get().getTlsContext().getCommonTlsContext()
1197-
.getTlsCertificateCertificateProviderInstance()
1196+
.getTlsCertificateProviderInstance()
11981197
.getCertificateName()).isEqualTo("CERT3");
11991198
}
12001199

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

Lines changed: 45 additions & 102 deletions
Large diffs are not rendered by default.

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

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,6 @@
4747
import io.envoyproxy.envoy.config.route.v3.WeightedCluster;
4848
import io.envoyproxy.envoy.extensions.filters.http.router.v3.Router;
4949
import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CertificateProviderPluginInstance;
50-
import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CommonTlsContext;
5150
import io.grpc.BindableService;
5251
import io.grpc.ChannelCredentials;
5352
import io.grpc.Context;
@@ -2245,7 +2244,6 @@ public void cdsResponseWithCircuitBreakers() {
22452244
* CDS response containing UpstreamTlsContext for a cluster.
22462245
*/
22472246
@Test
2248-
@SuppressWarnings("deprecation")
22492247
public void cdsResponseWithUpstreamTlsContext() {
22502248
DiscoveryRpcCall call = startResourceWatcher(XdsClusterResource.getInstance(), CDS_RESOURCE,
22512249
cdsResourceWatcher);
@@ -2269,9 +2267,9 @@ public void cdsResponseWithUpstreamTlsContext() {
22692267
verify(cdsResourceWatcher, times(1))
22702268
.onChanged(cdsUpdateCaptor.capture());
22712269
CdsUpdate cdsUpdate = cdsUpdateCaptor.getValue();
2272-
CommonTlsContext.CertificateProviderInstance certificateProviderInstance =
2270+
CertificateProviderPluginInstance certificateProviderInstance =
22732271
cdsUpdate.upstreamTlsContext().getCommonTlsContext().getCombinedValidationContext()
2274-
.getValidationContextCertificateProviderInstance();
2272+
.getDefaultValidationContext().getCaCertificateProviderInstance();
22752273
assertThat(certificateProviderInstance.getInstanceName()).isEqualTo("cert-instance-name");
22762274
assertThat(certificateProviderInstance.getCertificateName()).isEqualTo("cert1");
22772275
verifyResourceMetadataAcked(CDS, CDS_RESOURCE, clusterEds, VERSION_1, TIME_INCREMENT);
@@ -2282,7 +2280,6 @@ public void cdsResponseWithUpstreamTlsContext() {
22822280
* CDS response containing new UpstreamTlsContext for a cluster.
22832281
*/
22842282
@Test
2285-
@SuppressWarnings("deprecation")
22862283
public void cdsResponseWithNewUpstreamTlsContext() {
22872284
DiscoveryRpcCall call = startResourceWatcher(XdsClusterResource.getInstance(), CDS_RESOURCE,
22882285
cdsResourceWatcher);
@@ -2344,7 +2341,6 @@ public void cdsResponseErrorHandling_badUpstreamTlsContext() {
23442341
* CDS response containing OutlierDetection for a cluster.
23452342
*/
23462343
@Test
2347-
@SuppressWarnings("deprecation")
23482344
public void cdsResponseWithOutlierDetection() {
23492345
DiscoveryRpcCall call = startResourceWatcher(XdsClusterResource.getInstance(), CDS_RESOURCE,
23502346
cdsResourceWatcher);
@@ -2413,7 +2409,6 @@ public void cdsResponseWithOutlierDetection() {
24132409
* CDS response containing OutlierDetection for a cluster.
24142410
*/
24152411
@Test
2416-
@SuppressWarnings("deprecation")
24172412
public void cdsResponseWithInvalidOutlierDetectionNacks() {
24182413

24192414
DiscoveryRpcCall call = startResourceWatcher(XdsClusterResource.getInstance(), CDS_RESOURCE,

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

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -613,18 +613,15 @@ protected Message buildLeastRequestLbConfig(int choiceCount) {
613613
}
614614

615615
@Override
616-
@SuppressWarnings("deprecation")
617616
protected Message buildUpstreamTlsContext(String instanceName, String certName) {
618617
CommonTlsContext.Builder commonTlsContextBuilder = CommonTlsContext.newBuilder();
619618
if (instanceName != null && certName != null) {
620-
CommonTlsContext.CertificateProviderInstance providerInstance =
621-
CommonTlsContext.CertificateProviderInstance.newBuilder()
622-
.setInstanceName(instanceName)
623-
.setCertificateName(certName)
624-
.build();
625619
CommonTlsContext.CombinedCertificateValidationContext combined =
626620
CommonTlsContext.CombinedCertificateValidationContext.newBuilder()
627-
.setValidationContextCertificateProviderInstance(providerInstance)
621+
.setDefaultValidationContext(CertificateValidationContext.newBuilder()
622+
.setCaCertificateProviderInstance(CertificateProviderPluginInstance.newBuilder()
623+
.setInstanceName(instanceName)
624+
.setCertificateName(certName)))
628625
.build();
629626
commonTlsContextBuilder.setCombinedValidationContext(combined);
630627
}
@@ -751,7 +748,6 @@ protected Message buildDropOverload(String category, int dropPerMillion) {
751748
.build();
752749
}
753750

754-
@SuppressWarnings("deprecation")
755751
@Override
756752
protected FilterChain buildFilterChain(
757753
List<String> alpn, Message tlsContext, String transportSocketName,

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

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -317,7 +317,6 @@ static void verifyWatcher(
317317
.isSameInstanceAs(sslContextProvider);
318318
}
319319

320-
@SuppressWarnings("deprecation")
321320
static CommonTlsContext.Builder addFilenames(
322321
CommonTlsContext.Builder builder, String certChain, String privateKey, String trustCa) {
323322
TlsCertificate tlsCert =
@@ -329,13 +328,10 @@ static CommonTlsContext.Builder addFilenames(
329328
CertificateValidationContext.newBuilder()
330329
.setTrustedCa(DataSource.newBuilder().setFilename(trustCa))
331330
.build();
332-
CommonTlsContext.CertificateProviderInstance certificateProviderInstance =
333-
builder.getValidationContextCertificateProviderInstance();
334331
CommonTlsContext.CombinedCertificateValidationContext.Builder combinedBuilder =
335332
CommonTlsContext.CombinedCertificateValidationContext.newBuilder();
336333
combinedBuilder
337-
.setDefaultValidationContext(certContext)
338-
.setValidationContextCertificateProviderInstance(certificateProviderInstance);
334+
.setDefaultValidationContext(certContext);
339335
return builder
340336
.addTlsCertificates(tlsCert)
341337
.setCombinedValidationContext(combinedBuilder.build());

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

Lines changed: 35 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CertificateProviderPluginInstance;
2424
import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CertificateValidationContext;
2525
import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CommonTlsContext;
26-
import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CommonTlsContext.CertificateProviderInstance;
2726
import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CommonTlsContext.CombinedCertificateValidationContext;
2827
import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.DownstreamTlsContext;
2928
import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.UpstreamTlsContext;
@@ -63,48 +62,26 @@ public class CommonTlsContextTestsUtil {
6362
public static final String BAD_CLIENT_KEY_FILE = "badclient.key";
6463

6564
/** takes additional values and creates CombinedCertificateValidationContext as needed. */
66-
@SuppressWarnings("deprecation")
67-
static CommonTlsContext buildCommonTlsContextWithAdditionalValues(
65+
private static CommonTlsContext buildCommonTlsContextWithAdditionalValues(
6866
String certInstanceName, String certName,
6967
String validationContextCertInstanceName, String validationContextCertName,
7068
Iterable<StringMatcher> matchSubjectAltNames,
7169
Iterable<String> alpnNames) {
72-
73-
CommonTlsContext.Builder builder = CommonTlsContext.newBuilder();
74-
75-
CertificateProviderInstance certificateProviderInstance = CertificateProviderInstance
76-
.newBuilder().setInstanceName(certInstanceName).setCertificateName(certName).build();
77-
if (certificateProviderInstance != null) {
78-
builder.setTlsCertificateCertificateProviderInstance(certificateProviderInstance);
79-
}
80-
CertificateProviderInstance validationCertificateProviderInstance =
81-
CertificateProviderInstance.newBuilder().setInstanceName(validationContextCertInstanceName)
82-
.setCertificateName(validationContextCertName).build();
83-
CertificateValidationContext certValidationContext =
84-
matchSubjectAltNames == null
85-
? null
86-
: CertificateValidationContext.newBuilder()
87-
.addAllMatchSubjectAltNames(matchSubjectAltNames)
88-
.build();
89-
if (validationCertificateProviderInstance != null) {
90-
CombinedCertificateValidationContext.Builder combinedBuilder =
91-
CombinedCertificateValidationContext.newBuilder()
92-
.setValidationContextCertificateProviderInstance(
93-
validationCertificateProviderInstance);
94-
if (certValidationContext != null) {
95-
combinedBuilder = combinedBuilder.setDefaultValidationContext(certValidationContext);
96-
}
97-
builder.setCombinedValidationContext(combinedBuilder);
98-
} else if (validationCertificateProviderInstance != null) {
99-
builder
100-
.setValidationContextCertificateProviderInstance(validationCertificateProviderInstance);
101-
} else if (certValidationContext != null) {
102-
builder.setValidationContext(certValidationContext);
103-
}
104-
if (alpnNames != null) {
105-
builder.addAllAlpnProtocols(alpnNames);
106-
}
107-
return builder.build();
70+
@SuppressWarnings("deprecation") // gRFC A29 predates match_typed_subject_alt_names
71+
CertificateValidationContext.Builder certificateValidationContextBuilder
72+
= CertificateValidationContext.newBuilder()
73+
.addAllMatchSubjectAltNames(matchSubjectAltNames);
74+
return CommonTlsContext.newBuilder()
75+
.setTlsCertificateProviderInstance(CertificateProviderPluginInstance.newBuilder()
76+
.setInstanceName(certInstanceName)
77+
.setCertificateName(certName))
78+
.setCombinedValidationContext(CombinedCertificateValidationContext.newBuilder()
79+
.setDefaultValidationContext(certificateValidationContextBuilder
80+
.setCaCertificateProviderInstance(CertificateProviderPluginInstance.newBuilder()
81+
.setInstanceName(validationContextCertInstanceName)
82+
.setCertificateName(validationContextCertName))))
83+
.addAllAlpnProtocols(alpnNames)
84+
.build();
10885
}
10986

11087
/** Helper method to build DownstreamTlsContext for multiple test classes. */
@@ -152,7 +129,7 @@ public static DownstreamTlsContext buildTestDownstreamTlsContext(
152129
useSans ? Arrays.asList(
153130
StringMatcher.newBuilder()
154131
.setExact("spiffe://grpc-sds-testing.svc.id.goog/ns/default/sa/bob")
155-
.build()) : null,
132+
.build()) : Arrays.asList(),
156133
Arrays.asList("managed-tls"));
157134
}
158135
return buildDownstreamTlsContext(commonTlsContext, /* requireClientCert= */ false);
@@ -199,7 +176,6 @@ public static X509Certificate getCertFromResourceName(String resourceName)
199176
}
200177
}
201178

202-
@SuppressWarnings("deprecation")
203179
private static CommonTlsContext buildCommonTlsContextForCertProviderInstance(
204180
String certInstanceName,
205181
String certName,
@@ -210,10 +186,10 @@ private static CommonTlsContext buildCommonTlsContextForCertProviderInstance(
210186
CommonTlsContext.Builder builder = CommonTlsContext.newBuilder();
211187
if (certInstanceName != null) {
212188
builder =
213-
builder.setTlsCertificateCertificateProviderInstance(
214-
CommonTlsContext.CertificateProviderInstance.newBuilder()
215-
.setInstanceName(certInstanceName)
216-
.setCertificateName(certName));
189+
builder.setTlsCertificateProviderInstance(
190+
CertificateProviderPluginInstance.newBuilder()
191+
.setInstanceName(certInstanceName)
192+
.setCertificateName(certName));
217193
}
218194
builder =
219195
addCertificateValidationContext(
@@ -248,35 +224,28 @@ private static CommonTlsContext buildNewCommonTlsContextForCertProviderInstance(
248224
return builder.build();
249225
}
250226

251-
@SuppressWarnings("deprecation")
252227
private static CommonTlsContext.Builder addCertificateValidationContext(
253228
CommonTlsContext.Builder builder,
254229
String rootInstanceName,
255230
String rootCertName,
256231
CertificateValidationContext staticCertValidationContext) {
257-
CertificateProviderInstance providerInstance = null;
258-
if (rootInstanceName != null) {
259-
providerInstance = CertificateProviderInstance.newBuilder()
260-
.setInstanceName(rootInstanceName)
261-
.setCertificateName(rootCertName)
262-
.build();
263-
}
264-
if (providerInstance != null) {
265-
builder = builder.setValidationContextCertificateProviderInstance(providerInstance);
232+
if (staticCertValidationContext == null && rootInstanceName == null) {
233+
return builder;
266234
}
267-
CombinedCertificateValidationContext.Builder combined =
268-
CombinedCertificateValidationContext.newBuilder();
269-
if (providerInstance != null) {
270-
combined = combined.setValidationContextCertificateProviderInstance(providerInstance);
271-
}
272-
if (staticCertValidationContext != null) {
273-
combined = combined.setDefaultValidationContext(staticCertValidationContext);
235+
CertificateValidationContext.Builder contextBuilder;
236+
if (staticCertValidationContext == null) {
237+
contextBuilder = CertificateValidationContext.newBuilder();
238+
} else {
239+
contextBuilder = staticCertValidationContext.toBuilder();
274240
}
275-
if (combined.hasValidationContextCertificateProviderInstance()
276-
|| combined.hasDefaultValidationContext()) {
277-
builder = builder.setCombinedValidationContext(combined.build());
241+
if (rootInstanceName != null) {
242+
contextBuilder.setCaCertificateProviderInstance(CertificateProviderPluginInstance.newBuilder()
243+
.setInstanceName(rootInstanceName)
244+
.setCertificateName(rootCertName));
245+
builder.setValidationContext(contextBuilder.build());
278246
}
279-
return builder;
247+
return builder.setCombinedValidationContext(CombinedCertificateValidationContext.newBuilder()
248+
.setDefaultValidationContext(contextBuilder));
280249
}
281250

282251
private static CommonTlsContext.Builder addNewCertificateValidationContext(

‎xds/src/test/java/io/grpc/xds/internal/security/certprovider/CertificateProviderStoreTest.java‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,6 @@ public void notifyCertUpdatesNotSupported_expectExceptionOnSecondCall() {
123123
}
124124

125125
@Test
126-
@SuppressWarnings("deprecation")
127126
public void onePluginSameConfig_sameInstance() {
128127
registerPlugin("plugin1");
129128
CertificateProvider.Watcher mockWatcher1 = mock(CertificateProvider.Watcher.class);
@@ -167,7 +166,6 @@ public void onePluginSameConfig_sameInstance() {
167166
}
168167

169168
@Test
170-
@SuppressWarnings("deprecation")
171169
public void onePluginSameConfig_secondWatcherAfterFirstNotify() {
172170
registerPlugin("plugin1");
173171
CertificateProvider.Watcher mockWatcher1 = mock(CertificateProvider.Watcher.class);
@@ -275,7 +273,6 @@ public void twoPlugins_differentInstance() {
275273
mockWatcher1, handle1, certProviderProvider1, mockWatcher2, handle2, certProviderProvider2);
276274
}
277275

278-
@SuppressWarnings("deprecation")
279276
private static void checkDifferentInstances(
280277
CertificateProvider.Watcher mockWatcher1,
281278
CertificateProviderStore.Handle handle1,

0 commit comments

Comments
 (0)