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

test: Fix the search for VIPs in cilium service list #15968

Merged
merged 1 commit into from May 4, 2021

Conversation

pchaigno
Copy link
Member

In several different service tests, we check that cilium is handling or not handling a service VIP. To that end, we search for the given VIP in the output of cilium service list. That output looks something like:

ID   Frontend              Service Type   Backend
1    10.87.240.1:443       ClusterIP      1 => 35.247.116.7:443
2    10.87.245.217:443     ClusterIP      1 => 10.84.1.94:443
3    10.87.240.10:53       ClusterIP      1 => 10.84.1.175:53
                                          2 => 10.84.1.28:53
4    10.87.241.252:80      ClusterIP      1 => 10.84.1.104:8080

Searching for the VIP directly in the output may return false positives however. For instance, searching for 10.87.241.25 in the above would match the last line when the IP doesn't actually match. Instead we should search for VIP:, that is, with anchors at the beginning and end of the VIP.

Fixes: #13752

@pchaigno pchaigno added kind/bug/CI This is a bug in the testing code. area/CI Continuous Integration testing issue or flake ci/flake This is a known failure that occurs in the tree. Please investigate me! needs-backport/1.9 labels Apr 30, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Apr 30, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.0-rc2 Apr 30, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.9.7 Apr 30, 2021
@pchaigno pchaigno added the release-note/ci This PR makes changes to the CI. label Apr 30, 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 30, 2021
In several different service tests, we check that cilium is handling or
not handling a service VIP. To that end, we search for the given VIP in
the output of 'cilium service list'. That output looks something like:

    ID   Frontend              Service Type   Backend
    1    10.87.240.1:443       ClusterIP      1 => 35.247.116.7:443
    2    10.87.245.217:443     ClusterIP      1 => 10.84.1.94:443
    3    10.87.240.10:53       ClusterIP      1 => 10.84.1.175:53
                                              2 => 10.84.1.28:53
    4    10.87.241.252:80      ClusterIP      1 => 10.84.1.104:8080

Searching for the VIP directly in the output may return false positives
however. For instance, searching for 10.87.241.25 in the above would match
the last line when the IP doesn't actually match. Instead we should
search for " VIP:", that is, with anchors at the beginning and end of
the VIP.

Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno force-pushed the fix-search-vip-in-cilium-svc-list branch from db2af87 to 0e112b8 Compare May 1, 2021 09:25
@pchaigno pchaigno marked this pull request as ready for review May 3, 2021 21:21
@pchaigno pchaigno requested a review from a team as a code owner May 3, 2021 21:21
@pchaigno pchaigno requested review from a team, christarazi and joamaki May 3, 2021 21:21
@pchaigno
Copy link
Member Author

pchaigno commented May 3, 2021

k8s-1.21-kernel-4.9 is failing with known flake #15097. Other tests are passing.

Copy link
Member

@aditighag aditighag left a comment

Choose a reason for hiding this comment

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

🚀

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.

🚀

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 4, 2021
@ti-mo ti-mo merged commit bb7fab8 into cilium:master May 4, 2021
@pchaigno pchaigno deleted the fix-search-vip-in-cilium-svc-list branch May 4, 2021 13:32
@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
@brb brb mentioned this pull request May 12, 2021
@joestringer joestringer added this to Needs backport from master in 1.9.8 May 13, 2021
@joestringer joestringer removed this from Needs backport from master in 1.9.7 May 13, 2021
@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
@aanm aanm moved this from Needs backport from master to Backport done to v1.9 in 1.9.8 May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI Continuous Integration testing issue or flake ci/flake This is a known failure that occurs in the tree. Please investigate me! kind/bug/CI This is a bug in the testing code. 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.9.8
Backport done to v1.9
Development

Successfully merging this pull request may close these issues.

CI, GKE: Connectivity Checks service.kubernetes.io/service-proxy-name label implementation
9 participants