Skip to content

Commit 8515729

Browse files
shivaspeaksshivaspeaks
committed
address comments
1 parent 60a9356 commit 8515729

5 files changed

Lines changed: 93 additions & 70 deletions

File tree

‎xds/src/main/java/io/grpc/xds/internal/matcher/CelCommon.java‎

Lines changed: 16 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -51,36 +51,30 @@ final class CelCommon {
5151
})
5252
.build();
5353

54-
/**
55-
* Set of allowed function names based on gRFC A106.
56-
*/
57-
private static final ImmutableSet<String> ALLOWED_FUNCTIONS = ImmutableSet.of(
58-
"size", "matches", "contains", "startsWith", "endsWith", "timestamp", "duration",
59-
"int", "uint", "double", "string", "bytes", "bool", "==", "!=", ">", "<", ">=", "<=",
60-
"&&", "||", "!", "+", "-", "*", "/", "%", "in", "has", "or");
54+
55+
56+
private static final ImmutableSet<String> ALLOWED_EXACT_OVERLOAD_IDS = ImmutableSet.of(
57+
"equals", "not_equals", "logical_and", "logical_or", "logical_not");
6158

6259
/**
6360
* Regular expression pattern to validate internal CEL overload IDs.
6461
*
6562
* <p>
6663
* Standard CEL operators and conversion functions often have empty names in the
67-
* AST
68-
* and are identified solely by their overload IDs (e.g., {@code equals} for
69-
* {@code ==},
70-
* {@code divide_int64} for {@code /}).
64+
* AST and are identified solely by their overload IDs (e.g., {@code equals} for
65+
* {@code ==}, {@code divide_int64} for {@code /}).
7166
*
7267
* <p>
7368
* This pattern matches allowed overload IDs by their prefixes (e.g.,
74-
* {@code divide},
75-
* {@code size}), optionally followed by numeric types (e.g., {@code int64}) and
76-
* type-specific
77-
* suffixes (e.g., {@code _string}, {@code _int64}).
69+
* {@code divide}, {@code size}), optionally followed by numeric types
70+
* (e.g., {@code int64}) and type-specific suffixes (e.g., {@code _string},
71+
* {@code _int64}).
7872
*/
79-
private static final Pattern ALLOWED_OVERLOAD_ID_PATTERN = Pattern.compile(
73+
private static final Pattern ALLOWED_OVERLOAD_ID_PREFIX_PATTERN = Pattern.compile(
8074
"^(size|matches|contains|startsWith|endsWith|starts_with|ends_with|"
8175
+ "timestamp|duration|in|index|has|int|uint|double|string|bytes|bool|"
82-
+ "equals|not_equals|less|less_equals|greater|greater_equals|"
83-
+ "add|subtract|multiply|divide|modulo|logical_and|logical_or|logical_not)"
76+
+ "less|less_equals|greater|greater_equals|"
77+
+ "add|subtract|multiply|divide|modulo|negate)"
8478
+ "[0-9]*(_.*)?$");
8579

8680
static final CelRuntime RUNTIME = CelRuntimeFactory.standardCelRuntimeBuilder()
@@ -110,7 +104,8 @@ static void checkAllowedReferences(CelAbstractSyntaxTree ast) {
110104
if (name.isEmpty()) {
111105
boolean allowed = false;
112106
for (String id : ref.overloadIds()) {
113-
if (ALLOWED_OVERLOAD_ID_PATTERN.matcher(id).matches()) {
107+
if (ALLOWED_EXACT_OVERLOAD_IDS.contains(id)
108+
|| ALLOWED_OVERLOAD_ID_PREFIX_PATTERN.matcher(id).matches()) {
114109
allowed = true;
115110
break;
116111
}
@@ -120,9 +115,9 @@ static void checkAllowedReferences(CelAbstractSyntaxTree ast) {
120115
"CEL expression references unknown function with overload IDs: "
121116
+ ref.overloadIds());
122117
}
123-
} else if (!ALLOWED_FUNCTIONS.contains(name)) {
118+
} else {
124119
throw new IllegalArgumentException(
125-
"CEL expression references unknown function: " + name);
120+
"CEL expression references unsupported named function: " + name);
126121
}
127122
}
128123
}

‎xds/src/main/java/io/grpc/xds/internal/matcher/CelStringExtractor.java‎

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -66,23 +66,22 @@ public static CelStringExtractor compile(CelAbstractSyntaxTree ast)
6666
* fails.
6767
*/
6868
public String extract(Object input) throws CelEvaluationException {
69-
Object result;
70-
try {
71-
if (input instanceof CelVariableResolver) {
72-
result = program.eval((CelVariableResolver) input);
73-
} else {
74-
throw new CelEvaluationException(
75-
"Unsupported input type for CEL evaluation: " + input.getClass().getName());
76-
}
77-
} catch (CelEvaluationException e) {
78-
if (defaultValue != null) {
79-
return defaultValue;
80-
}
81-
throw e;
82-
}
69+
if (input instanceof CelVariableResolver) {
70+
try {
71+
Object result = program.eval((CelVariableResolver) input);
8372

84-
if (result instanceof String) {
85-
return (String) result;
73+
if (result instanceof String) {
74+
return (String) result;
75+
}
76+
} catch (CelEvaluationException e) {
77+
if (defaultValue == null) {
78+
throw e;
79+
}
80+
}
81+
} else if (defaultValue == null) {
82+
throw new CelEvaluationException(
83+
"Unsupported input type for CEL evaluation: "
84+
+ (input == null ? "null" : input.getClass().getName()));
8685
}
8786
return defaultValue;
8887
}

‎xds/src/main/java/io/grpc/xds/internal/matcher/GrpcCelEnvironment.java‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,10 @@
1616

1717
package io.grpc.xds.internal.matcher;
1818

19-
import com.google.common.base.Splitter;
2019
import com.google.common.collect.ImmutableSet;
2120
import dev.cel.runtime.CelVariableResolver;
2221
import io.grpc.Metadata;
2322
import java.util.AbstractMap;
24-
import java.util.List;
2523
import java.util.Optional;
2624
import java.util.Set;
2725
import javax.annotation.Nullable;
@@ -30,7 +28,6 @@
3028
* CEL Environment for gRPC xDS matching.
3129
*/
3230
final class GrpcCelEnvironment implements CelVariableResolver {
33-
private static final Splitter SPLITTER = Splitter.on('.').limit(2);
3431
private final MatchContext context;
3532

3633
GrpcCelEnvironment(MatchContext context) {
@@ -42,10 +39,6 @@ public Optional<Object> find(String name) {
4239
if (name.equals("request")) {
4340
return Optional.of(new LazyRequestMap(this));
4441
}
45-
List<String> components = SPLITTER.splitToList(name);
46-
if (components.size() == 2 && components.get(0).equals("request")) {
47-
return Optional.ofNullable(getRequestField(components.get(1)));
48-
}
4942
return Optional.empty();
5043
}
5144

‎xds/src/test/java/io/grpc/xds/internal/matcher/CelCommonTest.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,4 +122,10 @@ public void checkAllowedReferences_unknownVariable_throws() throws Exception {
122122
.contains("CEL expression references unknown variable: unknown_var");
123123
}
124124
}
125+
126+
@Test
127+
public void checkAllowedReferences_negation() throws Exception {
128+
assertAllowed("-(1) == -1");
129+
assertAllowed("-(1.0) == -1.0");
130+
}
125131
}

‎xds/src/test/java/io/grpc/xds/internal/matcher/CelEnvironmentTest.java‎

Lines changed: 56 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -91,9 +91,13 @@ public void celEnvironment_resolvesRequestField() {
9191

9292
GrpcCelEnvironment env = new GrpcCelEnvironment(context);
9393

94-
Optional<Object> result = env.find("request.path");
94+
Optional<Object> result = env.find("request");
9595
assertThat(result.isPresent()).isTrue();
96-
assertThat(result.get()).isEqualTo("/foo");
96+
assertThat(result.get()).isInstanceOf(Map.class);
97+
98+
@SuppressWarnings("unchecked")
99+
Map<String, Object> requestMap = (Map<String, Object>) result.get();
100+
assertThat(requestMap.get("path")).isEqualTo("/foo");
97101
}
98102

99103
@Test
@@ -201,9 +205,11 @@ public void celEnvironment_method_fallback() {
201205

202206
GrpcCelEnvironment env = new GrpcCelEnvironment(context);
203207

204-
Optional<Object> result = env.find("request.method");
208+
Optional<Object> result = env.find("request");
205209
assertThat(result.isPresent()).isTrue();
206-
assertThat(result.get()).isEqualTo("POST");
210+
@SuppressWarnings("unchecked")
211+
Map<String, Object> requestMap = (Map<String, Object>) result.get();
212+
assertThat(requestMap.get("method")).isEqualTo("POST");
207213
}
208214

209215
@Test
@@ -233,15 +239,15 @@ public void celEnvironment_timeField_supportedButNull() {
233239
MatchContext context = MatchContext.newBuilder().build();
234240
GrpcCelEnvironment env = new GrpcCelEnvironment(context);
235241

236-
Optional<Object> result = env.find("request.time");
237-
assertThat(result.isPresent()).isFalse();
238-
239-
// But it should be present in the map key set
240-
Map<?, ?> requestMap = (Map<?, ?>) env.find("request").get();
242+
Optional<Object> result = env.find("request");
243+
assertThat(result.isPresent()).isTrue();
244+
@SuppressWarnings("unchecked")
245+
Map<String, Object> requestMap = (Map<String, Object>) result.get();
241246
assertThat(requestMap.containsKey("time")).isTrue();
242247
assertThat(requestMap.get("time")).isNull();
243248
}
244249

250+
245251
@Test
246252
public void headersWrapper_size() {
247253
Metadata metadata = new Metadata();
@@ -265,24 +271,32 @@ public void celEnvironment_accessAllFields() {
265271
.setMethod("GET")
266272
.build();
267273

268-
269274
GrpcCelEnvironment env = new GrpcCelEnvironment(context);
270-
271-
assertThat(env.find("request.host").get()).isEqualTo("host");
272-
assertThat(env.find("request.id").get()).isEqualTo("id");
273-
assertThat(env.find("request.method").get()).isEqualTo("GET");
274-
assertThat(env.find("request.scheme").get()).isEqualTo("");
275-
assertThat(env.find("request.protocol").get()).isEqualTo("");
276-
assertThat(env.find("request.query").get())
277-
.isEqualTo("");
278-
assertThat(env.find("request.headers").get()).isInstanceOf(HeadersWrapper.class);
275+
Optional<Object> result = env.find("request");
276+
assertThat(result.isPresent()).isTrue();
277+
@SuppressWarnings("unchecked")
278+
Map<String, Object> requestMap = (Map<String, Object>) result.get();
279+
280+
assertThat(requestMap.get("host")).isEqualTo("host");
281+
assertThat(requestMap.get("id")).isEqualTo("id");
282+
assertThat(requestMap.get("method")).isEqualTo("GET");
283+
assertThat(requestMap.get("scheme")).isEqualTo("");
284+
assertThat(requestMap.get("protocol")).isEqualTo("");
285+
assertThat(requestMap.get("query")).isEqualTo("");
286+
assertThat(requestMap.get("headers")).isInstanceOf(HeadersWrapper.class);
279287
}
280288

281289
@Test
282290
public void celEnvironment_find_unknownField() {
283291
MatchContext context = MatchContext.newBuilder().build();
284292
GrpcCelEnvironment env = new GrpcCelEnvironment(context);
285-
assertThat(env.find("request.unknown").isPresent()).isFalse();
293+
294+
Optional<Object> result = env.find("request");
295+
assertThat(result.isPresent()).isTrue();
296+
@SuppressWarnings("unchecked")
297+
Map<String, Object> requestMap = (Map<String, Object>) result.get();
298+
assertThat(requestMap.get("unknown")).isNull();
299+
286300
assertThat(env.find("other").isPresent()).isFalse();
287301
}
288302

@@ -426,8 +440,12 @@ public void celEnvironment_resolvesRefererAndUserAgent() {
426440
.build();
427441
GrpcCelEnvironment env = new GrpcCelEnvironment(context);
428442

429-
assertThat(env.find("request.referer").get()).isEqualTo("http://example.com");
430-
assertThat(env.find("request.useragent").get()).isEqualTo("grpc-test");
443+
Optional<Object> result = env.find("request");
444+
assertThat(result.isPresent()).isTrue();
445+
@SuppressWarnings("unchecked")
446+
Map<String, Object> requestMap = (Map<String, Object>) result.get();
447+
assertThat(requestMap.get("referer")).isEqualTo("http://example.com");
448+
assertThat(requestMap.get("useragent")).isEqualTo("grpc-test");
431449
}
432450

433451
@Test
@@ -441,8 +459,11 @@ public void celEnvironment_joinsMultipleHeaderValues() {
441459
.build();
442460
GrpcCelEnvironment env = new GrpcCelEnvironment(context);
443461

444-
// Tests the String.join logic in getHeader
445-
assertThat(env.find("request.referer").get()).isEqualTo("v1,v2");
462+
Optional<Object> result = env.find("request");
463+
assertThat(result.isPresent()).isTrue();
464+
@SuppressWarnings("unchecked")
465+
Map<String, Object> requestMap = (Map<String, Object>) result.get();
466+
assertThat(requestMap.get("referer")).isEqualTo("v1,v2");
446467
}
447468

448469
@Test
@@ -451,7 +472,12 @@ public void celEnvironment_find_invalidFormat() {
451472
GrpcCelEnvironment env = new GrpcCelEnvironment(context);
452473

453474
assertThat(env.find("other.path").isPresent()).isFalse();
454-
assertThat(env.find("request.a.b").isPresent()).isFalse();
475+
476+
Optional<Object> result = env.find("request");
477+
assertThat(result.isPresent()).isTrue();
478+
@SuppressWarnings("unchecked")
479+
Map<String, Object> requestMap = (Map<String, Object>) result.get();
480+
assertThat(requestMap.get("a")).isNull();
455481
}
456482

457483
@Test
@@ -469,7 +495,11 @@ public void celEnvironment_missingHeader_returnsEmptyString() {
469495
MatchContext context = MatchContext.newBuilder().build();
470496
GrpcCelEnvironment env = new GrpcCelEnvironment(context);
471497

472-
assertThat(env.find("request.referer").get()).isEqualTo("");
498+
Optional<Object> result = env.find("request");
499+
assertThat(result.isPresent()).isTrue();
500+
@SuppressWarnings("unchecked")
501+
Map<String, Object> requestMap = (Map<String, Object>) result.get();
502+
assertThat(requestMap.get("referer")).isEqualTo("");
473503
}
474504

475505

0 commit comments

Comments
 (0)