- 
                Notifications
    
You must be signed in to change notification settings  - Fork 25.6k
 
[Entitlements] Integrate PluginsLoader with PolicyManager #117239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Entitlements] Integrate PluginsLoader with PolicyManager #117239
Conversation
        
          
                server/src/main/java/org/elasticsearch/plugins/PluginsLoader.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                server/src/main/java/org/elasticsearch/plugins/PluginsLoader.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                server/src/main/java/org/elasticsearch/plugins/PluginsLoader.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      …ate-plugin-loader-with-policy-manager
| 
           Pinging @elastic/es-core-infra (Team:Core/Infra)  | 
    
…ate-plugin-loader-with-policy-manager
        
          
                libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyManager.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      …ate-plugin-loader-with-policy-manager
…ate-plugin-loader-with-policy-manager
…r' of github.com:ldematte/elasticsearch into entitlements/integrate-plugin-loader-with-policy-manager
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good for a first pass. As a followup, I think we need to do more work at initialization time. A lot of the mapping between module and entitlements is lazy and linear here, but it should be very quick hash lookups.
        
          
                ...src/main/java/org/elasticsearch/entitlement/runtime/api/ElasticsearchEntitlementChecker.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | 
               | 
          ||
| public void checkFlagEntitlement(Class<?> callerClass, FlagEntitlementType type) { | ||
| private static List<Entitlement> lookupEntitlementsForModule(Policy policy, String moduleName) { | ||
| for (int i = 0; i < policy.scopes.size(); ++i) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we scanning module names, this could be a map right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The map is lazily created, but it could be done at initialization time instead. I will move that in a follow up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am an unending source of nit picks.
        
          
                libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyManager.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyManager.java
          
            Show resolved
            Hide resolved
        
      
          
 I agree, I will move the lazy map creation to initialization time in a followup today  | 
    
…ate-plugin-loader-with-policy-manager
…r' of github.com:ldematte/elasticsearch into entitlements/integrate-plugin-loader-with-policy-manager
          💚 Backport successful
  | 
    
…7239) This PR expands `PolicyManager` to actually use `Policy` and `Entitlement` classes for checks, instead of hardcoding them. It also introduces a separate `PluginsResolver`, with a dedicated function to map a Class to a Plugin (name). `PluginsResolver` is initialized with data from `PluginsLoader`, and then its resolve function is used internally in `PolicyManager` to find a plugin policy (and then test against the entitlements declared in the policy).
…118055) This PR expands `PolicyManager` to actually use `Policy` and `Entitlement` classes for checks, instead of hardcoding them. It also introduces a separate `PluginsResolver`, with a dedicated function to map a Class to a Plugin (name). `PluginsResolver` is initialized with data from `PluginsLoader`, and then its resolve function is used internally in `PolicyManager` to find a plugin policy (and then test against the entitlements declared in the policy).
This PR expands
PolicyManagerto actually usePolicyandEntitlementclasses for checks, instead of hardcoding them.It also introduces a separate
PluginsResolver, with a dedicated function to map a Class to a Plugin (name).PluginsResolveris initialized with data fromPluginsLoader, and then its resolve function is used internally inPolicyManagerto find a plugin policy (and then test against the entitlements declared in the policy).