fix: sgs need to sync when neccessary

charts/kube-ovn-v2/templates/controller/controller-deployment.yaml

@@ -158,6 +158,7 @@ spec:
           - --ovsdb-con-timeout={{- .Values.features.OVSDB_CON_TIMEOUT }}
           - --ovsdb-inactivity-timeout={{- .Values.features.OVSDB_INACTIVITY_TIMEOUT }}
           - --enable-live-migration-optimize={{- .Values.features.enableLiveMigrationOptimization }}
+          - --enable-security-group={{- .Values.features.enableSecurityGroup }}
           - --enable-ovn-lb-prefer-local={{- .Values.features.ENABLE_OVN_LB_PREFER_LOCAL }}
           - --image={{ .Values.global.registry.address }}/{{ .Values.global.images.kubeovn.repository }}:{{ .Values.global.images.kubeovn.tag }}
           - --non-primary-cni-mode={{- .Values.cni.nonPrimaryCNI }}
@@ -246,4 +247,3 @@ spec:
           secret:
             optional: true
             secretName: kube-ovn-tls
-

charts/kube-ovn-v2/values.yaml

@@ -226,6 +226,9 @@ features:
   # -- Enable optimized live migrations for VMs
   # @section -- Opt-in/out Features
   enableLiveMigrationOptimization: true
+  # -- Enable the security group feature including the default deny-all security group
+  # @section -- Opt-in/out Features
+  enableSecurityGroup: true
 
   ENABLE_BIND_LOCAL_IP: true
   LS_DNAT_MOD_DL_DST: true

charts/kube-ovn/templates/controller-deploy.yaml

@@ -149,6 +149,7 @@ spec:
           - --ovsdb-con-timeout={{- .Values.func.OVSDB_CON_TIMEOUT }}
           - --ovsdb-inactivity-timeout={{- .Values.func.OVSDB_INACTIVITY_TIMEOUT }}
           - --enable-live-migration-optimize={{- .Values.func.ENABLE_LIVE_MIGRATION_OPTIMIZE }}
+          - --enable-security-group={{- .Values.func.ENABLE_SECURITY_GROUP }}
           - --enable-ovn-lb-prefer-local={{- .Values.func.ENABLE_OVN_LB_PREFER_LOCAL }}
           - --image={{ .Values.global.registry.address }}/{{ .Values.global.images.kubeovn.repository }}:{{ .Values.global.images.kubeovn.tag }}
           - --skip-conntrack-dst-cidrs={{- .Values.networking.SKIP_CONNTRACK_DST_CIDRS }}

charts/kube-ovn/values.yaml

@@ -85,6 +85,7 @@ func:
   OVSDB_CON_TIMEOUT: 3
   OVSDB_INACTIVITY_TIMEOUT: 10
   ENABLE_LIVE_MIGRATION_OPTIMIZE: true
+  ENABLE_SECURITY_GROUP: true
   ENABLE_OVN_LB_PREFER_LOCAL: false
 
 ipv4:

dist/images/install.sh

@@ -57,6 +57,7 @@ SET_VXLAN_TX_OFF=${SET_VXLAN_TX_OFF:-false}
 OVSDB_CON_TIMEOUT=${OVSDB_CON_TIMEOUT:-3}
 OVSDB_INACTIVITY_TIMEOUT=${OVSDB_INACTIVITY_TIMEOUT:-10}
 ENABLE_LIVE_MIGRATION_OPTIMIZE=${ENABLE_LIVE_MIGRATION_OPTIMIZE:-true}
+ENABLE_SECURITY_GROUP=${ENABLE_SECURITY_GROUP:-true}
 ENABLE_OVN_LB_PREFER_LOCAL=${ENABLE_OVN_LB_PREFER_LOCAL:-false}
 
 PROBE_HTTP_SCHEME="HTTP"
@@ -5309,6 +5310,7 @@ spec:
           - --ovsdb-con-timeout=$OVSDB_CON_TIMEOUT
           - --ovsdb-inactivity-timeout=$OVSDB_INACTIVITY_TIMEOUT
           - --enable-live-migration-optimize=$ENABLE_LIVE_MIGRATION_OPTIMIZE
+          - --enable-security-group=$ENABLE_SECURITY_GROUP
           - --enable-ovn-lb-prefer-local=$ENABLE_OVN_LB_PREFER_LOCAL
           - --image=$REGISTRY/kube-ovn:$VERSION
           securityContext:

pkg/controller/config.go

@@ -100,6 +100,7 @@ type Configuration struct {
 	EnableOVNIPSec              bool
 	CertManagerIPSecCert        bool
 	EnableLiveMigrationOptimize bool
+	EnableSecurityGroup         bool
 
 	ExternalGatewaySwitch   string
 	ExternalGatewayConfigNS string
@@ -203,6 +204,7 @@ func ParseFlags() (*Configuration, error) {
 		argEnableOVNIPSec              = pflag.Bool("enable-ovn-ipsec", false, "Whether to enable ovn ipsec")
 		argCertManagerIPSecCert        = pflag.Bool("cert-manager-ipsec-cert", false, "Whether to use cert-manager for signing IPSec certificates")
 		argEnableLiveMigrationOptimize = pflag.Bool("enable-live-migration-optimize", true, "Whether to enable kubevirt live migration optimize")
+		argEnableSecurityGroup         = pflag.Bool("enable-security-group", true, "Whether to enable the security group feature including the default deny-all security group")
 
 		argExternalGatewayConfigNS = pflag.String("external-gateway-config-ns", "kube-system", "The namespace of configmap external-gateway-config, default: kube-system")
 		argExternalGatewaySwitch   = pflag.String("external-gateway-switch", "external", "The name of the external gateway switch which is a ovs bridge to provide external network, default: external")
@@ -310,6 +312,7 @@ func ParseFlags() (*Configuration, error) {
 		EnableOVNIPSec:                 *argEnableOVNIPSec,
 		CertManagerIPSecCert:           *argCertManagerIPSecCert,
 		EnableLiveMigrationOptimize:    *argEnableLiveMigrationOptimize,
+		EnableSecurityGroup:            *argEnableSecurityGroup,
 		BfdMinTx:                       *argBfdMinTx,
 		BfdMinRx:                       *argBfdMinRx,
 		BfdDetectMult:                  *argBfdDetectMult,

pkg/controller/controller.go

@@ -716,7 +716,7 @@ func Run(ctx context.Context, config *Configuration) {
 		controller.vlanSynced, controller.podsSynced, controller.namespacesSynced, controller.nodesSynced,
 		controller.serviceSynced, controller.endpointSlicesSynced, controller.deploymentsSynced, controller.configMapsSynced,
 		controller.ovnEipSynced, controller.ovnFipSynced, controller.ovnSnatRuleSynced,
-		controller.ovnDnatRuleSynced,
+		controller.ovnDnatRuleSynced, controller.sgSynced,
 	}
 	if controller.config.EnableLb {
 		cacheSyncs = append(cacheSyncs, controller.switchLBRuleSynced, controller.vpcDNSSynced)
@@ -1512,11 +1512,13 @@ func (c *Controller) initResourceOnce() {
 		util.LogFatalAndExit(err, "failed to initialize node chassis")
 	}
 
-	if err := c.initDefaultDenyAllSecurityGroup(); err != nil {
-		util.LogFatalAndExit(err, "failed to initialize 'deny_all' security group")
-	}
-	if err := c.syncSecurityGroup(); err != nil {
-		util.LogFatalAndExit(err, "failed to sync security group")
+	if c.config.EnableSecurityGroup {
+		if err := c.initDefaultDenyAllSecurityGroup(); err != nil {
+			util.LogFatalAndExit(err, "failed to initialize 'deny_all' security group")
+		}
+		if err := c.syncSecurityGroup(); err != nil {
+			util.LogFatalAndExit(err, "failed to sync security group")
+		}
 	}
 
 	if err := c.syncVpcNatGatewayCR(); err != nil {

pkg/controller/gc.go

@@ -832,7 +832,9 @@ func (c *Controller) gcSecurityGroup() error {
 	denyAllPg := ovs.GetSgPortGroupName(util.DenyAllSecurityGroup)
 	defaultPg := ovs.GetSgPortGroupName(util.DefaultSecurityGroupName)
 	for _, pg := range pgs {
-		if pg.Name == denyAllPg || pg.Name == defaultPg || pg.ExternalIDs[networkPolicyKey] != "" {
+		// denyAllPg is only preserved when security group feature is enabled;
+		// defaultPg is always preserved as it may be referenced by LSP creation regardless of the feature flag.
+		if (c.config.EnableSecurityGroup && pg.Name == denyAllPg) || pg.Name == defaultPg || pg.ExternalIDs[networkPolicyKey] != "" {
 			continue
 		}
 		// if port group not exist in security group, delete it

pkg/controller/gc_test.go

@@ -1,11 +1,15 @@
 package controller
 
 import (
+	"context"
 	"testing"
 
 	"github.com/scylladb/go-set/strset"
 	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
+	kubeovnv1 "github.com/kubeovn/kube-ovn/pkg/apis/kubeovn/v1"
+	"github.com/kubeovn/kube-ovn/pkg/ovs"
 	"github.com/kubeovn/kube-ovn/pkg/ovsdb/ovnnb"
 	"github.com/kubeovn/kube-ovn/pkg/util"
 )
@@ -49,3 +53,137 @@ func Test_logicalRouterPortFilter(t *testing.T) {
 		}
 	}
 }
+
+func Test_gcSecurityGroup(t *testing.T) {
+	t.Parallel()
+
+	denyAllPg := ovs.GetSgPortGroupName(util.DenyAllSecurityGroup)
+	defaultPg := ovs.GetSgPortGroupName(util.DefaultSecurityGroupName)
+
+	t.Run("EnableSecurityGroup=true preserves deny_all port group", func(t *testing.T) {
+		t.Parallel()
+		fc := newFakeController(t)
+		ctrl := fc.fakeController
+		ctrl.config.EnableSecurityGroup = true
+
+		// Create a SecurityGroup CR so its port group is not orphaned
+		sg := &kubeovnv1.SecurityGroup{
+			ObjectMeta: metav1.ObjectMeta{Name: "existing-sg"},
+		}
+		_, err := ctrl.config.KubeOvnClient.KubeovnV1().SecurityGroups().Create(
+			context.Background(), sg, metav1.CreateOptions{})
+		require.NoError(t, err)
+
+		existingSgPg := ovs.GetSgPortGroupName("existing-sg")
+		orphanedPg := ovs.GetSgPortGroupName("orphaned-sg")
+
+		portGroups := []ovnnb.PortGroup{
+			{Name: denyAllPg, ExternalIDs: map[string]string{"vendor": util.CniTypeName, "sg": util.DenyAllSecurityGroup}},
+			{Name: defaultPg, ExternalIDs: map[string]string{"vendor": util.CniTypeName, "sg": util.DefaultSecurityGroupName}},
+			{Name: existingSgPg, ExternalIDs: map[string]string{"vendor": util.CniTypeName, "sg": "existing-sg"}},
+			{Name: orphanedPg, ExternalIDs: map[string]string{"vendor": util.CniTypeName, "sg": "orphaned-sg"}},
+			{Name: "np-pg", ExternalIDs: map[string]string{"vendor": util.CniTypeName, networkPolicyKey: "some-np"}},
+		}
+
+		fc.mockOvnClient.EXPECT().
+			ListPortGroups(map[string]string{"vendor": util.CniTypeName}).
+			Return(portGroups, nil)
+		// Only the orphaned port group should be deleted; deny_all, default, existing-sg, and np-pg are preserved
+		fc.mockOvnClient.EXPECT().
+			DeletePortGroup(orphanedPg).
+			Return(nil)
+
+		err = ctrl.gcSecurityGroup()
+		require.NoError(t, err)
+	})
+
+	t.Run("EnableSecurityGroup=false garbage-collects deny_all port group", func(t *testing.T) {
+		t.Parallel()
+		fc := newFakeController(t)
+		ctrl := fc.fakeController
+		ctrl.config.EnableSecurityGroup = false
+
+		// Create a SecurityGroup CR
+		sg := &kubeovnv1.SecurityGroup{
+			ObjectMeta: metav1.ObjectMeta{Name: "existing-sg"},
+		}
+		_, err := ctrl.config.KubeOvnClient.KubeovnV1().SecurityGroups().Create(
+			context.Background(), sg, metav1.CreateOptions{})
+		require.NoError(t, err)
+
+		existingSgPg := ovs.GetSgPortGroupName("existing-sg")
+		orphanedPg := ovs.GetSgPortGroupName("orphaned-sg")
+
+		portGroups := []ovnnb.PortGroup{
+			// deny_all should now be GC'd because EnableSecurityGroup=false
+			{Name: denyAllPg, ExternalIDs: map[string]string{"vendor": util.CniTypeName, "sg": util.DenyAllSecurityGroup}},
+			{Name: defaultPg, ExternalIDs: map[string]string{"vendor": util.CniTypeName, "sg": util.DefaultSecurityGroupName}},
+			{Name: existingSgPg, ExternalIDs: map[string]string{"vendor": util.CniTypeName, "sg": "existing-sg"}},
+			{Name: orphanedPg, ExternalIDs: map[string]string{"vendor": util.CniTypeName, "sg": "orphaned-sg"}},
+			{Name: "np-pg", ExternalIDs: map[string]string{"vendor": util.CniTypeName, networkPolicyKey: "some-np"}},
+		}
+
+		fc.mockOvnClient.EXPECT().
+			ListPortGroups(map[string]string{"vendor": util.CniTypeName}).
+			Return(portGroups, nil)
+		// Both deny_all and orphaned port groups should be deleted
+		// deny_all is no longer preserved because EnableSecurityGroup=false
+		fc.mockOvnClient.EXPECT().
+			DeletePortGroup(denyAllPg, orphanedPg).
+			Return(nil)
+
+		err = ctrl.gcSecurityGroup()
+		require.NoError(t, err)
+	})
+
+	t.Run("no orphaned port groups results in no deletion", func(t *testing.T) {
+		t.Parallel()
+		fc := newFakeController(t)
+		ctrl := fc.fakeController
+		ctrl.config.EnableSecurityGroup = true
+
+		sg := &kubeovnv1.SecurityGroup{
+			ObjectMeta: metav1.ObjectMeta{Name: "existing-sg"},
+		}
+		_, err := ctrl.config.KubeOvnClient.KubeovnV1().SecurityGroups().Create(
+			context.Background(), sg, metav1.CreateOptions{})
+		require.NoError(t, err)
+
+		existingSgPg := ovs.GetSgPortGroupName("existing-sg")
+
+		portGroups := []ovnnb.PortGroup{
+			{Name: denyAllPg, ExternalIDs: map[string]string{"vendor": util.CniTypeName, "sg": util.DenyAllSecurityGroup}},
+			{Name: defaultPg, ExternalIDs: map[string]string{"vendor": util.CniTypeName, "sg": util.DefaultSecurityGroupName}},
+			{Name: existingSgPg, ExternalIDs: map[string]string{"vendor": util.CniTypeName, "sg": "existing-sg"}},
+		}
+
+		fc.mockOvnClient.EXPECT().
+			ListPortGroups(map[string]string{"vendor": util.CniTypeName}).
+			Return(portGroups, nil)
+		// DeletePortGroup should NOT be called
+
+		err = ctrl.gcSecurityGroup()
+		require.NoError(t, err)
+	})
+
+	t.Run("network policy port groups are always preserved", func(t *testing.T) {
+		t.Parallel()
+		fc := newFakeController(t)
+		ctrl := fc.fakeController
+		ctrl.config.EnableSecurityGroup = false
+
+		portGroups := []ovnnb.PortGroup{
+			{Name: "np-pg-1", ExternalIDs: map[string]string{"vendor": util.CniTypeName, networkPolicyKey: "policy-a"}},
+			{Name: "np-pg-2", ExternalIDs: map[string]string{"vendor": util.CniTypeName, networkPolicyKey: "policy-b"}},
+			{Name: defaultPg, ExternalIDs: map[string]string{"vendor": util.CniTypeName, "sg": util.DefaultSecurityGroupName}},
+		}
+
+		fc.mockOvnClient.EXPECT().
+			ListPortGroups(map[string]string{"vendor": util.CniTypeName}).
+			Return(portGroups, nil)
+		// No port groups should be deleted
+
+		err := ctrl.gcSecurityGroup()
+		require.NoError(t, err)
+	})
+}

pkg/controller/security_group.go

@@ -141,6 +141,10 @@ func (c *Controller) updateDenyAllSgPorts() error {
 }
 
 func (c *Controller) handleAddOrUpdateSg(key string, force bool) error {
+	if !c.config.EnableSecurityGroup {
+		return nil
+	}
+
 	c.sgKeyMutex.LockKey(key)
 	defer func() { _ = c.sgKeyMutex.UnlockKey(key) }()
 	klog.Infof("handle add/update security group %s", key)
@@ -319,6 +323,10 @@ func (c *Controller) patchSgStatus(sg *kubeovnv1.SecurityGroup) {
 }
 
 func (c *Controller) handleDeleteSg(key string) error {
+	if !c.config.EnableSecurityGroup {
+		return nil
+	}
+
 	c.sgKeyMutex.LockKey(key)
 	defer func() { _ = c.sgKeyMutex.UnlockKey(key) }()
 	klog.Infof("handle delete security group %s", key)
@@ -332,6 +340,10 @@ func (c *Controller) handleDeleteSg(key string) error {
 }
 
 func (c *Controller) syncSgLogicalPort(key string) error {
+	if !c.config.EnableSecurityGroup {
+		return nil
+	}
+
 	c.sgKeyMutex.LockKey(key)
 	defer func() { _ = c.sgKeyMutex.UnlockKey(key) }()
 	klog.Infof("sync lsp for security group %s", key)