更改sync算法

cmd/climc/shell/secgrouprules.go

@@ -59,7 +59,6 @@ func init() {
 	})
 
 	type SecGroupRulesCreateOptions struct {
-		NAME      string `help:"Name of security group rule to create"`
 		SECGROUP  string `help:"Secgroup ID or Name" metavar:"Secgroup"`
 		Direction string `help:"Direction of rule" choices:"in|out"`
 		Action    string `help:"Action of rule" choices:"allow|deny"`
@@ -72,7 +71,6 @@ func init() {
 
 	R(&SecGroupRulesCreateOptions{}, "secgroup-rule-create", "Create all security group rule", func(s *mcclient.ClientSession, args *SecGroupRulesCreateOptions) error {
 		params := jsonutils.NewDict()
-		params.Add(jsonutils.NewString(args.NAME), "name")
 		if len(args.Desc) > 0 {
 			params.Add(jsonutils.NewString(args.Desc), "description")
 		}

pkg/cloudprovider/resources.go

@@ -144,7 +144,7 @@ type ICloudVM interface {
 	GetBios() string
 	GetMachine() string
 
-	SyncSecurityGroup(secgroupId string, name string, rules []*secrules.SecurityRule) error
+	SyncSecurityGroup(secgroupId string, name string, rules []secrules.SecurityRule) error
 	GetHypervisor() string
 
 	// GetSecurityGroup() ICloudSecurityGroup

pkg/compute/models/guests.go

@@ -1146,20 +1146,20 @@ func (self *SGuest) getAdminSecgroupName() string {
 	return ""
 }
 
-func (self *SGuest) GetSecRules() []*secrules.SecurityRule {
+func (self *SGuest) GetSecRules() []secrules.SecurityRule {
 	return self.getSecRules()
 }
 
-func (self *SGuest) getSecRules() []*secrules.SecurityRule {
+func (self *SGuest) getSecRules() []secrules.SecurityRule {
 	if secgrp := self.getSecgroup(); secgrp != nil {
 		return secgrp.getSecRules()
 	}
 	if rule, err := secrules.ParseSecurityRule(options.Options.DefaultSecurityRules); err == nil {
-		return []*secrules.SecurityRule{rule}
+		return []secrules.SecurityRule{*rule}
 	} else {
 		log.Errorf("Default SecurityRules error: %v", err)
 	}
-	return []*secrules.SecurityRule{}
+	return []secrules.SecurityRule{}
 }
 
 func (self *SGuest) getSecurityRules() string {
@@ -3193,8 +3193,8 @@ func (manager *SGuestManager) getIpsByExit(ips []string, isExitOnly bool) []stri
 	return extRet
 }
 
-func (manager *SGuestManager) getExpiredPendingDeleteGuests() ([]SGuest) {
-	deadline := time.Now().Add(time.Duration(options.Options.PendingDeleteExpireSeconds)*time.Second)
+func (manager *SGuestManager) getExpiredPendingDeleteGuests() []SGuest {
+	deadline := time.Now().Add(time.Duration(options.Options.PendingDeleteExpireSeconds) * time.Second)
 
 	q := manager.Query()
 	q = q.IsTrue("pending_deleted").LT("pending_deleted_at", deadline).In("hypervisor", []string{"aliyun"}).Limit(options.Options.PendingDeleteMaxCleanBatchSize)

pkg/compute/models/secgrouprules.go

@@ -3,6 +3,7 @@ package models
 import (
 	"context"
 	"fmt"
+	"sort"
 	"strconv"
 	"strings"
 
@@ -13,7 +14,6 @@ import (
 	"yunion.io/x/onecloud/pkg/mcclient"
 	"yunion.io/x/pkg/util/compare"
 	"yunion.io/x/pkg/util/secrules"
-	"yunion.io/x/pkg/util/sets"
 	"yunion.io/x/pkg/util/stringutils"
 	"yunion.io/x/sqlchemy"
 )
@@ -41,6 +41,25 @@ type SSecurityGroupRule struct {
 	SecgroupID  string `width:"128" charset:"ascii" create:"required"`
 }
 
+type SecurityGroupRuleSet []SSecurityGroupRule
+
+func (v SecurityGroupRuleSet) Len() int {
+	return len(v)
+}
+
+func (v SecurityGroupRuleSet) Swap(i, j int) {
+	v[i], v[j] = v[j], v[i]
+}
+
+func (v SecurityGroupRuleSet) Less(i, j int) bool {
+	if v[i].Priority < v[j].Priority {
+		return true
+	} else if v[i].Priority == v[j].Priority {
+		return strings.Compare(v[i].String(), v[j].String()) <= 0
+	}
+	return false
+}
+
 func (manager *SSecurityGroupRuleManager) AllowCreateItem(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, data jsonutils.JSONObject) bool {
 	return true
 }
@@ -133,6 +152,8 @@ func (manager *SSecurityGroupRuleManager) ValidateCreateData(
 				key += ":"
 			}
 			fields = append(fields, key)
+		} else if field == "cidr" {
+			data.Add(jsonutils.NewString("0.0.0.0/0"), "cidr")
 		}
 	}
 	if _, err := secrules.ParseSecurityRule(strings.Join(fields, " ")); err != nil {
@@ -187,7 +208,7 @@ func (self *SSecurityGroupRule) ValidateUpdateData(ctx context.Context, userCred
 	return self.SResourceBase.ValidateUpdateData(ctx, userCred, query, data)
 }
 
-func (self *SSecurityGroupRule) GetRule() string {
+func (self *SSecurityGroupRule) String() string {
 	var fields []string
 	for _, field := range []string{"direction", "action", "cidr", "protocol", "ports"} {
 		switch field {
@@ -199,7 +220,7 @@ func (self *SSecurityGroupRule) GetRule() string {
 		case "action":
 			fields = append(fields, self.Action)
 		case "cidr":
-			if len(self.CIDR) > 0 {
+			if len(self.CIDR) > 0 && self.CIDR != "0.0.0.0/0" {
 				fields = append(fields, self.CIDR)
 			}
 		case "protocol":
@@ -217,6 +238,43 @@ func (self *SSecurityGroupRule) GetRule() string {
 	return fields[0] + strings.Join(fields[1:], " ")
 }
 
+func (self *SSecurityGroupRule) SingleRules() ([]secrules.SecurityRule, error) {
+	rules := make([]secrules.SecurityRule, 0)
+	ruleStr := self.String()
+	if rule, err := secrules.ParseSecurityRule(ruleStr); err != nil {
+		return nil, err
+	} else if len(rule.Ports) > 0 {
+		for _, port := range rule.Ports {
+			_rule := secrules.SecurityRule{
+				Priority:    int(self.Priority),
+				Action:      rule.Action,
+				IPNet:       rule.IPNet,
+				Protocol:    rule.Protocol,
+				Direction:   rule.Direction,
+				PortStart:   -1,
+				PortEnd:     -1,
+				Ports:       []int{port},
+				Description: self.Description,
+			}
+			rules = append(rules, _rule)
+		}
+	} else {
+		_rule := secrules.SecurityRule{
+			Priority:    int(self.Priority),
+			Action:      rule.Action,
+			IPNet:       rule.IPNet,
+			Protocol:    rule.Protocol,
+			Direction:   rule.Direction,
+			PortStart:   rule.PortStart,
+			PortEnd:     rule.PortEnd,
+			Ports:       []int{},
+			Description: self.Description,
+		}
+		rules = append(rules, _rule)
+	}
+	return rules, nil
+}
+
 func (self *SSecurityGroupRule) PostCreate(ctx context.Context, userCred mcclient.TokenCredential, ownerProjId string, query jsonutils.JSONObject, data jsonutils.JSONObject) {
 	self.SResourceBase.PostCreate(ctx, userCred, ownerProjId, query, data)
 
@@ -255,50 +313,59 @@ func (manager *SSecurityGroupRuleManager) getRulesBySecurityGroup(secgroup *SSec
 func (manager *SSecurityGroupRuleManager) SyncRules(ctx context.Context, userCred mcclient.TokenCredential, secgroup *SSecurityGroup, rules []secrules.SecurityRule) ([]SSecurityGroupRule, []SSecurityGroupRule, compare.SyncResult) {
 	syncResult := compare.SyncResult{}
 
-	if _dbRules, err := manager.getRulesBySecurityGroup(secgroup); err != nil {
+	if dbRules, err := manager.getRulesBySecurityGroup(secgroup); err != nil {
 		return nil, nil, syncResult
 	} else {
-		dbRules := make([]secrules.SecurityRule, len(_dbRules))
 
-		originRules := make(map[string]*SSecurityGroupRule, len(dbRules))
-		oldRules, oldStrs := make(map[string]secrules.SecurityRule, len(dbRules)), sets.NewString()
+		sort.Sort(SecurityGroupRuleSet(dbRules))
+		sort.Sort(secrules.SecurityRuleSet(rules))
 
-		for i := 0; i < len(_dbRules); i += 1 {
-			_rule := _dbRules[i]
-			if rule, err := secrules.ParseSecurityRule(_rule.GetRule()); err != nil {
-				syncResult.AddError(err)
-			} else {
-				rule.Priority = int(_rule.Priority)
-				rule.Description = _rule.Description
-				if str := jsonutils.Marshal(rule).String(); !oldStrs.Has(str) {
-					oldStrs.Insert(str)
-					oldRules[str] = *rule
-					originRules[str] = &_rule
-				} else if err := _rule.Delete(ctx, userCred); err != nil {
-					syncResult.AddError(err)
+		i, j := 0, 0
+		for i < len(rules) || j < len(dbRules) {
+			if i < len(rules) && j < len(dbRules) {
+				dbStr := dbRules[j].String()
+				ruleStr := rules[i].String()
+				cmp := strings.Compare(dbStr, ruleStr)
+				if cmp == 0 {
+					if dbRules[j].Description != rules[i].Description {
+						if _, err := manager.TableSpec().Update(dbRules[j], func() error {
+							dbRules[j].Description = rules[i].Description
+							return nil
+						}); err != nil {
+							log.Errorf("Update SecurityGroupRule failed: %v", err)
+						}
+					}
+					i += 1
+					j += 1
+				} else if cmp > 0 {
+					if err := dbRules[j].Delete(ctx, userCred); err != nil {
+						syncResult.AddError(err)
+					} else {
+						syncResult.Delete()
+					}
+					j += 1
+				} else {
+					if _, err := manager.newFromCloudSecurityGroup(rules[i], secgroup); err != nil {
+						syncResult.AddError(err)
+					} else {
+						syncResult.Add()
+					}
+					i += 1
 				}
-			}
-		}
-
-		newRules, newStrs := make(map[string]secrules.SecurityRule, len(rules)), sets.NewString()
-		for _, rule := range rules {
-			if str := jsonutils.Marshal(rule).String(); !newStrs.Has(str) {
-				newStrs.Insert(str)
-				newRules[str] = rule
-			}
-		}
-		for _, _rule := range newStrs.Difference(oldStrs).List() {
-			rule := newRules[_rule]
-			if _, err := manager.newFromCloudSecurityGroup(rule, secgroup); err != nil {
-				syncResult.AddError(err)
-			} else {
-				syncResult.Add()
-			}
-		}
-		for _, _rule := range oldStrs.Difference(newStrs).List() {
-			syncResult.Delete()
-			if err := originRules[_rule].Delete(ctx, userCred); err != nil {
-				syncResult.AddError(err)
+			} else if i >= len(rules) {
+				if err := dbRules[j].Delete(ctx, userCred); err != nil {
+					syncResult.AddError(err)
+				} else {
+					syncResult.Delete()
+				}
+				j += 1
+			} else if j >= len(dbRules) {
+				if _, err := manager.newFromCloudSecurityGroup(rules[i], secgroup); err != nil {
+					syncResult.AddError(err)
+				} else {
+					syncResult.Add()
+				}
+				i += 1
 			}
 		}
 	}

pkg/compute/models/secgroups.go

@@ -99,17 +99,19 @@ func (self *SSecurityGroup) getSecurityRules() (rules []SSecurityGroupRule) {
 	sql := secgrouprules.Query().Filter(sqlchemy.Equals(secgrouprules.Field("secgroup_id"), self.Id))
 	if err := db.FetchModelObjects(SecurityGroupRuleManager, sql, &rules); err != nil {
 		log.Errorf("GetGuests fail %s", err)
-		return nil
+		return
 	}
 	return
 }
 
-func (self *SSecurityGroup) getSecRules() []*secrules.SecurityRule {
-	rules := make([]*secrules.SecurityRule, 0)
-	for _, rule := range self.getSecurityRules() {
-		r, _ := secrules.ParseSecurityRule(rule.GetRule())
-		r.Priority = int(rule.Priority)
-		rules = append(rules, r)
+func (self *SSecurityGroup) getSecRules() []secrules.SecurityRule {
+	rules := make([]secrules.SecurityRule, 0)
+	for _, _rule := range self.getSecurityRules() {
+		singleRules, err := _rule.SingleRules()
+		if err != nil {
+			log.Errorf(err.Error())
+		}
+		rules = append(rules, singleRules...)
 	}
 	return rules
 }
@@ -118,7 +120,7 @@ func (self *SSecurityGroup) getSecurityRuleString() string {
 	secgrouprules := self.getSecurityRules()
 	var rules []string
 	for _, rule := range secgrouprules {
-		rules = append(rules, rule.GetRule())
+		rules = append(rules, rule.String())
 	}
 	return strings.Join(rules, SECURITY_GROUP_SEPARATOR)
 }
@@ -202,22 +204,32 @@ func (manager *SSecurityGroupManager) SyncSecgroups(ctx context.Context, userCre
 		}
 
 		for i := 0; i < len(commondb); i += 1 {
-			if err = commondb[i].SyncWithCloudSecurityGroup(userCred, commonext[i]); err != nil {
-				syncResult.UpdateError(err)
-			} else {
-				localSecgroups = append(localSecgroups, commondb[i])
-				remoteSecgroups = append(remoteSecgroups, commonext[i])
-				syncResult.Update()
+			if rules, err := commonext[i].GetRules(); err != nil {
+				syncResult.Error(err)
+			} else if len(rules) > 0 {
+				if err = commondb[i].SyncWithCloudSecurityGroup(userCred, commonext[i]); err != nil {
+					syncResult.UpdateError(err)
+				} else {
+					localSecgroups = append(localSecgroups, commondb[i])
+					remoteSecgroups = append(remoteSecgroups, commonext[i])
+					SecurityGroupRuleManager.SyncRules(ctx, userCred, &commondb[i], rules)
+					syncResult.Update()
+				}
 			}
 		}
 
 		for i := 0; i < len(added); i += 1 {
-			if new, err := manager.newFromCloudVpc(added[i]); err != nil {
+			if rules, err := added[i].GetRules(); err != nil {
 				syncResult.AddError(err)
-			} else {
-				localSecgroups = append(localSecgroups, *new)
-				remoteSecgroups = append(remoteSecgroups, added[i])
-				syncResult.Add()
+			} else if len(rules) > 0 {
+				if new, err := manager.newFromCloudVpc(added[i]); err != nil {
+					syncResult.AddError(err)
+				} else {
+					localSecgroups = append(localSecgroups, *new)
+					remoteSecgroups = append(remoteSecgroups, added[i])
+					SecurityGroupRuleManager.SyncRules(ctx, userCred, new, rules)
+					syncResult.Add()
+				}
 			}
 		}
 	}

pkg/compute/tasks/cloud_provider_sync_info_task.go

@@ -152,32 +152,13 @@ func syncVpcSecGroup(ctx context.Context, provider *models.SCloudprovider, task
 		logSyncFailed(provider, task, msg)
 		return
 	} else {
-		localSecgroups, removeSecgroups, result := models.SecurityGroupManager.SyncSecgroups(ctx, task.UserCred, secgroups)
+		_, _, result := models.SecurityGroupManager.SyncSecgroups(ctx, task.UserCred, secgroups)
 		msg := result.Result()
 		log.Infof("SyncSecurityGroup for VPC %s result: %s", localVpc.Name, msg)
 		if result.IsError() {
 			logSyncFailed(provider, task, msg)
 			return
 		}
-		//db.OpsLog.LogEvent(provider, db.ACT_SYNC_HOST_COMPLETE, msg, task.GetUserCred())
-		for i := 0; i < len(localSecgroups); i += 1 {
-			syncSecgroupRules(ctx, provider, task, &localSecgroups[i], removeSecgroups[i])
-		}
-	}
-}
-
-func syncSecgroupRules(ctx context.Context, provider *models.SCloudprovider, task *CloudProviderSyncInfoTask, localSecgroup *models.SSecurityGroup, remoteSecgroup cloudprovider.ICloudSecurityGroup) {
-	rules, err := remoteSecgroup.GetRules()
-	if err != nil {
-		logSyncFailed(provider, task, err.Error())
-		return
-	}
-	_, _, result := models.SecurityGroupRuleManager.SyncRules(ctx, task.UserCred, localSecgroup, rules)
-	msg := result.Result()
-	log.Infof("SyncRules for secgroups %s result: %s", localSecgroup.Name, msg)
-	if result.IsError() {
-		logSyncFailed(provider, task, msg)
-		return
 	}
 }
 

pkg/util/aliyun/instance.go

@@ -514,7 +514,7 @@ func (self *SInstance) GetVNCInfo() (jsonutils.JSONObject, error) {
 	return ret, nil
 }
 
-func (self *SInstance) SyncSecurityGroup(secgroupId string, name string, rules []*secrules.SecurityRule) error {
+func (self *SInstance) SyncSecurityGroup(secgroupId string, name string, rules []secrules.SecurityRule) error {
 	if vpc, err := self.getVpc(); err != nil {
 		return err
 	} else if len(secgroupId) == 0 {

pkg/util/aliyun/securitygroup.go

@@ -2,6 +2,7 @@ package aliyun
 
 import (
 	"fmt"
+	"sort"
 	"strings"
 	"time"
 
@@ -9,7 +10,6 @@ import (
 	"yunion.io/x/log"
 	"yunion.io/x/onecloud/pkg/httperrors"
 	"yunion.io/x/pkg/util/secrules"
-	"yunion.io/x/pkg/util/sets"
 	"yunion.io/x/pkg/utils"
 )
 
@@ -58,6 +58,25 @@ type SSecurityGroup struct {
 	RegionId          string
 }
 
+type PermissionSet []SPermission
+
+func (v PermissionSet) Len() int {
+	return len(v)
+}
+
+func (v PermissionSet) Swap(i, j int) {
+	v[i], v[j] = v[j], v[i]
+}
+
+func (v PermissionSet) Less(i, j int) bool {
+	if v[i].Priority < v[j].Priority {
+		return true
+	} else if v[i].Priority == v[j].Priority {
+		return strings.Compare(v[i].String(), v[j].String()) <= 0
+	}
+	return false
+}
+
 func (self *SSecurityGroup) GetId() string {
 	return self.SecurityGroupId
 }
@@ -76,7 +95,7 @@ func (self *SSecurityGroup) GetRules() ([]secrules.SecurityRule, error) {
 		return rules, err
 	} else {
 		for _, permission := range secgrp.Permissions.Permission {
-			if rule, err := secrules.ParseSecurityRule(permission.toString()); err != nil {
+			if rule, err := secrules.ParseSecurityRule(permission.String()); err != nil {
 				return rules, err
 			} else {
 				priority := permission.Priority
@@ -184,6 +203,52 @@ func (self *SRegion) createSecurityGroup(vpcId string, name string, desc string)
 	return body.GetString("SecurityGroupId")
 }
 
+func (self *SRegion) modifySecurityGroupRule(secGrpId string, rule *secrules.SecurityRule) error {
+	params := make(map[string]string)
+	params["RegionId"] = self.RegionId
+	params["SecurityGroupId"] = secGrpId
+	params["NicType"] = string(IntranetNicType)
+	params["Description"] = rule.Description
+	params["PortRange"] = fmt.Sprintf("%d/%d", rule.PortStart, rule.PortEnd)
+	protocol := rule.Protocol
+	if len(rule.Protocol) == 0 || rule.Protocol == secrules.PROTO_ANY {
+		protocol = "all"
+	}
+	params["IpProtocol"] = protocol
+	if rule.PortStart == 0 && rule.PortEnd == 0 {
+		if protocol == "udp" || protocol == "tcp" {
+			params["PortRange"] = "1/65535"
+		} else {
+			params["PortRange"] = "-1/-1"
+		}
+	}
+	if rule.Action == secrules.SecurityRuleAllow {
+		params["Policy"] = "accept"
+	} else {
+		params["Policy"] = "drop"
+	}
+	params["Priority"] = fmt.Sprintf("%d", rule.Priority)
+	if rule.Direction == secrules.SecurityRuleIngress {
+		if rule.IPNet != nil {
+			params["SourceCidrIp"] = rule.IPNet.String()
+		} else {
+			params["SourceCidrIp"] = "0.0.0.0/0"
+		}
+		_, err := self.ecsRequest("ModifySecurityGroupRule", params)
+		return err
+	} else { // rule.Direction == secrules.SecurityRuleEgress {
+		//阿里云不支持出方向API接口调用
+		return nil
+		// if rule.IPNet != nil {
+		// 	params["DestCidrIp"] = rule.IPNet.String()
+		// } else {
+		// 	params["DestCidrIp"] = "0.0.0.0/0"
+		// }
+		// _, err := self.ecsRequest("ModifySecurityGroupRule", params)
+		// return err
+	}
+}
+
 func (self *SRegion) modifySecurityGroup(secGrpId string, name string, desc string) error {
 	params := make(map[string]string)
 	params["RegionId"] = self.RegionId
@@ -196,25 +261,45 @@ func (self *SRegion) modifySecurityGroup(secGrpId string, name string, desc stri
 	return err
 }
 
+func (self *SRegion) addSecurityGroupRules(secGrpId string, rule *secrules.SecurityRule) error {
+	if len(rule.Ports) != 0 {
+		for _, port := range rule.Ports {
+			rule.PortStart, rule.PortEnd = port, port
+			if err := self.addSecurityGroupRule(secGrpId, rule); err != nil {
+				return err
+			}
+		}
+	} else {
+		return self.addSecurityGroupRule(secGrpId, rule)
+	}
+	return nil
+}
+
 func (self *SRegion) addSecurityGroupRule(secGrpId string, rule *secrules.SecurityRule) error {
 	params := make(map[string]string)
 	params["RegionId"] = self.RegionId
 	params["SecurityGroupId"] = secGrpId
 	params["NicType"] = string(IntranetNicType)
-	params["PortRange"] = fmt.Sprintf("%d/%d", rule.PortStart, rule.PortEnd)
 	params["Description"] = rule.Description
+	params["PortRange"] = fmt.Sprintf("%d/%d", rule.PortStart, rule.PortEnd)
 	protocol := rule.Protocol
 	if len(rule.Protocol) == 0 || rule.Protocol == secrules.PROTO_ANY {
 		protocol = "all"
-		params["PortRange"] = "-1/-1"
 	}
 	params["IpProtocol"] = protocol
+	if rule.PortStart == 0 && rule.PortEnd == 0 {
+		if protocol == "udp" || protocol == "tcp" {
+			params["PortRange"] = "1/65535"
+		} else {
+			params["PortRange"] = "-1/-1"
+		}
+	}
 	if rule.Action == secrules.SecurityRuleAllow {
 		params["Policy"] = "accept"
 	} else {
 		params["Policy"] = "drop"
 	}
-	params["Priority"] = fmt.Sprintf("%d", rule.Priority)
+	params["Priority"] = fmt.Sprintf("%d", 101-rule.Priority)
 	if rule.Direction == secrules.SecurityRuleIngress {
 		if rule.IPNet != nil {
 			params["SourceCidrIp"] = rule.IPNet.String()
@@ -243,9 +328,15 @@ func (self *SRegion) delSecurityGroupRule(secGrpId string, rule *secrules.Securi
 	protocol := rule.Protocol
 	if len(rule.Protocol) == 0 || rule.Protocol == secrules.PROTO_ANY {
 		protocol = "all"
-		params["PortRange"] = "-1/-1"
 	}
 	params["IpProtocol"] = protocol
+	if rule.PortStart == 0 && rule.PortEnd == 0 {
+		if protocol == "udp" || protocol == "tcp" {
+			params["PortRange"] = "1/65535"
+		} else {
+			params["PortRange"] = "-1/-1"
+		}
+	}
 	if rule.Action == secrules.SecurityRuleAllow {
 		params["Policy"] = "accept"
 	} else {
@@ -284,7 +375,7 @@ func (self *SRegion) createDefaultSecurityGroup(vpcId string) (string, error) {
 		PortStart: -1,
 		PortEnd:   -1,
 	}
-	err = self.addSecurityGroupRule(secId, &inRule)
+	err = self.addSecurityGroupRules(secId, &inRule)
 	if err != nil {
 		return "", err
 	}
@@ -296,7 +387,7 @@ func (self *SRegion) createDefaultSecurityGroup(vpcId string) (string, error) {
 		PortStart: -1,
 		PortEnd:   -1,
 	}
-	err = self.addSecurityGroupRule(secId, &outRule)
+	err = self.addSecurityGroupRules(secId, &outRule)
 	if err != nil {
 		return "", err
 	}
@@ -324,10 +415,6 @@ func (self *SRegion) getSecurityGroupByTag(vpcId, secgroupId string) (*SSecurity
 }
 
 func (self *SPermission) String() string {
-	return self.toString()
-}
-
-func (self *SPermission) toString() string {
 	action := secrules.SecurityRuleDeny
 	if strings.ToLower(self.Policy) == "accept" {
 		action = secrules.SecurityRuleAllow
@@ -340,17 +427,30 @@ func (self *SPermission) toString() string {
 	if direction == "out" {
 		cidr = self.DestCidrIp
 	}
+	if cidr == "0.0.0.0/0" {
+		cidr = ""
+	}
 	protocol := strings.ToLower(self.IpProtocol)
 	if protocol == "all" {
 		protocol = "any"
 	}
 	port, ports := "", strings.Split(self.PortRange, "/")
-	if ports[0] == ports[1] && (ports[0] != "-1") {
-		port = ports[0]
-	} else {
+	if ports[0] == ports[1] {
+		if ports[0] != "-1" {
+			port = ports[0]
+		}
+	} else if ports[0] != "1" && ports[1] != "65535" {
 		port = fmt.Sprintf("%s-%s", ports[0], ports[1])
 	}
-	return fmt.Sprintf("%s:%s %s %s %s", direction, string(action), cidr, protocol, port)
+	result := fmt.Sprintf("%s:%s", direction, string(action))
+	if len(cidr) > 0 {
+		result += fmt.Sprintf(" %s", cidr)
+	}
+	result += fmt.Sprintf(" %s", protocol)
+	if len(port) > 0 {
+		result += fmt.Sprintf(" %s", port)
+	}
+	return result
 }
 
 func (self *SRegion) addTagToSecurityGroup(secgroupId, key, value string, index int) error {
@@ -371,11 +471,11 @@ func (self *SRegion) revokeSecurityGroup(secgroupId, instanceId string, keep boo
 	if secgroup, err := self.GetSecurityGroupDetails(secgroupId); err != nil {
 		return err
 	} else {
-		for _, r := range secgroup.Permissions.Permission {
-			if rule, err := secrules.ParseSecurityRule(r.toString()); err != nil {
+		for _, permission := range secgroup.Permissions.Permission {
+			if rule, err := secrules.ParseSecurityRule(permission.String()); err != nil {
 				return err
 			} else {
-				rule.Priority = r.Priority
+				rule.Priority = permission.Priority
 				if err := self.delSecurityGroupRule(secgroup.SecurityGroupId, rule); err != nil {
 					return err
 				}
@@ -383,13 +483,13 @@ func (self *SRegion) revokeSecurityGroup(secgroupId, instanceId string, keep boo
 		}
 		if rule, err := secrules.ParseSecurityRule("in:allow any"); err != nil {
 			rule.Priority = 100
-			if err := self.addSecurityGroupRule(secgroup.SecurityGroupId, rule); err != nil {
+			if err := self.addSecurityGroupRules(secgroup.SecurityGroupId, rule); err != nil {
 				return err
 			}
 		}
 		if rule, err := secrules.ParseSecurityRule("out:allow any"); err != nil {
 			rule.Priority = 100
-			if err := self.addSecurityGroupRule(secgroup.SecurityGroupId, rule); err != nil {
+			if err := self.addSecurityGroupRules(secgroup.SecurityGroupId, rule); err != nil {
 				return err
 			}
 		}
@@ -397,48 +497,68 @@ func (self *SRegion) revokeSecurityGroup(secgroupId, instanceId string, keep boo
 	return nil
 }
 
-func (self *SRegion) syncSecgroupRules(secgroupId string, rules []*secrules.SecurityRule) error {
+func (self *SRegion) syncSecgroupRules(secgroupId string, rules []secrules.SecurityRule) error {
 	if secgroup, err := self.GetSecurityGroupDetails(secgroupId); err != nil {
 		return err
 	} else {
-		newRules, newStr := make(map[string]*secrules.SecurityRule), sets.NewString()
-		for _, rule := range rules {
-			rule.Priority = 101 - rule.Priority
-			if len(rule.Ports) > 0 {
-				for _, port := range rule.Ports {
-					rule.PortStart, rule.PortEnd = port, port
-					if jsonStr := jsonutils.Marshal(rule).String(); !newStr.Has(jsonStr) {
-						newStr.Insert(jsonStr)
-						newRules[jsonStr] = rule
+
+		sort.Sort(secrules.SecurityRuleSet(rules))
+		sort.Sort(PermissionSet(secgroup.Permissions.Permission))
+
+		i, j := 0, 0
+		for i < len(rules) || j < len(secgroup.Permissions.Permission) {
+			if i < len(rules) && j < len(secgroup.Permissions.Permission) {
+				permissionStr := secgroup.Permissions.Permission[j].String()
+				ruleStr := rules[i].String()
+				cmp := strings.Compare(permissionStr, ruleStr)
+				if cmp == 0 {
+					if secgroup.Permissions.Permission[j].Description != rules[i].Description {
+						rules[i].Priority = secgroup.Permissions.Permission[j].Priority
+						if err := self.modifySecurityGroupRule(secgroupId, &rules[i]); err != nil {
+							log.Errorf("modifySecurityGroupRule error %v", rules[i])
+							return err
+						}
+					}
+					i += 1
+					j += 1
+				} else if cmp > 0 {
+					if rule, err := secrules.ParseSecurityRule(permissionStr); err != nil {
+						return err
+					} else {
+						rule.Priority = secgroup.Permissions.Permission[j].Priority
+						if err := self.delSecurityGroupRule(secgroupId, rule); err != nil {
+							log.Errorf("delSecurityGroupRule error %v", rule)
+							return err
+						}
+					}
+					j += 1
+				} else {
+					if err := self.addSecurityGroupRules(secgroupId, &rules[i]); err != nil {
+						log.Errorf("addSecurityGroupRule error %v", rules[i])
+						return err
+					}
+					i += 1
+				}
+			} else if i >= len(rules) {
+				permissionStr := secgroup.Permissions.Permission[j].String()
+				if rule, err := secrules.ParseSecurityRule(permissionStr); err != nil {
+					return err
+				} else {
+					rule.Priority = secgroup.Permissions.Permission[j].Priority
+					if err := self.delSecurityGroupRule(secgroupId, rule); err != nil {
+						log.Errorf("delSecurityGroupRule error %v", rule)
+						return err
 					}
 				}
-			} else if jsonStr := jsonutils.Marshal(rule).String(); !newStr.Has(jsonStr) {
-				newStr.Insert(jsonStr)
-				newRules[jsonStr] = rule
-			}
-		}
-
-		oldRules, oldStr := make(map[string]*secrules.SecurityRule), sets.NewString()
-		for _, r := range secgroup.Permissions.Permission {
-			if rule, err := secrules.ParseSecurityRule(r.toString()); err != nil {
-				return err
-			} else {
-				rule.Priority = r.Priority
-				rule.Description = r.Description
-				if jsonStr := jsonutils.Marshal(rule).String(); !oldStr.Has(jsonStr) {
-					oldStr.Insert(jsonStr)
-					oldRules[jsonStr] = rule
+				j += 1
+			} else if j >= len(secgroup.Permissions.Permission) {
+				if err := self.addSecurityGroupRules(secgroupId, &rules[i]); err != nil {
+					log.Errorf("addSecurityGroupRule error %v", rules[i])
+					return err
 				}
+				i += 1
 			}
 		}
-		for _, jsonStr := range newStr.Difference(oldStr).List() {
-			rule := newRules[jsonStr]
-			self.addSecurityGroupRule(secgroupId, rule)
-		}
-		for _, jsonStr := range oldStr.Difference(newStr).List() {
-			rule := oldRules[jsonStr]
-			self.delSecurityGroupRule(secgroupId, rule)
-		}
 	}
 	return nil
 }

pkg/util/aliyun/vpc.go

@@ -187,7 +187,7 @@ func (self *SVpc) Delete() error {
 	return self.region.DeleteVpc(self.VpcId)
 }
 
-func (self *SVpc) syncSecurityGroup(secgroupId string, name string, rules []*secrules.SecurityRule) (string, error) {
+func (self *SVpc) syncSecurityGroup(secgroupId string, name string, rules []secrules.SecurityRule) (string, error) {
 	secgrpId := ""
 	if secgroup, err := self.region.getSecurityGroupByTag(self.VpcId, secgroupId); err != nil {
 		if secgrpId, err = self.region.createSecurityGroup(self.VpcId, name, ""); err != nil {
@@ -198,7 +198,7 @@ func (self *SVpc) syncSecurityGroup(secgroupId string, name string, rules []*sec
 		//addRules
 		log.Debugf("Add Rules for %s", secgrpId)
 		for _, rule := range rules {
-			if err := self.region.addSecurityGroupRule(secgrpId, rule); err != nil {
+			if err := self.region.addSecurityGroupRule(secgrpId, &rule); err != nil {
 				return "", err
 			}
 		}

pkg/util/esxi/virtualmachine.go

@@ -35,7 +35,7 @@ func (self *SVirtualMachine) GetGlobalId() string {
 	return self.getUuid()
 }
 
-func (self *SVirtualMachine) SyncSecurityGroup(secgroupId, name string, rules []*secrules.SecurityRule) error {
+func (self *SVirtualMachine) SyncSecurityGroup(secgroupId, name string, rules []secrules.SecurityRule) error {
 	return nil
 }
 

vendor/yunion.io/x/pkg/util/secrules/secrules.go

@@ -63,6 +63,25 @@ var (
 	ErrInvalidPort      = errors.New("invalid port")
 )
 
+type SecurityRuleSet []SecurityRule
+
+func (v SecurityRuleSet) Len() int {
+	return len(v)
+}
+
+func (v SecurityRuleSet) Swap(i, j int) {
+	v[i], v[j] = v[j], v[i]
+}
+
+func (v SecurityRuleSet) Less(i, j int) bool {
+	if v[i].Priority > v[j].Priority {
+		return true
+	} else if v[i].Priority == v[j].Priority {
+		return strings.Compare(v[i].String(), v[j].String()) <= 0
+	}
+	return false
+}
+
 func parsePortString(ps string) (int, error) {
 	p, err := strconv.ParseUint(ps, 10, 16)
 	if err != nil || p == 0 {