search

OndrejHome / ansible.targetcli-modules

Modules for managing targetcli via Ansible
GNU General Public License v3.0
10 stars 12 forks source link

Implemented user authentication and parameter #3

Open slueder opened 5 years ago

slueder commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Hi Sven,

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

I have checked documentation for Linux SCSI Target and to my understanding the changes will be able to handle following in terms of 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 commented 5 years ago

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 commented 5 years ago

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 notifications@github.com Sent: 18 June 2019 15:47 To: OndrejHome/ansible.targetcli-modules ansible.targetcli-modules@noreply.github.com Cc: slueder mail@svenlueder.de; Mention mention@noreply.github.com 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 https://github.com/OndrejHome/ansible.targetcli-modules/pull/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 commented 5 years ago

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

slueder commented 5 years ago

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 commented 5 years ago

Hi Sven, Thank you for update. :) Ondrej