Hi Wilder,
On Tue, Jun 24, 2014 at 5:25 PM, Wilder Rodrigues < wrodrig...@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 >
