cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rohit Yadav <rohityada...@gmail.com>
Subject Re: Why to have API Commands for Admin actions?
Date Tue, 24 Jun 2014 20:41:09 GMT
Hi Wilder,


On Tue, Jun 24, 2014 at 5:25 PM, Wilder Rodrigues <
WRodrigues@schubergphilis.com> wrote:

> Hi all,
>
> We are currently working on the redundant VPC implementation. In order to
> take the right steps from the beginning, we started analyzing the existing
> code base, from the API commands into the
> VPCVirtualNetworkAppliancaManagerImpl.
>
> Although it's not related to the feature itself, we found out that the
> current way of using the APICommand annotation and the CreateVPCCmd class
> (and its derived) is not really clear. For example, there are 2 command
> classes to create a VPC. The difference between them is: one has
> ResponseView.Full parameter in the @APICommand (ie. CreateVPCCmdByAdmin);
> and the other has ResponseView.Rstricted parameter in the @APICommand (i.e.
> CreateVPCCmd). Moreover, the call to responseGenerator.createVpcresponde()
> method uses a ResponseView enum according to what has been specified in the
> annotation parameter.
>
> We understand that having a different enum in the
> responseGenerator.createVpcresponde() method will affect many things,
> because it goes deep into the code until reaches the APIDBUtils and the
> database. It is also checked in the ApiServer class, when the command is
> evaluated based on a string passed to the getCmdClass() method.
>

You're right.


> Since we can identify the user in the account manager, also checking the
> kind of access the user has, what is the point in having the Annotation +
> the Enum in the code?


It may sound bit weird, one of the reasons is for developers to know
looking at annotation what the API is supposed to do, it is also used to
sort of group them (though it lacks clarity). There was some refactoring
work to fix it (but it failed to address everything) [1]. IMO I never spend
more time on this, I thought by now (2014) we would have a new api
layer/service :)

Api doc generation and plugin such as ApiDiscovery [2] uses annotations and
enums to map relations (there is for example a way that barely works in
cloudmonkey, the CLI, to auto complete API params using these things but it
does not always work). You


> Keep in mind that the latter is passed several times as parameters to
> other methods, which adds many "ifs" and unnecessary complexity.


> We could also make possible to use the Annotation in the method itself,
> which could avoid having to pass the Enum to the method. That means that
> the Enum would be removed from the annotation and we would use the
> annotation only to identify the API name, response object and entity type.
> In order to check the user's credentials, we would use the checkAccess()
> from the account manager, as it is already being used in the ApiDispatcher
> class.
>

You may do it, but ideally I would want the functionality to stay
refactored in one place, so keep checkAccess and other ACL stuff in
separate pure (ACL) plugins and not mix them in other than ACL plugins.


> I know those are huge changes, if we actually agree in going for anything
> like this in the future. But the Admin commands are not doing much except
> for change the enum which is passed to the create response method. Most of
> the content of the execute() method is a copy/paste from the extended
> Command class.
>

I appreciate you reaching out on this. IMO it will be needed to fix it
everywhere (which will be huge change like you said) and bigger challenge
is to keep the layer backward compatible, test it and what not. But I'll
support anyone taking such a task and contribute to as well.

IMHO we should avoid changing the outside layer(s) too much or often such
as API stuff since these are the layer(s) a lot of people depend on while
building things on top of CloudStack (not necessarily have interests in the
whole ACS codebase) -- this has to do with investment of time/energy by
people to learn/adapt/change and it's more challenging than just the
technical ability to fixing things. As a project we don't want to
disappoint users.

Instead I hope for separating the whole business layers (users, accounts,
api etc.) as distribute services from the core orchestration parts of
CloudStack, the API becomes RESTful (or unRESTful using something like
Thrift) with versioning (which can be done for both RESTful HTTP APIs and
unRESTful Thrift based APIs).

[1]
https://cwiki.apache.org/confluence/display/CLOUDSTACK/CloudStack+API+refactoring
[2]
https://cwiki.apache.org/confluence/display/CLOUDSTACK/API+Discovery+Service

Regards.


>
> Cheers,
> Wilder
>

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