Skip to content

Commit 31a57f7

Browse files
m-nagarajanm-nagarajan
authored
[server][test] Allow and skip acl checks for health check uri from clients to server (#798)
Fast client warms up H2 connections by hitting the /health endpoint in servers but as uri validation for requests from clients to servers happens in StoreAclHandler, added changes to allow the /heatlh uri and by-pass acl validation since it is just a health check endpoint without any pii.
1 parent 91d249d commit 31a57f7

File tree

2 files changed

+102
-23
lines changed

2 files changed

+102
-23
lines changed

Diff for: ‎internal/venice-common/src/main/java/com/linkedin/venice/acl/handler/StoreAclHandler.java

+11-4
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,10 @@
2727
import io.netty.util.ReferenceCountUtil;
2828
import java.net.URI;
2929
import java.security.cert.X509Certificate;
30+
import java.util.Arrays;
31+
import java.util.HashSet;
3032
import java.util.Objects;
33+
import java.util.Set;
3134
import java.util.stream.Collectors;
3235
import javax.net.ssl.SSLPeerUnverifiedException;
3336
import javax.net.ssl.SSLSession;
@@ -93,21 +96,25 @@ public void channelRead0(ChannelHandlerContext ctx, HttpRequest req) throws SSLP
9396
String uri = req.uri();
9497
// Parse resource type and store name
9598
String[] requestParts = URI.create(uri).getPath().split("/");
96-
if (requestParts.length < 3) {
99+
// invalid request if requestParts.length < 3 except for HEALTH check from venice client
100+
if (requestParts.length < 3
101+
&& !(requestParts.length == 2 && requestParts[1].toUpperCase().equals(QueryAction.HEALTH.toString()))) {
97102
NettyUtils.setupResponseAndFlush(
98103
HttpResponseStatus.BAD_REQUEST,
99-
("Invalid request uri: " + uri).getBytes(),
104+
("Invalid request uri: " + uri).getBytes(),
100105
false,
101106
ctx);
102107
return;
103108
}
104109

105110
/**
106-
* Skip ACL for requests to /metadata and /admin as there's no sensitive information in the response.
111+
* Skip ACL for requests to /metadata, /admin and /health as there's no sensitive information in the response.
107112
*/
113+
Set<QueryAction> queriesToSkipAcl =
114+
new HashSet<>(Arrays.asList(QueryAction.METADATA, QueryAction.ADMIN, QueryAction.HEALTH));
108115
try {
109116
QueryAction queryAction = QueryAction.valueOf(requestParts[1].toUpperCase());
110-
if (queryAction.equals(QueryAction.METADATA) || queryAction.equals(QueryAction.ADMIN)) {
117+
if (queriesToSkipAcl.contains(queryAction)) {
111118
ReferenceCountUtil.retain(req);
112119
ctx.fireChannelRead(req);
113120
return;

Diff for: ‎internal/venice-common/src/test/java/com/linkedin/venice/acl/StoreAclHandlerTest.java

+91-19
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,19 @@ public class StoreAclHandlerTest {
4545
private boolean[] isSystemStore = { false };
4646
private boolean[] isFailOpen = { false };
4747
private boolean[] isMetadata = { false };
48+
private boolean[] isHealthCheck = { false };
49+
private boolean[] isBadUri = { false };
50+
51+
private void resetAllConditions() {
52+
hasAccess[0] = false;
53+
hasAcl[0] = false;
54+
hasStore[0] = false;
55+
isSystemStore[0] = false;
56+
isFailOpen[0] = false;
57+
isMetadata[0] = false;
58+
isHealthCheck[0] = false;
59+
isBadUri[0] = false;
60+
}
4861

4962
@BeforeMethod
5063
public void setUp() throws Exception {
@@ -79,28 +92,28 @@ public void setUp() throws Exception {
7992
@Test
8093
public void accessGranted() throws Exception {
8194
hasAccess[0] = true;
82-
enumerate(hasAcl, hasStore, isSystemStore, isFailOpen, isMetadata);
95+
enumerate(hasAcl, hasStore, isSystemStore, isFailOpen, isMetadata, isHealthCheck);
8396

8497
verify(ctx, never()).writeAndFlush(argThat(new ContextMatcher(HttpResponseStatus.FORBIDDEN)));
8598
verify(ctx, never()).writeAndFlush(argThat(new ContextMatcher(HttpResponseStatus.UNAUTHORIZED)));
8699

87100
// Store doesn't exist 8 times
88101
verify(ctx, times(8)).writeAndFlush(argThat(new ContextMatcher(HttpResponseStatus.BAD_REQUEST)));
89102

90-
// No access control 20 times + access control 4 times
91-
verify(ctx, times(24)).fireChannelRead(req);
103+
// No access control (METADATA/HEALTH/ADMIN or system store) => 52 times, access control => 4 times
104+
verify(ctx, times(56)).fireChannelRead(req);
92105
}
93106

94107
@Test
95108
public void accessDenied() throws Exception {
96109
hasAccess[0] = false;
97-
enumerate(hasAcl, hasStore, isSystemStore, isFailOpen, isMetadata);
110+
enumerate(hasAcl, hasStore, isSystemStore, isFailOpen, isMetadata, isHealthCheck);
98111

99112
// Store doesn't exist 8 times
100113
verify(ctx, times(8)).writeAndFlush(argThat(new ContextMatcher(HttpResponseStatus.BAD_REQUEST)));
101114

102-
// No access control 20 times
103-
verify(ctx, times(20)).fireChannelRead(req);
115+
// No access control ((METADATA/HEALTH/ADMIN or system store) => 52 times
116+
verify(ctx, times(52)).fireChannelRead(req);
104117

105118
// 1 of the 4 rejects is due to internal error
106119
verify(ctx, times(1)).writeAndFlush(argThat(new ContextMatcher(HttpResponseStatus.UNAUTHORIZED)));
@@ -112,12 +125,12 @@ public void accessDenied() throws Exception {
112125
@Test
113126
public void storeExists() throws Exception {
114127
hasStore[0] = true;
115-
enumerate(hasAccess, hasAcl, isFailOpen, isSystemStore, isMetadata);
128+
enumerate(hasAccess, hasAcl, isFailOpen, isSystemStore, isMetadata, isHealthCheck);
116129

117130
verify(ctx, never()).writeAndFlush(argThat(new ContextMatcher(HttpResponseStatus.BAD_REQUEST)));
118131

119-
// No access control 24 times, access control 4 times granted
120-
verify(ctx, times(28)).fireChannelRead(req);
132+
// No access control (METADATA/HEALTH/ADMIN or system store) => 56 times, access control => 4 times granted
133+
verify(ctx, times(60)).fireChannelRead(req);
121134

122135
// 1 of the 4 rejects is due to internal error
123136
verify(ctx, times(1)).writeAndFlush(argThat(new ContextMatcher(HttpResponseStatus.UNAUTHORIZED)));
@@ -129,9 +142,10 @@ public void storeExists() throws Exception {
129142
@Test
130143
public void storeMissing() throws Exception {
131144
hasStore[0] = false;
132-
enumerate(hasAccess, hasAcl, isFailOpen, isSystemStore, isMetadata);
145+
enumerate(hasAccess, hasAcl, isFailOpen, isSystemStore, isMetadata, isHealthCheck);
133146

134-
verify(ctx, times(16)).fireChannelRead(req);
147+
// No access control (METADATA/HEALTH/ADMIN) => 48 times
148+
verify(ctx, times(48)).fireChannelRead(req);
135149
verify(ctx, never()).writeAndFlush(argThat(new ContextMatcher(HttpResponseStatus.FORBIDDEN)));
136150
verify(ctx, never()).writeAndFlush(argThat(new ContextMatcher(HttpResponseStatus.UNAUTHORIZED)));
137151
verify(ctx, times(16)).writeAndFlush(argThat(new ContextMatcher(HttpResponseStatus.BAD_REQUEST)));
@@ -141,27 +155,73 @@ public void storeMissing() throws Exception {
141155
public void aclDisabledForSystemStore() throws Exception {
142156
isSystemStore[0] = true;
143157
hasStore[0] = true;
144-
enumerate(hasAccess, hasAcl, isFailOpen, isMetadata);
158+
enumerate(hasAccess, hasAcl, isFailOpen, isMetadata, isHealthCheck);
145159

146160
verify(ctx, never()).writeAndFlush(any());
147-
verify(ctx, times(16)).fireChannelRead(req);
161+
// No access control (METADATA/HEALTH/ADMIN or system store) => 32 times
162+
verify(ctx, times(32)).fireChannelRead(req);
148163
}
149164

150165
@Test
151166
public void aclDisabledForMetadataEndpoint() throws Exception {
152167
isMetadata[0] = true;
153-
enumerate(hasAccess, hasAcl, isSystemStore, isFailOpen);
168+
enumerate(hasAccess, hasAcl, isSystemStore, isFailOpen, isHealthCheck);
154169

155170
verify(ctx, never()).writeAndFlush(any());
156-
verify(ctx, times(16)).fireChannelRead(req);
171+
// No access control (METADATA) => 32 times
172+
verify(ctx, times(32)).fireChannelRead(req);
173+
}
174+
175+
@Test
176+
public void aclDisabledForHealthCheckEndpoint() throws Exception {
177+
isHealthCheck[0] = true;
178+
enumerate(hasAccess, hasAcl, isSystemStore, isFailOpen, isMetadata);
179+
180+
verify(ctx, never()).writeAndFlush(any());
181+
// No access control (HEALTH) => 32 times
182+
verify(ctx, times(32)).fireChannelRead(req);
183+
}
184+
185+
@Test
186+
public void isBadUri() throws Exception {
187+
isBadUri[0] = true;
188+
enumerate(hasAcl, hasStore, hasAccess, isSystemStore, isFailOpen, isMetadata, isHealthCheck);
189+
190+
// all 128 times should fail for BAD_REQUEST
191+
verify(ctx, times(128)).writeAndFlush(argThat(new ContextMatcher(HttpResponseStatus.BAD_REQUEST)));
157192
}
158193

159194
@Test
160195
public void aclMissing() throws Exception {
161-
enumerate(hasStore, hasAcl, hasAccess, isSystemStore, isFailOpen, isMetadata);
196+
hasAcl[0] = false;
197+
enumerate(hasStore, hasAccess, isSystemStore, isFailOpen, isMetadata, isHealthCheck);
162198

199+
// No access control (METADATA/HEALTH/ADMIN or system store) => 52 times, access control => 2 times granted
200+
verify(ctx, times(54)).fireChannelRead(req);
201+
verify(ctx, times(8)).writeAndFlush(argThat(new ContextMatcher(HttpResponseStatus.BAD_REQUEST)));
163202
verify(ctx, times(1)).writeAndFlush(argThat(new ContextMatcher(HttpResponseStatus.UNAUTHORIZED)));
203+
verify(ctx, times(1)).writeAndFlush(argThat(new ContextMatcher(HttpResponseStatus.FORBIDDEN)));
204+
}
205+
206+
@Test
207+
public void aclPresent() throws Exception {
208+
hasAcl[0] = true;
209+
enumerate(hasStore, hasAccess, isSystemStore, isFailOpen, isMetadata, isHealthCheck);
210+
211+
// No access control (METADATA/HEALTH/ADMIN or system store) => 52 times, access control => 2 times granted
212+
verify(ctx, times(54)).fireChannelRead(req);
213+
verify(ctx, times(8)).writeAndFlush(argThat(new ContextMatcher(HttpResponseStatus.BAD_REQUEST)));
214+
verify(ctx, never()).writeAndFlush(argThat(new ContextMatcher(HttpResponseStatus.UNAUTHORIZED)));
215+
verify(ctx, times(2)).writeAndFlush(argThat(new ContextMatcher(HttpResponseStatus.FORBIDDEN)));
216+
}
217+
218+
@Test
219+
public void testAllCases() throws Exception {
220+
enumerate(hasAcl, hasStore, hasAccess, isSystemStore, isFailOpen, isMetadata, isHealthCheck, isBadUri);
164221

222+
verify(ctx, times(108)).fireChannelRead(req);
223+
verify(ctx, times(144)).writeAndFlush(argThat(new ContextMatcher(HttpResponseStatus.BAD_REQUEST)));
224+
verify(ctx, times(1)).writeAndFlush(argThat(new ContextMatcher(HttpResponseStatus.UNAUTHORIZED)));
165225
// One of the cases is impossible in reality. See StoreAclHandler.java comments
166226
verify(ctx, times(3)).writeAndFlush(argThat(new ContextMatcher(HttpResponseStatus.FORBIDDEN)));
167227
}
@@ -177,26 +237,35 @@ private void update() throws Exception {
177237
when(metadataRepo.getStoreOrThrow(any())).thenThrow(new VeniceNoStoreException("storename"));
178238
}
179239
when(store.isSystemStore()).thenReturn(isSystemStore[0]);
180-
if (isMetadata[0]) {
240+
if (isBadUri[0]) {
241+
when(req.uri()).thenReturn("/badUri");
242+
} else if (isMetadata[0]) {
181243
when(req.uri()).thenReturn("/metadata/storename/random");
244+
} else if (isHealthCheck[0]) {
245+
when(req.uri()).thenReturn("/health");
182246
} else {
183247
when(req.uri()).thenReturn("/storage/storename/random");
184248
}
185249
}
186250

187251
/**
188-
* Generate every possible combination for a given list of booleans
252+
* Generate every possible combination for a given list of booleans based on variables passed
253+
* to boolean[]... conditions. If all variables (8 in count) are passed, then there will be 256
254+
* combinations:
189255
*
190-
* for (int i = 0; i < 32; i++) { | i= 0 1 2 3 4 ...
256+
* for (int i = 0; i < 256; i++) { | i= 0 1 2 3 4 ...
191257
* _hasAccess= (i>>0) % 2 == 1| F T F T F ...
192258
* _hasAcl= (i>>1) % 2 == 1| F F T T F ...
193259
* _hasStore= (i>>2) % 2 == 1| F F F F T ...
194260
* _isAccessControlled= (i>>3) % 2 == 1| F F F F F ...
195261
* _isFailOpen= (i>>4) % 2 == 1| F F F F F ...
196262
* _isMetadata= (i>>5) % 2 == 1| F F F F F ...
263+
* _isHealthCheck= (i>>6) % 2 == 1| F F F F F ...
264+
* _isBadUri= (i>>7) % 2 == 1| F F F F F ...
197265
* }
198266
*/
199267
private void enumerate(boolean[]... conditions) throws Exception {
268+
// enumerate for all possible combinations
200269
int len = conditions.length;
201270
for (int i = 0; i < Math.pow(2, len); i++) {
202271
for (int j = 0; j < len; j++) {
@@ -208,6 +277,9 @@ private void enumerate(boolean[]... conditions) throws Exception {
208277
update();
209278
aclHandler.channelRead0(ctx, req);
210279
}
280+
281+
// reset all supported conditions to the default to remove changes from this test
282+
resetAllConditions();
211283
}
212284

213285
public static class ContextMatcher implements ArgumentMatcher<FullHttpResponse> {

0 commit comments

Comments
 (0)