Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-20474

getACL methods are too expensive when current ACL is SYSTEM

      There are many occasions when a long block of code is running as ACL.SYSTEM (generally anything that is not handling a HTTP or CLI request), yet permission checks (done as part of e.g. Jenkins.getAllItems) call many getACL methods on model objects, which in turn ask the AuthorizationStrategy to make a new ACL instance, which can be rather expensive in some cases, and then ask that implementation about SYSTEM, which may actually be a shortcut in the strategy but by that point a lot of work has already been doneā€”all wasted, since SYSTEM must have full permissions regardless of strategy.

      It would be better for core should ensure that Jenkins.getACL and other getACL methods calling Jenkins.getInstance().getAuthorizationStrategy().getACL(this) (AbstractItem, Computer, Job, Node, User, Cloud, View) return a proxy ACL whose hasPermission checks for SYSTEM immediately (returning true in this case), only consulting the AuthorizationStrategy for another Authentication. (The proxy ACL could even be a cached part of the model object, avoiding all object construction in this case.)

          [JENKINS-20474] getACL methods are too expensive when current ACL is SYSTEM

          Jesse Glick added a comment -

          Would solve issues like JENKINS-19623 without the need for plugin changes.

          Jesse Glick added a comment - Would solve issues like JENKINS-19623 without the need for plugin changes.

          Oleg Nenashev added a comment -

          Could be there any use-case, when the ACL.SYSTEM is explicitly used in security settings?

          I've used such hasPermissions() to protect several Groovy scripts from improper usage from a System, but it is a dirty hack in any case.

          Oleg Nenashev added a comment - Could be there any use-case, when the ACL.SYSTEM is explicitly used in security settings? I've used such hasPermissions() to protect several Groovy scripts from improper usage from a System, but it is a dirty hack in any case.

          Jesse Glick added a comment -

          I think there is no valid use case for ACL.hasPermission(SYSTEM, Permission) to return false.

          Jesse Glick added a comment - I think there is no valid use case for ACL.hasPermission(SYSTEM, Permission) to return false.

          Oleg Nenashev added a comment -

          jglick, AFAIK the patch has been applied to the core.
          Could you confirm it?

          Oleg Nenashev added a comment - jglick , AFAIK the patch has been applied to the core. Could you confirm it?

          Jesse Glick added a comment -

          I know of no change to core in this area.

          Jesse Glick added a comment - I know of no change to core in this area.

          Kevin Yu added a comment -

          We are currently experiencing a bottleneck with 1900+ jobs because of this issue. The UI for admin is pretty much un-usable. CPU usage and memory seems fine.

          Kevin Yu added a comment - We are currently experiencing a bottleneck with 1900+ jobs because of this issue. The UI for admin is pretty much un-usable. CPU usage and memory seems fine.

          Daniel Beck added a comment -

          samsun387

          The UI for admin is pretty much un-usable

          Entirely different issue, as you're not logging in as SYSTEM, but as an actual user. File a performance related issue with your chosen authorization strategy plugin (but only if stack traces show it's actually the culprit).

          Daniel Beck added a comment - samsun387 The UI for admin is pretty much un-usable Entirely different issue, as you're not logging in as SYSTEM, but as an actual user. File a performance related issue with your chosen authorization strategy plugin (but only if stack traces show it's actually the culprit).

          Code changed in jenkins
          User: Jesse Glick
          Path:
          core/src/main/java/hudson/security/ACL.java
          core/src/main/java/hudson/security/AccessControlled.java
          test/src/test/java/hudson/security/ACLTest.java
          http://jenkins-ci.org/commit/jenkins/79481041974776746f71c4e5e68304c4cb91c71e
          Log:
          JENKINS-20474 Optimize away ACL construction and AuthorizationStrategy calls when checking permissions for SYSTEM.

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Jesse Glick Path: core/src/main/java/hudson/security/ACL.java core/src/main/java/hudson/security/AccessControlled.java test/src/test/java/hudson/security/ACLTest.java http://jenkins-ci.org/commit/jenkins/79481041974776746f71c4e5e68304c4cb91c71e Log: JENKINS-20474 Optimize away ACL construction and AuthorizationStrategy calls when checking permissions for SYSTEM.

          Code changed in jenkins
          User: Jesse Glick
          Path:
          core/src/main/java/hudson/security/ACL.java
          core/src/main/java/hudson/security/AccessControlled.java
          test/src/test/java/hudson/security/ACLTest.java
          http://jenkins-ci.org/commit/jenkins/969ed923452ca402d027f04f1bad66f9b610d2e9
          Log:
          Merge pull request #3177 from jglick/SYSTEM-JENKINS-20474

          JENKINS-20474 Optimize away ACL construction and AuthorizationStrategy calls when checking permissions for SYSTEM

          Compare: https://github.com/jenkinsci/jenkins/compare/041e9fe1f25f...969ed923452c

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Jesse Glick Path: core/src/main/java/hudson/security/ACL.java core/src/main/java/hudson/security/AccessControlled.java test/src/test/java/hudson/security/ACLTest.java http://jenkins-ci.org/commit/jenkins/969ed923452ca402d027f04f1bad66f9b610d2e9 Log: Merge pull request #3177 from jglick/SYSTEM- JENKINS-20474 JENKINS-20474 Optimize away ACL construction and AuthorizationStrategy calls when checking permissions for SYSTEM Compare: https://github.com/jenkinsci/jenkins/compare/041e9fe1f25f...969ed923452c

            jglick Jesse Glick
            jglick Jesse Glick
            Votes:
            1 Vote for this issue
            Watchers:
            4 Start watching this issue

              Created:
              Updated:
              Resolved: