Skip to content

Remove instructions to place GCE key in /etc/foreman#3841

Merged
maximiliankolb merged 1 commit intotheforeman:masterfrom
ekohl:gce-key-remove-redundant-instructions
Aug 13, 2025
Merged

Remove instructions to place GCE key in /etc/foreman#3841
maximiliankolb merged 1 commit intotheforeman:masterfrom
ekohl:gce-key-remove-redundant-instructions

Conversation

@ekohl
Copy link
Member

@ekohl ekohl commented May 6, 2025

What changes are you introducing?

Remove the instructions to place the key in /etc/foreman.

Why are you introducing these changes? (Explanation, links to references, issues, etc.)

Hammer uploads the key to Foreman (just like in the UI procedure) and never read again. This means there's no point in storing the file in /etc/foreman with specific permissions.

It was introduced in 40b1180 (#1949).

Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)

I changed the Hammer command to run as root for two reasons: the scp command also uses root and the installer sets up Hammer for root by default. 1137d3a changed it to non-root and this goes against that.

An alternative is to change the instruction to a prerequisite to have Hammer set up and the key file present. Then you only need to list the Hammer command.

Checklists

  • I am okay with my commits getting squashed when you merge this PR.
  • I am familiar with the contributing guidelines.

Please cherry-pick my commits into:

  • Foreman 3.14/Katello 4.16
  • Foreman 3.13/Katello 4.15 (EL9 only)
  • Foreman 3.12/Katello 4.14 (Satellite 6.16; orcharhino 7.2 on EL9 only)
  • Foreman 3.11/Katello 4.13 (orcharhino 6.11 on EL8 only; orcharhino 7.0 on EL8+EL9; orcharhino 7.1 with Leapp)
  • Foreman 3.10/Katello 4.12
  • Foreman 3.9/Katello 4.11 (Satellite 6.15; orcharhino 6.8/6.9/6.10)
  • Foreman 3.8/Katello 4.10
  • Foreman 3.7/Katello 4.9 (Satellite 6.14)
  • We do not accept PRs for Foreman older than 3.7.

@github-actions github-actions bot added Needs tech review Requires a review from the technical perspective Needs style review Requires a review from docs style/grammar perspective Needs testing Requires functional testing labels May 6, 2025
@github-actions
Copy link

github-actions bot commented May 6, 2025

@maximiliankolb
Copy link
Contributor

One instance of "GCE_KEY" in the rendered docs is not italic:
image

You might need double underscores. @ekohl

@ekohl
Copy link
Member Author

ekohl commented May 7, 2025

@maximiliankolb did you have a look at my thoughts on whether we need the scp command at all and the run hammer as root or non-root option?

@maximiliankolb
Copy link
Contributor

@maximiliankolb did you have a look at my thoughts on whether we need the scp command at all and the run hammer as root or non-root option?

I had a glance and did not really have any input on it.

On second though: Maybe we should make it more explicit by prefixing the file name with /root/? This would indicate that a) the key is in the home dir of the root user and therefore not readable by others, and b) that you need to run the Hammer CLI command as root.

@ekohl
Copy link
Member Author

ekohl commented May 19, 2025

Should we drop the scp instruction and make it a prerequisite? Perhaps also add a recommendation to remove the file after uploading it?

@maximiliankolb
Copy link
Contributor

Maybe a prerequisite to create the JSON file on *.google.com and then in the procedure to either upload it via browser or scp it to Foreman Server?

@ekohl
Copy link
Member Author

ekohl commented May 19, 2025

Perhaps maybe this is good enough now and we can iterate on it when we feel the need to?

@maximiliankolb
Copy link
Contributor

@ekohl This suggestion is still open: #3841 (comment) But I can also look into this after merging this PR.

Copy link
Contributor

@Lennonka Lennonka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we could suggest an alternative path to the user that would promote a good practice where to put the key. Consider this optional for this PR.

@Lennonka Lennonka added tech review done No issues from the technical perspective style review done No issues from docs style/grammar perspective and removed Needs tech review Requires a review from the technical perspective Needs style review Requires a review from docs style/grammar perspective labels Jun 23, 2025
@Lennonka
Copy link
Contributor

And I think it might be a good idea for some to test the new procedure.

@maximiliankolb
Copy link
Contributor

triage: kindly asking for a test from team rocket. cc @Lennonka

@ekohl
Copy link
Member Author

ekohl commented Jun 26, 2025

I'm wondering if we could suggest an alternative path to the user that would promote a good practice where to put the key. Consider this optional for this PR.

I'd prefer if others can take over. This PR was more of an out-of-hand bug report.

Some notes, in case it wasn't clear. Perhaps we can add a step to remove the key afterwards because it's not used. That's the whole point of this PR: it's only read by Hammer and then uploaded to Foreman where it's stored in the database.

This also means the user can run the Hammer command from their own desktop (if they have Hammer installed & configured). They can drop the scp command too then.

@Lennonka
Copy link
Contributor

Oh, I missed that. Thank you. Adding a step to remove the key would make it clearer.

@Lennonka Lennonka removed the Needs testing Requires functional testing label Jun 26, 2025
@Lennonka
Copy link
Contributor

In that case, the location of the key is irrelevant and this probably doesn't need testing.

@Lennonka
Copy link
Contributor

Lennonka commented Jul 7, 2025

@ekohl Friendly reminder to add the step to remove the key

Hammer uploads the key to Foreman (just like in the UI procedure) and
never read again. This means there's no point in storing the file in
/etc/foreman with specific permissions.

Fixes: 40b1180 ("Remake GCE for Foreman Google plugin")
@ekohl ekohl force-pushed the gce-key-remove-redundant-instructions branch from 7259b30 to a759109 Compare July 11, 2025 15:22
@ekohl
Copy link
Member Author

ekohl commented Jul 11, 2025

I've dropped the scp step.

@maximiliankolb maximiliankolb requested a review from Lennonka July 14, 2025 09:39
@maximiliankolb maximiliankolb merged commit f00267f into theforeman:master Aug 13, 2025
9 of 10 checks passed
@ekohl ekohl deleted the gce-key-remove-redundant-instructions branch August 13, 2025 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

style review done No issues from docs style/grammar perspective tech review done No issues from the technical perspective

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants