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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6e8b72a
fix nxos_acl issues
Apr 4, 2018
649e061
typo fix
Apr 4, 2018
aa87337
typo fix in sanity.yaml
Apr 4, 2018
d38ce8e
another typo fix in sanity.yaml
Apr 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 28 additions & 5 deletions lib/ansible/modules/network/nxos/nxos_acl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@ def get_acl(module, acl_name, seq_number):
for acl in all_acl_body:
if acl.get('acl_name') == acl_name:
acl_body = acl
break

try:
acl_entries = acl_body['TABLE_seqno']['ROW_seqno']
Expand All @@ -226,7 +227,7 @@ def get_acl(module, acl_name, seq_number):
temp['action'] = 'remark'
else:
temp['action'] = each.get('permitdeny')
temp['proto'] = each.get('proto', each.get('proto_str', each.get('ip')))
temp['proto'] = str(each.get('proto', each.get('proto_str', each.get('ip'))))
temp['src'] = each.get('src_any', each.get('src_ip_prefix'))
temp['src_port_op'] = each.get('src_port_op')
temp['src_port1'] = each.get('src_port1_num')
Expand Down Expand Up @@ -458,13 +459,35 @@ def main():
delta_options = {}

if not existing_core.get('remark'):
delta_core = dict(
dcore = dict(
set(proposed_core.items()).difference(
existing_core.items())
)
delta_options = dict(
set(proposed_options.items()).difference(
existing_options.items())
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.

proposed_core.items())
)
delta_core = dcore
if delta_core:
delta_options = proposed_options
else:
doptions = dict(
set(proposed_options.items()).difference(
existing_options.items())
)
# check the diff in the other way just in case
if not doptions:
doptions = dict(
set(existing_options.items()).difference(
proposed_options.items())
)
delta_options = doptions
else:
delta_core = dict(
set(proposed_core.items()).difference(
existing_core.items())
)

if state == 'present':
Expand Down
184 changes: 180 additions & 4 deletions test/integration/targets/nxos_acl/tests/common/sanity.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@
nxos_acl: &remove
name: TEST_ACL
seq: 10
state: absent
state: delete_acl
provider: "{{ connection }}"
ignore_errors: yes

- name: "Configure ACL"
nxos_acl: &configure
- name: "Configure ACE10"
nxos_acl: &conf10
name: TEST_ACL
seq: 10
action: permit
Expand All @@ -27,6 +27,8 @@
ack: 'enable'
dscp: 'af43'
dest: any
dest_port_op: neq
dest_port1: 1899
urg: 'enable'
psh: 'enable'
established: 'enable'
Expand All @@ -44,13 +46,187 @@
- "result.changed == true"

- name: "Check Idempotence"
nxos_acl: *configure
nxos_acl: *conf10
register: result

- assert: &false
that:
- "result.changed == false"

- name: "Change ACE10"
nxos_acl: &chg10
name: TEST_ACL
seq: 10
action: deny
proto: tcp
src: 1.1.1.1/24
src_port_op: range
src_port1: 1900
src_port2: 1910
ack: 'enable'
dscp: 'af43'
dest: any
dest_port_op: neq
dest_port1: 1899
urg: 'enable'
psh: 'enable'
established: 'enable'
log: 'enable'
fin: 'enable'
rst: 'enable'
syn: 'enable'
time_range: "{{time_range|default(omit)}}"
state: present
provider: "{{ connection }}"
register: result

- assert: *true

- name: "Check Idempotence"
nxos_acl: *chg10
register: result

- assert: *false

- name: "ace remark"
nxos_acl: &remark
name: TEST_ACL
seq: 20
action: remark
remark: test_remark
state: present
provider: "{{ connection }}"
register: result

- assert: *true

- name: "Check Idempotence"
nxos_acl: *remark
register: result

- assert: *false

- name: "change remark"
nxos_acl: &chgremark
name: TEST_ACL
seq: 20
action: remark
remark: changed_remark
state: present
provider: "{{ connection }}"
register: result

- assert: *true

- name: "Check Idempotence"
nxos_acl: *chgremark
register: result

- assert: *false

- name: "ace 30"
nxos_acl: &ace30
name: TEST_ACL
seq: 30
action: deny
proto: 24
src: any
dest: any
fragments: enable
precedence: network
state: present
provider: "{{ connection }}"
register: result

- assert: *true

- name: "Check Idempotence"
nxos_acl: *ace30
register: result

- assert: *false

- name: "change ace 30 options"
nxos_acl: &chgace30opt
name: TEST_ACL
seq: 30
action: deny
proto: 24
src: any
dest: any
precedence: network
state: present
provider: "{{ connection }}"
register: result

- assert: *true

- name: "Check Idempotence"
nxos_acl: *chgace30opt
register: result

- assert: *false

- name: "ace 40"
nxos_acl: &ace40
name: TEST_ACL
seq: 40
action: permit
proto: udp
src: any
src_port_op: neq
src_port1: 1200
dest: any
precedence: network
state: present
provider: "{{ connection }}"
register: result

- assert: *true

- name: "Check Idempotence"
nxos_acl: *ace40
register: result

- assert: *false

- name: "change ace 40"
nxos_acl: &chgace40
name: TEST_ACL
seq: 40
action: permit
proto: udp
src: any
dest: any
precedence: network
state: present
provider: "{{ connection }}"
register: result

- assert: *true

- name: "Check Idempotence"
nxos_acl: *chgace40
register: result

- assert: *false

- name: "remove ace 30"
nxos_acl: &remace30
name: TEST_ACL
seq: 30
state: absent
provider: "{{ connection }}"
register: result

- assert: *true

- name: "Check Idempotence"
nxos_acl: *remace30
register: result

- assert: *false

- name: "Remove ACL"
nxos_acl: *remove
register: result
Expand Down