Skip to content

Fixing SFlowIpv6Record SFlowIpv4Record does not resolve the problem o…#401

Open
lflxp wants to merge 3 commits intogoogle:masterfrom
lflxp:master
Open

Fixing SFlowIpv6Record SFlowIpv4Record does not resolve the problem o…#401
lflxp wants to merge 3 commits intogoogle:masterfrom
lflxp:master

Conversation

@lflxp
Copy link

@lflxp lflxp commented Jan 2, 2018

When I parsed the sFlow V5 with gopacket, I found that the length of the SFlowIpv4Record interception was wrong, and it was found that the lack of SFlowBaseFlowRecord resulted in the lack of SFlowBaseFlowRecord

@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@florianl florianl requested a review from safchain January 2, 2018 11:01
@florianl florianl removed the request for review from safchain January 2, 2018 11:57
@lflxp
Copy link
Author

lflxp commented Jan 2, 2018

I signed it!

@ezak
Copy link

ezak commented Jan 3, 2018 via email

@gconnell
Copy link
Contributor

gconnell commented Jan 3, 2018

I believe the CLA bot is specifically looking for a CLA from "李学坪"

Also, currently Travis is complaining about this test failure:

$ go test github.com/google/gopacket/layers
--- FAIL: TestDecodeExtendedIpv4TunnelIngressFlow (0.00s)
	sflow_test.go:1097: Failed to decode packet: runtime error: slice bounds out of range
	base_test.go:25: Checking packet layers, want [SFlow]
	base_test.go:27:   Got layer DecodeFailure, 108 bytes, payload of 0 bytes
	base_test.go:30: PACKET: 108 bytes
		- Layer 1 (108 bytes) = DecodeFailure	Packet decoding error: runtime error: slice bounds out of range
		
	base_test.go:38:   Layer 0 mismatch: got DecodeFailure want SFlow
panic: interface conversion: gopacket.ApplicationLayer is nil, not *layers.SFlowDatagram [recovered]
	panic: interface conversion: gopacket.ApplicationLayer is nil, not *layers.SFlowDatagram

si := SFlowIpv4Record{}
var ir SFlowFlowDataFormat

*data, ir = (*data)[4:], SFlowFlowDataFormat(binary.BigEndian.Uint32((*data)[:4]))
Copy link
Contributor

Choose a reason for hiding this comment

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

I did test it and it seems to be not working for tunneling record. I tested it with Openswitch. Did you see any issue with tunneling record or was it with another kind of record ?

Copy link
Contributor

Choose a reason for hiding this comment

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

safchain, do you have a test case that could be added to the tests in this pull?

Copy link
Contributor

Choose a reason for hiding this comment

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

@gconnell there is already a test that this PR modifies. According to my test with Openvswitch, it doesn't seem to be working properly with this patch.

@lflxp Maybe the IPV4/IPV6 records are used somewhere else by sFlow protocol needing SFlowBaseFlowRecord. In that case we will need to use a king of decodeSFlowIpv4Data used by decodeSFlowIpv4Record and decodeExtendedIpv4TunnelEgress.

Below two records from a lab, first one without the patch second one with it. In the second one the SFlowIpv4Record is incorrect.

{SFlowBaseFlowRecord:{EnterpriseID:Standard SFlow Format:Extended IPv4 Tunnel Ingress Record FlowDataLength:32} SFlowIpv4Record:{Length:0 Protocol:47 IPSrc:10.41.20.10 IPDst:10.41.20.14 PortSrc:0 PortDst:0 TCPFlags:0 TOS:0}}

{SFlowBaseFlowRecord:{EnterpriseID:Standard SFlow Format:Extended IPv4 Tunnel Ingress Record FlowDataLength:32} SFlowIpv4Record:{SFlowBaseFlowRecord:{EnterpriseID:Standard SFlow Format: FlowDataLength:47} Length:170464266 Protocol:170464270 IPSrc:0.0.0.0 IPDst:0.0.0.0 PortSrc:0 PortDst:0 TCPFlags:3 TOS:1001}}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants