[BUG] boto_vpc.route_table_present - not idempotent

[dfidler]

Description

The _subnetnames and _subnetids parameters of the salt.states.boto_vpc.route_table_present method overwrite the existing subnet associations of the route table.

That means that if you have a state file that..

1. creates vpc
2. creates internet gateway
3. creates route table
4. creates subnets (and specify the route table)

This will create all of the objects just fine. However, if you run it again, the third step will remove all subnet associations from the route table, and then the fourth step will add them back.

This creates a hard dependency on the order in which the objects are created. You MUST create the subnets first and then, and only then, can you create the route table. Also, the route table state file must be updated to include the names/ids of any subnets that you try to create.

This also renders the "route_table_name" parameter of "subnet_present" useless.

It also makes it impossible to encapsulate the networking and application objects in a single set of state files. The creation of your route table + subnets MUST be a monolithic state file, which limits its usefulness.

I've put this as a bug, rather than an enhancement mostly because this behavior makes subnet_present::route_table_name useless if you're enforcing state with route_table_present. Otherwise, I would probably have raised this as an enhancement.

Setup

Setup Script

``` cat < /srv/salt/issue-setup.sls {% set profile = salt['pillar.get']('aws') %} {% set prefix = salt['pillar.get']('aws:resource_prefix', 'sedemo_') %} {% set region = 'eu-west-1' %} {{prefix}}vpc: boto_vpc.present: - name: {{ prefix }}vpc - region: {{ region }} - cidr_block: 172.16.0.0/16 - instance_tenancy: default - dns_hostnames: False - tags: App: SaltStack Name: {{ prefix }}vpc {{prefix}}ig: boto_vpc.internet_gateway_present: - name: {{ prefix }}ig - vpc_name: {{ prefix }}vpc - region: {{ region }} - tags: Name: {{ prefix }}ig - require: - {{prefix}}vpc {{prefix}}rt: boto_vpc.route_table_present: - name: {{ prefix }}rt - region: {{ region }} - vpc_name: {{ prefix }}vpc - routes: - destination_cidr_block: 0.0.0.0/0 internet_gateway_name: {{ prefix }}ig - tags: Name: {{ prefix }}rt - require: - {{prefix}}vpc - {{prefix}}ig {% for net in ['128', '129'] %} {{prefix}}net_{{net}}: boto_vpc.subnet_present: - name: {{ prefix }}net_{{ net }} - region: {{ region }} - cidr_block: 172.16.{{net}}.0/24 - vpc_name: {{prefix}}vpc - availability_zone: eu-west-1b - tags: Name: {{ prefix }}net_{{ net }} - route_table_name: {{ prefix }}rt - require: - {{prefix}}vpc - {{prefix}}rt {% endfor %} EOF ```

Steps to Reproduce the behavior

Run the following twice:

salt-call state.apply issue-setup

On the second run, you will see this...

############## SNIP  ##############
----------
          ID: dkf_rt
    Function: boto_vpc.route_table_present
      Result: True
     Comment: Route table dkf_rt (rtb-0009d30188aebb16b) present.
     Started: 21:49:47.278640
    Duration: 3067.967 ms
     Changes:
              ----------
              new:
              old:
                  - subnet-0bc4a6abaa06b7d04
                  - subnet-0f3e70b41c898fc97

Notice how the subnet associations have been deleted. This is going to cause a very brief network outage for all subnets in this routing table

----------
          ID: dkf_net_128
    Function: boto_vpc.subnet_present
      Result: True
     Comment: Subnet present. Subnet successfully associated with route table dkf_rt.
     Started: 21:49:50.347023
    Duration: 1579.11 ms
     Changes:
              ----------
              new:
                  ----------
                  subnet:
                      ----------
                      availability_zone:
                          eu-west-1b
                      cidr_block:
                          172.16.128.0/24
                      explicit_route_table_association_id:
                          rtbassoc-0156e973854a669ca
                      id:
                          subnet-0bc4a6abaa06b7d04
                      tags:
                          ----------
                          Name:
                              dkf_net_128
                      vpc_id:
                          vpc-0f7082f722d991fd5
              old:
                  ----------
                  subnet:
                      ----------
                      availability_zone:
                          eu-west-1b
                      cidr_block:
                          172.16.128.0/24
                      id:
                          subnet-0bc4a6abaa06b7d04
                      tags:
                          ----------
                          Name:
                              dkf_net_128
                      vpc_id:
                          vpc-0f7082f722d991fd5
----------
          ID: dkf_net_129
    Function: boto_vpc.subnet_present
      Result: True
     Comment: Subnet present. Subnet successfully associated with route table dkf_rt.
     Started: 21:49:51.926422
    Duration: 1662.893 ms
     Changes:
              ----------
              new:
                  ----------
                  subnet:
                      ----------
                      availability_zone:
                          eu-west-1b
                      cidr_block:
                          172.16.129.0/24
                      explicit_route_table_association_id:
                          rtbassoc-09cf473f15103bfe5
                      id:
                          subnet-0f3e70b41c898fc97
                      tags:
                          ----------
                          Name:
                              dkf_net_129
                      vpc_id:
                          vpc-0f7082f722d991fd5
              old:
                  ----------
                  subnet:
                      ----------
                      availability_zone:
                          eu-west-1b
                      cidr_block:
                          172.16.129.0/24
                      id:
                          subnet-0f3e70b41c898fc97
                      tags:
                          ----------
                          Name:
                              dkf_net_129
                      vpc_id:
                          vpc-0f7082f722d991fd5

And now all of the subnets have been re-associated with the route table.

Expected behavior

I would not expect the second execution of the state file to generate any changes.

Suggested solutions:

1. the route_tablepresent::subnet* params to be additive, rather than absolute, and then add an "overwrite_subnets={True, FALSE}" (and update the documentation to reflect the behavior change)
2. Add additional parameters (like salt.states.group does for group membership)

My personal preference is the first option rather than the second because it seems to be more intuitive to me - the addition of the "overwrite_subnets" makes the behavior explicit. Where as the second option creates bloat (IMHO).

There are probably other options that I've not thought about but those two are what immediately came to mind.

Versions Report

NOTE: I have applied https://github.com/saltstack/salt/commit/32ed7d05f0af1f07a0144cf5594370a5d65e70b3 to be able to create the routing tables in my sample script.

salt --versions-report

(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.) ``` Salt Version: Salt: 3002.5 Dependency Versions: cffi: 1.14.5 cherrypy: unknown dateutil: 2.8.1 docker-py: Not Installed gitdb: 0.6.4 gitpython: 1.0.1 Jinja2: 2.11.1 libgit2: Not Installed M2Crypto: 0.35.2 Mako: Not Installed msgpack: 0.6.2 msgpack-pure: Not Installed mysql-python: Not Installed pycparser: 2.20 pycrypto: Not Installed pycryptodome: 3.10.1 pygit2: Not Installed Python: 3.6.8 (default, Nov 16 2020, 16:55:22) python-gnupg: Not Installed PyYAML: 3.13 PyZMQ: 17.0.0 smmap: 0.9.0 timelib: Not Installed Tornado: 4.5.3 ZMQ: 4.1.4 System Versions: dist: centos 7 Core locale: UTF-8 machine: x86_64 release: 3.10.0-957.1.3.el7.x86_64 system: Linux version: CentOS Linux 7 Core ```

[sagetherage]

@dfidler v3002.5 is vulnerable, please upgrade to v3002.6