Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixes #17132: Call promote API in rudder-node-to-relay #2238

Closed
wants to merge 2 commits into from
Closed

Fixes #17132: Call promote API in rudder-node-to-relay #2238

wants to merge 2 commits into from

Conversation

ElaadF
Copy link
Member

@ElaadF ElaadF commented Apr 20, 2020

peckpeck
peckpeck reviewed Apr 20, 2020
View reviewed changes
rudder-webapp/SOURCES/rudder-node-to-relay Outdated
RUDDER_JSON="${RUDDER_VAR}/cfengine-community/inputs/rudder.json"

rudder_json_value() {
grep "$1" "${RUDDER_JSON}" | sed 's/.*"'$1'" *: *"\(.*\)",.*/\1/'
Copy link
Member

Choose a reason for hiding this comment

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

you read a json with grep and sed here but you use jq a few lines later

@ElaadF ElaadF force-pushed the ust_17132/call_promote_api_in_rudder_node_to_relay branch from 8b9828e to b26b2de Compare April 20, 2020 14:24
@ElaadF
Copy link
Member Author

ElaadF commented Apr 20, 2020

Commit modified

peckpeck
peckpeck reviewed Apr 20, 2020
View reviewed changes
rudder-webapp/SOURCES/rudder-node-to-relay Outdated
RUDDER_JSON="${RUDDER_VAR}/cfengine-community/inputs/rudder.json"

rudder_json_value() {
cat "${RUDDER_JSON}" | jq -r ".$1"
Copy link
Member

Choose a reason for hiding this comment

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

"cat file |" is an anti pattern, use "jq .. < file", even "jq .. file" since it is supported
moreover using -r with jq seems uselss to me

Copy link
Member

Choose a reason for hiding this comment

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

my bad i mixed it with -R

peckpeck
peckpeck reviewed Apr 20, 2020
View reviewed changes
rudder-webapp/SOURCES/rudder-node-to-relay Outdated
API_URL="https://127.0.0.1/rudder/api/latest/scaleoutrelay/promote/${RELAY_UUID}"
TOKEN_SYSTEM=$(cat /var/rudder/run/api-token)
curl_command="${DOWNLOAD_COMMAND} --header \"X-API-Token: ${TOKEN_SYSTEM}\" --request POST \"${API_URL}\""
response=$(eval ${curl_command} )
Copy link
Member

Choose a reason for hiding this comment

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

try avoiding eval as much as possible, because it is a frequent source of bugs
Here you could use : response=$(${DOWNLOAD_COMMAND} --header "X-API-Token: ${TOKEN_SYSTEM}" --request POST "${API_URL}")
provided that you make a little change to DOWNLOAD_COMMAND

Copy link
Member

Choose a reason for hiding this comment

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

you also need to check $? after this line in case of an error in the http query itself

peckpeck
peckpeck reviewed Apr 20, 2020
View reviewed changes
rudder-webapp/SOURCES/rudder-node-to-relay Outdated
CERTIFICATE_OPTION="--insecure"
fi

DOWNLOAD_COMMAND="curl --silent --show-error ${CERTIFICATE_OPTION} --location --proxy '' --globoff"
Copy link
Member

Choose a reason for hiding this comment

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

use --proxy= insteal of --proxy '' to make it usable without eval

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand that part, we don't want to use the proxy, so we let the parameter empty ? If yes we can't use --noproxy ?

Copy link
Member

Choose a reason for hiding this comment

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

--xx= is equivalent to --xx ''
but without '' which make it usable without eval
you can also use --noproxy in that case, but i guess this command was coy pasted from somewhere we didn't know the url hostname beforehand

peckpeck
peckpeck reviewed Apr 20, 2020
View reviewed changes
rudder-webapp/SOURCES/rudder-node-to-relay Outdated
## Directives
#######################################################################################################################
${ECHO} -n "INFO: Triggering promises generation..."
curl --proxy '' -k "https://localhost/rudder/api/deploy/reload"
Copy link
Member

Choose a reason for hiding this comment

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

why not use DOWNLOAD_COMMAND here and why 127.0.0.1 at one place and localhost in the other one
I prefer localhost

peckpeck
peckpeck reviewed Apr 20, 2020
View reviewed changes
rudder-webapp/SOURCES/rudder-node-to-relay Outdated
# Rudder is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
#
# You should have received a copy of the GNU General Public License
# along with Rudder. If not, see <http://www.gnu.org/licenses/>.
#
#####################################################################################

ECHO=/bin/echo
Copy link
Member

Choose a reason for hiding this comment

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

since we're at it: this is useless

@amousset
Copy link
Member

Should be distributed with the plugin or with rudder-agent

@ElaadF
Copy link
Member Author

ElaadF commented Apr 20, 2020

PR updated with a new commit

@ElaadF
Copy link
Member Author

ElaadF commented Apr 20, 2020

Commit modified

@ElaadF ElaadF force-pushed the ust_17132/call_promote_api_in_rudder_node_to_relay branch from fc82cb4 to 0c9807d Compare April 20, 2020 17:27
@VinceMacBuche VinceMacBuche changed the base branch from master to branches/rudder/6.1 April 20, 2020 23:00
@ElaadF ElaadF requested a review from peckpeck April 21, 2020 08:09
peckpeck
peckpeck reviewed Apr 21, 2020
View reviewed changes
rudder-webapp/SOURCES/rudder-node-to-relay Outdated
LDAP_SERVER='localhost'
LDAP_PORT='389'
if [ ${code} -ne 0 ]; then
echo "Failed to promote ${RELAY_UUID} to relay" 1>&2
Copy link
Member

Choose a reason for hiding this comment

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

diaplay the code here, it is always usefull for debugging

@ElaadF ElaadF force-pushed the ust_17132/call_promote_api_in_rudder_node_to_relay branch from 0c9807d to 47115fd Compare April 21, 2020 10:04
@ElaadF
Copy link
Member Author

ElaadF commented Apr 21, 2020

Commit modified

@ElaadF ElaadF requested a review from peckpeck April 21, 2020 10:04
@ElaadF ElaadF force-pushed the ust_17132/call_promote_api_in_rudder_node_to_relay branch from 47115fd to 8f497f2 Compare April 21, 2020 10:05
@ElaadF
Copy link
Member Author

ElaadF commented Apr 21, 2020

Commit modified

@ElaadF
Copy link
Member Author

ElaadF commented Apr 22, 2020

Like @amousset said, we should distributed it with the plugin scale-out-relay Normation/rudder-plugins#287

@ElaadF ElaadF closed this Apr 22, 2020
@VinceMacBuche VinceMacBuche reopened this Apr 22, 2020
@VinceMacBuche
Copy link
Member

closing we will make another pr to remove this file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amousset amousset Awaiting requested review from amousset

@Fdall Fdall Awaiting requested review from Fdall

@peckpeck peckpeck Awaiting requested review from peckpeck

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@ElaadF @amousset @VinceMacBuche @peckpeck