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

Implemented user authentication and parameter #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

slueder
Copy link

@slueder slueder commented Jun 7, 2019

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 OndrejHome self-assigned this Jun 7, 2019
@OndrejHome
Copy link
Owner

Hi Sven,
Thank you for the PR on this role related to targetcli-modules changes.

  1. I'm getting issue when I try to run this role without specifying the new initiators_auth section, which breaks playbooks I use typically with this role. Also it seems that I cannot use this role for creating non-authenticated targetcli server.

My typical use-case for this role can be seen in playbook mentioned in this blogpost - https://www.famera.cz/blog/ansible/creating_ha_clusters_with_storage_v2.html
There you could see that I dynamically detect the WWNs of initiators to be configured with targetcli role. The error that i get looks like following:

TASK [OndrejHome.targetcli : define ACLs for iSCSI target] ****************************************************************************************************
fatal: [192.168.xx.xx]: FAILED! => {"msg": "The task includes an option with an undefined variable. The error was: 'dict object' has no attribute 'initiators_auth'\n\nThe error appears to have been in '...roles/OndrejHome.targetcli/tasks/main.yml': line 42, column 3, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n\n- name: define ACLs for iSCSI target\n  ^ here\n"}

I can see that you have tried to supply the default filters to role, but in this case it doesn't work. I have tried few approaches to avoid this but so far without success. I will have again over week. However this is at the moment a blocker as I cannot create non-authenticated iSCSI with this change anymore. In case you have idea how to fix this feel free to reply. I will check mostly if there is some sane workaround to get this working.

  1. #no_log: true this kind of code should not be present here. It needs fixing in the targetcli-modules so we don't get warnings for using attribute names like "password" without hiding passwords.

I would at this moment focus on getting the targetcli-modules to be merged firsts before making further changes here. Only point that stays valid for now is how to handle '1.' which I will also have a look at, but it will take time.

As with the other PR, feel free to ask if you have any questions to above ;)

Ondrej

@OndrejHome
Copy link
Owner

Hi Sven,

Thank you for your patience. So I think I have found the reason for the error I see - older Ansible version :) I was using Ansible 2.7.10 and recently after upgrade to 2.8.0 "it just works".
While it would be nice to have some workaround for older ansible versions I think I can live with this working from 2.8.0 if that is reflected in meta/main.yml variable min_ansible_version (needs to be changed to 2.8).

ansible 2.7.10
  config file = /home/ondrej/.ansible.cfg
  configured module search path = ['/home/ondrej/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib64/python3.6/site-packages/ansible
  executable location = /usr/lib/python-exec/python3.6/ansible
  python version = 3.6.5 (default, Apr 20 2018, 20:45:10) [GCC 6.4.0]

fatal: [192.168.22.55]: FAILED! => {"msg": "The task includes an option with an undefined variable. The error was: 'dict object' has no attribute 'initiators_auth'\n\nThe error appears to have been in '/home/ondrej/tmp/pr_review/roles/OndrejHome.targetcli/tasks/main.yml': line 42, column 3, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n\n- name: define ACLs for iSCSI target\n  ^ here\n"}
ansible 2.8.0
  config file = /home/ondrej/.ansible.cfg
  configured module search path = ['/home/ondrej/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib64/python3.6/site-packages/ansible
  executable location = /usr/lib/python-exec/python3.6/ansible
  python version = 3.6.5 (default, Apr 20 2018, 20:45:10) [GCC 6.4.0]

TASK [OndrejHome.targetcli : assing LUNs to initiators] *******************************************************************************************************
ok: [192.168.22.55] => (item=[{'wwn': 'iqn.1994-05.com.redhat:target', 'initiators': ['iqn.1994-05.com.redhat:ebf9c7efb13']}, {'path': '/dev/c7vg/LV1', 'name': 'test1', 'type': 'block'}])

So as for now the changes in OndrejHome.targetcli-modules needs to be done before this can be merged and additional change of min_ansible_version to 2.8 is needed.

@slueder please let me know if you can have a look at this in upcoming two weeks or if you would need more time. If there are any questions feel free to ask anytime.

@OndrejHome OndrejHome self-requested a review June 18, 2019 13:41
Copy link
Owner

@OndrejHome OndrejHome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally meta/main.yml needs to be changed. min_ansible_version: 2.8 should be used there.

with_subelements:
- "{{ iscsi_targets }}"
- initiators
notify:
- save targetcli configuration
#no_log: true
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should not be present here but in OndrejHome.targetcli-modules ansible role modules

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants