[WIP] CLOUDSTACK-6276: project support in affinitygroups

[resmo]

No description provided.

[asfbot]

cloudstack-pull-requests #623 FAILURE
Looks like there's a problem with this pull request

[asfbot]

cloudstack-pull-requests #624 FAILURE
Looks like there's a problem with this pull request

[asfbot]

cloudstack-pull-requests #625 FAILURE
Looks like there's a problem with this pull request

[asfbot]

cloudstack-pull-requests #626 FAILURE
Looks like there's a problem with this pull request

[asfbot]

cloudstack-pull-requests #627 FAILURE
Looks like there's a problem with this pull request

[asfbot]

cloudstack-pull-requests #645 FAILURE
Looks like there's a problem with this pull request

[rohityadavcloud]

ping, any update on this? still WIP?

[resmo]

hey @bhaisaab, still WIP. I ask for some help to finish it on the ML, because I will not have time to work on it the next months.

[wilderrodrigues]

Hi @resmo and @bhaisaab

I will check with @remibergsma / @DaanHoogland / @snuf if that's something we could get involved in. I'm okay with helping, as long as I have some time dedicated in our sprints.

Would you have some more details on how far you are, @resmo ? Will it break existing functionality if we merge? If not and if I get the time, we can get it merge and I will continue from where you left.

Cheers,
Wilder

[resmo]

Hi @wilderrodrigues that would be awesome!

It is not supposed to break backward compatibility. In my opinion the functionality is implemented and should work, but I hadn't a change to really test it and the current tests are not green as well. So adding unit tests and looking at the current unit tests is needed.

As it is my first serious work in cloudstack, don't hesitate to correct any mistake I made. :)

[wilderrodrigues]

Hi @resmo

Can I then get your changes, from your PR, work on it and push via another PR? Would that be okay with you?

If so, I think we could close this PR. What do you think? @bhaisaab @DaanHoogland your thoughts on that...?

Cheers,
Wilder

[DaanHoogland]

@wilderrodrigues as long as you don't squash them and keep the attribution intact, i'm fine with that. otherwise you can also make pull requests to resmo's fork and let him add it to his PR

[resmo]

@wilderrodrigues yes, please. Feel free.

[resmo]

@wilderrodrigues did you take it over? can I close this PR?

[wilderrodrigues]

I will start on it this week, hopefully! Just keep the PR open, I will get it and create a PR on top of your with the tests. In this way we keep track of everything

Cheers,
Wilder

[resmo]

@wilderrodrigues ok. thx!

[pdube]

Hey @wilderrodrigues @resmo,

I was looking to fixing this as well. Is there any way I can contribute? What is the status of this?

Thanks,
pdube

[ustcweizhou]

@pdube you can create a pull request on github when you finish the coding and testing.
This PR have lots of issues, so it will not be merged of course.
I have similar fix for cloudstack 4.2.1.

[pdube]

@ustcweizhou I have checked it out into another branch. I am working on it. And will create a PR when I am done.

[pdube]

@ustcweizhou Also if you have a fix for it, why not port it forward?

[ustcweizhou]

@pdube If you are almost done, I will wait for your PR. Otherwise, I can work on porting it.

[pdube]

@ustcweizhou I am working on it now, but have a few questions about resource ownership

[ustcweizhou]

@pdube

try this change:

diff --git a/server/src/com/cloud/acl/AffinityGroupAccessChecker.java b/server/src/com/cloud/acl/AffinityGroupAccessChecker.java
index 57f7b37..c6e44d6 100644
--- a/server/src/com/cloud/acl/AffinityGroupAccessChecker.java
+++ b/server/src/com/cloud/acl/AffinityGroupAccessChecker.java
@@ -30,6 +30,9 @@ import org.apache.cloudstack.affinity.dao.AffinityGroupDomainMapDao;

 import com.cloud.domain.DomainVO;
 import com.cloud.exception.PermissionDeniedException;
+import com.cloud.projects.ProjectVO;
+import com.cloud.projects.dao.ProjectAccountDao;
+import com.cloud.projects.dao.ProjectDao;
 import com.cloud.user.Account;
 import com.cloud.user.AccountManager;
 import com.cloud.utils.exception.CloudRuntimeException;
@@ -44,6 +47,10 @@ public class AffinityGroupAccessChecker extends DomainChecker {
     AccountManager _accountMgr;
     @Inject
     AffinityGroupDomainMapDao _affinityGroupDomainMapDao;
+    @Inject
+    ProjectDao _projectDao;
+    @Inject
+    ProjectAccountDao _projectAccountDao;

     @Override
     public boolean checkAccess(Account caller, ControlledEntity entity, AccessType accessType) throws PermissionDeniedException {
@@ -72,6 +79,15 @@ public class AffinityGroupAccessChecker extends DomainChecker {
             } else {
                 //acl_type account
                 if (caller.getId() != group.getAccountId()) {
+                    //check if the group belongs to a project
+                    ProjectVO project = _projectDao.findByProjectAccountId(group.getAccountId());
+                    if (project != null) {
+                        if (AccessType.ModifyProject.equals(accessType) && _projectAccountDao.canModifyProjectAccount(caller.getId(), group.getAccountId())) {
+                            return true;
+                        } else if (!AccessType.ModifyProject.equals(accessType) && _projectAccountDao.canAccessProjectAccount(caller.getId(), group.getAccountId())) {
+                            return true;
+                        }
+                    }
                     throw new PermissionDeniedException(caller + " does not have permission to operate with resource " + entity);
                 } else {
                     return true;

[pdube]

@ustcweizhou Is that actually necessary?

[pdube]

Why is that access control done differently?

[ustcweizhou]

@pdube
you may notice this code in UserVmManagerImpl.java
_accountMgr.checkAccess(caller, null, true, owner, ag);

normally it will check
(1) caller can access owner (project account of project)
(2) caller can access/modify ag (affinity group)
(3) The owner (project account of project) and the account of ag are the same.

I guess you have modified the account of ag (when create an affinity group from project view) from account to project account of project, If not, the (3) will fail, sure.
If yes, the (2) will fail, because affinity group has special AccessChecker that the caller can only be the account of the affinity group. However, when we log in as Account A and go to Project view, the call is A, not the project account of the project.

Do you need my help to fix the issues or test the changes ?

[pdube]

@ustcweizhou Thanks for the response. I understand the code. I was wondering why there was a need for a special access checker for AffinityGroups. After more investigation, it seems like the DomainChecker explicitly fails for affinity groups, which seems a bit strange.

[ustcweizhou]

@pdube
me either, I donot know why/who designed it, but I will not change it.
you may notice the DomainChecked return false if the entity is AffinityGroup.

By design, the AffinityGroupAccessChecked check the access for affinity groups. only the owner of the affinity group can use the ag, others (like domain admin) cannot.
For other entities, it is possible.

[pdube]

@ustcweizhou Does this mean that the root admin account cannot create a vm with an affinity group owned by a user?

[ustcweizhou]

@pdube
you are right.
system/root admin can access every entity, but root admin cannot create a vm using other account' affinity groups.

Affinity Group AffinityGroup[6f164261-99b7-4333-a9b4-0103888e9d94] does not belong to the VM's account