Restructuring the module data about ansible-collection-smallstep

[maxhoesel]

The provisioner module is definitely the worst one to read, especially with the new x509 and ssh options. That said, I wouldn't exactly call it horrendous, it's a complex task that has many configurable options. Searching with Ctrl-F should work pretty well.

As for restructuring the variables, I can't see that happening anytime soon. Yes, it would look a little nicer in the documentation, but it also comes with a lot of drawbacks:

• It makes parsing the module parameters more complicated. Right now, most module parameters map to a step-cli parameter, which means that we can just pass them to step-cli and let the tool handle validation. It also means that our docs are basically just an Ansible-based copy of the upstream smallstep docs. If we move them to individual dicts with suboptions, we have to extract them on a module-by-module basis first, which complicates things.
• AFAIK, it's not common practice: Most Ansible modules that wrap around an API just pass the parameters without reorganizing them. Some (like the netbox modules have separation between connection and data variables, but that's about it. Adding our own data structures would be much more opinionated than I'd want these modules to be.
• Moving module parameters into suboptions also breaks backwards compatibility in a big way - everyone would have to rewrite their playbooks and I'm not willing to force that onto people for a minor doc readability improvement.

[LecrisUT]

About backwards compatibility, that is perfectly doable as long as names do not clash.

About the common practice, most modules do not have a complex mutually exclusive set of options with so many arguments. Those that do tend to organize it a little like in nmcli or a bit more hidden like in openstack/server/auth. If it can be avoided I agree it should, but when there are practical applications, we should reconsider.

As for the mapping to the step-cli options, that's a very valid point and I can agree with that. How about keeping both methods in parallel for that?

But for the practical reason of step_ca_provisioner_module, this will allow us to incorporate it into the role, be able to write a loop over dictionary items to add all provisioners, and be able to ensure a unique list of provisioners without having to keep track of old ones. The current alternative is to semi-hardcode the providers which is not ideal.

PS: about the documentation, the reason I say that one comes by horrendous (due to the automatic structuring of ansible docs) is because it is too difficult to navigate through it to figure out which additional arguments beyond the cli-mapped ones (like ca_url) are needed to get it working.

[maxhoesel]

But for the practical reason of step_ca_provisioner_module, this will allow us to incorporate it into the role, be able to write a loop over dictionary items to add all provisioners, and be able to ensure a unique list of provisioners without having to keep track of old ones. The current alternative is to semi-hardcode the providers which is not ideal.

Sorry, can you elaborate on what you mean by "be able to write a loop over dictionary items to add all provisioners"? You can already do something like this:

- name: Provisionners are present
  maxhoesel.smallstep.step_ca_provisioner: "{{ item | combine(_default_args) }}"
  vars:
    _default_args:
      ca_config: /etc/step-ca/config/ca.json
  loop:
    - name: some-acme-provisioner
      type: ACME
    - name: some-jwk-provisioner
      jwk_specific_option: value

Or you can use omit to do it like this:

- name: Provisioners are present
  maxhoesel.smallstep.step_ca_provisioner:
    name: "{{ item.name }}"
    type: "{{ item.type }}"
    jwk_option_1: "{{ item.jwk_option_1 | default(omit) }}"
    aws_option_1: "{{ item_awj_option_1 | default(omit) }}"
   ...
  loop:
    - name: ACME
      type: ACME
    - name: AWS
      type: AWS
      aws_option_1: value
    - name: JWK
      type: JWK
      jwk_option_1: other-value

Aside from that, I just don't really see a big enough benefit to justify this, sorry. I agree that the docs can make it hard to figure out what options are needed, but that's not just a problem with our collection, but rather upstream step-cli. I myself don't know what parameters are required for all of the supported provisioners, and there's no way to know until you try it out. The examples should be correct and help with this however.

And since outright replacing the variables is out of the question for compatibility concerns, we'd have to maintain both options in parallel for a good while, which would make the documentation even more dense. Plus, again, we'd be deviating from the upstream default.

[LecrisUT]

- name: Provisionners are present
  maxhoesel.smallstep.step_ca_provisioner: "{{ item | combine(_default_args) }}"
  vars:
    _default_args:
      ca_config: /etc/step-ca/config/ca.json
  loop:
    - name: some-acme-provisioner
      type: ACME
    - name: some-jwk-provisioner
      jwk_specific_option: value

Or you can use omit to do it like this:

- name: Provisioners are present
  maxhoesel.smallstep.step_ca_provisioner:
    name: "{{ item.name }}"
    type: "{{ item.type }}"
    jwk_option_1: "{{ item.jwk_option_1 | default(omit) }}"
    aws_option_1: "{{ item_awj_option_1 | default(omit) }}"
   ...
  loop:
    - name: ACME
      type: ACME
    - name: AWS
      type: AWS
      aws_option_1: value
    - name: JWK
      type: JWK
      jwk_option_1: other-value

This way does not allow having 2 JWK provisioners for example. I also think it is not good practice to allow passing the options for 2 different providers, because it is ambiguous which one the user really wanted to setup. The second option in principle avoids that, but it still does not allow to set a fixed set of provisioners.

What I think is better is to have the task look like this:

- name: Provide provisioners
  maxhoesel.smallstep.step_ca_provisioner:
    provisioners: "{{ step_ca_provisioners }}"
    ca_url: "{{ step_ca_url}}"

and the vars to be passed (including as simple role vars) to be:

step_ca_url: https://step-ca.example.com
step_ca_provisioners:
  - name: ACME
    type: ACME
  - name: JWK
    type: JWK
    jwk_option_1: value
  - name: AWS
    type: AWS
    jwk_option_1: this_should_fail

This way it guarantees a unique set of provisioners are setup. In the current method, we would have to keep track of old provisioners if we want to switch them out, i.e.:

- name: Provisionners are present
  maxhoesel.smallstep.step_ca_provisioner: "{{ item | combine(_default_args) }}"
  vars:
    _default_args:
      ca_config: /etc/step-ca/config/ca.json
  loop:
    - name: old_jwk
      state: absent
      type: JWK
    - name: new_jwk
      state: present

This way we can add a default JWK provisioner that is easily overwritten or re-included as such:

---
# defaults/main.yaml
step_ca_provisioners: "{% if step_ca_jwk_password %}{{ step_ca_default_provisioner}}{% else %}{{ omit }}{% endif %}"
---
# vars/main.yaml
step_ca_default_provisioner:
  name: JWK
  type: JWK
  jwk_password: "{{ step_ca_jwk_password | default('') }}"
---
# host_vars/host_that_uses_default.yaml
step_ca_url: https://step-ca.example.com
step_ca_jwk_passwork: sekret
---
# host_vars/host_that_expands_provisioners.yaml
step_ca_url: https://step-ca.example.com
step_ca_jwk_passwork: sekret
step_ca_provisioners:
  - "{{ step_ca_default_provisioner }}"
  - name: ACME
    type: ACME

I don't think the names are consistent with the current variables, and the if-else statement syntax might need some filter. I think also we do not have the yaml item to dictionary available for the last one, but we could design something similar around it.

I agree that the docs can make it hard to figure out what options are needed, but that's not just a problem with our collection, but rather upstream step-cli.

Well the upstream doc has half of the issue figured, i.e. grouping the available flags for each type and showing which ones are required (those that do not have square brackets). Paired and mutually exclusive flags are not shown in that format though.

And since outright replacing the variables is out of the question for compatibility concerns, we'd have to maintain both options in parallel for a good while, which would make the documentation even more dense. Plus, again, we'd be deviating from the upstream default.

How about separate step_ca_provisioner and step_ca_provisioners_list?