any vpc can use any external subnet#5268
Conversation
Signed-off-by: zbb88888 <jmdxjsjgcxy@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @zbb88888, 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!
Summary of Changes
Hello! I'm gemini-code-assist, here to provide a summary of this pull request. This PR, authored by zbb88888, aims to enhance the networking capabilities by allowing any Virtual Private Cloud (VPC) to utilize any external subnet, removing the previous limitation where typically only the default VPC could connect to the default external network. The changes involve significant refactoring within the VPC controller to manage external connectivity and BFD sessions for custom VPCs, as well as minor adjustments in the external gateway controller and API definitions.
Highlights
- Enhanced VPC External Connectivity: The core change allows any custom VPC to connect to external networks, not just the default VPC. This includes connecting to the default external subnet and/or additional external subnets specified in the VPC configuration.
- VPC Controller Refactoring: The logic for handling VPC external connections has been refactored into a dedicated function (
handleUpdateVpcExternal) within the VPC controller, centralizing the management of adding and removing external subnet connections based on the VPC spec and status. - Support for Multiple External Subnets: The controller now correctly handles the
ExtraExternalSubnetsfield, allowing a VPC to connect to multiple external subnets (primarily provider network VLAN subnets) in addition to the default external subnet. - BFD Integration for Custom VPCs: BFD session management, previously more tied to the default external gateway, is now integrated into the custom VPC external handling logic, enabling BFD for custom VPCs connected to the default external subnet gateway.
- Centralized Status Updates: VPC status updates related to external connectivity (
EnableExternal,ExtraExternalSubnets,EnableBfd) are now handled in a single, centralized function (updateVpcExternalStatus), improving consistency.
Changelog
Click here to see the changelog
- pkg/apis/kubeovn/v1/vpc.go
- Added clarifying comments to the
EnableExternalandExtraExternalSubnetsfields in theVpcSpecstruct to better explain their intended use.
- Added clarifying comments to the
- pkg/controller/external_gw.go
- Added a check in
resyncExternalGatewayto skip default VPC external gateway handling if the default VPC is configured withExtraExternalSubnets, deferring this case to the VPC controller.
- Added a check in
- pkg/controller/vpc.go
- Refactored the main VPC external connectivity logic from
handleAddOrUpdateVpcinto a new helper functionhandleUpdateVpcExternal(Diff 3). - Modified
handleAddOrUpdateVpcto callhandleUpdateVpcExternalwhen a VPC's external status needs reconciliation (Diff 3). - Updated
handleUpdateVpcExternalto manage connections for both the default external subnet andExtraExternalSubnetsfor any VPC based on spec and status differences (Diff 3). - Added a check to prevent using logical gateway external subnets for external access in the VPC controller's handling (Diff 1).
- Improved error handling when the external subnet gateway address is not available (Diff 2).
- Integrated BFD session creation and deletion logic for custom VPCs enabling BFD with external connectivity (Diff 3).
- Removed redundant VPC status updates from
handleAddVpcExternalSubnetandhandleDelVpcExternalSubnet(Diff 4, 6). - Renamed
updateVpcAddExternalStatustoupdateVpcExternalStatusand updated it to centrally manage theEnableExternal,ExtraExternalSubnets, andEnableBfdstatus fields based on the spec (Diff 7). - Changed the log level for deleting VPC LRPs from debug (V(3)) to info (Diff 5).
- Refactored the main VPC external connectivity logic from
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 is currently in preview and 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 to provide feedback.
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.
Routers connect with flair,
Sending packets through the air.
Subnets align,
Connections shine,
Networking beyond compare.
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 aims to enhance VPC external connectivity by allowing any VPC to use any external subnet, which is a valuable feature for flexibility. The changes primarily involve refactoring the external subnet handling logic within the VPC controller and adjusting the external gateway controller's behavior for the default VPC.
The refactoring in pkg/controller/vpc.go into the new handleUpdateVpcExternal function is a significant part of this effort. While the overall direction is good, there are a few potential issues related to the logic for connecting the default external subnet when extra subnets are also specified, and the logic for disconnecting extra subnets when external access is disabled. Additionally, some areas could benefit from further refactoring for clarity and maintainability.
Summary of Findings
- Default External Subnet Connection Logic: In
handleUpdateVpcExternal, ifEnableExternal: trueandExtraExternalSubnetsare also specified, the default external subnet might not be connected during initial enablement. This could be a functional bug ifEnableExternalis meant to always manage the default external subnet connection. (Severity: High) - Incorrect Disabling Logic for ExtraExternalSubnets: In
handleUpdateVpcExternal, whenEnableExternalis set to false, the logic for removingExtraExternalSubnetsincorrectly depends on the current spec. It should remove all active extra subnets listed in the status. (Severity: High) - Function Complexity: The function
handleUpdateVpcExternalinpkg/controller/vpc.gois quite long. Breaking it into smaller helper functions could improve maintainability. (Severity: Medium) - Clarity in Status Update Function: The function
updateVpcExternalStatusinpkg/controller/vpc.gomodifies a spec field (vpc.Spec.EnableExternal) which is confusing for a status update function. (Severity: Medium) - Parameter Typo: In
pkg/controller/vpc.go, the parameterdefualtExternalSubnetExistinhandleUpdateVpcExternalis misspelled. (Severity: Low, not commented due to review settings)
Merge Readiness
This pull request introduces significant changes to how VPC external subnets are handled. While the refactoring is a good step towards more flexible external connectivity, there are a couple of high-severity issues identified in pkg/controller/vpc.go concerning the logic for connecting the default external subnet and disconnecting extra subnets. These issues could lead to incorrect network configurations.
Additionally, there are medium-severity suggestions for improving code clarity and maintainability.
I recommend that the high-severity issues be addressed before merging this pull request. As an AI reviewer, I am not authorized to approve pull requests; please ensure further review and approval from other maintainers.
| if vpc.Spec.EnableExternal { | ||
| if !vpc.Status.EnableExternal { | ||
| // add external connection | ||
| if vpc.Spec.ExtraExternalSubnets == nil && defualtExternalSubnetExist { | ||
| // only connect default external subnet | ||
| klog.Infof("connect default external subnet %s with vpc %s", c.config.ExternalGatewaySwitch, vpc.Name) | ||
| if err := c.handleAddVpcExternalSubnet(vpc.Name, c.config.ExternalGatewaySwitch); err != nil { | ||
| klog.Errorf("failed to add external subnet %s connection for vpc %s, error %v", c.config.ExternalGatewaySwitch, vpc.Name, err) | ||
| return err | ||
| } | ||
| // TODO: support multi external nic | ||
| if custVpcEnableExternalMultiHopEcmp { | ||
| klog.Infof("remove normal static ecmp route for vpc %s", vpc.Name) | ||
| // auto remove normal type static route, if using ecmp based bfd | ||
| if err := c.reconcileCustomVpcDelNormalStaticRoute(vpc.Name); err != nil { | ||
| klog.Errorf("failed to reconcile del vpc %q normal static route", vpc.Name) | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| if cachedVpc.Spec.ExtraExternalSubnets != nil { | ||
| sort.Strings(vpc.Spec.ExtraExternalSubnets) | ||
| } | ||
| // add external subnets only in spec and delete external subnets only in status | ||
| if !slices.Equal(vpc.Spec.ExtraExternalSubnets, vpc.Status.ExtraExternalSubnets) { | ||
| for _, subnetStatus := range cachedVpc.Status.ExtraExternalSubnets { | ||
| if !slices.Contains(cachedVpc.Spec.ExtraExternalSubnets, subnetStatus) { | ||
| klog.Infof("delete external subnet %s connection for vpc %s", subnetStatus, vpc.Name) | ||
| if err := c.handleDelVpcExternalSubnet(vpc.Name, subnetStatus); err != nil { | ||
| klog.Errorf("failed to delete external subnet %s connection for vpc %s, error %v", subnetStatus, vpc.Name, err) | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| for _, subnetSpec := range cachedVpc.Spec.ExtraExternalSubnets { | ||
| if !slices.Contains(cachedVpc.Status.ExtraExternalSubnets, subnetSpec) { | ||
| klog.Infof("connect external subnet %s with vpc %s", subnetSpec, vpc.Name) | ||
| if err := c.handleAddVpcExternalSubnet(key, subnetSpec); err != nil { | ||
| klog.Errorf("failed to add external subnet %s connection for vpc %s, error %v", subnetSpec, key, err) | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| if err := c.updateVpcAddExternalStatus(key, true); err != nil { | ||
| klog.Errorf("failed to update additional external subnets status, %v", err) | ||
|
|
||
| // only connect provider network vlan external subnet | ||
| for _, subnet := range vpc.Spec.ExtraExternalSubnets { | ||
| klog.Infof("connect external subnet %s with vpc %s", subnet, vpc.Name) | ||
| if err := c.handleAddVpcExternalSubnet(vpc.Name, subnet); err != nil { | ||
| klog.Errorf("failed to add external subnet %s connection for vpc %s, error %v", subnet, vpc.Name, err) | ||
| return err | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There appears to be a potential issue with how the default external subnet is handled when vpc.Spec.EnableExternal is true, a default external subnet exists (defualtExternalSubnetExist), and vpc.Spec.ExtraExternalSubnets are also specified, particularly during the initial enablement (!vpc.Status.EnableExternal).
Currently:
- If
vpc.Spec.ExtraExternalSubnets == nil && defualtExternalSubnetExist(line 635), the default external subnet is connected. - Then, the loop at line 645 connects subnets from
vpc.Spec.ExtraExternalSubnets.
If vpc.Spec.ExtraExternalSubnets is not nil, the condition at line 635 is false, and the default external subnet is not connected by that block. The loop at line 645 then only connects the ExtraExternalSubnets.
This means if EnableExternal: true and ExtraExternalSubnets are provided, the default external subnet (e.g., ovn-external) might not be connected, even if it exists and EnableExternal is intended to manage it (as per the comment // EnableExternal only handle default external subnet in pkg/apis/kubeovn/v1/vpc.go).
Could you clarify if this is the intended behavior? If EnableExternal: true should always attempt to connect the default external subnet (when available), this logic might need adjustment to ensure it's connected regardless of the presence of ExtraExternalSubnets.
| for _, subnet := range vpc.Status.ExtraExternalSubnets { | ||
| if !slices.Contains(vpc.Spec.ExtraExternalSubnets, subnet) { | ||
| klog.Infof("disconnect external subnet %s to vpc %s", subnet, vpc.Name) | ||
| if err := c.handleDelVpcExternalSubnet(vpc.Name, subnet); err != nil { | ||
| klog.Errorf("failed to delete external subnet %s connection for vpc %s, error %v", subnet, vpc.Name, err) | ||
| return err | ||
| } | ||
| } | ||
| if vpc.Spec.EnableBfd { | ||
| // create bfd between lrp and physical switch gw | ||
| // bfd status down means current lrp binding chassis node external nic lost external network connectivity | ||
| // should switch lrp to another node | ||
| lrpEipName := fmt.Sprintf("%s-%s", key, c.config.ExternalGatewaySwitch) | ||
| v4ExtGw, _ := util.SplitStringIP(externalSubnet.Spec.Gateway) | ||
| // TODO: dualstack | ||
| if _, err := c.OVNNbClient.CreateBFD(lrpEipName, v4ExtGw, c.config.BfdMinRx, c.config.BfdMinTx, c.config.BfdDetectMult, nil); err != nil { | ||
| klog.Error(err) | ||
| } |
There was a problem hiding this comment.
When !vpc.Spec.EnableExternal && vpc.Status.EnableExternal (i.e., external access is being disabled for the VPC), the logic for removing ExtraExternalSubnets seems incorrect.
The current loop iterates vpc.Status.ExtraExternalSubnets and only disconnects a subnet if !slices.Contains(vpc.Spec.ExtraExternalSubnets, subnet).
However, if vpc.Spec.EnableExternal is false, all external connections, including all ExtraExternalSubnets that were previously active (and are in vpc.Status.ExtraExternalSubnets), should be removed. The contents of vpc.Spec.ExtraExternalSubnets should be irrelevant at this point for deciding which active connections to tear down, as the overarching EnableExternal flag is false.
This could lead to stale connections if EnableExternal is set to false but some subnets remain listed in vpc.Spec.ExtraExternalSubnets that were also in vpc.Status.ExtraExternalSubnets.
Shouldn't all subnets listed in vpc.Status.ExtraExternalSubnets be disconnected here, similar to how the default external subnet is unconditionally disconnected at line 617?
for _, subnet := range vpc.Status.ExtraExternalSubnets {
klog.Infof("disconnect external subnet %s to vpc %s because EnableExternal is false", subnet, vpc.Name)
if err := c.handleDelVpcExternalSubnet(vpc.Name, subnet); err != nil {
klog.Errorf("failed to delete external subnet %s connection for vpc %s, error %v", subnet, vpc.Name, err)
return err
}
}| func (c *Controller) handleUpdateVpcExternal(vpc *kubeovnv1.Vpc, custVpcEnableExternalEcmp, defualtExternalSubnetExist bool, externalSubnetGW string) error { | ||
| if vpc.Name == util.DefaultVpc { | ||
| if c.config.EnableEipSnat || !defualtExternalSubnetExist { | ||
| klog.Infof("external_gw hanlde default ovn external gw for default vpc %s", vpc.Name) | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| // handle any vpc external | ||
| if !vpc.Spec.EnableExternal && !vpc.Status.EnableExternal { | ||
| // no need to handle external connection | ||
| return nil | ||
| } | ||
|
|
||
| if vpc.Spec.EnableExternal && !defualtExternalSubnetExist && vpc.Spec.ExtraExternalSubnets == nil { | ||
| // at least have a external subnet | ||
| err := fmt.Errorf("failed to get external subnet for enable external vpc %s", vpc.Name) | ||
| klog.Error(err) | ||
| return err | ||
| } | ||
|
|
||
| if !vpc.Spec.EnableExternal && vpc.Status.EnableExternal { | ||
| // disable external to del all external subnets | ||
| klog.Infof("disconnect default external subnet %s to vpc %s", c.config.ExternalGatewaySwitch, vpc.Name) | ||
| if err := c.handleDelVpcExternalSubnet(vpc.Name, c.config.ExternalGatewaySwitch); err != nil { | ||
| klog.Errorf("failed to delete external subnet %s connection for vpc %s, error %v", c.config.ExternalGatewaySwitch, vpc.Name, err) | ||
| return err | ||
| } | ||
| for _, subnet := range vpc.Status.ExtraExternalSubnets { | ||
| if !slices.Contains(vpc.Spec.ExtraExternalSubnets, subnet) { | ||
| klog.Infof("disconnect external subnet %s to vpc %s", subnet, vpc.Name) | ||
| if err := c.handleDelVpcExternalSubnet(vpc.Name, subnet); err != nil { | ||
| klog.Errorf("failed to delete external subnet %s connection for vpc %s, error %v", subnet, vpc.Name, err) | ||
| return err | ||
| } | ||
| } | ||
| if vpc.Spec.EnableBfd { | ||
| // create bfd between lrp and physical switch gw | ||
| // bfd status down means current lrp binding chassis node external nic lost external network connectivity | ||
| // should switch lrp to another node | ||
| lrpEipName := fmt.Sprintf("%s-%s", key, c.config.ExternalGatewaySwitch) | ||
| v4ExtGw, _ := util.SplitStringIP(externalSubnet.Spec.Gateway) | ||
| // TODO: dualstack | ||
| if _, err := c.OVNNbClient.CreateBFD(lrpEipName, v4ExtGw, c.config.BfdMinRx, c.config.BfdMinTx, c.config.BfdDetectMult, nil); err != nil { | ||
| klog.Error(err) | ||
| } | ||
| } | ||
|
|
||
| if vpc.Spec.EnableExternal { | ||
| if !vpc.Status.EnableExternal { | ||
| // add external connection | ||
| if vpc.Spec.ExtraExternalSubnets == nil && defualtExternalSubnetExist { | ||
| // only connect default external subnet | ||
| klog.Infof("connect default external subnet %s with vpc %s", c.config.ExternalGatewaySwitch, vpc.Name) | ||
| if err := c.handleAddVpcExternalSubnet(vpc.Name, c.config.ExternalGatewaySwitch); err != nil { | ||
| klog.Errorf("failed to add external subnet %s connection for vpc %s, error %v", c.config.ExternalGatewaySwitch, vpc.Name, err) | ||
| return err | ||
| } | ||
| // TODO: support multi external nic | ||
| if custVpcEnableExternalMultiHopEcmp { | ||
| klog.Infof("remove normal static ecmp route for vpc %s", vpc.Name) | ||
| // auto remove normal type static route, if using ecmp based bfd | ||
| if err := c.reconcileCustomVpcDelNormalStaticRoute(vpc.Name); err != nil { | ||
| klog.Errorf("failed to reconcile del vpc %q normal static route", vpc.Name) | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| if cachedVpc.Spec.ExtraExternalSubnets != nil { | ||
| sort.Strings(vpc.Spec.ExtraExternalSubnets) | ||
| } | ||
| // add external subnets only in spec and delete external subnets only in status | ||
| if !slices.Equal(vpc.Spec.ExtraExternalSubnets, vpc.Status.ExtraExternalSubnets) { | ||
| for _, subnetStatus := range cachedVpc.Status.ExtraExternalSubnets { | ||
| if !slices.Contains(cachedVpc.Spec.ExtraExternalSubnets, subnetStatus) { | ||
| klog.Infof("delete external subnet %s connection for vpc %s", subnetStatus, vpc.Name) | ||
| if err := c.handleDelVpcExternalSubnet(vpc.Name, subnetStatus); err != nil { | ||
| klog.Errorf("failed to delete external subnet %s connection for vpc %s, error %v", subnetStatus, vpc.Name, err) | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| for _, subnetSpec := range cachedVpc.Spec.ExtraExternalSubnets { | ||
| if !slices.Contains(cachedVpc.Status.ExtraExternalSubnets, subnetSpec) { | ||
| klog.Infof("connect external subnet %s with vpc %s", subnetSpec, vpc.Name) | ||
| if err := c.handleAddVpcExternalSubnet(key, subnetSpec); err != nil { | ||
| klog.Errorf("failed to add external subnet %s connection for vpc %s, error %v", subnetSpec, key, err) | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| if err := c.updateVpcAddExternalStatus(key, true); err != nil { | ||
| klog.Errorf("failed to update additional external subnets status, %v", err) | ||
|
|
||
| // only connect provider network vlan external subnet | ||
| for _, subnet := range vpc.Spec.ExtraExternalSubnets { | ||
| klog.Infof("connect external subnet %s with vpc %s", subnet, vpc.Name) | ||
| if err := c.handleAddVpcExternalSubnet(vpc.Name, subnet); err != nil { | ||
| klog.Errorf("failed to add external subnet %s connection for vpc %s, error %v", subnet, vpc.Name, err) | ||
| return err | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if !cachedVpc.Spec.EnableBfd && cachedVpc.Status.EnableBfd { | ||
| lrpEipName := fmt.Sprintf("%s-%s", key, c.config.ExternalGatewaySwitch) | ||
| if err := c.OVNNbClient.DeleteBFDByDstIP(lrpEipName, ""); err != nil { | ||
| klog.Error(err) | ||
| return err | ||
| } | ||
| if err := c.handleDeleteVpcStaticRoute(key); err != nil { | ||
| klog.Errorf("failed to delete bfd route for vpc %s, error %v", key, err) | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| if !cachedVpc.Spec.EnableExternal && cachedVpc.Status.EnableExternal { | ||
| // disconnect vpc to default external | ||
| if err := c.handleDelVpcExternalSubnet(key, c.config.ExternalGatewaySwitch); err != nil { | ||
| klog.Errorf("failed to delete external connection for vpc %s, error %v", key, err) | ||
| return err | ||
| // diff to add | ||
| for _, subnet := range vpc.Spec.ExtraExternalSubnets { | ||
| if !slices.Contains(vpc.Status.ExtraExternalSubnets, subnet) { | ||
| klog.Infof("connect external subnet %s with vpc %s", subnet, vpc.Name) | ||
| if err := c.handleAddVpcExternalSubnet(vpc.Name, subnet); err != nil { | ||
| klog.Errorf("failed to add external subnet %s connection for vpc %s, error %v", subnet, vpc.Name, err) | ||
| return err | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if cachedVpc.Status.ExtraExternalSubnets != nil && !cachedVpc.Spec.EnableExternal { | ||
| // disconnect vpc to extra external subnets | ||
| for _, subnet := range cachedVpc.Status.ExtraExternalSubnets { | ||
| klog.Infof("disconnect external network %s to vpc %s", subnet, vpc.Name) | ||
| if err := c.handleDelVpcExternalSubnet(key, subnet); err != nil { | ||
| klog.Error(err) | ||
| // diff to del | ||
| for _, subnet := range vpc.Status.ExtraExternalSubnets { | ||
| if !slices.Contains(vpc.Spec.ExtraExternalSubnets, subnet) { | ||
| klog.Infof("disconnect external subnet %s to vpc %s", subnet, vpc.Name) | ||
| if err := c.handleDelVpcExternalSubnet(vpc.Name, subnet); err != nil { | ||
| klog.Errorf("failed to delete external subnet %s connection for vpc %s, error %v", subnet, vpc.Name, err) | ||
| return err | ||
| } | ||
| } | ||
| if err := c.updateVpcAddExternalStatus(key, false); err != nil { | ||
| klog.Errorf("failed to update additional external subnets status, %v", err) | ||
| } | ||
| } | ||
|
|
||
| // custom vpc enable bfd | ||
| if vpc.Spec.EnableBfd && vpc.Name != util.DefaultVpc && defualtExternalSubnetExist { | ||
| // create bfd between lrp and physical switch gw | ||
| // bfd status down means current lrp binding chassis node external nic lost external network connectivity | ||
| // should switch lrp to another node | ||
| lrpEipName := fmt.Sprintf("%s-%s", vpc.Name, c.config.ExternalGatewaySwitch) | ||
| v4ExtGw, _ := util.SplitStringIP(externalSubnetGW) | ||
| // TODO: dualstack | ||
| if _, err := c.OVNNbClient.CreateBFD(lrpEipName, v4ExtGw, c.config.BfdMinRx, c.config.BfdMinTx, c.config.BfdDetectMult, nil); err != nil { | ||
| klog.Error(err) | ||
| return err | ||
| } | ||
| // TODO: support multi external nic | ||
| if custVpcEnableExternalEcmp { | ||
| klog.Infof("remove normal static ecmp route for vpc %s", vpc.Name) | ||
| // auto remove normal type static route, if using ecmp based bfd | ||
| if err := c.reconcileCustomVpcDelNormalStaticRoute(vpc.Name); err != nil { | ||
| klog.Errorf("failed to reconcile del vpc %q normal static route", vpc.Name) | ||
| return err | ||
| } | ||
| } | ||
| } | ||
|
|
||
| bfdPortName, bfdPortNodes, err := c.reconcileVpcBfdLRP(vpc) | ||
| if err != nil { | ||
| klog.Error(err) | ||
| return err | ||
| } | ||
| if vpc.Spec.BFDPort == nil || !vpc.Spec.BFDPort.Enabled { | ||
| vpc.Status.BFDPort = kubeovnv1.BFDPortStatus{} | ||
| } else { | ||
| vpc.Status.BFDPort = kubeovnv1.BFDPortStatus{ | ||
| Name: bfdPortName, | ||
| IP: vpc.Spec.BFDPort.IP, | ||
| Nodes: bfdPortNodes, | ||
| if !vpc.Spec.EnableBfd && vpc.Status.EnableBfd { | ||
| lrpEipName := fmt.Sprintf("%s-%s", vpc.Name, c.config.ExternalGatewaySwitch) | ||
| if err := c.OVNNbClient.DeleteBFDByDstIP(lrpEipName, ""); err != nil { | ||
| klog.Error(err) | ||
| return err | ||
| } | ||
| if err := c.handleDeleteVpcStaticRoute(vpc.Name); err != nil { | ||
| klog.Errorf("failed to delete bfd route for vpc %s, error %v", vpc.Name, err) | ||
| return err | ||
| } | ||
| } | ||
| if _, err = c.config.KubeOvnClient.KubeovnV1().Vpcs(). | ||
| UpdateStatus(context.Background(), vpc, metav1.UpdateOptions{}); err != nil { | ||
| klog.Error(err) | ||
|
|
||
| if err := c.updateVpcExternalStatus(vpc.Name, vpc.Spec.EnableExternal); err != nil { | ||
| klog.Errorf("failed to update vpc external subnets status, %v", err) | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The new function handleUpdateVpcExternal is quite comprehensive, covering various scenarios for enabling, disabling, and managing BFD for external connections. However, it's over 120 lines long.
To improve readability and maintainability, would it be beneficial to break this function down into smaller, more focused helper functions? For example, separate functions for:
- Handling the disabling of external connections.
- Handling the initial enablement of external connections.
- Handling the diffing of
ExtraExternalSubnets. - Managing BFD setup and teardown.
| } | ||
| vpc := cachedVpc.DeepCopy() | ||
| if addExternalStatus && vpc.Spec.ExtraExternalSubnets != nil { | ||
| vpc.Spec.EnableExternal = enableExternal |
There was a problem hiding this comment.
This line vpc.Spec.EnableExternal = enableExternal within updateVpcExternalStatus is a bit confusing. This function's primary purpose is to update the status of the VPC. While vpc is a deep copy and this change won't alter the actual CRD's spec (since it's a status patch), modifying a spec field here can be misleading for future readers.
The logic within this function correctly uses the enableExternal parameter (which should reflect the desired spec state) to determine the status fields. Could this line be removed to avoid potential confusion, as the spec of the cachedVpc or the enableExternal parameter itself should be the source of truth for spec values?
Pull Request Test Coverage Report for Build 15248317977Warning: 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 |
dosubot
bot
added
size:L
Co-authored-by: 张祖建 <zhangzujian.7@gmail.com>
4 participants
Pull Request
What type of this PR
Examples of user facing changes:
any vpc can use any external subnet
For ovn-cluster to use the default
externalsubnet:For a custom VPC to use the default
externalsubnet:For any VPC to use any provider-network vlan (external) subnet
Which issue(s) this PR fixes
Fixes #(issue-number)