Salt: Multiple issues with libcloud_dns

[max-arnold]

Description of Issue/Question

I found so many issues while trying to use libcloud_dns, that it looks like it was never tested at all.

1. Documentation doesn't mention that libcloud is not a cloud module. Using terms like profile and provider is really confusing, and it looks like I'm not the only one who spent hours on this: #44390

salt-cloud docs also mention libcloud and providers/profiles https://docs.saltstack.com/en/latest/topics/cloud/#configuration-inheritance, so these features are easy to misunderstand.

1. The example just doesn't work:

https://docs.saltstack.com/en/latest/ref/states/all/salt.states.libcloud_dns.html

my-zone:
  libcloud_dns.zone_present:
    - name: mywebsite.com
    - profile: profile1


local:
----------
          ID: my-zone
    Function: libcloud_dns.zone_present
        Name: mywebsite.com
      Result: False
     Comment: Missing parameter domain for state libcloud_dns.zone_present
              Missing parameter type for state libcloud_dns.zone_present
     Changes:

1. Once I replaced name with domain and added type: master I've got this:

  File "/Users/user/.virtualenvs/salt/lib/python3.6/site-packages/salt/states/libcloud_dns.py", line 94, in zone_present
    result = __salt__['libcloud_dns.create_zone'](domain, profile, type)
  File "/Users/user/.virtualenvs/salt/lib/python3.6/site-packages/salt/modules/libcloud_dns.py", line 213, in create_zone
    zone = conn.create_record(domain, type=type, ttl=ttl)
TypeError: create_record() got an unexpected keyword argument 'ttl'

This is because create_zone calls conn.create_record instead of conn.create_zone: https://github.com/saltstack/salt/blob/develop/salt/modules/libcloud_dns.py#L213

Unit tests are nominally present, but unable to catch this, because they just test for True and don't care which function was actually invoked: https://github.com/saltstack/salt/blob/develop/tests/unit/states/test_libcloud_dns.py#L123

1. I fixed this too, then I got the red warning while running it with test=True:

    Warnings: 'name' is an invalid keyword argument for
              'libcloud_dns.zone_present'. If you were trying to pass additional
              data to be used in a template context, please populate 'context'
              with 'key: value' pairs. Your approach will work until Salt
              Fluorine is out. Please update your state files.

Maybe it indeed was a bad idea to rename name to domain?

1. On the second run the warning has gone, but I discovered that the state module didn't respect test=True and actually created the zone on step (4). This could lead to really bad production incidents, and violates one of the biggest expectations about Salt https://docs.saltstack.com/en/latest/ref/states/testing.html

Something like this (pretty lame and with some false positives, but anyway) could flag new state modules which do not handle the test=True for code review:

comm -23 <(find salt/states -name '*.py' | sort -u) <(find salt/states -name '*.py' -exec grep -l -E '__opts__.*test' \{\} \; | sort -u )

1. Now my state looks like this

my-zone:
  libcloud_dns.zone_present:
    - domain: mywebsite.com
    - type: master
    - profile: dns_profile1

and I'm about to add some DNS records from the example https://docs.saltstack.com/en/latest/ref/states/all/salt.states.libcloud_dns.html

my-zone:
  libcloud_dns.zone_present:
    - domain: mywebsite.com
    - type: master
    - profile: dns_profile1
  libcloud_dns.record_present:
    - name: www
    - zone: mywebsite.com
    - type: A
    - data: 12.34.32.3
    - profile: dns_profile1

Guess what?

local:
    Data failed to compile:
----------
    ID 'my-zone' in SLS 'dns' contains multiple state declarations of the same type

1. Ok, let's split the state, maybe it will finally work:

my-zone:
  libcloud_dns.zone_present:
    - domain: mywebsite.com
    - type: master
    - profile: dns_profile1

my-zone-records:
  libcloud_dns.record_present:
    - name: www
    - zone: mywebsite.com
    - type: A
    - data: 12.34.32.3
    - profile: dns_profile1

Nope!

local:
----------
          ID: my-zone-records
    Function: libcloud_dns.record_present
        Name: www
      Result: False
     Comment: An exception occurred in this state: Traceback (most recent call last):
                File "/Users/user/.virtualenvs/salt/lib/python3.6/site-packages/salt/state.py", line 1913, in call
                  **cdata['kwargs'])
                File "/Users/user/.virtualenvs/salt/lib/python3.6/site-packages/salt/loader.py", line 1898, in wrapper
                  return f(*args, **kwargs)
                File "/Users/user/.virtualenvs/salt/lib/python3.6/site-packages/salt/states/libcloud_dns.py", line 152, in record_present
                  type, data, profile)
                File "/Users/user/.virtualenvs/salt/lib/python3.6/site-packages/salt/modules/libcloud_dns.py", line 278, in create_record
                  return _simple_record(conn.create_record(name, zone, record_type, data))
                File "/Users/user/.virtualenvs/salt/lib/python3.6/site-packages/libcloud/dns/drivers/gandi.py", line 221, in create_record
                  if 'ttl' in extra:
              TypeError: argument of type 'NoneType' is not iterable
     Started: 00:16:42.041820
    Duration: 3150.661 ms
     Changes:

Maybe the state module was written against some old version of apache-libcloud, but the current iterface wants an extra dictionary which holds all the parameters, including ttl:

https://github.com/apache/libcloud/blob/trunk/libcloud/dns/drivers/gandi.py#L232
https://github.com/apache/libcloud/blob/trunk/libcloud/dns/drivers/route53.py#L190
https://github.com/apache/libcloud/blob/trunk/libcloud/dns/drivers/linode.py#L162

At this point I stopped going down this rabbit hole and decided to submit an issue. Sorry for being grumpy, but this is just bugs upon bugs and is really frustrating.

python-3.6.6, salt-2018.3.3, apache-libcloud-2.3.0

CC @tonybaloney

[tonybaloney]

I'll pick this up

[tonybaloney]

All issues raised fixed in #50296

TBH, this could also do with an integration test module. I've added a load of unit tests in the execution module which found all the bugs you raised (and a couple of others), mainly around kwarg naming and the extra dictionary.

[dwoz]

@max-arnold Thank you for reporting this.

@tonybaloney Thanks for the PR and quick turn around.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[max-arnold]

Still relevant

[stale[bot]]

Thank you for updating this issue. It is no longer marked as stale.