Skip to content

Commit 1d4d18e

Browse files
AgraVatorAgraVator
committed
fix: suggested changes
1 parent 0648217 commit 1d4d18e

3 files changed

Lines changed: 313 additions & 6 deletions

File tree

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

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,8 @@ private CompositeFilter() {
8383
}
8484

8585
static final class Provider implements Filter.Provider {
86+
private static final ThreadLocal<Integer> recursionDepth = ThreadLocal.withInitial(() -> 0);
87+
8688
@Override
8789
public String[] typeUrls() {
8890
return new String[] {
@@ -117,6 +119,11 @@ public ConfigOrError<CompositeFilterConfig> parseFilterConfig(Message rawProtoMe
117119
return ConfigOrError.fromError("Invalid message type: "
118120
+ rawProtoMessage.getClass().getName());
119121
}
122+
int currentDepth = recursionDepth.get();
123+
if (currentDepth > 8) {
124+
return ConfigOrError.fromError("Maximum recursion depth of 8 exceeded");
125+
}
126+
recursionDepth.set(currentDepth + 1);
120127
try {
121128
Any any = (Any) rawProtoMessage;
122129
if (any.is(ExtensionWithMatcher.class)) {
@@ -127,6 +134,8 @@ public ConfigOrError<CompositeFilterConfig> parseFilterConfig(Message rawProtoMe
127134
}
128135
} catch (InvalidProtocolBufferException e) {
129136
return ConfigOrError.fromError("Invalid proto: " + e);
137+
} finally {
138+
recursionDepth.set(currentDepth);
130139
}
131140
return ConfigOrError.fromError("Unsupported message type in parseFilterConfig");
132141
}
@@ -141,6 +150,11 @@ public ConfigOrError<CompositeFilterConfig> parseFilterConfigOverride(Message ra
141150
return ConfigOrError.fromError("Invalid message type: "
142151
+ rawProtoMessage.getClass().getName());
143152
}
153+
int currentDepth = recursionDepth.get();
154+
if (currentDepth > 8) {
155+
return ConfigOrError.fromError("Maximum recursion depth of 8 exceeded");
156+
}
157+
recursionDepth.set(currentDepth + 1);
144158
try {
145159
Any any = (Any) rawProtoMessage;
146160
if (any.is(ExtensionWithMatcherPerRoute.class)) {
@@ -149,6 +163,8 @@ public ConfigOrError<CompositeFilterConfig> parseFilterConfigOverride(Message ra
149163
}
150164
} catch (InvalidProtocolBufferException e) {
151165
return ConfigOrError.fromError("Invalid proto: " + e);
166+
} finally {
167+
recursionDepth.set(currentDepth);
152168
}
153169
return ConfigOrError.fromError("Unsupported message type in "
154170
+ "parseFilterConfigOverride");
@@ -524,10 +540,7 @@ private static class CompositeClientCall<ReqT, RespT> extends io.grpc.ClientCall
524540

525541
@Override
526542
public void start(Listener<RespT> responseListener, Metadata headers) {
527-
if (started) {
528-
delegate.start(responseListener, headers);
529-
return;
530-
}
543+
Preconditions.checkState(!started, "Already started");
531544
started = true;
532545

533546
UnifiedMatcher.MatchingData data = new MatchingDataImpl(headers, callOptions);

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,7 @@ static synchronized FilterRegistry getDefaultRegistry() {
3838
new FaultFilter.Provider(),
3939
new RouterFilter.Provider(),
4040
new RbacFilter.Provider(),
41-
new GcpAuthenticationFilter.Provider(),
42-
new CompositeFilter.Provider());
41+
new GcpAuthenticationFilter.Provider());
4342
}
4443
return instance;
4544
}

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

Lines changed: 295 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,13 +31,17 @@
3131
import io.envoyproxy.envoy.extensions.common.matching.v3.ExtensionWithMatcherPerRoute;
3232
import io.envoyproxy.envoy.extensions.filters.http.composite.v3.Composite;
3333
import io.envoyproxy.envoy.extensions.filters.http.composite.v3.ExecuteFilterAction;
34+
import io.envoyproxy.envoy.type.matcher.v3.HttpRequestHeaderMatchInput;
3435
import io.grpc.CallOptions;
3536
import io.grpc.Channel;
3637
import io.grpc.ClientCall;
3738
import io.grpc.ClientInterceptor;
3839
import io.grpc.Metadata;
3940
import io.grpc.MethodDescriptor;
41+
import io.grpc.ServerCall;
42+
import io.grpc.ServerCallHandler;
4043
import io.grpc.ServerInterceptor;
44+
import io.grpc.Status;
4145
import io.grpc.xds.Filter.FilterConfig;
4246
import io.grpc.xds.internal.UnifiedMatcher;
4347
import java.util.concurrent.ScheduledExecutorService;
@@ -46,6 +50,7 @@
4650
import org.junit.Test;
4751
import org.junit.runner.RunWith;
4852
import org.junit.runners.JUnit4;
53+
import org.mockito.ArgumentCaptor;
4954
import org.mockito.Mock;
5055
import org.mockito.MockitoAnnotations;
5156

@@ -734,4 +739,294 @@ public void parseFilterConfigWithEmptyConfig() {
734739
assertThat(result.config.matcher).isNull();
735740
}
736741

742+
@Test
743+
public void clientInterceptorUsesOverrideMatcher() {
744+
// Base matcher that matches "foo=bar"
745+
Matcher.OnMatch matchAction = Matcher.OnMatch.newBuilder()
746+
.setAction(com.github.xds.core.v3.TypedExtensionConfig.newBuilder()
747+
.setName("action")
748+
.setTypedConfig(Any.newBuilder()
749+
.setTypeUrl("type.googleapis.com/envoy.extensions.filters.http.composite"
750+
+ ".v3.ExecuteFilterAction")
751+
.setValue(ExecuteFilterAction.newBuilder()
752+
.setTypedConfig(TypedExtensionConfig.newBuilder()
753+
.setName("child")
754+
.setTypedConfig(Any.newBuilder().setTypeUrl(FAKE_TYPE_URL).build())
755+
.build())
756+
.build().toByteString())
757+
.build())
758+
.build())
759+
.build();
760+
761+
Matcher baseMatcherProto = Matcher.newBuilder()
762+
.setMatcherList(Matcher.MatcherList.newBuilder()
763+
.addMatchers(Matcher.MatcherList.FieldMatcher.newBuilder()
764+
.setPredicate(Matcher.MatcherList.Predicate.newBuilder()
765+
.setSinglePredicate(Matcher.MatcherList.Predicate.SinglePredicate.newBuilder()
766+
.setInput(com.github.xds.core.v3.TypedExtensionConfig.newBuilder()
767+
.setName("request_headers")
768+
.setTypedConfig(Any.pack(
769+
HttpRequestHeaderMatchInput.newBuilder()
770+
.setHeaderName("foo")
771+
.build()))
772+
.build())
773+
.setValueMatch(StringMatcher.newBuilder().setExact("bar").build())
774+
.build())
775+
.build())
776+
.setOnMatch(matchAction)
777+
.build())
778+
.build())
779+
.build();
780+
781+
ExtensionWithMatcher baseProto = ExtensionWithMatcher.newBuilder()
782+
.setExtensionConfig(TypedExtensionConfig.newBuilder().setName("composite").build())
783+
.setXdsMatcher(baseMatcherProto)
784+
.build();
785+
786+
ConfigOrError<CompositeFilter.CompositeFilterConfig> baseResult = provider
787+
.parseFilterConfig(Any.pack(baseProto));
788+
789+
// Override matcher that is empty (skips everything)
790+
Matcher overrideMatcherProto = Matcher.newBuilder().build();
791+
ExtensionWithMatcherPerRoute overrideProto = ExtensionWithMatcherPerRoute.newBuilder()
792+
.setXdsMatcher(overrideMatcherProto)
793+
.build();
794+
795+
ConfigOrError<CompositeFilter.CompositeFilterConfig> overrideResult = provider
796+
.parseFilterConfigOverride(Any.pack(overrideProto));
797+
798+
CompositeFilter filter = (CompositeFilter) provider.newInstance("composite");
799+
ClientInterceptor interceptor = filter.buildClientInterceptor(
800+
baseResult.config, overrideResult.config, mock(ScheduledExecutorService.class));
801+
802+
Channel next = mock(Channel.class);
803+
ClientCall nextCall = mock(ClientCall.class);
804+
when(next.newCall(any(), any())).thenReturn(nextCall);
805+
806+
MethodDescriptor.Marshaller<Void> marshaller = mock(MethodDescriptor.Marshaller.class);
807+
MethodDescriptor<Void, Void> method = MethodDescriptor.<Void, Void>newBuilder()
808+
.setType(MethodDescriptor.MethodType.UNARY)
809+
.setFullMethodName("service/method")
810+
.setRequestMarshaller(marshaller)
811+
.setResponseMarshaller(marshaller)
812+
.build();
813+
814+
ClientCall<Void, Void> call = interceptor.interceptCall(method, CallOptions.DEFAULT, next);
815+
816+
Metadata headers = new Metadata();
817+
headers.put(Metadata.Key.of("foo", Metadata.ASCII_STRING_MARSHALLER), "bar");
818+
// This would match base, but should be overridden
819+
820+
call.start(mock(ClientCall.Listener.class), headers);
821+
822+
// Verify it SKIPS (calls next directly, never calls fakeClientInterceptor)
823+
verify(fakeClientInterceptor, org.mockito.Mockito.never()).interceptCall(any(), any(), any());
824+
verify(next).newCall(any(), any());
825+
verify(nextCall).start(any(), eq(headers));
826+
}
827+
828+
@Test
829+
public void serverInterceptorDelegates() {
830+
// Setup Config with simple matcher equivalent logic
831+
Matcher.OnMatch matchAction = Matcher.OnMatch.newBuilder()
832+
.setAction(com.github.xds.core.v3.TypedExtensionConfig.newBuilder()
833+
.setName("action")
834+
.setTypedConfig(Any.newBuilder()
835+
.setTypeUrl("type.googleapis.com/envoy.extensions.filters.http.composite"
836+
+ ".v3.ExecuteFilterAction")
837+
.setValue(ExecuteFilterAction.newBuilder()
838+
.setTypedConfig(TypedExtensionConfig.newBuilder()
839+
.setName("child")
840+
.setTypedConfig(Any.newBuilder().setTypeUrl(FAKE_TYPE_URL).build())
841+
.build())
842+
.build().toByteString())
843+
.build())
844+
.build())
845+
.build();
846+
847+
Matcher matcher = Matcher.newBuilder()
848+
.setMatcherList(Matcher.MatcherList.newBuilder()
849+
.addMatchers(Matcher.MatcherList.FieldMatcher.newBuilder()
850+
.setPredicate(Matcher.MatcherList.Predicate.newBuilder()
851+
.setSinglePredicate(Matcher.MatcherList.Predicate.SinglePredicate.newBuilder()
852+
.setInput(com.github.xds.core.v3.TypedExtensionConfig.newBuilder()
853+
.setName("request_headers")
854+
.setTypedConfig(Any.pack(
855+
HttpRequestHeaderMatchInput.newBuilder()
856+
.setHeaderName("foo")
857+
.build()))
858+
.build())
859+
.setValueMatch(StringMatcher.newBuilder().setExact("bar").build())
860+
.build())
861+
.build())
862+
.setOnMatch(matchAction)
863+
.build())
864+
.build())
865+
.build();
866+
867+
ExtensionWithMatcher proto = ExtensionWithMatcher.newBuilder()
868+
.setExtensionConfig(TypedExtensionConfig.newBuilder().setName("composite").build())
869+
.setXdsMatcher(matcher)
870+
.build();
871+
872+
ConfigOrError<CompositeFilter.CompositeFilterConfig> result = provider
873+
.parseFilterConfig(Any.pack(proto));
874+
875+
CompositeFilter filter = (CompositeFilter) provider.newInstance("composite");
876+
ServerInterceptor interceptor = filter.buildServerInterceptor(result.config, null);
877+
878+
ServerCall call = mock(ServerCall.class);
879+
when(call.getAttributes()).thenReturn(io.grpc.Attributes.EMPTY);
880+
881+
Metadata headers = new Metadata();
882+
headers.put(Metadata.Key.of("foo", Metadata.ASCII_STRING_MARSHALLER), "bar");
883+
884+
ServerCallHandler next = mock(ServerCallHandler.class);
885+
ServerCall.Listener listener = mock(ServerCall.Listener.class);
886+
when(next.startCall(any(), any())).thenReturn(listener);
887+
888+
interceptor.interceptCall(call, headers, next);
889+
890+
verify(fakeServerInterceptor).interceptCall(eq(call), eq(headers), any());
891+
}
892+
893+
@Test
894+
public void clientInterceptorClosesFiltersOnClose() {
895+
// Setup Config with simple matcher equivalent logic
896+
Matcher.OnMatch matchAction = Matcher.OnMatch.newBuilder()
897+
.setAction(com.github.xds.core.v3.TypedExtensionConfig.newBuilder()
898+
.setName("action")
899+
.setTypedConfig(Any.newBuilder()
900+
.setTypeUrl("type.googleapis.com/envoy.extensions.filters.http.composite"
901+
+ ".v3.ExecuteFilterAction")
902+
.setValue(ExecuteFilterAction.newBuilder()
903+
.setTypedConfig(TypedExtensionConfig.newBuilder()
904+
.setName("child")
905+
.setTypedConfig(Any.newBuilder().setTypeUrl(FAKE_TYPE_URL).build())
906+
.build())
907+
.build().toByteString())
908+
.build())
909+
.build())
910+
.build();
911+
912+
Matcher matcher = Matcher.newBuilder()
913+
.setMatcherList(Matcher.MatcherList.newBuilder()
914+
.addMatchers(Matcher.MatcherList.FieldMatcher.newBuilder()
915+
.setPredicate(Matcher.MatcherList.Predicate.newBuilder()
916+
.setSinglePredicate(Matcher.MatcherList.Predicate.SinglePredicate.newBuilder()
917+
.setInput(com.github.xds.core.v3.TypedExtensionConfig.newBuilder()
918+
.setName("request_headers")
919+
.setTypedConfig(Any.pack(
920+
HttpRequestHeaderMatchInput.newBuilder()
921+
.setHeaderName("foo")
922+
.build()))
923+
.build())
924+
.setValueMatch(StringMatcher.newBuilder().setExact("bar").build())
925+
.build())
926+
.build())
927+
.setOnMatch(matchAction)
928+
.build())
929+
.build())
930+
.build();
931+
932+
ExtensionWithMatcher proto = ExtensionWithMatcher.newBuilder()
933+
.setExtensionConfig(TypedExtensionConfig.newBuilder().setName("composite").build())
934+
.setXdsMatcher(matcher)
935+
.build();
936+
937+
ConfigOrError<CompositeFilter.CompositeFilterConfig> result = provider
938+
.parseFilterConfig(Any.pack(proto));
939+
940+
CompositeFilter filter = (CompositeFilter) provider.newInstance("composite");
941+
ClientInterceptor interceptor = filter.buildClientInterceptor(result.config, null,
942+
mock(ScheduledExecutorService.class));
943+
944+
Channel next = mock(Channel.class);
945+
ClientCall childCall = mock(ClientCall.class);
946+
when(fakeClientInterceptor.interceptCall(any(), any(), any())).thenReturn(childCall);
947+
948+
MethodDescriptor.Marshaller<Void> marshaller = mock(MethodDescriptor.Marshaller.class);
949+
MethodDescriptor<Void, Void> method = MethodDescriptor.<Void, Void>newBuilder()
950+
.setType(MethodDescriptor.MethodType.UNARY)
951+
.setFullMethodName("service/method")
952+
.setRequestMarshaller(marshaller)
953+
.setResponseMarshaller(marshaller)
954+
.build();
955+
956+
ClientCall<Void, Void> call = interceptor.interceptCall(method, CallOptions.DEFAULT, next);
957+
958+
Metadata headers = new Metadata();
959+
headers.put(Metadata.Key.of("foo", Metadata.ASCII_STRING_MARSHALLER), "bar");
960+
961+
ClientCall.Listener responseListener = mock(ClientCall.Listener.class);
962+
call.start(responseListener, headers);
963+
964+
// Capture the listener passed to childCall
965+
ArgumentCaptor<ClientCall.Listener> listenerCaptor =
966+
ArgumentCaptor.forClass(ClientCall.Listener.class);
967+
verify(childCall).start(listenerCaptor.capture(), eq(headers));
968+
969+
ClientCall.Listener capturedListener = listenerCaptor.getValue();
970+
971+
// Trigger onClose
972+
capturedListener.onClose(Status.OK, new Metadata());
973+
974+
// Verify filter.close() was called
975+
verify(fakeFilter).close();
976+
}
977+
978+
@Test
979+
public void parseFilterConfigExceedsRecursionLimit() {
980+
// Setup matcher that resolves to fakeProvider
981+
Matcher.OnMatch matchAction = Matcher.OnMatch.newBuilder()
982+
.setAction(com.github.xds.core.v3.TypedExtensionConfig.newBuilder()
983+
.setName("action")
984+
.setTypedConfig(Any.newBuilder()
985+
.setTypeUrl("type.googleapis.com/envoy.extensions.filters.http.composite"
986+
+ ".v3.ExecuteFilterAction")
987+
.setValue(ExecuteFilterAction.newBuilder()
988+
.setTypedConfig(TypedExtensionConfig.newBuilder()
989+
.setName("child")
990+
.setTypedConfig(Any.newBuilder().setTypeUrl(FAKE_TYPE_URL).build())
991+
.build())
992+
.build().toByteString())
993+
.build())
994+
.build())
995+
.build();
996+
997+
Matcher matcherProto = Matcher.newBuilder()
998+
.setMatcherList(Matcher.MatcherList.newBuilder()
999+
.addMatchers(Matcher.MatcherList.FieldMatcher.newBuilder()
1000+
.setOnMatch(matchAction)
1001+
.build())
1002+
.build())
1003+
.build();
1004+
1005+
ExtensionWithMatcher configProto = ExtensionWithMatcher.newBuilder()
1006+
.setExtensionConfig(TypedExtensionConfig.newBuilder().setName("composite").build())
1007+
.setXdsMatcher(matcherProto)
1008+
.build();
1009+
1010+
final Any configAny = Any.pack(configProto);
1011+
1012+
// Mock fakeProvider to call provider.parseFilterConfig recursively
1013+
when(fakeProvider.parseFilterConfig(any()))
1014+
.thenAnswer(new org.mockito.stubbing.Answer<ConfigOrError>() {
1015+
private int depth = 0;
1016+
@Override
1017+
public ConfigOrError answer(
1018+
org.mockito.invocation.InvocationOnMock invocation) throws Throwable {
1019+
depth++;
1020+
if (depth > 15) { // Safety break
1021+
return ConfigOrError.fromError("Infinite recursion safety break");
1022+
}
1023+
return provider.parseFilterConfig(configAny);
1024+
}
1025+
});
1026+
1027+
ConfigOrError<CompositeFilter.CompositeFilterConfig> result = provider
1028+
.parseFilterConfig(configAny);
1029+
1030+
assertThat(result.errorDetail).contains("Maximum recursion depth of 8 exceeded");
1031+
}
7371032
}

0 commit comments

Comments
 (0)