vpc controller: only manage static routes with kube-ovn as vendor

[isabelleatkins]

Pull Request

• [ x ] Make sure you have followed Kube-OVN Code Style.

What type of this PR

This PR adds a small feature:

• it sets the external_ids.vendor on static routes (as it does on policy routes, and logical routers) to the CNIName (kube-ovn)
• the VPC reconciler then only tries to manage static routes whose vendor matches the CNI name
  • before, if a user directly configured a static route in OVN, the VPC reconciler would delete it, since it would see that it wasn't on the VPC defined static routes
  • now, it'll only "clean up" kube-ovn programmed OVN static routes, and leave any static routes whose vendor is not equal to the CNI Name

[gemini-code-assist]

Summary of Changes

Hello @isabelleatkins, I'm Gemini Code Assist¹! 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 the VPC controller's static route management by introducing vendor-specific filtering. The controller will now only reconcile static routes that are explicitly tagged as being managed by Kube-OVN, preventing accidental deletion or modification of static routes that may have been manually configured in OVN by users. This change improves the robustness and predictability of network configurations within Kube-OVN.

Highlights

• Selective Static Route Management: The VPC controller now only manages static routes in OVN that have the 'kube-ovn' vendor tag in their external_ids. This prevents the controller from interfering with static routes manually configured by users in OVN.
• Preventing Unintended Deletions: Previously, the VPC reconciler would delete any static route not explicitly defined in the VPC spec. With this change, it will only 'clean up' kube-ovn programmed OVN static routes, leaving other manually configured routes untouched.
• Vendor Tagging for Static Routes: New static routes added by the VPC controller will now automatically include the 'vendor: kube-ovn' external ID, aligning with how policy routes and logical routers are already tagged.
• New Test Cases: Comprehensive test cases have been added to validate the new static route management logic, covering scenarios like managing only kube-ovn routes, deleting orphaned routes, and handling empty VPC static route configurations.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces a valuable feature to prevent the VPC controller from managing static routes it doesn't own by using a 'vendor' tag. This is a great step towards better interoperability with manual OVN configurations.

However, my review found a critical issue in the implementation. While the code now filters for the vendor tag when listing existing routes, it fails to add this tag when creating new static routes within the main reconciliation loop. This will lead to an infinite loop of route additions. Additionally, the new tests are validating this incorrect behavior.

I've left detailed comments on the affected parts of the code with suggestions for fixes. Addressing these issues is crucial for the feature to work as intended.

[coveralls]

Pull Request Test Coverage Report for Build 18936482776

Details

• 4 of 8 (50.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.4%) to 21.906%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/controller/vpc.go	4	8	50.0%

Totals
Change from base Build 18930556811:	0.4%
Covered Lines:	11126
Relevant Lines:	50789

---

💛 - Coveralls

[isabelleatkins]

/gemini review

[gemini-code-assist]

Code Review

This pull request correctly modifies the VPC controller to only manage static routes with the kube-ovn vendor, preventing accidental deletion of user-defined routes. The changes are logical and well-implemented by filtering routes based on external_ids and adding the vendor ID to newly created routes. I've added a couple of comments: one suggesting a minor refactoring to reduce code duplication, and another pointing out a missing test case to fully validate the new behavior.

[gemini-code-assist]

The test suite is missing a key scenario to validate the main goal of this PR. You should add a test case to ensure that static routes without the kube-ovn vendor are ignored and not deleted by the controller.

This would involve:

1. Creating a static route in OVN with ExternalIDs that are either nil or do not contain the vendor: kube-ovn key-value pair.
2. Asserting that mockOvnClient.EXPECT().DeleteLogicalRouterStaticRoute(...) is not called for this route.

[isabelleatkins]

This is implicitly tested - if there was an additional call to DeleteLogicalRouterStaticRoute, the test would fail. In addition - we check that the ListLogicalRouterStaticRoute is called with the externalIDs filter.

[gemini-code-assist]

To improve maintainability and reduce code duplication, consider defining externalIDs as a package-level variable. This map is also defined in handleAddOrUpdateVpc (line 342) and in the new test file pkg/controller/vpc_test.go.

You could define it once at the top of the file and reuse it:

var kubeovnExternalIDs = map[string]string{"vendor": util.CniTypeName}

Then, you can use kubeovnExternalIDs in both addStaticRouteToVpc and handleAddOrUpdateVpc, as well as in the tests.

[isabelleatkins]

I've tested this locally by building and patching kube-ovn to use that image, and then:

1. Directly added to OVN two ECMP routes with the same prefix 1.2.3.0/24 that had vendor=
2. Added two kube-ovn subnet defined static src routes for prefixes 192.168.0.0/24 and 192.168.1.0/24 (therefore this new code set vendor=kube-ovn)
3. Directly added to OVN one ecmp-symmetric-reply route for prefix 1.2.0.0/16 that I manually set vendor=kube-ovn

I then kicked the kube-ovn-controller pod

I then saw the static route in step 3 deleted in OVN:

The picture shows two lr-route list commands - one before the kick, one after.

[oilbeater]

There's a conflict because the latest commit also changed controller_test.go.

And I think we need to figure out how to add this annotation to existing route rules when upgrading from an older version.