Skip to content

Commit ce4e16c

Browse files
shivaspeaksshivaspeaks
committed
resolve comments
1 parent b9452ae commit ce4e16c

6 files changed

Lines changed: 169 additions & 86 deletions

File tree

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,10 @@ static void checkAllowedReferences(CelAbstractSyntaxTree ast) {
102102
if (name.isEmpty()) {
103103
boolean allowed = false;
104104
for (String id : ref.overloadIds()) {
105+
if (id.equals("add_string") || id.equals("add_list") || id.endsWith("_to_string")) {
106+
allowed = false;
107+
break;
108+
}
105109
if (ALLOWED_EXACT_OVERLOAD_IDS.contains(id)
106110
|| ALLOWED_OVERLOAD_ID_PREFIX_PATTERN.matcher(id).matches()) {
107111
allowed = true;
@@ -114,6 +118,13 @@ static void checkAllowedReferences(CelAbstractSyntaxTree ast) {
114118
+ ref.overloadIds());
115119
}
116120
} else {
121+
// Standard conversion functions (like string(x)) are named in the AST.
122+
// We must explicitly reject 'string' here since it's disabled in the environment.
123+
if (name.equals("string")) {
124+
throw new IllegalArgumentException(
125+
"CEL expression references unknown function with overload IDs: "
126+
+ ref.overloadIds());
127+
}
117128
throw new IllegalArgumentException(
118129
"CEL expression references unsupported named function: " + name);
119130
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ private Object getRequestField(String requestField) {
4747
switch (requestField) {
4848
case "headers": return new HeadersWrapper(context);
4949
case "host": return orEmpty(context.getHost());
50-
case "id": return orEmpty(context.getId());
50+
case "id": return getHeader("x-request-id");
5151
case "method": return or(context.getMethod(), "POST");
5252
case "path":
5353
case "url_path":

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

Lines changed: 47 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -61,30 +61,34 @@ public String get(Object key) {
6161

6262
@Nullable
6363
private String getHeader(String headerName) {
64-
if (headerName.endsWith(Metadata.BINARY_HEADER_SUFFIX)) {
65-
Iterable<byte[]> values = context.getMetadata().getAll(
66-
Metadata.Key.of(headerName, Metadata.BINARY_BYTE_MARSHALLER));
64+
try {
65+
if (headerName.endsWith(Metadata.BINARY_HEADER_SUFFIX)) {
66+
Iterable<byte[]> values = context.getMetadata().getAll(
67+
Metadata.Key.of(headerName, Metadata.BINARY_BYTE_MARSHALLER));
68+
if (values == null) {
69+
return null;
70+
}
71+
StringBuilder sb = new StringBuilder();
72+
boolean first = true;
73+
for (byte[] value : values) {
74+
if (!first) {
75+
sb.append(",");
76+
}
77+
first = false;
78+
sb.append(BaseEncoding.base64().omitPadding().encode(value));
79+
}
80+
return sb.toString();
81+
}
82+
Metadata metadata = context.getMetadata();
83+
Iterable<String> values = metadata.getAll(
84+
Metadata.Key.of(headerName, Metadata.ASCII_STRING_MARSHALLER));
6785
if (values == null) {
6886
return null;
6987
}
70-
StringBuilder sb = new StringBuilder();
71-
boolean first = true;
72-
for (byte[] value : values) {
73-
if (!first) {
74-
sb.append(",");
75-
}
76-
first = false;
77-
sb.append(BaseEncoding.base64().encode(value));
78-
}
79-
return sb.toString();
80-
}
81-
Metadata metadata = context.getMetadata();
82-
Iterable<String> values = metadata.getAll(
83-
Metadata.Key.of(headerName, Metadata.ASCII_STRING_MARSHALLER));
84-
if (values == null) {
88+
return String.join(",", values);
89+
} catch (IllegalArgumentException e) {
8590
return null;
8691
}
87-
return String.join(",", values);
8892
}
8993

9094
@Override
@@ -93,34 +97,44 @@ public boolean containsKey(Object key) {
9397
return false;
9498
}
9599
String headerName = ((String) key).toLowerCase(java.util.Locale.ROOT);
96-
if ("te".equals(headerName)) {
100+
if (headerName.equals("te")) {
97101
return false;
98102
}
99-
if (PSEUDO_HEADERS.contains(headerName)) {
103+
if (PSEUDO_HEADERS.contains(headerName) || headerName.equals("host")) {
100104
return true;
101105
}
102-
if (headerName.endsWith(Metadata.BINARY_HEADER_SUFFIX)) {
106+
try {
107+
if (headerName.endsWith(Metadata.BINARY_HEADER_SUFFIX)) {
108+
return context.getMetadata().containsKey(
109+
Metadata.Key.of(headerName, Metadata.BINARY_BYTE_MARSHALLER));
110+
}
103111
return context.getMetadata().containsKey(
104-
Metadata.Key.of(headerName, Metadata.BINARY_BYTE_MARSHALLER));
112+
Metadata.Key.of(headerName, Metadata.ASCII_STRING_MARSHALLER));
113+
} catch (IllegalArgumentException e) {
114+
return false;
105115
}
106-
return context.getMetadata().containsKey(
107-
Metadata.Key.of(headerName, Metadata.ASCII_STRING_MARSHALLER));
108116
}
109117

110118
@Override
111119
public Set<String> keySet() {
112-
return ImmutableSet.<String>builder()
113-
.addAll(context.getMetadata().keys())
114-
.addAll(PSEUDO_HEADERS)
115-
.build();
120+
ImmutableSet.Builder<String> builder = ImmutableSet.builder();
121+
for (String key : context.getMetadata().keys()) {
122+
String lowerKey = key.toLowerCase(java.util.Locale.ROOT);
123+
// Filter out any keys we provide specialized aliases/values for.
124+
if (!lowerKey.equals("te")
125+
&& !lowerKey.equals("host")
126+
&& !PSEUDO_HEADERS.contains(lowerKey)) {
127+
builder.add(key);
128+
}
129+
}
130+
builder.addAll(PSEUDO_HEADERS);
131+
builder.add("host");
132+
return builder.build();
116133
}
117134

118135
@Override
119136
public int size() {
120-
// Metadata.keys() returns a Set of unique keys, so we can just add the sizes.
121-
// Note: This counts the number of unique header names, which is consistent with
122-
// keySet().size().
123-
return context.getMetadata().keys().size() + PSEUDO_HEADERS.size();
137+
return keySet().size();
124138
}
125139

126140
@Override

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

Lines changed: 6 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -18,52 +18,36 @@
1818

1919
import com.google.common.base.Preconditions;
2020
import io.grpc.Metadata;
21-
import javax.annotation.Nullable;
2221

2322
public final class MatchContext {
2423
private final Metadata metadata;
25-
@Nullable
2624
private final String path;
27-
@Nullable
2825
private final String host;
29-
@Nullable
3026
private final String method;
31-
@Nullable
32-
private final String id;
3327

34-
public MatchContext(Metadata metadata, @Nullable String path,
35-
@Nullable String host, @Nullable String method,
36-
@Nullable String id) {
28+
public MatchContext(Metadata metadata, String path,
29+
String host, String method) {
3730
this.metadata = Preconditions.checkNotNull(metadata, "metadata");
38-
this.path = path;
39-
this.host = host;
40-
this.method = method;
41-
this.id = id;
31+
this.path = Preconditions.checkNotNull(path, "path");
32+
this.host = Preconditions.checkNotNull(host, "host");
33+
this.method = Preconditions.checkNotNull(method, "method");
4234
}
4335

4436
public Metadata getMetadata() {
4537
return metadata;
4638
}
4739

48-
@Nullable
4940
public String getPath() {
5041
return path;
5142
}
5243

53-
@Nullable
5444
public String getHost() {
5545
return host;
5646
}
5747

58-
@Nullable
5948
public String getMethod() {
6049
return method;
6150
}
62-
63-
@Nullable
64-
public String getId() {
65-
return id;
66-
}
6751

6852
public static Builder newBuilder() {
6953
return new Builder();
@@ -74,7 +58,6 @@ public static final class Builder {
7458
private String path;
7559
private String host;
7660
private String method;
77-
private String id;
7861

7962
public Builder setMetadata(Metadata metadata) {
8063
this.metadata = metadata;
@@ -96,13 +79,8 @@ public Builder setMethod(String method) {
9679
return this;
9780
}
9881

99-
public Builder setId(String id) {
100-
this.id = id;
101-
return this;
102-
}
103-
10482
public MatchContext build() {
105-
return new MatchContext(metadata, path, host, method, id);
83+
return new MatchContext(metadata, path, host, method);
10684
}
10785
}
10886
}

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,11 @@ public void checkAllowedReferences_functions() throws Exception {
9393
assertAllowed("int(1) == 1");
9494
assertAllowed("uint(1) == 1u");
9595
assertAllowed("double(1) == 1.0");
96-
assertAllowed("string(1) == '1'");
96+
97+
// Disallowed functions / overloads
98+
assertDisallowed("string(1) == '1'");
99+
assertDisallowed("'a' + 'b' == 'ab'");
100+
assertDisallowed("[1] + [2] == [1, 2]");
97101
}
98102

99103
@Test

0 commit comments

Comments
 (0)