Dev - Nexus
  1. Dev - Nexus
  2. NEXUS-5096

Security should cache created WildcardPermission objects, not recreating them over and over again

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0.6, 2.1
    • Component/s: Security
    • Labels:
      None
    • Global Rank:
      27081

      Description

      Problem with security is that it's written in "dry" way. Domain object does not really exists, and (usually Modello generated) configuration object are used directly for actions like lookups and other. Related issue was fixed, but clearly reflected the problem:

      https://github.com/sonatype/security/commit/7176ee1b68c29679091b58054773bd716514fc87

      The configuration objects are suited for persisting, hence, they are not optimized for "runtime". This kind of their use is just wrong.

      What happens is that the model says "permission Z is a string a:b:c", but Security and Shiro "speaks" in WildcardPermission. Hence, whenever a permission is retrieved, what actually happens is new WildcardPermission("a:b:c").

      Obviously, this is not noticeable on small scale, but it does not scale with bigger numbers (count of permissions being fetched from a Realm) very well...

      This affects all Nexus versions.

      Typical case: tested with Nexus 2.1-SNAPSHOT. Created 850 staging repositories under one single profile, and queried for them over REST. Staging REST tries to filter the returned list, by doing security checks. The query was running for 48 minutes (used plain XML realm and user "deployment", as user "admin" is special, and same query for him finished in 2.5seconds!). During these 48 minutes, Yourkit showed that that Nexus spends 97% of CPU time in org.sonatype.security.realms.XmlRolePermissionResolver.resolvePermissionsInRole(String), and JVM allocated 17M object, out of what 16.5M were Shiro's WildcardPermission instances.

        Activity

        Hide
        Tamás Cservenák added a comment -

        Implemented this change, and:

        • runtime of the same query as above changed from 48 minutes to 4.5 minutes.
        • allocation count changed from 17,000,000 to 18,000.

        Beer, anyone?

        Show
        Tamás Cservenák added a comment - Implemented this change, and: runtime of the same query as above changed from 48 minutes to 4.5 minutes. allocation count changed from 17,000,000 to 18,000. Beer, anyone?
        Hide
        Stuart McCulloch added a comment -

        +1

        Show
        Stuart McCulloch added a comment - +1
        Hide
        Tamás Cservenák added a comment -

        Created branch with backported change, ready to be released (and security version in nexus bumped from 2.4 to 2.4.1)
        https://github.com/sonatype/security/tree/security-2.4.x

        Show
        Tamás Cservenák added a comment - Created branch with backported change, ready to be released (and security version in nexus bumped from 2.4 to 2.4.1) https://github.com/sonatype/security/tree/security-2.4.x
        Hide
        Peter Lynch added a comment -

        remove fix version 2.1 since it will first arrive in 2.0.5.1

        Show
        Peter Lynch added a comment - remove fix version 2.1 since it will first arrive in 2.0.5.1
        Hide
        Tamás Cservenák added a comment -

        I disagree with this versioning, Peter.

        IMO, 2.1 and 2.0.5.1 are de facto developed in parallel (actually, the "plan" for 2.0.5.1 was conceived after we did work on 2.1, and is actually a bugfix, so to say, not part of "masterplan"), and I do think that version 2.1 should refer to this issue too (think JIRA's release notes).

        There is no "2.0.5.1 < 2.1", maybe only in Aether version implementation, but in real life, we could release 2.1 and then, on next day 2.0.5.1. nothing stops you doing that. In case like above, while fix would be present (and is notable fix) in 2.1, it would "arrive" later to 2.0.5.1. I'd not "hide" it in a dot-dot-dot release.

        Also, the use of "backport" word hints same thing. Finally, this goes against practice we had so far (see NEXUS-4870, NEXUS-4912, NEXUS-2301 and many others).

        ===

        This is just an opinion, but I do feel that field FixFor should contain both versions (IMO it is kinda "orthogonal" to the timeline as we develop and release), and not only the version we (plan to) release first.

        Show
        Tamás Cservenák added a comment - I disagree with this versioning, Peter. IMO, 2.1 and 2.0.5.1 are de facto developed in parallel (actually, the "plan" for 2.0.5.1 was conceived after we did work on 2.1, and is actually a bugfix, so to say, not part of "masterplan"), and I do think that version 2.1 should refer to this issue too (think JIRA's release notes). There is no "2.0.5.1 < 2.1", maybe only in Aether version implementation, but in real life, we could release 2.1 and then, on next day 2.0.5.1. nothing stops you doing that. In case like above, while fix would be present (and is notable fix) in 2.1, it would "arrive" later to 2.0.5.1 . I'd not "hide" it in a dot-dot-dot release. Also, the use of "backport" word hints same thing. Finally, this goes against practice we had so far (see NEXUS-4870 , NEXUS-4912 , NEXUS-2301 and many others). === This is just an opinion, but I do feel that field FixFor should contain both versions (IMO it is kinda "orthogonal" to the timeline as we develop and release), and not only the version we (plan to) release first.
        Hide
        Peter Lynch added a comment -

        No sweat.

        Show
        Peter Lynch added a comment - No sweat.
        Hide
        Rich Seddon added a comment -

        I've verified that this fix makes a substantial improvement using the customer's configuration. Listing of repositories was timing out, now it finishes in about 10 seconds.

        Show
        Rich Seddon added a comment - I've verified that this fix makes a substantial improvement using the customer's configuration. Listing of repositories was timing out, now it finishes in about 10 seconds.
        Hide
        Rich Seddon added a comment -

        Verified fixed.

        Show
        Rich Seddon added a comment - Verified fixed.

          People

          • Assignee:
            Tamás Cservenák
            Reporter:
            Tamás Cservenák
            Last Updated By:
            Rich Seddon
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:
              Date of First Response: