Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix K8s NP Service Logging #4780

Merged
merged 1 commit into from
Apr 24, 2023
Merged

Fix K8s NP Service Logging #4780

merged 1 commit into from
Apr 24, 2023

Conversation

qiyueyao
Copy link
Contributor

@qiyueyao qiyueyao commented Mar 29, 2023 •
edited
Loading

Service for K8s NetworkPolicy is not logging as expected.
This solution checks CnpDenyRegMark to differentiate Antrea native policies and K8s default drops before fetching conjunction ID.
And also renames Cnp to AP in CnpConjIDField and CnpDenyRegMark.

Fixes #4765

@qiyueyao
Copy link
Contributor Author

qiyueyao commented Mar 29, 2023 •
edited
Loading

Previously used separating CNPConjIDField and EndpointIPField.
There's also another solution, to not check the return value from GetPolicyInfoFromConjunction, assume when no policy is found for the provided conjunction ID, then this is K8s drop.
But I personally think this solution is not very reasonable, it omits error checking (the error message generated for this issue), and the provided conjunction ID for K8s drop action is actually an old IP address as described in the issue. Open for comments.
Final solution: CnpDenyRegMark.

@qiyueyao qiyueyao marked this pull request as draft March 30, 2023 00:39
@tnqn
Copy link
Member

tnqn commented Mar 30, 2023

There's also another solution, to not check the return value from GetPolicyInfoFromConjunction, assume when no policy is found for the provided conjunction ID, then this is K8s drop. But I personally think this solution is not very reasonable, it omits error checking (the error message generated for this issue), and the provided conjunction ID for K8s drop action is actually an old IP address as described in the issue. Open for comments.

I see there is a "CnpDenyRegMark" which is loaded for ACNP/ANP drop/deny, maybe we could check it to differentitate if it's a K8s or Antrea policy? The name of the variable needs to be more precise.

@qiyueyao qiyueyao force-pushed the log-k8s-service branch 2 times, most recently from a480bb0 to ae3d46e Compare March 31, 2023 06:20
@qiyueyao qiyueyao marked this pull request as ready for review March 31, 2023 06:23
tnqn
tnqn reviewed Apr 6, 2023
View reviewed changes
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

Code LGTM, is it possible to add a test for the scenario which didn't work previously?

@qiyueyao
Copy link
Contributor Author

qiyueyao commented Apr 6, 2023 •
edited
Loading

Code LGTM, is it possible to add a test for the scenario which didn't work previously?

Added TestAuditLoggingK8sService e2e test, and refactored the audit logging tests, UT has been modified to cover this.

@qiyueyao qiyueyao requested a review from tnqn April 10, 2023 22:56
tnqn
tnqn reviewed Apr 11, 2023
View reviewed changes
test/e2e/antreapolicy_test.go Outdated Show resolved Hide resolved
test/e2e/antreapolicy_test.go Outdated Show resolved Hide resolved
test/e2e/antreapolicy_test.go Outdated Show resolved Hide resolved
tnqn
tnqn reviewed Apr 13, 2023
View reviewed changes
test/e2e/antreapolicy_test.go Outdated Show resolved Hide resolved
test/e2e/antreapolicy_test.go Outdated Show resolved Hide resolved
test/e2e/antreapolicy_test.go Outdated Show resolved Hide resolved
@qiyueyao qiyueyao force-pushed the log-k8s-service branch 2 times, most recently from 228c607 to 3f9e22f Compare April 13, 2023 20:50
tnqn
tnqn reviewed Apr 14, 2023
View reviewed changes
test/e2e/antreapolicy_test.go Outdated Show resolved Hide resolved
tnqn
tnqn previously approved these changes Apr 17, 2023
View reviewed changes
@tnqn
Copy link
Member

tnqn commented Apr 17, 2023

/test-all
/test-ipv6-e2e
/test-ipv6-only-e2e

@tnqn
Copy link
Member

tnqn commented Apr 18, 2023

test-ipv6-e2e failed:

=== RUN   TestAntreaPolicy/TestGroupAuditLogging/Case=AuditLoggingK8sService
I0417 12:35:14.783013    2501 k8s_util.go:642] Creating/updating NetworkPolicy 'x-g8nadcqk/allow-xa-to-service'
I0417 12:35:14.891057    2501 util.go:44] Confirming ready status costs 100.226575ms
    antreapolicy_test.go:4006: Audit log does not contain expected entry from client (2001:db8:42:3::2:4f90) to server (2001:db8:42:3::2:4fa8)
    antreapolicy_test.go:4006: Audit log does not contain expected entry from client (2001:db8:42:2::3990) to server (2001:db8:42:3::2:4fa8)
    antreapolicy_test.go:4012: Missing entries in audit log: expected 4 but found 2
    antreapolicy_test.go:4006: Audit log does not contain expected entry from client (2001:db8:42:3::2:4f90) to server (2001:db8:42:3::2:4fa8)
    antreapolicy_test.go:4006: Audit log does not contain expected entry from client (2001:db8:42:2::3990) to server (2001:db8:42:3::2:4fa8)
    antreapolicy_test.go:4012: Missing entries in audit log: expected 4 but found 2
    antreapolicy_test.go:4006: Audit log does not contain expected entry from client (2001:db8:42:3::2:4f90) to server (2001:db8:42:3::2:4fa8)
    antreapolicy_test.go:4006: Audit log does not contain expected entry from client (2001:db8:42:2::3990) to server (2001:db8:42:3::2:4fa8)
    antreapolicy_test.go:4012: Missing entries in audit log: expected 4 but found 2
    antreapolicy_test.go:4006: Audit log does not contain expected entry from client (2001:db8:42:3::2:4f90) to server (2001:db8:42:3::2:4fa8)
    antreapolicy_test.go:4006: Audit log does not contain expected entry from client (2001:db8:42:2::3990) to server (2001:db8:42:3::2:4fa8)
    antreapolicy_test.go:4012: Missing entries in audit log: expected 4 but found 2
    antreapolicy_test.go:4006: Audit log does not contain expected entry from client (2001:db8:42:3::2:4f90) to server (2001:db8:42:3::2:4fa8)
    antreapolicy_test.go:4006: Audit log does not contain expected entry from client (2001:db8:42:2::3990) to server (2001:db8:42:3::2:4fa8)
    antreapolicy_test.go:4012: Missing entries in audit log: expected 4 but found 2
    antreapolicy_test.go:4006: Audit log does not contain expected entry from client (2001:db8:42:3::2:4f90) to server (2001:db8:42:3::2:4fa8)
    antreapolicy_test.go:4006: Audit log does not contain expected entry from client (2001:db8:42:2::3990) to server (2001:db8:42:3::2:4fa8)
    antreapolicy_test.go:4012: Missing entries in audit log: expected 4 but found 2
    antreapolicy_test.go:4006: Audit log does not contain expected entry from client (2001:db8:42:3::2:4f90) to server (2001:db8:42:3::2:4fa8)
    antreapolicy_test.go:4006: Audit log does not contain expected entry from client (2001:db8:42:2::3990) to server (2001:db8:42:3::2:4fa8)
    antreapolicy_test.go:4012: Missing entries in audit log: expected 4 but found 2
    antreapolicy_test.go:4006: Audit log does not contain expected entry from client (2001:db8:42:3::2:4f90) to server (2001:db8:42:3::2:4fa8)
    antreapolicy_test.go:4006: Audit log does not contain expected entry from client (2001:db8:42:2::3990) to server (2001:db8:42:3::2:4fa8)
    antreapolicy_test.go:4012: Missing entries in audit log: expected 4 but found 2
    antreapolicy_test.go:4006: Audit log does not contain expected entry from client (2001:db8:42:3::2:4f90) to server (2001:db8:42:3::2:4fa8)
    antreapolicy_test.go:4006: Audit log does not contain expected entry from client (2001:db8:42:2::3990) to server (2001:db8:42:3::2:4fa8)
    antreapolicy_test.go:4012: Missing entries in audit log: expected 4 but found 2
    antreapolicy_test.go:4006: Audit log does not contain expected entry from client (2001:db8:42:3::2:4f90) to server (2001:db8:42:3::2:4fa8)
    antreapolicy_test.go:4006: Audit log does not contain expected entry from client (2001:db8:42:2::3990) to server (2001:db8:42:3::2:4fa8)
    antreapolicy_test.go:4012: Missing entries in audit log: expected 4 but found 2
    antreapolicy_test.go:4006: Audit log does not contain expected entry from client (2001:db8:42:3::2:4f90) to server (2001:db8:42:3::2:4fa8)
    antreapolicy_test.go:4006: Audit log does not contain expected entry from client (2001:db8:42:2::3990) to server (2001:db8:42:3::2:4fa8)
    antreapolicy_test.go:4012: Missing entries in audit log: expected 4 but found 2
    antreapolicy_test.go:4017: Error when polling audit log files for required entries: timed out waiting for the condition
I0417 12:35:34.564887    2501 k8s_util.go:663] Deleting NetworkPolicy 'x-g8nadcqk/allow-xa-to-service'
I0417 12:35:34.571264    2501 k8s_util.go:570] Deleting Service nginx in ns x-g8nadcqk
    fixtures.go:472: Deleting Pod 'test-server-rfa96krc'

@qiyueyao
Fix K8s NP Service Logging
7178fe3 
Service for K8s NetworkPolicy is not logging as expected.
This solution checks the CnpDenyRegMark in the case of K8s
default drop before fetching the conjunction ID match.
And also renames Cnp to AP in CnpConjIDField and CnpDenyRegMark.

Fixes antrea-io#4765

Signed-off-by: Qiyue Yao <yaoq@vmware.com>
@qiyueyao
Copy link
Contributor Author

After some investigation, in dual stack case, wget service only generates one log item with the first available ipv4 address, unlike using connect in probe. There is no error, only one packetIn was sent, updated the e2e test.

@qiyueyao
Copy link
Contributor Author

qiyueyao commented Apr 20, 2023 •
edited
Loading

/test-ipv6-e2e

@qiyueyao
Copy link
Contributor Author

/test-all
/test-ipv6-e2e
/test-ipv6-only-e2e

@tnqn tnqn added the action/release-note Indicates a PR that should be included in release notes. label Apr 24, 2023
@tnqn tnqn merged commit cc8e7c7 into antrea-io:main Apr 24, 2023
@qiyueyao qiyueyao deleted the log-k8s-service branch April 25, 2023 00:00
jainpulkit22 pushed a commit to urharshitha/antrea that referenced this pull request Apr 28, 2023
@qiyueyao
Fix K8s NP Service Logging (antrea-io#4780)
a7fc38b 
Service for K8s NetworkPolicy is not logging as expected.
This solution checks the CnpDenyRegMark in the case of K8s
default drop before fetching the conjunction ID match.
And also renames Cnp to AP in CnpConjIDField and CnpDenyRegMark.

Fixes antrea-io#4765

Signed-off-by: Qiyue Yao <yaoq@vmware.com>
ceclinux pushed a commit to ceclinux/antrea that referenced this pull request Jun 5, 2023
@qiyueyao @ceclinux
Fix K8s NP Service Logging (antrea-io#4780)
920c738 
Service for K8s NetworkPolicy is not logging as expected.
This solution checks the CnpDenyRegMark in the case of K8s
default drop before fetching the conjunction ID match.
And also renames Cnp to AP in CnpConjIDField and CnpDenyRegMark.

Fixes antrea-io#4765

Signed-off-by: Qiyue Yao <yaoq@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnqn tnqn tnqn approved these changes

@Dyanngg Dyanngg Awaiting requested review from Dyanngg

@hongliangl hongliangl Awaiting requested review from hongliangl

@xliuxu xliuxu Awaiting requested review from xliuxu

Assignees
No one assigned
Labels
action/release-note Indicates a PR that should be included in release notes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

K8s Networkpolicy audit logging not working for Service access
2 participants
@qiyueyao @tnqn