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.

Sign up for GitHub

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

Jump to bottom

fix nxos_acl issues #38283

Merged
merged 4 commits into from
Apr 6, 2018
Merged

fix nxos_acl issues #38283

merged 4 commits into from
Apr 6, 2018

Conversation

saichint
Copy link
Contributor

@saichint saichint commented Apr 4, 2018 •
edited

SUMMARY

fixes #38282

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

nxos_acl

ANSIBLE VERSION
ansible 2.6.0 (devel fed20b825f) last updated 2018/02/15 12:51:12 (GMT -400)
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/root/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /root/agents-ci/ansible/lib/ansible
  executable location = /root/agents-ci/ansible/bin/ansible
  python version = 2.7.6 (default, Oct 26 2016, 20:30:19) [GCC 4.8.4]
ADDITIONAL INFORMATION
  • This PR fixes nxos_acl issues #38282
  • Integration tests enhanced for changes in configuration and idempotent checks.
  • Tested on all platforms and they pass

** Note: Due to to severity of the issue, please consider this PR for cherry picking to 2.5 **

@ansibot
Copy link
Contributor

ansibot commented Apr 4, 2018

cc @GGabriele @jedelman8 @mikewiebe @privateip @rahushen @rcarrillocruz @trishnaguha
click here for bot help

@ansibot ansibot added bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. networking Network category nxos Cisco NXOS community support:network This issue/PR relates to code supported by the Ansible Network Team. test This PR relates to tests. labels Apr 4, 2018
@trishnaguha trishnaguha self-assigned this Apr 5, 2018
@trishnaguha trishnaguha removed the needs_triage Needs a first human triage before being processed. label Apr 5, 2018
trishnaguha
trishnaguha reviewed Apr 5, 2018
View reviewed changes
lib/ansible/modules/network/nxos/nxos_acl.py
if not dcore:
# check the diff in the other way just in case
dcore = dict(
set(existing_core.items()).difference(
Copy link
Member

Choose a reason for hiding this comment

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

Wondering why check the other way around? Is this for idempotence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not just for idempotence. There are 2 ways the existing acl can be changed.

  1. we can add 1 or more parameters to the existing acl
  2. we can remove 1 or more parameters from the existing acl

The current code is only taking care of the first change. The 2nd one is being ignored and so if user wants to remove some parameters, there is no way, the playbook run does not even throw an error, it does not change anything and silently exits. The problem exists for both core and options.

Here is an example:

>>> proposed_options = {'c' : 'd'}

>>> existing_options = {'a' : 'b', 'c' : 'd'}

>>> dict(set(proposed_options.items()).difference(existing_options.items()))
{}

>>> dict(set(existing_options.items()).difference(proposed_options.items()))
{'a': 'b'}

So as you can see, the 2nd statement detects if the difference is in the removing part.
Since this is intended behavior, this is a bug.
You can check the documentation:

ansible/lib/ansible/modules/network/nxos/nxos_acl.py

Lines 44 to 45 in cdd21e2

If there is any difference, what is in Ansible will be pushed (configured
options will be overridden). This is to improve security, but at the

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. Makes sense.

@trishnaguha trishnaguha merged commit 1bf2965 into ansible:devel Apr 6, 2018
@trishnaguha trishnaguha added this to the 2.5.0 milestone Apr 6, 2018
@trishnaguha trishnaguha added this to Need CP into 2.5.1 in zzz NOT USED: Networking Bugs Apr 6, 2018
@trishnaguha
Copy link
Member

cherry-picked to 2.5

@trishnaguha trishnaguha moved this from Need CP into 2.5.1 to Done in zzz NOT USED: Networking Bugs Apr 6, 2018
trishnaguha pushed a commit to trishnaguha/ansible that referenced this pull request Apr 6, 2018
@trishnaguha
fix nxos_acl issues (ansible#38283)
e3e3fb6 
* fix nxos_acl issues

* typo fix

* typo fix in sanity.yaml

* another typo fix in sanity.yaml

(cherry picked from commit 1bf2965)
trishnaguha added a commit that referenced this pull request Apr 6, 2018
@trishnaguha
stable-2.5 nxos bugfix cherry-pick (#38372)
536dff5 
* nxos_vlan purge (#38202)

Signed-off-by: Trishna Guha <trishnaguha17@gmail.com>
(cherry picked from commit 119352b)

* fix nxos_aaa_server issues (#38117)

(cherry picked from commit 697c301)

* fix nxos_aaa_server_host issues (#38188)

(cherry picked from commit 24cc6b8)

* fix nxos_static_route issues (#37614)

* fix nxos_static_route issues

* remove nxos_static_route from ignore

(cherry picked from commit 0df5cfd)

* fix nxos_acl issues (#38283)

* fix nxos_acl issues

* typo fix

* typo fix in sanity.yaml

* another typo fix in sanity.yaml

(cherry picked from commit 1bf2965)

* nxos_acl_interface tests addition (#38230)

(cherry picked from commit b8cb382)

* update changelog with nxos bugfixes for 2.5

Signed-off-by: Trishna Guha <trishnaguha17@gmail.com>

* revert ignore.txt

Signed-off-by: Trishna Guha <trishnaguha17@gmail.com>
@saichint saichint deleted the acl branch April 6, 2018 15:19
ilicmilan pushed a commit to ilicmilan/ansible that referenced this pull request Nov 7, 2018
@ilicmilan
fix nxos_acl issues (ansible#38283)
2854780 
* fix nxos_acl issues

* typo fix

* typo fix in sanity.yaml

* another typo fix in sanity.yaml
@dagwieers dagwieers added the cisco Cisco technologies label Feb 23, 2019
@ansible ansible locked and limited conversation to collaborators Apr 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@trishnaguha trishnaguha trishnaguha approved these changes

Assignees

@trishnaguha trishnaguha

Labels
bug This issue/PR relates to a bug. cisco Cisco technologies core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. networking Network category nxos Cisco NXOS community support:network This issue/PR relates to code supported by the Ansible Network Team. test This PR relates to tests.
Projects
No open projects
zzz NOT USED: Networking Bugs
  
Done
Milestone
2.5.0
Development

Successfully merging this pull request may close these issues.

nxos_acl issues
4 participants
@saichint @ansibot @trishnaguha @dagwieers