Skip to content
New issue

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

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

Already on GitHub? # to your account

add --cert-name and --deploy-hook options #230

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

Conversation

Hoeze
Copy link

@Hoeze Hoeze commented Jan 28, 2025

Can be used like this:

certbot_certs:
  - name: squid
    domains:
      - "{{ ansible_hostname }}.{{ domain_zone }}"
    deploy_hook: >-
      install -o squid -g squid -m 0600 $RENEWED_LINEAGE/fullchain.pem /etc/squid/fullchain.pem;
      install -o squid -g squid -m 0600 $RENEWED_LINEAGE/privkey.pem /etc/squid/privkey.pem;
      systemctl reload squid || true

@Hoeze
Copy link
Author

Hoeze commented Jan 28, 2025

Does someone know why this fails? Seems unrelated to my PR:

  fatal: [instance]: FAILED! => {"ansible_facts": {}, "changed": false, "failed_modules": {"ansible.legacy.setup": {"ansible_facts": {"discovered_interpreter_python": "/usr/bin/python3.9"}, "failed": true, "module_stderr": "sudo: PAM account management error: Authentication service cannot retrieve authentication info\nsudo: a password is required\n", "module_stdout": "", "msg": "MODULE FAILURE: No start of json char found\nSee stdout/stderr for the exact error", "rc": 1}}, "msg": "The following modules failed to execute: ansible.legacy.setup\n"}

@Hoeze Hoeze marked this pull request as ready for review January 29, 2025 12:17
@Hoeze Hoeze marked this pull request as draft January 29, 2025 13:32
@Hoeze Hoeze force-pushed the master branch 2 times, most recently from 0425d5d to 943abd8 Compare January 29, 2025 14:27
@Hoeze Hoeze marked this pull request as ready for review January 29, 2025 14:39
@Hoeze
Copy link
Author

Hoeze commented Jan 29, 2025

This PR fixes the CI issue and adds --cert-name and --deploy-hook.
Also, it automatically determines changes to the domain names of a cert before taking actions.

It would have been nice to have a second domain to test adding certificates in the CI.
Anyways, I just tested this PR back-and-forth on my current project, without issues.

@geerlingguy I would be very happy if you could review+merge this PR :)

@geerlingguy
Copy link
Owner

How do these changes interact with the updates from #208 from @theS1LV3R ? Sorry don't have time to do a full review at the current moment :)

@Hoeze
Copy link
Author

Hoeze commented Jan 29, 2025

  • Allow for certificates to be expanded to include new domains #208 relies on the certbot command output to determine whether running it changed anything.
    This could be problematic. At least in my case it led to requesting a new certificate each time, exhausting my request limit.
  • This PR checks first whether an update is necessary at all by comparing the list of domain names. Moreover, it enables us to remove additional domains. As far as I can tell, --expand only adds additional domains.

So at least for me, this PR makes --expand obsolete.
@geerlingguy I hope this helps.

@geerlingguy
Copy link
Owner

Is it possible to rebase this PR on master? I don't see the changes from https://github.com/geerlingguy/ansible-role-certbot/pull/208/files being updated, it looks like the branch forks from somewhere prior to current master.

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

Successfully merging this pull request may close these issues.

3 participants