fix: test secgroup rules to ovn ACL and recode guest adminSecgroup (#13358)

Co-authored-by: Qiu Jian <qiujian@yunionyun.com>

pkg/compute/models/guest_actions.go

@@ -1215,10 +1215,14 @@ func (self *SGuest) PerformAddSecgroup(ctx context.Context, userCred mcclient.To
 	return nil, self.StartSyncTask(ctx, userCred, true, "")
 }
 
-func (self *SGuest) saveDefaultSecgroupId(userCred mcclient.TokenCredential, secGrpId string) error {
-	if secGrpId != self.SecgrpId {
+func (self *SGuest) saveDefaultSecgroupId(userCred mcclient.TokenCredential, secGrpId string, isAdmin bool) error {
+	if (!isAdmin && secGrpId != self.SecgrpId) || (isAdmin && secGrpId != self.AdminSecgrpId) {
 		diff, err := db.Update(self, func() error {
-			self.SecgrpId = secGrpId
+			if isAdmin {
+				self.AdminSecgrpId = secGrpId
+			} else {
+				self.SecgrpId = secGrpId
+			}
 			return nil
 		})
 		if err != nil {
@@ -1279,8 +1283,51 @@ func (self *SGuest) PerformRevokeSecgroup(ctx context.Context, userCred mcclient
 	return nil, self.StartSyncTask(ctx, userCred, true, "")
 }
 
+func (self *SGuest) PerformRevokeAdminSecgroup(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, input api.GuestRevokeSecgroupInput) (jsonutils.JSONObject, error) {
+	if !db.IsAdminAllowPerform(ctx, userCred, self, "revoke-admin-secgroup") {
+		return nil, httperrors.NewForbiddenError("not allow to revoke admin secgroup")
+	}
+
+	if !utils.IsInStringArray(self.Status, []string{api.VM_READY, api.VM_RUNNING, api.VM_SUSPEND}) {
+		return nil, httperrors.NewInputParameterError("Cannot assign security rules in status %s", self.Status)
+	}
+
+	var notes string
+	adminSecgrpId := ""
+	if len(options.Options.DefaultAdminSecurityGroupId) > 0 {
+		adminSecgrp, _ := SecurityGroupManager.FetchSecgroupById(options.Options.DefaultAdminSecurityGroupId)
+		if adminSecgrp != nil {
+			adminSecgrpId = adminSecgrp.Id
+			notes = fmt.Sprintf("reset admin secgroup to %s(%s)", adminSecgrp.Name, adminSecgrp.Id)
+		}
+	}
+	if adminSecgrpId == "" {
+		notes = "clean admin secgroup"
+	}
+
+	err := self.saveDefaultSecgroupId(userCred, adminSecgrpId, true)
+	if err != nil {
+		return nil, errors.Wrap(err, "saveDefaultSecgroupId")
+	}
+
+	logclient.AddActionLogWithContext(ctx, self, logclient.ACT_VM_REVOKESECGROUP, notes, userCred, true)
+	return nil, self.StartSyncTask(ctx, userCred, true, "")
+}
+
 // +onecloud:swagger-gen-ignore
 func (self *SGuest) PerformAssignSecgroup(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, input api.GuestAssignSecgroupInput) (jsonutils.JSONObject, error) {
+	return self.performAssignSecgroup(ctx, userCred, query, input, false)
+}
+
+func (self *SGuest) PerformAssignAdminSecgroup(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, input api.GuestAssignSecgroupInput) (jsonutils.JSONObject, error) {
+	if !db.IsAdminAllowPerform(ctx, userCred, self, "assign-admin-secgroup") {
+		return nil, httperrors.NewForbiddenError("not allow to assign admin secgroup")
+	}
+
+	return self.performAssignSecgroup(ctx, userCred, query, input, true)
+}
+
+func (self *SGuest) performAssignSecgroup(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, input api.GuestAssignSecgroupInput, isAdmin bool) (jsonutils.JSONObject, error) {
 	if !utils.IsInStringArray(self.Status, []string{api.VM_READY, api.VM_RUNNING, api.VM_SUSPEND}) {
 		return nil, httperrors.NewInputParameterError("Cannot assign security rules in status %s", self.Status)
 	}
@@ -1299,12 +1346,12 @@ func (self *SGuest) PerformAssignSecgroup(ctx context.Context, userCred mcclient
 		return nil, httperrors.NewInputParameterError("The secgroup name %s does not meet the requirements, please change the name", secObj.GetName())
 	}
 
-	err = self.saveDefaultSecgroupId(userCred, input.SecgroupId)
+	err = self.saveDefaultSecgroupId(userCred, input.SecgroupId, isAdmin)
 	if err != nil {
 		return nil, err
 	}
 
-	notes := map[string]string{"name": secObj.GetName(), "id": secObj.GetId()}
+	notes := map[string]string{"name": secObj.GetName(), "id": secObj.GetId(), "is_admin": fmt.Sprintf("%v", isAdmin)}
 	logclient.AddActionLogWithContext(ctx, self, logclient.ACT_VM_ASSIGNSECGROUP, notes, userCred, true)
 	return nil, self.StartSyncTask(ctx, userCred, true, "")
 }
@@ -1404,7 +1451,7 @@ func (self *SGuest) saveSecgroups(ctx context.Context, userCred mcclient.TokenCr
 			return errors.Wrapf(err, "New guest secgroup for guest %s with secgroup %s", self.Name, id)
 		}
 	}
-	return self.saveDefaultSecgroupId(userCred, secgroupIds[0])
+	return self.saveDefaultSecgroupId(userCred, secgroupIds[0], false)
 }
 
 func (self *SGuest) newGuestSecgroup(ctx context.Context, secgroupId string) error {
@@ -2616,7 +2663,7 @@ func (self *SGuest) RevokeAllSecgroups(ctx context.Context, userCred mcclient.To
 			return errors.Wrap(err, "Delete")
 		}
 	}
-	return self.saveDefaultSecgroupId(userCred, api.SECGROUP_DEFAULT_ID)
+	return self.saveDefaultSecgroupId(userCred, options.Options.DefaultSecurityGroupId, false)
 }
 
 func (self *SGuest) DoPendingDelete(ctx context.Context, userCred mcclient.TokenCredential) {

pkg/compute/models/guests.go

@@ -143,7 +143,7 @@ type SGuest struct {
 	// example: default
 	SecgrpId string `width:"36" charset:"ascii" nullable:"true" list:"user" get:"user" create:"optional"`
 	// 管理员可见安全组Id
-	AdminSecgrpId string `width:"36" charset:"ascii" nullable:"true" list:"admin" get:"domain"`
+	AdminSecgrpId string `width:"36" charset:"ascii" nullable:"true" list:"domain" get:"domain"`
 
 	SrcIpCheck  tristate.TriState `nullable:"false" default:"true" create:"optional" list:"user" update:"user"`
 	SrcMacCheck tristate.TriState `nullable:"false" default:"true" create:"optional" list:"user" update:"user"`
@@ -654,12 +654,13 @@ func (manager *SGuestManager) initHostname() error {
 	return nil
 }
 
-func (manager *SGuestManager) InitializeData() error {
+func (manager *SGuestManager) clearSecgroups() error {
 	guests := make([]SGuest, 0, 10)
 	q := manager.Query()
 	q = q.In("hypervisor", []string{api.HYPERVISOR_ESXI, api.HYPERVISOR_NUTANIX}).Filter(
-		sqlchemy.NOT(
-			sqlchemy.IsNullOrEmpty(q.Field("secgrp_id")),
+		sqlchemy.OR(
+			sqlchemy.IsNotEmpty(q.Field("secgrp_id")),
+			sqlchemy.IsNotEmpty(q.Field("admin_secgrp_id")),
 		),
 	)
 	err := db.FetchModelObjects(manager, q, &guests)
@@ -670,10 +671,50 @@ func (manager *SGuestManager) InitializeData() error {
 	for i := range guests {
 		db.Update(&guests[i], func() error {
 			guests[i].SecgrpId = ""
+			guests[i].AdminSecgrpId = ""
 			return nil
 		})
 	}
-	return manager.initHostname()
+	return nil
+}
+
+func (manager *SGuestManager) initAdminSecgroupId() error {
+	if len(options.Options.DefaultAdminSecurityGroupId) == 0 {
+		return nil
+	}
+	adminSec, _ := SecurityGroupManager.FetchSecgroupById(options.Options.DefaultAdminSecurityGroupId)
+	if adminSec == nil {
+		return nil
+	}
+	adminSecId := adminSec.Id
+	guests := make([]SGuest, 0, 10)
+	q := manager.Query()
+	q = q.In("hypervisor", []string{api.HYPERVISOR_KVM}).IsNullOrEmpty("admin_secgrp_id")
+	err := db.FetchModelObjects(manager, q, &guests)
+	if err != nil {
+		return errors.Wrap(err, "db.FetchModelObjects")
+	}
+	// remove secgroup for esxi nutanix guest
+	for i := range guests {
+		db.Update(&guests[i], func() error {
+			guests[i].AdminSecgrpId = adminSecId
+			return nil
+		})
+	}
+	return nil
+}
+
+func (manager *SGuestManager) InitializeData() error {
+	if err := manager.initHostname(); err != nil {
+		return errors.Wrap(err, "initHostname")
+	}
+	if err := manager.clearSecgroups(); err != nil {
+		return errors.Wrap(err, "cleanSecgroups")
+	}
+	if err := manager.initAdminSecgroupId(); err != nil {
+		return errors.Wrap(err, "initAdminSecgroupId")
+	}
+	return nil
 }
 
 func (guest *SGuest) GetHypervisor() string {
@@ -907,6 +948,12 @@ func (guest *SGuest) IsNetworkAllocated() bool {
 }
 
 func (guest *SGuest) CustomizeCreate(ctx context.Context, userCred mcclient.TokenCredential, ownerId mcclient.IIdentityProvider, query jsonutils.JSONObject, data jsonutils.JSONObject) error {
+	if len(guest.SecgrpId) > 0 && len(options.Options.DefaultAdminSecurityGroupId) > 0 {
+		adminSec, _ := SecurityGroupManager.FetchSecgroupById(options.Options.DefaultAdminSecurityGroupId)
+		if adminSec != nil {
+			guest.AdminSecgrpId = adminSec.Id
+		}
+	}
 	guest.HostId = ""
 	return guest.SVirtualResourceBase.CustomizeCreate(ctx, userCred, ownerId, query, data)
 }
@@ -1497,7 +1544,7 @@ func (manager *SGuestManager) validateCreateData(
 		}
 		input.SecgroupId = secGrpObj.GetId()
 	} else {
-		input.SecgroupId = "default"
+		input.SecgroupId = options.Options.DefaultSecurityGroupId
 	}
 
 	maxSecgrpCount := GetDriver(hypervisor).GetMaxSecurityGroupCount()
@@ -2383,8 +2430,8 @@ func (self *SGuest) getSecurityGroupsRules() string {
 	q.Filter(sqlchemy.In(q.Field("secgroup_id"), secgroupids)).Desc(q.Field("priority"), q.Field("action"))
 	secrules := []SSecurityGroupRule{}
 	if err := db.FetchModelObjects(SecurityGroupRuleManager, q, &secrules); err != nil {
-		log.Errorf("Get rules error: %v", err)
-		return options.Options.DefaultSecurityRules
+		log.Errorf("Get security group rules error: %v", err)
+		return ""
 	}
 	rules := []string{}
 	for _, rule := range secrules {
@@ -2398,9 +2445,8 @@ func (self *SGuest) getAdminSecurityRules() string {
 	if secgrp != nil {
 		ret, _ := secgrp.getSecurityRuleString()
 		return ret
-	} else {
-		return options.Options.DefaultAdminSecurityRules
 	}
+	return ""
 }
 
 func (self *SGuest) isGpu() bool {

pkg/compute/models/secgroups.go

@@ -1317,9 +1317,12 @@ func (sm *SSecurityGroupManager) TotalCnt(secIds []string) (map[string]api.SSecu
 }
 
 func (self *SSecurityGroup) ValidateDeleteCondition(ctx context.Context, info jsonutils.JSONObject) error {
-	if self.Id == api.SECGROUP_DEFAULT_ID {
+	if self.Id == options.Options.DefaultSecurityGroupId {
 		return httperrors.NewProtectedResourceError("not allow to delete default security group")
 	}
+	if self.Id == options.Options.DefaultAdminSecurityGroupId {
+		return httperrors.NewProtectedResourceError("not allow to delete default admin security group")
+	}
 	cnts, err := SecurityGroupManager.TotalCnt([]string{self.Id})
 	if err != nil {
 		return errors.Wrapf(err, "SecurityGroupManager.TotalCnt")

pkg/compute/options/options.go

@@ -31,8 +31,8 @@ type ComputeOptions struct {
 	DefaultMemoryOvercommitBound  float32 `default:"1.0" help:"Default memory overcommit bound for host, default to 1"`
 	DefaultStorageOvercommitBound float32 `default:"1.0" help:"Default storage overcommit bound for storage, default to 1"`
 
-	DefaultSecurityRules      string `help:"Default security rules" default:"allow any"`
-	DefaultAdminSecurityRules string `help:"Default admin security rules" default:""`
+	DefaultSecurityGroupId      string `help:"Default security rules" default:"default"`
+	DefaultAdminSecurityGroupId string `help:"Default admin security rules" default:""`
 
 	DefaultDiskSizeMB int `default:"10240" help:"Default disk size in MB if not specified, default to 10GiB" json:"default_disk_size"`
 

pkg/vpcagent/models/modelset.go

@@ -239,7 +239,7 @@ func (set Guests) joinHosts(subEntries Hosts) bool {
 
 func (set Guests) joinSecurityGroups(subEntries SecurityGroups) bool {
 	correct := true
-	j := func(guest *Guest, fname, secgroupId string) (*SecurityGroup, bool) {
+	j := func(guest *Guest, fname, secgroupId string, isAdmin bool) (*SecurityGroup, bool) {
 		if secgroupId == "" {
 			return nil, true
 		}
@@ -249,12 +249,15 @@ func (set Guests) joinSecurityGroups(subEntries SecurityGroups) bool {
 				fname, secgroupId, guest.Name, guest.Id)
 			return nil, false
 		}
-		guest.SecurityGroups[secgroupId] = secgroup
+		if !isAdmin {
+			// do not save admin security group, store it in AdminSecurityGroup instead
+			guest.SecurityGroups[secgroupId] = secgroup
+		}
 		return secgroup, true
 	}
 	for _, g := range set {
-		adminSecgroup, c0 := j(g, "admin_secgrp_id", g.AdminSecgrpId)
-		_, c1 := j(g, "secgrp_id", g.SecgrpId)
+		adminSecgroup, c0 := j(g, "admin_secgrp_id", g.AdminSecgrpId, true)
+		_, c1 := j(g, "secgrp_id", g.SecgrpId, false)
 		g.AdminSecurityGroup = adminSecgroup
 		if !(c0 && c1) {
 			correct = false

pkg/vpcagent/models/secgrouprules_sort.go

@@ -21,29 +21,25 @@ import (
 func (el *Guest) OrderedSecurityGroupRules() []*SecurityGroupRule {
 	rs := []*SecurityGroupRule{}
 	for _, secgroup := range el.SecurityGroups {
-		rs = append(rs, secgroup.securityGroupRules()...)
+		rs = append(rs, secgroup.securityGroupRules(0)...)
+	}
+	if el.AdminSecurityGroup != nil {
+		rs = append(rs, el.AdminSecurityGroup.securityGroupRules(100)...)
 	}
 	sort.Slice(rs, SecurityGroupRuleLessFunc(rs))
-	if el.AdminSecurityGroup != nil {
-		rs = append(rs, el.AdminSecurityGroup.OrderedSecurityGroupRules()...)
-	}
 	return rs
 }
 
-func (el *SecurityGroup) securityGroupRules() []*SecurityGroupRule {
+func (el *SecurityGroup) securityGroupRules(basePriority int64) []*SecurityGroupRule {
 	rs := make([]*SecurityGroupRule, 0, len(el.SecurityGroupRules))
 	for _, r := range el.SecurityGroupRules {
+		r = r.Copy()
+		r.Priority += basePriority
 		rs = append(rs, r)
 	}
 	return rs
 }
 
-func (el *SecurityGroup) OrderedSecurityGroupRules() []*SecurityGroupRule {
-	rs := el.securityGroupRules()
-	sort.Slice(rs, SecurityGroupRuleLessFunc(rs))
-	return rs
-}
-
 func SecurityGroupRuleLessFunc(rs []*SecurityGroupRule) func(i, j int) bool {
 	return func(i, j int) bool {
 		return rs[i].Priority < rs[i].Priority

pkg/vpcagent/ovn/secgrouprules_acl.go

@@ -168,5 +168,6 @@ func ruleToAcl(lport string, rule *agentmodels.SecurityGroupRule) (*ovn_nb.ACL,
 		Match:     match,
 		Action:    action,
 	}
+
 	return acl, nil
 }

pkg/vpcagent/ovn/secgrouprules_acl_test.go

@@ -0,0 +1,121 @@
+// Copyright 2019 Yunion
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License
+
+package ovn
+
+import (
+	"fmt"
+	"reflect"
+	"testing"
+
+	"yunion.io/x/jsonutils"
+	"yunion.io/x/ovsdb/schema/ovn_nb"
+	"yunion.io/x/pkg/util/secrules"
+
+	"yunion.io/x/onecloud/pkg/compute/models"
+	agentmodels "yunion.io/x/onecloud/pkg/vpcagent/models"
+)
+
+func TestRuleToACL(t *testing.T) {
+	lport := "local-port120"
+	cases := []struct {
+		rule *agentmodels.SecurityGroupRule
+		acl  *ovn_nb.ACL
+	}{
+		{
+			// egress deny 100.10.10.0/24
+			rule: &agentmodels.SecurityGroupRule{
+				SSecurityGroupRule: models.SSecurityGroupRule{
+					Direction: string(secrules.SecurityRuleEgress),
+					CIDR:      "100.10.10.0/24",
+					Action:    string(secrules.SecurityRuleDeny),
+					Protocol:  secrules.PROTO_ANY,
+					Priority:  100,
+				},
+			},
+			acl: &ovn_nb.ACL{
+				Direction: aclDirFromLport,
+				Action:    "drop",
+				Match:     fmt.Sprintf("inport == %q && ip4 && ip4.dst == 100.10.10.0/24", lport),
+				Priority:  100,
+			},
+		},
+		{
+			// egress allow any
+			rule: &agentmodels.SecurityGroupRule{
+				SSecurityGroupRule: models.SSecurityGroupRule{
+					Direction: string(secrules.SecurityRuleEgress),
+					CIDR:      "",
+					Action:    string(secrules.SecurityRuleAllow),
+					Protocol:  secrules.PROTO_ANY,
+					Priority:  10,
+				},
+			},
+			acl: &ovn_nb.ACL{
+				Direction: aclDirFromLport,
+				Action:    "allow-related",
+				Match:     fmt.Sprintf("inport == %q && ip4", lport),
+				Priority:  10,
+			},
+		},
+		{
+			// ingress deny all
+			rule: &agentmodels.SecurityGroupRule{
+				SSecurityGroupRule: models.SSecurityGroupRule{
+					Direction: string(secrules.SecurityRuleIngress),
+					CIDR:      "",
+					Action:    string(secrules.SecurityRuleDeny),
+					Protocol:  secrules.PROTO_ANY,
+					Priority:  100,
+				},
+			},
+			acl: &ovn_nb.ACL{
+				Direction: aclDirToLport,
+				Action:    "drop",
+				Match:     fmt.Sprintf("outport == %q && ip4", lport),
+				Priority:  100,
+			},
+		},
+		{
+			// ingress allow ssh
+			rule: &agentmodels.SecurityGroupRule{
+				SSecurityGroupRule: models.SSecurityGroupRule{
+					Direction: string(secrules.SecurityRuleIngress),
+					CIDR:      "",
+					Action:    string(secrules.SecurityRuleAllow),
+					Protocol:  secrules.PROTO_TCP,
+					Ports:     "22",
+					Priority:  100,
+				},
+			},
+			acl: &ovn_nb.ACL{
+				Direction: aclDirToLport,
+				Action:    "allow-related",
+				Match:     fmt.Sprintf("outport == %q && ip4 && tcp && tcp.dst == 22", lport),
+				Priority:  100,
+			},
+		},
+	}
+
+	for _, c := range cases {
+		got, err := ruleToAcl(lport, c.rule)
+		if err != nil {
+			t.Errorf("ruleToACL fail %s", err)
+		} else {
+			if !reflect.DeepEqual(got, c.acl) {
+				t.Errorf("want: %s got: %s", jsonutils.Marshal(c.acl), jsonutils.Marshal(got))
+			}
+		}
+	}
+}