api: Warn if query parameters have multiple values by davidjumani · Pull Request #5009 · apache/cloudstack

davidjumani · 2021-05-10T11:56:59Z

Description

When duplicate query parameters are passed with the same key but different values, ACS parses only the last occurrence.
Eg: keyword=rick&keyword=basic
It returns values matching basic

This PR fixes it so it throws an error when multiple values for the same key is sent

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)

Bug Severity

How Has This Been Tested?

http://localhost:8080/client/api/?command=listVirtualMachines&keyword=rick&keyword=basic

WARN  [c.c.a.ApiServlet] (qtp875678495-281:ctx-f7a451f1) (logid:742779bd) Query parameter 'keyword' has multiple values [rick, basic]

davidjumani · 2021-05-10T12:01:56Z

@blueorangutan package

blueorangutan · 2021-05-10T12:03:02Z

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

blueorangutan · 2021-05-10T12:36:05Z

Packaging result: ✖️ centos7 ✖️ centos8 ✖️ debian. SL-JID 557

davidjumani · 2021-05-10T12:40:19Z

@blueorangutan package

blueorangutan · 2021-05-10T12:41:02Z

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

blueorangutan · 2021-05-10T13:11:12Z

Packaging result: ✖️ centos7 ✖️ centos8 ✖️ debian. SL-JID 558

DaanHoogland · 2021-05-10T14:16:22Z

+                        String message = String.format("Query parameter '%s' has multiple values", param.getName());
+                        s_logger.error(message);
+                        throw new ServerApiException(ApiErrorCode.MALFORMED_PARAMETER_ERROR, message);


this is a repeating bit (also happening in ApuServlet) maybe we can move (parts of) it to a common utility?

blueorangutan · 2021-05-10T20:07:22Z

Packaging result: ✔️ centos7 ✖️ centos8 ✔️ debian. SL-JID 564

davidjumani · 2021-05-11T03:38:46Z

@blueorangutan test

blueorangutan · 2021-05-11T03:40:03Z

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

blueorangutan · 2021-05-11T20:44:17Z

Trillian test result (tid-646)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 59837 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5009-t646-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_diagnostics.py
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_network.py
Intermittent failure detected: /marvin/tests/smoke/test_router_dns.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_router_nics.py
Smoke tests completed. 83 look OK, 4 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_invalid_upgrade_kubernetes_cluster	`Failure`	3611.06	test_kubernetes_clusters.py
test_02_deploy_and_upgrade_kubernetes_cluster	`Failure`	3605.16	test_kubernetes_clusters.py
test_03_deploy_and_scale_kubernetes_cluster	`Failure`	0.04	test_kubernetes_clusters.py
test_04_basic_lifecycle_kubernetes_cluster	`Failure`	0.05	test_kubernetes_clusters.py
test_05_delete_kubernetes_cluster	`Failure`	0.04	test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster	`Failure`	0.05	test_kubernetes_clusters.py
test_08_deploy_and_upgrade_kubernetes_ha_cluster	`Failure`	0.04	test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster	`Failure`	0.04	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	89.94	test_kubernetes_clusters.py
test_router_dns_guestipquery	`Failure`	613.66	test_router_dns.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL	`Failure`	485.13	test_vpc_redundant.py
test_01_VPC_nics_after_destroy	`Failure`	933.88	test_vpc_router_nics.py

GutoVeronezi · 2021-06-01T13:15:06Z

@davidjumani although you link the issue, could you improve the description of this PR with more context?

yadvr · 2021-08-31T09:00:48Z

@davidjumani I'm not sure if it may cause any regression, since we're tight for a RC this or early next week, I'm moving the milestone.

davidjumani · 2022-02-25T05:44:01Z

@blueorangutan package

blueorangutan · 2022-02-25T05:47:02Z

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

blueorangutan · 2022-02-25T06:21:45Z

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

davidjumani · 2022-02-25T06:22:19Z

@blueorangutan test

blueorangutan · 2022-02-25T06:23:03Z

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

blueorangutan · 2022-02-25T16:50:55Z

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

Test	Result	Time (s)	Test File

weizhouapache · 2022-03-02T09:27:56Z

this might cause backwards compatibility issue. I suggest to hold on
cc @nvazquez @DaanHoogland @davidjumani

DaanHoogland · 2022-03-02T11:02:20Z

this might cause backwards compatibility issue. I suggest to hold on cc @nvazquez @DaanHoogland @davidjumani

what kind of issues, @weizhouapache ?

weizhouapache · 2022-03-02T12:16:44Z

this might cause backwards compatibility issue. I suggest to hold on cc @nvazquez @DaanHoogland @davidjumani

what kind of issues, @weizhouapache ?

@DaanHoogland
I think we should allow duplicated query parameters. and display a warning not throw an error if possible.
some users might pass duplicated parameters in scripts and etc, it worked in the past, but with this PR it won't work any more.

davidjumani · 2022-03-04T05:45:20Z

@weizhouapache I've updated it to a warning and to not throw an error

davidjumani · 2022-03-04T05:45:30Z

@blueorangutan package

blueorangutan · 2022-03-04T05:46:03Z

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

blueorangutan · 2022-03-04T06:21:22Z

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

davidjumani · 2022-03-04T07:02:17Z

@blueorangutan test

blueorangutan · 2022-03-04T07:03:03Z

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

weizhouapache

code lgtm

nvazquez · 2022-03-06T11:45:15Z

@blueorangutan test

blueorangutan · 2022-03-06T11:46:02Z

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

blueorangutan · 2022-03-06T20:47:59Z

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

Test	Result	Time (s)	Test File

davidjumani · 2022-03-08T07:01:13Z

@blueorangutan package

blueorangutan · 2022-03-08T07:02:04Z

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

blueorangutan · 2022-03-08T07:37:19Z

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

davidjumani · 2022-03-08T08:12:33Z

Smoke tests won't be needed as they passed the previous time and only cosmetic changes were made

nvazquez · 2022-03-08T13:46:33Z

@davidjumani I've manually tested and looks good, I have some minor improvement request:

If passing multiples values for a parameter we use the last, however if I attempt the same on cmk, cmk only passes the first parameter. Can we change the value used to be the first instead of the last so they are consistent?

DaanHoogland · 2022-03-08T14:58:32Z

+                        String message = String.format("Query parameter '%s' has multiple values [%s, %s]. Only the last value will be respected." +
+                            "It is advised to pass only a single parameter", param.getName(), param.getValue(), parameterMap.get(param.getName()));
+                        s_logger.warn(message);


is there a joint ApiServer/ApiServlet Utility to put this warning message in, instead of copying the exact same text?
Alternatively can we change the text to indicate where/why we object to the duplicate parameter?

davidjumani · 2022-03-08T15:07:31Z

@davidjumani I've manually tested and looks good, I have some minor improvement request:

If passing multiples values for a parameter we use the last, however if I attempt the same on cmk, cmk only passes the first parameter. Can we change the value used to be the first instead of the last so they are consistent?

This is a cmk specific implementation. If an API is called via a browser or tool which simply creates / forwards the requests the last value is respected

api: Ensure single query parameter value

4d4d8d5

davidjumani changed the base branch from master to 4.15 May 10, 2021 12:01

DaanHoogland approved these changes May 10, 2021

View reviewed changes

sureshanaparti added this to the 4.15.2.0 milestone Jul 16, 2021

sureshanaparti added the component:api label Jul 16, 2021

yadvr modified the milestones: 4.15.2.0, 4.16.0.0 Aug 31, 2021

yadvr changed the base branch from 4.15 to main August 31, 2021 09:01

yadvr modified the milestones: 4.16.0.0, 4.16.1.0 Sep 14, 2021

yadvr modified the milestones: 4.16.1.0, 4.17.0.0 Nov 25, 2021

Merge branch 'main' of github.com:apache/cloudstack into fix-query-param

05d2491

davidjumani marked this pull request as ready for review February 25, 2022 05:43

change error to warn

a314adc

davidjumani changed the title ~~api: Ensure single query parameter value~~ api: Warn if query parameters have multiple values Mar 4, 2022

weizhouapache approved these changes Mar 4, 2022

View reviewed changes

nvazquez reviewed Mar 8, 2022

View reviewed changes

Comment thread server/src/main/java/com/cloud/api/ApiServlet.java Outdated

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

Address comments

cf2c1c4

DaanHoogland reviewed Mar 8, 2022

View reviewed changes

nvazquez approved these changes Mar 8, 2022

View reviewed changes

nvazquez merged commit 5534b7a into apache:main Mar 8, 2022

Conversation

davidjumani commented May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Bug Severity

How Has This Been Tested?

Uh oh!

davidjumani commented May 10, 2021

Uh oh!

blueorangutan commented May 10, 2021

Uh oh!

blueorangutan commented May 10, 2021

Uh oh!

davidjumani commented May 10, 2021

Uh oh!

blueorangutan commented May 10, 2021

Uh oh!

blueorangutan commented May 10, 2021

Uh oh!

DaanHoogland May 10, 2021

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented May 10, 2021

Uh oh!

davidjumani commented May 11, 2021

Uh oh!

blueorangutan commented May 11, 2021

Uh oh!

blueorangutan commented May 11, 2021

Uh oh!

GutoVeronezi commented Jun 1, 2021

Uh oh!

yadvr commented Aug 31, 2021

Uh oh!

davidjumani commented Feb 25, 2022

Uh oh!

blueorangutan commented Feb 25, 2022

Uh oh!

blueorangutan commented Feb 25, 2022

Uh oh!

davidjumani commented Feb 25, 2022

Uh oh!

blueorangutan commented Feb 25, 2022

Uh oh!

blueorangutan commented Feb 25, 2022

Uh oh!

weizhouapache commented Mar 2, 2022

Uh oh!

DaanHoogland commented Mar 2, 2022

Uh oh!

weizhouapache commented Mar 2, 2022

Uh oh!

davidjumani commented Mar 4, 2022

Uh oh!

davidjumani commented Mar 4, 2022

Uh oh!

blueorangutan commented Mar 4, 2022

Uh oh!

blueorangutan commented Mar 4, 2022

Uh oh!

davidjumani commented Mar 4, 2022

Uh oh!

blueorangutan commented Mar 4, 2022

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

nvazquez commented Mar 6, 2022

Uh oh!

blueorangutan commented Mar 6, 2022

Uh oh!

blueorangutan commented Mar 6, 2022

Uh oh!

Uh oh!

Uh oh!

davidjumani commented Mar 8, 2022

Uh oh!

blueorangutan commented Mar 8, 2022

Uh oh!

davidjumani commented May 10, 2021 •

edited

Loading

nvazquez commented Mar 8, 2022 •

edited

Loading