-
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
Remove redundant log in fillPodInfo/fillServiceInfo and update deny connection store. #5592
Conversation
d0eb624
to
e5ed605
Compare
test/e2e/flowaggregator_test.go
Outdated
func testHelper(t *testing.T, data *TestData, podAIPs, podBIPs, podCIPs, podDIPs, podEIPs *PodIPs, isIPv6 bool) { | ||
svcB, svcC, err := createPerftestServices(data, isIPv6) | ||
func testHelper(t *testing.T, data *TestData, isIPv6 bool) { | ||
svcB, svcC, svcD, svcE, err := createPerftestServices(data, isIPv6) |
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.
should we also move createPerftestServices
to TestFlowAggregator
, and maybe simplify the code by creating one Service for each Pod indiscriminately?
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 it should be kept in testHelper as the services need to be created based on the isIPv6 condition?
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 doesn't have to be done in this PR necessarily, but it would be better to create the Services once, in TestFlowAggregator
. The IPFamilies
field in the ServiceSpec
is a list, and based on what the cluster supports, it can be set to [IPv4]
, [IPv4, IPv6]
, or [IPv6]
. Then the test can access the service using the correct IP. What do you think?
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 should be a good idea as IPFamilies
is a list. Since CreateServiceWithAnnotations
is used by many functions, I think it might be better to open another PR to make this change.
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 opened another PR to change the type of IPFamily
. #5690
if dstSvcAddress != nil { | ||
denyConn.DestinationServiceAddress = *dstSvcAddress | ||
} | ||
if dstSvcPort != 0 { | ||
denyConn.DestinationServicePort = dstSvcPort | ||
} |
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 don't get this. How do we know this is ServiceAddress and ServicePort? Every tracked connection can have these two fields.
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 these represent the Original Direction IPv4 Destination Address
and Original Direction Transport Layer Source Port
. If they do not belong to a service (by checking the CTMark), we won't fill the service information.
I think we do the same strategy for the connection in the conntrack table?
DestinationServiceAddress: conn.TupleOrig.IP.DestinationAddress, |
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.
Then this is perhaps a naming thing, is there any reason why we don't call them OriginalDestinationAddress and OriginalDestinationPort? They mean the ServiceAddress and ServicePort when this is a Service connection.
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 don't know the reason why we name it DestinationServiceAddress/DestinationServicePort. I agree with you that it should be filled only when it is a service connection. I think we can either rename it or fill this field only when CTMark belongs to a service. Changes for the latter one will be easier as we don't need to rename these two fields for all the packages. What do you think?
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.
@yuntanghsu I don't think you addressed this? Did you discuss it with Quan offline or did you plan to have a follow up PR?
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.
No, I don't. I was waiting for @tnqn 's opinion. If we need to rename it, I think it's better to open another PR to address this issue. Otherwise, we can fill these two fields only when the CTMark belongs to a service in this PR.
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, I thought you were going to update the PR to go with the second option (omit fields if we cannot identify the packet as belonging to Service traffic). That's assuming that we don't use these fields for anything else.
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.
Actually, I found that we won't see the values for these two fields in the FlowAggregator or the ClickHouse if it is a non-service traffic. We reset these two fields before we export the records if DestinationServicePortName
is an empty string (non-service connection or we can't find the service info).
case "destinationClusterIPv4": |
case "destinationServicePort": |
I think the current logic is:
- We temporarily fill
OriginalDestinationAddress
andOriginalDestinationPort
toDestinationServiceAddress
andDestinationServicePort
- We use these two values to check if we can find service info. If we find the info, we fill
DestinationServicePortName
- If
DestinationServicePortName
is an empty string, we resetDestinationServiceAddress
andDestinationServicePort
because it is a non-service traffic or we can't find the service information.
Do you think we still need to omit it when we do Poll() or when we have packet from PacketIn?
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 talked to Yun-Tang offline. We think it makes more sense to rename the fields to OriginalDestinationAddress
and OriginalDestinationPort
. He will do it in a follow-up PR.
Signed-off-by: Yun-Tang Hsu <hsuy@vmware.com>
Signed-off-by: Yun-Tang Hsu <hsuy@vmware.com>
Signed-off-by: Yun-Tang Hsu <hsuy@vmware.com>
Signed-off-by: Yun-Tang Hsu <hsuy@vmware.com>
Signed-off-by: Yun-Tang Hsu <hsuy@vmware.com>
5baf97d
to
1d0fd4e
Compare
16b2b53
to
10c0cf5
Compare
Signed-off-by: Yun-Tang Hsu <hsuy@vmware.com>
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 |
…onnection store. (antrea-io#5592) 1. Remove redundant logs in fillPodInfo/fillServiceInfo to avoid Flow Exporter from flooding logs. 2. Add Mark field for deny connections. We were missing this field previously, resulting in trying to fill service information for non-service IPs. 3. Update the DestinationServiceAddress for deny connections when we can find this information in PacketIn. 4. Update e2e/unit tests to verify that the Service-relateds field are filled correctly. Fixes antrea-io#5573 Signed-off-by: Yun-Tang Hsu <hsuy@vmware.com>
…onnection store. (#5592) 1. Remove redundant logs in fillPodInfo/fillServiceInfo to avoid Flow Exporter from flooding logs. 2. Add Mark field for deny connections. We were missing this field previously, resulting in trying to fill service information for non-service IPs. 3. Update the DestinationServiceAddress for deny connections when we can find this information in PacketIn. 4. Update e2e/unit tests to verify that the Service-relateds field are filled correctly. Fixes #5573 Signed-off-by: Yun-Tang Hsu <hsuy@vmware.com>
Exporter from flooding logs.
previously, resulting in trying to fill service information for
non-service IPs.
find this information in PacketIn.
filled correctly.
Fixes #5573