[controller][server] Allow current_version request server endpoint in…

… acl handler (linkedin#847) * [controller][server] Allow current_version server endpoint in acl handler Server metadata fetch backup version deletion calls server current version endpoint to validate the backup version. But in server Acl handler check fails for QueryAction.CURRENT_VERSION as its not authorized such request. This PR allows such metadata calls in server. --------- Co-authored-by: Sourav Maji <[email protected]>
m-nagarajan · Feb 6, 2024 · f3ce088 · f3ce088
1 parent 35188a3
commit f3ce088
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 6 deletions.
diff --git a/internal/venice-common/src/main/java/com/linkedin/venice/acl/handler/StoreAclHandler.java b/internal/venice-common/src/main/java/com/linkedin/venice/acl/handler/StoreAclHandler.java
@@ -109,10 +109,10 @@ public void channelRead0(ChannelHandlerContext ctx, HttpRequest req) throws SSLP
     }
 
     /**
-     *  Skip ACL for requests to /metadata, /admin and /health as there's no sensitive information in the response.
+     *  Skip ACL for requests to /metadata, /admin /current_version and /health as there's no sensitive information in the response.
      */
-    Set<QueryAction> queriesToSkipAcl =
-        new HashSet<>(Arrays.asList(QueryAction.METADATA, QueryAction.ADMIN, QueryAction.HEALTH));
+    Set<QueryAction> queriesToSkipAcl = new HashSet<>(
+        Arrays.asList(QueryAction.METADATA, QueryAction.ADMIN, QueryAction.HEALTH, QueryAction.CURRENT_VERSION));
     try {
       QueryAction queryAction = QueryAction.valueOf(requestParts[1].toUpperCase());
       if (queriesToSkipAcl.contains(queryAction)) {

diff --git a/internal/venice-common/src/test/java/com/linkedin/venice/acl/StoreAclHandlerTest.java b/internal/venice-common/src/test/java/com/linkedin/venice/acl/StoreAclHandlerTest.java
@@ -46,6 +46,9 @@ public class StoreAclHandlerTest {
   private boolean[] isSystemStore = { false };
   private boolean[] isFailOpen = { false };
   private boolean[] isMetadata = { false };
+
+  private boolean[] isCurrentVersion = { false };
+
   private boolean[] isHealthCheck = { false };
   private boolean[] isBadUri = { false };
 
@@ -60,6 +63,7 @@ private void resetAllConditions() {
     isMetadata[0] = false;
     isHealthCheck[0] = false;
     isBadUri[0] = false;
+    isCurrentVersion[0] = false;
   }
 
   @BeforeMethod
@@ -178,6 +182,16 @@ public void aclDisabledForMetadataEndpoint() throws Exception {
     verify(ctx, times(32)).fireChannelRead(req);
   }
 
+  @Test
+  public void aclDisabledForCurrentVersionEndpoint() throws Exception {
+    isCurrentVersion[0] = true;
+    enumerate(hasAccess, hasAcl, isSystemStore, isFailOpen, isHealthCheck);
+
+    verify(ctx, never()).writeAndFlush(any());
+    // No access control (CURRENT_VERSION) => 32 times
+    verify(ctx, times(32)).fireChannelRead(req);
+  }
+
   @Test
   public void aclDisabledForHealthCheckEndpoint() throws Exception {
     isHealthCheck[0] = true;
@@ -255,6 +269,8 @@ private void update() throws Exception {
       when(req.uri()).thenReturn("/badUri");
     } else if (isMetadata[0]) {
       when(req.uri()).thenReturn(String.format("/metadata/%s/random", storeNameInRequest));
+    } else if (isCurrentVersion[0]) {
+      when(req.uri()).thenReturn("/current_version/storename/random");
     } else if (isHealthCheck[0]) {
       when(req.uri()).thenReturn("/health");
     } else {

diff --git a/...roller/src/main/java/com/linkedin/venice/controller/StoreBackupVersionCleanupService.java b/...roller/src/main/java/com/linkedin/venice/controller/StoreBackupVersionCleanupService.java
@@ -151,7 +151,7 @@ private boolean validateAllRouterOnCurrentVersion(Store store, String clusterNam
     for (Instance routerInstance: liveRouterInstances) {
       try {
         HttpGet routerRequest =
-            new HttpGet(routerInstance.getHostUrl(true) + "/" + TYPE_CURRENT_VERSION + "/" + store.getName());
+            new HttpGet(routerInstance.getHostUrl(true) + TYPE_CURRENT_VERSION + "/" + store.getName());
         HttpResponse response = getHttpAsyncClient().execute(routerRequest, null).get(500, TimeUnit.MILLISECONDS);
         if (response.getStatusLine().getStatusCode() != HttpStatus.SC_OK) {
           LOGGER.warn(
@@ -188,8 +188,7 @@ private boolean validateAllServerOnCurrentVersion(Store store, String clusterNam
     for (Instance instance: instances) {
       try {
         HttpGet serverRequest = new HttpGet(
-            instance.getHostUrl(true) + "/" + QueryAction.CURRENT_VERSION.toString().toLowerCase() + "/"
-                + store.getName());
+            instance.getHostUrl(true) + QueryAction.CURRENT_VERSION.toString().toLowerCase() + "/" + store.getName());
         HttpResponse response = getHttpAsyncClient().execute(serverRequest, null).get(500, TimeUnit.MILLISECONDS);
         if (response.getStatusLine().getStatusCode() != HttpStatus.SC_OK) {
           LOGGER.warn(