Skip to content

policy: make ToServices selectors work for in-cluster services too #34208

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
Merged
merged 1 commit into from
Oct 23, 2024
Merged

policy: make ToServices selectors work for in-cluster services too #34208

merged 1 commit into from
Oct 23, 2024

Conversation

chaunceyjiang
Copy link
Member

Fixes: #34021

policy: make ToServices selectors work for in-cluster services too

@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 Aug 6, 2024
@github-actions github-actions bot added the sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. label Aug 6, 2024
squeed
squeed reviewed Aug 12, 2024
View reviewed changes
@chaunceyjiang chaunceyjiang force-pushed the to_service branch 3 times, most recently from 8c0af93 to fb2a3c0 Compare August 30, 2024 13:31
@chaunceyjiang chaunceyjiang marked this pull request as ready for review August 30, 2024 13:44
@chaunceyjiang chaunceyjiang requested review from a team as code owners August 30, 2024 13:44
@chaunceyjiang
Copy link
Member Author

/test

@squeed
Copy link
Contributor

squeed commented Aug 31, 2024

This needs a unit test :)

@chaunceyjiang chaunceyjiang requested a review from a team as a code owner August 31, 2024 16:33
@chaunceyjiang
Copy link
Member Author

/test

@chaunceyjiang chaunceyjiang force-pushed the to_service branch 2 times, most recently from 3f32f31 to c17b0be Compare September 2, 2024 03:21
@chaunceyjiang
Copy link
Member Author

/test

@chaunceyjiang
Copy link
Member Author

@squeed PTAL.

learnitall
learnitall approved these changes Sep 5, 2024
View reviewed changes
Copy link
Contributor

@learnitall learnitall left a comment

Choose a reason for hiding this comment

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

API changes LGTM, thanks. Do note though that my familiarity with this code is limited, I'd take my approval with a grain of salt.

doniacld
doniacld reviewed Sep 9, 2024
View reviewed changes
doniacld
doniacld approved these changes Sep 9, 2024
View reviewed changes
Copy link
Contributor

@doniacld doniacld left a comment

Choose a reason for hiding this comment

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

One minor comment otherwise LGTM!

odinuge
odinuge reviewed Sep 9, 2024
View reviewed changes
Copy link
Member

@odinuge odinuge left a comment

Choose a reason for hiding this comment

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

Some question about the intention here, and the thoughts around namespaces. Maybe I'm missing some context, but I think its useful to square them out.

I also think its worth mentioning how this is supposed to work with headless services where clusterIP: None, and add a test showing that the implementation works the same as the intention. There are a lot of components at play here, so its a bit tricky to test this properly without having an integration/e2e test imo.

Thanks for working on this! I'm really excited about being able to use this!

@odinuge
Copy link
Member

odinuge commented Sep 9, 2024

I think its also important to note in docs (and potentially changelog) that this requires the service selector to use only labels that are identity-Relevant Labels in order to match the endpoints, and that its not sufficient that the IPs are added to the endpoint or endpointslices objects for the service.

chaunceyjiang
chaunceyjiang commented Sep 9, 2024
View reviewed changes
@chaunceyjiang
Copy link
Member Author

@squeed PTAL.

@squeed squeed added the dont-merge/blocked Another PR must be merged before this one. label Oct 9, 2024
@squeed
Copy link
Contributor

squeed commented Oct 9, 2024

This looks good to me!

I would like to get #35293 merged first, as it needs to be backported and there will be a merge conflict. But then this can go in :-).

@squeed
Copy link
Contributor

squeed commented Oct 14, 2024

Getting there. The blocking PR is still hung on reviews, then this can go in :-).

@chaunceyjiang chaunceyjiang force-pushed the to_service branch 3 times, most recently from e187a04 to 57c60c1 Compare October 23, 2024 04:47
@chaunceyjiang
policy: make ToServices selectors work for in-cluster services too
360feb3 
Fixes: cilium#34021

Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
@chaunceyjiang
Copy link
Member Author

/test

@squeed squeed removed the dont-merge/blocked Another PR must be merged before this one. label Oct 23, 2024
@squeed
Copy link
Contributor

squeed commented Oct 23, 2024

Removed blocking label.

@squeed
Copy link
Contributor

squeed commented Oct 23, 2024

@chaunceyjiang nice work :). Once this lands, we also need to update the docs.

@squeed squeed enabled auto-merge October 23, 2024 06:57
@chaunceyjiang
Copy link
Member Author

Once this lands, we also need to update the docs.

OK.

@chaunceyjiang
Copy link
Member Author

/ci-ipsec-e2e

@squeed squeed added this pull request to the merge queue Oct 23, 2024
@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 Oct 23, 2024
Merged via the queue into cilium:main with commit 0b0c38f Oct 23, 2024
63 checks passed
@chaunceyjiang chaunceyjiang deleted the to_service branch October 23, 2024 08:13
@chaunceyjiang
Copy link
Member Author

@squeed Hi, is the fromServices block mentioned in the issue #34021 still needed to be pushed forward? If needed, I will complete it #34627 after finishing the documentation for this PR, .

@chaunceyjiang chaunceyjiang mentioned this pull request Oct 23, 2024
Merged
@chaunceyjiang chaunceyjiang restored the to_service branch October 31, 2024 04:14
@chaunceyjiang chaunceyjiang deleted the to_service branch October 31, 2024 04:15
@joestringer joestringer added release-note/major This PR introduces major new functionality to Cilium. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Nov 1, 2024
@joestringer
Copy link
Member

I've relabeled the release note category for this to "major" as I think this is a significant new change that users may be interested to find out about and start to use. I think it would be neat to feature this in the release announcement for the upcoming v1.17 release ETA early next year.

@joestringer joestringer mentioned this pull request Jul 17, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@squeed squeed squeed approved these changes

+3 more reviewers

@odinuge odinuge odinuge left review comments

@learnitall learnitall learnitall approved these changes

@doniacld doniacld doniacld approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/major This PR introduces major new functionality to Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enhancement: make ToServices selectors work for in-cluster services too / add FromServices
7 participants
@chaunceyjiang @squeed @odinuge @joestringer @learnitall @doniacld @michi-covalent