> On Oct. 6, 2012, 4:42 p.m., Rohit Yadav wrote:
> > Abhinav suggests on the issue the deleting VR still fails.
> > This is expected as the commit e1dc0024ea5d869dd9945b920ba85dd2a24a51c1
> > only fixes issues on the API layer.
> > The deletion fails on ACL. Please see diff again on this review and let's
> > work out a solution.
>
> Alena Prokharchyk wrote:
> Rohit, I looked at the exception, it's a completely different error now.
> I reviewed your patch, the solution still will break the current ACL
> concepts. We can't do "includingRemoved" in the domain checker. Domain
> checker is used by the API layer, and with the fix proposed, the following
> case would fail (I mentioned it earlier):
>
>
> * have removed account. The removed account has some detached volume and
> user vm that weren't cleaned up yet
> * As ROOT admin, attach account's volume to account's vm. The patch makes
> it possible while we should allow just LISTING the resources belonging to the
> removed account, but never allow to manipulate/create/delete them.
>
>
> The fix in this case should be this: when deleteRouter/deleteNetwork call
> is being made by periodic thread that cleans up VRs/Networks belonging to the
> project/account, no ACL check should be made at all because the caller in
> this case is System user. This part of the code shouldn't have been even hit:
>
> if (caller.getType() == Account.ACCOUNT_TYPE_NORMAL) {
> Account account =
> _accountDao.findById(entity.getAccountId());
> .......
>
>
> You have to find out why caller of the destroyAccount call in
> VirtualNetworkApplianceImpl (executed as a part of cleanup project) is not
> System user, but Acct[3-user1] and fix this particular problem:
>
> com.cloud.exception.PermissionDeniedException: Acct[3-user1] does not
> have permission to operate with resource VM[DomainRouter|r-6-VM]
>
> After the caller passed to destroyCommand is system user, no check will
> be made, and the router will be deleted.
>
Alright, closed 84, filed new issue:
https://issues.apache.org/jira/browse/CLOUDSTACK-279
Alena, the caller is the useraccount which requested deletion, deletion of VR
is not async in separate thread. If you follow the code, it first marks the
project as deleted and moves on to cleanup resources. It fails to delete VR as
the account is marked removed and user has no acl to delete VR. If we don't
mark the project as removed, it will cause a lot more problem; for example if
it failed in between it won't cleanup remaining resources next time. One fix
can be to check that if one entity's (VR in this case) owner is removed, it
should be removed but I'm not sure about possible side effects, pl. advise?
- Rohit
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7168/#review12205
-----------------------------------------------------------
On Sept. 19, 2012, 3:38 p.m., Rohit Yadav wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7168/
> -----------------------------------------------------------
>
> (Updated Sept. 19, 2012, 3:38 p.m.)
>
>
> Review request for cloudstack, Abhinandan Prateek, Kishan Kavala, Nitin
> Mehta, Alena Prokharchyk, and Alex Huang.
>
>
> Description
> -------
>
> Domain ACL information should be valid even if account entry is marked
> removed. Patch fixes how account is obtained based on accountId, it
> finds among those entries which are marked deleted.
>
> In case of project deletion, the project is marked removed first and
> then each of its elements are cleared/cleaned/deleted. While deleting
> network and router it failed because ACL only checks accounts which are
> not marked deleted.
>
> Download original patch and git am <patch>:
> http://patchbin.baagi.org/p?id=40pdym
>
>
> This addresses bug CLOUDSTACK-84.
>
>
> Diffs
> -----
>
> server/src/com/cloud/acl/DomainChecker.java 6bc2cd3
> server/src/com/cloud/user/dao/AccountDao.java 3b7fa66
> server/src/com/cloud/user/dao/AccountDaoImpl.java 7300bb1
>
> Diff: https://reviews.apache.org/r/7168/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Rohit Yadav
>
>
