roles/lib_openshift: Handle /usr/local/bin/oc with sudo

[cgwalters]

The real changes here are in src/lib/{base,import}.py remember, the rest is
committed to git rather than built for some reason.

This is tested on CentOS AH (python2), I haven't yet tested the Python 3 path
here. I tried the suggestion in the PR for using Ansible's module but AFAICS
that would require passing down the module variable pretty far down into this
code. This implementation seems OK too.

Closes: #3410

[kwoodson]

@cgwalters, Thanks for taking a look at this.

I had a couple of thoughts regarding this. My first thought is that we should probably set the oc command's path during the constructor and do it once per instantiation. Regardless of which method we choose. It is not uncommon that we make multiple calls out to openshift_cmd during our execution path. I think a single call to setup the path might be cleaner and it being abstracted to its own function would make testing and mocking easier.

The other suggestion would be that the oc binary might not be in /usr/local/bin. It could be somewhere else as is in the case with oc cluster up. My suggestion would be to create a list of known PATH locations and pass them down into the environment:
self.common_paths = ['/usr/local/bin', os.expand_path('~/bin')]
curr_env.update({'PATH': '%s:%s' % (curr_env['PATH'], ':'.join(self.common_paths)})

code is untested

Another option is to add environment variables to ansible. It can take them at different levels. At the play level, task level, and the inventory file.

@abutcher, @twiest, @ashcrow, @tbielawa, @detiber
I'd appreciate it if a few others could voice their opinion.

[cgwalters]

Quick side note: Changing the current process' environment isn't threadsafe and will lead to very hard to debug crashes down the line. See rust-lang/rust#24741 for some links. While I'm not sure if Ansible uses threads today, it's really better to be safe here. Two Python projects that went through this pain recently are Anaconda and we hit it in the atomic command too.

[cgwalters]

Fixed an obvious logic error + some pylint issues ⬆️

[cgwalters]

As far as searching other paths...something like this? Any other dirs to add?

diff --git a/roles/lib_openshift/src/lib/base.py b/roles/lib_openshift/src/lib/base.py
index 0018a67..8733cbd 100644
--- a/roles/lib_openshift/src/lib/base.py
+++ b/roles/lib_openshift/src/lib/base.py
@@ -233,9 +233,18 @@ class OpenShiftCLI(object):
         else:
             import distutils.spawn
             basepath = distutils.spawn.find_executable(basecmd) # pylint: disable=no-name-in-module
-        if basepath is None and os.path.isfile('/usr/local/bin/' + basecmd):
-            basecmd = '/usr/local/bin/' + basecmd
-        cmds.append(basecmd)
+        if basepath is None:
+            for known_dir in ['/usr/local/bin/', os.path.expanduser('~/bin/')]:
+                path = known_dir + basecmd
+                if os.path.isfile(path):
+                    basepath = path
+                    break
+        if basepath is None:
+            # We didn't find it...maybe it is there somehow, but if not,
+            # we'll just let the execve below return an error.
+            cmds.append(basecmd)
+        else:
+            cmds.append(basepath)
 
         if self.all_namespaces:
             cmds.extend(['--all-namespaces'])

[detiber]

Not related to this PR, but @kwoodson is there any reason that we need to keep compat with the oadm binary?

[kwoodson]

I don't think so. It should be oc adm since the modules are going that direction.

[detiber]

@kwoodson @cgwalters I created #3416 that is the result of my working branch for testing some proposed changes to this PR. I pulled in #3414 to allow for better testing of the proposed changes as well.

[kwoodson]

@detiber, I like the abstracted oc commands into their own function. That will help testing and localization of how we build the oc command. Test integration will also help.

Should we remove the kubeconfig env variable passing to subrpocess? You mentioned modifying the path in multi-threaded applications can prove troublesome. I would be for adding the '--config %s' % self.kubeconfig to the oc code since they go hand-in-hand. @detiber, @cgwalters thoughts?

[detiber]

@kwoodson I actually wasn't aware of the env related issues that @cgwalters brought up. I do know that Ansible uses both multi-product and threading, so it is probably a good idea to convert existing env var use to command line options.

On a side note. Have you considered using a tool to wrap the subprocess management? sh provides a nice interface and helps with mocking for tests: https://amoffat.github.io/sh/. I think that would be post-3.5?

[ashcrow]

I like @cgwalters code here, but agree it would be nice to have it in a function that could be called by the modules.

FWIW ansible does use traditional threads as well as multiprocessing:

$ grep -ri 'import thread' | wc -l
3
$ grep -ri 'import multiprocess' | wc -l
3

@kwoodson / @detiber I'd be happy to take a stab at it combining @detiber's tests, @cgwalters code and meld it together into a reusable function. Looks like @detiber is already doing it 😄.

[detiber]

@ashcrow I'm working on updating my PR to @cgwalters branch now, since the test PR has been merged

[detiber]

@cgwalters @ashcrow @kwoodson if there are no objections, I would like to close this PR in favor of #3416

[ashcrow]

Since #3416 includes @cgwalters's changes and properly attributes him I have no objections.

[rhcarvalho]

Quick side note: Changing the current process' environment isn't threadsafe and will lead to very hard to debug crashes down the line.

@cgwalters @detiber

The above applies to changing the environment of the current process. For the sake of clarity, I understand in this PR the idea revolved around setting up the environment for spawning a new process. That has no problem, neither in Python nor Rust.

Example of copying the current process' env, modifying it and spawning a new process:

https://gist.github.com/rhcarvalho/bc319af47fb505cf1d396c99f5ea9cd1

Notice oc is not in my regular PATH.

[cgwalters]

Ah, I hadn't realized that subprocess would use the child's env for its own PATH lookups. That seems like a reasonable approach indeed.

[sdodson]

Superseded by #3416