Skip to content

JENKINS-61735 Get all environment variable #92

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.

Sign up for GitHub

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

Open
tgatinea wants to merge 6 commits into
base: master
Choose a base branch
from
+48 −16
Open

JENKINS-61735 Get all environment variable #92

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
21ba9f3
JENKINS-61735 Get all environment variable including NODE_SLAVE and S…
Mar 31, 2020
cfd5a0b
Take into account jakub-bochenski remarks
Apr 10, 2020
27028f4
Change constructor to not duplicate code
Apr 11, 2020
56056ae
Add test of STAGE_NAME and NODE_NAME in pipeline envVars
Apr 12, 2020
393b0ac
Explicitly set node to be master
Apr 12, 2020
454f85f
Set reference to issue above test
Apr 12, 2020
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
6 changes: 4 additions & 2 deletions src/main/java/jenkins/plugins/logstash/LogstashConsoleLogFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,16 @@
public class LogstashConsoleLogFilter extends ConsoleLogFilter implements Serializable
{

private hudson.EnvVars envVars;
Copy link

@jakub-bochenski jakub-bochenski Apr 8, 2020

Choose a reason for hiding this comment

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

please change to using imports

Copy link
Author

@tgatinea tgatinea Apr 10, 2020

Choose a reason for hiding this comment

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

OK

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

private transient Run<?, ?> run;
public LogstashConsoleLogFilter() {}

public LogstashConsoleLogFilter(Run<?, ?> run)
public LogstashConsoleLogFilter(Run<?, ?> run, hudson.EnvVars envVars)
Copy link

@jakub-bochenski jakub-bochenski Apr 8, 2020

Choose a reason for hiding this comment

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

I'm a bit confused. Who is going to initialize this value for the old-style builds? I only see a change for pipeline

Copy link

@mwinter69 mwinter69 Apr 8, 2020

Choose a reason for hiding this comment

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

It's set to null. But in that case some other checks need to be applied. See below

Copy link
Author

@tgatinea tgatinea Apr 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I planned to introduce this modif only for declarative pipeline as it is the STAGE_NAME that were missing in the environment variables previously.
For old-style builds, I guess that your are talking for instance about freestyle, the STAGE_NAME is not relevant. So I guess that the plugin should work as before.
For my usecase, I don't see a reason to make some evolution on old-style logs.

Copy link

@jakub-bochenski jakub-bochenski Apr 9, 2020

Choose a reason for hiding this comment

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

as @mwinter69 mentioned here #92 (comment) it will break the freestyle build use case

{
this.run = run;
this.envVars = envVars;
}
private static final long serialVersionUID = 1L;

Expand Down Expand Up @@ -62,7 +64,7 @@ public OutputStream decorateLogger(Run build, OutputStream logger) throws IOExce

LogstashWriter getLogStashWriter(Run<?, ?> build, OutputStream errorStream)
{
return new LogstashWriter(build, errorStream, null, build.getCharset());
return new LogstashWriter(build, errorStream, null, build.getCharset(), envVars);
}

private boolean isLogstashEnabled(Run<?, ?> build)
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/jenkins/plugins/logstash/LogstashNotifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ private boolean perform(Run<?, ?> run, TaskListener listener) {

// Method to encapsulate calls for unit-testing
LogstashWriter getLogStashWriter(Run<?, ?> run, OutputStream errorStream, TaskListener listener) {
return new LogstashWriter(run, errorStream, listener, run.getCharset());
return new LogstashWriter(run, errorStream, listener, run.getCharset(), null);
Copy link

@jakub-bochenski jakub-bochenski Apr 8, 2020

Choose a reason for hiding this comment

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

please define two constructors instead of passing null value

Copy link
Author

@tgatinea tgatinea Apr 10, 2020

Choose a reason for hiding this comment

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

OK

}

@Override
Expand Down
6 changes: 4 additions & 2 deletions src/main/java/jenkins/plugins/logstash/LogstashWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,10 @@ public class LogstashWriter {
private final LogstashIndexerDao dao;
private boolean connectionBroken;
private final Charset charset;
private final hudson.EnvVars envVars;

public LogstashWriter(Run<?, ?> run, OutputStream error, TaskListener listener, Charset charset) {
public LogstashWriter(Run<?, ?> run, OutputStream error, TaskListener listener, Charset charset, hudson.EnvVars envVars) {
this.envVars = envVars;
this.errorStream = error != null ? error : System.err;
this.build = run;
this.listener = listener;
Expand Down Expand Up @@ -154,7 +156,7 @@ BuildData getBuildData() {
if (build instanceof AbstractBuild) {
return new BuildData((AbstractBuild<?, ?>) build, new Date(), listener);
} else {
return new BuildData(build, new Date(), listener);
return new BuildData(build, new Date(), listener, envVars);
}
}

Expand Down
11 changes: 2 additions & 9 deletions src/main/java/jenkins/plugins/logstash/persistence/BuildData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,21 +212,14 @@ public BuildData(AbstractBuild<?, ?> build, Date currentTime, TaskListener liste
}

// Pipeline project build
public BuildData(Run<?, ?> build, Date currentTime, TaskListener listener) {
public BuildData(Run<?, ?> build, Date currentTime, TaskListener listener, hudson.EnvVars envVars) {
initData(build, currentTime);

buildVariables = envVars;
rootProjectName = projectName;
rootFullProjectName = fullProjectName;
rootProjectDisplayName = displayName;
rootBuildNum = buildNum;

try {
// TODO: sensitive variables are not filtered, c.f. https://stackoverflow.com/questions/30916085
buildVariables = build.getEnvironment(listener);
} catch (IOException | InterruptedException e) {
LOGGER.log(WARNING,"Unable to get environment for " + build.getDisplayName(),e);
buildVariables = new HashMap<>();
}
Copy link

@mwinter69 mwinter69 Apr 8, 2020

Choose a reason for hiding this comment

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

It is wrong to remove this block. For non pipeline builds this would result in a loss of information. So in case the buildVariables is null it should be filled as done before.

Copy link
Author

@tgatinea tgatinea Apr 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't catch. I thought that this part of code (line 215. public BuildData(Run build, Date currentTime, TaskListener listener, hudson.EnvVars envVars) {) was on "ly for declarative pipeline and not for non pipeline builds.
But maybe the comment line 214 "// Pipeline project build" is not correct.
That's why I've considered that "envars" could not be null and that it was not necessary to do the build.getEnvironment(listener)
Note that my concern is to get more info than build.getEnvironment(listener) because with this command, you will not recover the environment variables of the stage itself and you will not get the STAGE_NAME and NODE_NAME that are to my mind very important variables that needs to be pushed into a Graylog. Otherwise you cannot make any correlation between info that might be recovered of one line of log and the stage and node that was at the origin of this line of log.

Copy link

@jakub-bochenski jakub-bochenski Apr 9, 2020

Choose a reason for hiding this comment

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

I think best way to resolve this would be to add some automated tests to show it's working

Copy link
Author

@tgatinea tgatinea Apr 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm missing a test that ensures that the information you want is included.
Another topic. What envvars are injected by the context? Do they contain sensitive information maybe like passwords?

Concerning sensitive info, you are probably right, but that need to be checked.
I've no idea how to distinguish sensitive information in a set of environment variables.

Copy link

@jakub-bochenski jakub-bochenski Apr 9, 2020

Choose a reason for hiding this comment

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

Filtering for sensitive variables is not supported in Pipeline. At least it wasn't last time I checked.
I think it's good enough if we make sure the sensitive variables for old-style builds are filtered

Copy link

@mwinter69 mwinter69 Apr 10, 2020

Choose a reason for hiding this comment

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

This constructor is indeed not called for instances of AbstractBuild. But it is not limited to WorkflowRun. It will also be called for any other implementation directly inheriting from Run (e.g. AsyncRun, ExternalRun, see https://jenkins.io/doc/developer/extensions/jenkins-core/#run)
So I would prefer to keep this code.
(Though any of these job types would never have been working together with logstash plugin before the pipeline support was added)

Copy link
Author

@tgatinea tgatinea Apr 10, 2020

Choose a reason for hiding this comment

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

OK. I suggest to keep this code but to execute it only if enVars is non null or empty if I figure out to create a new contructor (I guess it's related with your comment src/main/java/jenkins/plugins/logstash/LogstashNotifier.java line 110).

}

private void initData(Run<?, ?> build, Date currentTime) {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/jenkins/plugins/logstash/pipeline/LogstashSendStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ protected Void run() throws Exception
Run<?, ?> run = getContext().get(Run.class);
TaskListener listener = getContext().get(TaskListener.class);
PrintStream errorStream = listener.getLogger();
LogstashWriter logstash = new LogstashWriter(run, errorStream, listener, run.getCharset());
LogstashWriter logstash = new LogstashWriter(run, errorStream, listener, run.getCharset(), null);
logstash.writeBuildLog(maxLines);
if (failBuild && logstash.isConnectionBroken())
{
Expand Down
4 changes: 3 additions & 1 deletion src/main/java/jenkins/plugins/logstash/pipeline/LogstashStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ public boolean start() throws Exception {
context
.newBodyInvoker()
.withContext(createConsoleLogFilter(context))
.withContext(context.get(hudson.EnvVars.class))
.withCallback(BodyExecutionCallback.wrap(context))
.start();
return false;
Expand All @@ -68,7 +69,8 @@ private ConsoleLogFilter createConsoleLogFilter(StepContext context)
throws IOException, InterruptedException {
ConsoleLogFilter original = context.get(ConsoleLogFilter.class);
Run<?, ?> build = context.get(Run.class);
ConsoleLogFilter subsequent = new LogstashConsoleLogFilter(build);
hudson.EnvVars envVars = context.get(hudson.EnvVars.class);
Copy link

@mwinter69 mwinter69 Apr 8, 2020

Choose a reason for hiding this comment

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

Not sure but have you checked if this will return something? Maybe envVars is something that is always injected, if not has to be aded as required context after line 110.

ConsoleLogFilter subsequent = new LogstashConsoleLogFilter(build, envVars);
return BodyInvoker.mergeConsoleLogFilters(original, subsequent);
}

Expand Down
2 changes: 1 addition & 1 deletion src/test/java/jenkins/plugins/logstash/LogstashWriterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ static LogstashWriter createLogstashWriter(final AbstractBuild<?, ?> testBuild,
final String url,
final LogstashIndexerDao indexer,
final BuildData data) {
return new LogstashWriter(testBuild, error, null, testBuild.getCharset()) {
return new LogstashWriter(testBuild, error, null, testBuild.getCharset(), null) {
@Override
LogstashIndexerDao getIndexerDao() {
return indexer;
Expand Down