-
Notifications
You must be signed in to change notification settings - Fork 149
docs: Clarify first boot registration service #1751
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
base: main
Are you sure you want to change the base?
Conversation
41b068d to
de557b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request clarifies the documentation for a first-boot registration service. It improves the wording, fixes typos, removes a duplicated command, and corrects a significant bug in the example systemd service where a wrong path was used in ConditionPathExists. These changes greatly improve the clarity and correctness of the documentation. I have added one comment with a suggestion to further improve the robustness of the systemd service example, ensuring it can retry on failure and that credentials are securely cleaned up.
| ExecStartPre=/bin/rm -f /etc/management-client/.register-on-first-boot | ||
| ExecStart=/usr/bin/management-client register --activation-key ${CLIENT_ACTIVATION_KEY} | ||
| ExecStartPre=/bin/rm -f /etc/management-client/.run_next_boot | ||
| ExecStop=/bin/rm -f /etc/management-client/.credentials |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple of issues with the current service definition that could lead to unexpected behavior:
-
Registration retry: Using
ExecStartPreto remove the condition file (.register-on-first-boot) means that if the registration inExecStartfails (e.g., due to a transient network error), the service won't attempt to register again on the next boot. The condition file should only be removed upon successful registration. -
Credential cleanup: For a
Type=oneshotservice withoutRemainAfterExit=yes,ExecStopis not executed when theExecStartcommand completes. This means the credentials file (/etc/management-client/.credentials) will not be removed as intended, leaving secrets on the disk.
A more robust approach is to handle both the conditional file removal and credential cleanup within a single ExecStart command. The suggested change below implements this. The trap command ensures /etc/management-client/.credentials is removed when the shell exits, regardless of success or failure. The && ensures /etc/management-client/.register-on-first-boot is only removed if the registration command is successful.
| ExecStartPre=/bin/rm -f /etc/management-client/.register-on-first-boot | |
| ExecStart=/usr/bin/management-client register --activation-key ${CLIENT_ACTIVATION_KEY} | |
| ExecStartPre=/bin/rm -f /etc/management-client/.run_next_boot | |
| ExecStop=/bin/rm -f /etc/management-client/.credentials | |
| ExecStart=/bin/sh -c "trap '/bin/rm -f /etc/management-client/.credentials' EXIT; /usr/bin/management-client register --activation-key ${CLIENT_ACTIVATION_KEY} && /bin/rm -f /etc/management-client/.register-on-first-boot" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feedback is valid, but I didn't want to alter the existing behavior. For an example, I believe the current version is sufficient.
de557b8 to
2f4f834
Compare
|
Just make a rebase for |
The original text roughly contained the steps to set up a service to register with on the next (first) boot, but contained some minor issues. Aside from typos/phrasing: - It talked of 'startup' and 'next boot', but effectively this would only get executed on first boot. - Command `touch .run_next_boot` was contained twice. Signed-off-by: mhorky <mhorky@redhat.com>
2f4f834 to
cc615a0
Compare
The original text roughly contained the steps to set up a service to register with on the next (first) boot, but contained some minor issues. Aside from typos/phrasing:
touch .run_next_bootwas contained twice.