Use mime type from config field in Elasticsearch indexer while posting HTTP request #41

VamanKulkarni · 2017-12-16T10:26:59Z

## Problem:
Due to this bug in Logstash itself, build logs which are sent to Logstash over HTTP(logstash-input-http) using Elasticsearch indexer gets indexed as plain text instead of JSON.

This plugin uses ContentType.APPLICATION_JSON which sets the Content-Type header to "application/json; charset=UTF-8" and that is not recognized by Logstash and treats the json string as plain text.

## What has been done in this PR
Introduced (as per @jakub-bochenski suggestion) a new configuration field for Mime type which gives ability for users to override Mime type. It defaults to application/json. This mime type is then set as request header inElasticSeachDAO

jakub-bochenski

Please add at least one unit test covering this code, other than that looks good on first look

VamanKulkarni · 2017-12-19T18:04:49Z

Thanks @jakub-bochenski .
I came across a problem while trying to write unit test.
My commit introduces
Descriptor logstashPluginConfig = (Descriptor) Jenkins.getInstance().getDescriptor(LogstashInstallation.class);
to the ElasticSearchDao.getHttpPost() method.
Mocking Jenkins.getInstance() is not possible with mockito. This page suggests to use PowerMock for mocking static methods. However, powermock has issues with mockito > 2.0. (powermock/powermock#681).

You may be already aware of this problem. Do you have a way out?

jakub-bochenski · 2017-12-19T18:36:11Z

Apparently mockito can do it: #37 (review)

On second thoughts maybe it would be better to add this setting to the publisher/wrapper (per job) instead of doing this globally for the whole jenkins instance

VamanKulkarni · 2017-12-20T15:37:36Z

@jakub-bochenski I am working on your suggestion to add mimeType setting to wrapper.
Need some inputs from you as I am pretty new to plugin development.
I declared instance variable mimeType in LogstashBuildWrapper class and able to populate it with the value specified from UI. However, I am yet to figure out how to pass on/access this variable in ElasticSearchDao so that I can set it as content type to the post request. Would it be possible for you to provide some pointers around this? Meanwhile, I will continue looking. Thanks!

jakub-bochenski · 2017-12-20T19:50:54Z

However, I am yet to figure out how to pass on/access this variable in ElasticSearchDao

In general we don't have a mechanism to pass backend-specific options from the job config to the dao layer.
For the record we don't have a facility to pass this from the global config either, hence e.g. you need to put the scheme and host into the host field for Elastisearch backend type.

I think for this we need to go through the LogstashWriter and extend IndexerDaoFactory.getInstance to accept backend-specific configuration settings.

I'd refactor LogstashWriter to not call IndexerDaoFactory.getInstance directly and instead accept a ready instance of the IndexerDao.

Given the scope of refactoring needed I'm OK with doing this in global config as you did originally

VamanKulkarni · 2017-12-21T16:23:18Z

Apparently mockito can do it: #37 (review)

@jakub-bochenski May be I am missing something bigtime but, I checked #37 and it is used to mock a final method. I didn't find any examples of MockMaker for mocking static methods. If you have any pointer to such example handy let me know. :)

If mocking static method is not too straight forward then, I am thinking to overload getHttpPost with getHttpPost(String data, String mimeType) and use this overloaded method for unit testing. Feel free to suggest.

jakub-bochenski · 2017-12-21T17:17:51Z

@VamanKulkarni my bad.
There is another PR from @mwinter #38 and it adds powermock. Maybe he can comment in detail here, but I assume it means there is a workable solution for static mocks.

Still, if we can avoid mocking static methods by doing some refactoring I'd prefer that (if the effort to refactor is not big).

VamanKulkarni · 2017-12-22T17:48:51Z

@jakub-bochenski I've refactored code to make it little more unit test'ble and avoided mocking static method within unit tests. Take a look at the changes when you get a chance and let me know if you have any comments.

jakub-bochenski · 2017-12-22T19:23:15Z

src/main/resources/jenkins/plugins/logstash/LogstashInstallation/global.jelly

@@ -22,6 +22,10 @@
        checkUrl="'${resURL}/descriptorByName/LogstashInstallation/checkString?value='+escape(this.value)" />
    </f:entry>
    <f:advanced>
+      <f:entry title="${%MIME type}" field="mimeType">
+      <f:textbox value="${descriptor.mimeType}" default="application/json"


I'm not sure if the existing installations will get null or the default value.
Can we set the default to match the previous value?

I am not sure if I understand that correctly. Even in case of null, Content-Type will be set to ContentType.APPLICATION_JSON. Default value is no different then the previous one it is just that it is set in different way :) i.e. the default value is set to application/json and the String entity is initialized with charset=UTF-8.

OK I get it now.
Can we make it more explicit in the description that the mimeType input doesn't accept any parameters (incl. charset)?

jakub-bochenski · 2017-12-22T19:25:22Z

src/main/java/jenkins/plugins/logstash/LogstashInstallation.java

+            return FormValidation.error(Messages.ValueIsRequired());
+        }
+      // Mime type validation as per RFC-4288.
+      if (!value.matches("^[\\w#&+_$.\\-\\^]+/[\\w#&+_$.\\-\\^]+$")) {


I think it's better to reuse https://hc.apache.org/httpcomponents-core-ga/httpcore/apidocs/org/apache/http/entity/ContentType.html#parse(java.lang.String)

I did think of this earlier. But I did not end up using it in my code because I realized it does not actually parse Mime-Type. for e.g. ContentType.parse("application/json%%") (which is clearly not a valid mime-type) does not complain instead it returns ContentType object with this invalid mimetype.
For this reason I did not use it in the code and ended up validating by my own as per RFC-4288. This would potentially eliminate unwanted surprises that could cause with invalid mime-type.

OK, I assumed

ParseException - if the given text does not represent a valid Content-Type value.

means it does validate it

It just validates Content-Type as a whole and does not look for mime-type validity.

You could use javax.activation.MimeType
This does validate the mimetype. Though it seems & is a valid character in a mimetype. Mimetype fails when you use §
see RFC 2045

jakub-bochenski · 2017-12-28T17:22:44Z

@VamanKulkarni can you check if it would be possible to integrate this changes into the #43 refactoring?

VamanKulkarni · 2017-12-29T15:08:21Z

@jakub-bochenski Okay. I shall take a look at integrating my changes with #43 . I am not sure about the process though. Do I take #43 changes on to my fork, then integrate and update this PR or do I update the PR #43 itself with my changes or is there any other way?

jakub-bochenski · 2017-12-29T15:41:14Z

@VamanKulkarni I'd suggest a new PR forking #43
It seems like a simple addition of a new property to https://github.com/jenkinsci/logstash-plugin/pull/43/files#diff-ebc0a0ba452d2cfc22ad4a3b6644508c and the related jelly.

VamanKulkarni · 2018-01-02T15:53:42Z

@jakub-bochenski : I shall wait until changes in PR #43 are finalized and merged just to avoid any possibility of rework that could arise out of reviewing #43. :)

jakub-bochenski · 2018-01-30T21:30:25Z

@VamanKulkarni #43 is now merged

VamanKulkarni · 2018-02-18T09:07:37Z

@jakub-bochenski I've updated PR with the changes as per new code refactoring. Please take a look.

jakub-bochenski · 2018-02-20T12:53:27Z

LGTM
@mwinter69 would you mind taking a look at those changes?

BTW sorry about not responding lately but I have some health issues. Hopefully it's nothing serious, still I won't be able to put in much work for now

mwinter69 · 2018-02-20T22:56:41Z

@jakub-bochenski get well soon

Does it make sense to allow any mimetype other than application/json?
Couldn't we just stick to making sure that ContentType is application/json and set the Charset to UTF-8?

StringEntity input = new StringEntity(data, StandardCharsets.UTF_8); input.setContentType(ContentType.APPLICATION_JSON.getMimeType());

VamanKulkarni · 2018-02-26T05:58:46Z

@jakub-bochenski Hope you are getting better :)
@mwinter69 My initial proposal was exactly the same. @jakub-bochenski and I had a discussion over this and decided to go with making it configurable. Please take a look at the comments section https://wiki.jenkins.io/display/JENKINS/Logstash+Plugin

jakub-bochenski · 2018-02-26T10:49:59Z

Thanks for your consideration :)

I admit this might violate YAGNI, however my 2 top arguments for adding this option are:

this change should be opt it, preserving the previous behavior by default,
if we need at least a on/off toggle because of 1. why not add an input text to make the solution more robust, it doesn't complicate the code much

mwinter69 · 2018-02-26T21:00:45Z

src/main/java/jenkins/plugins/logstash/persistence/ElasticSearchDao.java

+    String mimeType = this.getMimeType();
+    // char encoding is set to UTF_8 since this request posts a JSON string
+    StringEntity input = new StringEntity(data, StandardCharsets.UTF_8);
+    mimeType = (mimeType != null) ? mimeType : ContentType.APPLICATION_JSON.toString();


mimeType will never be null. If you save the global configuration with no content for the mimeType you will get an empty string. Only when you update the plugin without ever saving the configuration it will be null.

mwinter69 · 2018-02-26T21:04:12Z

src/main/java/jenkins/plugins/logstash/configuration/ElasticSearch.java

@@ -163,5 +178,17 @@ public FormValidation doCheckUrl(@QueryParameter("value") String value)
      }
      return FormValidation.ok();
    }
+    public FormValidation doCheckMimeType(@QueryParameter("value") String value) {
+      if (StringUtils.isBlank(value)) {


I think a blank mimeType should be valid. In that case it should be set to null in the DAO so you have the old behaviour. This should be mentioned in the help.
You can also save with a blank mimeType which would result in no mimeType being configured in the StringEntity. Not sure if this will break anything,

@mwinter69 IMHO, I think we should not allow user to save config with blank mimeType and it would not be of much help/value addition if we do not configure mimeType on StringEntity considering the nature of elasticsearch and logstash. I think we should set application/json by default which will make both ES and logstash happy.

Does this also fail when you try to save with an empty string? If not then you can run into problems at runtime as you will pass an empty string. Maybe use Util.fixEmptyAndTrim in the DAO to avoid this or do not allow to save with an illegal mimetype.

@mwinter69 Apologies for delays in response. Recovering from bad health. Correct me if I mis-understand understand your question. As per current logic, user is not allowed to save an empty string for mimeType. It doesn't displays an error if user tries to save empty/invalid mimeType.

Go to the configuration page and make the mimetype field empty. When you leave the field it will tell you that a value is required. Now click on apply. Congratulations you have set the mimetype to an empty string which will most likely fail.
You should throw an exception in the setter method if no valid mimetype is given.

Looking into this it seems that Stapler is swallowing the exception and only logging when you try to use a setter method,
By initializing the mimetype in the constructor, and throwing the exception there you can avoid that it gets saved.

Thanks for the inputs. I am hoping that I understood your suggestion correctly. Here is what I did. I initialized mimeType in ElasticSearchDao() constructor and if it is invalid, thrown an exception just like how its done for uri.toURL(); currently. However, I realized that ElasticSeachDao is not instantiated every time we click on "Save" button on jenkins configuration page. Hence throwing exception in side ElasticSearchDao() did not work for me meaning UI does not show any exception. It only throws exception when you run a job.

In an alternative approach and also you mentioned this in one of your earlier comments that we can allow blank mimeType. If the mimeType is blank we can set it to null in ElasticSearch and with current implementation if mimeType is null it will be set to default application/json value. Does this sound acceptable?

The dao is the wrong place to do the checks. You have to do it in the constructor of configuration/ElasticSearch. Alternatively one could extend the base class LogstashIndexer with a validate method and call the validate method in the LogstashConfiguration.configure before save()

@mwinter69 Pushed updated code.

@jakub-bochenski Updated code with your review comments

jakub-bochenski · 2018-04-03T17:31:51Z

src/main/java/jenkins/plugins/logstash/LogstashConfiguration.java

@@ -171,6 +172,10 @@ public void migrateData()
  @Override
  public boolean configure(StaplerRequest staplerRequest, JSONObject json) throws FormException
  {
+
+    // Keep a reference to the current configuration to revert back in case of an exception.
+    LogstashIndexer<?> currentIndexer = logstashIndexer;


maybe previousIndexer would be a better name? "Current" might be understood as referring to the one being processed now

jakub-bochenski · 2018-04-03T17:34:08Z

src/main/java/jenkins/plugins/logstash/LogstashConfiguration.java

+
+    try {
+      // Validate mimetype and throw exception in case found invalid.
+      if (logstashIndexer instanceof ElasticSearch) {


Why not just declare a noop validate() method on LogstashIndexer and override it in ElasticSearch implementation?

mwinter69 · 2018-04-10T05:54:32Z

src/main/java/jenkins/plugins/logstash/LogstashConfiguration.java

@@ -180,6 +185,19 @@ public boolean configure(StaplerRequest staplerRequest, JSONObject json) throws
    {
      activeIndexer = logstashIndexer;
    }
+
+    try {


This check need to be done before assigning it to activeIndexer as activeIndexer is the one that is really used.

jakub-bochenski · 2018-04-10T11:03:57Z

src/main/java/jenkins/plugins/logstash/configuration/LogstashIndexer.java

+   * 
+   * @throws Exception
+   */
+  public void validate() throws Exception {


[minor] it would be better to have something more specific than Exception in the signature (e.g. ConstraintViolationException); OTOH it would force us to catch and wrap MimeTypeParseException, so I'm not sure if it's worth it

OTOH it would force us to catch and wrap MimeTypeParseException, so I'm not sure if it's worth it

Thats the reason I didn't use specific exception there. In this case I feel we should not bother about the specific exception. If validate() throws any exception we know it is due to invalid indexer config so simply displaying wrapped exception would be sufficient . Let me know if you feel there is a value in that I can make a change.

VamanKulkarni · 2018-05-07T15:34:15Z

@jakub-bochenski @mwinter69 Any further comments ?

jakub-bochenski · 2018-05-07T15:56:57Z

LGTM

jakub-bochenski · 2018-05-07T15:59:18Z

@mwinter69 please let me know if you approve so that I can merge it

mwinter69

Except this minor thing it looks good

mwinter69 · 2018-05-07T16:10:51Z

src/main/java/jenkins/plugins/logstash/LogstashConfiguration.java

@@ -171,15 +172,33 @@ public void migrateData()
  @Override
  public boolean configure(StaplerRequest staplerRequest, JSONObject json) throws FormException
  {
+
+    // Keep a reference to the current configuration to revert back in case of an exception.
+    LogstashIndexer<?> previousIndexer = logstashIndexer;


I think we don't need this variable, We already have activeIndexer which holds the active configuration and can revert to this.

mwinter69 · 2018-05-07T16:11:02Z

src/main/java/jenkins/plugins/logstash/LogstashConfiguration.java

+      // Exception will be thrown here so that it gets displayed on UI. 
+      // But before that revert back to original configuration (in-memory) 
+      // so that when user refreshes the configuration page, last saved settings will be displayed again.
+      logstashIndexer = previousIndexer;


revert to activeIndexer

VamanKulkarni · 2018-05-08T14:38:37Z

@mwinter69 Updated code with review comments

jakub-bochenski · 2018-05-08T14:42:59Z

@VamanKulkarni looks like the tests are failing with NPE now

VamanKulkarni · 2018-05-08T14:52:42Z

@jakub-bochenski You are right.. I missed to add a file in the commit. Updated the PR now.

Drop charset from content type in Elasticsearch indexer

a359903

jakub-bochenski requested changes Dec 18, 2017

View reviewed changes

Refactor to make code unit-testable

117ee91

jakub-bochenski approved these changes Dec 22, 2017

View reviewed changes

jakub-bochenski reviewed Dec 22, 2017

View reviewed changes

jakub-bochenski mentioned this pull request Dec 28, 2017

move configuration from ToolInstallation to GlobalConfiguration #43

Merged

Merge branch 'master' into master

d6e2d73

jakub-bochenski mentioned this pull request Dec 29, 2017

findbugs: fix default encoding issues #45

Merged

Merge branch 'master' into master

7576829

VamanKulkarni force-pushed the master branch 2 times, most recently from 77a9425 to 5e71bcf Compare February 18, 2018 09:01

Updates required due to code refactoring

4146ffe

VamanKulkarni force-pushed the master branch from 5e71bcf to 4146ffe Compare February 18, 2018 09:03

jakub-bochenski approved these changes Feb 20, 2018

View reviewed changes

mwinter69 reviewed Feb 26, 2018

View reviewed changes

Merge branch 'master' of https://github.com/jenkinsci/logstash-plugin

722137e

jakub-bochenski reviewed Apr 3, 2018

View reviewed changes

Do not allow invalid mimetype to be saved

6a670a5

VamanKulkarni force-pushed the master branch from 07efc5f to 6a670a5 Compare April 4, 2018 16:40

mwinter69 reviewed Apr 10, 2018

View reviewed changes

jakub-bochenski reviewed Apr 10, 2018

View reviewed changes

Vaman Kulkarni added 2 commits April 14, 2018 19:58

Merge branch 'master' of https://github.com/jenkinsci/logstash-plugin

4fabced

Incorporate review comments

e67b6a8

mwinter69 reviewed May 7, 2018

View reviewed changes

Incorporate review comments

ed77e45

VamanKulkarni force-pushed the master branch from c6b3b20 to ed77e45 Compare May 8, 2018 14:51

jakub-bochenski merged commit d08c486 into jenkinsci:master May 8, 2018

Use mime type from config field in Elasticsearch indexer while posting HTTP request #41

Use mime type from config field in Elasticsearch indexer while posting HTTP request #41

Conversation

VamanKulkarni commented Dec 16, 2017

jakub-bochenski left a comment

Choose a reason for hiding this comment

VamanKulkarni commented Dec 19, 2017

jakub-bochenski commented Dec 19, 2017

VamanKulkarni commented Dec 20, 2017

jakub-bochenski commented Dec 20, 2017 • edited Loading

VamanKulkarni commented Dec 21, 2017

jakub-bochenski commented Dec 21, 2017

VamanKulkarni commented Dec 22, 2017

Choose a reason for hiding this comment

VamanKulkarni Dec 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakub-bochenski Dec 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakub-bochenski commented Dec 28, 2017

VamanKulkarni commented Dec 29, 2017

jakub-bochenski commented Dec 29, 2017

VamanKulkarni commented Jan 2, 2018

jakub-bochenski commented Jan 30, 2018

VamanKulkarni commented Feb 18, 2018

jakub-bochenski commented Feb 20, 2018

mwinter69 commented Feb 20, 2018

VamanKulkarni commented Feb 26, 2018

jakub-bochenski commented Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VamanKulkarni Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VamanKulkarni commented May 7, 2018

jakub-bochenski commented May 7, 2018

jakub-bochenski commented May 7, 2018

mwinter69 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VamanKulkarni commented May 8, 2018

jakub-bochenski commented May 8, 2018

VamanKulkarni commented May 8, 2018

jakub-bochenski commented Dec 20, 2017 •

edited

Loading

VamanKulkarni Dec 23, 2017 •

edited

Loading

jakub-bochenski Dec 28, 2017 •

edited

Loading

jakub-bochenski commented Feb 26, 2018 •

edited

Loading

VamanKulkarni Mar 14, 2018 •

edited

Loading