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

Ehcache 3 ticket registry and Ehcache 3 used in X509 CRL cache #4488

Merged
merged 33 commits into from Dec 5, 2019

Conversation

@hdeadman
Copy link
Member

hdeadman commented Nov 27, 2019

This is adds an Ehcache 3 ticket registry that can be used standalone or using Terracotta for a clustered cache. I don't have experience with Terracotta so I don't really know what configuration options are appropriate. The existing ehcache 2 registry should be able to be used in an app that also uses the x509 module with an ehcache 3 based cache.

To do:

  • test if service ticket created on one cas server available quickly from terracotta cache accessed from second cas server
@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 28, 2019

Codecov Report

Merging #4488 into master will decrease coverage by 23.16%.
The diff coverage is 24.24%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master    #4488       +/-   ##
=============================================
- Coverage     50.32%   27.16%   -23.17%     
+ Complexity     8363     4292     -4071     
=============================================
  Files          2584     2588        +4     
  Lines         52673    52853      +180     
  Branches       4194     4213       +19     
=============================================
- Hits          26508    14357    -12151     
- Misses        24184    37380    +13196     
+ Partials       1981     1116      -865
Flag Coverage Δ Complexity Δ
#AmazonWebServices 7.9% <15.15%> (+0.02%) 967 <2> (+1) ⬆️
#Cassandra 8.09% <15.15%> (+0.07%) 1067 <2> (+9) ⬆️
#CosmosDb 0% <0%> (ø) 0 <0> (ø) ⬇️
#CouchDb ? ?
#Couchbase ? ?
#DynamoDb 9.46% <15.15%> (+0.03%) 1277 <2> (+3) ⬆️
#FileSystem 9.15% <15.15%> (ø) 1299 <2> (-4) ⬇️
#Groovy 12.1% <15.15%> (ø) 1839 <2> (ø) ⬇️
#Ignite 7.16% <15.15%> (+0.03%) 893 <2> (+1) ⬆️
#InfluxDb 4.66% <15.15%> (+0.04%) 366 <2> (+2) ⬆️
#JDBC 12.83% <15.15%> (-87.17%) 1998 <2> (+1998)
#JMS ? ?
#LDAP 12.23% <24.24%> (-87.77%) 1710 <4> (+1710)
#MAIL 10.37% <15.15%> (-89.63%) 1483 <2> (+1483)
#MSSQL ? ?
#MariaDb ? ?
#Memcached ? ?
#MongoDb ? ?
#MySQL ? ?
#OAUTH ? ?
#OAUTHUMA ? ?
#OIDC ? ?
#Oracle ? ?
#PostgreSQL ? ?
#REST 14.47% <15.15%> (-0.01%) 2230 <2> (-2)
#Radius 7.34% <15.15%> (+0.02%) 937 <2> (+1) ⬆️
#Redis ? ?
#Simple ? ?
#ZooKeeper ? ?
Impacted Files Coverage Δ Complexity Δ
...as/monitor/config/EhCacheMonitorConfiguration.java 0% <ø> (-100%) 0 <0> (-2)
...cacheTicketRegistryTicketCatalogConfiguration.java 0% <ø> (-100%) 0 <0> (-1)
...java/org/apereo/cas/monitor/EhCacheStatistics.java 0% <ø> (-40%) 0 <0> (-6)
...org/apereo/cas/monitor/EhCacheHealthIndicator.java 0% <ø> (-100%) 0 <0> (-4)
...ache3TicketRegistryTicketCatalogConfiguration.java 0% <0%> (ø) 0 <0> (?)
...eo/cas/ticket/registry/EhCache3TicketRegistry.java 0% <0%> (ø) 0 <0> (?)
...cas/config/EhcacheTicketRegistryConfiguration.java 0% <0%> (-76.93%) 0 <0> (-10)
...reo/cas/ticket/registry/EhCacheTicketRegistry.java 0% <0%> (-87.5%) 0 <0> (-20)
...as/config/Ehcache3TicketRegistryConfiguration.java 0% <0%> (ø) 0 <0> (?)
...ation/model/support/ehcache/EhcacheProperties.java 100% <100%> (ø) 1 <1> (ø) ⬇️
... and 1174 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95c2a25...5576fe6. Read the comment docs.

@hdeadman hdeadman force-pushed the ehcache3 branch from bcbe9f8 to f874bf4 Nov 30, 2019
@hdeadman

This comment has been minimized.

Copy link
Member Author

hdeadman commented Dec 1, 2019

I am ready for feedback and plan to do some testing in a dev environment using a snapshot after this is merged. I am currently using the ehcache 2 library and I plan to use a deployment with both the old and new ehcache modules built-in and use the enabled property to toggle back and forth. I purposely didn't support configuration via an ehcache.xml but that would be an option if there was a good reason to so and that reason couldn't be supported with additional property support.

hdeadman added 2 commits Dec 1, 2019
Copy link
Member

mmoayyed left a comment

Outstanding! Comments follow.

.travis.yml Outdated Show resolved Hide resolved
ci/tests/ehcache3/run-tests-ehcache3.sh Outdated Show resolved Hide resolved
.create(Duration.ofSeconds(storageTimeout))
.access(Duration.ofSeconds(storageTimeout))
.update(Duration.ofSeconds(storageTimeout)).build();
Comment on lines 128 to 130

This comment has been minimized.

Copy link
@mmoayyed

mmoayyed Dec 1, 2019

Member

This is interesting. How do we handle idle timeouts here, and specially those that might be different from hard-timeouts? Can this handle an expiration policy to say "2 hours of idleness with a 8-hour hard timeout" ? I suspect you might need to design your own ExpiryPolicy of some kind to handle and separate create timeouts from update/access timeouts.

This comment has been minimized.

Copy link
@hdeadman

hdeadman Dec 3, 2019

Author Member

I changed this to be the built-in "timeToIdleExpiration". This post https://stackoverflow.com/questions/43306560/changing-the-ttl-of-individual-entries-in-ehcache-3?rq=1 talks about differences between ehcache2 and 3 with regards to expiration. The ehcache 2 idle timeouts were set to the same thing as the overall timeout and I would think CAS's built-in registry cleaner would kick in to time things out if they are supposed to timeout, is that not the case?

I tried to address all your other feedback and it looks like all tests passed on latest travis run. Let me know if there are more issues.

This comment has been minimized.

Copy link
@mmoayyed

mmoayyed Dec 4, 2019

Member

Thank you. I'll review.

The ehcache 2 idle timeouts were set to the same thing as the overall timeout and I would think CAS's built-in registry cleaner would kick in to time things out if they are supposed to timeout, is that not the case?

That's correct, yes. I was hoping the registry would be able to clean up after itself removing the need for the cleaner to be turned on. If you set up a policy with an 8-hour hard timeout and a 2-hour idle timeout, it would be easier for the registry to automatically remove/expire the item after 2 hours, rather than having the cleaner pull the registry, calculate expiration periods and remove stuff. The operations to fetch available-tickets from ehcache and remove those that are invalidated by idle timeouts generally tends to cause massive heap surges and spikes, specially over a cluster, unless v3 changes things in that area to improve performance. (i.e. this is how hazelcast works)

It's fine if v3 does the same thing as v2 and we are keeping the same behavior anyway; this is something we can re-iterate and tune later.

This comment has been minimized.

Copy link
@mmoayyed

mmoayyed Dec 4, 2019

Member

This all looks great to me; the only thing that I am not sure about is, I think the call here should change to timeToLiveExpiration() instead. The TTL is what you'd want. The storage-timeout you get from the catalog is typically the hard timeout of a ticket expiration policy, which should be equal to the lifetime/ttl of an item. Effectively, this should be "CAS says tokens should be kept for 8 hours; ehcache keeps tickets alive for 8 hours"...and then we'd use a cleaner to handle idle timeouts as a separate process. Using storageTimeout values for timeToIdleExpiration() seems like a mismatch.

apereocas-bot and others added 4 commits Dec 2, 2019
combine ehcache and ehcache3 docs
combine ehcache and ehcache3 tests under same tag
deprecate ehcache 2.x classes
@hdeadman hdeadman closed this Dec 2, 2019
@hdeadman hdeadman deleted the ehcache3 branch Dec 2, 2019
@hdeadman hdeadman restored the ehcache3 branch Dec 2, 2019
@hdeadman hdeadman reopened this Dec 2, 2019
hdeadman and others added 4 commits Dec 3, 2019
@apereo apereo deleted a comment from Detetivepro1 Dec 4, 2019
@mmoayyed mmoayyed merged commit 5471de4 into master Dec 5, 2019
3 of 5 checks passed
3 of 5 checks passed
Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Summary 2 potential rules
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details
@mmoayyed mmoayyed deleted the ehcache3 branch Dec 5, 2019
@mmoayyed

This comment has been minimized.

Copy link
Member

mmoayyed commented Dec 5, 2019

Excellent work. Thanks Hal.

mmoayyed pushed a commit that referenced this pull request Dec 5, 2019
…ket registry and Ehcache 3 used in X509 CRL cache (#4488)

* cleanup duplicate method call

* wip - ehcache 3 ticket registry

* rename method from getAllExpired to getAllUnexpired

* wip - ehcache3 updates

* wip - ehcache3 updates

* wip  - connect terracott aserver working.

* ehcache3 - add tests to ci, make registry configurable (enabled or not) by property

* turn on logging for tests

* change x509 crl cache to use ehcache3

* checkstyle

* remove duplicate dependency

* fix compile error

* update javadoc

* update ehcache 2 documentation with working manual peer discovery config

* checkstyle

* have ticket registry use existing caches if they exist

* fix path

* fix linefeeds and exclude ehcache tests from simple

* switch to use jsr107 cachemanager so spring cache manager can wrap it

* update documentation and make timeouts configurable

* checkstyle and documentation link fix

* fix stack overflow by calling wrong getTicket method

* checkstyle

* address feedback

combine ehcache and ehcache3 docs
combine ehcache and ehcache3 tests under same tag
deprecate ehcache 2.x classes

* checkstyle

* checkstyle

* make scripts executable

* fix ci script path

* change to TTL expiration policy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.