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

connectivity test: Fix flow validation for nodeport service tests #2103

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

Conversation

jrajahalme
Copy link
Member

@jrajahalme jrajahalme commented Nov 10, 2023

Fix NodeportService.Address() IPv6 check and generalize NodeportService by using in []corev1.NodeAddress instead of *corev1.Node so that test cases can customize the addresses to be used. Use NodeportService to simplify special casing for nodeport tests.

Pass the IP family of the current node address to t.NewAction(), so that the address of the correct family is used in flow validation. This fixes the bug where IPv4 address is being used to validate an IPv6 flow like below:

[=] Test [no-policies-extra]
  [.] Action [no-policies-extra/pod-to-remote-nodeport/curl-0: cilium-test/client2-646b88fb9b-w5rck (10.244.1.113) -> cilium-test/echo-other-node (echo-other-node:8080)]
  πŸ“„ Following flows...
  πŸ“„ Validating flows for peer cilium-test/client2-646b88fb9b-w5rck
  ℹ️  SYN and(ip(src=10.244.1.113),or(tcp(dstPort=30171),tcp(dstPort=8080)),tcpflags(syn)) not found
  ❌ Aborting flow matching: context deadline exceeded
  ❌ Flow validation failed for peer cilium-test/client2-646b88fb9b-w5rck: 1 failures (first: -1, last: -1, matched: 0)

  πŸ“„ Flow logs for peer cilium-test/client2-646b88fb9b-w5rck:
  ❓ [0] Nov 10 06:46:22.847: cilium-test/client2-646b88fb9b-w5rck:50890 -> [fc00:c111::3]:30171 from-endpoint FORWARDED TRAFFIC_DIRECTION_UNKNOWN DROP_REASON_UNKNOWN (TCP Flags: SYN)
  ❓ [1] Nov 10 06:46:22.847: cilium-test/client2-646b88fb9b-w5rck:50890 -> [fc00:c111::3]:30171 to-stack FORWARDED EGRESS DROP_REASON_UNKNOWN (TCP Flags: SYN)
  ❓ [2] Nov 10 06:46:22.848: [fc00:c111::3]:30171 -> cilium-test/client2-646b88fb9b-w5rck:50890 from-host FORWARDED TRAFFIC_DIRECTION_UNKNOWN DROP_REASON_UNKNOWN (TCP Flags: SYN, ACK)
  ❓ [3] Nov 10 06:46:22.848: [fc00:c111::3]:30171 -> cilium-test/client2-646b88fb9b-w5rck:50890 to-endpoint FORWARDED EGRESS DROP_REASON_UNKNOWN (TCP Flags: SYN, ACK)

@jrajahalme jrajahalme requested review from a team as code owners November 10, 2023 09:24
@jrajahalme jrajahalme marked this pull request as draft November 10, 2023 09:24
@jrajahalme jrajahalme added the kind/bug Something isn't working label Nov 10, 2023
@jrajahalme jrajahalme marked this pull request as ready for review November 10, 2023 09:57
@jrajahalme jrajahalme requested a review from a team as a code owner November 10, 2023 09:57
net.Addr.To16 succeeds for both IPv4 and IPv6 addresses, so it alone can
not be used to test if an address is an IPv6 address.

Also, if any family is allowed, nodeportService.Address should still
return address of type corev1.NodeInternalIP.

Generalize ToNodeportService by taking []corev1.NodeAddress as an
argument, so that it can be used from more test cases.

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

Added back the use of AltDstPort on nodeport validation, but this time it is the containerPort that is the alt one.

Use TestPeer of type NodeportService to simplify special casing for
nodeport tests.

Pass the ip family of the current node address to t.NewAction, so that
the address of the correct family is used in flow validation. This fixes
the bug where IPv4 address is being used to validate an IPv6 flow like
below:

[=] Test [no-policies-extra]
  [.] Action [no-policies-extra/pod-to-remote-nodeport/curl-0: cilium-test/client2-646b88fb9b-w5rck (10.244.1.113) -> cilium-test/echo-other-node (echo-other-node:8080)]
  πŸ“„ Following flows...
  πŸ“„ Validating flows for peer cilium-test/client2-646b88fb9b-w5rck
  ℹ️  SYN and(ip(src=10.244.1.113),or(tcp(dstPort=30171),tcp(dstPort=8080)),tcpflags(syn)) not found
  ❌ Aborting flow matching: context deadline exceeded
  ❌ Flow validation failed for peer cilium-test/client2-646b88fb9b-w5rck: 1 failures (first: -1, last: -1, matched: 0)

  πŸ“„ Flow logs for peer cilium-test/client2-646b88fb9b-w5rck:
  ❓ [0] Nov 10 06:46:22.847: cilium-test/client2-646b88fb9b-w5rck:50890 -> [fc00:c111::3]:30171 from-endpoint FORWARDED TRAFFIC_DIRECTION_UNKNOWN DROP_REASON_UNKNOWN (TCP Flags: SYN)
  ❓ [1] Nov 10 06:46:22.847: cilium-test/client2-646b88fb9b-w5rck:50890 -> [fc00:c111::3]:30171 to-stack FORWARDED EGRESS DROP_REASON_UNKNOWN (TCP Flags: SYN)
  ❓ [2] Nov 10 06:46:22.848: [fc00:c111::3]:30171 -> cilium-test/client2-646b88fb9b-w5rck:50890 from-host FORWARDED TRAFFIC_DIRECTION_UNKNOWN DROP_REASON_UNKNOWN (TCP Flags: SYN, ACK)
  ❓ [3] Nov 10 06:46:22.848: [fc00:c111::3]:30171 -> cilium-test/client2-646b88fb9b-w5rck:50890 to-endpoint FORWARDED EGRESS DROP_REASON_UNKNOWN (TCP Flags: SYN, ACK)

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants