Moving the local gear override, to later in the file

[sferich888]

Enterprise Customer who would like to have a consistent workflow with external database's need the "capability" to override defined function. The simplest solution is to move the load of a gear defined profile to later in the file allowing override functions to be defined.

@a13m @joelsmith @tiwillia can you all review this.

[tiwillia]

@sferich888, We probably don't want users to be able to override some of these functions, such as help, rm, ctl_all, among others. It would be best to move only the database functions we want users to be able to override above the sourcing of the .bash_profile.

[sferich888]

@tiwillia I figured that was the case but was not sure what functions we want to allow override on. In the context of online I'm not sure you want to allow any overrides

[tiwillia]

There should be no reason to overwrite any of these functions. If, for example, the mongo command needs to be used to connect to an external database, a user can use scl enable mongodb24 mongo instead. The same goes for the rest of the database helper methods.

@abhgupta should users be able to override any of the functions defined in rhcsh? Specifcially looking at the mysql, psql, and mongo helper functions. This seems a little unnecessary.

[sferich888]

@tiwillia In short what your saying is that this, has important code that should not be overidded, among other examples.

[abhgupta]

@tiwillia lets discuss this tomorrow

[sferich888]

Is likely better to solve this with a different type of override.

diff --git a/node/misc/bin/rhcsh b/node/misc/bin/rhcsh
index cd6e380..c504433 100755
--- a/node/misc/bin/rhcsh
+++ b/node/misc/bin/rhcsh
@@ -228,6 +228,9 @@ function mongo () {
     if [[ -n "${OPENSHIFT_MONGODB_DB_GEAR_DNS}" ]] && [[ $(/usr/bin/scl -l) =~ mongodb24 ]]
     then
         scl enable mongodb24 "$(printf %q\  "${cmdline[@]}")"
+    elif [[ -n "${OPENSHIFT_MONGODB_DB_EXTERNAL}" ]] && [[ ${OPENSHIFT_MONGODB_DB_EXTERNAL} == "True" ]]
+    then 
+         [-f ~/app-root/data/.bash_mongo_override ] && source ~/app-root/data/.bash_mongo_override
     else
         command "${cmdline[@]}"
     fi

This allows someone to use an ENV to pick up an override file, and then determine how they want to run the DB.

[abhgupta]

@sferich888 Just had a discussion with @tiwillia and our initial take is that there doesn't seem to be any real harm in letting the user just override any/all of these functions. These functions are run as the gear user and not as root. Also, these restrictions (blocking mco) and precautions (checking for rm -rf of the git repo) can be easily overcome by the user. These restrictions/precautions seem to be put in place to prevent the user from accidentally shooting themselves in the foot rather than preventing them from abusing the system.

Having said that, we are currently faced with user requirements where users need to override some of these functions. We can try to push back and avoid allowing these overrides (don't see any reason for this), or we can try to be selective and only allow overrides at certain points (makes even less sense). I am in favor of going ahead with the initial change where user-defined functions are sourced in at the bottom, allowing them to override thses function definitions.

I would like a quick input/go-ahead from any one of @danmcp @brenton @jwhonce

[jwhonce]

@abhgupta Moving sourcing of .bash_profile to that low in the file would allow users to override their gear's environment variables and the shell TMOUT. While the TMOUT can be fixed by moving the source a couple of lines, allowing the user full rein on overriding all gear environment variables is asking for trouble. Currently the user can set any environment variable they want in their bash_profile and the system promptly resets the value to what is expected.

👎

[abhgupta]

Had a discussion with @jwhonce on irc and his take on this is:

my concern with this implementation is you will fail security audits with other customers because the system is no longer setting up the environment the user is

[sferich888]

@jwhonce I agree and why I think it's better to allow for some functions have a variable that allows you to source in an alternative operation.

Similar to what I suggested above, in the comments.

[abhgupta]

@sferich888 The problem still exists as you cannot guarantee that the user is actually providing the alternative implementation of the function being overridden and not something entirely different.

We could potentially place the 3 (or so) functions mongo/psql/mysql/anything-else towards the top, source the user's file after that, and then continue on with setting the environment variables and other functions that we don't want the user to be able to override easily.

[sferich888]

@abhgupta I agree, ill take some time to move the functions and important aground to accommodate this.

[sferich888]

@abhgupta @tiwillia @jwhonce I have moved the functions and the source line around however even after doing this (as suggested) I still worry that we are still allowing for "safety measure" to be overritten I was not sure if it was saft to move these two lines below the source of env's or not? (my guess is its not).

[tiwillia]

With this change the cartridge sdk is sourced, the PATH variables are set, and environment variables are loaded all before the .bash_profile is sourced. These can all be overwritten by the .bash_profile. Wouldn't it be best to leave the source of the .bash_profile at the top of the file and only move the three db functions above it?

[sferich888]

@tiwillia this will not work because environment variables are based / sourced using conditional wrapping the lines of code I highlighted. These are used in the functions that I moved up in the script.

IMO it looks like there is no good way to solve this with out re-designing what / why these functions are in the places they are.

[jwhonce]

@sferich888 FYI -- these functions and the whole login pipeline was brought along from v1. Since we had v1 and v2 gears running on the same node, we chose not to break that contract.

[tiwillia]

@sferich888 It shouldn't matter if the $OPENSHIFT_* variables are defined before or after the mysql, mongo, and psql functions are defined. The functions only use the variables when they are called, not defined. They should not be called until after the environment variables are set. There should be no problem with leaving rhcsh as it was and only moving the three database functions above where we source the .bash_profile

[sferich888]

@a13m can you chime in on this and tell us if this makes since for Online? I think what we have now may work for Enterprise but, I want to look at something that works for both places. (Note sure if that is possible).

[tiwillia]

Lets go ahead and [test] while @a13m reviews this in the online context.

[openshift-bot]

Evaluated for online test up to 2d79f5a

[openshift-bot]

Online Test Results: SUCCESS (https://ci.dev.openshift.redhat.com/jenkins/job/test_pull_requests/9153/)

[tiwillia]

@a13m has given us the go-ahead.

[merge] please.

[openshift-bot]

Evaluated for online merge up to 2d79f5a

[openshift-bot]

Online Merge Results: SUCCESS (https://ci.dev.openshift.redhat.com/jenkins/job/test_pull_requests/9153/) (Image: devenv_5827)

[tiwillia]

Merge failure occurred due to a known issue caused by an update to the fakefs gem. This PR will be re-merged once that issue has been resolved.

[thrasher-redhat]

@tiwillia

Openshift-bot may have gone rogue...or that was one crazy jenkins job that tooks 8 months. We may want to make sure this change is still desired. If it is, we'll have to make sure we track it for 2.2.11 and any online releases.

[danmcp]

@thrasher-redhat @tiwillia It merged because it had a passing test and a valid request to merge. We can back it out if needed though.

[abhgupta]

@danmcp I had raised this in order to understand why/how this PR was just merged after 8 months - and to make sure that we account for this change and get QE's attention on it for upcoming releases. However, it just occurred to me that perhaps the bot was run with the "quick merge" (I forget the parameter/flag name) option specified, allowing this PR to be merged. Is that right?

[danmcp]

@abhgupta That is correct.

[tiwillia]

This change was originally submitted to resolve https://bugzilla.redhat.com/show_bug.cgi?id=1258033