Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a list for dehydrated_domains #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jpiron
Copy link

@jpiron jpiron commented Feb 23, 2021

The README says it is a list while it is a string. Not sure why though.

@eengstrom
Copy link
Contributor

I don't know what @azielke will say, but I would argue that changing the documentation would be more useful. Yes, it's a string, but technically it's also a list of domains for which to have certs generated. The bonus of the string method is that you can embed comments into the string, and those are then put into the /etc/dehydrated/domains.txt file verbatim. e.g.:

# Wildcard
*.example.com example.com > STAR.example.com
# Website, covering ALL our domains
www.example.com example.com www.example.info example.info www.example.net example.net > www.example.com
# specialty project, which I should deprecate....
plan9.example.com

@jpiron
Copy link
Author

jpiron commented Feb 26, 2021

Comments could also be added with a list (even if it's less 'cleaner' than with a string) :

dehydrated_domains:
  - "# Wildcard"
  - "*.example.com example.com > STAR.example.com"

But IMO the main argument in favor of a list is that the role is likely to be used along with other apache/nginx/... roles which require some kind of vhost list (e.g: https://github.com/geerlingguy/ansible-role-apache/blob/master/defaults/main.yml#L28) and dehydrated_domains is likely to be set to something dynamic like: dehydrated_domains: "{{ apache_vhosts_ssl | map(attribute='servername') }}. Using a string requires the user to add a | join('\n') to the previous expression.

With a list we could also probably combine both dehydrated_domains and dehydrated_cert_config variables to something like:

dehydrated_domains:
  - name: foo.bar
    state: present 
    challengetype:
    wellknown:
    key_algo:
    keysize:

And it allows the feature to append new domains to request certificate for after dehydrated has been configured (see: 872636f).

@eengstrom
Copy link
Contributor

I get that you can embed comments in the list, so then it's a matter of style.

When you bring in the notion of generating the list of domains (as a list) based upon some other variable (e.g. apache_vhosts_ssl), then I see the use case a bit more. And then the ability to use the lineinfile: to modify the file is a clear win.

However, what happens when I want to remote a previous domain/cert? In that case the extra effort to use | join('\n') seems like it might be a better approach again.

As to your last point and commit ref, If I understand your entire process, you want to install dehydrated, and then allow other roles / playbooks to update the list of domains? Otherwise, why break off the configuration of the domains.txt file into a separate task file?

(note I'm not addressing your slightly more complicated dictionary approach to specify the domains - interesting, but not backward compatible - not sure what to make of it yet)

@jpiron
Copy link
Author

jpiron commented Mar 1, 2021

Indeed the removal is not handled by the list. This would require to add a state attribute, something like:

dehydrated_domains:
 - name: foo.bar
   state: present 

The last point is indeed about allowing other playbooks to update the list of domains as I tried to explain it in the commit message here. Let me know if it's not clear enough.

@azielke
Copy link
Contributor

azielke commented Apr 2, 2021

in its current state, it is a problem with the documentation. Currently, it's supposed to just be a simple string, so adding/removing etc... is handled instantly without having to have an "state: absent" item or similar present.

Looking at @jpiron's commit 872636f, if I see it right, your intention is just to add more and more domains to the domains.txt in different places that don't know about each other. This would mean, that just templating out the domains.txt wouldn't work, because it'd be overwritten with every call. So something lineinfile would be the logical choice for generating the domains.txt, but that would break backwards compatibilty, which I'd like to avoid (maybe let the role output a warning that dehydrated_domains as a string is deprecated for some time).

Both variants (list/dict) have their pros and cons, as I see it, a dict would allow for potential future dehydrated features without modifying this role, but a dict is better readable. But matching a dict with lineinfile on the domains.txt could be a challenge.

But I'd keep the domains and cert_config separate - mainly because they are two separate things in dehydrated currently. e.g. you can create 2 certificates for the same domains but with different key types or lengths.

azielke added a commit that referenced this pull request Apr 2, 2021
Modify documentation of dehydrated_domains to make it clearer, that it is a plain string (see #22)
@jpiron
Copy link
Author

jpiron commented Apr 22, 2021

Looking at @jpiron's commit 872636f, if I see it right, your intention is just to add more and more domains to the domains.txt

Indeed the current proposed implementation only handle addition, but it could be updated to something like:

dehydrated_domains:
      - name: domain_1.foo.bar
        state: present
      - name: domain_2.foo.bar
        state: absent

to manage both additions and removals.

But I'd keep the domains and cert_config separate - mainly because they are two separate things in dehydrated currently. e.g. you can create 2 certificates for the same domains but with different key types or lengths.

👍 I didn't know about that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants