Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
tep opened this issue Oct 31, 2022 · 6 comments
Closed

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

tep opened this issue Oct 31, 2022 · 6 comments
Labels
enhancement New feature or request

Comments

@tep
Copy link

tep commented Oct 31, 2022

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

This should be overridable.

@datamattsson
Copy link
Collaborator

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.

@datamattsson datamattsson added the enhancement New feature or request label Oct 31, 2022
@tep
Copy link
Author

tep commented Nov 1, 2022

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
Copy link
Collaborator

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
Copy link
Author

tep commented Nov 4, 2022

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
Copy link
Collaborator

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:

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
Copy link
Collaborator

Fixed in #40

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants