-
Notifications
You must be signed in to change notification settings - Fork 364
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
[IPv6] Support flow exporter #1541
Conversation
Thanks for your PR. The following commands are available:
|
019d81a
to
d14e98a
Compare
Codecov Report
@@ Coverage Diff @@
## master #1541 +/- ##
==========================================
+ Coverage 63.31% 64.30% +0.99%
==========================================
Files 170 181 +11
Lines 14250 15427 +1177
==========================================
+ Hits 9023 9921 +898
- Misses 4292 4472 +180
- Partials 935 1034 +99
Flags with carried forward coverage won't be shown. Click here to find out more.
|
/test-e2e /test-conformance /test-networkpolicy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main comment is about destinationClusterIPv6. Otherwise, it looks good to me.
ebc6fbf
to
6ed7a7c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lzhecheng Currently we consider only the connections in Antrea connection zone of conntrack table and ignore connections in the default zone.
There is a separate connection zone for IPv6. Don't we need to consider that connection zone if the stack is IPv6? Wondering if the flow exporter e2e test is passing with IPv6 stack or not.
I think I have it? |
6580cfa
to
1e173eb
Compare
/test-windows-conformance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes.
LGTM.
/test-ipv6-all |
/test-e2e |
/test-ipv6-e2e |
09d082e
to
6a50de8
Compare
/test-e2e |
/test-all |
/test-conformance |
|
||
return sentBytes, nil | ||
} | ||
|
||
func (exp *flowExporter) sendDataSet(dataSet ipfix.IPFIXSet, record flowexporter.FlowRecord, templateID uint16) error { | ||
func (exp *flowExporter) sendDataSet(dataSet ipfix.IPFIXSet, record flowexporter.FlowRecord, templateID uint16, isIPv6 bool) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the args templateID
and isIPv6
? I thinkrecord flowexporter.FlowRecord
and exp *flowExporter
should have these already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Removed.
@@ -195,9 +227,15 @@ func (exp *flowExporter) sendFlowRecords() error { | |||
return nil | |||
} | |||
|
|||
func (exp *flowExporter) sendTemplateSet(templateSet ipfix.IPFIXSet, templateID uint16) (int, error) { | |||
func (exp *flowExporter) sendTemplateSet(templateSet ipfix.IPFIXSet, templateID uint16, isIPv6 bool) (int, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
templateID is not needed as we assign the templateID to exp *flowExporter
in L179 and L189 and can use that inside the function based on isIPv6
. Is my understanding correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Updated.
ie.Value = record.Conn.TupleOrig.DestinationAddress | ||
} else { | ||
// Same as destinationClusterIPv4. | ||
ie.Value = net.IP{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: simplified version net.IP.Parse("::0")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems "::"
works as well. Updated.
pkg/agent/flowexporter/types.go
Outdated
@@ -70,4 +70,6 @@ type FlowRecord struct { | |||
PrevBytes uint64 | |||
PrevReversePackets uint64 | |||
PrevReverseBytes uint64 | |||
V4Enabled bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just need one field isIPv6 as one flow record corresponds to either v4 or v6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
@@ -35,9 +35,32 @@ func NewFlowRecords(connStore *connections.ConnectionStore) *FlowRecords { | |||
} | |||
|
|||
// BuildFlowRecords builds the flow record map from connection map in connection store | |||
func (fr *FlowRecords) BuildFlowRecords() error { | |||
func (fr *FlowRecords) BuildFlowRecords(v4Enabled bool, v6Enabled bool) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not need the args. From IP in key, we can say if it is v4 or v6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
d4de2f4
to
a579a20
Compare
/test-e2e |
/test-all |
fr.recordsMap[key] = record | ||
return nil | ||
} | ||
|
||
// fr.addOrUpdateFlowRecord method does not return any error, hence no error handling required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could change the comment that still says the method name of flow record rather than function name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. fr.addOrUpdateFlowRecord
-> addOrUpdateFlowRecord
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing comments.
LGTM.
/test-all |
/test-conformance |
/test-e2e |
/test-ipv6-only-conformance |
1 similar comment
/test-ipv6-only-conformance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple nits, otherwise LGTM
// Poll calls into conntrackDumper interface to dump conntrack flows. In a dual-stack setup, there will be two connsLens | ||
// get returned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Poll calls into conntrackDumper interface to dump conntrack flows. It returns the number of connections for each address family, as a slice. In dual-stack clusters, the slice will contain 2 values (number of IPv4 connections first, then number of IPv6 connections).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
IANAReverseInfoElements = []string{ | ||
"reversePacketTotalCount", | ||
"reverseOctetTotalCount", | ||
"reversePacketDeltaCount", | ||
"reverseOctetDeltaCount", | ||
} | ||
AntreaInfoElements = []string{ | ||
AntreaInfoElementsCommon = []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does not need to be exported I think... could be antreaInfoElementsCommon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. Updated.
|
||
func getElemList(ianaIE []string, antreaIE []string) []*ipfixentities.InfoElementWithValue { | ||
// Following consists of all elements that are in IANAInfoElements and AntreaInfoElements (globals) | ||
// Need only element name and other are dummys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/other are dummys/other fields are set to dummy values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
Co-authored-by: Antonin Bas <[email protected]> Co-authored-by: srikartati <[email protected]>
/test-all |
/test-conformance |
Thank you a lot for the effort to review! @antoninbas @srikartati |
Co-authored-by: Antonin Bas <[email protected]> Co-authored-by: srikartati <[email protected]>
No description provided.