ansible-collections / netapp.ontap

Ansible collection to support NetApp ONTAP configuration.
https://galaxy.ansible.com/netapp/ontap
GNU General Public License v3.0
55 stars 35 forks source link

na_ontap_user_role warning and idempotence issue when using "path: DEFAULT" #180

Open mamoep opened 11 months ago

mamoep commented 11 months ago

Summary

Using "path: DEFAULT" in na_ontap_user_role creates a warning and breaks idempotence of the module.

I could identify the line of code that is responsible, but I don't know why it is done: https://github.com/ansible-collections/netapp.ontap/blob/11d8b50293be61d3aeddacabf0d3c185644a3c13/plugins/modules/na_ontap_user_role.py#L360C50-L360C50

Component Name

na_ontap_user_role

Ansible Version

$ ansible --version
ansible [core 2.14.2]
  config file = /home/user/ansible/ansible.cfg
  configured module search path = ['/home/user/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python3.11/site-packages/ansible
  ansible collection location = /home/user/.ansible/collections:/usr/share/ansible/collections
  executable location = /usr/bin/ansible
  python version = 3.11.2 (main, Jun  6 2023, 07:39:01) [GCC 8.5.0 20210514 (Red Hat 8.5.0-18)] (/usr/bin/python3.11)
  jinja version = 3.1.2
  libyaml = True

ONTAP Collection Version

$ ansible-galaxy collection list
# /home/user/.ansible/collections/ansible_collections
Collection         Version
------------------ -------
brocade.fos        1.3.3
netapp.ontap       22.6.0
netapp.storagegrid 21.11.1

ONTAP Version

NetApp Release 9.11.1P8: Fri Apr 07 00:02:50 UTC 2023

Playbook

- hosts: cluster.local
  collections:
    - netapp.ontap
  become: false
  connection: local
  name: Test Role Creation via REST
  vars:
    login: &login
      hostname: "{{ inventory_hostname }}"
      username: "{{ username }}"
      password: "{{ password }}"
      https: true
      validate_certs: false

  tasks:
  - name: Create user role REST in ONTAP 9.11.1.
    netapp.ontap.na_ontap_user_role:
      <<: *login
      state: present
      privileges:
        - path: DEFAULT
          access: readonly
        - path: 'snapmirror'
          access: all
      name: test-role

Steps to Reproduce

run playbook

Expected Results

No warning and "ok" output on second run.

Actual Results

First run

PLAY [Test Role Creation via REST] *********************************************************************************************************************************************************************

TASK [Gathering Facts] **********************************************************************************************************************************************************
ok: [cluster.local]

TASK [Create user role REST in ONTAP 9.11.1.] ***********************************************************************************************************************************
[WARNING]: Create operation also affected additional related commands: [{'path': 'snapmirror', 'access': 'all'}]
changed: [cluster.local]

Second run:

PLAY [Test Role Creation via REST] *********************************************************************************************************************************************************************

TASK [Gathering Facts] **********************************************************************************************************************************************************
ok: [cluster.local]

TASK [Create user role REST in ONTAP 9.11.1.] ***********************************************************************************************************************************
[WARNING]: modify is required, desired: [{'path': 'DEFAULT', 'access': 'readonly'}, {'path': 'snapmirror', 'access': 'all'}] and new current: [{'path': 'snapmirror', 'access':
'all'}]
changed: [cluster.local]
carchi8py commented 11 months ago

@mamoep I'll look at this again but there not an easy way around this.

By default if you don't give a path ONTAP will set the Path DEFAULT so you first one will show as change

Your second run your playbook has no path, but ontap has it set to DEFAULT so we will remove it. Resulting in the playbook showing as changed again (after removing ontap add it back). SO to fix this we add that bit of code.

So if you want Default just leave path blank.

mamoep commented 11 months ago

Sorry I don't understand your explanation. No matter how many times I run the example playbook again, it doesn't change anything in ONTAP after the first run. I tried leaving path blank like path: '' and like path:, both options error out.

I still consider this a code bug.

mamoep commented 11 months ago

I think after some more testing (without the questioned code) I understand the idea behind that part of the code but I still believe it is a bad idea. If I omit the DEFAULT specification I end up with automatically added "DEFAULT: none" (and a useful warning about it).

    netapp.ontap.na_ontap_user_role:
      state: present
      privileges:
      - path: "vserver"
        access: readonly
      name: test-role2

[WARNING]: Create operation also affected additional related commands: [{'path': 'DEFAULT', 'access': 'none'}, {'path': 'vserver', 'access': 'readonly'}]

           Role          Command/                                      Access
Vserver    Name          Directory                               Query Level
---------- ------------- --------- ----------------------------------- --------
cluster    test-role2    DEFAULT                                       none
                         vserver                                       readonly

On the next run that "DEFAULT: none" gets removed by the module, because it wasn't part of the privileges.


           Role          Command/                                      Access
Vserver    Name          Directory                               Query Level
---------- ------------- --------- ----------------------------------- --------
cluster    test-role2    vserver                                       readonly

You can't handle the DEFAULT permissions like this. It is an important part of the role specification and should be treated as such. You already have the warning for exactly this problem category (flipping unspecified but needed paths). Please consider removing this special "DEFAULT" handling for technically correct results.

mamoep commented 8 months ago

Still waiting for your feedback on this.

carchi8py commented 8 months ago

Sorry for the delay on this

So the problem we are trying to solve

If you ran this today, the rest of the API will auto-create a Default. So the next run of this would try to delete auto delete that default.

Let me talk to the entire team and see if there is another way we can get around this. The big problem here is even if the user doesn't add DEFAULT to their playbook, ONTAP will add it resulting in the second run having issue.

mamoep commented 8 months ago

There are also other entries added automatically, depending on the path and access that is requested. That is only handled with a warning currently. In my opinion DEFAULT has to be treated in the same manner.

Example new role:

privileges:
- path: 'volume show'
access: readonly
name: foobar

Results in:

           Role          Command/                                      Access
Vserver    Name          Directory                               Query Level
---------- ------------- --------- ----------------------------------- --------
dfvftsim01 foobar        DEFAULT                                       none
                         volume create                                 readonly
                         volume modify                                 readonly
                         volume show                                   readonly

The special handling of DEFAULT produces all kinds of undesired results in my tests. Another example is a role that is almost admin, but restricts some commands. There is no way to get this done with the module currently.

           Role          Command/                                      Access
Vserver    Name          Directory                               Query Level
---------- ------------- --------- ----------------------------------- --------
dfvftsim01 admin-restricted
                         DEFAULT                                       all
                         security login create                         readonly
                         security login delete                         readonly
                         security login modify                         readonly
                         security login role create
                                                 -role admin-restricted readonly
                         security login role delete
                                                 -role admin-restricted readonly
                         security login role modify
                                                 -role admin-restricted readonly
                         security login role show
                                                 -role admin-restricted readonly
                         security login show                           readonly
rbenigno commented 2 months ago

Any update on this issue? Is there a workaround?