[JENKINS-26583] - Attempt to tweak the injection priorities

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<String, String> 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));
 

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

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<String, String> 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<String, String> env) {
-                env.putAll(resultVariables);
-            }
-        };
+        return new NonOverridingEnvironment(build, resultVariables);
     }
 
     @CheckForNull

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<String, String> overrides = null;
         for (Map.Entry<String, String> storedVar : currentEnvMap.entrySet()) {
             final String varName = storedVar.getKey();

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<String, String> vars;
+
+    public NonOverridingEnvironment(@Nonnull Run<?, ?> run, @CheckForNull Map<String, String> vars) {
+        this.vars = vars;
+        this.run = run;
+    }
+
+    @Override
+    public void buildEnvVars(Map<String, String> env) {
+        append(run, env, vars);
+    }
+
+    public static void append(@Nonnull Run<?, ?> run, @Nonnull Map<String, String> env, @CheckForNull Map<String, String> toInject) {
+        if (toInject == null) {
+            return; // nothing to inject
+        }
+
+        for (Map.Entry<String, String> 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});
+            }
+        }
+    }
+}

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().reverseView()) {
+            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,8 +161,9 @@ public static Map<String, String> 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()) {
+            for (EnvironmentContributor ec : EnvironmentContributor.all().reverseView()) {
                 ec.buildEnvironmentFor(run, envVars, new LogTaskListener(LOGGER, Level.ALL));
                 result.putAll(envVars);
             }
@@ -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<String, String> getEnvVarsPreviousSteps(
             @Nonnull Run<?, ?> build, @Nonnull EnvInjectLogger logger)