Skip to content
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

[JENKINS-58399] Performance: Avoid finding implying permissions on each `hasPermission()` call #83

Merged

Conversation

@AbhyudayaSharma
Copy link
Member

commented Jun 24, 2019

The permissions that a imply a permission are calculated every time. This pull request caches the results in a ConcurrentHashMap.

@AbhyudayaSharma AbhyudayaSharma requested a review from jenkinsci/gsoc2019-role-strategy Jun 24, 2019

@AbhyudayaSharma AbhyudayaSharma self-assigned this Jun 24, 2019

@oleg-nenashev oleg-nenashev self-requested a review Jun 24, 2019

@AbhyudayaSharma

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

Results, compared with #80 again:

image

@AbhyudayaSharma AbhyudayaSharma marked this pull request as ready for review Jun 24, 2019

private static final ConcurrentMap<Permission, Set<Permission>> implyingPermissionCache = new ConcurrentHashMap<>();

static {
Permission.getAll().forEach(RoleMap::cacheImplyingPermissions);

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jun 24, 2019

Member

What if the permissions change during the execution, e.g. by loading a plugin with new permissions?

This comment has been minimized.

Copy link
@AbhyudayaSharma

AbhyudayaSharma Jun 24, 2019

Author Member

If the permission is not in the map, it calculates the implying permissions and stores them back in the map:

private static Set<Permission> getImplyingPermissions(Permission p) {
Set<Permission> implyingPermissions = implyingPermissionCache.get(p);
if (implyingPermissions != null) {
return implyingPermissions;
} else {
return cacheImplyingPermissions(p);
}
}

}
} else if (Settings.TREAT_USER_AUTHORITIES_AS_ROLES) {
try {
UserDetails userDetails = cache.getIfPresent(sid);
if (userDetails == null) {
userDetails = Jenkins.getActiveInstance().getSecurityRealm().loadUserByUsername(sid);
userDetails = Jenkins.getInstance().getSecurityRealm().loadUserByUsername(sid);

This comment has been minimized.

Copy link
@res0nance

res0nance Jun 24, 2019

Contributor

Probably want to use Jenkins.get() here since getInstance() is deprecated and can return null

This comment has been minimized.

Copy link
@AbhyudayaSharma

AbhyudayaSharma Jun 24, 2019

Author Member

Role Strategy uses an old version of Jenkins, so Jenkins.get() is not available to use right now.

This comment has been minimized.

Copy link
@casz

casz Jun 24, 2019

Member

Not the case for the current jenkins.version they use:
https://github.com/jenkinsci/role-strategy-plugin/blob/master/pom.xml#L44

Though perhaps a good time to upgrade

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jul 2, 2019

Member

2.138.x would be fine with me, and it supports the required API

}
private boolean hasPermission(String sid, Permission permission, RoleType roleType, AccessControlled controlledItem) {
final Set<Permission> permissions = getImplyingPermissions(permission);
final boolean[] hasPermission = { false };

This comment has been minimized.

Copy link
@res0nance

res0nance Jun 24, 2019

Contributor

Wondering why hasPermission is an array when only 1 boolean is ever used

This comment has been minimized.

Copy link
@AbhyudayaSharma

AbhyudayaSharma Jun 24, 2019

Author Member

I guess that's just to make the boolean final but still mutable. Seems to come directly from https://stackoverflow.com/a/4520163/6306974

@AbhyudayaSharma AbhyudayaSharma force-pushed the AbhyudayaSharma:implying-permission-cache branch 2 times, most recently from ea9a564 to 6b4b79d Jun 24, 2019

@AbhyudayaSharma AbhyudayaSharma force-pushed the AbhyudayaSharma:implying-permission-cache branch from 6b4b79d to ae93e1e Jun 25, 2019

@AbhyudayaSharma
Copy link
Member Author

left a comment

The cache needed to be invalidated when the dangerous permissions changed. I've made some changes to allow us to respond to changes in DangerousPermissionHandlingMode.

* @param mode the {@code DangerousPermissionHandlingMode} to be set
*/
@Restricted(NoExternalUse.class)
public static void setCURRENT(@Nonnull DangerousPermissionHandlingMode mode) {

This comment has been minimized.

Copy link
@AbhyudayaSharma

AbhyudayaSharma Jun 25, 2019

Author Member

Had to use uppercase CURRENT to allow compatibility from the following Groovy script:

import org.jenkinsci.plugins.rolestrategy.permissions.*

DangerousPermissionHandlingMode.CURRENT = DangerousPermissionHandlingMode.UNDEFINED
@SuppressFBWarnings(value = "MS_SHOULD_BE_REFACTORED_TO_BE_FINAL", justification = "Groovy script console access")
public static /* allow script access */ DangerousPermissionHandlingMode CURRENT;
private static DangerousPermissionHandlingMode CURRENT;

This comment has been minimized.

Copy link
@AbhyudayaSharma

AbhyudayaSharma Jun 25, 2019

Author Member

Made it private. Now the access is through getters and setters allowing us to have hooks when this is changed and invalidate dangerous permissions from the cache.

@AbhyudayaSharma

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2019

Wow, now #81 has been merged, the results are looking much better:

image

(the comparison is with #80 again)

@casz

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Wow that is some seriously awesome benchmark improvements

@oleg-nenashev oleg-nenashev self-requested a review Jul 2, 2019

@oleg-nenashev
Copy link
Member

left a comment

i do not see anything wrong with this pull request. This is a pretty complicated area, and there is a high risk of regressions which may cause security issues. My suggestion would be to make this caching optional and configurable in Role Strategy settings

@@ -79,6 +83,12 @@
private final SortedMap <Role,Set<String>> grantedRoles;

private static final Logger LOGGER = Logger.getLogger(RoleMap.class.getName());

private static final ConcurrentMap<Permission, Set<Permission>> implyingPermissionCache = new ConcurrentHashMap<>();

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jul 2, 2019

Member

It causes interlocks between RoleMap instances, and this cache may also roll over in tests since it is a static field (Maven Surefire plugin runs tests in the same Java context). Is it an intended behavior?

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

https://github.com/jenkinsci/extended-read-permission-plugin/blob/master/src/main/java/hudson/plugins/extendedread/PluginImpl.java#L32 for example of permissions management in runtime. If a system groovy script disables permissions in such way, caching will not reflect it.

w.r.t. caching. It can be either opt-in or opt-out. CC @jenkinsci/gsoc2019-role-strategy

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

Also notes for reviewers. https://javadoc.jenkins.io/hudson/security/Permission.html is final, and hence permission implementations cannot define the impliedBy behavior in runtime unless they use reflection. So the static caching should be fine unless we use Groovy magic or whatever.

CC Jenkins Security team, because they might be able to comment on the risks we might be missing. @daniel-beck @Wadeck @jeffret-b @jvz

@oleg-nenashev
Copy link
Member

left a comment

Recording my approval just to make it explicit. Cache invalidation calls can be delivered in a separate PR

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

My plan is to integrate it on Thursday if no negative feedback

@oleg-nenashev oleg-nenashev changed the title Avoid finding implying permissions on each `hasPermission` call Performance: Avoid finding implying permissions on each `hasPermission()` call Jul 9, 2019

@AbhyudayaSharma AbhyudayaSharma changed the title Performance: Avoid finding implying permissions on each `hasPermission()` call [JENKINS-58399] Performance: Avoid finding implying permissions on each `hasPermission()` call Jul 9, 2019

@jeffret-b
Copy link

left a comment

It's basically trading off memory space and a little bit of complexity for performance, which is what a cache does. There's always a risk of problems with increased complexity. The performance improvement is good value for the tradeoff, though. I can't find any concerns.

I've built the PR and run the autotests, but haven't done any further testing or confirmation of performance gains.

Nice improvement.

@oleg-nenashev oleg-nenashev merged commit f410ae3 into jenkinsci:master Jul 11, 2019

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

Thanks @AbhyudayaSharma !

@AbhyudayaSharma AbhyudayaSharma deleted the AbhyudayaSharma:implying-permission-cache branch Jul 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.