contiv / netplugin

Container networking for various use cases
Apache License 2.0
515 stars 178 forks source link

Validation of network policy create #361

Open pradvara opened 8 years ago

pradvara commented 8 years ago

We are able to create network policy with the same name multiple times, there should a check for returning the existence of the policy

[root@vm1 wordpress]# netctl policy create policy1 INFO[0000] Creating policy default:policy1 [root@vm1 wordpress]# netctl policy create policy1 INFO[0000] Creating policy default:policy1 [root@vm1 wordpress]# netctl policy create policy1 INFO[0000] Creating policy default:policy1

gaurav-dalvi commented 8 years ago

This is applicable for group creation as well.

[admin@netmaster ~]$ netctl group create -t t1 -p app2db n1 g1 [admin@netmaster ~]$ netctl group create -t t1 -p app2db n1 g1

[admin@netmaster ~]$ netctl group create -t t1 n1 g2 [admin@netmaster ~]$ netctl group create -t t1 n1 g2 [admin@netmaster ~]$ netctl group create -t t1 n1 g2

gaurav-dalvi commented 8 years ago

@shaleman : Is this expected behavior ? We can fix this. It does not happen with Network and Tenant creation.

admin@netmaster ~]$ netctl tenant create t1 INFO[0000] Creating tenant: t1
[admin@netmaster ~]$ netctl tenant create t1 INFO[0000] Creating tenant: t1
ERRO[0000] Cant change tenant parameters after its created [github.com/contiv/netplugin/netmaster/objApi.(*APIController).TenantUpdate apiController.go 981]

[admin@netmaster ~]$ netctl network create --tenant t1 --subnet=20.1.1.0/24 --gateway=20.1.1.254 -e "vlan" n1 [admin@netmaster ~]$ netctl network create --tenant t1 --subnet=20.1.1.0/24 --gateway=20.1.1.254 -e "vlan" n1 ERRO[0000] Cant change network parameters after its created [github.com/contiv/netplugin/netmaster/objApi.(*APIController).NetworkUpdate apiController.go 589]

shaleman commented 8 years ago

@pradvara @gaurav-dalvi The REST API we provide should be completely idempotent, meaning we should be able to change anything and any number of times and the last value we set should be retained. This is the general expectation on most of the REST objects.

But, there are few cases where we don't allow an object to be modified. For example, currently, we can't change parameters of network after its created. Similarly, you can't change a rule once its created. Only way to change is by deleting and recreating it. Both of these are a current implementation limitations that will be fixed in near future.

Now, netctl 'create' action maps to a POST operation on a REST object. So, effectively every 'create' operation becomes a 'create/update' operation(contivmodel backend internally figures out if the object exist and converts 'create' to 'update' operation). This is little confusing. I don't have a good solution for it. One option is to map 'create' to PUT operation and 'update' to POST operation and fail duplicate creates and fail 'update' without a previous 'create' operation. Note that, in contiv-ui, this difference does not exist.

so, in the problems you reported above, "policy create" and "group create" are just becoming 'update' operations. In case of network, we are failing network update even though the contents are same. May be we can avoid this.

gaurav-dalvi commented 8 years ago

@shaleman Thanks for detailed explanation. Why cant we have same behavior for all objects ?

For eg: Just like Tenant and Network we can restrict user creating same multiple groups / policies again and again ?

shaleman commented 8 years ago

@gaurav-dalvi May be i wasn't too clear.

  1. you should be able to "set" a tenant/network/groups/policy REST objects any number of times.
  2. Not being able to set the network REST object twice is a bug.
  3. There is an orthogonal question whether we should split netctl's "create" operations into separate "create" and a "set"
pradvara commented 8 years ago

@shaleman Thank you, so as you suggested are there plans to map the "policy create" and "group create" to "PUT" operation so that it would throw an error on duplication, and have a separate operation for "update" like "netctl policy update" which in turn uses POST

truncj commented 7 years ago

Running into the same issue when trying to update an existing network. I'm imagining there's no plans of adding this type of "put" functionality?