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

aci_rest: change not detected #35041

Closed
marinus81 opened this issue Jan 18, 2018 · 23 comments
Closed

aci_rest: change not detected #35041

marinus81 opened this issue Jan 18, 2018 · 23 comments
Labels
aci Cisco ACI community affects_2.4 This issue/PR affects Ansible v2.4 bug This issue/PR relates to a bug. cisco Cisco technologies module This issue/PR relates to a module. networking Network category support:certified This issue/PR relates to certified code. waiting_on_vendor This issue requires actions by the vendor. Please inquire the vendor's help for any progress.

Comments

@marinus81
Copy link
Contributor

ISSUE TYPE
  • Bug Report
  • maybe Enhancement request?
COMPONENT NAME

aci_rest

ANSIBLE VERSION
ansible 2.4.2.0
  config file = None
  configured module search path = [u'/home/dhamann/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/local/lib/python2.7/dist-packages/ansible
  executable location = /usr/local/bin/ansible
  python version = 2.7.12 (default, Nov 20 2017, 18:23:56) [GCC 5.4.0 20160609]

CONFIGURATION

DEFAULT_DEBUG(env: ANSIBLE_DEBUG) = False
DEFAULT_STRATEGY(env: ANSIBLE_STRATEGY) = linear

OS / ENVIRONMENT

N/A

SUMMARY

aci_rest module does not always detect changed config, but reports it as unchanged ("ok:")

STEPS TO REPRODUCE

Depending on how you push the config to APIC the aci_rest module detects it as changed or not:

This example will always report as unchanged:

- name: Create CDP Policies
      tags:
        - create
        - network
      aci_rest:
        host: "{{ apic_hostname }}"
        username: "{{ apic_username }}"
        password: "{{ apic_password }}"
        validate_certs: false
        method: post
        path: "api/node/mo/uni/infra.xml?rsp-subtree=modified"
        content: |
          <infraInfra>
            <cdpIfPol adminSt="{{item.state}}"  name="{{item.name}}" descr="CDP set to {{item.state}}"/>
          </infraInfra>
      with_items:
        - "{{cdp_policies}}"

This playbook works as expected ("ok:" if nothing has changed and "changed:" otherwise)

- name: Create CDP Policies
      tags:
        - create
        - network
      aci_rest:
        host: "{{ apic_hostname }}"
        username: "{{ apic_username }}"
        password: "{{ apic_password }}"
        validate_certs: false
        method: post
        path: "api/node/mo/uni/infra/.xml?rsp-subtree=modified"
        content: |
            <cdpIfPol adminSt="{{item.state}}"  name="{{item.name}}" descr="CDP set to {{item.state}}"/>
      with_items:
        - "{{cdp_policies}}"

Note the difference in path: parameter (infra.xml vs. infra/.xml) as well as adjusted content.

EXPECTED RESULTS

In both cases changes to actual APIC config should result in "changed:" status

ACTUAL RESULTS

The reason why the first playbook does not behave as expected is that APIC does not populate the "status=" parameter as expected by the aci_rest module (it will only report change if this parameter is set to either of 'created', 'modified', 'deleted')
(see https://github.com/datacenter/aci-ansible/blob/e87db8f15e34ea7dc2e1b30fd445a4c59561bbc9/library/aci_rest.py#L270-L284)

ok: [localhost] => (item={u'state': u'enabled', u'name': u'cdp_on2'}) => {
    "changed": false, 
    "error_code": 0, 
    "error_text": "Success", 
    "imdata": [
        {
            "infraInfra": {
                "attributes": {
                    "childAction": "deleteNonPresent", 
                    "dn": "uni/infra", 
                    "lcOwn": "local", 
                    "modTs": "2017-08-17T01:40:52.471+01:00", 
                    "monPolDn": "uni/fabric/monfab-default", 
                    "name": "infra", 
                    "nameAlias": "", 
                    "ownerKey": "", 
                    "ownerTag": "", 
                    "rn": "", 
                    "status": "", 
                    "uid": "0"
                }, 
                "children": [
                    {
                        "cdpIfPol": {
                            "attributes": {
                                "adminSt": "enabled", 
                                "childAction": "deleteNonPresent", 
                                "descr": "", 
                                "extMngdBy": "", 
                                "lcOwn": "local", 
                                "modTs": "2018-01-17T09:45:05.902+01:00", 
                                "monPolDn": "", 
                                "name": "cdp_on2", 
                                "nameAlias": "", 
                                "ownerKey": "", 
                                "ownerTag": "", 
                                "rn": "cdpIfP-cdp_on2", 
                                "status": "", 
                                "uid": "17805"
                            }
                        }
                    }
                ]
            }
        }
    ], 
    "invocation": {
        "module_args": {
            "content": "<infraInfra>\n  <cdpIfPol adminSt=\"enabled\"  name=\"cdp_on2\" />\n</infraInfra>\n", 
            "host": "apic.ddsdnlab.at", 
            "hostname": "apic.ddsdnlab.at", 
            "method": "post", 
            "password": "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER", 
            "path": "api/node/mo/uni/infra.xml?rsp-subtree=modified", 
            "protocol": "https", 
            "src": null, 
            "timeout": 30, 
            "use_proxy": true, 
            "use_ssl": true, 
            "username": "ansible", 
            "validate_certs": false
        }
    }, 
    "item": {
        "name": "cdp_on2", 
        "state": "enabled"
    }, 
    "response": "OK (544 bytes)", 
    "status": 200, 
    "totalCount": "1", 
    "url": "https://apic.ddsdnlab.at/api/node/mo/uni/infra.xml?rsp-subtree=modified"
}

On the other hand the second playbook runs as expected as returned data from APIC contains (status='modified'):

changed: [localhost] => (item={u'state': u'enabled', u'name': u'cdp_on'}) => {
    "changed": true, 
    "error_code": 0, 
    "error_text": "Success", 
    "imdata": [
        {
            "cdpIfPol": {
                "attributes": {
                    "adminSt": "enabled", 
                    "childAction": "deleteNonPresent", 
                    "descr": "CDP set to enabled", 
                    "dn": "uni/infra/cdpIfP-cdp_on", 
                    "extMngdBy": "", 
                    "lcOwn": "local", 
                    "modTs": "2018-01-18T05:26:34.072+01:00", 
                    "monPolDn": "uni/fabric/monfab-default", 
                    "name": "cdp_on", 
                    "nameAlias": "", 
                    "ownerKey": "", 
                    "ownerTag": "", 
                    "rn": "", 
                    "status": "modified", 
                    "uid": "15374"
                }
            }
        }
    ], 
    "invocation": {
        "module_args": {
            "content": "<cdpIfPol adminSt=\"enabled\"  name=\"cdp_on\" descr=\"CDP set to enabled\"/>\n", 
            "host": "apic.ddsdnlab.at", 
            "hostname": "apic.ddsdnlab.at", 
            "method": "post", 
            "password": "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER", 
            "path": "api/node/mo/uni/infra/.xml?rsp-subtree=modified", 
            "protocol": "https", 
            "src": null, 
            "timeout": 30, 
            "use_proxy": true, 
            "use_ssl": true, 
            "username": "ansible", 
            "validate_certs": false
        }
    }, 
    "item": {
        "name": "cdp_on", 
        "state": "enabled"
    }, 
    "response": "OK (375 bytes)", 
    "status": 200, 
    "totalCount": "1", 
    "url": "https://apic.ddsdnlab.at/api/node/mo/uni/infra/.xml?rsp-subtree=modified"
}

Given this (weired?) behavior of APIC (I confirmed this behavior is the same on APIC versions 3.0, 3.1 and 2.1), I think the current implementation of aci_changed() might be insufficient.

@ansibot
Copy link
Contributor

ansibot commented Jan 18, 2018

Files identified in the description:

If these files are inaccurate, please update the component name section of the description or use the !component bot command.

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Jan 18, 2018

@ansibot ansibot added aci Cisco ACI community affects_2.4 This issue/PR affects Ansible v2.4 bug_report module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. networking Network category support:community This issue/PR relates to code supported by the Ansible community. labels Jan 18, 2018
@dagwieers
Copy link
Contributor

dagwieers commented Jan 18, 2018

Thanks for reporting this.

I have experienced similar problems, but aci_rest relies on this information from the APIC and it seems the APIC is not correctly returning this information to us in every case. If the APIC does not tell us there was a change, there is no practical way aci_rest is able to detect this.

This should be reported to Cisco TAC. Can you please open a ticket ?

PS I would have expected it to work because aci_rest adds rsp-subtree=modified to the path (so you don't have to), which should have worked IMO.
PS2 We do not intend to change the payload provided by the user, but I suspect if you add status=modified to the payload it may work as well.

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Jan 18, 2018
@marinus81
Copy link
Contributor Author

marinus81 commented Jan 18, 2018

I'll try to see if I can get some response from Cisco on this (not sure if I can open a TAC case for my LAB equipment). However as this behavior is consistent across two major releases I guess this is either a major flaw in their software or works as intended.

Some further investigations using postman revealed that without rsp-subtree=modified there is no response form APIC on the modified items at all (imdata in the response is always empty, regardless if something was modified on APIC or not). This seems to be in line with Cisco Documentation:

Filter strings can be applied to POST operations; if you want to retrieve the results of your POST operation in the response, for example, you can pass the rsp-subtree=modified query string to indicate that you want the response to include any objects that have been modified by your POST operation.

On the flip side this means (and some initial tests with postman confirm this): as long as rsp-subtree=modified is applied to the POST query string and no objects were modified by the request, the response will not contain any data (i.e. imdata is empty).
Taking this thought a bit further - if we could ensure that the URL aci_rest uses always has rsp-subtree=modified appended - wouldn't it be sufficient to check if imdata has any content? If it has, something has been modified - if it does not have any content nothing was modified

Examples from Postman:

First call (items do not yet exist):

POST  https://{{hostname}}/api/node/mo/uni/infra.xml?rsp-subtree=modified

Body:
<infraInfra>
  <cdpIfPol adminSt="disabled" descr="asdf" name="cdp_off2" />
  <cdpIfPol adminSt="enabled"  name="cdp_on2" />
</infraInfra>

-----------

RESPONSE Body:

<?xml version="1.0" encoding="UTF-8"?>
<imdata totalCount="1">
    <infraInfra childAction="deleteNonPresent" dn="uni/infra" lcOwn="local" modTs="2017-08-17T01:40:52.471+01:00" monPolDn="uni/fabric/monfab-default" name="infra" nameAlias="" ownerKey="" ownerTag="" rn="" status="" uid="0">
        <cdpIfPol adminSt="enabled" childAction="deleteNonPresent" descr="" extMngdBy="" lcOwn="local" modTs="2018-01-18T14:07:22.661+01:00" monPolDn="" name="cdp_on2" nameAlias="" ownerKey="" ownerTag="" rn="cdpIfP-cdp_on2" status="" uid="20488"/>
        <cdpIfPol adminSt="disabled" childAction="deleteNonPresent" descr="asdf" extMngdBy="" lcOwn="local" modTs="2018-01-18T14:07:22.661+01:00" monPolDn="" name="cdp_off2" nameAlias="" ownerKey="" ownerTag="" rn="cdpIfP-cdp_off2" status="" uid="20488"/>
    </infraInfra>
</imdata>

Note: imdata contains both cdpIfPol items (both did not exist, so both are modified) but status=""

Sending exactly the same POST request a second time:

POST  https://{{hostname}}/api/node/mo/uni/infra.xml?rsp-subtree=modified

Body:
<infraInfra>
  <cdpIfPol adminSt="disabled" descr="asdf" name="cdp_off2" />
  <cdpIfPol adminSt="enabled"  name="cdp_on2" />
</infraInfra>

-----------

RESPONSE Body:

<?xml version="1.0" encoding="UTF-8"?>
<imdata totalCount="0"></imdata>

Note: imdata does not contain any data as nothing was modified

Now modifying only one of the two items (changed descr from "asdf" to "fdsa"):

POST  https://{{hostname}}/api/node/mo/uni/infra.xml?rsp-subtree=modified

Body:
<infraInfra>
  <cdpIfPol adminSt="disabled" descr="fdsa" name="cdp_off2" />
  <cdpIfPol adminSt="enabled"  name="cdp_on2" />
</infraInfra>

-----------

RESPONSE Body:

<?xml version="1.0" encoding="UTF-8"?>
<imdata totalCount="1">
    <infraInfra childAction="deleteNonPresent" dn="uni/infra" lcOwn="local" modTs="2017-08-17T01:40:52.471+01:00" monPolDn="uni/fabric/monfab-default" name="infra" nameAlias="" ownerKey="" ownerTag="" rn="" status="" uid="0">
        <cdpIfPol adminSt="disabled" childAction="deleteNonPresent" descr="fdsa" extMngdBy="" lcOwn="local" modTs="2018-01-18T14:11:22.541+01:00" monPolDn="uni/fabric/monfab-default" name="cdp_off2" nameAlias="" ownerKey="" ownerTag="" rn="cdpIfP-cdp_off2" status="" uid="20488"/>
    </infraInfra>
</imdata>

Note: imdata contains only one cdpIfPol item (the one that was modified).

@dagwieers
Copy link
Contributor

So,

  1. Yes, aci_rest always provides rsp-subtree=modified to every POST/DELETE request.
  2. Yes, we check all objects to see if the status has either created, modified or deleted.

this is according to what the documentation states.

The issue you reported was related to the fact that if you provided a different path, there was no change reported even though 1. and 2. were OK. So to me that looks like a bug, albeit a consistent bug (and not a regression).

Regarding looking at the imdata content, I am not sure if that's always correct. Currently that is an assumption, although I guess you could open a second ticket to ask if your assumption is always correct.

@marinus81
Copy link
Contributor Author

I'm fairly new to collaborating on open source software, so please excuse if this is not the right place to discuss, or the wrong approach I took. That said - I believe I identified an issue with the aci_rest module which does not work as intended in all possible (but IMO valid) use cases. I also provided examples where this can be seen, as well as a brief root cause analysis. I also provided an idea how the root cause can be addressed (regardless if the root cause is a bug in the APIC REST API or in Cisco's documentation of it - I also reached out to my local Cisco contacts if they can get internal confirmation on this).
However, since I do not expect Cisco will fix the root cause for all of the ACI releases out there, I do strongly believe that my proposed solution is a) simpler than the current approach, and b) also addresses above issue.

Just to be sure - you're saying I should raise another issue and ask if the current implementation could be changed to my suggestion?

@dagwieers
Copy link
Contributor

dagwieers commented Jan 19, 2018

@marinus81 Your proposed solution is based on an assumption that I don't know if it holds true in all cases. That's why I would much rather stick to the implemented and documented solution rather than to implement a work-around of which we are not certain it will work in every case. Even if that means that aci_rest is as unreliable as the APIC itself for specific payloads.

The aci_rest module is a wrapper around the APIC REST API and is affected by any issues with the APIC REST API. So I am not a great fan of trying to fix APIC issues in the aci_rest module.

@rsmeyers
Copy link

Let's fix this on the right side, being, on APIC side, can you get me the SR you opened, I'll have somebody in my team yank it and work with engineering on this

@dagwieers
Copy link
Contributor

dagwieers commented Jan 19, 2018

@marinus81 I have been playing with your example and this does work too:

- name: Create CDP Policies
  aci_rest:
    host: "{{ apic_hostname }}"
    private_key: pki/admin.key
    method: post
    path: /api/node/mo/uni/infra.xml
    content: |
      <cdpIfPol adminSt="{{ item.state }}"  name="{{ item.name }}" descr="CDP set to {{ item.state }}"/>
  with_items:
  - "{{ cdp_policies }}"

Since you're already in the infra-namespace (dn=uni/infra), I don't think you need to repeat <infraInfra>.

@marinus81
Copy link
Contributor Author

@rsmeyers I opened a Cisco TAC case for this #683814655

@dagwieers Thanks, indeed this works - i did "repeat" <infraInfra> due to my initial tests with Postman where I created multiple <cdpIfPol> instances in the same Request (as I also did in the Postman examples above). If multiple <cdpIfPol> instances are created in one call, the APIC REST API requires <infraInfra>. Just for the sake of completeness (as unrelated to Ansible of course):

POST  https://{{hostname}}/api/node/mo/uni/infra.xml?rsp-subtree=modified
<cdpIfPol adminSt="disabled" descr="bbbb" name="cdp_off2" />

Response:
<?xml version="1.0" encoding="UTF-8"?>
<imdata totalCount="1">
    <cdpIfPol adminSt="disabled" childAction="deleteNonPresent" descr="bbbb" dn="uni/infra/cdpIfP-cdp_off2" extMngdBy="" lcOwn="local" modTs="2018-01-22T07:00:36.807+01:00" monPolDn="" name="cdp_off2" nameAlias="" ownerKey="" ownerTag="" rn="" status="created" uid="20488"/>
</imdata>
POST  https://{{hostname}}/api/node/mo/uni/infra.xml?rsp-subtree=modified
<cdpIfPol adminSt="disabled" descr="bbbb" name="cdp_off2" />
<cdpIfPol adminSt="enabled"  name="cdp_on2" />

Response:
<?xml version="1.0" encoding="UTF-8"?>
<imdata totalCount="1">
    <error code="122" text="class cdpIfPol not found"/>
</imdata>
POST  https://{{hostname}}/api/node/mo/uni/infra.xml?rsp-subtree=modified
<infraInfra>
  <cdpIfPol adminSt="disabled" descr="bbbb" name="cdp_off2" />
  <cdpIfPol adminSt="enabled"  name="cdp_on2" />
</infraInfra>

Response:
<?xml version="1.0" encoding="UTF-8"?>
<imdata totalCount="1">
    <infraInfra childAction="deleteNonPresent" dn="uni/infra" lcOwn="local" modTs="2017-08-17T01:40:52.471+01:00" monPolDn="uni/fabric/monfab-default" name="infra" nameAlias="" ownerKey="" ownerTag="" rn="" status="" uid="0">
        <cdpIfPol adminSt="enabled" childAction="deleteNonPresent" descr="" extMngdBy="" lcOwn="local" modTs="2018-01-22T07:03:03.063+01:00" monPolDn="" name="cdp_on2" nameAlias="" ownerKey="" ownerTag="" rn="cdpIfP-cdp_on2" status="" uid="20488"/>
    </infraInfra>
</imdata>

Also just for completeness (and I promise my last statement on this topic ;) - I understand a complete change in implementation logic is not done lightly): Should it every be necessary to revisit this - the suggested alternative implementation of change detection is not just an assumption, but also documented in the official API documentation:

The create or update operation should target a specific managed object, so the literal string /mo indicates that the DN of the managed object will be provided, followed next by the actual DN. Filter strings can be applied to POST operations; if you want to retrieve the results of your POST operation in the response, for example, you can pass the rsp-subtree=modified query string to indicate that you want the response to include any objects that have been modified by your POST operation.
Source

@dagwieers
Copy link
Contributor

dagwieers commented Jan 22, 2018

@marinus81 The assumption being that there are no other objects returned, for other reasons. For example, if there's an error, you get a totalCount > 0 with one or more error messages returned. That's one way additional items are being returned, and maybe there are other (future) changes to this that fulfill the referenced documentation.

So to repeat, our implementation relies on 2 documented behaviours. When using rsp-subtree=modified

  1. the APIC returns the modified objects (but maybe more?)
  2. the returned objects indicate status="created", status="modified" or status="deleted"

Now both are essential to me for it to work flawlessly, and 2. is failing in some specific cases. The solution is not to become less strict and only rely on one for aci_rest.

@rsmeyers
Copy link

@marinus81 I'm working with the current case-owner on this. We will keep you closely updated.

@dagwieers dagwieers added the waiting_on_vendor This issue requires actions by the vendor. Please inquire the vendor's help for any progress. label Jan 31, 2018
@dagwieers
Copy link
Contributor

dagwieers commented Feb 16, 2018

If you are affected by this bug, there is a clever workaround you can implement in your playbook (as hinted by @marinus81). Add the following to your aci_rest task:

- aci_rest:
    method: post
    <snip>
  register: this
  changed_when: this.imdata != []

This will only report a change when the APIC returns information, which usually happens when the APIC makes a change to the configuration.

@ogenstad
Copy link
Contributor

@rsmeyers, has there been any update on this from Cisco? To me it seems like a simple bug to be fixed within ACI.

@rsmeyers
Copy link

we are working with engineering on this and evaluating possible fixes in the code

@ansibot ansibot added bug This issue/PR relates to a bug. and removed bug_report labels Mar 1, 2018
@fhlmbrg
Copy link
Contributor

fhlmbrg commented Sep 14, 2018

Just stumbled upon this issue with APICs running 2.2(3j) not reporting a change as Changed using aci_rest. The workaround by @dagwieers @marinus81 fixes it for now.

@ansibot ansibot added support:core This issue/PR relates to code supported by the Ansible Engineering Team. and removed support:community This issue/PR relates to code supported by the Ansible community. labels Sep 14, 2018
@dagwieers
Copy link
Contributor

@holmahenkel If you can, please open a detailed ticket with Cisco to get your case resolved.

@ansibot ansibot added support:community This issue/PR relates to code supported by the Ansible community. support:certified This issue/PR relates to certified code. and removed support:core This issue/PR relates to code supported by the Ansible Engineering Team. support:community This issue/PR relates to code supported by the Ansible community. labels Oct 8, 2018
@dagwieers dagwieers added the cisco Cisco technologies label Feb 22, 2019
@ansibot
Copy link
Contributor

ansibot commented Mar 2, 2019

@ansibot
Copy link
Contributor

ansibot commented Mar 10, 2019

@ansibot
Copy link
Contributor

ansibot commented May 29, 2019

@ansibot
Copy link
Contributor

ansibot commented Feb 16, 2020

@aciguru
Copy link
Contributor

aciguru commented Apr 14, 2020

Moved to

CiscoDevNet/ansible-aci#28

@dagwieers
Copy link
Contributor

@gundalow Can you close this ticket? Thanks!

@ansible ansible locked and limited conversation to collaborators May 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
aci Cisco ACI community affects_2.4 This issue/PR affects Ansible v2.4 bug This issue/PR relates to a bug. cisco Cisco technologies module This issue/PR relates to a module. networking Network category support:certified This issue/PR relates to certified code. waiting_on_vendor This issue requires actions by the vendor. Please inquire the vendor's help for any progress.
Projects
None yet
Development

No branches or pull requests

8 participants