[router][server] ACL handler efficiency improvement (#1517)

- Re-ordered some operations to minimize ACL handling overhead, based on profiling.

- Changed a HashSet<QueryAction> into an EnumSet.

- Replaced a store repo call from getStore() == null to !hasStore().

internal/venice-common/src/main/java/com/linkedin/venice/acl/handler/AbstractStoreAclHandler.java

@@ -62,7 +62,6 @@ public void channelRead0(ChannelHandlerContext ctx, HttpRequest req) throws SSLP
     }
 
     String uri = req.uri();
-    String method = req.method().name();
 
     // Parse resource type and store name
     String[] requestParts = URI.create(uri).getPath().split("/");
@@ -84,14 +83,14 @@ public void channelRead0(ChannelHandlerContext ctx, HttpRequest req) throws SSLP
 
     // When there is no store present in the metadata repository, pass the ACL check and let the next handler handle the
     // case of deleted or migrated store
-    if (metadataRepository.getStore(storeName) == null) {
+    if (!metadataRepository.hasStore(storeName)) {
       ReferenceCountUtil.retain(req);
       ctx.fireChannelRead(req);
       return;
     }
 
     X509Certificate clientCert = extractClientCert(ctx);
-
+    String method = req.method().name();
     AccessResult accessResult = checkAccess(uri, clientCert, storeName, method);
     switch (accessResult) {
       case GRANTED:
@@ -126,27 +125,27 @@ protected boolean isAccessAlreadyApproved(ChannelHandlerContext ctx) {
    */
   protected abstract REQUEST_TYPE validateRequest(String[] requestParts);
 
+  /**
+   * N.B.: This function is called on the hot path, so it's important to make it as efficient as possible. The order of
+   *       operations is carefully considered so that short-circuiting comes into play as much as possible. We also try
+   *       to minimize the overhead of logging wherever possible (e.g., by minimizing expensive calls, such as the one
+   *       to {@link IdentityParser#parseIdentityFromCert(X509Certificate)}).
+   */
   protected AccessResult checkAccess(String uri, X509Certificate clientCert, String storeName, String method) {
-    if (VeniceSystemStoreUtils.isSystemStore(storeName)) {
-      return AccessResult.GRANTED;
-    }
-
-    String client = identityParser.parseIdentityFromCert(clientCert);
     try {
-      /**
-       * TODO: Consider making this the first check, so that we optimize for the hot path. If rejected, then we
-       *       could check whether the request is for a system store, METADATA, etc.
-       */
       if (accessController.hasAccess(clientCert, storeName, method)) {
         return AccessResult.GRANTED;
       }
 
+      if (VeniceSystemStoreUtils.isSystemStore(storeName)) {
+        return AccessResult.GRANTED;
+      }
+
       // Fact:
       // Request gets rejected.
       // Possible Reasons:
       // A. ACL not found. OR,
       // B. ACL exists but caller does not have permission.
-      String errLine = String.format("%s requested %s %s", client, method, uri);
 
       if (!accessController.isFailOpen() && !accessController.hasAcl(storeName)) { // short circuit, order matters
         // Case A
@@ -160,13 +159,21 @@ protected AccessResult checkAccess(String uri, X509Certificate clientCert, Strin
         // Requested resource exists but does not have ACL.
         // Action:
         // return 401 Unauthorized
-        LOGGER.warn("Requested store does not have ACL: {}", errLine);
-        LOGGER.debug(
-            "Existing stores: {}",
-            () -> metadataRepository.getAllStores().stream().map(Store::getName).sorted().collect(Collectors.toList()));
-        LOGGER.debug(
-            "Access-controlled stores: {}",
-            () -> accessController.getAccessControlledResources().stream().sorted().collect(Collectors.toList()));
+        if (LOGGER.isWarnEnabled()) {
+          LOGGER.warn(
+              "Requested store does not have ACL: {} requested {} {}",
+              identityParser.parseIdentityFromCert(clientCert),
+              method,
+              uri);
+          if (LOGGER.isDebugEnabled()) {
+            LOGGER.debug(
+                "Existing stores: {}",
+                metadataRepository.getAllStores().stream().map(Store::getName).sorted().collect(Collectors.toList()));
+            LOGGER.debug(
+                "Access-controlled stores: {}",
+                accessController.getAccessControlledResources().stream().sorted().collect(Collectors.toList()));
+          }
+        }
         return AccessResult.UNAUTHORIZED;
       } else {
         // Case B
@@ -186,17 +193,35 @@ protected AccessResult checkAccess(String uri, X509Certificate clientCert, Strin
         // Caller does not have permission to access the resource.
         // Action:
         // return 403 Forbidden
-        LOGGER.debug("Unauthorized access rejected: {}", errLine);
+        if (LOGGER.isDebugEnabled()) {
+          LOGGER.debug(
+              "Unauthorized access rejected: {} requested {} {}",
+              identityParser.parseIdentityFromCert(clientCert),
+              method,
+              uri);
+        }
         return AccessResult.FORBIDDEN;
       }
     } catch (AclException e) {
-      String errLine = String.format("%s requested %s %s", client, method, uri);
-
       if (accessController.isFailOpen()) {
-        LOGGER.warn("Exception occurred! Access granted: {} {}", errLine, e);
+        if (LOGGER.isWarnEnabled()) {
+          LOGGER.warn(
+              "Exception occurred! Access granted: {} requested {} {}",
+              identityParser.parseIdentityFromCert(clientCert),
+              method,
+              uri,
+              e);
+        }
         return AccessResult.GRANTED;
       } else {
-        LOGGER.warn("Exception occurred! Access rejected: {} {}", errLine, e);
+        if (LOGGER.isWarnEnabled()) {
+          LOGGER.warn(
+              "Exception occurred! Access rejected: {} requested {} {}",
+              identityParser.parseIdentityFromCert(clientCert),
+              method,
+              uri,
+              e);
+        }
         return AccessResult.ERROR_FORBIDDEN;
       }
     }

services/venice-router/src/test/java/com/linkedin/venice/router/acl/RouterStoreAclHandlerTest.java

@@ -298,9 +298,8 @@ private void validateStringArg(String arg, String argName) {
 
   @Test
   public void testAllRequestTypes() throws SSLPeerUnverifiedException, AclException {
-    Store store = mock(Store.class);
     ReadOnlyStoreRepository metadataRepo = mock(ReadOnlyStoreRepository.class);
-    when(metadataRepo.getStore(storeName)).thenReturn(store);
+    when(metadataRepo.hasStore(storeName)).thenReturn(true);
     HelixReadOnlyStoreConfigRepository storeConfigRepository = mock(HelixReadOnlyStoreConfigRepository.class);
     StoreConfig storeConfig = mock(StoreConfig.class);
     when(storeConfig.getCluster()).thenReturn(clusterName);

services/venice-server/src/main/java/com/linkedin/venice/listener/ServerStoreAclHandler.java

@@ -21,8 +21,7 @@
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.util.Attribute;
 import java.security.cert.X509Certificate;
-import java.util.Arrays;
-import java.util.HashSet;
+import java.util.EnumSet;
 import java.util.Set;
 import java.util.function.Consumer;
 import javax.net.ssl.SSLPeerUnverifiedException;
@@ -45,15 +44,14 @@ public class ServerStoreAclHandler extends AbstractStoreAclHandler<QueryAction>
    *  Skip ACL for requests to /metadata, /admin, /current_version, /health, /topic_partition_ingestion_context and
    *  /host_heartbeat_lag as there's no sensitive information in the response.
    */
-  private static final Set<QueryAction> QUERIES_TO_SKIP_ACL = new HashSet<>(
-      Arrays.asList(
-          QueryAction.METADATA,
-          QueryAction.STORE_PROPERTIES,
-          QueryAction.ADMIN,
-          QueryAction.HEALTH,
-          QueryAction.CURRENT_VERSION,
-          QueryAction.TOPIC_PARTITION_INGESTION_CONTEXT,
-          QueryAction.HOST_HEARTBEAT_LAG));
+  private static final Set<QueryAction> QUERIES_TO_SKIP_ACL = EnumSet.of(
+      QueryAction.METADATA,
+      QueryAction.STORE_PROPERTIES,
+      QueryAction.ADMIN,
+      QueryAction.HEALTH,
+      QueryAction.CURRENT_VERSION,
+      QueryAction.TOPIC_PARTITION_INGESTION_CONTEXT,
+      QueryAction.HOST_HEARTBEAT_LAG);
 
   public ServerStoreAclHandler(
       IdentityParser identityParser,

services/venice-server/src/test/java/com/linkedin/venice/listener/ServerStoreAclHandlerTest.java

@@ -24,7 +24,6 @@
 import com.linkedin.venice.meta.QueryAction;
 import com.linkedin.venice.meta.ReadOnlyStoreRepository;
 import com.linkedin.venice.meta.ServerAdminAction;
-import com.linkedin.venice.meta.Store;
 import com.linkedin.venice.meta.Version;
 import com.linkedin.venice.protocols.VeniceClientRequest;
 import io.grpc.Attributes;
@@ -207,9 +206,8 @@ public void testInterceptor() {
 
   @Test
   public void testAllRequestTypes() throws SSLPeerUnverifiedException, AclException {
-    Store store = mock(Store.class);
     ReadOnlyStoreRepository metadataRepo = mock(ReadOnlyStoreRepository.class);
-    when(metadataRepo.getStore(TEST_STORE_NAME)).thenReturn(store);
+    when(metadataRepo.hasStore(TEST_STORE_NAME)).thenReturn(true);
     ChannelHandlerContext ctx = mock(ChannelHandlerContext.class);
     HttpRequest request = mock(HttpRequest.class);
     Channel channel = mock(Channel.class);