apiserver : Ensure required parameters are not empty #5136

davidjumani · 2021-06-22T06:28:23Z

Description

Addon to #5135
Ensures that any required parameter is not empty / null

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?

(localhost) 🐱 > create domain 
💩 Missing required parameters:  name
(localhost) 🐱 > create domain name= 
🙈 Error: (HTTP 431, error code 4350) Empty or null value provided for API arg: name

davidjumani · 2021-06-22T06:57:54Z

@blueorangutan package

blueorangutan · 2021-06-22T06:59:03Z

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

DaanHoogland

clgtm

blueorangutan · 2021-06-22T07:34:12Z

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

davidjumani · 2021-06-22T08:01:38Z

@blueorangutan test

blueorangutan · 2021-06-22T08:02:03Z

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

blueorangutan · 2021-06-22T08:20:40Z

Trillian Build Failed (tid-1036)

blueorangutan · 2021-06-22T09:36:21Z

Trillian Build Failed (tid-1037)

davidjumani · 2021-06-22T10:23:05Z

@blueorangutan package

blueorangutan · 2021-06-22T10:24:03Z

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

blueorangutan · 2021-06-22T10:51:39Z

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

davidjumani · 2021-06-22T10:57:48Z

@blueorangutan test

blueorangutan · 2021-06-22T10:59:03Z

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

blueorangutan · 2021-06-22T21:37:05Z

Trillian test result (tid-1039)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36304 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5136-t1039-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_direct_download.py
Intermittent failure detected: /marvin/tests/smoke/test_network.py
Intermittent failure detected: /marvin/tests/smoke/test_outofbandmanagement.py
Intermittent failure detected: /marvin/tests/smoke/test_persistent_network.py
Intermittent failure detected: /marvin/tests/smoke/test_storage_policy.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermittent failure detected: /marvin/tests/smoke/test_host_maintenance.py
Smoke tests completed. 82 look OK, 6 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestL2Networks>:setup	`Error`	175.74	test_network.py
ContextSuite context=TestDirectDownloadTemplates>:setup	`Error`	0.00	test_direct_download.py
ContextSuite context=TestL2PersistentNetworks>:setup	`Error`	0.00	test_persistent_network.py
ContextSuite context=TestVMWareStoragePolicies>:setup	`Error`	0.00	test_storage_policy.py
ContextSuite context=TestUnmanageVM>:setup	`Error`	202.06	test_vm_life_cycle.py
test_02_cancel_host_maintenace_with_migration_jobs	`Error`	0.12	test_host_maintenance.py
test_03_cancel_host_maintenace_with_migration_jobs_failure	`Error`	0.15	test_host_maintenance.py
ContextSuite context=TestHostMaintenanceAgents>:setup	`Error`	0.22	test_host_maintenance.py

davidjumani · 2021-06-23T04:20:39Z

@blueorangutan package

blueorangutan · 2021-06-23T04:21:03Z

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

davidjumani · 2021-06-23T04:31:45Z

@blueorangutan package

blueorangutan · 2021-06-23T04:32:03Z

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

blueorangutan · 2021-06-23T04:58:03Z

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

davidjumani · 2021-06-23T04:59:05Z

@blueorangutan test matrix

blueorangutan · 2021-06-23T05:00:03Z

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

blueorangutan · 2021-06-23T16:25:48Z

Trillian test result (tid-1044)
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37848 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5136-t1044-xenserver-71.zip
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_password_server.py
Intermittent failure detected: /marvin/tests/smoke/test_host_maintenance.py
Smoke tests completed. 88 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

blueorangutan · 2021-06-23T17:24:53Z

Trillian test result (tid-1045)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 41453 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5136-t1045-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_portforwardingrules.py
Smoke tests completed. 87 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_08_deploy_and_upgrade_kubernetes_ha_cluster	`Failure`	532.32	test_kubernetes_clusters.py

davidjumani · 2021-06-24T03:39:34Z

@blueorangutan test centos7 vmware-65u2

blueorangutan · 2021-06-24T03:40:03Z

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

GabrielBrascher

Thanks for the PR @davidjumani.
Overall code LGTM; I'm just curious about the removal of the required = true from supportedservices.

api/src/main/java/org/apache/cloudstack/api/command/admin/network/CreateNetworkOfferingCmd.java

GabrielBrascher

Code LGTM

blueorangutan · 2021-06-26T21:36:33Z

Trillian test result (tid-1115)
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36752 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5136-t1115-vmware-65u2.zip
Intermittent failure detected: /marvin/tests/smoke/test_list_ids_parameter.py
Intermittent failure detected: /marvin/tests/smoke/test_nic.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Smoke tests completed. 86 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
runTest	`Error`	0.00	test_nic.py
test_01_migrate_VM_and_root_volume	`Error`	113.55	test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks	`Error`	102.44	test_vm_life_cycle.py

rohityadavcloud · 2021-06-29T07:32:40Z

server/src/main/java/com/cloud/api/dispatch/ParamProcessWorker.java

                continue;
            }
+            if (parameterAnnotation.required()){
+                validateNonEmptyString(paramObj, parameterAnnotation.name());


LGTM - my only worry is if it causes a regression as it create a new behaviour (nit)

shwstppr

LGTM. Tested locally with simulator,

For required empty params:

(local) 🐱 > create domain name=
🙈 Error: (HTTP 431, error code 4350) Empty or null value provided for API arg: name

For non-required empty params:

(local) 🐱 > create volume name= zoneid=1b1aecd6-4ee1-4a05-a02c-e34625387a1a diskofferingid=01c41386-2101-48eb-b68e-610d1b5252d2 
{
  "volume": {
    "account": "admin",
    "created": "2021-06-29T14:41:16+0530",
    "destroyed": false,
    "diskofferingdisplaytext": "Small Disk, 5 GB",
    "diskofferingid": "01c41386-2101-48eb-b68e-610d1b5252d2",
    "diskofferingname": "Small",
    "displayvolume": true,
    "domain": "ROOT",
    "domainid": "1f376e9f-d8b4-11eb-bd2c-645d8651f45a",
    "hypervisor": "None",
    "id": "0c4a7918-d2eb-42d3-ada9-b2880402a36c",
    "isextractable": true,
    "jobid": "ca5a09cf-10d8-49ac-ad00-cb01bf57750c",
    "jobstatus": 0,
    "name": "8f16fe2e-cd2d-4705-b4fc-f138b726edbd",
    "provisioningtype": "thin",
    "quiescevm": false,
    "size": 5368709120,
    "state": "Allocated",
    "storagetype": "shared",
    "supportsstoragesnapshot": false,
    "tags": [],
    "type": "DATADISK",
    "zoneid": "1b1aecd6-4ee1-4a05-a02c-e34625387a1a",
    "zonename": "Sandbox-simulator"
  }
}

DaanHoogland · 2021-06-30T06:52:13Z

rekicked the final failing travis job (5) but i think we can merge

sureshanaparti · 2021-06-30T12:43:23Z

@davidjumani I understand that optional parameters passed in the API are ignored, but these are still not null. In any case, the parameter value returned based on not null check, there can be issue (for example, not null Boolean params can be interpreted as true/false - whatever default value for Boolean in java, and the default parameter value is the opposite).

davidjumani · 2021-07-01T04:37:52Z

@sureshanaparti There are no changes to optional parameters, only parameters with required=true will now have to not be empty

sureshanaparti · 2021-07-01T05:00:17Z

@sureshanaparti There are no changes to optional parameters, only parameters with required=true will now have to not be empty

As the required parameters are validated here, before validating them through the validations annotation, this annotation validations = {ApiArgValidator.NotNullOrEmpty} is no longer valid for required parameters.

davidjumani · 2021-07-01T05:05:15Z

@blueorangutan package

blueorangutan · 2021-07-01T05:06:03Z

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

davidjumani · 2021-07-01T05:55:13Z

@blueorangutan package

blueorangutan · 2021-07-01T05:56:02Z

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

blueorangutan · 2021-07-01T06:40:34Z

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

davidjumani · 2021-07-01T07:09:35Z

@blueorangutan test

blueorangutan · 2021-07-01T07:10:03Z

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

blueorangutan · 2021-07-01T18:44:08Z

Trillian test result (tid-1184)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 39907 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5136-t1184-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 86 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	84.75	test_kubernetes_clusters.py
test_disable_oobm_ha_state_ineligible	`Error`	1511.13	test_hostha_kvm.py

davidjumani marked this pull request as ready for review June 22, 2021 06:36

DaanHoogland approved these changes Jun 22, 2021

View reviewed changes

davidjumani force-pushed the fix-required-param branch 3 times, most recently from 31c561d to 01a7c3d Compare June 22, 2021 10:22

apiserver : Ensure required parameters are not empty

84e5eba

davidjumani force-pushed the fix-required-param branch from 01a7c3d to 84e5eba Compare June 22, 2021 10:22

supported services are not required as they can be empty

821d2e3

davidjumani force-pushed the fix-required-param branch from 18068ea to 821d2e3 Compare June 23, 2021 04:29

GabrielBrascher reviewed Jun 24, 2021

View reviewed changes

api/src/main/java/org/apache/cloudstack/api/command/admin/network/CreateNetworkOfferingCmd.java Show resolved Hide resolved

GabrielBrascher approved these changes Jun 24, 2021

View reviewed changes

rohityadavcloud reviewed Jun 29, 2021

View reviewed changes

shwstppr approved these changes Jun 29, 2021

View reviewed changes

DaanHoogland added this to the 4.16.0.0 milestone Jun 30, 2021

DaanHoogland added Severity:Minor type:bug labels Jun 30, 2021

Removing redudndant validators

eb52423

davidjumani force-pushed the fix-required-param branch from 1389e79 to eb52423 Compare July 1, 2021 05:53

rohityadavcloud merged commit f98d35d into apache:main Jul 15, 2021

Pearl1594 mentioned this pull request Aug 20, 2021

Bypass empty string check for username and password #5337

Merged

10 tasks

apiserver : Ensure required parameters are not empty #5136

apiserver : Ensure required parameters are not empty #5136

Uh oh!

Conversation

davidjumani commented Jun 22, 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 Jun 22, 2021

Uh oh!

blueorangutan commented Jun 22, 2021

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jun 22, 2021

Uh oh!

davidjumani commented Jun 22, 2021

Uh oh!

blueorangutan commented Jun 22, 2021

Uh oh!

blueorangutan commented Jun 22, 2021

Uh oh!

blueorangutan commented Jun 22, 2021

Uh oh!

davidjumani commented Jun 22, 2021

Uh oh!

blueorangutan commented Jun 22, 2021

Uh oh!

blueorangutan commented Jun 22, 2021

Uh oh!

davidjumani commented Jun 22, 2021

Uh oh!

blueorangutan commented Jun 22, 2021

Uh oh!

blueorangutan commented Jun 22, 2021

Uh oh!

davidjumani commented Jun 23, 2021

Uh oh!

blueorangutan commented Jun 23, 2021

Uh oh!

davidjumani commented Jun 23, 2021

Uh oh!

blueorangutan commented Jun 23, 2021

Uh oh!

blueorangutan commented Jun 23, 2021

Uh oh!

davidjumani commented Jun 23, 2021

Uh oh!

blueorangutan commented Jun 23, 2021

Uh oh!

blueorangutan commented Jun 23, 2021

Uh oh!

blueorangutan commented Jun 23, 2021

Uh oh!

davidjumani commented Jun 24, 2021

Uh oh!

blueorangutan commented Jun 24, 2021

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jun 26, 2021

Uh oh!

rohityadavcloud Jun 29, 2021

Choose a reason for hiding this comment

Uh oh!

shwstppr left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Jun 30, 2021

Uh oh!

sureshanaparti commented Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

davidjumani commented Jun 22, 2021 •

edited

Loading

sureshanaparti commented Jun 30, 2021 •

edited

Loading