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

test: Adds test for BPF NAT engine handles unknown protocol packets #15914

Merged
merged 1 commit into from May 11, 2021
Merged

test: Adds test for BPF NAT engine handles unknown protocol packets #15914

merged 1 commit into from May 11, 2021

Conversation

navarrothiago
Copy link
Contributor

@navarrothiago navarrothiago commented Apr 28, 2021 •
edited

Test Case:

  • Startup Cilium in k8s1 and k8s2
  • Create an iperf3 server DaemonSet
  • Start communication with SCTP packets between iperf3 client (k8s1) and iperf3 server (k8s2)
  • Check if the client received bytes

Fixes: #10541

Signed-off-by: Thiago Navarro navarro@accuknox.com

@navarrothiago navarrothiago requested a review from a team as a code owner April 28, 2021 15:59
@navarrothiago navarrothiago requested review from a team and pchaigno April 28, 2021 15:59
@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 Apr 28, 2021
@navarrothiago
Copy link
Contributor Author

navarrothiago commented Apr 28, 2021 •
edited

This PR depends on iperf3. I didnt used the netperf, because the version that is installed does have support of SCTP. Let me know you have any suggestion.

@aanm aanm added the release-note/misc This PR makes changes that have no direct user impact. label Apr 28, 2021
@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 Apr 28, 2021
@aanm aanm added release-note/ci This PR makes changes to the CI. and removed release-note/misc This PR makes changes that have no direct user impact. labels Apr 28, 2021
pchaigno
pchaigno requested changes Apr 28, 2021
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this @navarrothiago!

A couple changes required below. In particular, I think we'll want to move the server and client to pods. I believe we already have images for iperf3.

test/k8sT/Services.go Outdated Show resolved Hide resolved
test/k8sT/Services.go Outdated Show resolved Hide resolved
test/k8sT/Services.go Outdated Show resolved Hide resolved
@pchaigno pchaigno requested a review from brb April 28, 2021 17:58
joamaki
joamaki reviewed Apr 28, 2021
View reviewed changes
test/k8sT/Services.go Outdated Show resolved Hide resolved
test/k8sT/Services.go Outdated Show resolved Hide resolved
@navarrothiago
Copy link
Contributor Author

@pchaigno, @joamaki Thanks for your review! I have done what you suggested. Let me know if I am track with the comments.

pchaigno
pchaigno reviewed Apr 29, 2021
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Thanks!

test/k8sT/manifests/iperf3-deployment.yaml Outdated Show resolved Hide resolved
test/k8sT/manifests/iperf3-deployment.yaml Outdated Show resolved Hide resolved
@navarrothiago
Copy link
Contributor Author

Depend on cilium/misc-scripts#1 PR.

pchaigno
pchaigno requested changes May 4, 2021
View reviewed changes
test/k8sT/manifests/iperf3-deployment.yaml Outdated Show resolved Hide resolved
test/k8sT/manifests/iperf3-deployment.yaml Outdated Show resolved Hide resolved
test/k8sT/Services.go Outdated Show resolved Hide resolved
pchaigno
pchaigno requested changes May 7, 2021
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

It's fine. I review the full changes each time.

test/helpers/utils.go Outdated Show resolved Hide resolved
pchaigno
pchaigno approved these changes May 7, 2021
View reviewed changes
test/k8sT/Services.go Outdated Show resolved Hide resolved
@navarrothiago
test: Adds test for BPF NAT engine handles unknown protocol packets
90a4d18 
Test Case:

- Startup Cilium in k8s1 and k8s2
- Create an iperf3 server DaemonSet
- Start communication with SCTP packets between iperf3 client (k8s1) and iperf3 server (k8s2)
- Check if the client received bytes

Fixes: cilium#10541

Signed-off-by: Thiago Navarro <navarro@accuknox.com>
@pchaigno
Copy link
Member

pchaigno commented May 7, 2021

test-1.16-netnext

@pchaigno
Copy link
Member

pchaigno commented May 7, 2021

test-1.20-4.19

@navarrothiago
Copy link
Contributor Author

I believe the test that was failed [Fail] K8sPolicyTest Multi-node policy test with L7 policy [It] using connectivity-check to check datapath is not related to this PR.

@pchaigno
Copy link
Member

pchaigno commented May 7, 2021 •
edited

Could be related since it's a timeout. Let's see how frequent it is:

test-1.16-netnext

@pchaigno
Copy link
Member

@navarrothiago Why did you request a review from me? There are no changes since my last review.

@navarrothiago
Copy link
Contributor Author

navarrothiago commented May 10, 2021 •
edited

@pchaigno

@navarrothiago Why did you request a review from me? There are no changes since my last review.

I thought that because the jobs finished, I should request a review again. Sorry if this is not the workflow as expected by Cilium team.

@pchaigno
Copy link
Member

I thought that because the jobs finished, I should request a review again. Sorry if this is not the workflow as expected by Cilium team.

If you need to ping someone, best to use Slack or mention that person in the PR. Once a reviewer approved a PR, there shouldn't be any need to request a new review unless you made substantial changes. I'd recommend reading https://docs.cilium.io/en/latest/contributing/development/contributing_guide/#how-to-contribute in full.

@pchaigno
Copy link
Member

This new test only runs on our net-next and 4.19 CI jobs, which are passing. Reviews are also covered. Marking as a ready to merge.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 11, 2021
@jrajahalme jrajahalme merged commit dc318ee into cilium:master May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brb brb brb approved these changes

@joamaki joamaki joamaki approved these changes

@pchaigno pchaigno pchaigno approved these changes

Assignees
No one assigned
Labels
area/CI-improvement Topic or proposal to improve the Continuous Integration workflow ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Test whether BPF NAT engine is not dropping unknown protocol packets
6 participants
@navarrothiago @pchaigno @brb @joamaki @aanm @jrajahalme