Bypass empty string check for username and password by Pearl1594 · Pull Request #5337 · apache/cloudstack

Pearl1594 · 2021-08-19T07:29:06Z

Description

Fixes: #5332
This PR attempts to bypass the empty string check added in ParamProcessorValidator - for username and password, so that in case of vmware, it doesn't fail during addition of hosts (where username and password is obtained from cluster details)

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

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Added a host to a VMware cluster via UI without any issues.

yadvr · 2021-08-19T07:36:37Z

I think we had fixed the issue in Trillian to work-around that @Pearl1594 cc @nvazquez

harikrishna-patnala

LGTM. the old UI behaves the same as this PR.

yadvr · 2021-08-19T07:46:42Z

@blueorangutan package

blueorangutan · 2021-08-19T07:47:03Z

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

blueorangutan · 2021-08-19T08:33:17Z

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

Pearl1594 · 2021-08-19T08:35:17Z

@blueorangutan test

blueorangutan · 2021-08-19T08:36:03Z

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

ravening · 2021-08-19T11:25:03Z

    private static final Logger s_logger = Logger.getLogger(ParamProcessWorker.class.getName());
    public final DateFormat inputFormat = new SimpleDateFormat("yyyy-MM-dd");
    public final DateFormat newInputFormat = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
+    public static final List<String> avoidList = new ArrayList<>(Arrays.asList("username", "password"));


How about using ApiConstants.USERNAME and PASSWORD ?

Done - Thanks @ravening

blueorangutan · 2021-08-19T18:33:56Z

Trillian test result (tid-1718)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34076 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5337-t1718-kvm-centos7.zip
Smoke tests completed. 89 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

weizhouapache · 2021-08-20T06:12:53Z

@Pearl1594
this has small code change, but might have big impact.
can we skip the username/password check only in some apis ?

Pearl1594 · 2021-08-20T06:18:53Z

We could do that @weizhouapache - However, my understanding is, if a null / empty uname / password is provided as part of any API args - the corresponding APIs logic would ideally validate the correctness of the input or the authorization would fails if it's empty - which is the assumption that we had even before this check was added by - #5136.

weizhouapache · 2021-08-20T06:31:45Z

We could do that @weizhouapache - However, my understanding is, if a null / empty uname / password is provided as part of any API args - the corresponding APIs logic would ideally validate the correctness of the input or the authorization would fails if it's empty - which is the assumption that we had even before this check was added by - #5136.

@Pearl1594
If you can make sure that all required username/password have been verified, this pr is good to me.

Otherwise, I suggest you change the fields from required to optional in the specific API (add host ?) and check if they are null in codes.

weizhouapache · 2021-08-20T06:39:35Z

We could do that @weizhouapache - However, my understanding is, if a null / empty uname / password is provided as part of any API args - the corresponding APIs logic would ideally validate the correctness of the input or the authorization would fails if it's empty - which is the assumption that we had even before this check was added by - #5136.

#5136 is good I think, it fixes some potential missing validations, and causes few regressions (for example the issue you mentioned in PR description). in my opinion, it's better to fix the regression (eg, optional username/password in specific api), instead of rolling back to old behavior (skip validation on username/password in all apis).

Pearl1594 · 2021-08-20T07:17:50Z

We could do that @weizhouapache - However, my understanding is, if a null / empty uname / password is provided as part of any API args - the corresponding APIs logic would ideally validate the correctness of the input or the authorization would fails if it's empty - which is the assumption that we had even before this check was added by - #5136.

@Pearl1594
If you can make sure that all required username/password have been verified, this pr is good to me.

Otherwise, I suggest you change the fields from required to optional in the specific API (add host ?) and check if they are null in codes.

Will do @weizhouapache - Thanks.

weizhouapache · 2021-08-20T09:38:29Z

@Pearl1594
should we check if username/password are null when hypervisor is not vmware ?

weizhouapache

code lgtm
thanks for fixing @Pearl1594

weizhouapache · 2021-08-20T11:05:39Z

@blueorangutan package

blueorangutan · 2021-08-20T11:06:03Z

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

Pearl1594 · 2021-08-20T11:15:35Z

@weizhouapache I believe this issue is only seen on main branch

DaanHoogland

clgtm

Co-authored-by: dahn <daan.hoogland@gmail.com>

yadvr · 2021-08-20T11:39:06Z

@blueorangutan package

blueorangutan · 2021-08-20T11:40:02Z

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

weizhouapache · 2021-08-20T11:47:48Z

@weizhouapache I believe this issue is only seen on main branch

@Pearl1594 ok, got it. thanks for confirm

blueorangutan · 2021-08-20T11:57:40Z

Packaging result: ✔️ el7 ✔️ debian. SL-JID 955

blueorangutan · 2021-08-20T12:17:03Z

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

Pearl1594 · 2021-08-20T12:18:41Z

@blueorangutan test

blueorangutan · 2021-08-20T12:19:02Z

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

blueorangutan · 2021-08-20T22:10:02Z

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

Test	Result	Time (s)	Test File
test_02_list_snapshots_with_removed_data_store	`Error`	51.50	test_snapshots.py

DaanHoogland · 2021-08-21T07:31:58Z

must be unlucky on that snapshot test
@blueorangutan test

blueorangutan · 2021-08-21T07:33:02Z

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

shwstppr

@Pearl1594 sorry but I don't understand point of this change. We are removing required tag for API params but making them required again in API handling.

Maybe the intention is to check username, password is not empty when they are passed but I don't think the check correspond to that. Sorry if I'm missing something.

weizhouapache · 2021-08-21T07:52:42Z

@Pearl1594 sorry but I don't understand point of this change. We are removing required tag for API params but making them required again in API handling.

Maybe the intention is to check username, password is not empty when they are passed but I don't think the check correspond to that. Sorry if I'm missing something.

username and password can be empty when add a vmware host.

weizhouapache · 2021-08-21T11:12:35Z

@Pearl1594 is it possible that username and password are empty in cluster details ?

blueorangutan · 2021-08-21T18:22:49Z

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

Test	Result	Time (s)	Test File
test_02_list_snapshots_with_removed_data_store	`Error`	52.38	test_snapshots.py
test_disable_oobm_ha_state_ineligible	`Error`	1511.24	test_hostha_kvm.py

Pearl1594 · 2021-08-23T11:25:26Z

@Pearl1594 is it possible that username and password are empty in cluster details ?

@weizhouapache even if that's the case - there's a check in place to handle it
https://github.com/apache/cloudstack/blob/main/server/src/main/java/com/cloud/resource/ResourceManagerImpl.java#L701-L706

weizhouapache · 2021-08-23T12:44:14Z

@Pearl1594 is it possible that username and password are empty in cluster details ?

@weizhouapache even if that's the case - there's a check in place to handle it
https://github.com/apache/cloudstack/blob/main/server/src/main/java/com/cloud/resource/ResourceManagerImpl.java#L701-L706

@Pearl1594 ok, that's good.

GutoVeronezi · 2021-08-23T18:02:32Z

            throw new InvalidParameterValueException("Can't specify cluster without specifying the pod");
        }
+        if (!HypervisorType.VMware.toString().equalsIgnoreCase(hypervisorType) &&
+                (Strings.isNullOrEmpty(username) || Strings.isNullOrEmpty(password))) {


We could import org.apache.commons.lang3.StringUtils here and use StringUtils.isAnyEmpty(username, password).

I agree with @GutoVeronezi.

Nowadays we already have some discussions about adopting org.apache.commons.lang3.StringUtils vs com.cloud.utils.StringUtils "facade" (@DaanHoogland 🙂); with the google.common.base.Strings adds a third variable to this "equation".

Bypass empty string check for username and password

6518fc0

yadvr added this to the 4.16.0.0 milestone Aug 19, 2021

yadvr approved these changes Aug 19, 2021

View reviewed changes

harikrishna-patnala approved these changes Aug 19, 2021

View reviewed changes

ravening reviewed Aug 19, 2021

View reviewed changes

use constants

eed997b

nvazquez approved these changes Aug 19, 2021

View reviewed changes

Pearl1594 closed this Aug 20, 2021

Pearl1594 reopened this Aug 20, 2021

weizhouapache closed this Aug 20, 2021

weizhouapache reopened this Aug 20, 2021

make username/password optional for addHost API

dcab1cc

weizhouapache reviewed Aug 20, 2021

View reviewed changes

Comment thread api/src/main/java/org/apache/cloudstack/api/command/admin/host/AddHostCmd.java Outdated

update api desc and intensify checks

ba6abc2

Pearl1594 requested a review from weizhouapache August 20, 2021 10:44

weizhouapache approved these changes Aug 20, 2021

View reviewed changes

DaanHoogland reviewed Aug 20, 2021

View reviewed changes

Comment thread server/src/main/java/com/cloud/resource/ResourceManagerImpl.java Outdated

DaanHoogland approved these changes Aug 20, 2021

View reviewed changes

Update server/src/main/java/com/cloud/resource/ResourceManagerImpl.java

9f1e45f

Co-authored-by: dahn <daan.hoogland@gmail.com>

shwstppr reviewed Aug 21, 2021

View reviewed changes

shwstppr approved these changes Aug 21, 2021

View reviewed changes

sureshanaparti approved these changes Aug 23, 2021

View reviewed changes

weizhouapache closed this Aug 23, 2021

weizhouapache reopened this Aug 23, 2021

GutoVeronezi reviewed Aug 23, 2021

View reviewed changes

yadvr merged commit e1e6ec1 into apache:main Aug 24, 2021

yadvr mentioned this pull request Aug 24, 2021

[UI] Unable to add ESX/ESXi host from UI, Username / Password inputs are missing on Add Host dialog. #5332

Closed

Pearl1594 mentioned this pull request Sep 1, 2021

travis: fix consistent failures noticed on few tests #5399

Merged

10 tasks

nvazquez added the type:bug label Oct 14, 2021

Conversation

Pearl1594 commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

yadvr commented Aug 19, 2021

Uh oh!

harikrishna-patnala left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Aug 19, 2021

Uh oh!

blueorangutan commented Aug 19, 2021

Uh oh!

blueorangutan commented Aug 19, 2021

Uh oh!

Pearl1594 commented Aug 19, 2021

Uh oh!

blueorangutan commented Aug 19, 2021

Uh oh!

ravening Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

Pearl1594 Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Aug 19, 2021

Uh oh!

weizhouapache commented Aug 20, 2021

Uh oh!

Pearl1594 commented Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weizhouapache commented Aug 20, 2021

Uh oh!

weizhouapache commented Aug 20, 2021

Uh oh!

Pearl1594 commented Aug 20, 2021

Uh oh!

weizhouapache commented Aug 20, 2021

Uh oh!

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Aug 20, 2021

Uh oh!

blueorangutan commented Aug 20, 2021

Uh oh!

Pearl1594 commented Aug 20, 2021

Uh oh!

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Aug 20, 2021

Uh oh!

blueorangutan commented Aug 20, 2021

Uh oh!

weizhouapache commented Aug 20, 2021

Uh oh!

blueorangutan commented Aug 20, 2021

Uh oh!

blueorangutan commented Aug 20, 2021

Uh oh!

Pearl1594 commented Aug 20, 2021

Uh oh!

blueorangutan commented Aug 20, 2021

Uh oh!

blueorangutan commented Aug 20, 2021

Uh oh!

DaanHoogland commented Aug 21, 2021

Uh oh!

blueorangutan commented Aug 21, 2021

Uh oh!

Pearl1594 commented Aug 19, 2021 •

edited

Loading

Pearl1594 commented Aug 20, 2021 •

edited

Loading