CLOUDSTACK-3049: update dynamic role for an account #3058

bwsw · 2018-11-26T22:58:18Z

Description

Existing 'updateAccount' API call now can update role as well as account name and network domain. During the implementation 'newname' attribute was marked as optional, previously in the API was annotated as required, which is not a right behavior.

New Dockerfile was added Dockerfile.smokedev which is convenient if you develop smoke tests.

Helper script was added to deploy datacenter and run tests from the same docker image which guarantees a predictable environment.

docker run -v ~/dev/tmp:/tmp \
      -v ~/IdeaProjects/cloudstack/test/integration/smoke:/root/test/integration/smoke \
      -it \
      --name simulator -p 8080:8080 -p8096:8096 simulator:4.12

# second console 
docker exec -it simulator bash
# deploy datacenter
bash /root/docker_run_tests.sh advanced smoke
# follow the instructions

Implements #3049

Types of changes

Enhancement (improves an existing feature and functionality)

Screenshots (if appropriate):

How Has This Been Tested?

The code was tested with cloudmonkey
Smoketest for Marvin was implemented

rohityadavcloud · 2018-11-27T07:09:24Z

Can you instead implement a cleaner approach, say create a new API?

rafaelweingartner

Thanks @bwsw. I have only one comment regarding a set of nested IFs that can be improved.

Differently from @rhtyd I do not think we need another API method. From my perspective, the role is part of the account. Therefore, when updating the account I should/"would want to" be able to change the role as well.

rafaelweingartner · 2018-11-27T11:20:51Z

server/src/main/java/com/cloud/user/AccountManagerImpl.java

-        if (duplicateAcccount != null && duplicateAcccount.getId() != account.getId()) {
-            throw new InvalidParameterValueException(
-                    "There already exists an account with the name:" + newAccountName + " in the domain:" + domainId + " with existing account id:" + duplicateAcccount.getId());
+        if(newAccountName != null) {


You can use StringUtils.isEmpty here. Then, you do not need all of these netsting

@rafaelweingartner
Hm, I don't think so or don't get the idea. I would like to distinguish three situations:

newAccountName is not specified

newAccountName is specified but empty string -> exception 1

newAccountName is specified and not empty string -> action or exception

So, don't get how StringUtils.isEmpty() helps.

What I am saying is the following:

Instead of (the current code):

if(newAccountName != null) { if (newAccountName.isEmpty()) { throw new InvalidParameterValueException("The new account name for account '" + account.getUuid() + "' " + "within domain '" + domainId + "' is empty string. Account will be not renamed."); } // check if the new proposed account name is absent in the domain Account existingAccount = _accountDao.findActiveAccount(newAccountName, domainId); if (existingAccount != null && existingAccount.getId() != account.getId()) { throw new InvalidParameterValueException("The account with the proposed name '" + newAccountName + "' exists in the domain '" + domainId + "' with existing account id '" + existingAccount.getId() + "'"); } acctForUpdate.setAccountName(newAccountName); }

You can do the following:

if(StringUtils.isBlank(newAccountName)) { throw new InvalidParameterValueException("The new account name for account '" + account.getUuid() + "' " + "within domain '" + domainId + "' is an empty string. Account will be not renamed."); } // check if the new proposed account name is absent in the domain Account existingAccount = _accountDao.findActiveAccount(newAccountName, domainId); if (existingAccount != null && existingAccount.getId() != account.getId()) { throw new InvalidParameterValueException("The account with the proposed name '" + newAccountName + "' exists in the domain '" + domainId + "' with existing account id '" + existingAccount.getId() + "'"); } acctForUpdate.setAccountName(newAccountName);

By doing that you remove one nesting level.

Did you understand what I am saying now?

@rafaelweingartner But how it will behave if newAccountName is null which is normal if API is not intending to change that. It is a normal case and whole code block must be skipped.

good catch. I was not paying attention to this detail. Then, I would simply do newAccountName != null && StringUtils.isBlank(newAccountName).

That is only a suggestion. You can leave the code as is if you think it is better this way.

@rafaelweingartner ok. I'll leave as is.

borisstoyanov · 2018-11-27T12:20:22Z

@blueorangutan package

blueorangutan · 2018-11-27T12:21:25Z

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

blueorangutan · 2018-11-27T12:46:57Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2464

borisstoyanov · 2018-11-27T12:50:22Z

@blueorangutan test

blueorangutan · 2018-11-27T12:51:25Z

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

bwsw · 2018-11-27T15:06:06Z

Can you instead implement a cleaner approach, say create a new API?

@rhtyd I considered both alternatives. My arguments for the current implementation:

it adds smoke tests for updateAccount and as you can see before the implementation there wasn't testing for the method at all.
updateAccount method was strangely implemented, e.g. one has to pass newname mandatory, this enhancement fixes the behavior as well.
It's natural, I agree with Rafael, otherwise, it's true to implement every update<Entity><Attribute> separately and it will be hell without scoping.

What do you think?

rohityadavcloud · 2018-11-27T15:12:04Z

It's end of my day, let me get back to you soon. In short, my hesitation stems from the fact that the operation in question has some security and privileges implications. As a general rule such an update param/operation must be privileged and checked. It's better to have this as separate api. Take example of other apis which change ownership, transfer relationships etc all cases which could too be solved by an update api as well.

rohityadavcloud · 2018-11-27T15:13:13Z

Another note that there is a translation implication of accounttype that maybe implemented as well. Due to backward compatibility, we support both account type and roleid for the create api for example.

bwsw · 2018-11-27T15:19:33Z

It's end of my day, let me get back to you soon. In short, my hesitation stems from the fact that the operation in question has some security and privileges implications. As a general rule such an update param/operation must be privileged and checked. It's better to have this as separate api. Take example of other apis which change ownership, transfer relationships etc all cases which could too be solved by an update api as well.

@rhtyd Well, if we decide that only global admin (not domain admin) can change that, then you are right, it's better to implement as a separate API call.

rafaelweingartner · 2018-11-27T17:14:38Z

@bwsw if you use the authorize field in the @Parameter definition for the new role (the role being changed/updated) I think that then you can achieve what @rhtyd wanted. I mean, then we would be able to allow the use of such parameter only for admins or root admins.

blueorangutan · 2018-11-27T19:04:09Z

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

Test	Result	Time (s)	Test File
test_nic_secondaryip_add_remove	`Error`	22.60	test_multipleips_per_nic.py
test_04_rvpc_network_garbage_collector_nics	`Failure`	441.49	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Failure`	282.94	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Error`	306.08	test_vpc_redundant.py

bwsw · 2018-11-27T21:41:41Z

@rhtyd @rafaelweingartner
Well, currently the implementation allows changing the account details only for admin or domain admin. Admin can set any role while domain admin can set only DomainAdmin, ResourceAdmin, User types.
I suppose that basically, it's what users need.

Please, approve the basic design and then I write the tests for security assurance. Just don't want to spend the time if the design initially doesn't fit.

rafaelweingartner · 2019-01-09T17:17:33Z

@blueorangutan package

blueorangutan · 2019-01-09T17:29:40Z

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

blueorangutan · 2019-01-09T17:50:43Z

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-2526

GabrielBrascher · 2019-01-09T20:21:04Z

@blueorangutan test

blueorangutan · 2019-01-09T21:18:41Z

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

blueorangutan · 2019-01-10T04:04:52Z

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

Test	Result	Time (s)	Test File
test_nic_secondaryip_add_remove	`Error`	33.79	test_multipleips_per_nic.py
test_04_rvpc_network_garbage_collector_nics	`Failure`	495.45	test_vpc_redundant.py

dhlaluku · 2019-01-15T18:39:41Z

server/src/main/java/com/cloud/user/AccountManagerImpl.java

-                    "There already exists an account with the name:" + newAccountName + " in the domain:" + domainId + " with existing account id:" + duplicateAcccount.getId());
+        if(newAccountName != null) {
+
+            if (newAccountName.isEmpty()) {


@bwsw could you please add a comment here to specify that you are checking that the user did not pass an empty string for an optional parameter. Otherwise I think it is a bit confusing why you check if a not null string is empty. At least I found it confusing until I saw the conversation between you and @rafaelweingartner. Overall LGTM

@bwsw can you please address @dhlaluku comment? Thanks!

dhlaluku

LGTM

bwsw · 2019-01-24T13:14:57Z

@GabrielBrascher Hi, Gabriel, you have mentioned this PR as a problematic. It has got two LGTM. Please, comment on what do you want me to improve to be merged?

GabrielBrascher · 2019-01-24T13:32:14Z

@bwsw this one is looking good indeed, I must have dropped this PR on the wrong list.

Just to mention, independently of the "list" that the issue/PR was placed I will be getting in touch and help reviewing, testing, and pinging contributors/reviewers.

Thanks for pinging me, keep the good work 👍

GabrielBrascher · 2019-01-24T13:32:43Z

@blueorangutan package

blueorangutan · 2019-01-24T13:32:46Z

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

blueorangutan · 2019-01-24T13:59:23Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2563

bwsw · 2019-01-24T14:10:54Z

@GabrielBrascher Sounds great.

GabrielBrascher · 2019-01-24T14:16:20Z

@blueorangutan test

blueorangutan · 2019-01-24T14:16:47Z

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

blueorangutan · 2019-01-24T20:26:37Z

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

Test	Result	Time (s)	Test File

GabrielBrascher · 2019-01-25T12:02:24Z

Thanks for the work guys, this one looks ready to merge (LGTMs + tests are good).

bwsw force-pushed the 3049-update-dynamic-role branch 3 times, most recently from 40b1d13 to f2cac02 Compare Nov 26, 2018

rafaelweingartner reviewed Nov 27, 2018

View changes

CLOUDSTACK-3049: Implemented role update for account.

de843db

bwsw force-pushed the 3049-update-dynamic-role branch from 6dcdb20 to de843db Compare Nov 27, 2018

GabrielBrascher assigned bwsw Dec 6, 2018

GabrielBrascher added this to the 4.12.0.0 milestone Dec 6, 2018

rafaelweingartner approved these changes Jan 9, 2019

View changes

rafaelweingartner added the type:enhancement label Jan 9, 2019

dhlaluku reviewed Jan 15, 2019

View changes

dhlaluku approved these changes Jan 15, 2019

View changes

GabrielBrascher merged commit d68712e into apache:master Jan 25, 2019
2 checks passed

nathanejohnson added a commit to myENA/cloudstack that referenced this issue Jan 26, 2019

CLOUDSTACK-3049: Implemented role update for account. (apache#3058)

e413247

apache / cloudstack Public

CLOUDSTACK-3049: update dynamic role for an account #3058

CLOUDSTACK-3049: update dynamic role for an account #3058

Conversation

bwsw commented Nov 26, 2018 • edited

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

rohityadavcloud commented Nov 27, 2018

rafaelweingartner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borisstoyanov commented Nov 27, 2018

blueorangutan commented Nov 27, 2018

blueorangutan commented Nov 27, 2018

borisstoyanov commented Nov 27, 2018

blueorangutan commented Nov 27, 2018

bwsw commented Nov 27, 2018

rohityadavcloud commented Nov 27, 2018

rohityadavcloud commented Nov 27, 2018

bwsw commented Nov 27, 2018

rafaelweingartner commented Nov 27, 2018

blueorangutan commented Nov 27, 2018

bwsw commented Nov 27, 2018

rafaelweingartner commented Jan 9, 2019

blueorangutan commented Jan 9, 2019

blueorangutan commented Jan 9, 2019

GabrielBrascher commented Jan 9, 2019

blueorangutan commented Jan 9, 2019

blueorangutan commented Jan 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhlaluku left a comment

bwsw commented Jan 24, 2019

GabrielBrascher commented Jan 24, 2019

GabrielBrascher commented Jan 24, 2019

blueorangutan commented Jan 24, 2019

blueorangutan commented Jan 24, 2019

bwsw commented Jan 24, 2019

GabrielBrascher commented Jan 24, 2019

blueorangutan commented Jan 24, 2019

blueorangutan commented Jan 24, 2019

GabrielBrascher commented Jan 25, 2019

bwsw commented Nov 26, 2018 •

edited