From 9ef6f9ff54c25e39f1bc8a2f7b0a8b650ff65158 Mon Sep 17 00:00:00 2001 From: alena Date: Tue, 3 May 2011 11:38:01 -0700 Subject: [PATCH] bug 9663: do account permission check against the volume when create snapshot status 9663: resolved fixed Conflicts: api/src/com/cloud/storage/snapshot/SnapshotService.java --- .../cloud/api/commands/CreateSnapshotCmd.java | 68 ++++++------ .../storage/snapshot/SnapshotService.java | 19 ++-- .../storage/snapshot/SnapshotManager.java | 103 +++++++++--------- .../storage/snapshot/SnapshotManagerImpl.java | 29 ++--- 4 files changed, 105 insertions(+), 114 deletions(-) diff --git a/api/src/com/cloud/api/commands/CreateSnapshotCmd.java b/api/src/com/cloud/api/commands/CreateSnapshotCmd.java index d8fd41c9cd8..d882ba9705e 100755 --- a/api/src/com/cloud/api/commands/CreateSnapshotCmd.java +++ b/api/src/com/cloud/api/commands/CreateSnapshotCmd.java @@ -34,30 +34,30 @@ import com.cloud.storage.Snapshot; import com.cloud.storage.Volume; import com.cloud.user.Account; -@Implementation(description="Creates an instant snapshot of a volume.", responseObject=SnapshotResponse.class) +@Implementation(description = "Creates an instant snapshot of a volume.", responseObject = SnapshotResponse.class) public class CreateSnapshotCmd extends BaseAsyncCreateCmd { - public static final Logger s_logger = Logger.getLogger(CreateSnapshotCmd.class.getName()); - private static final String s_name = "createsnapshotresponse"; + public static final Logger s_logger = Logger.getLogger(CreateSnapshotCmd.class.getName()); + private static final String s_name = "createsnapshotresponse"; - ///////////////////////////////////////////////////// - //////////////// API parameters ///////////////////// - ///////////////////////////////////////////////////// + // /////////////////////////////////////////////////// + // ////////////// API parameters ///////////////////// + // /////////////////////////////////////////////////// - @Parameter(name=ApiConstants.ACCOUNT, type=CommandType.STRING, description="The account of the snapshot. The account parameter must be used with the domainId parameter.") + @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, description = "The account of the snapshot. The account parameter must be used with the domainId parameter.") private String accountName; - @Parameter(name=ApiConstants.DOMAIN_ID, type=CommandType.LONG, description="The domain ID of the snapshot. If used with the account parameter, specifies a domain for the account associated with the disk volume.") + @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.LONG, description = "The domain ID of the snapshot. If used with the account parameter, specifies a domain for the account associated with the disk volume.") private Long domainId; - @Parameter(name=ApiConstants.VOLUME_ID, type=CommandType.LONG, required=true, description="The ID of the disk volume") + @Parameter(name = ApiConstants.VOLUME_ID, type = CommandType.LONG, required = true, description = "The ID of the disk volume") private Long volumeId; - @Parameter(name=ApiConstants.POLICY_ID, type=CommandType.LONG, description="policy id of the snapshot, if this is null, then use MANUAL_POLICY.") + @Parameter(name = ApiConstants.POLICY_ID, type = CommandType.LONG, description = "policy id of the snapshot, if this is null, then use MANUAL_POLICY.") private Long policyId; - - ///////////////////////////////////////////////////// - /////////////////// Accessors /////////////////////// - ///////////////////////////////////////////////////// + + // /////////////////////////////////////////////////// + // ///////////////// Accessors /////////////////////// + // /////////////////////////////////////////////////// public String getAccountName() { return accountName; @@ -70,26 +70,26 @@ public class CreateSnapshotCmd extends BaseAsyncCreateCmd { public Long getVolumeId() { return volumeId; } - + public Long getPolicyId() { - if( policyId != null) { - return policyId; - } else { - return Snapshot.MANUAL_POLICY_ID; - } + if (policyId != null) { + return policyId; + } else { + return Snapshot.MANUAL_POLICY_ID; + } } - ///////////////////////////////////////////////////// - /////////////// API Implementation/////////////////// - ///////////////////////////////////////////////////// + // /////////////////////////////////////////////////// + // ///////////// API Implementation/////////////////// + // /////////////////////////////////////////////////// @Override public String getCommandName() { return s_name; } - + public static String getResultObjectName() { - return "snapshot"; + return "snapshot"; } @Override @@ -110,27 +110,27 @@ public class CreateSnapshotCmd extends BaseAsyncCreateCmd { @Override public String getEventDescription() { - return "creating snapshot for volume: " + getVolumeId(); + return "creating snapshot for volume: " + getVolumeId(); } - + @Override public AsyncJob.Type getInstanceType() { - return AsyncJob.Type.Snapshot; + return AsyncJob.Type.Snapshot; } - + @Override - public void create() throws ResourceAllocationException{ - Snapshot snapshot = _snapshotService.allocSnapshot(this); + public void create() throws ResourceAllocationException { + Snapshot snapshot = _snapshotService.allocSnapshot(getVolumeId(), getPolicyId()); if (snapshot != null) { this.setEntityId(snapshot.getId()); } else { throw new ServerApiException(BaseCmd.INTERNAL_ERROR, "Failed to create snapshot"); } } - + @Override - public void execute() { - Snapshot snapshot = _snapshotService.createSnapshot(this); + public void execute() { + Snapshot snapshot = _snapshotService.createSnapshot(getVolumeId(), getPolicyId(), getEntityId()); if (snapshot != null) { SnapshotResponse response = _responseGenerator.createSnapshotResponse(snapshot); response.setResponseName(getCommandName()); diff --git a/api/src/com/cloud/storage/snapshot/SnapshotService.java b/api/src/com/cloud/storage/snapshot/SnapshotService.java index b5ca0eb2ab4..c72f1ac358c 100644 --- a/api/src/com/cloud/storage/snapshot/SnapshotService.java +++ b/api/src/com/cloud/storage/snapshot/SnapshotService.java @@ -19,7 +19,6 @@ package com.cloud.storage.snapshot; import java.util.List; -import com.cloud.api.commands.CreateSnapshotCmd; import com.cloud.api.commands.CreateSnapshotPolicyCmd; import com.cloud.api.commands.DeleteSnapshotCmd; import com.cloud.api.commands.DeleteSnapshotPoliciesCmd; @@ -31,14 +30,6 @@ import com.cloud.exception.ResourceAllocationException; import com.cloud.storage.Snapshot; public interface SnapshotService { - /** - * Create a snapshot of a volume - * - * @param cmd - * the API command wrapping the parameters for creating the snapshot (mainly volumeId) - * @return the Snapshot that was created - */ - Snapshot createSnapshot(CreateSnapshotCmd cmd); /** * List all snapshots of a disk volume. Optionally lists snapshots created by specified interval @@ -87,6 +78,14 @@ public interface SnapshotService { boolean deleteSnapshotPolicies(DeleteSnapshotPoliciesCmd cmd); - Snapshot allocSnapshot(CreateSnapshotCmd cmd) throws ResourceAllocationException; + Snapshot allocSnapshot(Long volumeId, Long policyId) throws ResourceAllocationException; + /** + * Create a snapshot of a volume + * + * @param cmd + * the API command wrapping the parameters for creating the snapshot (mainly volumeId) + * @return the Snapshot that was created + */ + Snapshot createSnapshot(Long volumeId, Long policyId, Long snapshotId); } diff --git a/server/src/com/cloud/storage/snapshot/SnapshotManager.java b/server/src/com/cloud/storage/snapshot/SnapshotManager.java index 8d8422089f1..253cc61a94e 100755 --- a/server/src/com/cloud/storage/snapshot/SnapshotManager.java +++ b/server/src/com/cloud/storage/snapshot/SnapshotManager.java @@ -22,52 +22,49 @@ import java.util.List; import com.cloud.exception.ResourceAllocationException; import com.cloud.storage.SnapshotPolicyVO; import com.cloud.storage.SnapshotVO; -import com.cloud.storage.Storage.ImageFormat; import com.cloud.storage.VolumeVO; import com.cloud.utils.db.Filter; /** -* -* SnapshotManager contains all of the code to work with volume snapshots. -* -*/ + * + * SnapshotManager contains all of the code to work with volume snapshots. + * + */ public interface SnapshotManager { - - public static final int HOURLYMAX = 8; - public static final int DAILYMAX = 8; - public static final int WEEKLYMAX = 8; - public static final int MONTHLYMAX = 12; - public static final int DELTAMAX = 16; - - /** - * Create a snapshot of a volume - * @param cmd the API command wrapping the parameters for creating the snapshot (mainly volumeId) - * @return the Snapshot that was created - */ - SnapshotVO createSnapshotImpl(Long volumeId, Long policyId, Long snapshotId); + + public static final int HOURLYMAX = 8; + public static final int DAILYMAX = 8; + public static final int WEEKLYMAX = 8; + public static final int MONTHLYMAX = 12; + public static final int DELTAMAX = 16; /** - * After successfully creating a snapshot of a volume, copy the snapshot to the secondary storage for - * 1) reliability - * 2) So that storage space on Primary is conserved. - * @param snapshot Info about the created snapshot on primary storage. - * @param startEventId event id of the scheduled event for this snapshot - * @return True if the snapshot was successfully backed up. + * After successfully creating a snapshot of a volume, copy the snapshot to the secondary storage for 1) reliability 2) So + * that storage space on Primary is conserved. + * + * @param snapshot + * Info about the created snapshot on primary storage. + * @param startEventId + * event id of the scheduled event for this snapshot + * @return True if the snapshot was successfully backed up. */ public boolean backupSnapshotToSecondaryStorage(SnapshotVO snapshot); - + /** - * Once a snapshot has completed, - * 1) If success, update the database entries - * 2) If success and there are excess snapshots for any of the policies given, delete the oldest one. - * 3) Schedule the next recurring snapshot. - * @param volumeId The volume for which the snapshot is being taken - * @param snapshotId The snapshot which has just completed - * @param policyIds The list of policyIds to which this snapshot belongs to - * @param backedUp If true, the snapshot has been successfully created. + * Once a snapshot has completed, 1) If success, update the database entries 2) If success and there are excess snapshots + * for any of the policies given, delete the oldest one. 3) Schedule the next recurring snapshot. + * + * @param volumeId + * The volume for which the snapshot is being taken + * @param snapshotId + * The snapshot which has just completed + * @param policyIds + * The list of policyIds to which this snapshot belongs to + * @param backedUp + * If true, the snapshot has been successfully created. */ void postCreateSnapshot(Long volumeId, Long snapshotId, Long policyId, boolean backedUp); - + /** * Destroys the specified snapshot from secondary storage */ @@ -77,41 +74,41 @@ public interface SnapshotManager { * Deletes snapshot scheduling policy. Delete will fail if this policy is assigned to one or more volumes */ boolean deletePolicy(long userId, Long policyId); - + /** * Lists all snapshots for the volume which are created using schedule of the specified policy */ /* - List listSnapsforPolicy(long policyId, Filter filter); - */ + * List listSnapsforPolicy(long policyId, Filter filter); + */ /** * List all policies which are assigned to the specified volume */ List listPoliciesforVolume(long volumeId); /** - * List all policies to which a specified snapshot belongs. For ex: A snapshot - * may belong to a hourly snapshot and a daily snapshot run at the same time + * List all policies to which a specified snapshot belongs. For ex: A snapshot may belong to a hourly snapshot and a daily + * snapshot run at the same time */ /* - List listPoliciesforSnapshot(long snapshotId); - */ + * List listPoliciesforSnapshot(long snapshotId); + */ /** - * List all snapshots for a specified volume irrespective of the policy which - * created the snapshot + * List all snapshots for a specified volume irrespective of the policy which created the snapshot */ List listSnapsforVolume(long volumeId); - void deletePoliciesForVolume(Long volumeId); + void deletePoliciesForVolume(Long volumeId); /** - * For each of the volumes in the account, - * (which can span across multiple zones and multiple secondary storages), - * delete the dir on the secondary storage which contains the backed up snapshots for that volume. - * This is called during deleteAccount. - * @param accountId The account which is to be deleted. + * For each of the volumes in the account, (which can span across multiple zones and multiple secondary storages), delete + * the dir on the secondary storage which contains the backed up snapshots for that volume. This is called during + * deleteAccount. + * + * @param accountId + * The account which is to be deleted. */ - boolean deleteSnapshotDirsForAccount(long accountId); + boolean deleteSnapshotDirsForAccount(long accountId); void validateSnapshot(Long userId, SnapshotVO snapshot); @@ -121,10 +118,12 @@ public interface SnapshotManager { /** * Create a snapshot of a volume - * @param cmd the API command wrapping the parameters for creating the snapshot (mainly volumeId) + * + * @param cmd + * the API command wrapping the parameters for creating the snapshot (mainly volumeId) * @return the Snapshot that was created */ - SnapshotVO createSnapshotOnPrimary(VolumeVO volume, Long polocyId, Long snapshotId) throws ResourceAllocationException; + SnapshotVO createSnapshotOnPrimary(VolumeVO volume, Long polocyId, Long snapshotId) throws ResourceAllocationException; List listPoliciesforSnapshot(long snapshotId); diff --git a/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java b/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java index 58c58946099..0286f78da99 100755 --- a/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java +++ b/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java @@ -38,7 +38,6 @@ import com.cloud.agent.api.DeleteSnapshotBackupCommand; import com.cloud.agent.api.DeleteSnapshotsDirCommand; import com.cloud.agent.api.ManageSnapshotAnswer; import com.cloud.agent.api.ManageSnapshotCommand; -import com.cloud.api.commands.CreateSnapshotCmd; import com.cloud.api.commands.CreateSnapshotPolicyCmd; import com.cloud.api.commands.DeleteSnapshotCmd; import com.cloud.api.commands.DeleteSnapshotPoliciesCmd; @@ -229,10 +228,10 @@ public class SnapshotManagerImpl implements SnapshotManager, SnapshotService, Ma Thread.sleep(_pauseInterval * 1000); } catch (InterruptedException e) { } - + s_logger.debug("Retrying..."); } - + s_logger.warn("After " + _totalRetries + " retries, the command " + cmd.getClass().getName() + " did not succeed."); return null; @@ -327,7 +326,8 @@ public class SnapshotManagerImpl implements SnapshotManager, SnapshotService, Ma @Override @DB - public SnapshotVO createSnapshotImpl(Long volumeId, Long policyId, Long snapshotId) { + @ActionEvent(eventType = EventTypes.EVENT_SNAPSHOT_CREATE, eventDescription = "creating snapshot", async = true) + public SnapshotVO createSnapshot(Long volumeId, Long policyId, Long snapshotId) { VolumeVO v = _volsDao.findById(volumeId); Account owner = _accountMgr.getAccount(v.getAccountId()); SnapshotVO snapshot = null; @@ -419,15 +419,6 @@ public class SnapshotManagerImpl implements SnapshotManager, SnapshotService, Ma return snapshot; } - @Override - @ActionEvent(eventType = EventTypes.EVENT_SNAPSHOT_CREATE, eventDescription = "creating snapshot", async = true) - public SnapshotVO createSnapshot(CreateSnapshotCmd cmd) { - Long volumeId = cmd.getVolumeId(); - Long policyId = cmd.getPolicyId(); - Long snapshotId = cmd.getEntityId(); - return createSnapshotImpl(volumeId, policyId, snapshotId); - } - private SnapshotVO updateDBOnCreate(Long id, String snapshotPath, long preSnapshotId) { SnapshotVO createdSnapshot = _snapshotDao.findByIdIncludingRemoved(id); createdSnapshot.setPath(snapshotPath); @@ -1169,21 +1160,23 @@ public class SnapshotManagerImpl implements SnapshotManager, SnapshotService, Ma } @Override - public SnapshotVO allocSnapshot(CreateSnapshotCmd cmd) throws ResourceAllocationException { - Long volumeId = cmd.getVolumeId(); - Long policyId = cmd.getPolicyId(); + public SnapshotVO allocSnapshot(Long volumeId, Long policyId) throws ResourceAllocationException { + Account caller = UserContext.current().getCaller(); VolumeVO volume = _volsDao.findById(volumeId); if (volume == null) { - throw new CloudRuntimeException("Creating snapshot failed due to volume:" + volumeId + " doesn't exist"); + throw new InvalidParameterValueException("Creating snapshot failed due to volume:" + volumeId + " doesn't exist"); } if (volume.getState() != Volume.State.Ready) { - throw new InvalidParameterValueException("VolumeId: " + volumeId + " is not in Created state but " + volume.getState() + ". Cannot take snapshot."); + throw new InvalidParameterValueException("VolumeId: " + volumeId + " is not in " + Volume.State.Ready + " state but " + volume.getState() + ". Cannot take snapshot."); } StoragePoolVO storagePoolVO = _storagePoolDao.findById(volume.getPoolId()); if (storagePoolVO == null) { throw new InvalidParameterValueException("VolumeId: " + volumeId + " please attach this volume to a VM before create snapshot for it"); } + // Verify permissions + _accountMgr.checkAccess(caller, volume); + Account owner = _accountMgr.getAccount(volume.getAccountId()); if (_accountMgr.resourceLimitExceeded(owner, ResourceType.snapshot)) { ResourceAllocationException rae = new ResourceAllocationException("Maximum number of snapshots for account: " + owner.getAccountName() + " has been exceeded.");