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

Add FQDN TCP DNS support #4612

Merged
merged 3 commits into from
Mar 17, 2023
Merged

Add FQDN TCP DNS support #4612

merged 3 commits into from
Mar 17, 2023

Conversation

GraysonWu
Copy link
Contributor

@GraysonWu GraysonWu commented Feb 7, 2023

Fixes #4225

  1. Use tp_src=53,tcp_flags=+psh+ack to match the TCP DNS response,
    which can skip the handshake packets and match the packet containing
    the actual data.

  2. To achieve 1., an additional OVS fix patch should be applied.

  3. While parsing TCP DNS response, we need to trim the TCP option
    part to retrieve the data. And while sending it out via packetOut we
    should construct the exact packet received by the Antrea agent.

Signed-off-by: graysonwu wgrayson@vmware.com

@codecov
Copy link

codecov bot commented Feb 7, 2023

Codecov Report

Merging #4612 (f247791) into main (a63314f) will decrease coverage by 1.19%.
The diff coverage is 41.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4612      +/-   ##
==========================================
- Coverage   69.58%   68.39%   -1.19%     
==========================================
  Files         400      403       +3     
  Lines       59122    60199    +1077     
==========================================
+ Hits        41141    41175      +34     
- Misses      15175    16169     +994     
- Partials     2806     2855      +49     
Flag Coverage Δ *Carryforward flag
e2e-tests 38.32% <ø> (-0.02%) ⬇️ Carriedforward from 480f02e
integration-tests 34.34% <0.00%> (-0.14%) ⬇️
kind-e2e-tests 27.10% <10.18%> (-20.59%) ⬇️
unit-tests 59.84% <41.66%> (+0.22%) ⬆️

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
pkg/agent/controller/networkpolicy/fqdn.go 70.51% <0.00%> (-6.33%) ⬇️
pkg/agent/types/networkpolicy.go 94.59% <ø> (ø)
pkg/ovs/openflow/ofctrl_packetin.go 75.00% <54.54%> (-1.67%) ⬇️
pkg/agent/openflow/network_policy.go 80.92% <100.00%> (-0.37%) ⬇️
pkg/agent/openflow/pipeline.go 87.88% <100.00%> (-1.16%) ⬇️
pkg/ovs/openflow/ofctrl_builder.go 84.83% <100.00%> (+0.15%) ⬆️
...g/agent/controller/serviceexternalip/controller.go 35.83% <0.00%> (-43.35%) ⬇️
...nt/apiserver/handlers/serviceexternalip/handler.go 29.62% <0.00%> (-22.23%) ⬇️
pkg/agent/flowexporter/connections/conntrack.go 75.75% <0.00%> (-18.19%) ⬇️
pkg/agent/ipassigner/responder/arp_responder.go 55.29% <0.00%> (-17.65%) ⬇️
... and 66 more

@GraysonWu GraysonWu force-pushed the fqdn-tcp branch 5 times, most recently from dc613c8 to 480f02e Compare February 15, 2023 00:13
@tnqn tnqn mentioned this pull request Feb 24, 2023
@GraysonWu GraysonWu force-pushed the fqdn-tcp branch 6 times, most recently from dbf63b3 to 7daa730 Compare March 2, 2023 01:08
@GraysonWu GraysonWu changed the title [WIP] Add FQDN TCP DNS support Add FQDN TCP DNS support Mar 2, 2023
@vicky-liu vicky-liu added this to the Antrea v1.11 release milestone Mar 2, 2023
@GraysonWu GraysonWu force-pushed the fqdn-tcp branch 6 times, most recently from 23c8fd8 to 4e7b113 Compare March 7, 2023 03:08
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 overall

pkg/agent/openflow/network_policy.go Outdated Show resolved Hide resolved
pkg/agent/openflow/pipeline.go Outdated Show resolved Hide resolved
pkg/agent/openflow/network_policy.go Outdated Show resolved Hide resolved
pkg/agent/types/networkpolicy.go Outdated Show resolved Hide resolved
pkg/ovs/openflow/ofctrl_packetin.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
@GraysonWu GraysonWu force-pushed the fqdn-tcp branch 4 times, most recently from 5b59f03 to 782dec7 Compare March 7, 2023 23:29
@GraysonWu GraysonWu force-pushed the fqdn-tcp branch 2 times, most recently from 2db5e01 to de3a491 Compare March 9, 2023 07:10
pkg/agent/controller/networkpolicy/fqdn.go Outdated Show resolved Hide resolved
pkg/agent/openflow/network_policy.go Outdated Show resolved Hide resolved
pkg/agent/openflow/network_policy.go Show resolved Hide resolved
pkg/agent/openflow/network_policy.go Outdated Show resolved Hide resolved
pkg/agent/openflow/network_policy.go Outdated Show resolved Hide resolved
pkg/ovs/openflow/ofctrl_packetin.go Outdated Show resolved Hide resolved
pkg/ovs/openflow/ofctrl_packetin.go Outdated Show resolved Hide resolved
pkg/ovs/openflow/ofctrl_packetin.go Outdated Show resolved Hide resolved
pkg/ovs/openflow/ofctrl_packetin_test.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
tnqn
tnqn previously approved these changes Mar 15, 2023
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 added the action/release-note Indicates a PR that should be included in release notes. label Mar 15, 2023
@tnqn
Copy link
Member

tnqn commented Mar 15, 2023

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

@tnqn
Copy link
Member

tnqn commented Mar 15, 2023

@GraysonWu please resolve the conflict so we can merge

1. Use tp_src=53,tcp_flags=+psh+ack to match the TCP DNS response,
which can skip the handshake packets and match the packet containing
the actual data.

2. To achieve 1., additional OVS fix patch should be applied.

3. While paresing TCP DNS response, we need to trim the TCP option
part to retrieve the data. And while sending it out via packetOut we
should construct the exact packet received by the Antrea agent.

4. Using SendEthPacketOut to send the eth packet via packetOut save us
from retrieving all L2/3/4 info and make sure that we send out the
packet which is exactly the same as what we received.

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

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

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

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

@hjiajing
Copy link
Contributor

/test-multicluster-e2e

@GraysonWu
Copy link
Contributor Author

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

@GraysonWu
Copy link
Contributor Author

/test-ipv6-only-e2e

@GraysonWu
Copy link
Contributor Author

The failure in ipv6-only-e2e is not related. Tried to run it on main branch and also failed. Opened issue #4717 to track it.

@GraysonWu GraysonWu requested a review from tnqn March 16, 2023 23:43
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 88f524b into antrea-io:main Mar 17, 2023
@tnqn tnqn mentioned this pull request Mar 24, 2023
jainpulkit22 pushed a commit to urharshitha/antrea that referenced this pull request Apr 28, 2023
1. Use tp_src=53,tcp_flags=+psh+ack to match the TCP DNS response,
which can skip the handshake packets and match the packet containing
the actual data.

2. To achieve 1., additional OVS fix patch should be applied.

3. While paresing TCP DNS response, we need to trim the TCP option
part to retrieve the data. And while sending it out via packetOut we
should construct the exact packet received by the Antrea agent.

4. Using SendEthPacketOut to send the eth packet via packetOut save us
from retrieving all L2/3/4 info and make sure that we send out the
packet which is exactly the same as what we received.

Signed-off-by: graysonwu <wgrayson@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/release-note Indicates a PR that should be included in release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FQDN NetworkPolicy may not work if DNS over TCP is used
4 participants