-
Notifications
You must be signed in to change notification settings - Fork 363
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 #1444
Conversation
Thanks for your PR. The following commands are available:
|
@antoninbas could you please help push a newer version of ipfix-collector? |
812b60d
to
b4c2fc3
Compare
Codecov Report
@@ Coverage Diff @@
## ipv6 #1444 +/- ##
===========================================
- Coverage 67.71% 41.61% -26.11%
===========================================
Files 165 85 -80
Lines 13060 10729 -2331
===========================================
- Hits 8844 4465 -4379
- Misses 3284 5864 +2580
+ Partials 932 400 -532
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@lzhecheng please review and merge #1449 |
91c42c5
to
caab675
Compare
/test-e2e |
/test-e2e |
/test-e2e |
/test-e2e |
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 the PR, Zhecheng.
As Antrea Proxy plans to support IPv6, "destinationClusterIP" IPFIX field in Antrea registry need to support both fields IPv4 and IPv6. Currently, in Antrea registry, we have only IPv4 field for clusterIP. Will raise a PR in go-ipfix to support both fields.
cmd/antrea-agent/options_test.go
Outdated
[]string{"1.2.3.4", "80"}, | ||
}, | ||
{ | ||
"[fe80:1:2::3]:80:tcp", |
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.
Don't we need 16 bytes for IPv6 slice?
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.
I think we are using string as input and output here?
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 question was about number of bytes in considered address: fe80:1:2::3. I think rest of the bytes are 0s in 16 byte address--is that correct? Maybe a better example for IPv6 address is to consider whole 16 byte address?
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. Got it. Updated.
} | ||
|
||
// TestFlowExporter_sendDataRecord tests essentially if element names in the switch-case matches globals | ||
// IANAInfoElements and AntreaInfoElements. | ||
// IANAInfoElementsIPv4 and AntreaInfoElements. |
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.
why no test with IANAInfoElementsIPv6? Are you waiting for the fix in go-ipfix?
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.
Make sense. Updated test for IPv6.
@@ -132,7 +131,7 @@ func checkRecordsWithPodIPs(t *testing.T, data *TestData, podAIP string, podBIP | |||
templateRecords = templateRecords + 1 | |||
} | |||
|
|||
if strings.Contains(record, podAIP) && strings.Contains(record, podBIP) { | |||
if strings.Contains(record, lengthenIPv6Addr(isIPv6, podAIP)) && strings.Contains(record, lengthenIPv6Addr(isIPv6, podBIP)) { |
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.
Are we seeing correct IPv6 address in flow records?
With the current go-ipfix code, ipv6 addresses do not get encoded on to flow records in the exporter code.
https://github.com/vmware/go-ipfix/blob/master/pkg/entities/ie.go#L390
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.
I see that ipv6 encoding is being supported in release-0.2.0 branch, which is currently used in Antrea. However, it is missing in the master branch. This needs to be fixed if we move to future releases.
https://github.com/vmware/go-ipfix/blob/release-0.2.0/pkg/entities/record.go#L208
Ignore the above comment.
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 FYI, the master branch issue is taken care of in this PR: vmware/go-ipfix#64
091d85e
to
1a52ad3
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.
We support destination clusterIP IPv6 address in this patch release: https://github.com/vmware/go-ipfix/releases/tag/v0.2.3
Please use that to add IPv6 destination cluster IP for Pod-To-Service flows.
cmd/antrea-agent/options_test.go
Outdated
[]string{"1.2.3.4", "80"}, | ||
}, | ||
{ | ||
"[fe80:1:2::3]:80:tcp", |
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 question was about number of bytes in considered address: fe80:1:2::3. I think rest of the bytes are 0s in 16 byte address--is that correct? Maybe a better example for IPv6 address is to consider whole 16 byte address?
@@ -132,7 +131,7 @@ func checkRecordsWithPodIPs(t *testing.T, data *TestData, podAIP string, podBIP | |||
templateRecords = templateRecords + 1 | |||
} | |||
|
|||
if strings.Contains(record, podAIP) && strings.Contains(record, podBIP) { | |||
if strings.Contains(record, lengthenIPv6Addr(isIPv6, podAIP)) && strings.Contains(record, lengthenIPv6Addr(isIPv6, podBIP)) { |
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 FYI, the master branch issue is taken care of in this PR: vmware/go-ipfix#64
30fe68c
to
2145879
Compare
* ping failure when crossing nodes * incorrect ipset: without CIDR of current node * TestPodTrafficShaping for IPv6 * TestAntctlProxy: skip if ipv6 * DeleteRoutes parameter change
Thanks for your PR. The following commands are available:
|
Support IPv4 or IPv6 flow exporter address.
I see there are a lot of commits are added to the PR--a total of 86 files. Is this PR still valid? Is it because of rebasing the ipv6 branch with the master? |
Yes, it is due to ipv6 branch updating to the master. This PR should rebase then. |
@@ -185,7 +192,10 @@ func (o *Options) validateFlowExporterConfig() error { | |||
return fmt.Errorf("IPFIX flow collector address should be provided") | |||
} else { | |||
// Check if it is TCP or UDP | |||
strSlice := strings.Split(o.config.FlowCollectorAddr, ":") | |||
strSlice, err := parseFlowCollectorAddr(o.config.FlowCollectorAddr) |
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.
Maybe add a comment in antrea-agent.conf about IPv6 is supported for flowCollectorAddr.
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.
@@ -189,7 +192,11 @@ func (cs *ConnectionStore) Poll() (int, error) { | |||
// We do not expect any error as resetConn is not returning any error | |||
cs.ForAllConnectionsDo(resetConn) | |||
|
|||
filteredConnsList, totalConns, err := cs.connDumper.DumpFlows(openflow.CtZone) | |||
var zone uint16 = openflow.CtZone | |||
if cs.serviceCIDR != nil && cs.serviceCIDR.IP.To4() == nil { |
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.
If ConnectionStore.serviceCIDR is used only here, we just need to pass and save a v4 or v6 flag in the ConnectionStore struct?
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.
Make sense. Updated.
dd85a9e
to
267d1c0
Compare
PR #1541 replaces this PR after ipv6 branch is merged to master. |
Support IPv4 or IPv6 flow exporter address.