opensecuritycontroller / security-mgr-sample-plugin

Apache License 2.0
5 stars 11 forks source link

Security Group and Security Group Interface Rest Implementation. #33

Closed sudhirappaji closed 6 years ago

sudhirappaji commented 6 years ago

This PR contains the Rest Implementation for:

  1. SecurityGroup 2.SecurityGroupInterface

The SecurityGroupMemberStatus is not part of this PR and this shall be addressed later in another PR.

emanoelxavier commented 6 years ago

@sudhirappaji it seems like there may be an implicit relationship between SG and the device (virtual system in OSC), where all the SGs should be under the device parent. When the SG entity was added this relationship was not put in place or enforced by the plugin. The scope of this user story was indeed to simply have the rest layer added to the already in place db entities of SG and SGIs. If we believe this relationship must be enforced by the SDK implementation it seems like a different request to me. If we agree this should be done I think the right thing to do would be tracking with a future user story to have this improved in the plugin given that it was not there in the first place. @arvindn05 thoughts? @emanoelxavier , Agree we track this as an improvement in a new user story. I will raise the user story and let you know, so that you can review the same.

larkinscarvalho commented 6 years ago

@sudhirappaji Please add a new separate comment and do not update the comments which are already provided as it creates confusion.

Implementing plugin for Security group and Security Group Interface were a part of stubs plugin. I had to work on it to make Policy Mapping working. I believe the missing functionality should be implemented by the stubs author.

arvindn05 commented 6 years ago

@emanoelxavier @sudhirappaji As per the acceptance criteria we wanted to implement the Security group and its DB connections. @larkinscarvalho was able to help out by creating the basic implementation. I dont think we need a different story to make sure the SG gets added under the device(VS)

Implement DB and Rest API for the following entity:
  SecurityGroup
  SecurityGroupInterface
emanoelxavier commented 6 years ago

@arvindn05 it is indeed under the scope of the current US. @sudhirappaji and I discussed and he will address. He will post here how the relationship will be created and enforced between the device and the SG.

emanoelxavier commented 6 years ago

@sudhirappaji the relationships you need to reflect are: 1XN between Device and SG 1XN between Device and SGI (one should be able to add an SGI without the SG) 1X1 between SG and SGI (already there). As you address that also keep in mind this note from Larkins Similarly, for SGI. Also, Device + SG + SGI on manager should be a unique combination.

sudhirappaji commented 6 years ago

@emanoelxavier @arvindn05 , for the device as indicated by Emanoel I assume that OSC passes VS which as device 'MgrId'. The SDK implementation (both SGI and SG) uses this Id to map to device. Uses MgrId for create, update,delete, list etc. Please revert back if my understanding is correct?

sudhirappaji commented 6 years ago

@emanoelxavier , as you mentioned the SGI and SG can be independently created/updated. I will update the REST path accordingly and not build dependency on SG while creating SGI in the rest path. Rather SG id will be passed as part of entity. --> I would keep the REST url for SGI as is and any changes needed can be addressed as part of review.

arvindn05 commented 6 years ago

for the device as indicated by Emanoel I assume that OSC passes VS which as device 'MgrId'. The SDK implementation (both SGI and SG) uses this Id to map to device. Uses MgrId for create, update,delete, list etc. Please revert back if my understanding is correct?

@sudhirappaji yes. that is correct.

emanoelxavier commented 6 years ago

I would keep the REST url for SGI as is and any changes needed can be addressed as part of review.

I believe the REST url for the SGI should not contain the SG as a parent id. You likely can have the SGI rest api in its own class as well, maybe routed to /securitygroupinterfaces and no /sg part in it. And yes, if the provided SGI entity is related to an SG (as you put, SG id as part of the entity) then you can create the SGI entity and add the relationship to the SG, if no SG existed you can then fail. If no SG info is provided as part of the SGI then you can simply create a new SGI entity associated with the device (it should always be associated with the device/VS no matter if it has an SG parent or not). I hope that clarifies.

sudhirappaji commented 6 years ago

@emanoelxavier , updated the code for Device relation to SG and SGI. Please have a look.

sudhirappaji commented 6 years ago

@emanoelxavier , tested with OSC build#85 (deployed) and is working. The following scenario is tested and working fine ==> Create the security group and bind. From Rest API able to see the SG and SGI created. ==> updated the SG in OSC UI and was reflected via the REST request. ==> deleted or unbind in OSC UI and was reflected via the REST ==> deleted in DB via Rest and synced through OSC UI. The SG and SGI was recreated.

sudhirappaji commented 6 years ago

@emanoelxavier , I see this is yet to be merged and will schedule for a demo your Saturday morning? Also tested on deployed OSC Build#93 and tests are passing as expected.

sudhirappaji commented 6 years ago

@emanoelxavier , I have merged with the master and tested this with Build#114 on deployed OSC and tests are passing.

emanoelxavier commented 6 years ago

thanks @sudhirappaji I believe I had approved this before. @larkinscarvalho @arvindn05 pls let sudhir know if this can be merged.

sudhirappaji commented 6 years ago

@arvindn05 , I have incorporated all your review remarks and please check ...

sudhirappaji commented 6 years ago

@arvindn05 , I believe i have addressed all your comments. please take a look.

sudhirappaji commented 6 years ago

@arvindn05 , I have incorporated the review comments, hopefully this can be closed now.

arvindn05 commented 6 years ago

we are close to being done...please update for the comments and i can approve the final changes.

sudhirappaji commented 6 years ago

@arvindn05 , I have addressed your comments.