You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2021/01/14 09:26:22 UTC
[GitHub] [cloudstack] ravening opened a new pull request #4584: Allow users to update volume name
ravening opened a new pull request #4584:
URL: https://github.com/apache/cloudstack/pull/4584
### Description
Provide an api support to update volume name by all users
Can be done both through api and UI
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [X] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [X] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [ ] Minor
- [X] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
<!-- see how your change affects other areas of the code, etc. -->
Through API
```
update volume id=3e8f9553-e1c9-4ac0-bb43-10d7d928daf2 name=test-admin
```
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #4584: Allow users to update volume name
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4584:
URL: https://github.com/apache/cloudstack/pull/4584#discussion_r560139399
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/volume/UpdateVolumeCmd.java
##########
@@ -52,30 +52,33 @@
@Parameter(name=ApiConstants.ID, type=CommandType.UUID, entityType=VolumeResponse.class, description="the ID of the disk volume")
private Long id;
- @Parameter(name = ApiConstants.PATH, type = CommandType.STRING, description = "The path of the volume")
+ @Parameter(name = ApiConstants.PATH, type = CommandType.STRING, description = "The path of the volume", authorized = {RoleType.Admin})
private String path;
@Parameter(name = ApiConstants.CHAIN_INFO,
type = CommandType.STRING,
description = "The chain info of the volume",
- since = "4.4")
+ since = "4.4", authorized = {RoleType.Admin})
private String chainInfo;
@Parameter(name = ApiConstants.STORAGE_ID,
type = CommandType.UUID,
entityType = StoragePoolResponse.class,
description = "Destination storage pool UUID for the volume",
- since = "4.3")
+ since = "4.3", authorized = {RoleType.Admin})
private Long storageId;
- @Parameter(name = ApiConstants.STATE, type = CommandType.STRING, description = "The state of the volume", since = "4.3")
+ @Parameter(name = ApiConstants.STATE, type = CommandType.STRING, description = "The state of the volume", since = "4.3", authorized = {RoleType.Admin})
private String state;
@Parameter(name = ApiConstants.DISPLAY_VOLUME,
type = CommandType.BOOLEAN,
description = "an optional field, whether to the display the volume to the end user or not.", authorized = {RoleType.Admin})
private Boolean displayVolume;
+ @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, description = "new name of the volume", since = "4.16")
Review comment:
Other parameters have sensitive info whereas the "name" parameter is not critical.
So regular users can change only name but not other important values
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on pull request #4584: Allow users to update volume name
Posted by GitBox <gi...@apache.org>.
ravening commented on pull request #4584:
URL: https://github.com/apache/cloudstack/pull/4584#issuecomment-767529262
> Hi @ravening
> why did you close this PR before merge? 🤔
@mib1185 sorry it got closed after some rebase with my branch.. I have opened new pr with ui changes in https://github.com/apache/cloudstack/pull/4618
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #4584: Allow users to update volume name
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4584:
URL: https://github.com/apache/cloudstack/pull/4584#discussion_r560139674
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/volume/UpdateVolumeCmd.java
##########
@@ -52,30 +52,33 @@
@Parameter(name=ApiConstants.ID, type=CommandType.UUID, entityType=VolumeResponse.class, description="the ID of the disk volume")
private Long id;
- @Parameter(name = ApiConstants.PATH, type = CommandType.STRING, description = "The path of the volume")
+ @Parameter(name = ApiConstants.PATH, type = CommandType.STRING, description = "The path of the volume", authorized = {RoleType.Admin})
private String path;
@Parameter(name = ApiConstants.CHAIN_INFO,
type = CommandType.STRING,
description = "The chain info of the volume",
- since = "4.4")
+ since = "4.4", authorized = {RoleType.Admin})
Review comment:
admins and regular users can run this command. but regular users can only change name
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening closed pull request #4584: Allow users to update volume name
Posted by GitBox <gi...@apache.org>.
ravening closed pull request #4584:
URL: https://github.com/apache/cloudstack/pull/4584
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #4584: Allow users to update volume name
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #4584:
URL: https://github.com/apache/cloudstack/pull/4584#issuecomment-762843611
code lgtm
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on pull request #4584: Allow users to update volume name
Posted by GitBox <gi...@apache.org>.
ravening commented on pull request #4584:
URL: https://github.com/apache/cloudstack/pull/4584#issuecomment-762808382
> I am trying to understand in which scenario it is necessary to give freedom for ordinary users to change names within the environment.
> You can explain it to me because I couldn't reach a conclusion on my own.
@RodrigoDLopez there is no harm in changing the volume name. so anybody can do it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] mib1185 commented on pull request #4584: Allow users to update volume name
Posted by GitBox <gi...@apache.org>.
mib1185 commented on pull request #4584:
URL: https://github.com/apache/cloudstack/pull/4584#issuecomment-767509741
Hi @ravening
why did you close this PR before merge? 🤔
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] RodrigoDLopez commented on a change in pull request #4584: Allow users to update volume name
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez commented on a change in pull request #4584:
URL: https://github.com/apache/cloudstack/pull/4584#discussion_r558354265
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/volume/UpdateVolumeCmd.java
##########
@@ -52,30 +52,33 @@
@Parameter(name=ApiConstants.ID, type=CommandType.UUID, entityType=VolumeResponse.class, description="the ID of the disk volume")
private Long id;
- @Parameter(name = ApiConstants.PATH, type = CommandType.STRING, description = "The path of the volume")
+ @Parameter(name = ApiConstants.PATH, type = CommandType.STRING, description = "The path of the volume", authorized = {RoleType.Admin})
private String path;
@Parameter(name = ApiConstants.CHAIN_INFO,
type = CommandType.STRING,
description = "The chain info of the volume",
- since = "4.4")
+ since = "4.4", authorized = {RoleType.Admin})
Review comment:
Correct me if I'm wrong, but I believe that only administrators can execute the updateVolume command.
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/volume/UpdateVolumeCmd.java
##########
@@ -52,30 +52,33 @@
@Parameter(name=ApiConstants.ID, type=CommandType.UUID, entityType=VolumeResponse.class, description="the ID of the disk volume")
private Long id;
- @Parameter(name = ApiConstants.PATH, type = CommandType.STRING, description = "The path of the volume")
+ @Parameter(name = ApiConstants.PATH, type = CommandType.STRING, description = "The path of the volume", authorized = {RoleType.Admin})
private String path;
@Parameter(name = ApiConstants.CHAIN_INFO,
type = CommandType.STRING,
description = "The chain info of the volume",
- since = "4.4")
+ since = "4.4", authorized = {RoleType.Admin})
private String chainInfo;
@Parameter(name = ApiConstants.STORAGE_ID,
type = CommandType.UUID,
entityType = StoragePoolResponse.class,
description = "Destination storage pool UUID for the volume",
- since = "4.3")
+ since = "4.3", authorized = {RoleType.Admin})
private Long storageId;
- @Parameter(name = ApiConstants.STATE, type = CommandType.STRING, description = "The state of the volume", since = "4.3")
+ @Parameter(name = ApiConstants.STATE, type = CommandType.STRING, description = "The state of the volume", since = "4.3", authorized = {RoleType.Admin})
private String state;
@Parameter(name = ApiConstants.DISPLAY_VOLUME,
type = CommandType.BOOLEAN,
description = "an optional field, whether to the display the volume to the end user or not.", authorized = {RoleType.Admin})
private Boolean displayVolume;
+ @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, description = "new name of the volume", since = "4.16")
Review comment:
You put roleAdmin in all the other parameters of this API, but not this one.
Any reason for this behavior? users without administrator permission will not be able to run this command. And ordinary users shouldn't be able to change the name of what doesn't belong to them.
It would be nice to capitalize the n in new as well
##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1763,7 +1763,15 @@ public Volume attachVolumeToVM(Long vmId, Long volumeId, Long deviceId) {
@Override
@ActionEvent(eventType = EventTypes.EVENT_VOLUME_UPDATE, eventDescription = "updating volume", async = true)
- public Volume updateVolume(long volumeId, String path, String state, Long storageId, Boolean displayVolume, String customId, long entityOwnerId, String chainInfo) {
+ public Volume updateVolume(long volumeId, String path, String state, Long storageId, Boolean displayVolume,
+ String customId, long entityOwnerId, String chainInfo, String name) {
+
+ Account caller = CallContext.current().getCallingAccount();
+ if (!_accountMgr.isRootAdmin(caller.getId())) {
+ if (path != null || state != null || storageId != null || displayVolume != null || customId != null || chainInfo != null) {
+ throw new InvalidParameterValueException("The domain admin and normal user are not allowed to update volume except volume name");
Review comment:
does it make sense to allow ordinary users the power to change names within the environment?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org