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

DNS Proxy: Adds UDP checksum for IPv6 Responses #29493

Merged
merged 1 commit into from Dec 5, 2023

Conversation

danehans
Copy link
Contributor

Updates the DNS proxy to compute a UDP checksum over the IPv6 response packet and the pseudo-header. If the computation yields a result of zero, it changes the checksum to hex FFFF for placement in the UDP header.

Fixes #28678

The DNS proxy will now compute a UDP checksum over the IPv6 response packet and the pseudo-header.

@danehans danehans requested review from a team as code owners November 29, 2023 19:43
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 29, 2023
@danehans danehans added the area/fqdn Affects the FQDN policies feature label Nov 29, 2023
@danehans
Copy link
Contributor Author

@squeed @tklauser I'm new to this area of the Cilium code so I'm open to suggestions for computing the UDP checksum. I manually tested the changes but I can add unit tests if this approach is acceptable.

@danehans
Copy link
Contributor Author

danehans commented Nov 29, 2023

The Conformance K8s Kind / Installation and Conformance test is failing due to:

STEP: Checking if the Service forwards traffic to TCP only @ 11/29/23 21:30:24.379
  Nov 29 21:30:24.392: INFO: Running '/usr/local/bin/kubectl --server=https://127.0.0.1:35901/ --kubeconfig=/home/runner/work/cilium/cilium/_artifacts/kubeconfig.conf --namespace=services-7471 exec execpodmzmjh -- /bin/sh -x -c echo hostName | nc -v -t -w 2 10.96.129.16 80'
  Nov 29 21:30:24.685: INFO: stderr: "+ echo hostName\n+ nc -v -t -w 2 10.96.129.16 80\nConnection to 10.96.129.16 80 port [tcp/http] succeeded!\n"
  Nov 29 21:30:24.6[85](https://github.com/cilium/cilium/actions/runs/7038674807/job/19156089632?pr=29493#step:15:86): INFO: stdout: "HTTP/1.1 400 Bad Request\r\nContent-Type: text/plain; charset=utf-8\r\nConnection: close\r\n\r\n400 Bad Request"
  Nov 29 21:30:24.685: INFO: Running '/usr/local/bin/kubectl --server=https://127.0.0.1:35901/ --kubeconfig=/home/runner/work/cilium/cilium/_artifacts/kubeconfig.conf --namespace=services-7471 exec execpodmzmjh -- /bin/sh -x -c echo hostName | nc -v -u -w 2 10.96.129.16 80'
  Nov 29 21:30:29.093: INFO: stderr: "+ echo hostName\n+ nc -v -u -w 2 10.96.129.16 80\nConnection to 10.96.129.16 80 port [udp/*] succeeded!\n"
  Nov 29 21:30:29.093: INFO: stdout: "pod1"
  Nov 29 21:30:34.094: INFO: Running '/usr/local/bin/kubectl --server=https://127.0.0.1:35901/ --kubeconfig=/home/runner/work/cilium/cilium/_artifacts/kubeconfig.conf --namespace=services-7471 exec execpodmzmjh -- /bin/sh -x -c echo hostName | nc -v -u -w 2 10.96.129.16 80'
  Nov 29 21:30:38.321: INFO: stderr: "+ nc -v -u -w 2 10.96.129.16 80\n+ echo hostName\nConnection to 10.96.129.16 80 port [udp/*] succeeded!\n"
  Nov 29 21:30:38.321: INFO: stdout: "pod1"
  Nov 29 21:30:43.321: INFO: Running '/usr/local/bin/kubectl --server=https://127.0.0.1:35[90](https://github.com/cilium/cilium/actions/runs/7038674807/job/19156089632?pr=29493#step:15:91)1 --kubeconfig=/home/runner/work/cilium/cilium/_artifacts/kubeconfig.conf --namespace=services-7471 exec execpodmzmjh -- /bin/sh -x -c echo hostName | nc -v -u -w 2 10.96.129.16 80'
  Nov 29 21:30:47.762: INFO: stderr: "+ + nc -v -u -w 2 10.96.129.16 80\necho hostName\nConnection to 10.96.129.16 80 port [udp/*] succeeded!\n"
  Nov 29 21:30:47.762: INFO: stdout: "pod1"
  Nov 29 21:30:52.763: INFO: Running '/usr/local/bin/kubectl --server=https://127.0.0.1:35901/ --kubeconfig=/home/runner/work/cilium/cilium/_artifacts/kubeconfig.conf --namespace=services-7471 exec execpodmzmjh -- /bin/sh -x -c echo hostName | nc -v -u -w 2 10.[96](https://github.com/cilium/cilium/actions/runs/7038674807/job/19156089632?pr=29493#step:15:97).129.16 80'
  Nov 29 21:30:57.251: INFO: stderr: "+ nc -v -u -w 2 10.96.129.16 80\n+ echo hostName\nConnection to 10.96.129.16 80 port [udp/*] succeeded!\n"
  Nov 29 21:30:57.251: INFO: stdout: "pod1"
  [FAILED] in [It] - test/e2e/network/service.go:3730 @ 11/29/23 21:30:57.251

Similar to #29315 flake. Maybe the 30-second timeout is not long enough for the change in the service to take effect so I'm rerunning the tests.

@danehans
Copy link
Contributor Author

/test

@gandro gandro added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Nov 30, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 30, 2023
Copy link
Contributor

@squeed squeed left a comment

Choose a reason for hiding this comment

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

Great catch! Could you add a unit test for checksum calculation? Then we can get this merged.

Fixes cilium#28678

Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>
@danehans
Copy link
Contributor Author

danehans commented Dec 1, 2023

/test

@danehans danehans requested a review from squeed December 1, 2023 22:56
Copy link
Contributor

@squeed squeed left a comment

Choose a reason for hiding this comment

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

I manually added another test case with real-world addresses, it passed. Thanks!

Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! 💯

Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Dec 5, 2023
@ti-mo ti-mo added this pull request to the merge queue Dec 5, 2023
Merged via the queue into cilium:main with commit 3e44799 Dec 5, 2023
61 checks passed
@danehans danehans deleted the issue_28678 branch March 8, 2024 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/fqdn Affects the FQDN policies feature ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

L7 DNS policy does not work with IPv6 DNS server
7 participants