Skip to content

Commit

Permalink
[controller] Fix regression in ACL handling for non-secure admin serv…
Browse files Browse the repository at this point in the history
…er (#1479)

VeniceController refactoring, where the DynamicAccessController (DAC) was inadvertently passed to the  
Admin Server even when SSL was disabled. The presence of a DAC was incorrectly interpreted as ACL  
checks being enabled, causing the Admin Server to enforce ACLs on both secure and non-secure  
channels. This happened because a valid DAC was applied universally to both secure and non-secure  
admin servers.  

The fix ensures that the DynamicAccessController is no longer passed to the Admin Server when using  
a non-secure channel. Additionally, when the NoOpDynamicAccessController is set, it is now explicitly  
treated as a signal that ACL checks are disabled for HTTP requests in the controller. These changes  
restore the expected behavior.
  • Loading branch information
sushantmane authored Jan 28, 2025
1 parent 5dfe33b commit 96dba39
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ AdminSparkServer createAdminServer(boolean secure) {
secure || multiClusterConfigs.isControllerEnforceSSLOnly(),
secure ? multiClusterConfigs.getSslConfig() : Optional.empty(),
secure && multiClusterConfigs.adminCheckReadMethodForKafka(),
accessController,
secure ? accessController : Optional.empty(),
multiClusterConfigs.getDisabledRoutes(),
multiClusterConfigs.getCommonConfig().getJettyConfigOverrides(),
multiClusterConfigs.getCommonConfig().isDisableParentRequestTopicForStreamPushes(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import com.linkedin.venice.acl.AclException;
import com.linkedin.venice.acl.DynamicAccessController;
import com.linkedin.venice.acl.NoOpDynamicAccessController;
import com.linkedin.venice.exceptions.VeniceException;
import java.security.cert.X509Certificate;
import java.util.Optional;
Expand Down Expand Up @@ -164,7 +165,7 @@ protected boolean isAclEnabled() {
/**
* {@link accessController} will be empty if ACL is not enabled.
*/
return accessController.isPresent();
return accessController.isPresent() && !(accessController.get() instanceof NoOpDynamicAccessController);
}

/**
Expand Down

0 comments on commit 96dba39

Please sign in to comment.