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 network policy ingress policy stats #2078

Merged
merged 1 commit into from
Apr 22, 2021
Merged

Fix network policy ingress policy stats #2078

merged 1 commit into from
Apr 22, 2021

Conversation

ceclinux
Copy link
Contributor

@ceclinux ceclinux commented Apr 12, 2021 •
edited
Loading

This PR fixes #1976. The miscalculation is caused by L2ForwardingCalcTable with rule

Specifically, packets to the gateway port or the tunnel port will also go to ConntrackCommitTable and bypass the NetworkPolicy ingress rule tables, as NetworkPolicy ingress rules are not enforced for these packets on the source Node.

Therefore, these packets will not get counted in this case.

@ceclinux ceclinux force-pushed the fix_ingress_stats_miscalculation branch from 4ef6d95 to 4b4bfe1 Compare April 12, 2021 13:31
@codecov-io
Copy link

codecov-io commented Apr 12, 2021 •
edited
Loading

Codecov Report

Merging #2078 (8e756c3) into main (599d663) will decrease coverage by 5.98%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2078      +/-   ##
==========================================
- Coverage   61.05%   55.07%   -5.99%     
==========================================
  Files         270      270              
  Lines       20366    20368       +2     
==========================================
- Hits        12435    11218    -1217     
- Misses       6636     7960    +1324     
+ Partials     1295     1190     -105
Flag Coverage Δ
kind-e2e-tests 40.42% <100.00%> (-11.34%) ⬇️
unit-tests 41.39% <0.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/agent/openflow/client.go 51.06% <100.00%> (-8.78%) ⬇️
pkg/apis/controlplane/helper.go 25.00% <0.00%> (-75.00%) ⬇️
pkg/apis/controlplane/v1beta2/helper.go 25.00% <0.00%> (-75.00%) ⬇️
pkg/controller/networkpolicy/mutate.go 0.00% <0.00%> (-71.77%) ⬇️
pkg/controller/networkpolicy/store/group.go 5.00% <0.00%> (-70.00%) ⬇️
pkg/controller/networkpolicy/validate.go 0.00% <0.00%> (-69.47%) ⬇️
pkg/k8s/name.go 33.33% <0.00%> (-66.67%) ⬇️
pkg/agent/controller/networkpolicy/packetin.go 6.20% <0.00%> (-65.52%) ⬇️
...formers/externalversions/security/v1alpha1/tier.go 0.00% <0.00%> (-64.29%) ⬇️
...ers/externalversions/core/v1alpha2/clustergroup.go 0.00% <0.00%> (-64.29%) ⬇️
... and 67 more

@ceclinux ceclinux marked this pull request as ready for review April 13, 2021 03:43
@ceclinux ceclinux changed the title Fix ingress policy stats(WIP) Fix ingress policy stats Apr 13, 2021
@ceclinux ceclinux requested a review from tnqn April 13, 2021 10:27
@ceclinux ceclinux changed the title Fix ingress policy stats Fix network policy ingress policy stats Apr 14, 2021
tnqn
tnqn reviewed Apr 14, 2021
View reviewed changes
pkg/agent/openflow/client.go Outdated Show resolved Hide resolved
test/e2e/networkpolicy_test.go Outdated Show resolved Hide resolved
@ceclinux ceclinux force-pushed the fix_ingress_stats_miscalculation branch 3 times, most recently from 5e9e8f1 to ebb5a73 Compare April 19, 2021 13:12
@tnqn
Copy link
Member

tnqn commented Apr 19, 2021

And please remove the issue link from the title of the commit message.

@ceclinux ceclinux force-pushed the fix_ingress_stats_miscalculation branch 7 times, most recently from 5b22320 to c00b230 Compare April 19, 2021 17:42
@ceclinux
Copy link
Contributor Author

ceclinux commented Apr 20, 2021 •
edited
Loading

And please remove the issue link from the title of the commit message.

commit message updated

@ceclinux
Copy link
Contributor Author

It seems updated TestNetworkPolicyStats will still failed in noencap mode. I will take a look later

@tnqn
Copy link
Member

tnqn commented Apr 20, 2021

Could you also wrap the commit message body to ~76 characters per line?
https://github.com/golang/go/wiki/CommitMessage

@ceclinux ceclinux force-pushed the fix_ingress_stats_miscalculation branch from c00b230 to 5f49a81 Compare April 20, 2021 16:04
@ceclinux
Copy link
Contributor Author

@tnqn Thanks for review. It seems the current changes make all the tests pass. However, I will update the code and reply the comments after looking at different trafficEncapModes to double check if the change will affect these modes or not. Will at you after that.

@ceclinux ceclinux force-pushed the fix_ingress_stats_miscalculation branch 3 times, most recently from 055d38b to 8cc89b8 Compare April 21, 2021 11:24
@ceclinux
Copy link
Contributor Author

Could you also wrap the commit message body to ~76 characters per line?
https://github.com/golang/go/wiki/CommitMessage

commit message formatted

@ceclinux ceclinux force-pushed the fix_ingress_stats_miscalculation branch from 8cc89b8 to 3eccfd0 Compare April 21, 2021 11:26
@codecov-commenter
Copy link

codecov-commenter commented Apr 21, 2021 •
edited
Loading

Codecov Report

Merging #2078 (2b8f42a) into main (599d663) will increase coverage by 0.20%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2078      +/-   ##
==========================================
+ Coverage   61.05%   61.26%   +0.20%     
==========================================
  Files         270      269       -1     
  Lines       20366    20422      +56     
==========================================
+ Hits        12435    12511      +76     
+ Misses       6636     6623      -13     
+ Partials     1295     1288       -7
Flag Coverage Δ
kind-e2e-tests 52.06% <100.00%> (+0.30%) ⬆️
unit-tests 41.43% <66.66%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/agent/openflow/pipeline.go 70.52% <100.00%> (+0.50%) ⬆️
pkg/controller/networkpolicy/tier.go 47.50% <0.00%> (-5.00%) ⬇️
pkg/antctl/raw/traceflow/command.go 23.82% <0.00%> (-1.92%) ⬇️
...ntroller/networkpolicy/networkpolicy_controller.go 69.35% <0.00%> (-1.75%) ⬇️
...agent/controller/traceflow/traceflow_controller.go 72.95% <0.00%> (-0.33%) ⬇️
pkg/agent/flowexporter/connections/connections.go 76.05% <0.00%> (-0.17%) ⬇️
pkg/antctl/antctl.go 100.00% <0.00%> (ø)
pkg/ipfix/ipfix_process.go 100.00% <0.00%> (ø)
...ntroller/networkpolicy/networkpolicy_controller.go 84.56% <0.00%> (ø)
pkg/ipfix/ipfix_set.go
... and 13 more

@ceclinux ceclinux force-pushed the fix_ingress_stats_miscalculation branch from 3eccfd0 to e2e347a Compare April 21, 2021 14:15
@tnqn
Copy link
Member

tnqn commented Apr 21, 2021

/test-all

tnqn
tnqn previously approved these changes Apr 21, 2021
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.

LGTM, @jianjuns @antoninbas you may want to take a look too as it changes the default table of l2ForwardingCalcTable to fix the ingress traffic stats.

antoninbas
antoninbas previously approved these changes Apr 21, 2021
View reviewed changes
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

LGTM

@tnqn
Copy link
Member

tnqn commented Apr 22, 2021

/test-integration

@tnqn
Copy link
Member

tnqn commented Apr 22, 2021

@ceclinux could you check the integration failure: https://jenkins.antrea-ci.rocks/job/antrea-integration-manual-for-pull-request/8/console? It seems related to this change

@ceclinux
Copy link
Contributor Author

@ceclinux could you check the integration failure: https://jenkins.antrea-ci.rocks/job/antrea-integration-manual-for-pull-request/8/console? It seems related to this change

sure.

@ceclinux
Fix NetworkPolicy Ingress rule stats miscalculation
2b8f42a 
Packets to the tunnel or gateway port will go directly to
ConntrackCommitTable and bypass the IngressMetricsTable, which causing these
packets uncounted when applying by certain Networkpolicy with ingress rules.

Fixed by forwarding these packets to IngressMetricsTable.
@ceclinux ceclinux dismissed stale reviews from antoninbas and tnqn via 2b8f42a April 22, 2021 10:11
@ceclinux ceclinux force-pushed the fix_ingress_stats_miscalculation branch from e2e347a to 2b8f42a Compare April 22, 2021 10:11
@ceclinux
Copy link
Contributor Author

/test-integration

1 similar comment
@ceclinux
Copy link
Contributor Author

/test-integration

@ceclinux
Copy link
Contributor Author

updated and integration tests passed @tnqn

@tnqn
Copy link
Member

tnqn commented Apr 22, 2021

/test-all

@tnqn tnqn merged commit 3335e73 into antrea-io:main Apr 22, 2021
antoninbas pushed a commit to antoninbas/antrea that referenced this pull request Apr 29, 2021
@ceclinux @antoninbas
Fix NetworkPolicy Ingress rule stats miscalculation (antrea-io#2078)
5f0571e 
Packets to the tunnel or gateway port will go directly to
ConntrackCommitTable and bypass the IngressMetricsTable, which causing these
packets uncounted when applying by certain Networkpolicy with ingress rules.

Fixed by forwarding these packets to IngressMetricsTable.
antoninbas pushed a commit that referenced this pull request Apr 30, 2021
@ceclinux @antoninbas
Fix NetworkPolicy Ingress rule stats miscalculation (#2078)
03954bf 
Packets to the tunnel or gateway port will go directly to
ConntrackCommitTable and bypass the IngressMetricsTable, which causing these
packets uncounted when applying by certain Networkpolicy with ingress rules.

Fixed by forwarding these packets to IngressMetricsTable.
@antoninbas antoninbas mentioned this pull request May 21, 2021
Closed
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

@antoninbas antoninbas antoninbas left review comments

@jianjuns jianjuns Awaiting requested review from jianjuns

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Miscalculation of Packats for e2e test TestNetworkPolicy
6 participants
@ceclinux @codecov-io @tnqn @codecov-commenter @antoninbas @vmwclabot