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.

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

[Bug Fix]Fix infinite rejection loop by adding ingress/egress bypass flows #2579

Merged
merged 1 commit into from Aug 16, 2021
Merged

[Bug Fix]Fix infinite rejection loop by adding ingress/egress bypass flows #2579

merged 1 commit into from Aug 16, 2021

Conversation

GraysonWu
Copy link
Contributor

@GraysonWu GraysonWu commented Aug 11, 2021 •
edited

This PR fixes issue #2548
This PR added some bypass flows in the ingress and egress table to make sure reject responses won't be enforced by network policies.

Why we need it
Previously, reject responses are controlled by network policies, which means that reject responses could also be rejected. It may cause an infinite loop of rejecting reject responses in the OVS pipeline.
Instead of Reject reject response, now we choose to Allow the reject responses.

Added Flows

table=45/50/60/85/90/100, priority=64990,ct_state=-new+rel,ip actions=resubmit(,61/101)
table=45/50/60/85/90/100, priority=64990,ip,reg0=0x2000000/0x7000000 actions=resubmit(,61/101)

Example
Suppose we have:

  • PodA on NodeA
  • PodB on NodeB
  • PolicyA: appliedTo: PodA, ingress: Reject from PodB
  • PolicyB: appliedTo: PodB, ingress: Reject from PodA, egress: Reject to PodA

When PodA tries to talk to PodB using UDP, the process will be:

  1. The packet will commit a conntrack on NodeA, then leave NodeA.
  2. NodeB receives this packet. Due to the ingress rule of PolicyB, a reject response is generated by the controller with reg loaded.
  3. Thanks to the second bypass flow above this reject response can bypass the egress rule of PolicyB, then leave NodeB
  4. NodeA receives this reject response, an ICMP Destination Host Prohibited, whose ct_state is -new+rel.
  5. Thanks to the first bypass flow above this reject response can bypass the ingress rule of PolicyA, then PodA will receive it.

If in the beginning, PodA uses TCP to talk to PodB, then in step 4 above a TCP RST whose ct_state is -new+est will be received. And the bypass flow for ct_state=-new+est has already existed, so we don't need to handle it in this PR.

@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2021 •
edited

Codecov Report

Merging #2579 (7f9d488) into main (fe12a1b) will increase coverage by 0.15%.
The diff coverage is 93.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2579      +/-   ##
==========================================
+ Coverage   60.23%   60.38%   +0.15%     
==========================================
  Files         282      282              
  Lines       22337    22403      +66     
==========================================
+ Hits        13454    13529      +75     
+ Misses       7459     7444      -15     
- Partials     1424     1430       +6
Flag Coverage Δ
e2e-tests ∅ <ø> (?)
kind-e2e-tests 47.34% <93.93%> (+0.16%) ⬆️
unit-tests 42.05% <0.00%> (-0.14%) ⬇️

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

Impacted Files Coverage Δ
pkg/agent/openflow/client.go 58.51% <0.00%> (-0.57%) ⬇️
pkg/agent/openflow/pipeline.go 74.86% <100.00%> (+1.22%) ⬆️
pkg/agent/util/ethtool/ethtool_linux.go 70.00% <0.00%> (-10.00%) ⬇️
pkg/agent/cniserver/pod_configuration.go 56.74% <0.00%> (+1.19%) ⬆️
pkg/ovs/openflow/ofctrl_builder.go 58.04% <0.00%> (+1.26%) ⬆️
...gent/controller/noderoute/node_route_controller.go 58.89% <0.00%> (+1.29%) ⬆️
pkg/monitor/controller.go 29.10% <0.00%> (+1.49%) ⬆️
pkg/controller/networkpolicy/status_controller.go 73.20% <0.00%> (+1.96%) ⬆️

tnqn
tnqn reviewed Aug 12, 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.

Code LGTM, but DCO and commit message is missing

@GraysonWu
Copy link
Contributor Author

Added.

@GraysonWu
Copy link
Contributor Author

/test-all

Dyanngg
Dyanngg previously approved these changes Aug 12, 2021
View reviewed changes
Copy link
Contributor

@Dyanngg Dyanngg left a comment •
edited

Choose a reason for hiding this comment

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

code LGTM, nit in the PR description: let's mention the TCP reject response will also hit ct_state -new+rel?

@GraysonWu
Copy link
Contributor Author

GraysonWu commented Aug 12, 2021 •
edited

code LGTM, nit in the PR description: let's mention the TCP reject response will also hit ct_state -new+rel?

@Dyanngg For TCP, it will hit the flow with ct_state=-new+est which we already have. Updated the description to reduce confusion.

antoninbas
antoninbas reviewed Aug 13, 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.

I don't understand why these flows are added by the establishedConnectionFlows function. A related connection is not an established connection, and the flow used to match reject traffic injected by the agent has nothing to do with established connections either?

@GraysonWu
Fix Reject loop issue
7f9d488 
Add two bypass flows in all egress tables(45,50,60) and ingress tables(85,90,100).
The bypass flow matching `ct_state=-new+rel` is used to bypass ICMP reply packets.
The bypass flow matching `reg0=0x2000000/0x7000000` is used to bypass reject responses generated by the controller.

Signed-off-by: wgrayson <wgrayson@vmware.com>
@GraysonWu
Copy link
Contributor Author

Good one. I separated them.

@antoninbas antoninbas requested a review from tnqn August 13, 2021 05:02
antoninbas
antoninbas reviewed Aug 13, 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, will wait for @tnqn to approve

@GraysonWu
Copy link
Contributor Author

/test-all

tnqn
tnqn approved these changes Aug 16, 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

@tnqn tnqn merged commit a9552a1 into antrea-io:main Aug 16, 2021
@tnqn
Copy link
Member

tnqn commented Aug 16, 2021

I edited the commit message when merging it. It would be better to wrap the message to ~76 characters per line in the original commit in the future.

@antoninbas antoninbas mentioned this pull request Aug 16, 2021
Merged
zyiou pushed a commit to zyiou/antrea that referenced this pull request Aug 17, 2021
@GraysonWu @zyiou
Fix Reject loop issue (antrea-io#2579)
1518e85 
Add two bypass flows in all egress tables(45,50,60) and ingress tables
(85,90,100):
- The bypass flow matching `ct_state=-new+rel` is used to bypass ICMP
  reply packets.
- The bypass flow matching `reg0=0x2000000/0x7000000` is used to bypass
  reject responses generated by the controller.

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

@antoninbas antoninbas antoninbas left review comments

@tnqn tnqn tnqn approved these changes

@Dyanngg Dyanngg Dyanngg left review comments

@wenyingd wenyingd Awaiting requested review from wenyingd

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@GraysonWu @codecov-commenter @tnqn @antoninbas @Dyanngg