Running oc with Proc to avoid command expansion

[vfreex]

This PR reimplements the mechanism to run oc command on a slave. It fixes https://bugzilla.redhat.com/show_bug.cgi?id=1625518

Why this is needed

We met issues when using pipeline syntax with Jenkins Client Plugin. When an option passed to a Selector contains spaces or special characters (like #, ;, etc), they are mistakenly explained by shell rather than passed to the oc process.

The current implementation

When using OpenShift Pipeline DSL, oc process is started with DurableTask,
which is designed to run a bash script or batch script on a slave.
This is not ideal because when the command arguments include special characters
(like whitespaces, #, !, ;), it's hard to escape them correctly.

The new implementation

The new implementation uses standard Jenkins API to start a oc process
on a slave. stdout and stderr outputs are piped to Jenkins master
so that polling is not needed.

[bparees]

/ok-to-test

[vfreex]

I believe the test failure is a problem of the test suite: To run oc rsh some-pod ps ax, the DSL should be written as

openshift.rsh("some-pod", "ps", "ax")

not

openshift.rsh("some-pod", "ps ax")

The latter should be interpreted as running an executable named ps ax without any arguments.

[bparees]

I believe the test failure is a problem of the test suite: To run oc rsh some-pod ps ax, the DSL should be written as

if the test was passing as it was, then people may have written pipelines expecting that behavior as well, so changing/breaking it is not an option.

[vfreex]

@bparees Right, that could break things.
But it seems to be that treating a string as a whole parameter without splitting at spaces is more reasonable, because oc rsh takes the arguments as executable argument1 argument2 ... rather than interpret them with a shell.

A practical use case is to pass environment variables with values containing spaces and special bash characters (like ;, #, $).
For example, suppose there's a Java application which accepts a 'JAVA_OPTS' environment variable to override jvm options, and we want to set it to '-Xms256m -Xmx2048m'.
Then we can run:
openshift.set('env', 'pod/mypod', 'JAVA_OPTS=-Xms256m -Xmx2048m').

[bparees]

But it seems to be that treating a string as a whole parameter without splitting at spaces is more reasonable, because oc rsh takes the arguments as executable argument1 argument2 ... rather than interpret them with a shell.

I don't disagree, but i also don't want to break existing users, so I think we need to think about the best way to introduce this change..we may have to introduce a flag so people are opting into the new behavior.

[vfreex]

@bparees Thanks for the explanation. I'll also think about the best way to make this change.

[vfreex]

@bparees Following are my thoughts to make the change compatible:

1. Introduce a switch on the configuration page. Users can make their choices on spiting arguments on spaces or not. By default, the option is set to split on spaces, but a warning message will show during build.
   2.Except that, we will not handle any other special characters. DurableTask will no longer be used to avoid potential shell injection.

What do you think?

[bparees]

What do you think?

it sounds ok to me, but let's wait for @gabemontero to come back on monday and see if he has any other ideas/thoughts that could avoid having to have a top level config field for this.

[vfreex]

@bparees Thanks. I'll adding an option to see if this change can pass all tests.

[gabemontero]

So on situation related to the second commit ... the precise failure may have provided some context and reminders of prior history for me (I can't find it now), but I suspect that the grouping of "ps aux" may have stemmed from groovy shortcomings and OpenShiftDSL method signature shortcomings that arose as a result.

For example, private Map buildCommonArgs(Object overb, List verbArgs, Object[] ouserArgsArray, Object... ooverrideArgs)

I vaguely recall groovy confusing the ouserArgsArray and ooverideArgs array.

Assuming my recollections are on the right path, I'd be curious if @vfreex could examine the arg array in https://github.com/openshift/jenkins-client-plugin/blob/master/src/main/resources/com/openshift/jenkins/plugins/OpenShiftDSL.groovy#L908-L914 and entries in the array along spaces, and create a new array with the extra entries, and get the "ps aux" scenario to pass.

If so, I'd say we could punt on the config opt in stuff.

If this is not the right track, then I'll need a repro of the failure with the relevant logs, etc.

Generally speaking, I really like @vfreex 's change here, and getting off Durable Task.

That said, my initial thought is not get this into 3.11, it is too late, and target this for 4.0, where it gets a full release worth of soaking with our upstream users. But we can discuss that in parallel as needed.

[vfreex]

@gabemontero Thanks for your comment. I examined the Groovy code and it seems to me the caller passes arguments to buildCommonArgs correctly, and the entries in ouserArgsArray are containing spaces as the user input.

Personally I don't expect the behavior of spiting arguments by spaces to become a long-term solution, otherwise it will be impossible to pass arguments including spaces unless introducing escape characters, which will increase the complexity. For example, to create a file named my test file.txt inside a pod, openshift.exec('my-pod', 'touch', my test file.txt') will create 3 files my test and file.txt and there is no way to escape.

[vfreex]

@gabemontero Thanks for digging out. I'll take a look soon.

[vfreex]

@gabemontero
Regarding to the error when verbose is enable, it seems to me the ProcStarter.readStderr() method doesn't work correctly as https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/Proc.java#L114 described.

What readStderr does is just setting reverseStderr to true, resulting in the err parameter passed to LocalProc is set to LocalProc.SELFPUMP_OUTPUT (
https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/Launcher.java#L934), otherwise err parameter will be set to null. But the construct of LocalProc will set the redirectError parameter of ProcessBuilder to true in both cases (https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/Proc.java#L219).

I'll file a bug report to Jenkins. At the same time, a workaround for this PR should be used. Options like:

1. providing a custom OutputStream via ProcStarter.stderr(errStream) to avoid using readStderr()
2. Directly use ProcessBuilder with launcher.getChannel().call().

[gabemontero]

sounds good on all points @vfreex - we'll await your next update

[vfreex]

@gabemontero Hi, I've updated the PR. The new change uses Proc.LocalProc to directly create a process on the node where filePath is located, and pipe the stdout and stderr to the Jenkins master.
Please take another look. Thank you!

[gabemontero]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabemontero, vfreex

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [gabemontero]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[gabemontero]

verified the verbose fix locally ... thanks again @vfreex

I'll initiate a new version shortly and update here and in the bugzilla

To remind on an earlier comment, given the depth of change, for now at least, we'll only pull into v4 of our openshift jenkins images. As time progresses and we get some traction with the upstream community, we can reassess updating older releases.

[gabemontero]

v1.0.17 of the client plugin as been initiated at the jenkins update center

[gabemontero]

Hey @vfreex ... fyi, turns out the switch from ProcStarter to Proc.LocalProc has broken the sidecar container scenario with the k8s plugin.

See https://bugzilla.redhat.com/show_bug.cgi?id=1657208

The oc command is getting executed in the jnlp default container using the alpine image, where of course it is not present.

The sidecar scenario works with the version of this plugin (the last being v1.0.16) that leveraged the jenkins Launcher to invoke oc.

As a shot in the dark, I did pick up your jenkinsci/kubernetes-plugin#377 by bumping the k8s plugin to 1.12.9, but it had no effect.

I've started considering the option 1) you noted in #176 (comment) as a means to both solve the verbose logging issue as well as re-engage the k8s plugin container decorator to get the command running in the right container.

If you have a sec and recall any issues if and when you tried that, please let me know.

thanks

[gabemontero]

my prototype seems to be working ... hope to have a PR up soon