feat(controller): add externalIDs support for VPC policyRoutes#6227
feat(controller): add externalIDs support for VPC policyRoutes#6227jimyag wants to merge 4 commits intokubeovn:masterfrom
Conversation
Summary of ChangesHello @jimyag, 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 enhances VPC policy routes by introducing support for custom externalIDs. This feature allows users to associate arbitrary key-value metadata with their policy routes, facilitating better tracking and management of network policies. A key aspect of this implementation is the reservation and protection of the "vendor" key within externalIDs, which is automatically set to "kube-ovn" to maintain system integrity and prevent user interference. The changes involve updating the CRD, modifying controller logic to correctly detect and apply externalIDs changes, and ensuring deep copy functionality for the new field. 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 adds support for externalIDs to VPC policy routes, allowing for better tracking and ownership of routes. The changes are well-implemented, including updates to the CRD, controller logic, and the addition of comprehensive unit tests. I've identified a couple of areas with code duplication that could be refactored to improve maintainability. Overall, this is a solid feature addition.
f2ff9ef to
6af3411
Compare
|
/gemini review |
Pull Request Test Coverage Report for Build 21583491910Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
There was a problem hiding this comment.
Code Review
This pull request introduces support for externalIDs on VPC policy routes, a valuable feature for tracking and metadata. The changes are well-structured across the CRDs, API types, and controller logic. The inclusion of comprehensive unit tests is commendable. I've identified a critical issue in the policy diffing logic that could cause unnecessary resource churn, and a minor optimization opportunity. Additionally, the PR includes a crucial fix for a deepcopy issue, which is a great improvement.
Add support for custom external_ids in VPC policyRoutes to allow users to track the source and ownership of policy routes. The "vendor" key is reserved and always set to "kube-ovn" to prevent user override. Changes include: - Add ExternalIDs field to PolicyRoute struct in CRD type definition - Update diffPolicyRouteWithExisted and diffPolicyRouteWithLogical to return (dels, adds, updates) for detecting externalIDs changes - Ensure vendor key cannot be overwritten by user-specified externalIDs - Update CRD YAML schema in both helm charts and install.sh - Add unit tests for externalIDs diff detection and vendor protection Fixes kubeovn#6226 Signed-off-by: jimyag <[email protected]>
Combine policy route variable declarations into single lines to comply with gofumpt formatting requirements. Signed-off-by: jimyag <[email protected]>
Add missing externalIDs field to PolicyRoute in charts/kube-ovn/templates/kube-ovn-crd.yaml for consistency with kube-ovn-v2 chart and install.sh. Signed-off-by: jimyag <[email protected]>
Fix the conversion from ovnnb.LogicalRouterPolicy to kubeovnv1.PolicyRoute by populating the NextHopIP field from Nexthops. Without this fix, getPolicyRouteItemKey generates incorrect keys for existing policies with next-hop IPs, causing unnecessary deletions and additions on every reconciliation. Signed-off-by: jimyag <[email protected]>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for externalIDs in VPC policy routes, allowing for custom metadata to be associated with them. The changes are well-implemented across the CRD definitions, Go type definitions, and the controller logic. The new functionality is also accompanied by a comprehensive set of unit tests, which is great. I have one suggestion to improve compatibility with older Go versions.
|
I'm interested in what your specific usecase is for this feature |
see #6226 |

Add support for custom external_ids in VPC policyRoutes to allow users to track the source and ownership of policy routes. The "vendor" key is reserved and always set to "kube-ovn" to prevent user override.
Changes
ExternalIDsfield toPolicyRoutestruct in CRD type definitiondiffPolicyRouteWithExistedanddiffPolicyRouteWithLogicalto detect externalIDs changes and trigger upsertcharts/kube-ovn-v2/crds/kube-ovn-crd.yamlcharts/kube-ovn/templates/kube-ovn-crd.yamldist/images/install.shUsage Example
Fixes #6226
Pull Request
What type of this PR
Which issue(s) this PR fixes
Fixes #6226