Spicerack puppetserver.destroy() raises an exception when certificate does not exist
Closed, ResolvedPublicBUG REPORT
Actions

Assigned To

Authored By

	taavi
	Mar 18 2024, 10:53 AM

Description

Calling Spicerack's puppetserver.destroy when a certificate does not exist fails with:

----- OUTPUT of 'sudo -i puppetse....wikimedia.cloud' -----
Error:
    Could not find files to clean for toolsbeta-static-2.toolsbeta.eqiad1.wikimedia.cloud
================
100.0% (1/1) of nodes failed to execute command 'sudo -i puppetse....wikimedia.cloud': toolsbeta-puppetserver-1.toolsbeta.eqiad1.wikimedia.cloud
0.0% (0/1) success ratio (< 100.0% threshold) for command: 'sudo -i puppetse....wikimedia.cloud'. Aborting.
0.0% (0/1) success ratio (< 100.0% threshold) of nodes successfully executed all commands. Aborting.
Exception raised while executing cookbook wmcs.vps.refresh_puppet_certs:
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/spicerack/_menu.py", line 250, in _run
    raw_ret = runner.run()
  File "/home/taavi/cookbooks_testing/cookbooks/wmcs_libs/common.py", line 751, in _wrapped_run
    return object.__getattribute__(self, __name)(*args, **kwargs)
  File "/home/taavi/cookbooks_testing/cookbooks/cookbooks/wmcs/vps/refresh_puppet_certs.py", line 146, in run
    _refresh_cert(spicerack=self.spicerack, remote_host=remote_host)
  File "/home/taavi/cookbooks_testing/cookbooks/cookbooks/wmcs/vps/refresh_puppet_certs.py", line 54, in _refresh_cert
    puppetserver.destroy(hostname=fqdn)
  File "/usr/lib/python3/dist-packages/spicerack/puppet.py", line 423, in destroy
    self.server_host.run_sync(f"puppetserver ca clean --certname {hostname}", print_progress_bars=False)
  File "/usr/lib/python3/dist-packages/spicerack/remote.py", line 514, in run_sync
    return self._execute(
  File "/usr/lib/python3/dist-packages/spicerack/remote.py", line 720, in _execute
    raise RemoteExecutionError(ret, "Cumin execution failed", worker.get_results())
spicerack.remote.RemoteExecutionError: Cumin execution failed (exit_code=2)

This is different from the old Puppet 5 Puppetserver behaviour, and does not match the documentation of that method which says:

If there is no certificate to remove it doesn't raise exception as the Puppet CA just outputs
'Nothing was deleted'.

Details

	Subject	Repo	Branch	Lines +/-
	puppet: PuppetServer.destroy improvement	operations/software/spicerack	master	+37 -4

Customize query in gerrit

Event Timeline

taavi created this task.Mar 18 2024, 10:53 AM

Restricted Application added a project: SRE-tools. · View Herald TranscriptMar 18 2024, 10:53 AM

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

That's indeed the current behaviour and clearly an error, thanks for reporting it!
The exit codes of the puppetserver ca clean command are not documented in Puppet, or at least I couldn't find them in the public docs/manpage/help messages/source code.
Ideally puppetserver should report two different set of errors, the ones in which there is a certificate but it failed to perform some cleaning operations and the one where the certificate does not exists at all, but it doesn't seem the case.
Given that it doesn't, I think we shouldn't rely on specific output messages of the CLI and exit codes as it could hide other errors now or in the future.

We have two possible approches, one is to update the documentation of spicerack to specify that it actually raises an error on missing ones.
The other is to check if it exists first with puppetserver ca list --certname $FQDN (with or without --format json) and ignore the missing ones.
Probably the latter is the better in terms of usability for the cookbook user. Thoughts?

Restricted Application added a project: Infrastructure-Foundations. · View Herald TranscriptMar 18 2024, 11:17 AM

If there would be a method to check whether a certificate exists or not we could update cookbooks to use that, in which case destroy raising an error would be fine.

We do have get_certificate_metadata() that raises spicerack.puppet.PuppetServerCheckError if the cert is not found (as opposed to other errors).
What I was suggesting is that we could do that check directly in destroy() in the puppetserver class so that it behaves the same of the old puppetmaster one.