You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2021/08/19 07:29:19 UTC
[GitHub] [cloudstack] Pearl1594 opened a new pull request #5337: Bypass empty string check for username and password
Pearl1594 opened a new pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337
### Description
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)
<!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
<!--- ********************************************************************************* -->
<!--- NOTE: AUTOMATATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ********************************************************************************* -->
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [X] 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
- [ ] BLOCKER
- [ ] Critical
- [X] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
Added a host to a VMware cluster via UI without any issues.
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache closed pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
weizhouapache closed pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-901721379
@Pearl1594 a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd merged pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
rhtyd merged pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-903075381
must be unlucky on that snapshot test
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-902982950
<b>Trillian test result (tid-1737)</b>
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-903156464
<b>Trillian test result (tid-1741)</b>
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-902632320
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#discussion_r692867216
##########
File path: server/src/main/java/com/cloud/resource/ResourceManagerImpl.java
##########
@@ -687,6 +688,10 @@ public Discoverer getMatchingDiscover(final Hypervisor.HypervisorType hypervisor
if ((clusterName != null || clusterId != null) && podId == null) {
throw new InvalidParameterValueException("Can't specify cluster without specifying the pod");
}
+ if (!hypervisorType.equalsIgnoreCase(HypervisorType.VMware.toString()) &&
+ (Strings.isNullOrEmpty(username) || Strings.isNullOrEmpty(password))) {
+ throw new InvalidParameterValueException("Username and Password need to be provided.");
+ }
Review comment:
maybe excessive but also a tat safer:
```suggestion
if (!HypervisorType.VMware.toString().equalsIgnoreCase(hypervisorType) &&
(Strings.isNullOrEmpty(username) || Strings.isNullOrEmpty(password))) {
throw new InvalidParameterValueException("Username and Password need to be provided.");
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-902651844
@Pearl1594 a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi commented on a change in pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on a change in pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#discussion_r694193322
##########
File path: server/src/main/java/com/cloud/resource/ResourceManagerImpl.java
##########
@@ -687,6 +688,10 @@ public Discoverer getMatchingDiscover(final Hypervisor.HypervisorType hypervisor
if ((clusterName != null || clusterId != null) && podId == null) {
throw new InvalidParameterValueException("Can't specify cluster without specifying the pod");
}
+ if (!HypervisorType.VMware.toString().equalsIgnoreCase(hypervisorType) &&
+ (Strings.isNullOrEmpty(username) || Strings.isNullOrEmpty(password))) {
Review comment:
We could import `org.apache.commons.lang3.StringUtils` here and use `StringUtils.isAnyEmpty(username, password)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-902650745
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 958
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-902146912
<b>Trillian test result (tid-1718)</b>
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
--- | --- | --- | ---
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#discussion_r692813167
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/admin/host/AddHostCmd.java
##########
@@ -54,7 +54,7 @@
@Parameter(name = ApiConstants.CLUSTER_NAME, type = CommandType.STRING, description = "the cluster name for the host")
private String clusterName;
- @Parameter(name = ApiConstants.PASSWORD, type = CommandType.STRING, required = true, description = "the password for the host")
+ @Parameter(name = ApiConstants.PASSWORD, type = CommandType.STRING, description = "the password for the host")
Review comment:
@Pearl1594
should we update the description if they are required by kvm/xenserver or other hypervisors ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#discussion_r692022378
##########
File path: server/src/main/java/com/cloud/api/dispatch/ParamProcessWorker.java
##########
@@ -70,6 +71,7 @@
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"));
Review comment:
How about using ApiConstants.USERNAME and PASSWORD ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-903077834
> @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-902631858
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-902488378
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 removed a comment on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
Pearl1594 removed a comment on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-901837205
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 commented on a change in pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#discussion_r692024759
##########
File path: server/src/main/java/com/cloud/api/dispatch/ParamProcessWorker.java
##########
@@ -70,6 +71,7 @@
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"));
Review comment:
Done - Thanks @ravening
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 closed pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
Pearl1594 closed pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-901688209
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-902640188
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: debian. SL-JID 955
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-902615869
@Pearl1594 @rhtyd should we fix it in 4.15.2.0 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-902569198
@Pearl1594
should we check if username/password are null when hypervisor is not vmware ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-901688020
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#discussion_r694829490
##########
File path: server/src/main/java/com/cloud/resource/ResourceManagerImpl.java
##########
@@ -687,6 +688,10 @@ public Discoverer getMatchingDiscover(final Hypervisor.HypervisorType hypervisor
if ((clusterName != null || clusterId != null) && podId == null) {
throw new InvalidParameterValueException("Can't specify cluster without specifying the pod");
}
+ if (!HypervisorType.VMware.toString().equalsIgnoreCase(hypervisorType) &&
+ (Strings.isNullOrEmpty(username) || Strings.isNullOrEmpty(password))) {
Review comment:
I agree with @GutoVeronezi.
Nowadays we already have some discussions about adopting `org.apache.commons.lang3.StringUtils` vs `com.cloud.utils.StringUtils` "_facade_" (@DaanHoogland :slightly_smiling_face:); with the `google.common.base.Strings` adds a third variable to this "equation".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-903100784
@Pearl1594 is it possible that username and password are empty in cluster details ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-902635785
> @weizhouapache I believe this issue is only seen on main branch
@Pearl1594 ok, got it. thanks for confirm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-902457982
@Pearl1594
this has small code change, but might have big impact.
can we skip the username/password check only in some apis ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-902615875
@weizhouapache a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-903730730
> > @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-903679596
> @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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-901719615
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian. SL-JID 930
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-902468743
> 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache closed pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
weizhouapache closed pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-902460531
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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-902651653
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-903075556
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-901682067
I think we had fixed the issue in Trillian to work-around that @Pearl1594 cc @nvazquez
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-901720898
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-901837205
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-902465532
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-902621006
@weizhouapache I believe this issue is only seen on main branch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 edited a comment on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
Pearl1594 edited a comment on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-902460531
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 - https://github.com/apache/cloudstack/pull/5136.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #5337: Bypass empty string check for username and password
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #5337:
URL: https://github.com/apache/cloudstack/pull/5337#issuecomment-902615666
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org