open-iscsi / targetd

Remote configuration of a LIO-based storage appliance
GNU General Public License v3.0
72 stars 28 forks source link

Add chown option to `nfs_export_add` #60

Closed dsonck92 closed 4 years ago

dsonck92 commented 4 years ago

I think this basic implementation should suffice given the current implementation. Later, when some form of access control is in place, this chown could be limited to what the client is allowed to set. The feature is configurable and default off as it's potentially a security hole.

dsonck92 commented 4 years ago

I've incorporated the tests as per request, I actually discovered a few bugs that way. I've also slightly reformatted some code so it's closer to PEP8 standards which seems to be followed globally in this project already.

tasleson commented 4 years ago

@dsonck92 @triluch @agrover Will hold off on merging this until we have some discussion on returning values for an existing API call that previously didn't return values. My thinking is that clients would only be checking to see if the request completed successfully and not check if some data was returned that they weren't expecting.

dsonck92 commented 4 years ago

As mentioned above, the go based provisioner (iscsi-targetd) does not actively check for the return type. However, once this API is returning values:

tasleson commented 4 years ago

Thanks @dsonck92 !

One of my pet peeves is when people break API's, thus my insistence on maintaining API compatibility.

tasleson commented 4 years ago

As mentioned above, the go based provisioner (iscsi-targetd) does not actively check for the return type. However, once this API is returning values:

* If a field is renamed but a client did not specify this field in a struct, this will not break

* If a field is renamed and the client did specify this field in a struct, it will likely break unless it was marked optional

I think we need to be careful here as languages like go with their struct JSON serialization may fail to de-serialize when things like this change. I'm thinking that having golang bindings would be good add at some point to allow easy use, and to provide another data point for API compatibility.

Obviously we can handling the incoming data in the service and whatever form that presents, but returned value once known is most likely going to stay as is.