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

check: make (Service)Address() return an IP address #1651

Merged
merged 1 commit into from
May 24, 2023
Merged

check: make (Service)Address() return an IP address #1651

merged 1 commit into from
May 24, 2023

Conversation

jibi
Copy link
Member

@jibi jibi commented May 23, 2023 •
edited

change the Address method to return an IP address when available (i.e. when ClusterIP is not empty, which happens in case of headless services) instead of just returning the service name.

towards: cilium/cilium#24151

@jibi jibi added the area/CI Continuous Integration testing issue or flake label May 23, 2023
@jibi jibi requested a review from tklauser May 23, 2023 06:55
@jibi jibi requested a review from a team as a code owner May 23, 2023 06:55
@jibi jibi temporarily deployed to ci May 23, 2023 06:55 — with GitHub Actions Inactive
jrajahalme
jrajahalme approved these changes May 23, 2023
View reviewed changes
Copy link
Member

@jrajahalme jrajahalme left a comment

Choose a reason for hiding this comment

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

LGTM

@jibi jibi marked this pull request as draft May 23, 2023 10:40
@jibi jibi force-pushed the pr/jibi/service-peer-address branch from bf2196f to 01210be Compare May 23, 2023 10:41
@jibi jibi temporarily deployed to ci May 23, 2023 10:41 — with GitHub Actions Inactive
@jibi jibi force-pushed the pr/jibi/service-peer-address branch from 01210be to f9aa4de Compare May 23, 2023 13:09
@jibi jibi temporarily deployed to ci May 23, 2023 13:09 — with GitHub Actions Inactive
@jibi jibi force-pushed the pr/jibi/service-peer-address branch from f9aa4de to f25d7bd Compare May 23, 2023 16:34
@jibi jibi temporarily deployed to ci May 23, 2023 16:35 — with GitHub Actions Inactive
@jibi jibi force-pushed the pr/jibi/service-peer-address branch from f25d7bd to 8c0bca1 Compare May 23, 2023 16:36
@jibi
check: make (Service)Address() return an IP address
ae9cee1 
change the Address method to return an IP address when available (i.e.
when ClusterIP is not empty, which happens in case of headless services)
instead of just returning the service name.

In case the IP family is set to any, this method will keep returning the
service name.

Signed-off-by: Gilberto Bertin <jibi@cilium.io>
@jibi jibi force-pushed the pr/jibi/service-peer-address branch from 8c0bca1 to ae9cee1 Compare May 23, 2023 21:12
@jibi jibi temporarily deployed to ci May 23, 2023 21:12 — with GitHub Actions Inactive
@jibi
Copy link
Member Author

jibi commented May 24, 2023

Always returning an IP (beside the clusterIP == "" case) causes the PodToService() and PodToHostPort() scenarios to fail, specifically the flow validation:

2023-05-23T13:17:40.1564008Z   ℹ️  SYN and(ip(src=10.244.0.252,dst=10.245.249.71),tcp(dstPort=8080),tcpflags(syn)) not found
2023-05-23T13:17:40.1569614Z   ❌ Aborting flow matching: context deadline exceeded
2023-05-23T13:17:40.1570235Z   ❌ Flow validation failed for peer test-namespace/client-64c655df64-mb6pd: 2 failures (first: -1, last: -1, matched: 0)

as it's not clear yet what's causing that, and I'm not able to reproduce this locally, for the meantime I switched the first condition to:

	if s.Service.Spec.ClusterIP == "" || family == IPFamilyAny {
		return s.Service.Name
	}

so that we keep the current behavior of returning the service name when the IP family is set to any

@jibi jibi marked this pull request as ready for review May 24, 2023 06:02
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 24, 2023
@tklauser tklauser merged commit 61491cb into main May 24, 2023
19 checks passed
@tklauser tklauser deleted the pr/jibi/service-peer-address branch May 24, 2023 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tklauser tklauser tklauser approved these changes

@jrajahalme jrajahalme jrajahalme approved these changes

Assignees
No one assigned
Labels
area/CI Continuous Integration testing issue or flake ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jibi @tklauser @jrajahalme