Make authentication request parameter order to be deterministic #8185

rRajivramachandran · 2023-11-03T16:00:34Z

Description

This PR is to fix a few tests whose results are non deterministic/flaky in the module plugins/network-elements/nicira-nvp.

Setup:

Java version: 11.0.20.1
Maven version: 3.8.8

Test failure reproduction:

The test com.cloud.network.nicira.NiciraRestClientTest#testExecuteLiveLockWhenControllerAllowsLoginAndFollowsWithUnauthorizedButDoesNotRediect assumes the order of keys to be maintained by a HashMap. This issue was verified using the NonDex plugin.

Steps:

git clone https://github.com/apache/cloudstack.git
cd cloudstack
mvn install -pl plugins/network-elements/nicira-nvp -am -DskipTests
mvn -pl plugins/network-elements/nicira-nvp edu.illinois:nondex-maven-plugin:2.1.1:nondex -Dtest=com.cloud.network.nicira.NiciraRestClientTest#testExecuteLiveLockWhenControllerAllowsLoginAndFollowsWithUnauthorizedButDoesNotRediect

Root cause and fix:

Test failure in NonDex mode:

[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 1.767 s <<< FAILURE! - in com.cloud.network.nicira.NiciraRestClientTest
[ERROR] testExecuteLiveLockWhenControllerAllowsLoginAndFollowsWithUnauthorizedButDoesNotRediect(com.cloud.network.nicira.NiciraRestClientTest)  Time elapsed: 1.761 s  <<< ERROR!
java.lang.NullPointerException

The NullPointerException occurs at the following location

cloudstack/plugins/network-elements/nicira-nvp/src/main/java/com/cloud/network/nicira/NiciraRestClient.java

Line 87 in fcbf540

final StatusLine statusLine = response.getStatusLine();

because the mockResponse created in the test, does not get returned for the execute method.

The HttpRequestMatcher serializes the request object to a string

cloudstack/utils/src/test/java/com/cloud/utils/rest/HttpRequestMatcher.java

Line 68 in fcbf540

    
           payload = EntityUtils.toString(((HttpEntityEnclosingRequest) request).getEntity());

To verify that the issue was due to the HttpRequestMatcher used by mockito, the following line:

cloudstack/plugins/network-elements/nicira-nvp/src/test/java/com/cloud/network/nicira/NiciraRestClientTest.java

Line 190 in fcbf540

    
           when(httpClient.execute(eq(HTTP_HOST), HttpRequestMatcher.eq(loginRequest), eq(httpClientContext)))

was modfied to read when(httpClient.execute(eq(HTTP_HOST), any(HttpUriRequest.class), eq(httpClientContext))).

This allows matching any request class object. The test passed with this change when run on the NonDex tool confirming that the issue was here.

On adding logs to the HttpRequestMatcher class, it was deduced that the test fails when the order of keys returned by the login parameters is not the same as that configured in the test class.

Failing case(ordered shuffled):

2023-10-19 18:11:15,312 ERROR [root] (main:) Wanted payload:password=adminpassword&username=admin
2023-10-19 18:11:15,312 ERROR [root] (main:) Actual payload:username=admin&password=adminpassword
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 1.423 s <<< FAILURE! - in com.cloud.network.nicira.NiciraRestClientTest
[ERROR] testExecuteLiveLockWhenControllerAllowsLoginAndFollowsWithUnauthorizedButDoesNotRediect(com.cloud.network.nicira.NiciraRestClientTest)  Time elapsed: 1.413 s  <<< ERROR!
java.lang.NullPointerException

Passing case(order not shuffled):

2023-10-19 18:11:09,513 ERROR [root] (main:) Wanted payload:username=admin&password=adminpassword
2023-10-19 18:11:09,517 ERROR [root] (main:) Actual payload:username=admin&password=adminpassword
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.968 s - in com.cloud.network.nicira.NiciraRestClientTest

The test class configures login parameters here:

cloudstack/plugins/network-elements/nicira-nvp/src/test/java/com/cloud/network/nicira/NiciraRestClientTest.java

Lines 82 to 93 in fcbf540

    
           public static void setupClass() { 
        
               loginParameters.put("username", ADMIN); 
        
               loginParameters.put("password", ADMIN_PASSWORD); 
        
               request = HttpUriRequestBuilder.create() 
        
                   .method(HttpMethods.GET) 
        
                   .path("/path") 
        
                   .build(); 
        
               loginRequest = HttpUriRequestBuilder.create() 
        
                   .method(HttpMethods.POST) 
        
                   .methodParameters(loginParameters) 
        
                   .path(LOGIN_PATH) 
        
                   .build();

and the source code builds them here:

cloudstack/plugins/network-elements/nicira-nvp/src/main/java/com/cloud/network/nicira/NiciraRestClient.java

Lines 122 to 131 in fcbf540

    
           private HttpUriRequest createAuthenticationRequest() { 
        
               final Map<String, String> parameters = new HashMap<>(); 
        
               parameters.put("username", username); 
        
               parameters.put("password", password); 
        
               return HttpUriRequestBuilder.create() 
        
                   .method(HttpMethods.POST) 
        
                   .methodParameters(parameters) 
        
                   .path(loginUrl) 
        
                   .build(); 
        
           }

Fix

To make the map ordered for ensuring deterministic results during serialization/deserialization, changes were done at the source code and test by converting the HashMap to a LinkedHashMap.

Additional fixes

The change also fixed following tests in the same class which were flaky due to the same reason:

com.cloud.network.nicira.NiciraRestClientTest#testExecuteTwoConsecutiveUnauthorizedExecutions
com.cloud.network.nicira.NiciraRestClientTest#testExecuteUnauthorizedThenSuccess

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)
build/CI

Feature/Enhancement Scale or Bug Severity

The fix is backward compatible as it merely changes an unordered map implementation to an ordered one for ensuring that when string serialization of the map is done, results are deterministic.

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

The test passes when run with the NonDex tool(mvn -pl plugins/network-elements/nicira-nvp edu.illinois:nondex-maven-plugin:2.1.1:nondex -Dtest=com.cloud.network.nicira.NiciraRestClientTest#testExecuteLiveLockWhenControllerAllowsLoginAndFollowsWithUnauthorizedButDoesNotRediect).
The entire test suite of the module passes(mvn -pl plugins/network-elements/nicira-nvp test).

How did you try to break this feature and the system with this change?

The change does not affect any feature as it only makes an unordered data structured as ordered. This does not enforce any constraints on source code and is an internal implementation detail. However it makes the code more deterministic in cases where serialization to strings is done directly, which is the case for some tests

DaanHoogland

clgtm, but I think the nicira code needs to be phased out (out of scope for this PR).

rohityadavcloud · 2023-11-03T16:39:32Z

Nicira is likely not support, unmaintained - are you a Nicira user @rRajivramachandran - could you advise if there's a reason around the PR?

rRajivramachandran · 2023-11-03T16:44:29Z

@rohityadavcloud I am not a user of the tool directly. I am a UIUC(University of Illinois Urbana Champaign) student who is working on improving flaky tests in different open source projects. Would it be possible to merge this contribution

shwstppr · 2023-11-04T07:25:59Z

@blueorangutan package

blueorangutan · 2023-11-04T07:30:04Z

@shwstppr a [SL] 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.

codecov · 2023-11-04T07:36:47Z

Codecov Report

Merging #8185 (3b6553a) into main (121531e) will decrease coverage by 0.05%.
Report is 2 commits behind head on main.
The diff coverage is 50.00%.

@@             Coverage Diff              @@
##               main    #8185      +/-   ##
============================================
- Coverage     29.19%   29.15%   -0.05%     
+ Complexity    31015    30970      -45     
============================================
  Files          5181     5181              
  Lines        365255   365255              
  Branches      53427    53427              
============================================
- Hits         106646   106491     -155     
- Misses       243988   244153     +165     
+ Partials      14621    14611      -10

Flag	Coverage Δ
simulator-marvin-tests	`25.09% <0.00%> (-0.07%)`	⬇️
uitests	`4.50% <ø> (ø)`
unit-tests	`14.80% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...ava/com/cloud/network/nicira/NiciraRestClient.java	`82.66% <100.00%> (ø)`
...va/com/cloud/utils/rest/HttpUriRequestBuilder.java	`0.00% <0.00%> (ø)`

... and 33 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

shwstppr · 2023-11-06T07:47:19Z

@blueorangutan package

blueorangutan · 2023-11-06T08:04:10Z

@shwstppr a [SL] 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.

weizhouapache · 2023-11-06T08:43:41Z

Nicira is likely not support, unmaintained - are you a Nicira user @rRajivramachandran - could you advise if there's a reason around the PR?

we could remove the plugin in the near future

this PR is not useful, but we can merge.

DaanHoogland · 2023-11-06T08:53:43Z

Nicira is likely not support, unmaintained - are you a Nicira user @rRajivramachandran - could you advise if there's a reason around the PR?

we could remove the plugin in the near future

this PR is not useful, but we can merge.

If it helps @rRajivramachandran , ... I think this is not bothering anyone else

blueorangutan · 2023-11-06T09:06:31Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7643

…he#8185)

…ic (apache#8185)" This reverts commit a974300.

Make authentication request parameter order to be deterministic

3b6553a

boring-cyborg bot added the component:networking label Nov 3, 2023

DaanHoogland approved these changes Nov 3, 2023

View reviewed changes

rohityadavcloud approved these changes Nov 3, 2023

View reviewed changes

rRajivramachandran mentioned this pull request Nov 3, 2023

Status update TestingResearchIllinois/idoft#908

Merged

DaanHoogland merged commit e9b24b6 into apache:main Nov 6, 2023
24 of 25 checks passed

rRajivramachandran mentioned this pull request Nov 7, 2023

Updated accepted status and remove some tests TestingResearchIllinois/idoft#926

Merged

dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Nov 8, 2023

Make authentication request parameter order to be deterministic (apac…

a974300

…he#8185)

Dajeong-Park added a commit to Dajeong-Park/ablestack-cloud that referenced this pull request Nov 16, 2023

Revert "Make authentication request parameter order to be determinist…

7977324

…ic (apache#8185)" This reverts commit a974300.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make authentication request parameter order to be deterministic #8185

Make authentication request parameter order to be deterministic #8185

rRajivramachandran commented Nov 3, 2023

DaanHoogland left a comment

rohityadavcloud commented Nov 3, 2023

rRajivramachandran commented Nov 3, 2023 •

edited

shwstppr commented Nov 4, 2023

blueorangutan commented Nov 4, 2023

codecov bot commented Nov 4, 2023 •

edited

shwstppr commented Nov 6, 2023

blueorangutan commented Nov 6, 2023

weizhouapache commented Nov 6, 2023

DaanHoogland commented Nov 6, 2023

blueorangutan commented Nov 6, 2023

	public static void setupClass() {
	loginParameters.put("username", ADMIN);
	loginParameters.put("password", ADMIN_PASSWORD);
	request = HttpUriRequestBuilder.create()
	.method(HttpMethods.GET)
	.path("/path")
	.build();
	loginRequest = HttpUriRequestBuilder.create()
	.method(HttpMethods.POST)
	.methodParameters(loginParameters)
	.path(LOGIN_PATH)
	.build();

	private HttpUriRequest createAuthenticationRequest() {
	final Map<String, String> parameters = new HashMap<>();
	parameters.put("username", username);
	parameters.put("password", password);
	return HttpUriRequestBuilder.create()
	.method(HttpMethods.POST)
	.methodParameters(parameters)
	.path(loginUrl)
	.build();
	}

Make authentication request parameter order to be deterministic #8185

Make authentication request parameter order to be deterministic #8185

Conversation

rRajivramachandran commented Nov 3, 2023

Description

Setup:

Test failure reproduction:

Steps:

Root cause and fix:

Fix

Additional fixes

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

DaanHoogland left a comment

Choose a reason for hiding this comment

rohityadavcloud commented Nov 3, 2023

rRajivramachandran commented Nov 3, 2023 • edited

shwstppr commented Nov 4, 2023

blueorangutan commented Nov 4, 2023

codecov bot commented Nov 4, 2023 • edited

Codecov Report

shwstppr commented Nov 6, 2023

blueorangutan commented Nov 6, 2023

weizhouapache commented Nov 6, 2023

DaanHoogland commented Nov 6, 2023

blueorangutan commented Nov 6, 2023

rRajivramachandran commented Nov 3, 2023 •

edited

codecov bot commented Nov 4, 2023 •

edited