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

packetbeat: add support for NFS v3 and v4 protocols #1231

Merged
merged 1 commit into from Mar 30, 2016

Conversation

Projects
None yet
4 participants
@kofemann
Copy link
Contributor

commented Mar 24, 2016

This pull request is more an RFC. We will run it bit in production go get more experience. Comments are welcome. The example output:

{
  "@timestamp": "2016-03-28T06:18:18.431Z",
  "beat": {
    "hostname": "localhost",
    "name": "localhost"
  },
  "count": 1,
  "dst": "127.0.0.1",
  "dst_port": 2049,
  "nfs": {
    "minor_version": 1,
    "opcode": "GETATTR",
    "status": "NFSERR_NOENT",
    "tag": "",
    "version": 4
  },
  "rpc": {
    "auth_flavor": "unix",
    "call_size": 200,
    "cred": {
      "gid": 500,
      "gids": [
        491,
        499,
        500
      ],
      "machinename": "localhost",
      "stamp": 4597002,
      "uid": 500
    },
    "reply_size": 96,
    "status": "success",
    "time": 25631000,
    "time_str": "25.631ms",
    "xid": "2cf0c876"
  },
  "src": "127.0.0.1",
  "src_port": 975,
  "type": "nfs"
}
@elasticsearch-release

This comment has been minimized.

Copy link

commented Mar 24, 2016

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

@kofemann

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2016

The one of the questions I would like to know: the other beats use field names with dots, but elasticsearch 2.0 does not allow that. So is it ok in beats?

@kofemann kofemann force-pushed the kofemann:nfs branch 10 times, most recently from 57ee96e to eeda6f4 Mar 25, 2016

@kofemann

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2016

Ok. Now it's in the shape, that others can use it as well, IOW - this is a real pull request now.

@kofemann

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2016

Dashboard example
screenshot from 2016-03-28 17-17-26

@kofemann kofemann force-pushed the kofemann:nfs branch 2 times, most recently from 52dc216 to 8fa1d89 Mar 28, 2016

xdr Xdr
vers uint32
proc uint32
event *common.MapStr

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Mar 29, 2016

Member

I would recommend not using a pointer here. Maps are reference types so they are always passed by reference.

This comment has been minimized.

Copy link
@kofemann

kofemann Mar 29, 2016

Author Contributor

fixed.

}

func (nfs *Nfs) getV3Opcode() string {
if nfs.proc < 22 {

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Mar 29, 2016

Member

Instead of 22 use len(nfs_opnum3).

This comment has been minimized.

Copy link
@kofemann

kofemann Mar 29, 2016

Author Contributor

fixed


import "fmt"

const OP_ACCESS = 3

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Mar 29, 2016

Member

Consider grouping these constants in a single const block. It's common to group related constants this way.

const (
    OP_ACCESS = 3
    OP_CLOSE = 4
)

I think you could also use iota here.

const (
    OP_ACCESS = iota + 3
    OP_CLOSE
    OP_COMMIT
    ...
    OP_ILLEGAL = 10044
)

This comment has been minimized.

Copy link
@kofemann

kofemann Mar 29, 2016

Author Contributor

fixed

}

if !is_last {
logp.Warn("rpc", "multifragment rpc message")

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Mar 29, 2016

Member

logp.Warn doesn't accept a debug selector argument from what I recall.

This comment has been minimized.

Copy link
@kofemann

kofemann Mar 29, 2016

Author Contributor

fixed

// concatenate bytes
st.rawData = append(st.rawData, pkt.Payload...)
if len(st.rawData) > tcp.TCP_MAX_DATA_IN_STREAM {
logp.Debug("rpc", "Stream data too large, dropping TCP stream")

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Mar 29, 2016

Member

I would recommend creating a single debugf function for your package. And everywhere you write logp.Debug("rpc", "some message") replace it with debugf("some message"). It makes it easy to see what the debug selectors are for the package and it avoids the problem of typos with the selector.

Basically at the package level just add this:

var debugf = logp.MakeDebug("rpc")

This comment has been minimized.

Copy link
@kofemann

kofemann Mar 29, 2016

Author Contributor

fixed


var calls_seen = common.NewCache(1*time.Minute, 8192)

func (msg *RpcMessage) fillEvent(event *common.MapStr, results publish.Transactions, size int) {

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Mar 29, 2016

Member

No need to use a MapStr pointer as mentioned earlier.

This comment has been minimized.

Copy link
@kofemann

kofemann Mar 29, 2016

Author Contributor

this was not clear to me. Thanks.

rpc_prog_vers := msg.xdr.getUInt()
rpc_proc := msg.xdr.getUInt()

if rpc_prog == 100003 {

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Mar 29, 2016

Member

Seems like 100003 should be named constant?

This comment has been minimized.

Copy link
@kofemann

kofemann Mar 29, 2016

Author Contributor

fixed

@andrewkroh

This comment has been minimized.

Copy link
Member

commented Mar 29, 2016

Thanks for the contribution! Very cool screen shot. A couple miscellaneous points:

  • Try running your package against goimports. It should change the ordering on some of the imports.
  • Some of the variable names are using underscores. The Go convention is mixedCase. Try running golint against your package.
  • I see you added some pcaps. To ensure that there are no regressions we like to have some system tests in-place that replay the pcaps and check the outputs. Could you add some test cases like these ones that use those pcaps?
  • And finally, to make sure that everyone understands what the fields mean it would be great if you could document their meanings. We put each field into fields.yml then we generate documentation from it by running make update.

@kofemann kofemann force-pushed the kofemann:nfs branch from 8fa1d89 to 60a4306 Mar 29, 2016

@kofemann

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2016

I have addresses all issues you pointed out.. With style it was not clear, the code used mixed style in various places, like tcptuple.Dst_ip.String(). Any way, Tests added, rebased to current master, branch updated.

@andrewkroh

This comment has been minimized.

Copy link
Member

commented Mar 29, 2016

The one of the questions I would like to know: the other beats use field names with dots, but elasticsearch 2.0 does not allow that. So is it ok in beats?

Because field names containing dots are not allowed in ES 2.0 we do not use them in Beats. We use nested maps which gives the appearance of field names with dots because you can use dot-notation to Kibana and ES to reference the fields. For example:

event = common.MapStr{}
nfs = common.MapStr{}
event["nfs"] = nfs
nfs["version"] = 4

It appears that you figured this out. 👍 I neglected to see the question until now.


assert o["type"] == "nfs"
assert o["rpc.auth_flavor"] == "unix"
assert "rpc.time" in o

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Mar 29, 2016

Member

There appears to be a few tabs here that are throwing off the indentation. Please convert them to spaces. Thanks

This comment has been minimized.

Copy link
@kofemann

kofemann Mar 29, 2016

Author Contributor

sorry about that. fixed.

@kofemann kofemann force-pushed the kofemann:nfs branch from 60a4306 to 1ae698c Mar 29, 2016

@andrewkroh

This comment has been minimized.

Copy link
Member

commented Mar 29, 2016

Looks good. I would like to get at least one more reviewer on this. @elastic/beats

@kofemann

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2016

I will create a separate PR for dashboard later on.

@@ -1288,6 +1288,74 @@ trans_event:
description: >
The cursor identifier returned in the OP_REPLY. This must be the value that was returned from the database.
- name: rpc

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Mar 29, 2016

Member

Please run make update to re-generate the packetbeat/docs/fields.asciidoc document.

@monicasarbu

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2016

@kofemann Thank you for the complete PR and the nice dashboard. I have only a small concern.

I think it would be easier for the users to understand if we rename the Packetbeat module to nfs to be the same with the name of the configuration section nfs. In this case, we can move all the RPC information from rpc under nfs.rpc.

@kofemann kofemann force-pushed the kofemann:nfs branch from 1ae698c to 90f1a9b Mar 29, 2016

@kofemann

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2016

@monicasarbu I had the same idea during prototyping was renaming it already several times :). The reason to call it rpc is that it will be easy to add other oncrpc/sunrpc based protocols as well. Nevertheless, I am fine to rename it. @andrewkroh any strong opinion?

@andrewkroh

This comment has been minimized.

Copy link
Member

commented Mar 29, 2016

I would rename to nfs and re-organize event structure now. If in the future new RPC based protocols are added we can refactor a bit to reuse this code.

@kofemann kofemann force-pushed the kofemann:nfs branch from 90f1a9b to 0efe789 Mar 29, 2016

@kofemann

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2016

updated. The fields are still rpc.xxx and nfs.xxx. This makes more sense. You don't put tcp relates staff into http namspace

packetbeat: add support for NFS v3 and v4 protocols
Signed-off-by: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de>

@kofemann kofemann force-pushed the kofemann:nfs branch from 0efe789 to ce41d1a Mar 30, 2016

@monicasarbu

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2016

@kofemann Ok, I agree with you that it makes more sense to have rpc and nfs inside the nfs event type.

@monicasarbu monicasarbu merged commit 928d8d7 into elastic:master Mar 30, 2016

3 checks passed

CLA Commit author has signed the CLA
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

monicasarbu added a commit to monicasarbu/beats that referenced this pull request Mar 30, 2016

@kofemann kofemann deleted the kofemann:nfs branch Mar 30, 2016

@kofemann

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2016

thanks! About dashboard: shold it go into packetbeat/etc/kibana ? Is there any conventions?

@monicasarbu

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2016

@kofemann I am working now at a simple way to import and export the Kibana dashboard. For now, you can only export all the Kibana dashboards and dependencies (visualizations, searches, index-patterns) using the python script from https://github.com/elastic/beats-dashboards/tree/master/save. Please send us a PR against the beats-dashboards repo with all the exported dashboards and dependencies. I can help you to sort them out once the PR is opened. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.