Skip to content

Commit 005557f

Browse files
authored
xds: Reject duplicate endpoint addresses in EDS (#13127)
Fixes #13119. Reject duplicate endpoint socket addresses while parsing EDS resources. Previously, these resources were ACKed and could trigger a panic in outlier detection. Track addresses across the original locality entries, including entries that may later be overwritten in the locality-keyed output map. Reject duplicates via ResourceInvalidException so the existing NACK path retains the last accepted resource and version. Validation includes additional addresses when dual-stack is enabled. The same IP with different ports remains valid, and zero-weight localities remain ignored. Regression tests cover duplicates within and across localities and priorities, overwritten locality entries, equivalent IPv6 addresses, dual-stack addresses, cold start, repeated NACKs, and recovery. ### Testing - Original parser: 14 of 18 new parameterized regression cases fail. - Patched parser: all 272 cases in GrpcXdsClientImplV3Test pass using a standalone JUnit harness with grpc-java 1.85.0 dependencies. - Checkstyle 10.26.1 passes for both modified files. ### Gradle build success ``` [16:17:13] /Users/valter/projects/grpc-java-pr git:(fix-13119-duplicate-eds-endpoints) > ./gradlew -PskipCodegen=true -PskipAndroid=true \ :grpc-xds:test --tests io.grpc.xds.GrpcXdsClientImplV3Test \ :grpc-xds:checkstyleMain :grpc-xds:checkstyleTest Welcome to Gradle 8.14.5! Here are the highlights of this release: - Java 24 support - GraalVM Native Image toolchain selection - Enhancements to test reporting - Build Authoring improvements For more details see https://docs.gradle.org/8.14.5/release-notes.html Starting a Gradle Daemon (subsequent builds will be faster) *** Skipping the build of codegen and compilation of proto files because skipCodegen=true * Skipping the build of Android projects because skipAndroid=true > Task :grpc-services:compileJava Note: Some input files use or override a deprecated API. Note: Recompile with -Xlint:deprecation for details. [Incubating] Problems report is available at: file:///Users/valter/projects/grpc-java-pr/build/reports/problems/problems-report.html Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0. You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins. For more on this, please refer to https://docs.gradle.org/8.14.5/userguide/command_line_interface.html#sec:command_line_warnings in the Gradle documentation. BUILD SUCCESSFUL in 3m 9s 83 actionable tasks: 83 executed ```
1 parent 15d64a4 commit 005557f

3 files changed

Lines changed: 227 additions & 1 deletion

File tree

‎xds/src/main/java/io/grpc/xds/XdsEndpointResource.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,7 @@ private static EdsUpdate processClusterLoadAssignment(ClusterLoadAssignment assi
116116
throws ResourceInvalidException {
117117
Map<Integer, Set<Locality>> priorities = new HashMap<>();
118118
Map<Locality, LocalityLbEndpoints> localityLbEndpointsMap = new LinkedHashMap<>();
119+
Set<java.net.SocketAddress> endpointAddresses = new HashSet<>();
119120
List<Endpoints.DropOverload> dropOverloads = new ArrayList<>();
120121
int maxPriority = -1;
121122
for (io.envoyproxy.envoy.config.endpoint.v3.LocalityLbEndpoints localityLbEndpointsProto
@@ -144,6 +145,14 @@ private static EdsUpdate processClusterLoadAssignment(ClusterLoadAssignment assi
144145
throw new ResourceInvalidException("ClusterLoadAssignment has duplicate locality:"
145146
+ locality + " for priority:" + priority);
146147
}
148+
for (Endpoints.LbEndpoint endpoint : localityLbEndpoints.endpoints()) {
149+
for (java.net.SocketAddress address : endpoint.eag().getAddresses()) {
150+
if (!endpointAddresses.add(address)) {
151+
throw new ResourceInvalidException(
152+
"ClusterLoadAssignment has duplicate endpoint address: " + address);
153+
}
154+
}
155+
}
147156
}
148157
if (priorities.size() != maxPriority + 1) {
149158
throw new ResourceInvalidException("ClusterLoadAssignment has sparse priorities");

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -682,11 +682,15 @@ public void edsUpdatePriorityName_mergeTwoPriorities() {
682682
}
683683

684684
private LocalityLbEndpoints createEndpoints(Locality locality, int priority) {
685+
// Localities must have distinct endpoint addresses even when their priorities are merged.
686+
String address = ImmutableMap.of(
687+
LOCALITY1, "127.0.0.1", LOCALITY2, "127.0.0.2", LOCALITY3, "127.0.0.3")
688+
.get(locality);
685689
return LocalityLbEndpoints.newBuilder()
686690
.setLoadBalancingWeight(UInt32Value.of(70))
687691
.setLocality(locality)
688692
.setPriority(priority)
689-
.addLbEndpoints(newSocketLbEndpoint("127.0." + priority + ".1", 8080))
693+
.addLbEndpoints(newSocketLbEndpoint(address, 8080))
690694
.build();
691695
}
692696

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

Lines changed: 213 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import static com.google.common.truth.Truth.assertThat;
2020
import static com.google.common.truth.Truth.assertWithMessage;
2121
import static io.grpc.StatusMatcher.statusHasCode;
22+
import static io.grpc.xds.XdsEndpointResource.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS;
2223
import static org.mockito.ArgumentMatchers.any;
2324
import static org.mockito.ArgumentMatchers.argThat;
2425
import static org.mockito.ArgumentMatchers.eq;
@@ -47,6 +48,9 @@
4748
import com.google.protobuf.UInt32Value;
4849
import com.google.protobuf.util.Durations;
4950
import io.envoyproxy.envoy.config.cluster.v3.OutlierDetection;
51+
import io.envoyproxy.envoy.config.core.v3.Address;
52+
import io.envoyproxy.envoy.config.core.v3.SocketAddress;
53+
import io.envoyproxy.envoy.config.endpoint.v3.Endpoint;
5054
import io.envoyproxy.envoy.config.route.v3.FilterConfig;
5155
import io.envoyproxy.envoy.config.route.v3.WeightedCluster;
5256
import io.envoyproxy.envoy.extensions.filters.http.router.v3.Router;
@@ -4068,6 +4072,215 @@ public void edsResourceUpdated() {
40684072
verifySubscribedResourcesMetadataSizes(0, 0, 0, 1);
40694073
}
40704074

4075+
@Test
4076+
public void edsDuplicateEndpointAcrossLocalities_retainsLastGoodResource() {
4077+
DiscoveryRpcCall call = startResourceWatcher(XdsEndpointResource.getInstance(), EDS_RESOURCE,
4078+
edsResourceWatcher);
4079+
call.sendResponse(EDS, testClusterLoadAssignment, VERSION_1, "0000");
4080+
call.verifyRequest(EDS, EDS_RESOURCE, VERSION_1, "0000", NODE);
4081+
verify(edsResourceWatcher).onResourceChanged(edsUpdateCaptor.capture());
4082+
EdsUpdate lastGoodUpdate = edsUpdateCaptor.getValue().getValue();
4083+
4084+
Any invalidAssignment = Any.pack(mf.buildClusterLoadAssignment(EDS_RESOURCE,
4085+
ImmutableList.of(
4086+
mf.buildLocalityLbEndpoints("region1", "zone1", "subzone1",
4087+
mf.buildLbEndpoint("172.44.2.2", 8000, "healthy", 1, "host1"), 1, 0),
4088+
mf.buildLocalityLbEndpoints("region2", "zone2", "subzone2",
4089+
mf.buildLbEndpoint("172.44.2.2", 8000, "unhealthy", 2, "host2"), 1, 0)),
4090+
ImmutableList.of()));
4091+
List<String> errors = ImmutableList.of("EDS response ClusterLoadAssignment '" + EDS_RESOURCE
4092+
+ "' validation error: ClusterLoadAssignment has duplicate endpoint address: "
4093+
+ "/172.44.2.2:8000");
4094+
call.sendResponse(EDS, invalidAssignment, VERSION_2, "0001");
4095+
call.verifyRequestNack(EDS, EDS_RESOURCE, VERSION_1, "0001", NODE, errors);
4096+
verifyResourceMetadataNacked(EDS, EDS_RESOURCE, testClusterLoadAssignment, VERSION_1,
4097+
TIME_INCREMENT, VERSION_2, TIME_INCREMENT * 2, errors, true);
4098+
ArgumentCaptor<Status> statusCaptor = ArgumentCaptor.forClass(Status.class);
4099+
verify(edsResourceWatcher).onAmbientError(statusCaptor.capture());
4100+
assertThat(statusCaptor.getValue().getCode()).isEqualTo(Code.UNAVAILABLE);
4101+
assertThat(statusCaptor.getValue().getDescription()).contains(errors.get(0));
4102+
verifyNoMoreInteractions(edsResourceWatcher);
4103+
4104+
// A new watcher still receives the last accepted resource.
4105+
@SuppressWarnings("unchecked")
4106+
ResourceWatcher<EdsUpdate> watcher = mock(ResourceWatcher.class);
4107+
xdsClient.watchXdsResource(XdsEndpointResource.getInstance(), EDS_RESOURCE, watcher);
4108+
verify(watcher).onResourceChanged(StatusOr.fromValue(lastGoodUpdate));
4109+
verify(watcher).onAmbientError(statusCaptor.getValue());
4110+
4111+
call.sendResponse(EDS, invalidAssignment, VERSION_3, "0002");
4112+
call.verifyRequestNack(EDS, EDS_RESOURCE, VERSION_1, "0002", NODE, errors);
4113+
verifyResourceMetadataNacked(EDS, EDS_RESOURCE, testClusterLoadAssignment, VERSION_1,
4114+
TIME_INCREMENT, VERSION_3, TIME_INCREMENT * 3, errors, true);
4115+
verify(edsResourceWatcher, times(2)).onAmbientError(any());
4116+
verify(watcher, times(2)).onAmbientError(any());
4117+
verifyNoMoreInteractions(edsResourceWatcher, watcher);
4118+
4119+
Any validAssignment = Any.pack(mf.buildClusterLoadAssignment(EDS_RESOURCE,
4120+
ImmutableList.of(mf.buildLocalityLbEndpoints("region2", "zone2", "subzone2",
4121+
mf.buildLbEndpoint("172.44.2.2", 8001, "healthy", 1, "host2"), 1, 0)),
4122+
ImmutableList.of()));
4123+
call.sendResponse(EDS, validAssignment, "45", "0003");
4124+
call.verifyRequest(EDS, EDS_RESOURCE, "45", "0003", NODE);
4125+
verifyResourceMetadataAcked(EDS, EDS_RESOURCE, validAssignment, "45", TIME_INCREMENT * 4);
4126+
verify(edsResourceWatcher, times(2)).onResourceChanged(edsUpdateCaptor.capture());
4127+
assertThat(edsUpdateCaptor.getValue().getValue()).isNotEqualTo(lastGoodUpdate);
4128+
verify(watcher).onResourceChanged(edsUpdateCaptor.getValue());
4129+
}
4130+
4131+
@Test
4132+
public void edsDuplicateEndpointOnColdStart() {
4133+
DiscoveryRpcCall call = startResourceWatcher(XdsEndpointResource.getInstance(), EDS_RESOURCE,
4134+
edsResourceWatcher);
4135+
Any invalidAssignment = Any.pack(mf.buildClusterLoadAssignment(EDS_RESOURCE,
4136+
ImmutableList.of(
4137+
mf.buildLocalityLbEndpoints("region1", "zone1", "subzone1",
4138+
mf.buildLbEndpoint("172.44.2.2", 8000, "healthy", 1, "host1"), 1, 0),
4139+
mf.buildLocalityLbEndpoints("region2", "zone2", "subzone2",
4140+
mf.buildLbEndpoint("172.44.2.2", 8000, "healthy", 1, "host2"), 1, 0)),
4141+
ImmutableList.of()));
4142+
call.sendResponse(EDS, invalidAssignment, VERSION_1, "0000");
4143+
call.verifyRequestNack(EDS, EDS_RESOURCE, "", "0000", NODE,
4144+
ImmutableList.of("EDS response ClusterLoadAssignment '" + EDS_RESOURCE
4145+
+ "' validation error: ClusterLoadAssignment has duplicate endpoint address:"));
4146+
verify(edsResourceWatcher).onResourceChanged(edsUpdateCaptor.capture());
4147+
assertThat(edsUpdateCaptor.getValue().hasValue()).isFalse();
4148+
4149+
call.sendResponse(EDS, testClusterLoadAssignment, VERSION_2, "0001");
4150+
call.verifyRequest(EDS, EDS_RESOURCE, VERSION_2, "0001", NODE);
4151+
verify(edsResourceWatcher, times(2)).onResourceChanged(edsUpdateCaptor.capture());
4152+
assertThat(edsUpdateCaptor.getValue().hasValue()).isTrue();
4153+
validateGoldenClusterLoadAssignment(edsUpdateCaptor.getValue().getValue());
4154+
}
4155+
4156+
@Test
4157+
public void edsDuplicateEndpointWithinLocality() {
4158+
verifyEdsDuplicateEndpoint(ImmutableList.of(
4159+
mf.buildLocalityLbEndpoints("region1", "zone1", "subzone1",
4160+
ImmutableList.of(
4161+
mf.buildLbEndpoint("172.44.2.2", 8000, "healthy", 1, "host1"),
4162+
mf.buildLbEndpoint("172.44.2.2", 8000, "healthy", 1, "host2")), 1, 0)));
4163+
}
4164+
4165+
@Test
4166+
public void edsDuplicateEndpointAcrossPriorities() {
4167+
verifyEdsDuplicateEndpoint(ImmutableList.of(
4168+
mf.buildLocalityLbEndpoints("region1", "zone1", "subzone1",
4169+
mf.buildLbEndpoint("172.44.2.2", 8000, "healthy", 1, "host1"), 1, 0),
4170+
mf.buildLocalityLbEndpoints("region2", "zone2", "subzone2",
4171+
mf.buildLbEndpoint("172.44.2.2", 8000, "healthy", 1, "host2"), 1, 1)));
4172+
}
4173+
4174+
@Test
4175+
public void edsDuplicateEndpointInOverwrittenLocality() {
4176+
// The second entry overwrites the first in the locality-keyed output map.
4177+
verifyEdsDuplicateEndpoint(ImmutableList.of(
4178+
mf.buildLocalityLbEndpoints("region1", "zone1", "subzone1",
4179+
mf.buildLbEndpoint("172.44.2.2", 8000, "healthy", 1, "host1"), 1, 0),
4180+
mf.buildLocalityLbEndpoints("region1", "zone1", "subzone1",
4181+
mf.buildLbEndpoint("172.44.2.3", 8000, "healthy", 1, "host2"), 1, 1),
4182+
mf.buildLocalityLbEndpoints("region2", "zone2", "subzone2",
4183+
mf.buildLbEndpoint("172.44.2.2", 8000, "healthy", 1, "host3"), 1, 0)));
4184+
}
4185+
4186+
@Test
4187+
public void edsDuplicateEndpointWithEquivalentIpv6Addresses() {
4188+
verifyEdsDuplicateEndpoint(ImmutableList.of(
4189+
mf.buildLocalityLbEndpoints("region1", "zone1", "subzone1",
4190+
mf.buildLbEndpoint("2001:db8::1", 8000, "healthy", 1, "host1"), 1, 0),
4191+
mf.buildLocalityLbEndpoints("region2", "zone2", "subzone2",
4192+
mf.buildLbEndpoint("2001:0db8:0:0:0:0:0:1", 8000, "healthy", 1, "host2"), 1, 0)));
4193+
}
4194+
4195+
@Test
4196+
public void edsDuplicateEndpointInAdditionalAddresses() {
4197+
String originalDualStackProp =
4198+
System.setProperty(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, "false");
4199+
try {
4200+
io.envoyproxy.envoy.config.endpoint.v3.LbEndpoint endpoint =
4201+
(io.envoyproxy.envoy.config.endpoint.v3.LbEndpoint)
4202+
mf.buildLbEndpoint("172.44.2.2", 8000, "healthy", 1, "host1");
4203+
Message dualStackEndpoint = endpoint.toBuilder()
4204+
.setEndpoint(endpoint.getEndpoint().toBuilder()
4205+
.addAdditionalAddresses(Endpoint.AdditionalAddress.newBuilder()
4206+
.setAddress(Address.newBuilder().setSocketAddress(SocketAddress.newBuilder()
4207+
.setAddress("2001:db8::1").setPortValue(8000)))))
4208+
.build();
4209+
Any assignment = Any.pack(mf.buildClusterLoadAssignment(EDS_RESOURCE,
4210+
ImmutableList.of(
4211+
mf.buildLocalityLbEndpoints("region1", "zone1", "subzone1",
4212+
dualStackEndpoint, 1, 0),
4213+
mf.buildLocalityLbEndpoints("region2", "zone2", "subzone2",
4214+
mf.buildLbEndpoint("2001:db8::1", 8000, "healthy", 1, "host2"), 1, 0)),
4215+
ImmutableList.of()));
4216+
DiscoveryRpcCall call = startResourceWatcher(XdsEndpointResource.getInstance(), EDS_RESOURCE,
4217+
edsResourceWatcher);
4218+
call.sendResponse(EDS, assignment, VERSION_1, "0000");
4219+
call.verifyRequest(EDS, EDS_RESOURCE, VERSION_1, "0000", NODE);
4220+
4221+
System.setProperty(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, "true");
4222+
call.sendResponse(EDS, assignment, VERSION_2, "0001");
4223+
List<String> errors = ImmutableList.of("EDS response ClusterLoadAssignment '" + EDS_RESOURCE
4224+
+ "' validation error: ClusterLoadAssignment has duplicate endpoint address:");
4225+
call.verifyRequestNack(EDS, EDS_RESOURCE, VERSION_1, "0001", NODE, errors);
4226+
verifyResourceMetadataNacked(EDS, EDS_RESOURCE, assignment, VERSION_1, TIME_INCREMENT,
4227+
VERSION_2, TIME_INCREMENT * 2, errors, true);
4228+
verify(edsResourceWatcher).onResourceChanged(edsUpdateCaptor.capture());
4229+
assertThat(edsUpdateCaptor.getValue().hasValue()).isTrue();
4230+
} finally {
4231+
if (originalDualStackProp == null) {
4232+
System.clearProperty(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS);
4233+
} else {
4234+
System.setProperty(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, originalDualStackProp);
4235+
}
4236+
}
4237+
}
4238+
4239+
private void verifyEdsDuplicateEndpoint(List<Message> localities) {
4240+
DiscoveryRpcCall call = startResourceWatcher(XdsEndpointResource.getInstance(), EDS_RESOURCE,
4241+
edsResourceWatcher);
4242+
Any assignment = Any.pack(mf.buildClusterLoadAssignment(EDS_RESOURCE, localities,
4243+
ImmutableList.of()));
4244+
call.sendResponse(EDS, assignment, VERSION_1, "0000");
4245+
call.verifyRequestNack(EDS, EDS_RESOURCE, "", "0000", NODE,
4246+
ImmutableList.of("EDS response ClusterLoadAssignment '" + EDS_RESOURCE
4247+
+ "' validation error: ClusterLoadAssignment has duplicate endpoint address:"));
4248+
verify(edsResourceWatcher).onResourceChanged(edsUpdateCaptor.capture());
4249+
assertThat(edsUpdateCaptor.getValue().hasValue()).isFalse();
4250+
}
4251+
4252+
@Test
4253+
public void edsEndpointsWithSameIpAndDifferentPorts() {
4254+
DiscoveryRpcCall call = startResourceWatcher(XdsEndpointResource.getInstance(), EDS_RESOURCE,
4255+
edsResourceWatcher);
4256+
Any assignment = Any.pack(mf.buildClusterLoadAssignment(EDS_RESOURCE,
4257+
ImmutableList.of(
4258+
mf.buildLocalityLbEndpoints("region1", "zone1", "subzone1",
4259+
mf.buildLbEndpoint("172.44.2.2", 8000, "healthy", 1, "host1"), 1, 0),
4260+
mf.buildLocalityLbEndpoints("region2", "zone2", "subzone2",
4261+
mf.buildLbEndpoint("172.44.2.2", 8001, "healthy", 1, "host2"), 1, 0)),
4262+
ImmutableList.of()));
4263+
call.sendResponse(EDS, assignment, VERSION_1, "0000");
4264+
call.verifyRequest(EDS, EDS_RESOURCE, VERSION_1, "0000", NODE);
4265+
verify(edsResourceWatcher).onResourceChanged(edsUpdateCaptor.capture());
4266+
assertThat(edsUpdateCaptor.getValue().getValue().localityLbEndpointsMap).hasSize(2);
4267+
}
4268+
4269+
@Test
4270+
public void edsDuplicateEndpointInZeroWeightLocalityIgnored() {
4271+
DiscoveryRpcCall call = startResourceWatcher(XdsEndpointResource.getInstance(), EDS_RESOURCE,
4272+
edsResourceWatcher);
4273+
Any assignment = Any.pack(mf.buildClusterLoadAssignment(EDS_RESOURCE,
4274+
ImmutableList.of(lbEndpointHealthy,
4275+
mf.buildLocalityLbEndpoints("region2", "zone2", "subzone2",
4276+
mf.buildLbEndpoint("192.168.0.1", 8080, "healthy", 1, "host2"), 0, 0)),
4277+
ImmutableList.of()));
4278+
call.sendResponse(EDS, assignment, VERSION_1, "0000");
4279+
call.verifyRequest(EDS, EDS_RESOURCE, VERSION_1, "0000", NODE);
4280+
verify(edsResourceWatcher).onResourceChanged(edsUpdateCaptor.capture());
4281+
assertThat(edsUpdateCaptor.getValue().getValue().localityLbEndpointsMap).hasSize(1);
4282+
}
4283+
40714284
@Test
40724285
public void edsDuplicateLocalityInTheSamePriority() {
40734286
DiscoveryRpcCall call = startResourceWatcher(XdsEndpointResource.getInstance(), EDS_RESOURCE,

0 commit comments

Comments
 (0)