cni-server: fix vlan id reference#6365
Conversation
Signed-off-by: zhangzujian <zhangzujian.7@gmail.com>
Summary of ChangesHello, 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 addresses a bug in the CNI server where an incorrect field was referenced for the VLAN ID when creating FDB (Forwarding Database) entries. By updating the code to use the correct VLAN ID field, this change ensures that FDB entries are accurately generated, preventing potential network misconfigurations or connectivity issues related to VLANs. Highlights
Changelog
Activity
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 aims to fix a reference to a deprecated VLAN ID field. While the change correctly uses the new vlan.Spec.ID field, it overlooks backward compatibility for existing resources that might only have the deprecated vlan.Spec.VlanID field populated. This could lead to incorrect network behavior. I've added a comment with a suggestion to handle this transition gracefully.
| continue | ||
| } | ||
| index := fdbIndex{vlan.Spec.VlanID, subnet.Status.U2OInterconnectionMAC} | ||
| index := fdbIndex{vlan.Spec.ID, subnet.Status.U2OInterconnectionMAC} |
There was a problem hiding this comment.
While this change correctly moves towards using the non-deprecated ID field, it could introduce a bug. If an existing Vlan custom resource was created using only the deprecated vlanId field, vlan.Spec.ID would be 0 while vlan.Spec.VlanID holds the correct value. This would result in installing an FDB entry for VLAN 0, which is likely incorrect, as VLAN 0 is typically used for untagged traffic.
To ensure backward compatibility during the transition period, I suggest adding a fallback to the deprecated field when the new ID field is not set. A more robust long-term solution would be to implement migration logic in the vlan controller (or a conversion webhook) to populate ID from VlanID on resource reconciliation, making ID the single source of truth throughout the codebase.
vlanID := vlan.Spec.ID
if vlan.Spec.ID == 0 && vlan.Spec.VlanID != 0 {
vlanID = vlan.Spec.VlanID
}
index := fdbIndex{vlanID, subnet.Status.U2OInterconnectionMAC}Signed-off-by: zhangzujian <zhangzujian.7@gmail.com>
Signed-off-by: zhangzujian <zhangzujian.7@gmail.com>
Pull Request
What type of this PR
Examples of user facing changes:
Which issue(s) this PR fixes
Fixes #(issue-number)