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

[Filebeat] Add support to unix sockets in Nginx module #10944

Merged
merged 6 commits into from
Apr 15, 2019

Conversation

sayden
Copy link
Contributor

@sayden sayden commented Feb 26, 2019

Tries to cover this: #10309

As there's not example in the issue, I have configured an Nginx myself to listen unix sockets and then access it using netcat to generate a log output, which is included in the PR.

@sayden sayden added enhancement Filebeat Filebeat Team:Integrations Label for the Integrations team labels Feb 26, 2019
@sayden sayden self-assigned this Feb 26, 2019
@sayden sayden requested review from a team as code owners February 26, 2019 14:50
Copy link
Contributor

@kaiyan-sheng kaiyan-sheng left a comment

Choose a reason for hiding this comment

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

Looks good!

@sayden
Copy link
Contributor Author

sayden commented Feb 27, 2019

jenkins, test this

@@ -17,6 +17,9 @@
- name: remote_ip
type: alias
path: source.ip
- name: origin
type: array
path: nginx.access.origin
Copy link
Member

Choose a reason for hiding this comment

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

We should use source.address also when the source is a unix socket, so no need to add a field for this.

If you would need to add this field, it'd be:

- name: origin
  type: keyword
  description: >
     Some description

path is intended to be used with aliases, and migration for migrated fields.

Copy link
Member

Choose a reason for hiding this comment

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

For the use of keyword instead of array, take a look to https://www.elastic.co/guide/en/elasticsearch/reference/6.6/array.html

Copy link
Contributor Author

@sayden sayden Apr 15, 2019

Choose a reason for hiding this comment

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

Those fields have been moved to ECS so no need to update fields here finally

@@ -43,17 +51,24 @@
{
"script": {
"lang": "painless",
"source": "boolean isPrivate(def dot, def ip) { try { StringTokenizer tok = new StringTokenizer(ip, dot); int firstByte = Integer.parseInt(tok.nextToken()); int secondByte = Integer.parseInt(tok.nextToken()); if (firstByte == 10) { return true; } if (firstByte == 192 && secondByte == 168) { return true; } if (firstByte == 172 && secondByte >= 16 && secondByte <= 31) { return true; } if (firstByte == 127) { return true; } return false; } catch (Exception e) { return false; } } def found = false; for (def item : ctx.nginx.access.remote_ip_list) { if (!isPrivate(params.dot, item)) { ctx.source.ip = item; found = true; break; } } if (!found) { ctx.source.ip = ctx.nginx.access.remote_ip_list[0]; }",
"source": "boolean isPrivate(def dot, def ip) { try { StringTokenizer tok = new StringTokenizer(ip, dot); int firstByte = Integer.parseInt(tok.nextToken()); int secondByte = Integer.parseInt(tok.nextToken()); if (firstByte == 10) { return true; } if (firstByte == 192 && secondByte == 168) { return true; } if (firstByte == 172 && secondByte >= 16 && secondByte <= 31) { return true; } if (firstByte == 127) { return true; } return false; } catch (Exception e) { return false; } } try { ctx.source.ip = null; if (ctx.nginx.access.remote_ip_list == null) { return; } def found = false; for (def item : ctx.nginx.access.remote_ip_list) { if (!isPrivate(params.dot, item)) { ctx.source.ip = item; found = true; break; } } if (!found) { ctx.source.ip = ctx.nginx.access.remote_ip_list[0]; }} catch (Exception e) { ctx.source.ip = null; }",
Copy link
Member

Choose a reason for hiding this comment

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

It is a bit difficult to see changes here 😬, but, why setting source.ip to null if later it is removed in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question and sorry! I remember to think about "I'm gonna copy-paste pretty code on PR" and I finally forgot it

boolean isPrivate(def dot, def ip) {
    try {
        StringTokenizer tok = new StringTokenizer(ip, dot);
        int firstByte = Integer.parseInt(tok.nextToken());
        int secondByte = Integer.parseInt(tok.nextToken());
        if (firstByte == 10) {         return true;       }
        if (firstByte == 192 && secondByte == 168) {         return true;       }
        if (firstByte == 172 && secondByte >= 16 && secondByte <= 31) {         return true;       }
        if (firstByte == 127) {         return true;       }
        return false;
    } catch (Exception e) {
        return false;
    }
}

try {
    ctx.source.ip = null;
    if (ctx.nginx.access.remote_ip_list == null) { return; }
    def found = false;
    for (def item : ctx.nginx.access.remote_ip_list) {
        if (!isPrivate(params.dot, item)) {
            ctx.source.ip = item;
            found = true;
            break;
        }
    }
    
    if (!found) {
        ctx.source.ip = ctx.nginx.access.remote_ip_list[0];
    }
} catch (Exception e) {
    ctx.source.ip = null;
}

The thing is that "convert" step in the pipeline was failing by not recognizing "" as a valid string with the error:

{"type":"mapper_parsing_exception","reason":"failed to parse field [source.ip] of type [ip] in document with id '6SrYOWkBxc4oeub4XID4'","caused_by":{"type":"illegal_argument_exception","reason":"'' is not an IP string literal."}

But an ignore_missing works if the field doesn't exists or it's null. It doesn't work if field is just empty and ignore_failure seemed to just omit the entire line (not the processing step).

I have to admit that it was terribly difficult

Copy link
Member

Choose a reason for hiding this comment

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

Now it seems that we can use yaml here 😌 #11209

@sayden sayden added the in progress Pull request is currently in progress. label Mar 13, 2019
@sayden sayden force-pushed the feature/fb/nginx-sockets branch from 17e7721 to 656206e Compare April 12, 2019 10:12
@sayden sayden added review and removed in progress Pull request is currently in progress. labels Apr 12, 2019
@@ -43,17 +51,24 @@
{
"script": {
"lang": "painless",
"source": "boolean isPrivate(def dot, def ip) { try { StringTokenizer tok = new StringTokenizer(ip, dot); int firstByte = Integer.parseInt(tok.nextToken()); int secondByte = Integer.parseInt(tok.nextToken()); if (firstByte == 10) { return true; } if (firstByte == 192 && secondByte == 168) { return true; } if (firstByte == 172 && secondByte >= 16 && secondByte <= 31) { return true; } if (firstByte == 127) { return true; } return false; } catch (Exception e) { return false; } } def found = false; for (def item : ctx.nginx.access.remote_ip_list) { if (!isPrivate(params.dot, item)) { ctx.source.ip = item; found = true; break; } } if (!found) { ctx.source.ip = ctx.nginx.access.remote_ip_list[0]; }",
"source": "boolean isPrivate(def dot, def ip) { try { StringTokenizer tok = new StringTokenizer(ip, dot); int firstByte = Integer.parseInt(tok.nextToken()); int secondByte = Integer.parseInt(tok.nextToken()); if (firstByte == 10) { return true; } if (firstByte == 192 && secondByte == 168) { return true; } if (firstByte == 172 && secondByte >= 16 && secondByte <= 31) { return true; } if (firstByte == 127) { return true; } return false; } catch (Exception e) { return false; } } try { ctx.source.ip = null; if (ctx.nginx.access.remote_ip_list == null) { return; } def found = false; for (def item : ctx.nginx.access.remote_ip_list) { if (!isPrivate(params.dot, item)) { ctx.source.ip = item; found = true; break; } } if (!found) { ctx.source.ip = ctx.nginx.access.remote_ip_list[0]; }} catch (Exception e) { ctx.source.ip = null; }",
Copy link
Member

Choose a reason for hiding this comment

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

Now it seems that we can use yaml here 😌 #11209

@sayden sayden merged commit 9b7c782 into elastic:master Apr 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Filebeat Filebeat review Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants