Added recursive fetch of child domains for listUsageRecords API call #4717
Conversation
|
@blueorangutan package |
Spaceman1984
changed the title
api/src/main/java/org/apache/cloudstack/api/command/admin/usage/ListUsageRecordsCmd.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@Spaceman1984 I see a few issues. Please see the comments.
I see probable failures or wrong results:
- Domain admin calling API for an account from its subdomain
- Domain admin calling API for an account in its domain passed with accountid param while using recursive=true
|
@blueorangutan package |
|
@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
@blueorangutan package |
|
@blueorangutan test |
|
@Spaceman1984 can you test if it works as before if the domain admin can list usage records for an account within the (a) same domain, (b) a sub-domain, and (c) a different domain (the domain admin is not admin of that domain). |
| } | ||
|
|
||
| // Check if a domain admin is allowed to access the requested account info. | ||
| Account account = _accountService.getAccount(accountId); |
There was a problem hiding this comment.
@Spaceman1984 should we do a checkAccess() here as well with the provided caller and account?
There was a problem hiding this comment.
@rhtyd Using checkAccess() here will result in querying the database for every child domain. I'm querying the database once by asking for all the child domains of the current domain and checking if the requested account belongs to anyone of them.
| for (DomainVO domain : domains) | ||
| domainIds.add(domain.getId()); | ||
| sc.addAnd("domainId", SearchCriteria.Op.IN, domainIds.toArray()); | ||
| if (accountId != -1 && accountId != Account.ACCOUNT_ID_SYSTEM && !isAdmin) { |
There was a problem hiding this comment.
I don't understand, how do you mean?
| if (cmd.isRecursive()) { | ||
| SearchCriteria<DomainVO> sdc = _domainDao.createSearchCriteria(); | ||
| sdc.addOr("path", SearchCriteria.Op.LIKE, _domainDao.findById(domainId).getPath() + "%"); | ||
| List<DomainVO> domains = _domainDao.search(sdc, null); |
There was a problem hiding this comment.
Simplify, reuse existing DomainDao::findAllChildren
There was a problem hiding this comment.
@rhtyd Using findAllChildren here would not pass the top domain.
@rhtyd I have already tested these scenarios. The (c) case should fail. |
|
@rhtyd I don't care if it goes in 4.15 or not, as it is a minor enhancement. I wouldn't compare listUsageRecords to other list APIs though. Most testing done and ok. I just want to see another test run and am busy with that. |
|
|
||
| // By default users do not have access to this API. | ||
| // Adding checks here in case someone changes the default access. | ||
| checkUserAccess(cmd, accountId, caller, isNormalUser); |
There was a problem hiding this comment.
@Spaceman1984 is this method call required for domain admin ?
|
Trillian test result (tid-232)
|
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✖️ centos7 ✖️ centos8 ✖️ debian. SL-JID 224 |
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✖️ centos7 ✖️ centos8 ✖️ debian. SL-JID 228 |
|
Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 232 |
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-247)
|
|
Trillian test result (tid-262)
|
|
Is this ready for merging @DaanHoogland @Spaceman1984 ? |
|
Not until IR is done. |
|
Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 309 |
|
Trillian test result (tid-331)
|
|
@andrijapanicsb are you lgtm on this? |
|
LGTM |
Description
When calling the listUasageRecords API records per domain are fetched recursively. This is not the case if you specify a domain id.
This PR adds a new parameter to enable fetching records recursively (isRecursive) when passing the domain id.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Screenshots (if appropriate):
How Has This Been Tested?
This has been tested by calling the API with different parameters. With/without a domain id, with/without isRecursive and with/without an account id.
I have tested by calling the API logged in as a root admin, domain admin, and a user.