fix: sgs need to sync when neccessary

[zbb88888]

Pull Request

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

What type of this PR

Examples of user facing changes:

• Bug fixes

fix: deny all sg may not need if user not use kube-ovn sg
fix: all sg need to sync just after controller sg synced

add enableSecurityGroup just as enableNetworkPolicy

Which issue(s) this PR fixes

Fixes #(issue-number)

[gemini-code-assist]

Summary of Changes

Hello @zbb88888, 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 optimizes resource utilization by implementing a lazy initialization strategy for the default deny_all security group. Instead of creating this group unconditionally at startup, it is now only initialized if SecurityGroup CRDs are detected or when the first user-defined security group is processed. This prevents the creation of unneeded OVN resources in environments that do not leverage security groups.

Highlights

• Lazy Initialization: The deny_all security group initialization at startup is now skipped if no SecurityGroup CRDs currently exist in the cluster.
• Idempotent Creation: The deny_all security group is now ensured to be created when the first actual SecurityGroup CRD is processed, making its creation idempotent.
• Resource Optimization: This change reduces unnecessary OVN resource creation for users who are not utilizing SecurityGroup CRDs.

Changelog

• pkg/controller/controller.go
  • Modified the initResourceOnce function to check for existing SecurityGroup CRDs before initializing the deny_all security group and syncing security groups.
  • Added error handling for listing security groups during the conditional initialization.
• pkg/controller/security_group.go
  • Introduced a check in handleAddOrUpdateSg to ensure the deny_all security group is initialized if it doesn't exist when a new security group is being added or updated.

Activity

• No human activity has occurred on this pull request yet.

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. ↩

[coveralls]

Pull Request Test Coverage Report for Build 21905303159

Details

• 3 of 22 (13.64%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.05%) to 23.027%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/controller/config.go	0	2	0.0%
pkg/controller/controller.go	0	8	0.0%
pkg/controller/security_group.go	0	9	0.0%

Totals
Change from base Build 21899872515:	0.05%
Covered Lines:	12532
Relevant Lines:	54423

---

💛 - Coveralls

[gemini-code-assist]

Code Review

This pull request optimizes the initialization of the deny_all security group, making it lazy to reduce unnecessary resource creation. However, a critical race condition exists during controller startup due to a missing informer cache synchronization. This could lead to a temporary security bypass where the default-deny policy is not applied to ports with security groups until the cache synchronizes, potentially causing incorrect behavior.

[zbb88888]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a new flag init-default-deny-all-sg to make the initialization of the default deny-all security group optional. This is useful for users who do not use Kube-OVN's security group feature. The changes are consistently applied across Helm charts, the installation script, and the controller's configuration. Additionally, the security group cache is now properly synced before use.

However, I've identified a potential issue where the implementation is incomplete. Even when the new flag is set to false, other parts of the code may still try to operate on the deny_all security group, which would not exist, leading to errors. Please see my detailed comment for a fix.

[zbb88888]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a new enable-security-group flag to make the security group feature optional, with well-implemented changes across the Helm charts, installation script, and controller logic. However, two critical security issues were identified: a medium-severity issue where disabling the feature flag leads to an insecure partial-enforcement state, and a high-severity DoS vulnerability where malformed Pod annotations can block the enforcement of the default-deny policy cluster-wide. Additionally, there's an area for improvement regarding test coverage for the new logic and a minor typo in the pull request title ('neccessary' should be 'necessary').

[zbb88888]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a new flag enable-security-group to make the security group feature optional, which is a valuable addition for users who may not need this functionality. The changes are consistently applied across Helm charts, installation scripts, and the controller logic. When the feature is disabled, initialization is skipped, related resources are garbage collected, and handlers exit early. The new tests for garbage collection are well-written and cover the new logic. I have a few minor suggestions to simplify the code by removing some redundant checks.

[zbb88888]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a new enable-security-group flag to make the security group feature optional, which is a great enhancement for flexibility. The changes are consistently applied across Helm charts, the installation script, and the controller logic. The logic to conditionally initialize and garbage-collect security group resources based on this flag is well-implemented. Additionally, a fix is included to ensure security group caches are properly synced at startup. My only suggestion is to improve the robustness of a test mock to make it less dependent on argument order, enhancing test suite maintainability.

[gemini-code-assist]

The mock expectation for DeletePortGroup is order-sensitive because it lists the expected port group names as separate arguments. This makes the test fragile. The order of port groups to be deleted depends on the iteration order of the portGroups slice in gcSecurityGroup. While this is deterministic for a slice, it can easily break if the slice initialization is changed in the future.

Consider making the expectation order-insensitive. One way to do this with gomock is to use a custom matcher or DoAndReturn to verify the arguments without depending on their order.