You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "gpordeus (via GitHub)" <gi...@apache.org> on 2024/02/21 14:08:55 UTC
[PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
gpordeus opened a new pull request, #8689:
URL: https://github.com/apache/cloudstack/pull/8689
### Description
This PR adds a role access check to the `expungeVirtualMachine` command when calling `destroyVirtualMachine` with the expunge parameter.
Currently, if you are an admin (even if not Root), it bypasses the `allow.user.expunge.recover.vm` verification and you are always allowed to expunge when calling for `destroyVirtualMachine`.
The use case that called for this change was a need for a role of type domain admin to be unable to expunge VMs. It was then found that even with the DENY rule, the user could still expunge through `destroyVirtualMachine` (even on already destroyed VMs, with an API call) and the setting `allow.user.expunge.recover.vm` did nothing.
### 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)
- [ ] build/CI
### Feature/Enhancement Scale or Bug Severity
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [X] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
I created a role, based on the default Domain Admin, and changed the `expungeVirtualMachine` rule to `DENY`. I then created an account with said role.
I created two VMs and destroyed one of them, verifying that the expunge option did not show up on the GUI.
I then ran `destroy virtualmachine` on cloudmonkey with `expunge = true` on both VMs and both returned the error `Account does not have permission for expunging`. Calling the same command without the parameter destroyed the running VM successfully.
I repeated the tests with a role based on default User:
With `allow.user.expunge.recover.vm = true`, it behaved the same as the DomainAdmin-based one.
With `allow.user.expunge.recover.vm = false`, it did not allow the expunge action, no matter the role rules. Without the expunge parameter, the destroy action worked as expected.
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-1983160730
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. 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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "gpordeus (via GitHub)" <gi...@apache.org>.
gpordeus commented on code in PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#discussion_r1529192893
##########
test/integration/smoke/test_vm_life_cycle.py:
##########
@@ -955,6 +956,92 @@ def test_12_start_vm_multiple_volumes_allocated(self):
"Check virtual machine is in running state"
)
+ @attr(tags=["devcloud", "advanced", "advancedns", "smoke", "basic", "sg"], required_hardware="false")
+ def test_13_destroy_and_expunge_vm(self):
+ """Test destroy virtual machine with expunge parameter depending on whether the caller's role has expunge permission.
+ """
+ # Setup steps:
+ # 1. Create role with DENY expunge permission.
+ # 2. Create account with said role.
+ # 3. Create a VM of said account.
+ # 4. Create a VM of cls.account
+ # Validation steps:
+ # 1. Destroy the VM with the created account and verify it was not destroyed.
+ # 1. Destroy the other VM with cls.account and verify it was expunged.
+
+ role = Role.importRole(
+ self.apiclient,
+ {
+ "name": "MarvinFake Import Role ",
+ "type": "DomainAdmin",
+ "description": "Fake Import Domain Admin Role created by Marvin test",
+ "rules" : [{"rule":"list*", "permission":"allow","description":"Listing apis"},
+ {"rule":"get*", "permission":"allow","description":"Get apis"},
+ {"rule":"update*", "permission":"allow","description":"Update apis"},
+ {"rule":"queryAsyncJobResult", "permission":"allow","description":"Query async job result"},
+ {"rule":"deployVirtualMachine", "permission":"allow","description":"Deploy virtual machine"},
+ {"rule":"destroyVirtualMachine", "permission":"allow","description":"Destroy virtual machine"},
+ {"rule":"expungeVirtualMachine", "permission":"deny","description":"Expunge virtual machine"}]
+ },
+ )
+
+ domadm = Account.create(
+ self.apiclient,
+ self.services["account"],
+ admin=True,
+ roleid=role.id,
+ domainid=self.domain.id
+ )
+
+ self.cleanup.append(domadm)
+ self.cleanup.append(role)
Review Comment:
@DaanHoogland I tried adding the super call, but there are conflicts with test 12 and I don't have the time right now to fix it properly, so I've just added the swap to avoid the possible garbage.
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-1956775149
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/8689?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`592038a`)](https://app.codecov.io/gh/apache/cloudstack/commit/592038a30424713a731ede43000a169762b074dd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 23.14% compared to head [(`c53258d`)](https://app.codecov.io/gh/apache/cloudstack/pull/8689?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 4.36%.
> Report is 1 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #8689 +/- ##
============================================
- Coverage 23.14% 4.36% -18.79%
============================================
Files 5219 361 -4858
Lines 353412 28835 -324577
Branches 50883 5037 -45846
============================================
- Hits 81805 1259 -80546
+ Misses 259762 27437 -232325
+ Partials 11845 139 -11706
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/8689/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [simulator-marvin-tests](https://app.codecov.io/gh/apache/cloudstack/pull/8689/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/8689/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.36% <ø> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/cloudstack/pull/8689?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "gpordeus (via GitHub)" <gi...@apache.org>.
gpordeus commented on code in PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#discussion_r1526394427
##########
test/integration/smoke/test_vm_life_cycle.py:
##########
@@ -955,6 +956,92 @@ def test_12_start_vm_multiple_volumes_allocated(self):
"Check virtual machine is in running state"
)
+ @attr(tags=["devcloud", "advanced", "advancedns", "smoke", "basic", "sg"], required_hardware="false")
+ def test_13_destroy_and_expunge_vm(self):
+ """Test destroy virtual machine with expunge parameter depending on whether the caller's role has expunge permission.
+ """
+ # Setup steps:
+ # 1. Create role with DENY expunge permission.
+ # 2. Create account with said role.
+ # 3. Create a VM of said account.
+ # 4. Create a VM of cls.account
+ # Validation steps:
+ # 1. Destroy the VM with the created account and verify it was not destroyed.
+ # 1. Destroy the other VM with cls.account and verify it was expunged.
+
+ role = Role.importRole(
+ self.apiclient,
+ {
+ "name": "MarvinFake Import Role ",
+ "type": "DomainAdmin",
+ "description": "Fake Import Domain Admin Role created by Marvin test",
+ "rules" : [{"rule":"list*", "permission":"allow","description":"Listing apis"},
+ {"rule":"get*", "permission":"allow","description":"Get apis"},
+ {"rule":"update*", "permission":"allow","description":"Update apis"},
+ {"rule":"queryAsyncJobResult", "permission":"allow","description":"Query async job result"},
+ {"rule":"deployVirtualMachine", "permission":"allow","description":"Deploy virtual machine"},
+ {"rule":"destroyVirtualMachine", "permission":"allow","description":"Destroy virtual machine"},
+ {"rule":"expungeVirtualMachine", "permission":"deny","description":"Expunge virtual machine"}]
+ },
+ )
+
+ domadm = Account.create(
+ self.apiclient,
+ self.services["account"],
+ admin=True,
+ roleid=role.id,
+ domainid=self.domain.id
+ )
+
+ self.cleanup.append(domadm)
+ self.cleanup.append(role)
Review Comment:
@DaanHoogland I tried it, but then when it's time to clean up it ends up in error since the role is being deleted before the account. Would adding a manual swap, like this, be better?
```
self.cleanup.append(role)
domadm =[...]
self.cleanup[-1]=domadm
self.cleanup.append(role)
```
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "JoaoJandre (via GitHub)" <gi...@apache.org>.
JoaoJandre commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2287004701
@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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2248325242
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10459
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-1961283712
@gpordeus , this sounds like a good use case for an integration test. Will you considder that?
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-1962047621
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. 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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2293169672
@DaanHoogland a [LL] 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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2293173684
<b>[LL] Trillian Build Failed (tid-6968)<b/>
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#discussion_r1497658232
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -3303,17 +3305,38 @@ public UserVm rebootVirtualMachine(RebootVMCmd cmd) throws InsufficientCapacityE
return null;
}
+ /**
+ * Encapsulates AllowUserExpungeRecoverVm so we can unit test checkExpungeVmPermission.
+ */
+ protected boolean getConfigAllowUserExpungeRecoverVm(Long entityOwnerId) {
+ return AllowUserExpungeRecoverVm.valueIn(entityOwnerId);
+ }
+
+ protected void checkExpungeVmPermission (Account callingAccount, Long entityOwnerId) {
+ logger.debug(String.format("Checking if [%s] has permission for expunging VMs.", callingAccount));
+ if (!_accountMgr.isAdmin(callingAccount.getId()) && !getConfigAllowUserExpungeRecoverVm(entityOwnerId)) {
Review Comment:
I wonder if we should check the setting of the caller (`callingAccount.getId()`, not the `entityOwnerId`) here.
`entityOwnerId` was used in the past
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "gpordeus (via GitHub)" <gi...@apache.org>.
gpordeus commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-1961364011
> @gpordeus , this sounds like a good use case for an integration test. Will you consider that?
Sure, on it.
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "lucas-a-martins (via GitHub)" <gi...@apache.org>.
lucas-a-martins commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-1981725455
I manually tested, lgtm.
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2031512207
@blueorangutan test alma9 kvm-alma9
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2154354494
Packaging result [SF]: ✖️ el7 ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 9817
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2154379222
not sure why the bot removed the conflict sticker, but you still have some @gpordeus
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-1983157479
@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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-1962057441
Packaging result [SF]: ✖️ el7 ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 8764
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2100847555
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base 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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2097673417
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/8689?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `50.00000%` with `8 lines` in your changes are missing coverage. Please review.
> Project coverage is 23.13%. Comparing base [(`592038a`)](https://app.codecov.io/gh/apache/cloudstack/commit/592038a30424713a731ede43000a169762b074dd?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`007f1e1`)](https://app.codecov.io/gh/apache/cloudstack/pull/8689?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 221 commits behind head on main.
| [Files](https://app.codecov.io/gh/apache/cloudstack/pull/8689?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../src/main/java/com/cloud/vm/UserVmManagerImpl.java](https://app.codecov.io/gh/apache/cloudstack/pull/8689?src=pr&el=tree&filepath=server%2Fsrc%2Fmain%2Fjava%2Fcom%2Fcloud%2Fvm%2FUserVmManagerImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmVyL3NyYy9tYWluL2phdmEvY29tL2Nsb3VkL3ZtL1VzZXJWbU1hbmFnZXJJbXBsLmphdmE=) | 38.46% | [4 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/cloudstack/pull/8689?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #8689 +/- ##
============================================
- Coverage 23.14% 23.13% -0.02%
- Complexity 23348 23485 +137
============================================
Files 5219 5234 +15
Lines 353412 355729 +2317
Branches 50883 51238 +355
============================================
+ Hits 81805 82294 +489
- Misses 259762 261540 +1778
- Partials 11845 11895 +50
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/8689/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [simulator-marvin-tests](https://app.codecov.io/gh/apache/cloudstack/pull/8689/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.80% <50.00%> (-0.02%)` | :arrow_down: |
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/8689/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.34% <ø> (-0.03%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/cloudstack/pull/8689?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2287005539
@JoaoJandre a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. 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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#discussion_r1537274611
##########
test/integration/smoke/test_vm_life_cycle.py:
##########
@@ -955,6 +956,92 @@ def test_12_start_vm_multiple_volumes_allocated(self):
"Check virtual machine is in running state"
)
+ @attr(tags=["devcloud", "advanced", "advancedns", "smoke", "basic", "sg"], required_hardware="false")
+ def test_13_destroy_and_expunge_vm(self):
+ """Test destroy virtual machine with expunge parameter depending on whether the caller's role has expunge permission.
+ """
+ # Setup steps:
+ # 1. Create role with DENY expunge permission.
+ # 2. Create account with said role.
+ # 3. Create a VM of said account.
+ # 4. Create a VM of cls.account
+ # Validation steps:
+ # 1. Destroy the VM with the created account and verify it was not destroyed.
+ # 1. Destroy the other VM with cls.account and verify it was expunged.
+
+ role = Role.importRole(
+ self.apiclient,
+ {
+ "name": "MarvinFake Import Role ",
+ "type": "DomainAdmin",
+ "description": "Fake Import Domain Admin Role created by Marvin test",
+ "rules" : [{"rule":"list*", "permission":"allow","description":"Listing apis"},
+ {"rule":"get*", "permission":"allow","description":"Get apis"},
+ {"rule":"update*", "permission":"allow","description":"Update apis"},
+ {"rule":"queryAsyncJobResult", "permission":"allow","description":"Query async job result"},
+ {"rule":"deployVirtualMachine", "permission":"allow","description":"Deploy virtual machine"},
+ {"rule":"destroyVirtualMachine", "permission":"allow","description":"Destroy virtual machine"},
+ {"rule":"expungeVirtualMachine", "permission":"deny","description":"Expunge virtual machine"}]
+ },
+ )
+
+ domadm = Account.create(
+ self.apiclient,
+ self.services["account"],
+ admin=True,
+ roleid=role.id,
+ domainid=self.domain.id
+ )
+
+ self.cleanup.append(domadm)
+ self.cleanup.append(role)
Review Comment:
ok, can you add a comment about it? fine by me, but people need to be aware and not use this code as an example for how "it should be done".
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-1988683682
<b>[SF] Trillian Build Failed (tid-9434)<b/>
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2154296482
@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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "gpordeus (via GitHub)" <gi...@apache.org>.
gpordeus commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2155234774
@DaanHoogland Fixed, thanks for letting me know.
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-1988670933
@DaanHoogland a [SL] Trillian-Jenkins test job (alma9 mgmt + kvm-alma9) 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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "gpordeus (via GitHub)" <gi...@apache.org>.
gpordeus commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-1991871732
@DaanHoogland I've added the integration 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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#discussion_r1526511501
##########
test/integration/smoke/test_vm_life_cycle.py:
##########
@@ -955,6 +956,92 @@ def test_12_start_vm_multiple_volumes_allocated(self):
"Check virtual machine is in running state"
)
+ @attr(tags=["devcloud", "advanced", "advancedns", "smoke", "basic", "sg"], required_hardware="false")
+ def test_13_destroy_and_expunge_vm(self):
+ """Test destroy virtual machine with expunge parameter depending on whether the caller's role has expunge permission.
+ """
+ # Setup steps:
+ # 1. Create role with DENY expunge permission.
+ # 2. Create account with said role.
+ # 3. Create a VM of said account.
+ # 4. Create a VM of cls.account
+ # Validation steps:
+ # 1. Destroy the VM with the created account and verify it was not destroyed.
+ # 1. Destroy the other VM with cls.account and verify it was expunged.
+
+ role = Role.importRole(
+ self.apiclient,
+ {
+ "name": "MarvinFake Import Role ",
+ "type": "DomainAdmin",
+ "description": "Fake Import Domain Admin Role created by Marvin test",
+ "rules" : [{"rule":"list*", "permission":"allow","description":"Listing apis"},
+ {"rule":"get*", "permission":"allow","description":"Get apis"},
+ {"rule":"update*", "permission":"allow","description":"Update apis"},
+ {"rule":"queryAsyncJobResult", "permission":"allow","description":"Query async job result"},
+ {"rule":"deployVirtualMachine", "permission":"allow","description":"Deploy virtual machine"},
+ {"rule":"destroyVirtualMachine", "permission":"allow","description":"Destroy virtual machine"},
+ {"rule":"expungeVirtualMachine", "permission":"deny","description":"Expunge virtual machine"}]
+ },
+ )
+
+ domadm = Account.create(
+ self.apiclient,
+ self.services["account"],
+ admin=True,
+ roleid=role.id,
+ domainid=self.domain.id
+ )
+
+ self.cleanup.append(domadm)
+ self.cleanup.append(role)
Review Comment:
ah, yes legacy, The tearDown and tearDownClass methods should actually call super and this old test doesn't. In CloudstackTestCase the array is being reversed and some other checks are done as well. To fix this you may have to fix the setupClass, and setup methods as well. If you have the space please do, otherwise :shrug:
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland merged PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-1963885840
Packaging result [SF]: ✖️ el7 ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 8780
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2288951447
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 10661
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2027241774
@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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2033366912
<b>[SF] Trillian test result (tid-9650)</b>
Environment: kvm-alma9 (x2), Advanced Networking with Mgmt server a9
Total time taken: 54702 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8689-t9650-kvm-alma9.zip
Smoke tests completed. 129 look OK, 0 have errors, 0 did not run
Only failed and skipped 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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#discussion_r1526376403
##########
test/integration/smoke/test_vm_life_cycle.py:
##########
@@ -955,6 +956,92 @@ def test_12_start_vm_multiple_volumes_allocated(self):
"Check virtual machine is in running state"
)
+ @attr(tags=["devcloud", "advanced", "advancedns", "smoke", "basic", "sg"], required_hardware="false")
+ def test_13_destroy_and_expunge_vm(self):
+ """Test destroy virtual machine with expunge parameter depending on whether the caller's role has expunge permission.
+ """
+ # Setup steps:
+ # 1. Create role with DENY expunge permission.
+ # 2. Create account with said role.
+ # 3. Create a VM of said account.
+ # 4. Create a VM of cls.account
+ # Validation steps:
+ # 1. Destroy the VM with the created account and verify it was not destroyed.
+ # 1. Destroy the other VM with cls.account and verify it was expunged.
+
+ role = Role.importRole(
+ self.apiclient,
+ {
+ "name": "MarvinFake Import Role ",
+ "type": "DomainAdmin",
+ "description": "Fake Import Domain Admin Role created by Marvin test",
+ "rules" : [{"rule":"list*", "permission":"allow","description":"Listing apis"},
+ {"rule":"get*", "permission":"allow","description":"Get apis"},
+ {"rule":"update*", "permission":"allow","description":"Update apis"},
+ {"rule":"queryAsyncJobResult", "permission":"allow","description":"Query async job result"},
+ {"rule":"deployVirtualMachine", "permission":"allow","description":"Deploy virtual machine"},
+ {"rule":"destroyVirtualMachine", "permission":"allow","description":"Destroy virtual machine"},
+ {"rule":"expungeVirtualMachine", "permission":"deny","description":"Expunge virtual machine"}]
+ },
+ )
+
+ domadm = Account.create(
+ self.apiclient,
+ self.services["account"],
+ admin=True,
+ roleid=role.id,
+ domainid=self.domain.id
+ )
+
+ self.cleanup.append(domadm)
+ self.cleanup.append(role)
Review Comment:
```suggestion
role = Role.importRole(
self.apiclient,
{
"name": "MarvinFake Import Role ",
"type": "DomainAdmin",
"description": "Fake Import Domain Admin Role created by Marvin test",
"rules" : [{"rule":"list*", "permission":"allow","description":"Listing apis"},
{"rule":"get*", "permission":"allow","description":"Get apis"},
{"rule":"update*", "permission":"allow","description":"Update apis"},
{"rule":"queryAsyncJobResult", "permission":"allow","description":"Query async job result"},
{"rule":"deployVirtualMachine", "permission":"allow","description":"Deploy virtual machine"},
{"rule":"destroyVirtualMachine", "permission":"allow","description":"Destroy virtual machine"},
{"rule":"expungeVirtualMachine", "permission":"deny","description":"Expunge virtual machine"}]
},
)
self.cleanup.append(role)
domadm = Account.create(
self.apiclient,
self.services["account"],
admin=True,
roleid=role.id,
domainid=self.domain.id
)
self.cleanup.append(domadm)
```
@gpordeus , the reason for this is that any exception in the second create would leave the first object as garbage behind.
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "gpordeus (via GitHub)" <gi...@apache.org>.
gpordeus commented on code in PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#discussion_r1537971268
##########
test/integration/smoke/test_vm_life_cycle.py:
##########
@@ -955,6 +956,92 @@ def test_12_start_vm_multiple_volumes_allocated(self):
"Check virtual machine is in running state"
)
+ @attr(tags=["devcloud", "advanced", "advancedns", "smoke", "basic", "sg"], required_hardware="false")
+ def test_13_destroy_and_expunge_vm(self):
+ """Test destroy virtual machine with expunge parameter depending on whether the caller's role has expunge permission.
+ """
+ # Setup steps:
+ # 1. Create role with DENY expunge permission.
+ # 2. Create account with said role.
+ # 3. Create a VM of said account.
+ # 4. Create a VM of cls.account
+ # Validation steps:
+ # 1. Destroy the VM with the created account and verify it was not destroyed.
+ # 1. Destroy the other VM with cls.account and verify it was expunged.
+
+ role = Role.importRole(
+ self.apiclient,
+ {
+ "name": "MarvinFake Import Role ",
+ "type": "DomainAdmin",
+ "description": "Fake Import Domain Admin Role created by Marvin test",
+ "rules" : [{"rule":"list*", "permission":"allow","description":"Listing apis"},
+ {"rule":"get*", "permission":"allow","description":"Get apis"},
+ {"rule":"update*", "permission":"allow","description":"Update apis"},
+ {"rule":"queryAsyncJobResult", "permission":"allow","description":"Query async job result"},
+ {"rule":"deployVirtualMachine", "permission":"allow","description":"Deploy virtual machine"},
+ {"rule":"destroyVirtualMachine", "permission":"allow","description":"Destroy virtual machine"},
+ {"rule":"expungeVirtualMachine", "permission":"deny","description":"Expunge virtual machine"}]
+ },
+ )
+
+ domadm = Account.create(
+ self.apiclient,
+ self.services["account"],
+ admin=True,
+ roleid=role.id,
+ domainid=self.domain.id
+ )
+
+ self.cleanup.append(domadm)
+ self.cleanup.append(role)
Review Comment:
@DaanHoogland Yes, of course, you're right.
I've added the comments and changed the testDeployVM tearDown/tearDownClass since it didn't have any cleanup order issues and it is the first test class one sees in the 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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2292967071
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10680
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2298400741
@weizhouapache are you alright with this one, now?
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2154297304
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. 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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "BryanMLima (via GitHub)" <gi...@apache.org>.
BryanMLima commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2163734772
@weizhouapache, are your concerns met?
@DaanHoogland, could you trigger the CI one last time?
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-1960897760
@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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "gpordeus (via GitHub)" <gi...@apache.org>.
gpordeus commented on code in PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#discussion_r1497796941
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -3303,17 +3305,38 @@ public UserVm rebootVirtualMachine(RebootVMCmd cmd) throws InsufficientCapacityE
return null;
}
+ /**
+ * Encapsulates AllowUserExpungeRecoverVm so we can unit test checkExpungeVmPermission.
+ */
+ protected boolean getConfigAllowUserExpungeRecoverVm(Long entityOwnerId) {
+ return AllowUserExpungeRecoverVm.valueIn(entityOwnerId);
+ }
+
+ protected void checkExpungeVmPermission (Account callingAccount, Long entityOwnerId) {
+ logger.debug(String.format("Checking if [%s] has permission for expunging VMs.", callingAccount));
+ if (!_accountMgr.isAdmin(callingAccount.getId()) && !getConfigAllowUserExpungeRecoverVm(entityOwnerId)) {
Review Comment:
Sure, it seems better. I've adjusted it.
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-1962045647
@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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2294510468
<b>[SF] Trillian test result (tid-11094)</b>
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 52183 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8689-t11094-kvm-ol8.zip
Smoke tests completed. 137 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_DeployVmAffinityGroup | `Error` | 1.45 | test_affinity_groups.py
test_01_non_strict_host_anti_affinity | `Error` | 2.63 | test_nonstrict_affinity_group.py
test_02_non_strict_host_affinity | `Error` | 1.58 | test_nonstrict_affinity_group.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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "JoaoJandre (via GitHub)" <gi...@apache.org>.
JoaoJandre commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2247784825
@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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "gpordeus (via GitHub)" <gi...@apache.org>.
gpordeus closed pull request #8689: Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow
URL: https://github.com/apache/cloudstack/pull/8689
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "BryanMLima (via GitHub)" <gi...@apache.org>.
BryanMLima commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2163740396
@DaanHoogland, my bad, I did not see the comment at https://github.com/apache/cloudstack/pull/8878#issuecomment-2162594804, you can ignore my other comments.
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2154299872
Packaging result [SF]: ✖️ el7 ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 9816
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2027242187
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. 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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2071893746
@weizhouapache are all your concerns met?
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-1983284106
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8874
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-1964246862
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8787
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-1960899608
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. 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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2097671336
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base 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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2293167667
@blueorangutan LLtest
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2293204849
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) 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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2298469757
> @DaanHoogland code lgtm
>
> not tested
good, @lucas-a-martins has: https://github.com/apache/cloudstack/pull/8689#pullrequestreview-2015694745
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-1988667559
@blueorangutan test alma9 kvm-alma9
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2287107450
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 10642
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2247788271
@JoaoJandre a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. 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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2293202682
@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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2298435476
@DaanHoogland
code lgtm
not tested
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2027330541
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9091
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "gpordeus (via GitHub)" <gi...@apache.org>.
gpordeus closed pull request #8689: Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow
URL: https://github.com/apache/cloudstack/pull/8689
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-1960979261
Packaging result [SF]: ✔️ el7 ✖️ el8 ✖️ el9 ✔️ debian ✖️ suse15. SL-JID 8745
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-1961269765
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8755
--
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
Re: [PR] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8689:
URL: https://github.com/apache/cloudstack/pull/8689#issuecomment-2031513300
@DaanHoogland a [SL] Trillian-Jenkins test job (alma9 mgmt + kvm-alma9) 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