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: Extend the clusterIP tests with policy #15928

Merged
merged 1 commit into from Apr 30, 2021
Merged

test: Extend the clusterIP tests with policy #15928

merged 1 commit into from Apr 30, 2021

Conversation

aditighag
Copy link
Member

@aditighag aditighag commented Apr 29, 2021 •
edited

Mainly for PR #15321 - tests the case where a pod connects to itself via service clusterIP when selected by a policy.

Signed-off-by: Aditi Ghag aditi@cilium.io

@aditighag aditighag requested a review from a team as a code owner April 29, 2021 03:32
@aditighag aditighag requested review from a team and nebril April 29, 2021 03:32
@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 29, 2021
@aditighag aditighag marked this pull request as draft April 29, 2021 03:32
@aditighag aditighag requested a review from joamaki April 29, 2021 03:32
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.0-rc2 Apr 29, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.9.7 Apr 29, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.10 Apr 29, 2021
@aditighag
Copy link
Member Author

I'm contemplating moving policy creation to JustBeforeEach to avoid affecting any future tests added in this block.

@aditighag
Copy link
Member Author

test-only --focus="K8sServicesTest Checks service accessing itself (hairpin flow)" --kernel_version=net-next

@aditighag
Copy link
Member Author

test-only --focus="K8sServicesTest.* Checks service accessing itself.*" --kernel_version=net-next

@aditighag
test: Extend the hairpin flow test with policy
799bc1a 
Test for PR cilium#15321 - tests the case where a pod
connects to itself via service clusterIP when selected
by a policy.

Signed-off-by: Aditi Ghag <aditi@cilium.io>
@aditighag aditighag force-pushed the pr/aditighag/svc-loopback-test branch from eb1e2cd to 799bc1a Compare April 29, 2021 04:49
@aditighag
Copy link
Member Author

test-only --focus="K8sServicesTest.* Checks service accessing itself.*" --kernel_version=net-next

@aditighag aditighag marked this pull request as ready for review April 29, 2021 06:30
@aditighag aditighag requested a review from pchaigno April 29, 2021 06:31
@joamaki
Copy link
Contributor

joamaki commented Apr 29, 2021

Since I was assigned, could you for educational reasons explain to me how the test works with the added policy and how it checks that #15321 is correctly implemented? It's hard for me to decipher this yet :)

@pchaigno
Copy link
Member

@joamaki See the commit description:

Test for PR #15321 - tests the case where a pod connects to itself via service clusterIP when selected by a policy.

So Aditi added a policy to the existing ClusterIP tests to ensure we cover the case fixed by #15321, where we'd get a wrong policy denied when a pod tries to access itself via ClusterIP VIP. One of those ClusterIP tests is Checks service accessing itself (hairpin flow) so it tests exactly the case fixed in #15321.

@aditighag
Copy link
Member Author

aditighag commented Apr 29, 2021 •
edited

Failures are not related to this PR -

k8s-1.21-kernel-4.9 (test-1.21-4.9)

The failed block is different from Checks ClusterIP Connectivity block that this PR extends.

Travis CI - Pull Request

Unit test failure in a wireguard unit test. I'll take a quick look, and file a PR.
Edit: #15937

ConformanceGKE (ci-gke)

This should be an independent test suite, and shouldn't be affected from Gingko test changes, no? /cc @pchaigno

@pchaigno
Copy link
Member

ConformanceGKE (ci-gke)

This should be an independent test suite, and shouldn't be affected from Gingko test changes, no? /cc @pchaigno

Yes, that's correct. I think this is 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 Apr 29, 2021
christarazi
christarazi approved these changes Apr 29, 2021
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

🚀

@christarazi christarazi removed their assignment Apr 29, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.9 in 1.9.7 Apr 29, 2021
@rolinh rolinh merged commit 69f10ed into cilium:master Apr 30, 2021
@christarazi christarazi mentioned this pull request May 3, 2021
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.10 May 4, 2021
@glibsm glibsm mentioned this pull request May 4, 2021
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.9 to Backport done to v1.9 in 1.9.7 May 5, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.10 May 6, 2021
@brb brb mentioned this pull request May 7, 2021
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.0-rc2 May 7, 2021
@joestringer joestringer mentioned this pull request May 13, 2021
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.0-rc2 May 13, 2021
@joestringer joestringer mentioned this pull request May 13, 2021
Merged
@aanm aanm mentioned this pull request May 17, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joamaki joamaki joamaki approved these changes

@pchaigno pchaigno pchaigno approved these changes

@christarazi christarazi christarazi approved these changes

@nebril nebril Awaiting requested review from nebril

Assignees

@nebril nebril

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
No open projects
1.10.0-rc2
Backport done to v1.10
1.8.10
Backport done to v1.8
1.9.7
Backport done to v1.9
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@aditighag @joamaki @pchaigno @christarazi @brb @glibsm @joestringer @rolinh @nebril @ti-mo