NetworkPolicy: Provider-scoped policies for multi-network pods#6223
NetworkPolicy: Provider-scoped policies for multi-network pods#6223SkalaNetworks merged 1 commit intokubeovn:masterfrom
Conversation
Summary of ChangesHello @akbarkn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the flexibility of Kubernetes NetworkPolicies in multi-network environments. By introducing a new annotation, Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces provider-scoped network policies, allowing NetworkPolicy resources to be applied to specific provider networks for multi-homed pods. This is achieved through a new annotation, ovn.kubernetes.io/policy-for. The implementation includes parsing this annotation, filtering selected ports and peers based on the specified providers, and conditionally including Service ClusterIPs. The changes are well-supported by new unit tests for the annotation parsing logic and comprehensive e2e tests that validate both the provider scoping and the ClusterIP gating behavior. Overall, the changes are solid and well-tested. I have one suggestion to improve the efficiency of the annotation parsing logic.
Pull Request Test Coverage Report for Build 21872834558Details
💛 - Coveralls |
77f021a to
9bcdb92
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces provider-scoped network policies for multi-network pods, a valuable feature. The implementation is clean and follows existing patterns in the controller. The new annotation ovn.kubernetes.io/policy-for is parsed robustly, and the logic to filter ports and addresses based on the specified providers is correctly integrated. The addition of unit tests for the parsing logic and e2e tests for the overall feature is excellent and ensures the correctness and reliability of the changes. The code is of high quality, and I have no specific comments for improvement.
|
@akbarkn Thanks for the great feature I made a few comments. Just out of curiosity, if I create a netpol that forbids all traffic for a specific provider, will this also work? |
Yes. A policy with |
|
@SkalaNetworks Please review when you have time. Thank you. |
SkalaNetworks
left a comment
There was a problem hiding this comment.
apart from a few nits, this all seems good to me :)
|
@akbarkn Thanks, this looks good to me. If you can get some docs published on Kube-OVN userguide, it would be great! /lgtm @oilbeater this looks fine considering the initial proposal in issue #6205 |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for provider-scoped network policies in multi-network environments. The implementation is well-structured, adding a new annotation to control policy scope and updating the controller logic accordingly. The inclusion of both unit and e2e tests is commendable and ensures the new functionality is well-covered. I have a few suggestions to improve code style, compatibility, and test robustness.
@SkalaNetworks Thanks for reviewing. I will update the docs once this PR merged. |
06ea057 to
70c50d0
Compare
|
LGTM, only some lint issues here https://github.com/kubeovn/kube-ovn/actions/runs/21829170600/job/63069753460?pr=6223 |
|
@oilbeater am I authorized to merge PRs when they reach the dedicated state or do you want to add the extra check and handle that yourself? |
Of course, go ahead! |
- add ovn.kubernetes.io/policy-for parsing and provider filtering - gate Service ClusterIP inclusion to default VPC - add unit + e2e tests (skip if NAD CRD missing) Signed-off-by: akbarkn <akbarkusumanegaralth@gmail.com>
70c50d0 to
41ee629
Compare
|
@akbarkn thank you so much for this feature :) If the tests are a go, I'll merge this! |
|
@SkalaNetworks @oilbeater Thanks to you too 🚀 |
Pull Request
What type of this PR
Examples of user facing changes:
Describe your changes here
ovn.kubernetes.io/policy-forparsing and provider‑scoped NetworkPolicy application.parsePolicyFor.Which issue(s) this PR fixes
Fixes #6205