remove request listener to prevent untimely session invalidation by DaanHoogland · Pull Request #6393 · apache/cloudstack

DaanHoogland · 2022-05-16T15:32:22Z

Description

This PR removes the request handlin from the ApiSessionListener. This causes sessions to be invalidated. API calling from cmk would fail and succeed on and off due to this.

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

manually tested by listing accounts with cloudmonkey.

nvazquez · 2022-05-16T15:47:34Z

@blueorangutan package

blueorangutan · 2022-05-16T15:49:03Z

@nvazquez a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2022-05-16T16:27:08Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 3414

nvazquez · 2022-05-16T16:42:01Z

@blueorangutan test

blueorangutan · 2022-05-16T16:43:03Z

@nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

weizhouapache

verified OK

DaanHoogland · 2022-05-16T19:13:00Z

@weizhouapache @nvazquez I am ok with this as a temporary fix, but it is a functional regression as sessions are no longer properly administrated. We (I) will have to think about a more premanent fix. I actually think the real bug is in the way the ApiServlet handles sessions and not this listener, but they seem to be incompatible :(

blueorangutan · 2022-05-17T03:52:45Z

Trillian test result (tid-4195)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 38984 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6393-t4195-kvm-centos7.zip
Smoke tests completed. 95 look OK, 1 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers	`Failure`	461.57	test_vpc_redundant.py

sureshanaparti

code LGTM

acs-robot · 2022-05-19T08:23:51Z

Found UI changes, kicking a new UI QA build
@blueorangutan ui

blueorangutan · 2022-05-19T08:25:02Z

@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

blueorangutan · 2022-05-19T08:29:55Z

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6393 (SL-JID-1594)

acs-robot · 2022-05-19T09:02:50Z

Found UI changes, kicking a new UI QA build
@blueorangutan ui

blueorangutan · 2022-05-19T09:04:03Z

@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

blueorangutan · 2022-05-19T09:08:41Z

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6393 (SL-JID-1595)

blueorangutan · 2022-05-19T10:58:02Z

@DaanHoogland a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2022-05-19T11:33:54Z

Packaging result: ✔️ el7 ✔️ el8 ✖️ debian ✔️ suse15. SL-JID 3444

acs-robot · 2022-05-19T12:58:31Z

Found UI changes, kicking a new UI QA build
@blueorangutan ui

blueorangutan · 2022-05-19T12:59:03Z

@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

blueorangutan · 2022-05-19T13:04:14Z

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6393 (SL-JID-1598)

DaanHoogland · 2022-05-19T13:24:13Z

@blueorangutan package

blueorangutan · 2022-05-19T13:25:02Z

@DaanHoogland a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2022-05-19T14:00:45Z

Packaging result: ✔️ el7 ✔️ el8 ✖️ debian ✔️ suse15. SL-JID 3446

DaanHoogland · 2022-05-19T14:46:56Z

@blueorangutan package

acs-robot · 2022-05-19T14:47:07Z

Found UI changes, kicking a new UI QA build
@blueorangutan ui

blueorangutan · 2022-05-19T14:48:03Z

@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

blueorangutan · 2022-05-19T14:53:01Z

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6393 (SL-JID-1599)

DaanHoogland · 2022-05-19T15:02:15Z

@blueorangutan package

blueorangutan · 2022-05-19T15:03:03Z

@DaanHoogland a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

DaanHoogland · 2022-05-19T15:06:09Z

tested @nvazquez @weizhouapache ,
both repeated listing of accounts and (with latency) session count works.

blueorangutan · 2022-05-19T15:41:33Z

Packaging result: ✔️ el7 ✔️ el8 ✖️ debian ✔️ suse15. SL-JID 3447

sonarqubecloud · 2022-05-19T15:53:02Z

SonarCloud Quality Gate failed.

0 Bugs
2 Vulnerabilities
0 Security Hotspots
1 Code Smell

0.0% Coverage
0.0% Duplication

yadvr · 2022-05-20T10:13:31Z

-            } else if (HttpUtils.RESPONSE_TYPE_XML.equalsIgnoreCase(responseType)){
-                resp.setContentType(HttpUtils.XML_CONTENT_TYPE);
-            }
+            resp.setContentType(HttpUtils.XML_CONTENT_TYPE);


@DaanHoogland should we force xml?

It wouldn´t matter in the existing code as line 179 (former version 168) https://github.com/apache/cloudstack/pull/6393/files#diff-cc92beff292fdad7835437d622159db6e1ae90d0ebac7fb4ae73c813faa8896eR179 hard-codes the reponseType to HttpUtils.RESPONSE_TYPE_XML. The second branch would always be executed.

yadvr · 2022-05-20T10:16:37Z

Test cases suggested:

Normal login as different account/roles/types
Login/auth and run API using api-key and secret-keys
SAML based SSO login and logout
pre-shared key based running APIs

DaanHoogland · 2022-05-20T11:23:24Z

@blueorangutan test

blueorangutan · 2022-05-20T11:24:02Z

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2022-05-20T23:24:00Z

Trillian test result (tid-4218)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 41939 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6393-t4218-kvm-centos7.zip
Smoke tests completed. 96 look OK, 1 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers	`Failure`	463.57	test_vpc_redundant.py
test_04_rvpc_network_garbage_collector_nics	`Failure`	483.17	test_vpc_redundant.py

yadvr · 2022-05-22T02:16:04Z

@blueorangutan test

blueorangutan · 2022-05-22T02:17:04Z

@rohityadavcloud a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2022-05-22T02:21:47Z

Trillian Build Failed (tid-4227)

nvazquez · 2022-05-24T12:23:23Z

Hi @kohrar can you please verify the login/logout with this PR?

weizhouapache approved these changes May 16, 2022

View reviewed changes

nvazquez added type:bug component:api Severity:BLOCKER labels May 16, 2022

nvazquez added this to the 4.17.0.0 milestone May 16, 2022

nvazquez requested review from Pearl1594, borisstoyanov, vladimirpetrov and yadvr May 16, 2022 19:30

sureshanaparti approved these changes May 17, 2022

View reviewed changes

yadvr approved these changes May 17, 2022

View reviewed changes

yadvr reviewed May 17, 2022

View reviewed changes

Comment thread server/src/main/java/com/cloud/api/ApiSessionListener.java

DaanHoogland added 2 commits May 19, 2022 08:34

login/-out constants

4f00efb

no request listener

2379967

DaanHoogland force-pushed the sessionCountProblems branch from 7d66627 to 2379967 Compare May 19, 2022 08:21

boring-cyborg Bot added the component:saml label May 19, 2022

store session as value, using id as key

97f5aae

new sonar issues

f90ae8a

sonar issues

cdff5f9

nvazquez approved these changes May 19, 2022

View reviewed changes

yadvr reviewed May 20, 2022

View reviewed changes

nvazquez merged commit c123c3f into apache:main May 24, 2022

Conversation

DaanHoogland commented May 16, 2022

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

nvazquez commented May 16, 2022

Uh oh!

blueorangutan commented May 16, 2022

Uh oh!

blueorangutan commented May 16, 2022

Uh oh!

nvazquez commented May 16, 2022

Uh oh!

blueorangutan commented May 16, 2022

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented May 16, 2022

Uh oh!

blueorangutan commented May 17, 2022

Uh oh!

sureshanaparti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

acs-robot commented May 19, 2022

Uh oh!

blueorangutan commented May 19, 2022

Uh oh!

blueorangutan commented May 19, 2022

Uh oh!

acs-robot commented May 19, 2022

Uh oh!

blueorangutan commented May 19, 2022

Uh oh!

blueorangutan commented May 19, 2022

Uh oh!

blueorangutan commented May 19, 2022

Uh oh!

blueorangutan commented May 19, 2022

Uh oh!

acs-robot commented May 19, 2022

Uh oh!

blueorangutan commented May 19, 2022

Uh oh!

blueorangutan commented May 19, 2022

Uh oh!

DaanHoogland commented May 19, 2022

Uh oh!

blueorangutan commented May 19, 2022

Uh oh!

blueorangutan commented May 19, 2022

Uh oh!

DaanHoogland commented May 19, 2022

Uh oh!

acs-robot commented May 19, 2022

Uh oh!

blueorangutan commented May 19, 2022

Uh oh!

blueorangutan commented May 19, 2022

Uh oh!

DaanHoogland commented May 19, 2022

Uh oh!

blueorangutan commented May 19, 2022

Uh oh!

DaanHoogland commented May 19, 2022

Uh oh!

blueorangutan commented May 19, 2022

Uh oh!

sonarqubecloud Bot commented May 19, 2022

Uh oh!

yadvr May 20, 2022

Choose a reason for hiding this comment

Uh oh!