From b45bd462ebfd5d926d3ed66c8fec8003d1a25161 Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Thu, 28 Sep 2017 12:26:28 +0300 Subject: [PATCH 1/3] EnvInjectBuilder now always consults with other environment contributors when generating the environment --- .../plugins/envinject/EnvInjectBuilder.java | 11 +++- .../envinject/EnvInjectPluginAction.java | 3 +- .../plugins/envinject/util/RunHelper.java | 56 +++++++++++++++++++ 3 files changed, 68 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectBuilder.java b/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectBuilder.java index f81c2d0d..f25ad594 100644 --- a/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectBuilder.java +++ b/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectBuilder.java @@ -1,15 +1,18 @@ package org.jenkinsci.plugins.envinject; +import hudson.EnvVars; import hudson.Extension; import hudson.FilePath; import hudson.Launcher; import hudson.model.AbstractBuild; import hudson.model.AbstractProject; import hudson.model.BuildListener; +import hudson.model.EnvironmentContributor; import hudson.model.Result; import hudson.scm.SCM; import hudson.tasks.BuildStepDescriptor; import hudson.tasks.Builder; +import hudson.util.LogTaskListener; import org.jenkinsci.lib.envinject.EnvInjectLogger; import org.jenkinsci.plugins.envinject.service.EnvInjectActionSetter; import org.jenkinsci.plugins.envinject.service.EnvInjectEnvVars; @@ -19,6 +22,7 @@ import java.io.Serializable; import java.util.HashMap; import java.util.Map; +import java.util.logging.Level; import javax.annotation.Nonnull; import org.jenkinsci.plugins.envinject.util.RunHelper; @@ -80,7 +84,12 @@ public boolean perform(@Nonnull AbstractBuild build, @Nonnull Launcher lau // Prop file variables will be merged with other ones final Map propertiesEnvVars = envInjectEnvVarsService.getEnvVarsFileProperty(ws, logger, info.getPropertiesFilePath(), info.getPropertiesContentMap(previousEnvVars), variables); resultVariables = envInjectEnvVarsService.getMergedVariables(variables, propertiesEnvVars); - } + } + + // Whatever we inject, EnvironmentContributors should be able to override that (JENKINS-26583) + EnvVars contributedEnvVars = new EnvVars(); + RunHelper.consultOtherEnvironmentContributors(build, contributedEnvVars, listener); + resultVariables.putAll(contributedEnvVars); build.addAction(new EnvInjectBuilderContributionAction(resultVariables)); diff --git a/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectPluginAction.java b/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectPluginAction.java index 8e8f5f72..1b3979f2 100644 --- a/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectPluginAction.java +++ b/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectPluginAction.java @@ -96,7 +96,8 @@ public synchronized void buildEnvVars(@Nonnull AbstractBuild build, @Nonnu // Other extension points may contribute other variable values // before contributing actions is invoked. See AbstractBuild#getEnvironment() - // We take the externally updated variables as a source of truth and just override the missing ones + // We take the externally updated variables as a source of truth and just override the missing ones. + // Otherwise it causes JENKINS-26583 Map overrides = null; for (Map.Entry storedVar : currentEnvMap.entrySet()) { final String varName = storedVar.getKey(); diff --git a/src/main/java/org/jenkinsci/plugins/envinject/util/RunHelper.java b/src/main/java/org/jenkinsci/plugins/envinject/util/RunHelper.java index 702007ac..352cfc61 100644 --- a/src/main/java/org/jenkinsci/plugins/envinject/util/RunHelper.java +++ b/src/main/java/org/jenkinsci/plugins/envinject/util/RunHelper.java @@ -3,6 +3,7 @@ import hudson.EnvVars; import hudson.matrix.MatrixRun; import hudson.model.AbstractBuild; +import hudson.model.BuildListener; import hudson.model.Environment; import hudson.model.EnvironmentContributor; import hudson.model.Executor; @@ -27,6 +28,7 @@ import jenkins.model.Jenkins; import org.jenkinsci.lib.envinject.EnvInjectException; import org.jenkinsci.lib.envinject.EnvInjectLogger; +import org.jenkinsci.plugins.envinject.EnvInjectEnvVarsContributor; import org.jenkinsci.plugins.envinject.EnvInjectJobProperty; import org.jenkinsci.plugins.envinject.EnvInjectJobPropertyInfo; import org.jenkinsci.plugins.envinject.EnvInjectPluginAction; @@ -114,6 +116,40 @@ public static void getJDKVariables(@Nonnull Run run, TaskListener logger, } } } + + /** + * Consults with all Environment Contributors and sends their results + * to the destination {@link EnvVars} entity. + * {@link EnvInjectEnvVarsContributor} will be ignored. + * + * @param run Run + * @param envVars Target environment variables. + * The argument may contain some environment variables before the call, + * but it may be also empty. + * @param listener Listener and logger + * @throws InterruptedException Operation has been interrupted + * @throws IOException Operation error. + * Unhandled exceptions in {@link EnvironmentContributor} will be wrapped by this exception as well. + */ + public static void consultOtherEnvironmentContributors(@Nonnull Run run, @Nonnull EnvVars envVars, + @Nonnull BuildListener listener) + throws InterruptedException, IOException { + for (EnvironmentContributor ec : EnvironmentContributor.all()) { + if (ec instanceof EnvInjectEnvVarsContributor) { + // We skip EnvInject plugin, it should be invoked elsewhere + continue; + } + + try { + ec.buildEnvironmentFor(run, envVars, listener); + } catch (IOException | InterruptedException ex) { + // We just propagate the exception + throw ex; + } catch (Exception ex) { + throw new IOException("Unexpected exception in the EnvironmentContributor", ex); + } + } + } // Moved from EnvInjectVariableGetter @@ -125,6 +161,7 @@ public static Map getBuildVariables(@Nonnull Run run, @Non result.putAll(run.getCharacteristicEnvVars()); try { + // TODO: rework to consultEnvironmentContributors(), why result is within the cycle? EnvVars envVars = new EnvVars(); for (EnvironmentContributor ec : EnvironmentContributor.all()) { ec.buildEnvironmentFor(run, envVars, new LogTaskListener(LOGGER, Level.ALL)); @@ -184,6 +221,25 @@ public static EnvInjectJobProperty getEnvInjectJobProperty(@Nonnull Run bu return null; } + // Oleg: I just described the current behavior. It does not mean I understand why all of that + // is required for "get..PreviousSteps" + /** + * Gets Environment variables contributed by previous steps. + * + * The method consults with {@link Environment}s for {@link AbstractBuild}s, + * currently injected variables from {@link EnvInjectPluginAction}. + * If the {@link EnvInjectPluginAction} is missing (new build), it goes through + * system env vars, build characteristic variables and {@link EnvironmentContributor}s + * to construct the list. + * For {@link MatrixRun}s it also adds their env vars. + * + * @param build Current run + * @param logger Events logger + * @return Map of resolved environment variables + * @throws IOException Filesystem operation error + * @throws InterruptedException Interrupted (operation may do remoting calls) + * @throws EnvInjectException Failed to inject variables + */ @Nonnull public static Map getEnvVarsPreviousSteps( @Nonnull Run build, @Nonnull EnvInjectLogger logger) From d634fb1f18c87dc0898ce361a51797a61ed388ee Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Thu, 28 Sep 2017 12:52:18 +0300 Subject: [PATCH 2/3] [JENKINS-26583] - Make EnvInjectEnvVarsContributor a third-class citizen --- .../plugins/envinject/EnvInjectEnvVarsContributor.java | 10 +++++++++- .../jenkinsci/plugins/envinject/util/RunHelper.java | 4 ++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectEnvVarsContributor.java b/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectEnvVarsContributor.java index 2c454e72..eef03682 100644 --- a/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectEnvVarsContributor.java +++ b/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectEnvVarsContributor.java @@ -8,9 +8,17 @@ import java.util.Map; /** + * Contributes environment values to the environment. + * + * This extension has the low ordinal, and hence + * it will be processed first in {@link EnvironmentContributor#all()} reversed iterators. + * {@link jenkins.model.CoreEnvironmentContributor} has {@code -100}. + * Reverse ones are used in {@link Job#getEnvironment(Node, TaskListener)}... + * It means that other contributors will be always able to override values contributed by EnvInject + * * @since 1.92 */ -@Extension +@Extension(ordinal = -99) public class EnvInjectEnvVarsContributor extends EnvironmentContributor { @Override diff --git a/src/main/java/org/jenkinsci/plugins/envinject/util/RunHelper.java b/src/main/java/org/jenkinsci/plugins/envinject/util/RunHelper.java index 352cfc61..8abbc273 100644 --- a/src/main/java/org/jenkinsci/plugins/envinject/util/RunHelper.java +++ b/src/main/java/org/jenkinsci/plugins/envinject/util/RunHelper.java @@ -134,7 +134,7 @@ public static void getJDKVariables(@Nonnull Run run, TaskListener logger, public static void consultOtherEnvironmentContributors(@Nonnull Run run, @Nonnull EnvVars envVars, @Nonnull BuildListener listener) throws InterruptedException, IOException { - for (EnvironmentContributor ec : EnvironmentContributor.all()) { + for (EnvironmentContributor ec : EnvironmentContributor.all().reverseView()) { if (ec instanceof EnvInjectEnvVarsContributor) { // We skip EnvInject plugin, it should be invoked elsewhere continue; @@ -163,7 +163,7 @@ public static Map getBuildVariables(@Nonnull Run run, @Non try { // TODO: rework to consultEnvironmentContributors(), why result is within the cycle? EnvVars envVars = new EnvVars(); - for (EnvironmentContributor ec : EnvironmentContributor.all()) { + for (EnvironmentContributor ec : EnvironmentContributor.all().reverseView()) { ec.buildEnvironmentFor(run, envVars, new LogTaskListener(LOGGER, Level.ALL)); result.putAll(envVars); } From 04b6f58410addcb0d436bd1227725eb385be44e4 Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Thu, 28 Sep 2017 13:31:43 +0300 Subject: [PATCH 3/3] [JENKINS-26583] - EnvInjectListener does not longer override predefined variables --- .../plugins/envinject/EnvInjectListener.java | 14 ++--- .../util/NonOverridingEnvironment.java | 58 +++++++++++++++++++ 2 files changed, 63 insertions(+), 9 deletions(-) create mode 100644 src/main/java/org/jenkinsci/plugins/envinject/util/NonOverridingEnvironment.java diff --git a/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectListener.java b/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectListener.java index 502c154f..541db744 100644 --- a/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectListener.java +++ b/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectListener.java @@ -23,6 +23,8 @@ import java.util.Map; import javax.annotation.CheckForNull; import javax.annotation.Nonnull; + +import org.jenkinsci.plugins.envinject.util.NonOverridingEnvironment; import org.jenkinsci.plugins.envinject.util.RunHelper; /** @@ -138,7 +140,7 @@ public void beforeUse(AbstractBuild build, FilePath ws, BuildListener listener) } } - private Environment setUpEnvironmentJobPropertyObject(@Nonnull Run build, + private Environment setUpEnvironmentJobPropertyObject(final @Nonnull Run build, @Nonnull Launcher launcher, @Nonnull BuildListener listener, @Nonnull EnvInjectLogger logger) throws IOException, InterruptedException, EnvInjectException { @@ -203,13 +205,12 @@ private Environment setUpEnvironmentJobPropertyObject(@Nonnull Run build, //Add an action to share injected environment variables new EnvInjectActionSetter(rootPath).addEnvVarsToRun(build, mergedVariables); - return new Environment() { @Override public void buildEnvVars(Map env) { envInjectEnvVarsService.resolveVars(mergedVariables, mergedVariables); //resolve variables each other //however, here preCheckout of EnvBuildWrapper is not yet performed - env.putAll(mergedVariables); + NonOverridingEnvironment.append(build, env, mergedVariables); } }; } else { @@ -239,12 +240,7 @@ private Environment setUpEnvironmentWithoutJobPropertyObject(@Nonnull AbstractBu new EnvInjectActionSetter(rootPath).addEnvVarsToRun(build, resultVariables); } - return new Environment() { - @Override - public void buildEnvVars(Map env) { - env.putAll(resultVariables); - } - }; + return new NonOverridingEnvironment(build, resultVariables); } @CheckForNull diff --git a/src/main/java/org/jenkinsci/plugins/envinject/util/NonOverridingEnvironment.java b/src/main/java/org/jenkinsci/plugins/envinject/util/NonOverridingEnvironment.java new file mode 100644 index 00000000..39851a36 --- /dev/null +++ b/src/main/java/org/jenkinsci/plugins/envinject/util/NonOverridingEnvironment.java @@ -0,0 +1,58 @@ +package org.jenkinsci.plugins.envinject.util; + +import hudson.model.Environment; +import hudson.model.Run; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; + +import javax.annotation.CheckForNull; +import javax.annotation.Nonnull; +import java.util.Map; +import java.util.logging.Level; +import java.util.logging.Logger; + +/** + * Environment, which does not override already injected variables. + * @author Oleg Nenashev + */ +@Restricted(NoExternalUse.class) +public class NonOverridingEnvironment extends Environment { + + //TODO: Consider using RunListener to report collisions, causes too much buildlog spam now + private static final Logger LOGGER = Logger.getLogger(NonOverridingEnvironment.class.getName()); + + private final Run run; + + @CheckForNull + private final Map vars; + + public NonOverridingEnvironment(@Nonnull Run run, @CheckForNull Map vars) { + this.vars = vars; + this.run = run; + } + + @Override + public void buildEnvVars(Map env) { + append(run, env, vars); + } + + public static void append(@Nonnull Run run, @Nonnull Map env, @CheckForNull Map toInject) { + if (toInject == null) { + return; // nothing to inject + } + + for (Map.Entry storedVar : toInject.entrySet()) { + final String varName = storedVar.getKey(); + final String storedValue = storedVar.getValue(); + final String envValue = env.get(storedVar.getKey()); + if (envValue == null) { + LOGGER.log(Level.CONFIG, "Run {0}: Variable {1} is missing, injecting {2}", + new Object[] {run, varName, storedValue}); + env.put(varName, storedValue); + } else if (LOGGER.isLoggable(Level.FINER) && !envValue.equals(storedValue)) { + LOGGER.log(Level.FINER, "Run {0}: Variable {1} is already defined, keeping {2} instead of {3}", + new Object[] {run, varName, storedValue, envValue}); + } + } + } +}