Fixes #36969 - return the actual exit status of registration command

[nofaralfasi]

No description provided.

[nofaralfasi]

This change will provide the correct exit status for a failing registration command.
However, it doesn't address the problem of the curl command failure because the bash command at the end of the pipeline (| bash) is executed regardless of the curl command's outcome. The pipeline's exit status depends on the last command (bash), which returns 0 even if the curl command fails.

@stejskalleos Any suggestions on how to fix this issue? I was thinking we could save the curl command output in a temporary file and execute it only in the case of a successful curl exit status.

[nofaralfasi]

Modified the curl command format to accurately reflect its exit status in case of a failure.

[nofaralfasi]

@ShimShtein can you please review?

[ShimShtein]

Now it looks much better.
Didn't test it, but as far as the code goes - LGTM.

[stejskalleos]

I've been looking into the templates:

• Global Registration
• Host_init_config_template

And I don't think the proposed fix with the tmp is the right way to go.

My testing:
Create file in rails.root/public/file.sh

#!/bin/sh

echo "YAY"!
exit 1

Run

curl "http://foreman.example.com/file.sh" | bash; echo $?
=>
YAY!
1

So, the exit code is passed correctly, problem is somewhere else.

In the host_init_config template, we use set +e & set -e, but nothing like that
in the Global registration. I believe that's the problem, we should unify the workflows, catch the errors properly and return error code, not 0.

See https://github.com/theforeman/foreman/blob/develop/app/views/unattended/provisioning_templates/host_init_config/host_init_config_default.erb#L108

[ShimShtein]

@ekohl Need some bash expertise here:
TL;DR: we want to fail the script if the curl command fails. The original solution curl... | bash doesn't work (it will always return status 0).

Is this an acceptable solution, or is there a better one that we don't recognize?

[stejskalleos]

My two cents:
The solution in the PR is not wrong or bad IMO, it just adds a lot more complexity to the command, which I don't think is a good user experience.
I was able to achieve desired behavior with set -o pipefail

set -o pipefail
curl -sSk http://zz.redhat.com | bash;echo $?
=>
curl: (6) Could not resolve host: zz.redhat.com
6

Technically speaking, aren't we trying to fix behavior that is expected and correct?
Maybe the fix is to configure user's CI pipeline with set -o pipefail

[evgeni]

I'd say using pipefail is correct (it's also what Ansible suggests in their linter: https://ansible.readthedocs.io/projects/lint/rules/risky-shell-pipe/) and that's what we should incorporate in the generated command?

So make the PR here generate something like

"set -o pipefail && curl -sS #{insecure} '#{registration_url(@smart_proxy)}#{args_query if args_query != '?'}' #{command_headers} | bash"

[stejskalleos]

that's what we should incorporate in the generated command?

IMHO we could just add it to the documentation.

[evgeni]

Why docs instead of here?

[stejskalleos]

to keep the command short as possible.
I know that with current length of the auth token it's not a big addition, but still ...

[goarsna]

I had a look at it. IMO set -o pipefail is definitely the way we should go. Personally I would also tend to extend the command as proposed by @evgeni. I have the feeling that adding it to the docs is a little bit unwieldy. Yes, it is only one extra bullet point in a list of 25 you have to follow when registering a host, but it's somehow interrupting the workflow between copying the generated curl command and pasting it on the host.

[ShimShtein]

I am also for automatic actions - the less we expect from the user, the less are chances for command failure.

[maximiliankolb]

Ping @goarsna I believe this is related to #9808

[ares]

LGTM

[stejskalleos]

🍏 LGTM

Let's get this in!

Thanks @nofaralfasi and everyone for the reviews