foreman-documentation icon indicating copy to clipboard operation
foreman-documentation copied to clipboard
verified publisher icon theforeman

Remove instructions to place GCE key in /etc/foreman

Open ekohl opened this issue 7 months ago • 7 comments

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 40b118063bf980dbd92469793abfc1229b691d99 (https://github.com/theforeman/foreman-documentation/pull/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. 1137d3a971688a8bfef350962fdb1bc9afc17cae 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

  • [x] I am okay with my commits getting squashed when you merge this PR.
  • [x] 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.

ekohl avatar May 06 '25 14:05 ekohl

The PR preview for a759109204a93d694521a04542e298ed5d6c2675 is available at theforeman-foreman-documentation-preview-pr-3841.surge.sh

The following output files are affected by this PR:

show diff

show diff as HTML

github-actions[bot] avatar May 06 '25 14:05 github-actions[bot]

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

You might need double underscores. @ekohl

maximiliankolb avatar May 07 '25 06:05 maximiliankolb

@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?

ekohl avatar May 07 '25 08:05 ekohl

@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.

maximiliankolb avatar May 19 '25 13:05 maximiliankolb

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

ekohl avatar May 19 '25 14:05 ekohl

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?

maximiliankolb avatar May 19 '25 14:05 maximiliankolb

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

ekohl avatar May 19 '25 14:05 ekohl

@ekohl This suggestion is still open: https://github.com/theforeman/foreman-documentation/pull/3841#issuecomment-2857319044 But I can also look into this after merging this PR.

maximiliankolb avatar May 23 '25 07:05 maximiliankolb

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

Lennonka avatar Jun 23 '25 09:06 Lennonka

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

maximiliankolb avatar Jun 26 '25 12:06 maximiliankolb

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.

ekohl avatar Jun 26 '25 12:06 ekohl

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

Lennonka avatar Jun 26 '25 12:06 Lennonka

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

Lennonka avatar Jun 26 '25 12:06 Lennonka

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

Lennonka avatar Jul 07 '25 14:07 Lennonka

I've dropped the scp step.

ekohl avatar Jul 11 '25 15:07 ekohl