Implemented user authentication and parameter

[slueder]

Hi Ondrej,

you have done a very good job creating this module.
I needed some enhancements in the area of initiator-based authentication and connection-related parameters.
Therefore I enhanced the code a bit, please find the result attached to this pull request.

Do you mind including this in your code so that these changes can be maintained in this module for the future ?

Best regards and keep up the good work,
Sven

[OndrejHome]

Hi Sven,
Thank you for using modules and for your contribution. I will have a better look at this over weekend and let you know if this can be merged right away or if any changes are needed. Will get back to you soon.
Ondrej

[OndrejHome]

Hi Sven,

Thank you for your patience. Looking at this PR for targetcli-modules I can see 3 distinctive features in one commit:

• ability to specify 'parameter' in targetclis_iscsi module to control for example limit on maximum concurrent connections
• ability to specify 'auth' in targetcli_iscsi to enable WWN wide support for authentication
• ability to specify 'userid' and 'password' in targetcli_iscsi_acl for given wwn
  (Note: for easier readibility it is good to separate above things at least into separate commits ;) (this is just for future) )

I have checked documentation for Linux SCSI Target and to my understanding the changes
will be able to handle following in terms of authentication:

• paritally TPG authentication
• partially CHAP initiator authentication

1. Is there some particular reason why to support only setting 'userid' and 'password' for ACLS but any options for TPG1?
   (for this I need answer to progress with PR)

2. In terms of consistency I think it would make sense to set userid/password/mutual_userid/mutual_password for TPG1 and ACLS
   in same way - either providing just attribute 'auth' for both TPG1 and ACLS or creating 4 attributes for both TPG1 and ACLs. Now the code
   has both approaches implemented. Would it be possible for you to choose one of them and adjust the code for it?
   (I would subjectively prefer approach with 4 attributes as those can be later possibly validated or used to determine if any change needs to be done)

3. I'm not sure when the cmd = "targetcli '/iscsi/%(wwn)s/tpg1 set attribute authentication=1'" % module.params should be set. Definitely as it is
   now when setting 'auth' on TPG1, but I would expect that this might make sense also when ACLs are created with credentials. Here I assume that whatever that
   is set on the TPG1 is kind of "default" while ACLs can override this default. But to tell iSCSI to use authentication only parameter authentication=1 at TPG1
   level should be used. Here I'm thinking about use case when you set 'auth' only on ACLS but not on TPG1, in such case I assume that on TPG you should make sure
   that authentication=1 is present, right? (this point is optional for PR acceptance)

4. no_log=True should be used for password-like module arguments. For example on how how it looks you can check https://github.com/OndrejHome/ansible.pcs-modules-2/blob/ec5cb8e24ad0f40ec52343f2ede915840583db9c/library/pcs_auth.py#L85
   This would also allow to remove the commented out line with #no_log=True from PR for targetcli role. (this must be addressed to progress with PR)

5. For module documentation could you please make a separate example so the previous example "without authentication" is preserved and add new example "with authentication". This is to make visible that modules can be used in both cases and how to specify it. 'parameters' option can be present in both cases. (this is needed for progressing the PR).

Sven, in case you have any questions to my above suggestions and questions feel free to ask. From what I see there are no serious issues, but only things that I would consider that needs a bit of improvements before merging this so this is maintainable for future. I will leave you soon also feedback on the PR for targetcli PR in the PR for targetcli. Here lets stay focused only on targetcli-modules part.

Ondrej

[OndrejHome]

requested changes outlines in latest comment of PR.

[OndrejHome]

Hi @slueder this is just a reminder that I would need your input for these changes. In case that you would need more than 2 weeks for checking this out please let me know. If you have any questions feel free to ask anytime.

[slueder]

Hi Ondrej, Sorry for keeping you waiting about the answers to your comments and questions. I was returning today from being off and will send you an update / more detailed information. Thank you for getting in touch with me on this pull request, really appreciated. Best regards Sven From: Ondrej Faměra <[email protected]> Sent: 18 June 2019 15:47 To: OndrejHome/ansible.targetcli-modules <[email protected]> Cc: slueder <[email protected]>; Mention <[email protected]> Subject: Re: [OndrejHome/ansible.targetcli-modules] Implemented user authentication and parameter (#3) Hi @slueder <https://github.com/slueder> this is just a reminder that would need your input for these changes. In case that you would need more than 2 weeks for checking this out please let me know. If you have any questions feel free to ask anytime. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3?email_source=notifications&email_token=AISK3XOCTAIVVUAVGRKL6CTP3DROBA5CNFSM4HVXCDQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX6VS2A#issuecomment-503142760> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AISK3XLEXRMM5DL6QYEWB4DP3DROBANCNFSM4HVXCDQQ> . <https://github.com/notifications/beacon/AISK3XOI23KKBHCYPWHMB73P3DROBA5CNFSM4HVXCDQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX6VS2A.gif>

[OndrejHome]

Hi Sven,
thank you for response and welcome back :)

[slueder]

Hi Ondrej, Just another keep-alive message to inform you that due to other business priorities I was not able to invest any time into the review yet. I will work on this as soon as the situation improves, but this will take some additional days. Best regards Sven

[OndrejHome]

Hi Sven,
Thank you for update. :)
Ondrej