incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rohit Yadav <rohit.ya...@citrix.com>
Subject RE: Merge request: Merging api_refactoring on master
Date Sat, 22 Dec 2012 19:51:44 GMT
I totally understand the concerns, but the change was necessary also a security concern (you
don't want to expose internal DB IDs).
The internal ID was never supposed to be used in API calls or be known to the end user/admin-user,
the changes are enforcing the rule that UUID be translated to internal ID [1]
The UI does not break because of this change because type based checking is not done, for
example in the response jobid or id used to query other APIs without doing any type checking.
Btw, in response id/jobid was always an uuid string, and which would be used to query other
apis.

Earlier we had a IdentityMapper annotation to do the translation, the only difference is that
it would allow both UUID strings and ID long ints as params;

            case LONG:                                                              
                if (identityMapper != null)                                         
                    field.set(cmdObj, s_instance._identityDao.getIdentityId(identityMapper,
paramObj.toString()));
                else                                                                
                    field.set(cmdObj, Long.valueOf(paramObj.toString()));           
                break; 

If any app developer or sysadmin is using this approach, the changes should not break their
apps/scripts, if they are using hardcoded long int ids for some APIs they may have used long
ints, but chances are if you're using response (json/xml) to call other APIs, they would be
already using UUID strings and not long ints. Pl. suggest if there are folks here whose apps/scripts
would break? To have backward compatibility to previous behaviour I can implement a conditional
to accept internal IDs for sometime and we would discourage usage of internal IDs and encourage
usage of UUIDs, and in sometime we can deprecate IDs and get rid of code that accepts internal
IDs.

Regards.

[1] https://git-wip-us.apache.org/repos/asf?p=incubator-cloudstack.git;a=commit;h=5fdce84e81c10650d41de1837d7f4a1791fc4a6e
________________________________________
From: Chip Childers [chip.childers@sungard.com]
Sent: Saturday, December 22, 2012 7:55 PM
To: <cloudstack-dev@incubator.apache.org>
Subject: Re: Merge request: Merging api_refactoring on master

So it sounds like a ton of good work.

However, the proposed merge also sounds like it breaks public API
compatibility with 4.0.0 in both the uuid / id changes and in the list
result changes.

So I guess this is my first question: does the community agree that
the benefits of these changes outweigh the concerns about moving
straight from 4.0.0 to 5.0.0?

Rohit, I think we HAVE to have concerns us on that question before
this merge happens.

- chip

Sent from my iPhone.

On Dec 22, 2012, at 4:38 AM, Rohit Yadav <rohit.yadav@citrix.com> wrote:

> Hi everyone,
>
> I'm planning to merge api_refactoring branch on to master after 72 hour period which
would be Monday EOD. Pl. go through the email, and previous threads on api refactoring rework
and feel free to share your ideas, comments and vote to agree, disagree. If no one objects
I would like to ask the git Santa to merge it on Christmas 25 Dec :D (after 72 hour window)
>
> The reason why I want to merge around the next week is because I think we would have
lower frequencies of emails, review requests and people contributing, hence I can move around
a lot of code (mostly package renames to org.apache.cloudstack in cloud-api) and right now
the merge conflicts are really minimum, about 100-200 lines. (A top level issues to track
sub-issues: https://issues.apache.org/jira/browse/CLOUDSTACK-638)
>
> What will be affected:
> 0. Any class in cloud-api and on api-layer only
> 1. Any class that imports from/to cloud-api's response and cmd classes
>
> Some of the major changes that will be merged on master;
> 0. Over the wire (OTW) HTTP request to API server would send only UUID strings. All requests
done via UUIDs (and not CloudStack's internal db's IDs).
> 1. https://issues.apache.org/jira/browse/CLOUDSTACK-518 Fix @Parameter annotation to
have annotation field to a Response class which would give us entities (interface to VO objects).
This would get rid of all IdentityMapper using which was used earlier to get VO entities from
an annotated table name. This helps us to translate OTW UUIDs to CloudStack's DB's internal
IDs.
> 2. Separation of ACL Role access checker as an adapter, so organizations can implement
their own role based access checking. The mechanism would exist in CloudStack's API server
but policy checking is moved out of CloudStack. (https://issues.apache.org/jira/browse/CLOUDSTACK-639)
This works, but was tough to get it right the first time, there is better way which I'll share
before the merge.
> 3. Group APIs to org.apache.cloudstack.api.{command,response}.{entity1,entity2 etc.}
packages. This is mainly done for developers, so when they work on API layer they would know
which api has what level of security and as they are grouped based on entity type, it will
be easier to search. This was mostly file movement to org.apache.cloudstack.api package and
helped us track couple of classes which are no longer needed. Another aim was to move from
com.cloud to org.apache.cloudstack (only cloud-api for now).
> 4. Annotation work as described in 1., also for @ACL etc.
> 5. DB, ACL validation wip code
> 6. A lot of list api optimizations and response view work from our newest commiter, Min
Chen. The aim is to simply response, right now for. example when we listVMs we don't want
unnecessary (serialized) response objects which could be queried using uuids separately.
>
> Pl. ask away any doubts, questions and concerns you may have. It was challenging for
me as well to understand the functional spec, to know the why/what/how, and if you read the
old threads you can tell I did not get it the first time.
>
> A lot of annotation work is aimed to be completed over this weekend, so when the branch
is finally merged it won't break any functionality. At present the branch is quite stable
>
> Testing and how or why do it?
> 0. Prasanna, Meghna? can help us write few basic sets of unit tests and marvin integration
tests for OTW requests. We already have few of their patches on rb.
> 1. We can also have drivers to automate tests (Prasanna can talk more on this and on
his devcloud based continuous intergration server)
> 2. If I do it now, there would be a lot more eyes to point out bugs and I want more people
to participate in the refactoring work.
> 3. Right now, it builds and runs fine with minimal breaks and no functionality breakage
as most of the changes are only restricted to api-server (:cloud-api artifact). I'm able to
deploy a zone etc. To make the UUID thing work, I've put in hardcoded (for. ex. projectId=-1
which should be a string uuid not a long int value -1) stuff that saves the UI from being
broken which I'll remove after merging on master so UI engineers can help fix UI issues.
>
> Lastly, I would like to thank Min for her amazing patches and optimization work, Prachi
for her work on ACL, Fang, Prasanna, Likitha for their help with the refactoring work and
for their contributions. Community for asking questions, raising issues and thanks to Alex
for his guidance, reviews and kickass OOP concepts.
>
> Ref;
> http://www.slideshare.net/buildacloud/cloudstack-collaboration-conference-12-refactoring-cloud-stack
> https://git-wip-us.apache.org/repos/asf?p=incubator-cloudstack.git;a=shortlog;h=refs/heads/api_refactoring
> https://cwiki.apache.org/confluence/display/CLOUDSTACK/CloudStack+API+refactoring
>
> Regards.
> PS. will write a blog on it this weekend so folks can follow what's going on :)
> PPS. maybe explain in a video

Mime
View raw message