-
Notifications
You must be signed in to change notification settings - Fork 359
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
L7 Network Flow Export support in Antrea #5218
Conversation
98d13da
to
6cc14bb
Compare
8a11a69
to
b8555a2
Compare
57fb343
to
2799512
Compare
pkg/agent/controller/networkpolicy/l7engine/l7visbility_watcher.go
Outdated
Show resolved
Hide resolved
pkg/agent/controller/networkpolicy/l7engine/l7visbility_watcher.go
Outdated
Show resolved
Hide resolved
ad1eb60
to
56919fe
Compare
56919fe
to
8453bcf
Compare
ab34f38
to
1f4f8d1
Compare
bb15a54
to
cf33369
Compare
for connKey, conn := range cs.connections { | ||
l7event, ok := l7EventMap[connKey] |
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.
the placement of the second part of the comment is strange:
In case the L7 event is received later than the connkey become unavailable in the cs.connection, we will discard that event
When this happens, we never get to this part of the code, because the for loop (for connKey, conn := range cs.connections
) only considers connections in the store. So maybe that second sentence should go before the loop?
} | ||
// In case L7 event is received after the last planned export of the TCP connection, add | ||
// the event back to the queue to be exported in next export cycle. In case the L7 event | ||
// is received later than the connkey become unavailable in the cs.connection, we will |
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.
...is received after the connection is removed from the cs.connections store...
test/e2e/flowaggregator_test.go
Outdated
@@ -1865,6 +1879,67 @@ func getAndCheckFlowAggregatorMetrics(t *testing.T, data *TestData) error { | |||
return nil | |||
} | |||
|
|||
func testL7FlowExporterController(t *testing.T, data *TestData, isIPv6 bool) { | |||
skipIfFeatureDisabled(t, features.L7FlowExporter, true, false) | |||
_, serverIPs, cleanupFunc := createAndWaitForPod(t, data, data.createNginxPodOnNode, "l7flowexportertestpodserver", nodeName(0), data.testNamespace, false) |
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: define nodeName := nodeName(0)
at the beginning of the function and use this throughout the test case
@yuntanghsu could you take one more look at the new e2e test? |
cf33369
to
c2b6b31
Compare
@@ -325,3 +336,27 @@ func (cs *ConntrackConnectionStore) deleteConnWithoutLock(connKey flowexporter.C | |||
func (cs *ConntrackConnectionStore) GetPriorityQueue() *priorityqueue.ExpirePriorityQueue { | |||
return cs.connectionStore.expirePriorityQueue | |||
} | |||
|
|||
func (cs *ConntrackConnectionStore) fillL7EventInfo(l7EventMap map[flowexporter.Tuple]L7ProtocolFields) { | |||
// In case the L7 event is received after the connection is removed from the cs.connections store. |
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/store./store,
c2b6b31
to
528f774
Compare
func checkL7FlowExporterData(t *testing.T, record, appProtocolName string) { | ||
assert.Containsf(t, record, fmt.Sprintf("appProtocolName: %s", appProtocolName), "Record does not have correct Layer 7 protocol Name") | ||
} | ||
|
||
func checkL7FlowExporterDataClickHouse(t *testing.T, record *ClickHouseFullRow, appProtocolName string) { | ||
assert.Equal(t, record.AppProtocolName, appProtocolName, "Record does not have correct Layer 7 protocol Name") | ||
assert.NotEmpty(t, record.HttpVals, "Record does not have httpVals") | ||
} |
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 these two functions? I think they are only used once and the value of appProtocolName
is always http
?
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.
the two functions are for two different records, one for click house other one for collector
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. But each one of them is used only once. I was thinking maybe it's better to add line#1402 to line#1927 and line#1406-1407 to line#1939
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.
Sure , we can do that, however, this helps in future when we may support other protocols
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.
let's keep this as it is, this PR has been pending for a while
test/e2e/flowaggregator_test.go
Outdated
assert.Contains(record, testFlow1.srcPodName, "Record with srcIP does not have Pod name: %s", testFlow1.srcPodName) | ||
assert.Contains(record, fmt.Sprintf("sourcePodNamespace: %s", data.testNamespace), "Record does not have correct sourcePodNamespace: %s", data.testNamespace) | ||
assert.Contains(record, fmt.Sprintf("sourceNodeName: %s", nodeName), "Record does not have correct sourceNodeName: %s", nodeName) |
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 you can reuse checkPodAndNodeData
Like, checkPodAndNodeData(t, record, testFlow1.srcPodName, nodeName, "", "", data.testNamespace)
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.
those functions are specific to perftest labels, we may add a general function later!
528f774
to
129529b
Compare
L7 Network flow export enables Layer 7 flow export as we did for the l3/l4 earlier. This feature enables the user to export L7 protocol information using: - Pod or namespace Annotations Annotation key used is "visibility.antrea.io/l7-export" and the value is the direction for which the flow export is required, which could be (ingress/egress/both). Based of the annotation or the NP, the fields ("appProtocolName", "httpVals") are populated and exported. Signed-off-by: Tushar Tathgur <tathgurt@tathgurtPNQHP.vmware.com>
129529b
to
2c0a3bb
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.
LGTM
/test-all |
The new e2e test failed |
Signed-off-by: Tushar Tathgur <tathgurt@tathgurtPNQHP.vmware.com>
2c0a3bb
to
9dfc6f1
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.
LGTM
/test-all |
L7 Network flow export enables Layer 7 flow export as we did for the l3/l4 earlier.
This feature enables the user to export L7 protocol information using:
Annotation key used is "visibility.antrea.io/l7-export" and the value is the direction for which the flow export is required, which could be (ingress/egress/both).
Based of the annotation or the NP, the fields ("appProtocolName", "httpVals") are populated and exported.
### - Design:
L7FlowExport is to be designed in order to receive all the protected and unprotected flows of Layer7 type and to be exported.
In order to complete this feature, we need the following flows.
L7FlowExport using the Annotations.
New Fields to be added in the exporter.
appProtocolName: This field will help export only the l7 flows if required. It is used to also take care of future L7 protocols implementations which may require adding new fields and "appProtocolName" to provide the differentiating field for L7 flows from L3/L4 flows.
HttpVals: This field is used and should contain all the http fields (in order) corresponding to a network flow. The fields are:
[Hostname, URL, UserAgent, ContentType, Method, Protocol, Status, ContentLength]
** GO-IPFIX changes are present in vmware/go-ipfix#315