Adding pre- and post-hook

[TreeN0de]

Exposing the pre-hook and the post-hook of acme.sh through the variable PRE_HOOK and POST_HOOK. Therefor it is possible to trigger actions just before and after a certificate is issued (see https://github.com/acmesh-official/acme.sh/wiki/Using-pre-hook-post-hook-renew-hook-reloadcmd)
For example you can change some firewallrules

[polarathene]

Resolves: #333

[buchdag]

@TreeN0de thanks for the PR, for this to be merged it'll need:

• to be rebased to main so that tests can pass (I can handle this)
• environment variables names beginning with ACME_
• documentation for the feature
• tests for the feature

Optionally but ideally it should also work both globally when set on the acme-companion container or per proxied container when set on them, with priority given to the per-container setting over the global setting when both are set.

I know tests are painful to write with the half assed test suite I contributed a few years ago, but they're the only way for me to have some confidence in the fact that feature X, Y or Z should work when someone opens an issue.

[TreeN0de]

Hello, this is my first time to make a PR.
I have renamed the environment variable like recommended and added a documentation for the feature.
Now I try to write a test-case for the feature. Maybe I will need some help there.

I agree the feature should be available for each proxied container. This could be the next step.

I keep you posted.

[TreeN0de]

@buchdag
While writing a testcase I ran into a problem. I followed the documentation for the local tests, but I was only able to get the setups with boulder running. I tried it with the main branch. It seams there is an independent problem with pebble.
For the test of the global pre- and posthook I need to handover multiple parameter to the acme-companion container. In addition the parameter contains blanks like --env “ACME_PRE_HOOK=touch /tmp/prehook”.

I managed to find a workaround be using global variables. Therefore I made changes in the test-suit. I modified the function run_le_container in the file test-functions.sh and I added the global variable to the files local_test_env.sh and setup-local.sh.

In my opinion this could only be a temporary solution. I think a better way is to modify the handling of parameter in the function run_le_container

What do you suggest?

[polarathene]

In addition the parameter contains blanks like --env “ACME_PRE_HOOK=touch /tmp/prehook”.

Use single quotes within the double quotes to give the ENV a space separated value assignment: --env “ACME_PRE_HOOK='touch /tmp/prehook'”

---

I don't have familiarity with boulder/pebble, if this is for providing a local PKI (ACME CA endpoint) for testing, I would suggest adopting Smallstep's one which is great and AFAIK easier to work with. They have an image available on DockerHub that can be used too at smallstep/step-ca or a binary you can run locally as a CLI tool.

I've used it for provisioning certs locally, and intend to add it to the test suite of a project I am a maintainer of, acme-companion will be one of the provisioning options covered in those tests since we document it for users. If it'd be of interest I can link that when I've done that work.

Not sure if it'd be any better than your current approach with boulder/pebble though.

[TreeN0de]

In addition the parameter contains blanks like --env “ACME_PRE_HOOK=touch /tmp/prehook”.

Use single quotes within the double quotes to give the ENV a space separated value assignment: --env “ACME_PRE_HOOK='touch /tmp/prehook'”

Thanks for your answer but that’s not the Problem I meant. With your solution the environment variable is a string not a command.
My problem is a call of function in the test suit. I need to call something like this:
run_le_container "${1:?}" "$le_container_name" "--env ACME_PRE_HOOK=touch /tmp/prehook’ --env ACME_POST_HOOK=touch /tmp/posthook"

But the function splits the parameter at the blanks. No matter what quotes I tried.

[polarathene]

How are you calling the value? (I haven't reviewed/looked at the PR sorry)

I think something like this would run it:"$(${MY_STRING_VAR})". The $() part is meant to run a subshell that executes the contents AFAIK.

Alternatively you could instead take the path to a shell script file, and call that instead?

[TreeN0de]

Hey,

the reason I couldn’t get the test suit running properly is, that I used the version of the time I created my branch. But since than there were changes to get the test suit running.
I ran also into an other error. I started with the setup with “boulder” and the I wanted to move on with “pebble”. I got the error pebble.minica.pem is a directory an not an file. So I was interested where the folder cam from in the “boulder” setup. In the function “run_le_container” the file “pebble.minica.pem” is linked into the container, no matter what setup is used. Therefore I moved the line into the “pebble” specific part of the function.

I wasn’t able to find a away to pass the needed values into the function run_le_container. The parameter handling of the function run_le_container was not able to handle an blank inside a parameter. So I modified the parameter handling. I followed the parameter handling of the function
run_nginx_container, but there was the same problem. If a value is able to contain an blank, there is no way to figure out on the receiving side weather the blank is part of the value or is the start of a new value. Therefore I decided, that every argument should be called in with an own flag. If you want to pass two arguments you should use the flag two times. For the other test cases I left the option with no flag like before. Maybe the other test case should be modified in this way.
I suggest to modify the function run_nginx_container in the same way. If you want to test pre- and posthooks per proxied container, there seams no way around. On the other hand, I would like to be proven wrong. This could save me a lot of headache.

Unfortunately I wasn’t able to get the Github workflow test running. On the local tests it passes. My test case didn’t make any output when it passes, but the workflow says unexpected output.

[TreeN0de]

After the rebase and just recreating the expected-std-out.txt file, all tests in the workflow seam to pass.

[TreeN0de]

@buchdag
Can you please run the workflow for the last commit. In my fork the workflow passes all checks.
I hope all conditions for the merge are checked.

Now I am working on setting the hooks in the proxied container. Therefor I started a new branch “pre/post-hook-proxied“ in my fork.
Every think seams to work.
Here the status:

• implemented test cases for the feature with default and options in the proxied container
• documentation for the usage
• passes the Workflow
  I would be great if someone else would test it too.

What is the next step for the PR?

[buchdag]

@TreeN0de sorry I haven't had the time to look at all your work yet, I'll do it asap but probably no earlier than next week. Looks very promising 👍

[buchdag]

What is the next step for the PR?

Squashing the commits together into a small number of meaningful commits grouped by type and/or scope, ideally using Conventional Commits but I can probably handle that (both the commit squashing and renaming), don't bother before I take a look at the full PR.

[polarathene]

Squashing commits for merge is pretty good. I noticed this repo merges all commit history into master/main, which seems prone to noisy history with some PRs.

With squash merge you don't have to request the contributor to cleanup commits or handle that yourself if it's regarding the merged history since you can have a single conventional commit merged instead and refer to the PR commit history when necessary.

[buchdag]

@TreeN0de I've reworked this PR to fix things like commit squash, shell linting, using conventional commits and some docs rewording, is it ok if I force push to your branch ? You can check what I've done here.

It should be almost ready to merge, the only thing bothering me is this line in the reworked parameter handling of run_le_container() :

cli_args_arr+=("$(echo "${cli_args_arr_tmp[@]:1}")") #Tail

It's triggering ShellCheck's SC2116 and I have a hard time figuring if we absolutely need to use echo or if we could do it in another way that would not require disabling this warning.

[buchdag]

I took the liberty to force push to TreeN0de-pre/post-hook so we can go forward with this feature.

I've fixed the caveat I had with the unnecessary use of echo triggering SC2116 and more importantly I added per-container hooks, with tests and documentation.