Skip to content

Commit

Permalink
Fix some index-references and warn about remaining (ansible#55727)
Browse files Browse the repository at this point in the history
The ACI MultiSite PATCH API has a deficiency requiring some objects to be referenced by index.
This can cause silent corruption on concurrent access when changing/removing on object as
the wrong object may be referenced.

This PR removes some references by index, and documents this issue for
others.
  • Loading branch information
dagwieers authored May 7, 2019
1 parent 2007a79 commit beca366
Show file tree
Hide file tree
Showing 11 changed files with 39 additions and 12 deletions.
3 changes: 1 addition & 2 deletions lib/ansible/modules/network/aci/mso_schema_site_anp.py
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,7 @@ def main():

if anp is not None and anp_ref in anps:
anp_idx = anps.index(anp_ref)
# FIXME: Changes based on index are DANGEROUS
anp_path = '/sites/{0}/anps/{1}'.format(site_template, anp_idx)
anp_path = '/sites/{0}/anps/{1}'.format(site_template, anp)
mso.existing = schema_obj['sites'][site_idx]['anps'][anp_idx]

if state == 'query':
Expand Down
5 changes: 2 additions & 3 deletions lib/ansible/modules/network/aci/mso_schema_site_anp_epg.py
Original file line number Diff line number Diff line change
Expand Up @@ -181,8 +181,7 @@ def main():
epgs = [e['epgRef'] for e in schema_obj['sites'][site_idx]['anps'][anp_idx]['epgs']]
if epg is not None and epg_ref in epgs:
epg_idx = epgs.index(epg_ref)
# FIXME: Changes based on index are DANGEROUS
epg_path = '/sites/{0}/anps/{1}/epgs/{2}'.format(site_template, anp_idx, epg_idx)
epg_path = '/sites/{0}/anps/{1}/epgs/{2}'.format(site_template, anp, epg)
mso.existing = schema_obj['sites'][site_idx]['anps'][anp_idx]['epgs'][epg_idx]

if state == 'query':
Expand All @@ -192,7 +191,7 @@ def main():
mso.fail_json(msg="EPG '{epg}' not found".format(epg=epg))
mso.exit_json()

epgs_path = '/sites/{0}/anps/{1}/epgs'.format(site_template, anp_idx)
epgs_path = '/sites/{0}/anps/{1}/epgs'.format(site_template, anp)
ops = []

mso.previous = mso.existing
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@
type: str
choices: [ absent, present, query ]
default: present
notes:
- The ACI MultiSite PATCH API has a deficiency requiring some objects to be referenced by index.
This can cause silent corruption on concurrent access when changing/removing on object as
the wrong object may be referenced. This module is affected by this deficiency.
seealso:
- module: mso_schema_site_anp_epg
- module: mso_schema_template_anp_epg
Expand Down Expand Up @@ -223,7 +227,7 @@ def main():
mso.fail_json(msg="Static leaf '{leaf}/{vlan}' not found".format(leaf=leaf, vlan=vlan))
mso.exit_json()

leafs_path = '/sites/{0}/anps/{1}/epgs/{2}/staticLeafs'.format(site_template, anp_idx, epg_idx)
leafs_path = '/sites/{0}/anps/{1}/epgs/{2}/staticLeafs'.format(site_template, anp, epg)
ops = []

mso.previous = mso.existing
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,10 @@
type: str
choices: [ absent, present, query ]
default: present
notes:
- The ACI MultiSite PATCH API has a deficiency requiring some objects to be referenced by index.
This can cause silent corruption on concurrent access when changing/removing on object as
the wrong object may be referenced. This module is affected by this deficiency.
seealso:
- module: mso_schema_site_anp_epg
- module: mso_schema_template_anp_epg
Expand Down Expand Up @@ -266,7 +270,7 @@ def main():
mso.fail_json(msg="Static port '{portpath}' not found".format(portpath=portpath))
mso.exit_json()

ports_path = '/sites/{0}/anps/{1}/epgs/{2}/staticPorts'.format(site_template, anp_idx, epg_idx)
ports_path = '/sites/{0}/anps/{1}/epgs/{2}/staticPorts'.format(site_template, anp, epg)
ops = []

mso.previous = mso.existing
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@
type: str
choices: [ absent, present, query ]
default: present
notes:
- The ACI MultiSite PATCH API has a deficiency requiring some objects to be referenced by index.
This can cause silent corruption on concurrent access when changing/removing on object as
the wrong object may be referenced. This module is affected by this deficiency.
seealso:
- module: mso_schema_site_anp_epg
- module: mso_schema_template_anp_epg_subnet
Expand Down Expand Up @@ -230,7 +234,7 @@ def main():
mso.fail_json(msg="Subnet '{subnet}' not found".format(subnet=subnet))
mso.exit_json()

subnets_path = '/sites/{0}/anps/{1}/epgs/{2}/subnets'.format(site_template, anp_idx, epg_idx)
subnets_path = '/sites/{0}/anps/{1}/epgs/{2}/subnets'.format(site_template, anp, epg)
ops = []

mso.previous = mso.existing
Expand Down
5 changes: 5 additions & 0 deletions lib/ansible/modules/network/aci/mso_schema_site_bd_l3out.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@
type: str
choices: [ absent, present, query ]
default: present
notes:
- The ACI MultiSite PATCH API has a deficiency requiring some objects to be referenced by index.
This can cause silent corruption on concurrent access when changing/removing on object as
the wrong object may be referenced. This module is affected by this deficiency.
seealso:
- module: mso_schema_site_bd
- module: mso_schema_template_bd
Expand Down Expand Up @@ -181,6 +185,7 @@ def main():
l3outs = schema_obj['sites'][site_idx]['bds'][bd_idx]['l3Outs']
if l3out is not None and l3out in l3outs:
l3out_idx = l3outs.index(l3out)
# FIXME: Changes based on index are DANGEROUS
l3out_path = '/sites/{0}/bds/{1}/l3Outs/{2}'.format(site_template, bd, l3out_idx)
mso.existing = schema_obj['sites'][site_idx]['bds'][bd_idx]['l3Outs'][l3out_idx]

Expand Down
4 changes: 4 additions & 0 deletions lib/ansible/modules/network/aci/mso_schema_site_bd_subnet.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@
type: str
choices: [ absent, present, query ]
default: present
notes:
- The ACI MultiSite PATCH API has a deficiency requiring some objects to be referenced by index.
This can cause silent corruption on concurrent access when changing/removing on object as
the wrong object may be referenced. This module is affected by this deficiency.
seealso:
- module: mso_schema_site_bd
- module: mso_schema_template_bd
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@
type: str
choices: [ absent, present, query ]
default: present
notes:
- The ACI MultiSite PATCH API has a deficiency requiring some objects to be referenced by index.
This can cause silent corruption on concurrent access when changing/removing on object as
the wrong object may be referenced. This module is affected by this deficiency.
seealso:
- module: mso_schema_site_vrf_region
- module: mso_schema_site_vrf_region_cidr_subnet
Expand Down Expand Up @@ -204,6 +208,7 @@ def main():
cidrs = [c['ip'] for c in schema_obj['sites'][site_idx]['vrfs'][vrf_idx]['regions'][region_idx]['cidrs']]
if cidr is not None and cidr in cidrs:
cidr_idx = cidrs.index(cidr)
# FIXME: Changes based on index are DANGEROUS
cidr_path = '/sites/{0}/vrfs/{1}/regions/{2}/cidrs/{3}'.format(site_template, vrf, region, cidr_idx)
mso.existing = schema_obj['sites'][site_idx]['vrfs'][vrf_idx]['regions'][region_idx]['cidrs'][cidr_idx]

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@
type: str
choices: [ absent, present, query ]
default: present
notes:
- The ACI MultiSite PATCH API has a deficiency requiring some objects to be referenced by index.
This can cause silent corruption on concurrent access when changing/removing on object as
the wrong object may be referenced. This module is affected by this deficiency.
seealso:
- module: mso_schema_site_vrf_region_cidr
- module: mso_schema_template_vrf
Expand Down Expand Up @@ -221,6 +225,7 @@ def main():
subnets = [s['ip'] for s in schema_obj['sites'][site_idx]['vrfs'][vrf_idx]['regions'][region_idx]['cidrs'][cidr_idx]['subnets']]
if subnet is not None and subnet in subnets:
subnet_idx = subnets.index(subnet)
# FIXME: Changes based on index are DANGEROUS
subnet_path = '/sites/{0}/vrfs/{1}/regions/{2}/cidrs/{3}/subnets/{4}'.format(site_template, vrf, region, cidr_idx, subnet_idx)
mso.existing = schema_obj['sites'][site_idx]['vrfs'][vrf_idx]['regions'][region_idx]['cidrs'][cidr_idx]['subnets'][subnet_idx]

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -214,8 +214,7 @@ def main():
contract_ref = mso.contract_ref(**contract)
if (contract_ref, contract['type']) in contracts:
contract_idx = contracts.index((contract_ref, contract['type']))
# FIXME: Changes based on index are DANGEROUS
contract_path = '/templates/{0}/anps/{1}/epgs/{2}/contractRelationships/{3}'.format(template, anp, epg, contract_idx)
contract_path = '/templates/{0}/anps/{1}/epgs/{2}/contractRelationships/{3}'.format(template, anp, epg, contract)
mso.existing = schema_obj['templates'][template_idx]['anps'][anp_idx]['epgs'][epg_idx]['contractRelationships'][contract_idx]

if state == 'query':
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -244,8 +244,7 @@ def main():
filter_ref = mso.filter_ref(schema_id=filter_schema_id, template=filter_template, filter=filter_name)
if filter_ref in filters:
filter_idx = filters.index(filter_ref)
# FIXME: Changes based on index are DANGEROUS
filter_path = '/templates/{0}/contracts/{1}/{2}/{3}'.format(template, contract, filter_key, filter_idx)
filter_path = '/templates/{0}/contracts/{1}/{2}/{3}'.format(template, contract, filter_key, filter_name)
mso.existing = schema_obj['templates'][template_idx]['contracts'][contract_idx][filter_key][filter_idx]

if state == 'query':
Expand Down

0 comments on commit beca366

Please sign in to comment.