client: fix for jetty session timeout by marcaurele · Pull Request #3658 · apache/cloudstack · GitHub

Description

Related to #2329 (comment) the session timeout is not taken into account. This change creates a session handler explicitly and the timeout value should be correctly taken into account.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

How Has This Been Tested?

This has not been tested yet.

thx! let me test that

@blueorangutan package

@andrijapanicsb a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-334

@blueorangutan test matrix

@andrijapanicsb a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests

Hi @marcaurele thanks for the changes. I tested this but unfortunately I couldn't see any difference after changing the value on server.properties and restarting the management server

Trillian test result (tid-438)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34872 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3658-t438-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 77 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

Trillian test result (tid-439)
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 35868 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3658-t439-vmware-65u2.zip
Smoke tests completed. 77 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

Unfortunately, not working @marcaurele 😢

I did some testing on a simple server and I had to set the timeout after the server was started. I pushed an update, let me know if it's working now @andrijapanicsb

thx @marcaurele.

@blueorangutan package

@andrijapanicsb a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-335

Thanks @marcaurele, this is working now but I think it can be improved a bit:

Instead of creating a pair to pass the sessionHandler, it can be defined as a variable and set the timeout after the server starts, reducing the lines of code:

private SessionHandler sessionHandler;

...

private HandlerCollection createHandlers() {
   ...
   sessionHandler = webApp.getSessionHandler();
   ...
}

public void start() {
   ...
   server.start();
   // Must set the session timeout after the server has started
   sessionHandler.setMaxInactiveInterval(sessionTimeout * 60);
   ...
}

@marcaurele @nvazquez - I've tested both your solutions - it works fine! (tried both setting session to 2minutes and 45 minutes) - so all good!

@marcaurele please check PR #3663

@nvazquez IMO it does not look well coded to keep a reference of the SessionHandler as a class member to only skip having to return this pair of objects.
Then, I don't understand why you open a new PR to perform those changes? Is that so urgent? Aren't your comment enough?

@marcaurele no urgency, was trying to propose an improvement and tried with the comments but thought those were not being added until I click Submit on the review. So I created the PR for clarity but didn't realise the comments were already added, please discard them. Thanks for the fix

Guys, please let me know once you have agreed on the way forward, so I can merge this one.

I have closed the other PR, I'm ok with this one

@nvazquez I wish we could return multiple argument without creating a custom class in Java. Thanks for taking my point in consideration.

Rebase done to only leave the good commit. Good for review & merge on my side.

@blueorangutan package

@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-339

@blueorangutan test matrix

@nvazquez a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests

Trillian test result (tid-452)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 42011 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3658-t452-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 77 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

the latest code is NOT ok.
I've set timeout to 2 minutes and I don't get logged out on 2m30sec (nor on 3m30sec nor on 4m15sec - tested just in case...)

@marcaurele can you try https://stackoverflow.com/a/56091476

@andrijapanicsb weird as I don't see any difference so far from the previous 2 commits. Can you double check you're using the right branch and cleared up the compiled classes of the client module, just in case.

@rhtyd that's what it's doing, but I found out that the session must be set after doing the server.start(). I modified the jetty example embbeded project to try out.

my bad, give me 2 minutes...

all good @marcaurele.

LGTM from my side (manual testing).

LGTM

merging based on 2 LGTM, manual testing and automated test results.

thx @marcaurele !

For the record, this is the small embedded app to validate: https://github.com/marcaurele/embedded-jetty-uber-jar/tree/session-test

fixed inability to set a custom session duration via server.properties on mgmt server.

* 4.13: only update powerstate if sure it is the latest (#3743) ui: fix migrate host form no host popup (#3682) client: jetty session timeout set after server is started (#3658) Increase DHCP lease time to infinite (#3662)

fixed inability to set a custom session duration via server.properties on mgmt server.

marcaurele added the component:management-server label Oct 31, 2019

marcaurele requested a review from andrijapanicsb October 31, 2019 17:14

marcaurele self-assigned this Oct 31, 2019

nvazquez reviewed Nov 1, 2019

View reviewed changes

andrijapanicsb requested changes Nov 1, 2019

View reviewed changes

svenvogel added the type:bug label Nov 1, 2019

nvazquez requested changes Nov 4, 2019

View reviewed changes