Add auto_sign option to configure_puppet() - #22221
Draft
lpramuk wants to merge 1 commit into
Draft
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds an auto_sign option to configure_puppet(), switches proxy host from Host to Capsule to leverage smart proxy APIs, and improves error handling and documentation around Puppet certificate signing. Sequence diagram for configure_puppet auto_sign behaviorsequenceDiagram
participant ContentHost
participant Capsule
participant NailgunSmartProxy
ContentHost->>ContentHost: execute('/opt/puppetlabs/bin/puppet agent -t')
ContentHost->>Capsule: Capsule(hostname, ipv6)
alt auto_sign True
ContentHost->>NailgunSmartProxy: add_autosign_entry(cert_name)
else auto_sign False
ContentHost->>Capsule: execute('puppetserver ca sign --certname cert_name')
alt [sign command failed]
ContentHost->>Capsule: execute('tail -2 /var/log/puppetlabs/puppetserver/puppetserver.log')
ContentHost->>ContentHost: raise ContentHostError
end
end
opt run_agent True
ContentHost->>ContentHost: execute('/opt/puppetlabs/bin/puppet agent -t')
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- When
auto_sign=True, the agent run happens before the autosign entry is created, which may prevent the CSR from being auto-signed; consider adding the autosign entry before the first puppet agent invocation. - The new error path tails the puppetserver log but discards the content; consider including relevant log or command output in the
ContentHostErrormessage to aid debugging. - The
Capsulesubstitution forHostchanges the type ofproxy_host; please confirm that all methods used here (execute,nailgun_smart_proxy) behave consistently and add error handling aroundadd_autosign_entrysimilar to the manual sign path.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When `auto_sign=True`, the agent run happens before the autosign entry is created, which may prevent the CSR from being auto-signed; consider adding the autosign entry before the first puppet agent invocation.
- The new error path tails the puppetserver log but discards the content; consider including relevant log or command output in the `ContentHostError` message to aid debugging.
- The `Capsule` substitution for `Host` changes the type of `proxy_host`; please confirm that all methods used here (`execute`, `nailgun_smart_proxy`) behave consistently and add error handling around `add_autosign_entry` similar to the manual sign path.
## Individual Comments
### Comment 1
<location path="robottelo/hosts.py" line_range="1070-1074" />
<code_context>
# sat6 under the capsule --> certificates or on capsule via cli "puppetserver
# ca list", so that we sign it.
self.execute('/opt/puppetlabs/bin/puppet agent -t')
- proxy_host = Host(hostname=proxy_hostname, ipv6=self.network_type == NetworkType.IPV6)
- proxy_host.execute(f'puppetserver ca sign --certname {cert_name}')
+
+ proxy_host = Capsule(hostname=proxy_hostname, ipv6=self.network_type == NetworkType.IPV6)
+ if auto_sign:
+ proxy_host.nailgun_smart_proxy.add_autosign_entry(cert_name)
+ else:
+ result = proxy_host.execute(f'puppetserver ca sign --certname {cert_name}')
</code_context>
<issue_to_address>
**issue (bug_risk):** Auto-sign entry is added after the agent run, which may be too late to affect the current CSR.
Because the agent run (which submits the CSR) happens before the auto-sign entry is added, this run may still require manual signing and only future runs benefit from auto-sign. When `auto_sign` is enabled, add the auto-sign entry on the capsule before invoking `puppet agent -t` so the current CSR is signed automatically.
</issue_to_address>
### Comment 2
<location path="robottelo/hosts.py" line_range="1072-1075" />
<code_context>
+ if auto_sign:
+ proxy_host.nailgun_smart_proxy.add_autosign_entry(cert_name)
+ else:
+ result = proxy_host.execute(f'puppetserver ca sign --certname {cert_name}')
+ if result.status:
+ proxy_host.execute('tail -2 /var/log/puppetlabs/puppetserver/puppetserver.log')
+ raise ContentHostError(
+ f'Failed to sign the puppet certificate on the capsule {proxy_hostname}'
+ )
</code_context>
<issue_to_address>
**suggestion:** Error handling for certificate signing could expose more context from the failing command.
Right now the raised `ContentHostError` only reports the capsule hostname. Consider including `result.stderr` (and/or `stdout`) in the exception message so callers can see the underlying `puppetserver ca sign` failure reason without having to check external logs, which will simplify debugging in automated runs.
```suggestion
proxy_host = Capsule(hostname=proxy_hostname, ipv6=self.network_type == NetworkType.IPV6)
if auto_sign:
proxy_host.nailgun_smart_proxy.add_autosign_entry(cert_name)
else:
result = proxy_host.execute(f'puppetserver ca sign --certname {cert_name}')
if result.status:
proxy_host.execute('tail -2 /var/log/puppetlabs/puppetserver/puppetserver.log')
raise ContentHostError(
f'Failed to sign the puppet certificate on the capsule {proxy_hostname}. '
f'stderr: {result.stderr!r}, stdout: {result.stdout!r}'
)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
1070
to
+1074
| self.execute('/opt/puppetlabs/bin/puppet agent -t') | ||
| proxy_host = Host(hostname=proxy_hostname, ipv6=self.network_type == NetworkType.IPV6) | ||
| proxy_host.execute(f'puppetserver ca sign --certname {cert_name}') | ||
|
|
||
| proxy_host = Capsule(hostname=proxy_hostname, ipv6=self.network_type == NetworkType.IPV6) | ||
| if auto_sign: | ||
| proxy_host.nailgun_smart_proxy.add_autosign_entry(cert_name) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Auto-sign entry is added after the agent run, which may be too late to affect the current CSR.
Because the agent run (which submits the CSR) happens before the auto-sign entry is added, this run may still require manual signing and only future runs benefit from auto-sign. When auto_sign is enabled, add the auto-sign entry on the capsule before invoking puppet agent -t so the current CSR is signed automatically.
Comment on lines
+1072
to
+1075
| proxy_host = Capsule(hostname=proxy_hostname, ipv6=self.network_type == NetworkType.IPV6) | ||
| if auto_sign: | ||
| proxy_host.nailgun_smart_proxy.add_autosign_entry(cert_name) | ||
| else: |
Contributor
There was a problem hiding this comment.
suggestion: Error handling for certificate signing could expose more context from the failing command.
Right now the raised ContentHostError only reports the capsule hostname. Consider including result.stderr (and/or stdout) in the exception message so callers can see the underlying puppetserver ca sign failure reason without having to check external logs, which will simplify debugging in automated runs.
Suggested change
| proxy_host = Capsule(hostname=proxy_hostname, ipv6=self.network_type == NetworkType.IPV6) | |
| if auto_sign: | |
| proxy_host.nailgun_smart_proxy.add_autosign_entry(cert_name) | |
| else: | |
| proxy_host = Capsule(hostname=proxy_hostname, ipv6=self.network_type == NetworkType.IPV6) | |
| if auto_sign: | |
| proxy_host.nailgun_smart_proxy.add_autosign_entry(cert_name) | |
| else: | |
| result = proxy_host.execute(f'puppetserver ca sign --certname {cert_name}') | |
| if result.status: | |
| proxy_host.execute('tail -2 /var/log/puppetlabs/puppetserver/puppetserver.log') | |
| raise ContentHostError( | |
| f'Failed to sign the puppet certificate on the capsule {proxy_hostname}. ' | |
| f'stderr: {result.stderr!r}, stdout: {result.stdout!r}' | |
| ) |
lpramuk
marked this pull request as draft
July 22, 2026 17:39
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
auto_sign=Falseparameter toconfigure_puppet()— whenTrue, creates an autosign entry vianailgun_smart_proxy.add_autosign_entry()instead of manually signing the certificateproxy_hostfrom plainHosttoCapsuleto get access tonailgun_smart_proxyTest plan
configure_puppet()withoutauto_signstill manually signs the cert as beforeconfigure_puppet(auto_sign=True)creates an autosign entry instead🤖 Generated with Claude Code
Summary by Sourcery
Add optional automatic Puppet certificate signing support when configuring Puppet on a host.
New Features:
Enhancements: