incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Rohit Yadav" <rohit.ya...@citrix.com>
Subject Re: Review Request: CLOUDSTACK-84: FIX NPE error in listRouter etc. after deleting a user project.
Date Sun, 07 Oct 2012 06:40:09 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message