hpe-storage / truenas-csp

TrueNAS Container Storage Provider for HPE CSI Driver for Kubernetes
https://scod.hpedev.io
MIT License
65 stars 8 forks source link

CSP Backend uses a hardcoded value for "target_basename" #34

Closed tep closed 1 year ago

tep commented 1 year ago

CSP Backend uses a hardcoded value for target_basename (e.g. iqn.2011-08.org.truenas.ctl).

This should be overridable.

datamattsson commented 1 year ago

FreeNAS was not originally in scope for this CSP and a lot of these things got neglected. However, since the target prefix is a backend specific detail, it belongs in the StorageClass as a parameter and not as a global override.

tep commented 1 year ago

How about this?

TrueNAS already knows its target basename value - which is available through the API by calling iscsi/global - we can simply grab the proper value from there. Seems better than trying to keep configurations in sync.

n.b. I've updated PR #35 to do just that.

datamattsson commented 1 year ago

Thanks for the revision. I like this solution better, but I think we can improve it further. After noodling this a bit I think there shouldn't be a hard coded default at all. We don't want users to be frisky and think they can use any odd target prefix and I would like the CSP to actually validate that the target name has a valid prefix, which in this case is any of the two TrueNAS/FreeNAS prefixes. It should fail otherwise and tell the user why.

tep commented 1 year ago

Oddly enough, that's how I'd done things originally but switched to using a default value at the last minute to maintain consistent behavior.

My latest commit reverts back to raising falcon.HTTPInternalServerError if a basename value cannot be determined.

datamattsson commented 1 year ago

Thanks for collaborating on this. However, I want to make this solution even simpler.

The current hardcoded value should be made into a list (self.target_basenames = []) and verification of the target should be made in the Tokens class on_post function:

https://github.com/hpe-storage/truenas-csp/blob/339486d71a1c96607a0cd04682d82a12256c567a/truenascsp/truenascsp.py#L414

While examining this code I see that I completely left out the verification of the target name in the first place, which is unintentional.

datamattsson commented 1 year ago

Fixed in #40