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

Add DNS over TCP tests #2135

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Add DNS over TCP tests #2135

wants to merge 5 commits into from
+178 −21

Conversation

jrajahalme
Copy link
Member

Add a new dns-tcp test to the suite that tests DNS proxy forwarding over TCP to external address used in the test suite. No payload protocol is used or tested, so this is a bare DNS test.

Plenty of other test cases cover DNS proxying for UDP, so this new test is specific to TCP.

Flow validation is extended to work on tests without payload protocol and for DNS validation on a UDP or TCP only, or both.

@jrajahalme jrajahalme requested review from a team as code owners November 29, 2023 20:18
@jrajahalme jrajahalme changed the title Addd DNS over TCP tests Add DNS over TCP tests Nov 29, 2023
@tklauser
Copy link
Member

@jrajahalme there is a legitimate failure in the Go static checks: https://github.com/cilium/cilium-cli/actions/runs/7038093191?pr=2135

@jrajahalme
tests: Add DNS-only test for DNS over TCP
8122619 
Add a new test that tests that the DNS over TCP to the external (WORLD)
target works through the DNS proxy with a DNS-only policy.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme
tests: Move L4Protocol TCP to a non-zero value
b387511 
Use the zero value of L4Protocol for NONE, which needed to make DNS
requirement protocol specific in a following commit.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme
tests: Make DNS requirement protocol specific
5481dde 
Add L4Protocol ANY for "any applicable" protocol and use it for DNS
requirement.

Replace DNSRequired with DNSProtocol, which can be UDP, TCP, or ANY for
either of them.

Make Dig+tcp test validate only TCP.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme
connectivity test: Add --numeric option
87cd110 
Add command line option '--numeric' to print hubble flows without IP
translation. This helps in matching flow requirements to flows when
debugging failing flow validation, as the flow requirements are printed
(and executed) with IP addresses without translation.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme
Copy link
Member Author

@jrajahalme there is a legitimate failure in the Go static checks: https://github.com/cilium/cilium-cli/actions/runs/7038093191?pr=2135

Thanks for noting, fixed.

@tklauser
Copy link
Member

tklauser commented Dec 1, 2023

@jrajahalme the newly introduced test seems to be failing in the external workloads CI workflow:

2023-12-01T01:37:10.182657701Z ❌ 1/48 tests failed (2/314 actions), 11 tests skipped, 0 scenarios skipped:
2023-12-01T01:37:10.182662275Z Test [dns-tcp]:
2023-12-01T01:37:10.182676662Z   ❌ dns-tcp/pod-to-world/dns-tcp-google.com: cilium-test/client-75bff5f5b9-mrhr5 (10.0.1.230) -> google.com-dns-tcp (google.com:0)
2023-12-01T01:37:10.182681730Z   ❌ dns-tcp/pod-to-world/dns-tcp-google.com: cilium-test/client2-88575dbb7-flmx9 (10.0.1.232) -> google.com-dns-tcp (google.com:0)

@jrajahalme
.github: Enable flow validation in external workloads test
fb5d71e 
Enable test flow validation in external workloads test by forwarding the
hubble port in external-workloads-install.sh.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme
Copy link
Member Author

Added hubble flow forwarding to external workload install to see what it going on with the test failure.

@tklauser
Copy link
Member

@jrajahalme any progress on this? Should we move this PR to draft while you're investigating or just exclude the DNS over TCP tests on external workloads?

@tklauser tklauser marked this pull request as draft February 9, 2024 09:08
@tklauser
Copy link
Member

tklauser commented Feb 9, 2024

@jrajahalme any progress on this? Should we move this PR to draft while you're investigating or just exclude the DNS over TCP tests on external workloads?

I‘ve moved this to draft for now to avoid this PR from showing up in reviewer‘s queues. Please feel free to move it out of draft once the external workloads failure is resolved.

@tklauser tklauser added the needs-rebase This PR needs to be rebased because it has merge conflicts. label Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@mhofstetter mhofstetter Awaiting requested review from mhofstetter mhofstetter is a code owner automatically assigned from cilium/cli

@aspsk aspsk Awaiting requested review from aspsk aspsk is a code owner automatically assigned from cilium/sig-lb

@tklauser tklauser Awaiting requested review from tklauser tklauser is a code owner automatically assigned from cilium/ci-structure

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
needs-rebase This PR needs to be rebased because it has merge conflicts.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jrajahalme @tklauser @nathanjsweet