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.

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

JENKINS-47376: Fix global env variables #245

Merged
merged 5 commits into from
Feb 3, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,9 @@ public class ContainerExecDecorator extends LauncherDecorator implements Seriali
private static final String CONTAINER_READY_TIMEOUT_SYSTEM_PROPERTY = ContainerExecDecorator.class.getName() + ".containerReadyTimeout";
private static final long CONTAINER_READY_TIMEOUT = containerReadyTimeout();
private static final String COOKIE_VAR = "JENKINS_SERVER_COOKIE";
private static final String JENKINS_HOME = "JENKINS_HOME=";
private static final String[] BUILT_IN_ENV_VARS = new String[] { "BUILD_NUMBER", "BUILD_ID", "BUILD_URL",
"NODE_NAME", "JOB_NAME", "JENKINS_URL", "BUILD_TAG", "GIT_COMMIT", "GIT_URL", "GIT_BRANCH" };

private static final Logger LOGGER = Logger.getLogger(ContainerExecDecorator.class.getName());

private transient KubernetesClient client;
Expand All @@ -85,13 +87,17 @@ public class ContainerExecDecorator extends LauncherDecorator implements Seriali
@SuppressFBWarnings(value = "SE_TRANSIENT_FIELD_NOT_RESTORED", justification = "not needed on deserialization")
private transient Map<Integer, ContainerExecProc> processes = new HashMap<Integer, ContainerExecProc>();

private final String podName;
private final String namespace;
private final String containerName;
private final EnvironmentExpander environmentExpander;
private String podName;
private String namespace;
private String containerName;
private EnvironmentExpander environmentExpander;
private EnvVars globalVars;
private FilePath ws;

private final FilePath ws;
public ContainerExecDecorator() {
}

@Deprecated
public ContainerExecDecorator(KubernetesClient client, String podName, String containerName, String namespace, EnvironmentExpander environmentExpander, FilePath ws) {
this.client = client;
this.podName = podName;
Expand Down Expand Up @@ -126,26 +132,99 @@ public ContainerExecDecorator(KubernetesClient client, String podName, String co
this(client, podName, containerName, (String) null, null, null);
}

public KubernetesClient getClient() {
return client;
}

public void setClient(KubernetesClient client) {
this.client = client;
}

public String getPodName() {
return podName;
}

public void setPodName(String podName) {
this.podName = podName;
}

public String getNamespace() {
return namespace;
}

public void setNamespace(String namespace) {
this.namespace = namespace;
}

public String getContainerName() {
return containerName;
}

public void setContainerName(String containerName) {
this.containerName = containerName;
}

public EnvironmentExpander getEnvironmentExpander() {
return environmentExpander;
}

public void setEnvironmentExpander(EnvironmentExpander environmentExpander) {
this.environmentExpander = environmentExpander;
}

public EnvVars getGlobalVars() {
return globalVars;
}

public void setGlobalVars(EnvVars globalVars) {
this.globalVars = globalVars;
}

public FilePath getWs() {
return ws;
}

public void setWs(FilePath ws) {
this.ws = ws;
}

@Override
public Launcher decorate(final Launcher launcher, final Node node) {
return new Launcher.DecoratedLauncher(launcher) {
@Override
public Proc launch(ProcStarter starter) throws IOException {
LOGGER.log(Level.FINEST, "Launch proc with environment: {0}", Arrays.toString(starter.envs()));
boolean quiet = starter.quiet();
FilePath pwd = starter.pwd();

String [] cmdEnvs = starter.envs();

//check if the cmd is sourced from Jenkins, rather than another plugin; if so, skip cmdEnvs as we are getting other environment variables
for (String cmd : cmdEnvs) {
if (cmd.startsWith(JENKINS_HOME)) {
cmdEnvs = new String[0];
LOGGER.info("Skipping injection of procstarter cmdenvs due to JENKINS_HOME present");
break;
List<String> procStarter = Arrays.asList(starter.envs());
List<String> cmdEnvs = new ArrayList<String>();
// One issue that cropped up was that when executing sh commands, we would get the jnlp agent's injected
// environment variables as well, causing obvious problems such as JAVA_HOME being overwritten. The
// unsatisfying answer was to check for the presence of JENKINS_HOME in the cmdenvs and skip if present.

// check if the cmd is sourced from Jenkins, rather than another plugin; if so, skip cmdEnvs except for
// built-in ones.
boolean javaHome_detected = false;
for (String env : procStarter) {
if (env.contains("JAVA_HOME")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what does it mean if JAVA_HOME is there?

and due to the break any variable after JAVA_HOME is ignored, so the order of vars matter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed that break...The JAVA_HOME comes from not being able to tell if the execProc was dispatched by another plugin, thus necessitating the entire provided environment variables to be injected or from the master, thus overwriting whatever local environment variables there were.

At least, that the best I can understand from the various ways Jenkins envVars are provided.

LOGGER.log(Level.FINEST, "Detected JAVA_HOME in {0}", env);
javaHome_detected = true;
}
for (String builtEnvVar : BUILT_IN_ENV_VARS) {
if (env.contains(builtEnvVar)) {
LOGGER.log(Level.FINEST, "Found built-in env var {0} in {1}",
new String[] { builtEnvVar, env });
cmdEnvs.add(env);
}
}
}
String [] commands = getCommands(starter);
return doLaunch(quiet, cmdEnvs, starter.stdout(), pwd, commands);
if (!javaHome_detected) {
cmdEnvs = procStarter;
}
String[] commands = getCommands(starter);
return doLaunch(quiet, cmdEnvs.toArray(new String[cmdEnvs.size()]), starter.stdout(), pwd, commands);
}

private Proc doLaunch(boolean quiet, String [] cmdEnvs, OutputStream outputForCaller, FilePath pwd, String... commands) throws IOException {
Expand Down Expand Up @@ -314,6 +393,10 @@ public void onClose(int i, String s) {
String.format("cd \"%s\"%s", pwd, NEWLINE).getBytes(StandardCharsets.UTF_8));

}
//get global vars here, run the export first as they'll get overwritten.
if (globalVars != null) {
this.setupEnvironmentVariable(globalVars, watch);
}

EnvVars envVars = new EnvVars();
if (environmentExpander != null) {
Expand All @@ -322,6 +405,7 @@ public void onClose(int i, String s) {

//setup specific command envs passed into cmd
if (cmdEnvs != null) {
LOGGER.log(Level.FINEST, "Launching with env vars: {0}", Arrays.toString(cmdEnvs));
for (String cmdEnv : cmdEnvs) {
envVars.addLine(cmdEnv);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,24 +1,30 @@
package org.csanchez.jenkins.plugins.kubernetes.pipeline;

import static org.csanchez.jenkins.plugins.kubernetes.pipeline.Resources.*;

import java.io.Closeable;
import java.util.List;
import java.util.logging.Level;
import java.util.logging.Logger;

import hudson.FilePath;
import org.jenkinsci.plugins.workflow.steps.AbstractStepExecutionImpl;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import javax.annotation.Nonnull;

import org.jenkinsci.plugins.workflow.steps.BodyExecutionCallback;
import org.jenkinsci.plugins.workflow.steps.BodyInvoker;
import org.jenkinsci.plugins.workflow.steps.EnvironmentExpander;
import org.jenkinsci.plugins.workflow.steps.StepContext;
import org.jenkinsci.plugins.workflow.steps.StepExecution;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.EnvVars;
import hudson.FilePath;
import hudson.LauncherDecorator;
import hudson.slaves.EnvironmentVariablesNodeProperty;
import hudson.slaves.NodeProperty;
import hudson.slaves.NodePropertyDescriptor;
import hudson.util.DescribableList;
import io.fabric8.kubernetes.client.KubernetesClient;
import org.jenkinsci.plugins.workflow.steps.StepExecution;

import javax.annotation.Nonnull;

import static org.csanchez.jenkins.plugins.kubernetes.pipeline.Resources.closeQuietly;
import jenkins.model.Jenkins;

public class ContainerStepExecution extends StepExecution {

Expand Down Expand Up @@ -60,7 +66,23 @@ public boolean start() throws Exception {
client = nodeContext.connectToCloud();

EnvironmentExpander env = getContext().get(EnvironmentExpander.class);
decorator = new ContainerExecDecorator(client, nodeContext.getPodName(), containerName, nodeContext.getNamespace(), env, getContext().get(FilePath.class));
EnvVars globalVars = null;
Jenkins instance = Jenkins.getInstance();
DescribableList<NodeProperty<?>, NodePropertyDescriptor> globalNodeProperties = instance
.getGlobalNodeProperties();
List<EnvironmentVariablesNodeProperty> envVarsNodePropertyList = globalNodeProperties
.getAll(EnvironmentVariablesNodeProperty.class);
if (envVarsNodePropertyList != null && envVarsNodePropertyList.size() != 0) {
globalVars = envVarsNodePropertyList.get(0).getEnvVars();
}
decorator = new ContainerExecDecorator();
decorator.setClient(client);
decorator.setPodName(nodeContext.getPodName());
decorator.setContainerName(containerName);
decorator.setNamespace(nodeContext.getNamespace());
decorator.setEnvironmentExpander(env);
decorator.setWs(getContext().get(FilePath.class));
decorator.setGlobalVars(globalVars);
getContext().newBodyInvoker()
.withContext(BodyInvoker
.mergeLauncherDecorators(getContext().get(LauncherDecorator.class), decorator))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@

import com.google.common.collect.ImmutableMap;

import hudson.EnvVars;
import hudson.slaves.EnvironmentVariablesNodeProperty;
import hudson.slaves.NodeProperty;
import hudson.slaves.NodePropertyDescriptor;
import hudson.util.DescribableList;
import io.fabric8.kubernetes.api.model.Secret;
import io.fabric8.kubernetes.api.model.SecretBuilder;
import io.fabric8.kubernetes.client.KubernetesClient;
Expand All @@ -61,6 +66,7 @@ public class AbstractKubernetesPipelineTest {
protected static final String SECRET_KEY = "password";
protected static final String CONTAINER_ENV_VAR_FROM_SECRET_VALUE = "container-pa55w0rd";
protected static final String POD_ENV_VAR_FROM_SECRET_VALUE = "pod-pa55w0rd";
protected static final String GLOBAL = "GLOBAL";

@ClassRule
public static BuildWatcher buildWatcher = new BuildWatcher();
Expand Down Expand Up @@ -96,6 +102,14 @@ public void configureCloud() throws Exception {
JenkinsLocationConfiguration.get().setUrl(nonLocalhostUrl.toString());

r.jenkins.clouds.add(cloud);

DescribableList<NodeProperty<?>, NodePropertyDescriptor> list = r.jenkins.getGlobalNodeProperties();
EnvironmentVariablesNodeProperty newEnvVarsNodeProperty = new hudson.slaves.EnvironmentVariablesNodeProperty();
list.add(newEnvVarsNodeProperty);
EnvVars envVars = newEnvVarsNodeProperty.getEnvVars();
envVars.put("GLOBAL", "GLOBAL");
envVars.put("JAVA_HOME_X", "java-home-x");
r.jenkins.save();
}

private PodTemplate buildBusyboxTemplate(String label) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ public void declarative() throws Exception {
r.assertBuildStatusSuccess(r.waitForCompletion(b));
r.assertLogContains("Apache Maven 3.3.9", b);
r.assertLogContains("INSIDE_CONTAINER_ENV_VAR = " + CONTAINER_ENV_VAR_VALUE + "\n", b);
r.assertLogContains("OUTSIDE_CONTAINER_ENV_VAR = " + CONTAINER_ENV_VAR_VALUE + "\n", b);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -109,12 +109,29 @@ public void runInPodWithExistingTemplate() throws Exception {

@Test
public void runWithEnvVariables() throws Exception {
WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p");
WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "runWithEnvVariables");
p.setDefinition(new CpsFlowDefinition(loadPipelineScript("runWithEnvVars.groovy"), true));
WorkflowRun b = p.scheduleBuild2(0).waitForStart();
assertNotNull(b);
r.assertBuildStatusSuccess(r.waitForCompletion(b));
assertEnvVars(r, b);
r.assertLogContains("OUTSIDE_CONTAINER_BUILD_NUMBER = 1\n", b);
r.assertLogContains("INSIDE_CONTAINER_BUILD_NUMBER = 1\n", b);
r.assertLogContains("OUTSIDE_CONTAINER_JOB_NAME = runWithEnvVariables\n", b);
r.assertLogContains("INSIDE_CONTAINER_JOB_NAME = runWithEnvVariables\n", b);

// check that we are getting the correct java home
r.assertLogContains("INSIDE_JAVA_HOME =\n", b);
r.assertLogContains("JNLP_JAVA_HOME = /usr/lib/jvm/java-1.8-openjdk\n", b);
r.assertLogContains("JAVA7_HOME = /usr/lib/jvm/java-1.7-openjdk/jre\n", b);
r.assertLogContains("JAVA8_HOME = /usr/lib/jvm/java-1.8-openjdk/jre\n", b);

// check that we are not filtering too much
r.assertLogContains("INSIDE_JAVA_HOME_X = java-home-x\n", b);
r.assertLogContains("OUTSIDE_JAVA_HOME_X = java-home-x\n", b);
r.assertLogContains("JNLP_JAVA_HOME_X = java-home-x\n", b);
r.assertLogContains("JAVA7_HOME_X = java-home-x\n", b);
r.assertLogContains("JAVA8_HOME_X = java-home-x\n", b);
}

@Test
Expand Down Expand Up @@ -151,12 +168,15 @@ private void assertEnvVars(JenkinsRuleNonLocalhost r2, WorkflowRun b) throws Exc
r.assertLogContains("INSIDE_CONTAINER_ENV_VAR_FROM_SECRET = " + CONTAINER_ENV_VAR_FROM_SECRET_VALUE + "\n", b);
r.assertLogContains("INSIDE_POD_ENV_VAR = " + POD_ENV_VAR_VALUE + "\n", b);
r.assertLogContains("INSIDE_POD_ENV_VAR_FROM_SECRET = " + POD_ENV_VAR_FROM_SECRET_VALUE + "\n", b);
r.assertLogContains("INSIDE_GLOBAL = " + GLOBAL + "\n", b);

r.assertLogContains("OUTSIDE_CONTAINER_ENV_VAR =\n", b);
r.assertLogContains("OUTSIDE_CONTAINER_ENV_VAR_LEGACY =\n", b);
r.assertLogContains("OUTSIDE_CONTAINER_ENV_VAR_FROM_SECRET =\n", b);
r.assertLogContains("OUTSIDE_POD_ENV_VAR = " + POD_ENV_VAR_VALUE + "\n", b);
r.assertLogContains("OUTSIDE_POD_ENV_VAR_FROM_SECRET = " + POD_ENV_VAR_FROM_SECRET_VALUE + "\n", b);
r.assertLogContains("OUTSIDE_GLOBAL = " + GLOBAL + "\n", b);

}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ pipeline {
stages {
stage('Run maven') {
steps {
sh 'set'
sh "echo OUTSIDE_CONTAINER_ENV_VAR = ${CONTAINER_ENV_VAR}"
container('maven') {
sh 'echo INSIDE_CONTAINER_ENV_VAR = ${CONTAINER_ENV_VAR}'
sh 'mvn -version'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ node ('busybox') {
echo OUTSIDE_CONTAINER_ENV_VAR_FROM_SECRET = \$CONTAINER_ENV_VAR_FROM_SECRET
echo OUTSIDE_POD_ENV_VAR = \$POD_ENV_VAR
echo OUTSIDE_POD_ENV_VAR_FROM_SECRET = \$POD_ENV_VAR_FROM_SECRET
echo OUTSIDE_GLOBAL = \$GLOBAL
"""

stage('Run busybox') {
Expand All @@ -18,6 +19,7 @@ node ('busybox') {
echo INSIDE_CONTAINER_ENV_VAR_FROM_SECRET = \$CONTAINER_ENV_VAR_FROM_SECRET
echo INSIDE_POD_ENV_VAR = \$POD_ENV_VAR
echo INSIDE_POD_ENV_VAR_FROM_SECRET = \$POD_ENV_VAR_FROM_SECRET
echo INSIDE_GLOBAL = \$GLOBAL
"""
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,25 +11,55 @@ podTemplate(label: 'mypod',
secretEnvVar(key: 'CONTAINER_ENV_VAR_FROM_SECRET', secretName: 'container-secret', secretKey: 'password')
],
),
containerTemplate(name: 'java7', image: 'openjdk:7u151-jre-alpine', ttyEnabled: true, command: '/bin/cat'),
containerTemplate(name: 'java8', image: 'openjdk:8u151-jre-alpine', ttyEnabled: true, command: '/bin/cat')
]) {

node ('mypod') {

sh """
echo OUTSIDE_CONTAINER_BUILD_NUMBER = \$BUILD_NUMBER
echo OUTSIDE_CONTAINER_JOB_NAME = \$JOB_NAME
echo OUTSIDE_CONTAINER_ENV_VAR = \$CONTAINER_ENV_VAR
echo OUTSIDE_CONTAINER_ENV_VAR_LEGACY = \$CONTAINER_ENV_VAR_LEGACY
echo OUTSIDE_CONTAINER_ENV_VAR_FROM_SECRET = \$CONTAINER_ENV_VAR_FROM_SECRET
echo OUTSIDE_POD_ENV_VAR = \$POD_ENV_VAR
echo OUTSIDE_POD_ENV_VAR_FROM_SECRET = \$POD_ENV_VAR_FROM_SECRET
echo OUTSIDE_JAVA_HOME_X = \$JAVA_HOME_X
echo OUTSIDE_GLOBAL = \$GLOBAL
"""
stage('Run busybox') {
container('busybox') {
sh 'echo inside container'
sh """
echo INSIDE_CONTAINER_BUILD_NUMBER = \$BUILD_NUMBER
echo INSIDE_CONTAINER_JOB_NAME = \$JOB_NAME
echo INSIDE_CONTAINER_ENV_VAR = \$CONTAINER_ENV_VAR
echo INSIDE_CONTAINER_ENV_VAR_LEGACY = \$CONTAINER_ENV_VAR_LEGACY
echo INSIDE_CONTAINER_ENV_VAR_FROM_SECRET = \$CONTAINER_ENV_VAR_FROM_SECRET
echo INSIDE_POD_ENV_VAR = \$POD_ENV_VAR
echo INSIDE_POD_ENV_VAR_FROM_SECRET = \$POD_ENV_VAR_FROM_SECRET
echo INSIDE_JAVA_HOME_X = \$JAVA_HOME_X
echo INSIDE_JAVA_HOME = \$JAVA_HOME
echo INSIDE_GLOBAL = \$GLOBAL
"""
}
container('jnlp') {
sh """
echo JNLP_JAVA_HOME = \$JAVA_HOME
echo JNLP_JAVA_HOME_X = \$JAVA_HOME_X
"""
}
container('java7') {
sh """
echo JAVA7_HOME = \$JAVA_HOME
echo JAVA7_HOME_X = \$JAVA_HOME_X
"""
}
container('java8') {
sh """
echo JAVA8_HOME = \$JAVA_HOME
echo JAVA8_HOME_X = \$JAVA_HOME_X
"""
}
}
Expand Down