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

Add support for renewal hook scripts #20

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

andrew-vant
Copy link

#17 notes that support for renewal hooks would be useful. This manages the directories under /etc/letsencrypt/renewal-hooks.

I find something like this necessary for making certs available to non-root daemons -- in my case ejabberd.

@andrew-vant
Copy link
Author

Hrm. Not entirely sure why the CI build failed, or if it has anything to do with my addition. Its log seems to think the letsencrypt state isn't there.


certbot_renewal_{{ hookset }}_hook_{{ loop.index }}:
file.managed:
- name: /etc/letsencrypt/renewal-hooks/{{ hookset }}/{{ hook.split("/") | last }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should be able to supply whole scripts in model and not only source on salt filesystem.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand the request. Could you give an example?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest using metadata like this:

letsencrypt:
  client:
    hook:
      pre:
        # Use salt fileserver
        - source: salt://path/to/prehook1.sh
        # Or generate file by script as a content
        - script: |
            #!/bin/bash
            echo "Triggered pre-hook"

So you can supply scripts on salt filesystem or pass them directly.

Copy link
Author

@andrew-vant andrew-vant Dec 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting scripts directly in pillar fills me with fear and loathing, but I don't really have a good argument against supporting it, except perhaps that it would make the state more complicated. I'll see what I can do and then update the PR.

@andrew-vant
Copy link
Author

Updated with hook literals. I chose a slightly different pillar structure than suggested. I am still not sure why the CI check is failing.

@andrew-vant
Copy link
Author

Any further thoughts on this?

certbot_renewal_{{ hookset }}_phook_{{ loop.index }}:
file.managed:
- name: /etc/letsencrypt/renewal-hooks/{{ hookset }}/{{ basename }}
- contents_pillar: letsencrypt:client:pillarhooks:{{ hookset }}:{{ basename }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer structure that I proposed before:

letsencrypt:
  client:
    hook:
      pre:
        - source: salt://path/to/prehook1.sh
      deploy:
        - script: |
            #!/bin/bash
            echo "blahblah"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about it, but it seems awkward. First because the jinja logic gets tangled (lists of single-element dictionaries that require different file.managed arguments) and second because the - script notation has no way to specify a filename, so you're stuck with 'script1.sh', etc.

NixM0nk3y added a commit to NixM0nk3y/salt-formula-letsencrypt that referenced this pull request Aug 3, 2018
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.

2 participants