IBM-Security / ibmsecurity

Idempotent functions for IBM Security Appliance REST APIs. Currently covering ISAM and ISDS Appliances.
Apache License 2.0
47 stars 73 forks source link

python 3 ... too soon #345

Closed tombosmansibm closed 1 year ago

tombosmansibm commented 2 years ago

Closes #344

sygilber commented 2 years ago

I would not say you are "too" soon with Python V3 ... once a leader we are now the one dragging behind )-:

sygilber commented 2 years ago

Testing your patch on this PR directly and the only remaining error is the following:

An exception occurred during task execution. To see the full traceback, use -vvv. The error was: ansible.module_utils.connection.ConnectionError: Error> invalid action was specified, method not found in module! failed: [someappliance] (item={u'stanza_id': u'header-names', u'entries': []}) => {"ansible_loop_var": "item", "changed": false, "item": {"entries": [], "stanza_id": "header-names"}, "module_stderr": "Traceback (most recent call last):\n File \"/home/sygilber/.ansible/tmp/ansible-local-100450eh0nzo/ansible-tmp-1645799574.37-102736-13527176277094/AnsiballZ_isam.py\", line 102, in \n _ansiballz_main()\n File \"/home/sygilber/.ansible/tmp/ansible-local-100450eh0nzo/ansible-tmp-1645799574.37-102736-13527176277094/AnsiballZ_isam.py\", line 94, in _ansiballz_main\n invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)\n File \"/home/sygilber/.ansible/tmp/ansible-local-100450eh0nzo/ansible-tmp-1645799574.37-102736-13527176277094/AnsiballZ_isam.py\", line 40, in invoke_module\n runpy.run_module(mod_name='ansible_collections.ibm.isam.plugins.modules.isam', init_globals=None, run_name='main', alter_sys=True)\n File \"/usr/lib64/python2.7/runpy.py\", line 176, in run_module\n fname, loader, pkg_name)\n File \"/usr/lib64/python2.7/runpy.py\", line 82, in _run_module_code\n mod_name, mod_fname, mod_loader, pkg_name)\n File \"/usr/lib64/python2.7/runpy.py\", line 72, in _run_code\n exec code in run_globals\n File \"/tmp/ansible_ibm.isam.isam_payload_b6Jkey/ansible_ibm.isam.isam_payload.zip/ansible_collections/ibm/isam/plugins/modules/isam.py\", line 130, in \n File \"/tmp/ansible_ibm.isam.isam_payload_b6Jkey/ansible_ibm.isam.isam_payload.zip/ansible_collections/ibm/isam/plugins/modules/isam.py\", line 110, in main\n File \"/tmp/ansible_ibm.isam.isam_payload_b6Jkey/ansible_ibm.isam.isam_payload.zip/ansible/module_utils/connection.py\", line 190, in rpc\nansible.module_utils.connection.ConnectionError: Error> invalid action was specified, method not found in module!\n", "module_stdout": "", "msg": "MODULE FAILURE\nSee stdout/stderr for the exact error", "rc": 1}

sygilber commented 2 years ago

OK - seems the only remaining item to fix in this recent commit is the following which I do not think is a Python V2.7 vs V3.x issue.

We have this code pattern all over place where we can obtain legitimaly '[]' while reading from external properties files and the previous Python code would just accept it:

- role: ibm.isam.set_reverseproxy_conf
  set_reverseproxy_conf_entries:
    - { stanza_id: "acnt-mgt", entries: "[['some-entry','{{ lookup('ini', 'some-entry type=properties file={{ fact_rp_file }}') }}']]" }

I think we need to continue to support this pattern that keeps playbook clean without too much logic.

tombosmansibm commented 2 years ago

That seems wrong. You have nested {{ }} :

Is it this ?

tombosmansibm commented 2 years ago

Works fine for me, by the way (Python 3.9).

Note that your initial syntax is not correct (nested {{ ).

instances:
  - inst_name: "default"
    entries:
      - method: set
        stanza_id: server
        entries:
          - [ "use-http-only-cookies", "no" ]
          - [ "allow-unsollicited-logins", "no" ]
  - inst_name: "default"
    entries:
      - method: set
        stanza_id: acnt-mgt
        entries:
          - [ "some-entry", "{{ lookup('ini', 'some-entry type=properties file=fact_rp_file') }}" ]

with a files/fact_rp_file .

TASK [ibm.isam.configure_reverseproxy_instances : Configure reverse proxy instances] ** vrijdag 25 februari 2022 16:38:18 +0100 (0:00:00.031) 0:00:00.366 ** ok: [172.16.73.128] => (item={'method': set, inst_name: default, stanza_id: server, 'entries': ['use-http-only-cookies', 'no']['allow-unsollicited-logins', 'no']}) changed: [172.16.73.128] => (item={'method': set, inst_name: default, stanza_id: acnt-mgt, 'entries': ['some-entry', 'blablabla']})

sygilber commented 2 years ago

This code worked before as far as I can remember from 2017 to now. Your proposal is to transform 1 line of code into 6 lines ?

tombosmansibm commented 2 years ago

No, my proposal is to NOT use nested {{}} (which I almost can't believe ever worked :-) ). You can write it in one line as well. I personally prefer yaml syntax everywhere:

  - inst_name: "default"
    entries:
      - {method: set, stanza_id: acnt-mgt, entries: [[ "some-entry", "{{ lookup('ini', 'some-entry type=properties file=fact_rp_file') }}" ]]}
tombosmansibm commented 2 years ago

I've installed a python 2.7 virtual env now , and this works fine:

TASK [ibm.isam.configure_reverseproxy_instances : Configure reverse proxy instances] ***************************************************
vrijdag 25 februari 2022  16:52:35 +0100 (0:00:00.015)       0:00:00.109 ****** 
ok: [172.16.73.128] => (item={'method': set, inst_name: default, stanza_id: server, 'entries': [u'use-http-only-cookies', u'no'][u'allow-unsollicited-logins', u'no']})
changed: [172.16.73.128] => (item={'method': set, inst_name: default, stanza_id: acnt-mgt, 'entries': [u'some-entry', u'blablabla2']})
sygilber commented 2 years ago

Tom, this specific item about the "{{}}" may be related to Python 2.7 vs Python 3.X as when re-write this line:

rp_web_authn_methods: "{{ lookup('ini', 'web_authn_methods type=properties file={{ fact_rp_file }}') }}"

to

rp_web_authn_methods: "{{ lookup('ini', 'web_authn_methods type=properties file=fact_rp_file') }}"

i get other errors down the road.

I would just recommended for now for you to focus on the core issue which is the python code should consider as valid receiving an empty "[]".

tombosmansibm commented 2 years ago

Sorry, I see I've misinterpreted something. fact_rp_file is obviously a variable, not the filename directly.
And it's the empty [] that's the issue ... ok.

But both this syntax :

     - {method: set, stanza_id: acnt-mgt, entries: [[ "some-entry", "{{ lookup('ini', 'some-entry type=properties file='+fact_rp_file ) }}" ]]}

and this

      - {method: set, stanza_id: acnt-mgt, entries: [[ "some-entry", "{{ lookup('ini', 'some-entry type=properties file={{ fact_rp_file }}' ) }}" ]]}

both work for me, in Python 3 and Python 2.7.

tombosmansibm commented 2 years ago

OK, I've included a check for the "empty" value.

The problem was that after deleting the parameter, it tried to add it again with an empty value, which is not allowed.

ram-ibm commented 2 years ago

The syntax Sylvain is referring to is in a lot of places (for entries with the nested []) - I really hope that does not break!

tombosmansibm commented 2 years ago

The issue here is that the value is empty (or rather, an empty []). There’s no problem with the nested []…

sygilber commented 2 years ago

Tom. We are now finally on Python 3.6. Just wondering if you know when this PR will review and merged as we are observing the nested bug issue.

tombosmansibm commented 1 year ago

Not doing this anymore.
Python 2.7 is no longer supported.