Skip to content

Commit dc297fe

Browse files
AgraVatorAgraVator
committed
fix: suggested changes
1 parent be05900 commit dc297fe

17 files changed

Lines changed: 319 additions & 93 deletions

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

Lines changed: 34 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@
4848
import java.util.Collections;
4949
import java.util.List;
5050
import java.util.concurrent.ScheduledExecutorService;
51-
import java.util.concurrent.ThreadLocalRandom;
51+
import java.util.function.Function;
5252
import java.util.logging.Level;
5353
import java.util.logging.Logger;
5454
import javax.annotation.Nullable;
@@ -57,7 +57,7 @@
5757
import javax.net.ssl.SSLSession;
5858
import javax.net.ssl.StandardConstants;
5959

60-
public final class CompositeFilter implements Filter {
60+
final class CompositeFilter implements Filter {
6161

6262
static final String TYPE_URL_EXTENSION_WITH_MATCHER =
6363
"type.googleapis.com/envoy.extensions.common.matching.v3.ExtensionWithMatcher";
@@ -83,7 +83,9 @@ private CompositeFilter() {
8383
}
8484

8585
static final class Provider implements Filter.Provider {
86-
private static final ThreadLocal<Integer> recursionDepth = ThreadLocal.withInitial(() -> 0);
86+
87+
static Function<String, Filter.Provider> registryLookup =
88+
typeUrl -> FilterRegistry.getDefaultRegistry().get(typeUrl);
8789

8890
@Override
8991
public String[] typeUrls() {
@@ -110,7 +112,8 @@ public Filter newInstance(String name) {
110112
}
111113

112114
@Override
113-
public ConfigOrError<CompositeFilterConfig> parseFilterConfig(Message rawProtoMessage) {
115+
public ConfigOrError<CompositeFilterConfig> parseFilterConfig(
116+
Message rawProtoMessage, int depth) {
114117
if (!isSupported()) {
115118
return ConfigOrError.fromError("Composite Filter is experimental "
116119
+ "and disabled by default.");
@@ -119,29 +122,23 @@ public ConfigOrError<CompositeFilterConfig> parseFilterConfig(Message rawProtoMe
119122
return ConfigOrError.fromError("Invalid message type: "
120123
+ rawProtoMessage.getClass().getName());
121124
}
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);
127125
try {
128126
Any any = (Any) rawProtoMessage;
129127
if (any.is(ExtensionWithMatcher.class)) {
130128
ExtensionWithMatcher proto = any.unpack(ExtensionWithMatcher.class);
131-
return parseMatcherConfig(proto.getXdsMatcher());
129+
return parseMatcherConfig(proto.getXdsMatcher(), depth);
132130
} else if (any.is(Composite.class)) {
133131
return ConfigOrError.fromConfig(new CompositeFilterConfig(null));
134132
}
135133
} catch (InvalidProtocolBufferException e) {
136134
return ConfigOrError.fromError("Invalid proto: " + e);
137-
} finally {
138-
recursionDepth.set(currentDepth);
139135
}
140136
return ConfigOrError.fromError("Unsupported message type in parseFilterConfig");
141137
}
142138

143139
@Override
144-
public ConfigOrError<CompositeFilterConfig> parseFilterConfigOverride(Message rawProtoMessage) {
140+
public ConfigOrError<CompositeFilterConfig> parseFilterConfigOverride(
141+
Message rawProtoMessage, int depth) {
145142
if (!isSupported()) {
146143
return ConfigOrError.fromError("Composite Filter is experimental and disabled"
147144
+ " by default.");
@@ -150,35 +147,28 @@ public ConfigOrError<CompositeFilterConfig> parseFilterConfigOverride(Message ra
150147
return ConfigOrError.fromError("Invalid message type: "
151148
+ rawProtoMessage.getClass().getName());
152149
}
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);
158150
try {
159151
Any any = (Any) rawProtoMessage;
160152
if (any.is(ExtensionWithMatcherPerRoute.class)) {
161153
ExtensionWithMatcherPerRoute proto = any.unpack(ExtensionWithMatcherPerRoute.class);
162-
return parseMatcherConfig(proto.getXdsMatcher());
154+
return parseMatcherConfig(proto.getXdsMatcher(), depth);
163155
}
164156
} catch (InvalidProtocolBufferException e) {
165157
return ConfigOrError.fromError("Invalid proto: " + e);
166-
} finally {
167-
recursionDepth.set(currentDepth);
168158
}
169159
return ConfigOrError.fromError("Unsupported message type in "
170160
+ "parseFilterConfigOverride");
171161
}
172162

173163
private ConfigOrError<CompositeFilterConfig> parseMatcherConfig(
174-
@Nullable Matcher matcherProto) {
164+
@Nullable Matcher matcherProto, int depth) {
175165
if (matcherProto == null) {
176166
return ConfigOrError.fromConfig(new CompositeFilterConfig(null));
177167
}
178168

179169
try {
180170
UnifiedMatcher<FilterDelegate> matcher = UnifiedMatcher.create(matcherProto,
181-
Provider::createFilterDelegate);
171+
config -> createFilterDelegate(config, depth));
182172
return ConfigOrError.fromConfig(new CompositeFilterConfig(matcher));
183173
} catch (Exception e) {
184174
return ConfigOrError.fromError("Failed to create matcher: " + e.getMessage());
@@ -190,7 +180,7 @@ private boolean isSupported() {
190180
}
191181

192182
private static FilterDelegate createFilterDelegate(
193-
com.github.xds.core.v3.TypedExtensionConfig config) {
183+
com.github.xds.core.v3.TypedExtensionConfig config, int depth) {
194184
try {
195185
Any actionAny = config.getTypedConfig();
196186
if (actionAny.is(ExecuteFilterAction.class)) {
@@ -233,12 +223,12 @@ private static FilterDelegate createFilterDelegate(
233223
throw new IllegalArgumentException("Failed to unpack TypedStruct", e);
234224
}
235225

236-
Filter.Provider provider = FilterRegistry.getDefaultRegistry().get(typeUrl);
226+
Filter.Provider provider = registryLookup.apply(typeUrl);
237227
if (provider == null) {
238228
throw new IllegalArgumentException("Action filter not found: " + typeUrl);
239229
}
240-
ConfigOrError<? extends FilterConfig> parsed = provider
241-
.parseFilterConfig(rawConfig);
230+
ConfigOrError<? extends FilterConfig> parsed = Filter.Parser
231+
.parseFilterConfig(provider, rawConfig, depth + 1);
242232
if (parsed.errorDetail != null) {
243233
throw new IllegalArgumentException(
244234
"Failed to parse child filter: " + parsed.errorDetail);
@@ -274,10 +264,17 @@ public String typeUrl() {
274264
static final class FilterDelegate {
275265
final List<DelegateEntry> delegates;
276266
private final double threshold;
267+
private final ThreadSafeRandom random;
277268

278269
FilterDelegate(List<DelegateEntry> delegates, @Nullable FractionalPercent samplePercent) {
270+
this(delegates, samplePercent, ThreadSafeRandom.ThreadSafeRandomImpl.instance);
271+
}
272+
273+
FilterDelegate(List<DelegateEntry> delegates, @Nullable FractionalPercent samplePercent,
274+
ThreadSafeRandom random) {
279275
this.delegates = Collections.unmodifiableList(delegates);
280276
this.threshold = calculateThreshold(samplePercent);
277+
this.random = random;
281278
}
282279

283280
private static double calculateThreshold(@Nullable FractionalPercent samplePercent) {
@@ -309,7 +306,7 @@ boolean shouldExecute() {
309306
if (threshold <= 0.0) {
310307
return false;
311308
}
312-
return ThreadLocalRandom.current().nextDouble() < threshold;
309+
return random.nextDouble() < threshold;
313310
}
314311
}
315312

@@ -380,9 +377,7 @@ public <ReqT, RespT> io.grpc.ServerCall.Listener<ReqT> interceptCall(
380377
}
381378
}
382379
} catch (Throwable t) {
383-
for (Filter f : filters) {
384-
f.close();
385-
}
380+
closeAll(filters);
386381
throw t;
387382
}
388383

@@ -449,7 +444,7 @@ private UnifiedMatcher<FilterDelegate> getMatcher(
449444
return effective.matcher;
450445
}
451446

452-
private static class MatchingDataImpl implements UnifiedMatcher.MatchingData {
447+
static class MatchingDataImpl implements UnifiedMatcher.MatchingData {
453448
private final Metadata headers;
454449
private final io.grpc.CallOptions callOptions;
455450
private final io.grpc.Attributes attributes;
@@ -519,6 +514,12 @@ public String getRelayedInput(com.github.xds.core.v3.TypedExtensionConfig inputC
519514
}
520515
}
521516

517+
private static void closeAll(Iterable<Filter> filters) {
518+
for (Filter f : filters) {
519+
f.close();
520+
}
521+
}
522+
522523
private static class CompositeClientCall<ReqT, RespT> extends io.grpc.ClientCall<ReqT, RespT> {
523524
private final MethodDescriptor<ReqT, RespT> method;
524525
private final io.grpc.CallOptions callOptions;
@@ -568,9 +569,7 @@ public void start(Listener<RespT> responseListener, Metadata headers) {
568569
}
569570
}
570571
} catch (Throwable t) {
571-
for (Filter f : filters) {
572-
f.close();
573-
}
572+
closeAll(filters);
574573
throw t;
575574
}
576575

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ public FaultFilter newInstance(String name) {
104104
}
105105

106106
@Override
107-
public ConfigOrError<FaultConfig> parseFilterConfig(Message rawProtoMessage) {
107+
public ConfigOrError<FaultConfig> parseFilterConfig(Message rawProtoMessage, int depth) {
108108
HTTPFault httpFaultProto;
109109
if (!(rawProtoMessage instanceof Any)) {
110110
return ConfigOrError.fromError("Invalid config type: " + rawProtoMessage.getClass());
@@ -119,8 +119,9 @@ public ConfigOrError<FaultConfig> parseFilterConfig(Message rawProtoMessage) {
119119
}
120120

121121
@Override
122-
public ConfigOrError<FaultConfig> parseFilterConfigOverride(Message rawProtoMessage) {
123-
return parseFilterConfig(rawProtoMessage);
122+
public ConfigOrError<FaultConfig> parseFilterConfigOverride(
123+
Message rawProtoMessage, int depth) {
124+
return parseFilterConfig(rawProtoMessage, depth);
124125
}
125126

126127
private static ConfigOrError<FaultConfig> parseHttpFault(HTTPFault httpFault) {

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

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,13 +93,34 @@ default boolean isServerFilter() {
9393
* Parses the top-level filter config from raw proto message. The message may be either a {@link
9494
* com.google.protobuf.Any} or a {@link com.google.protobuf.Struct}.
9595
*/
96-
ConfigOrError<? extends FilterConfig> parseFilterConfig(Message rawProtoMessage);
96+
ConfigOrError<? extends FilterConfig> parseFilterConfig(Message rawProtoMessage, int depth);
9797

9898
/**
9999
* Parses the per-filter override filter config from raw proto message. The message may be
100100
* either a {@link com.google.protobuf.Any} or a {@link com.google.protobuf.Struct}.
101101
*/
102-
ConfigOrError<? extends FilterConfig> parseFilterConfigOverride(Message rawProtoMessage);
102+
ConfigOrError<? extends FilterConfig> parseFilterConfigOverride(
103+
Message rawProtoMessage, int depth);
104+
}
105+
106+
class Parser {
107+
private static final int MAX_RECURSION_DEPTH = 8;
108+
109+
static ConfigOrError<? extends FilterConfig> parseFilterConfig(
110+
Provider provider, Message rawProtoMessage, int depth) {
111+
if (depth >= MAX_RECURSION_DEPTH) {
112+
return ConfigOrError.fromError("Maximum recursion depth of 8 exceeded");
113+
}
114+
return provider.parseFilterConfig(rawProtoMessage, depth);
115+
}
116+
117+
static ConfigOrError<? extends FilterConfig> parseFilterConfigOverride(
118+
Provider provider, Message rawProtoMessage, int depth) {
119+
if (depth >= MAX_RECURSION_DEPTH) {
120+
return ConfigOrError.fromError("Maximum recursion depth of 8 exceeded");
121+
}
122+
return provider.parseFilterConfigOverride(rawProtoMessage, depth);
123+
}
103124
}
104125

105126
/** Uses the FilterConfigs produced above to produce an HTTP filter interceptor for clients. */

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,8 @@ public GcpAuthenticationFilter newInstance(String name) {
8686
}
8787

8888
@Override
89-
public ConfigOrError<GcpAuthenticationConfig> parseFilterConfig(Message rawProtoMessage) {
89+
public ConfigOrError<GcpAuthenticationConfig> parseFilterConfig(
90+
Message rawProtoMessage, int depth) {
9091
GcpAuthnFilterConfig gcpAuthnProto;
9192
if (!(rawProtoMessage instanceof Any)) {
9293
return ConfigOrError.fromError("Invalid config type: " + rawProtoMessage.getClass());
@@ -121,8 +122,8 @@ public ConfigOrError<GcpAuthenticationConfig> parseFilterConfig(Message rawProto
121122

122123
@Override
123124
public ConfigOrError<GcpAuthenticationConfig> parseFilterConfigOverride(
124-
Message rawProtoMessage) {
125-
return parseFilterConfig(rawProtoMessage);
125+
Message rawProtoMessage, int depth) {
126+
return parseFilterConfig(rawProtoMessage, depth);
126127
}
127128
}
128129

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ public RbacFilter newInstance(String name) {
9494
}
9595

9696
@Override
97-
public ConfigOrError<RbacConfig> parseFilterConfig(Message rawProtoMessage) {
97+
public ConfigOrError<RbacConfig> parseFilterConfig(Message rawProtoMessage, int depth) {
9898
RBAC rbacProto;
9999
if (!(rawProtoMessage instanceof Any)) {
100100
return ConfigOrError.fromError("Invalid config type: " + rawProtoMessage.getClass());
@@ -109,7 +109,7 @@ public ConfigOrError<RbacConfig> parseFilterConfig(Message rawProtoMessage) {
109109
}
110110

111111
@Override
112-
public ConfigOrError<RbacConfig> parseFilterConfigOverride(Message rawProtoMessage) {
112+
public ConfigOrError<RbacConfig> parseFilterConfigOverride(Message rawProtoMessage, int depth) {
113113
RBACPerRoute rbacPerRoute;
114114
if (!(rawProtoMessage instanceof Any)) {
115115
return ConfigOrError.fromError("Invalid config type: " + rawProtoMessage.getClass());

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,13 +61,14 @@ public RouterFilter newInstance(String name) {
6161
}
6262

6363
@Override
64-
public ConfigOrError<? extends FilterConfig> parseFilterConfig(Message rawProtoMessage) {
64+
public ConfigOrError<? extends FilterConfig> parseFilterConfig(
65+
Message rawProtoMessage, int depth) {
6566
return ConfigOrError.fromConfig(ROUTER_CONFIG);
6667
}
6768

6869
@Override
6970
public ConfigOrError<? extends FilterConfig> parseFilterConfigOverride(
70-
Message rawProtoMessage) {
71+
Message rawProtoMessage, int depth) {
7172
return ConfigOrError.fromError("Router Filter should not have override config");
7273
}
7374
}

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ interface ThreadSafeRandom {
2727

2828
long nextLong(long bound);
2929

30+
double nextDouble();
31+
3032
final class ThreadSafeRandomImpl implements ThreadSafeRandom {
3133

3234
static final ThreadSafeRandom instance = new ThreadSafeRandomImpl();
@@ -47,5 +49,10 @@ public long nextLong() {
4749
public long nextLong(long bound) {
4850
return ThreadLocalRandom.current().nextLong(bound);
4951
}
52+
53+
@Override
54+
public double nextDouble() {
55+
return ThreadLocalRandom.current().nextDouble();
56+
}
5057
}
5158
}

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -616,7 +616,8 @@ static StructOrError<Filter.FilterConfig> parseHttpFilter(
616616
"HttpFilter [" + filterName + "](" + typeUrl + ") is required but unsupported for " + (
617617
isForClient ? "client" : "server"));
618618
}
619-
ConfigOrError<? extends FilterConfig> filterConfig = provider.parseFilterConfig(rawConfig);
619+
ConfigOrError<? extends FilterConfig> filterConfig =
620+
Filter.Parser.parseFilterConfig(provider, rawConfig, 0);
620621
if (filterConfig.errorDetail != null) {
621622
return StructOrError.fromError(
622623
"Invalid filter config for HttpFilter [" + filterName + "]: " + filterConfig.errorDetail);

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -255,7 +255,7 @@ static StructOrError<Map<String, FilterConfig>> parseOverrideFilterConfigs(
255255
"HttpFilter [" + name + "](" + typeUrl + ") is required but unsupported");
256256
}
257257
ConfigOrError<? extends Filter.FilterConfig> filterConfig =
258-
provider.parseFilterConfigOverride(rawConfig);
258+
Filter.Parser.parseFilterConfigOverride(provider, rawConfig, 0);
259259
if (filterConfig.errorDetail != null) {
260260
return StructOrError.fromError(
261261
"Invalid filter config for HttpFilter [" + name + "]: " + filterConfig.errorDetail);

0 commit comments

Comments
 (0)