feat(BGP edge router) : BGP per VPC #5717
Conversation
adding logic to inject bgp sidecar to egress gateway
…is all, it cannot be set prefixes, and reduce listing gobgp-config crd's info
fix finalizer error in add handler change crd for gobgp config TODO set default policy check update handler run automatially after finishing add handler
dosubot
bot
added
size:XXL
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @donghee-park-rsk, 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 introduces a significant new feature that allows Kube-OVN to function as a BGP edge router. This enables seamless integration with external networks by allowing BGP peering on a per-VPC basis. The changes provide the necessary control plane and data plane components to advertise internal routes and receive external routes, enhancing Kube-OVN's capabilities in complex network environments.
Highlights
- New BGP Edge Router Capabilities: Introduces a new BGP edge router feature, enabling BGP peering per Virtual Private Cloud (VPC) to facilitate external network connectivity and route advertisement.
- New Custom Resource Definitions (CRDs): Three new CRDs have been added:
BgpEdgeRouterfor defining edge router instances,BgpEdgeRouterAdvertisementfor specifying routes to advertise, andGobgpConfigfor granular BGP policy configuration. - Enhanced BGP Speaker Functionality: The BGP speaker component now supports an 'edge router mode' and 'route server client' mode, along with improved logic for selecting the router ID based on external network interfaces.
- Automated BGP Route and Policy Management: New controller logic and helper scripts (
update-bgp-policy.sh,update-bgp-route.sh) are implemented to automatically manage BGP routes and policies on the edge router pods, ensuring dynamic network integration. - OVN Integration for Edge Routing: The controller now reconciles OVN resources such as port groups, address sets, BFD entries, and logical router policies to correctly steer traffic through the BGP edge router based on defined policies.
- Comprehensive E2E Testing: New end-to-end tests have been added to validate the functionality and stability of the BGP edge router feature, covering various scenarios including BFD and route advertisement.
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| 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 issue 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
-
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. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature, BGP per VPC, by adding BgpEdgeRouter, BgpEdgeRouterAdvertisement, and GobgpConfig CRDs, along with their corresponding controllers. The overall structure is well-designed. However, there are several areas for improvement. I've identified inconsistencies in CRD definitions across different files, which should be unified. Some shell scripts contain hardcoded values and lack IPv6 support, reducing flexibility. There are also potential bugs related to Go type definitions, code generation for deepcopy, and incomplete controller logic. Finally, several parts of the new controllers rely on parsing command-line output, which is brittle and should be replaced with structured data parsing (e.g., JSON) for better robustness.
| type Neighbors struct { | ||
| Address string `json:"address"` | ||
| ToAdvertise ToAdvertise `json:"toAdvertise"` | ||
| ToReceive ToReceive `json:"toReceive"` | ||
| } | ||
|
|
||
| type ToAdvertise struct { | ||
| Allowed Allowed `json:"allowed"` | ||
| } | ||
|
|
||
| type ToReceive struct { | ||
| Allowed Allowed `json:"allowed"` | ||
| } | ||
|
|
||
| type Allowed struct { | ||
| Mode string `json:"mode,omitempty"` | ||
| Prefixes []string `json:"prefixes,omitempty"` | ||
| } |
There was a problem hiding this comment.
The deepcopy functions for Neighbors and its nested structs (ToAdvertise, ToReceive, Allowed) are not being generated correctly, as indicated by the commented-out code in zz_generated.deepcopy.go. This will lead to runtime panics when deep-copying GobgpConfig objects. Please ensure the necessary // +k8s:deepcopy-gen=true tags are added to these struct definitions to enable proper code generation.
| - rule: "size(self.policies) != 0" | ||
| message: 'Each BGP Edge Router MUST have at least one policy' |
There was a problem hiding this comment.
The validation rule size(self.policies) != 0 is inconsistent with the one in dist/images/install.sh, which is size(self.policies) != 0 || size(self.selectors) != 0. The latter is more flexible as it allows using selectors as an alternative to policies. Additionally, the selectors field itself is missing from this CRD definition but present in dist/images/install.sh. Please unify the CRD definitions to ensure consistency and correctness across installation methods.
| // optional bgp image used by the workload | ||
| // if not specified, the default image passed in by kube-ovn-controller will be used | ||
| Image string `json:"image,omitempty"` | ||
| ASN uint32 `json:"asn"` |
There was a problem hiding this comment.
The ASN field in VpcEgressGatewayBGPConfig uses the JSON tag "asn". This is inconsistent with the CRD definition in charts/kube-ovn/templates/kube-ovn-crd.yaml (line 1206), which expects localAsn. This will cause deserialization issues. Please change the tag to json:"localAsn" for consistency.
| ASN uint32 `json:"asn"` | |
| ASN uint32 `json:"localAsn"` |
| bgpEdgeRouter: | ||
| type: string |
There was a problem hiding this comment.
The bgpEdgeRouter field is defined as a simple string. However, in dist/images/install.sh, the corresponding field is bgpEdgeRouterInfo, which is an object containing name and namespace. The object structure is more robust and expressive. Please consider unifying these definitions for consistency.
| validate_ip() { | ||
| local ip=$1 | ||
| if [[ ! $ip =~ ^([0-9]{1,3}\.){3}[0-9]{1,3}$ ]]; then | ||
| die "Invalid IPv4 address: $ip" | ||
| fi |
There was a problem hiding this comment.
The validate_ip function only validates IPv4 addresses. Since BGP neighbors can be IPv6, this function should be updated to validate both IPv4 and IPv6 addresses. This limitation also affects other parts of the script, such as set_default_action, which relies on this validation when parsing neighbor IPs.
| func (g *GobgpConfig) Subnet(subnets []string) []string { | ||
| if len(subnets) != 0 { | ||
| return subnets | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The Subnet function appears to be a copy-paste error from another struct. The GobgpConfig type does not have a subnet field in its spec, making this function irrelevant. It should be removed to improve code clarity.
| func (c *Controller) handlePodEventForBgpEdgeRouter(pod *corev1.Pod) error { | ||
| if !pod.DeletionTimestamp.IsZero() || pod.Annotations[util.AllocatedAnnotation] != "true" { | ||
| return nil | ||
| } | ||
| vpc := pod.Annotations[util.LogicalRouterAnnotation] | ||
| if vpc == "" { | ||
| return nil | ||
| } | ||
|
|
||
| router, err := c.bgpEdgeRouterLister.List(labels.Everything()) | ||
| if err != nil { | ||
| klog.Errorf("failed to list bgp edge router: %v", err) | ||
| utilruntime.HandleError(err) | ||
| return err | ||
| } | ||
|
|
||
| for _, ber := range router { | ||
| if ber.VPC(c.config.ClusterRouter) != vpc { | ||
| continue | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The handlePodEventForBgpEdgeRouter function appears to be incomplete. It correctly identifies the relevant BgpEdgeRouter based on the pod's VPC, but then it doesn't perform any action. This handler should likely enqueue the router for reconciliation to ensure its state is updated in response to pod events.
| foundRoutesSection := false | ||
|
|
||
| // Regex to match route lines starting with "*>" followed by CIDR | ||
| routeRegex := regexp.MustCompile(`^\*>\s+(\d+\.\d+\.\d+\.\d+/\d+)`) |
There was a problem hiding this comment.
The parseBgpAnnouncedRoutes function parses the plain text output of the gobgp command, which is brittle and can break if the output format changes. Furthermore, the current regex only supports IPv4. It is recommended to use the JSON output from gobgp (using the -j flag) and unmarshal it into structs for more robust and extensible parsing that can support both IPv4 and IPv6.
| return nil | ||
| } | ||
|
|
||
| func (c *Controller) validateSyncGobgpConfig(output string, gobgpConfig *kubeovnv1.GobgpConfig) (bool, error) { |
There was a problem hiding this comment.
The validateSyncGobgpConfig function relies on parsing the text output of gobgp commands using string matching. This approach is fragile and prone to breaking with future gobgp updates. For more robust validation, consider using the JSON output feature of gobgp (with the -j flag) and unmarshaling the result into Go structs.
| // Find the link by name, which is hardcoded to "net1" in this example. | ||
| // In a real-world scenario, you might want to make this configurable or discover it dynamically | ||
| // based on your network setup. | ||
| link, linkErr := netlink.LinkByName("net1") |
There was a problem hiding this comment.
The network interface name net1 is hardcoded. This makes the route syncer inflexible and may not work in all environments. This value should be made configurable, for example, through a command-line argument or an environment variable.
3 participants
Pull Request
What type of this PR
Examples of user facing changes:
BGP edge router allowing bgp per VPC
https://github.com/kubeovn/docs/blob/4593ffe7ab9c0fdff963825343f4c9b65c7ade19/docs/vpc/bgp-edge-router.en.md
Which issue(s) this PR fixes
Feature Request #5470