ngine-io / ansible-collection-cloudstack

CloudStack Ansible Collections
https://galaxy.ansible.com/ngine_io/cloudstack
GNU General Public License v3.0
22 stars 30 forks source link

Missing argument for cs_firewall: destcidrlist #76

Open nathanmcgarvey opened 3 years ago

nathanmcgarvey commented 3 years ago

There is an argument to the "createEgressFirewallRule" api call named "destcidrlist". (Ref: https://cloudstack.apache.org/api/apidocs-4.15/apis/createEgressFirewallRule.html)

It seems that this was omitted from the cs_firewall module, or that that module wasn't updated to include destcidrlist when cloudstack 4.10 was released. (https://docs.cloudstack.apache.org/projects/archived-cloudstack-release-notes/en/4.10/api-changes.html)

This means that you cannot make an egress rule with limited destination subnets, though the very similar, but opposing "cidrlist" functionality seems to be implemented.

resmo commented 3 years ago

@nathanmcgarvey this should already work:

- name: Allow inbound port 80/tcp
  ngine_io.cloudstack.cs_firewall:
    ip_address: 4.3.2.1
    zone: zone01
    port: 80
    cidr: 
      - 1.2.3.4/32
      - 1.2.3.5/32

or

- name: Allow inbound port 80/tcp
  ngine_io.cloudstack.cs_firewall:
    ip_address: 4.3.2.1
    zone: zone01
    port: 80
    cidrs: 
      - 1.2.3.4/32
      - 1.2.3.5/32

could you confirm?

nathanmcgarvey commented 3 years ago

@resmo I think your example works for ingress rules, but not when doing egress rules. E.g:

- name: Allow only HTTP outbound traffic for an IP
  ngine_io.cloudstack.cs_firewall:
    network: my_network
    zone: zone01
    type: egress
    port: 80
    cidr: 10.101.1.20

That example from the source code allows for the egress from 10.101.1.20 to any destination subnet. If you want to limit that destination subnet, that's the destcidrlist API call that doesn't seem to exist in the module.

From https://cloudstack.apache.org/api/apidocs-4.15/apis/createEgressFirewallRule.html:

cidrlist | the cidr list to forward traffic from. Multiple entries must be separated by a single comma character (,). | false destcidrlist | the cidr list to forward traffic to. Multiple entries must be separated by a single comma character (,). | false

If I'm reading the source correctly, it's around here that the destcidrlist doesn't' exist:

Line 330 in cs_firewall.py:

           args = {
                'cidrlist': self.module.params.get('cidrs'),
                'protocol': self.module.params.get('protocol'),
                'startport': self.module.params.get('start_port'),
                'endport': self.get_or_fallback('end_port', 'start_port'),
                'icmptype': self.module.params.get('icmp_type'),
                'icmpcode': self.module.params.get('icmp_code')
            }

            fw_type = self.module.params.get('type')
            if not self.module.check_mode:
                if fw_type == 'egress':
                    args['networkid'] = self.get_network(key='id')
                    res = self.query_api('createEgressFirewallRule', **args)
resmo commented 3 years ago

Thanks for the details again and sorry for not reading it carefully enough the first time, I see destcidrlist was new with 4.10. Expect an implementation soon-ish.

resmo commented 3 years ago

resolved by #84