Merge pull request #6731 from ioito/automated-cherry-pick-of-#6663-upstream-release-3.2

Automated cherry pick of #6663: fix: 优化安全组同步逻辑

pkg/cloudprovider/resources.go

@@ -20,7 +20,6 @@ import (
 
 	"yunion.io/x/jsonutils"
 	"yunion.io/x/pkg/tristate"
-	"yunion.io/x/pkg/util/secrules"
 
 	"yunion.io/x/onecloud/pkg/mcclient"
 	"yunion.io/x/onecloud/pkg/util/billing"
@@ -346,10 +345,10 @@ type ICloudSecurityGroup interface {
 	ICloudResource
 
 	GetDescription() string
-	GetRules() ([]secrules.SecurityRule, error)
+	GetRules() ([]SecurityRule, error)
 	GetVpcId() string
 
-	SyncRules(rules []secrules.SecurityRule) error
+	SyncRules(common, inAdds, outAdds, inDels, outDels []SecurityRule) error
 	Delete() error
 }
 

pkg/cloudprovider/securitygroup.go

@@ -14,7 +14,15 @@
 
 package cloudprovider
 
-import "yunion.io/x/pkg/util/secrules"
+import (
+	"sort"
+	"strings"
+
+	"yunion.io/x/log"
+	"yunion.io/x/pkg/util/secrules"
+)
+
+const DEFAULT_CLOUD_RULE_ID = "default_cloud_rule_id"
 
 type SecurityGroupCreateInput struct {
 	Name  string
@@ -22,3 +30,268 @@ type SecurityGroupCreateInput struct {
 	VpcId string
 	Rules []secrules.SecurityRule
 }
+
+type SecurityRule struct {
+	secrules.SecurityRule
+	Name       string
+	ExternalId string
+}
+
+type TPriorityOrder int
+
+var (
+	PriorityOrderByDesc = TPriorityOrder(1)
+	PriorityOrderByAsc  = TPriorityOrder(-1)
+)
+
+func (r SecurityRule) String() string {
+	return r.SecurityRule.String()
+}
+
+type SecurityRuleSet []SecurityRule
+
+func (srs SecurityRuleSet) Len() int {
+	return len(srs)
+}
+
+func (srs SecurityRuleSet) Swap(i, j int) {
+	srs[i], srs[j] = srs[j], srs[i]
+}
+
+func (srs SecurityRuleSet) Less(i, j int) bool {
+	return srs[i].Priority < srs[j].Priority || (srs[i].Priority == srs[j].Priority && srs[i].String() < srs[j].String())
+}
+
+func (srs SecurityRuleSet) AllowList() secrules.SecurityRuleSet {
+	rules := secrules.SecurityRuleSet{}
+	for _, r := range srs {
+		rules = append(rules, r.SecurityRule)
+	}
+	return rules.AllowList()
+}
+
+func AddDefaultRule(rules []SecurityRule, defaultRule SecurityRule, localRuleStr string, order TPriorityOrder, min, max int, onlyAllowRules bool) []SecurityRule {
+	if defaultRule.String() == localRuleStr {
+		return rules
+	}
+	defaultRule.ExternalId = DEFAULT_CLOUD_RULE_ID
+	if order == PriorityOrderByDesc {
+		defaultRule.Priority = min
+	} else {
+		defaultRule.Priority = max
+	}
+	defaultRule.Priority -= int(order)
+	if onlyAllowRules {
+		defaultRule.Priority = -1
+	}
+	return append(rules, defaultRule)
+}
+
+func SortSecurityRule(rules SecurityRuleSet, order TPriorityOrder, onlyAllowRules bool) {
+	if onlyAllowRules {
+		sort.Sort(rules)
+		return
+	}
+	if order == PriorityOrderByAsc {
+		sort.Sort(sort.Reverse(rules))
+		return
+	}
+	sort.Sort(rules)
+}
+
+func CompareRules(
+	minPriority, maxPriority int, order TPriorityOrder,
+	localRules secrules.SecurityRuleSet, remoteRules []SecurityRule,
+	defaultInRule, defaultOutRule SecurityRule,
+	onlyAllowRules bool, debug bool,
+) (common, inAdds, outAdds, inDels, outDels []SecurityRule) {
+	localInRules := secrules.SecurityRuleSet{}
+	localOutRules := secrules.SecurityRuleSet{}
+	for i := range localRules {
+		if localRules[i].Direction == secrules.DIR_IN {
+			localInRules = append(localInRules, localRules[i])
+		} else {
+			localOutRules = append(localOutRules, localRules[i])
+		}
+	}
+	inRules := SecurityRuleSet{}
+	outRules := SecurityRuleSet{}
+	for i := 0; i < len(remoteRules); i++ {
+		if remoteRules[i].Direction == secrules.DIR_IN {
+			inRules = append(inRules, remoteRules[i])
+		} else {
+			outRules = append(outRules, remoteRules[i])
+		}
+	}
+	var inCommon, outCommon = inRules, outRules
+
+	defaultLocalInRule := *secrules.MustParseSecurityRule("in:deny any")
+	defaultLocalOutRule := *secrules.MustParseSecurityRule("out:allow any")
+
+	inRules = AddDefaultRule(inRules, defaultInRule, defaultLocalInRule.String(), order, minPriority, maxPriority, onlyAllowRules)
+	outRules = AddDefaultRule(outRules, defaultOutRule, defaultLocalOutRule.String(), order, minPriority, maxPriority, onlyAllowRules)
+
+	if defaultLocalInRule.String() != defaultInRule.String() {
+		localInRules = append(localInRules, defaultLocalInRule)
+	}
+	if defaultLocalOutRule.String() != defaultOutRule.String() {
+		localOutRules = append(localOutRules, defaultLocalOutRule)
+	}
+
+	sort.Sort(localInRules)
+	sort.Sort(localOutRules)
+
+	localInAllowList := localInRules.AllowList()
+	localOutAllowList := localOutRules.AllowList()
+	if onlyAllowRules {
+		localInRules = localInAllowList
+		localOutRules = localOutAllowList
+	}
+
+	SortSecurityRule(inRules, order, onlyAllowRules)
+	SortSecurityRule(outRules, order, onlyAllowRules)
+
+	inAllowList := inRules.AllowList()
+	outAllowList := outRules.AllowList()
+	inEquals, outEquals := inAllowList.Equals(localInAllowList), outAllowList.Equals(localOutAllowList)
+	if inEquals && outEquals {
+		return
+	}
+
+	// priority从小到大排列(从默认规则开始对比)
+	sort.Sort(sort.Reverse(localInRules))
+	sort.Sort(sort.Reverse(localOutRules))
+
+	sort.Sort(sort.Reverse(inRules))
+	sort.Sort(sort.Reverse(outRules))
+
+	startPriority := minPriority - 1
+	if order == PriorityOrderByAsc {
+		startPriority = maxPriority + 1
+	}
+
+	var addPriority = func(priority int, order TPriorityOrder, inc int, min, max int, onlyAllowRules bool) int {
+		if onlyAllowRules {
+			return 0
+		}
+		inc = inc * int(order) //+ int(order)
+		priority += inc
+		if priority < min {
+			return min
+		}
+		if priority > max {
+			return max
+		}
+		return priority
+	}
+
+	var getInitPriority = func(init, min, max int) int {
+		if init < min || init > max {
+			return (min + max) / 2
+		}
+		return init
+	}
+
+	var compare = func(localRules secrules.SecurityRuleSet, remoteRules SecurityRuleSet) (common, add, del []SecurityRule) {
+		i, j, inc, prePriority := 0, 0, 1, 0
+		for i < len(localRules) || j < len(remoteRules) {
+			if i < len(localRules) && j < len(remoteRules) {
+				ruleStr := remoteRules[j].String()
+				localRuleStr := localRules[i].String()
+				if debug {
+					log.Debugf("compare local priority(%d) %s -> remote name(%s) priority(%d) %s\n", localRules[i].Priority, localRules[i].String(), remoteRules[j].Name, remoteRules[j].Priority, remoteRules[j].String())
+				}
+				cmp := strings.Compare(ruleStr, localRuleStr)
+				if cmp == 0 {
+					prePriority = remoteRules[j].Priority
+					if remoteRules[j].ExternalId == DEFAULT_CLOUD_RULE_ID {
+						remoteRules[j].Priority = addPriority(remoteRules[j].Priority, order, 1, minPriority, maxPriority, onlyAllowRules)
+					}
+					common = append(common, remoteRules[j])
+					i++
+					j++
+				} else if cmp < 0 {
+					if remoteRules[j].ExternalId != DEFAULT_CLOUD_RULE_ID {
+						del = append(del, remoteRules[j])
+					}
+					j++
+				} else {
+					initPriority := getInitPriority(prePriority, minPriority, maxPriority)
+					localRules[i].Priority = addPriority(initPriority, order, inc, minPriority, maxPriority, onlyAllowRules)
+					add = append(add, SecurityRule{SecurityRule: localRules[i]})
+					i++
+					inc++
+				}
+			} else if i >= len(localRules) {
+				if remoteRules[j].ExternalId != DEFAULT_CLOUD_RULE_ID {
+					del = append(del, remoteRules[j])
+				}
+				j++
+			} else if j >= len(remoteRules) {
+				initPriority := startPriority
+				if len(remoteRules) > 0 {
+					initPriority = remoteRules[len(remoteRules)-1].Priority
+				}
+				initPriority = getInitPriority(initPriority, minPriority, maxPriority) // 若是初始添加规则，尽量以中间为节点，避免仅出现天地规则
+				localRules[i].Priority = addPriority(initPriority, order, inc, minPriority, maxPriority, onlyAllowRules)
+				add = append(add, SecurityRule{SecurityRule: localRules[i]})
+				i++
+				inc++
+			}
+		}
+		return
+	}
+
+	type rulePair struct {
+		localRules  []secrules.SecurityRule
+		remoteRules []SecurityRule
+		protocol    string
+	}
+
+	var splitRules = func(localRules []secrules.SecurityRule, remoteRules []SecurityRule) []rulePair {
+		rules := map[string]rulePair{}
+		for _, r := range localRules {
+			pair, ok := rules[r.Protocol]
+			if !ok {
+				pair = rulePair{localRules: []secrules.SecurityRule{}, remoteRules: []SecurityRule{}, protocol: r.Protocol}
+			}
+			pair.localRules = append(pair.localRules, r)
+			rules[r.Protocol] = pair
+		}
+
+		for _, r := range remoteRules {
+			pair, ok := rules[r.Protocol]
+			if !ok {
+				pair = rulePair{localRules: []secrules.SecurityRule{}, remoteRules: []SecurityRule{}, protocol: r.Protocol}
+			}
+			pair.remoteRules = append(pair.remoteRules, r)
+			rules[r.Protocol] = pair
+		}
+
+		ret := []rulePair{}
+		for _, r := range rules {
+			ret = append(ret, r)
+		}
+		return ret
+	}
+
+	var compareRules = func(localRules []secrules.SecurityRule, remoteRules []SecurityRule) (common, add, dels []SecurityRule) {
+		pairs := splitRules(localRules, remoteRules)
+		for _, r := range pairs {
+			_common, _add, _dels := compare(r.localRules, r.remoteRules)
+			common = append(common, _common...)
+			add = append(add, _add...)
+			dels = append(dels, _dels...)
+		}
+		return
+	}
+
+	if !inEquals {
+		inCommon, inAdds, inDels = compareRules(localInRules, inRules)
+	}
+	if !outEquals {
+		outCommon, outAdds, outDels = compareRules(localOutRules, outRules)
+	}
+	common = append(inCommon, outCommon...)
+	return
+}

pkg/compute/models/cloudproviders.go

@@ -1405,6 +1405,14 @@ func (self *SCloudprovider) StartCloudproviderDeleteTask(ctx context.Context, us
 	return nil
 }
 
+func (self *SCloudprovider) GetRegionDriver() (IRegionDriver, error) {
+	driver := GetRegionDriver(self.Provider)
+	if driver == nil {
+		return nil, fmt.Errorf("failed to found region driver for %s", self.Provider)
+	}
+	return driver, nil
+}
+
 func (self *SCloudprovider) ClearSchedDescCache() error {
 	hosts := make([]SHost, 0)
 	q := HostManager.Query().Equals("manager_id", self.Id)

pkg/compute/models/regiondrivers.go

@@ -119,6 +119,12 @@ type IRegionDriver interface {
 
 	RequestCacheSecurityGroup(ctx context.Context, userCred mcclient.TokenCredential, region *SCloudregion, vpc *SVpc, secgroup *SSecurityGroup, classic bool, task taskman.ITask) error
 	RequestSyncSecurityGroup(ctx context.Context, userCred mcclient.TokenCredential, vpcId string, vpc *SVpc, secgroup *SSecurityGroup) (string, error)
+	GetSecurityGroupRuleOrder() cloudprovider.TPriorityOrder // Desc(priority值越大,优先级越高) Asc(priority值越小,优先级越高)
+	GetDefaultSecurityGroupInRule() cloudprovider.SecurityRule
+	GetDefaultSecurityGroupOutRule() cloudprovider.SecurityRule
+	GetSecurityGroupRuleMaxPriority() int
+	GetSecurityGroupRuleMinPriority() int
+	IsOnlySupportAllowRules() bool
 	IsSupportClassicSecurityGroup() bool
 	IsSecurityGroupBelongVpc() bool
 	IsVpcBelongGlobalVpc() bool

pkg/compute/models/secgrouprules.go

@@ -33,6 +33,7 @@ import (
 	"yunion.io/x/onecloud/pkg/cloudcommon/db"
 	"yunion.io/x/onecloud/pkg/cloudcommon/db/lockman"
 	"yunion.io/x/onecloud/pkg/cloudcommon/validators"
+	"yunion.io/x/onecloud/pkg/cloudprovider"
 	"yunion.io/x/onecloud/pkg/httperrors"
 	"yunion.io/x/onecloud/pkg/mcclient"
 	"yunion.io/x/onecloud/pkg/util/logclient"
@@ -454,13 +455,13 @@ func (manager *SSecurityGroupRuleManager) getRulesBySecurityGroup(secgroup *SSec
 	return rules, nil
 }
 
-func (manager *SSecurityGroupRuleManager) SyncRules(ctx context.Context, userCred mcclient.TokenCredential, secgroup *SSecurityGroup, rules secrules.SecurityRuleSet) compare.SyncResult {
+func (manager *SSecurityGroupRuleManager) SyncRules(ctx context.Context, userCred mcclient.TokenCredential, secgroup *SSecurityGroup, rules cloudprovider.SecurityRuleSet) compare.SyncResult {
 	syncResult := compare.SyncResult{}
-	priority, prePriority := 100, 0
+	priority, prePriority := 10, 0
 	for i := 0; i < len(rules); i++ {
 		// 这里避免了Rule规则优先级在 1-100之外的问题,ext.GetRules()不需要进行优先级转换
-		if prePriority != 0 && rules[i].Priority != prePriority && priority > 1 {
-			priority--
+		if prePriority != 0 && rules[i].Priority != prePriority && priority < 100 {
+			priority++
 		}
 		prePriority = rules[i].Priority
 		rules[i].Priority = priority
@@ -474,7 +475,7 @@ func (manager *SSecurityGroupRuleManager) SyncRules(ctx context.Context, userCre
 	return syncResult
 }
 
-func (manager *SSecurityGroupRuleManager) newFromCloudSecurityGroup(ctx context.Context, userCred mcclient.TokenCredential, rule secrules.SecurityRule, secgroup *SSecurityGroup) (*SSecurityGroupRule, error) {
+func (manager *SSecurityGroupRuleManager) newFromCloudSecurityGroup(ctx context.Context, userCred mcclient.TokenCredential, rule cloudprovider.SecurityRule, secgroup *SSecurityGroup) (*SSecurityGroupRule, error) {
 	lockman.LockClass(ctx, manager, db.GetLockClassKey(manager, userCred))
 	defer lockman.ReleaseClass(ctx, manager, db.GetLockClassKey(manager, userCred))
 

pkg/compute/models/secgroups.go

@@ -17,7 +17,6 @@ package models
 import (
 	"context"
 	"database/sql"
-	"sort"
 	"strings"
 	"time"
 
@@ -696,35 +695,46 @@ func (manager *SSecurityGroupManager) getSecurityGroups() ([]SSecurityGroup, err
 }
 
 func (manager *SSecurityGroupManager) newFromCloudSecgroup(ctx context.Context, userCred mcclient.TokenCredential, provider *SCloudprovider, extSec cloudprovider.ICloudSecurityGroup) (*SSecurityGroup, error) {
+	regionDriver, err := provider.GetRegionDriver()
+	if err != nil {
+		return nil, errors.Wrap(err, "provider.GetRegionDriver")
+	}
+
 	rules, err := extSec.GetRules()
 	if err != nil {
-		return nil, err
+		return nil, errors.Wrap(err, "extSec.GetRules")
 	}
-	inRules := secrules.SecurityRuleSet{}
-	outRules := secrules.SecurityRuleSet{}
-	for i := 0; i < len(rules); i++ {
+
+	inRules := []cloudprovider.SecurityRule{}
+	outRules := []cloudprovider.SecurityRule{}
+	for i := range rules {
 		if rules[i].Direction == secrules.DIR_IN {
 			inRules = append(inRules, rules[i])
 		} else {
 			outRules = append(outRules, rules[i])
 		}
 	}
-	sort.Sort(inRules)
-	sort.Sort(outRules)
-	inAllowList := inRules.AllowList()
-	outAllowList := outRules.AllowList()
+
+	maxPriority := regionDriver.GetSecurityGroupRuleMaxPriority()
+	minPriority := regionDriver.GetSecurityGroupRuleMinPriority()
+
+	defaultInRule := regionDriver.GetDefaultSecurityGroupInRule()
+	defaultOutRule := regionDriver.GetDefaultSecurityGroupOutRule()
+	order := regionDriver.GetSecurityGroupRuleOrder()
+	onlyAllowRules := regionDriver.IsOnlySupportAllowRules()
 
 	// 查询与provider在同域的安全组，比对寻找一个与云上安全组规则相同的安全组
 	secgroups := []SSecurityGroup{}
 	q := manager.Query().Equals("domain_id", provider.DomainId)
-	if err := db.FetchModelObjects(manager, q, &secgroups); err != nil {
-		log.Errorf("failed to fetch secgroups %v", err)
+	err = db.FetchModelObjects(manager, q, &secgroups)
+	if err != nil {
+		return nil, errors.Wrap(err, "db.FetchModelObjects")
 	}
-	for _, secgroup := range secgroups {
-		_inAllowList := secgroup.GetInAllowList()
-		_outAllowList := secgroup.GetOutAllowList()
-		if outAllowList.Equals(_outAllowList) && inAllowList.Equals(_inAllowList) {
-			return &secgroup, nil
+	for i := range secgroups {
+		localRules := secrules.SecurityRuleSet(secgroups[i].GetSecRules(""))
+		_, inAdds, outAdds, inDels, outDels := cloudprovider.CompareRules(minPriority, maxPriority, order, localRules, rules, defaultInRule, defaultOutRule, onlyAllowRules, false)
+		if len(inAdds) == 0 && len(outAdds) == 0 && len(inDels) == 0 && len(outDels) == 0 {
+			return &secgroups[i], nil
 		}
 	}
 
@@ -737,6 +747,7 @@ func (manager *SSecurityGroupManager) newFromCloudSecgroup(ctx context.Context,
 	if err != nil {
 		return nil, err
 	}
+
 	secgroup.Name = newName
 	secgroup.Description = extSec.GetDescription()
 	secgroup.ProjectId = provider.ProjectId
@@ -747,6 +758,11 @@ func (manager *SSecurityGroupManager) newFromCloudSecgroup(ctx context.Context,
 	}
 
 	//这里必须先同步下规则,不然下次对比此安全组规则为空
+	inRules = cloudprovider.AddDefaultRule(inRules, defaultInRule, "in:deny any", order, minPriority, maxPriority, onlyAllowRules)
+	cloudprovider.SortSecurityRule(inRules, order, onlyAllowRules)
+	outRules = cloudprovider.AddDefaultRule(outRules, defaultOutRule, "out:allow any", order, minPriority, maxPriority, onlyAllowRules)
+	cloudprovider.SortSecurityRule(outRules, order, onlyAllowRules)
+
 	SecurityGroupRuleManager.SyncRules(ctx, userCred, &secgroup, inRules)
 	SecurityGroupRuleManager.SyncRules(ctx, userCred, &secgroup, outRules)
 

pkg/compute/regiondrivers/aliyun.go

@@ -25,6 +25,7 @@ import (
 	"yunion.io/x/jsonutils"
 	"yunion.io/x/log"
 	"yunion.io/x/pkg/errors"
+	"yunion.io/x/pkg/util/secrules"
 	"yunion.io/x/pkg/utils"
 
 	billing_api "yunion.io/x/onecloud/pkg/apis/billing"
@@ -52,6 +53,26 @@ func init() {
 	models.RegisterRegionDriver(&driver)
 }
 
+func (self *SAliyunRegionDriver) GetSecurityGroupRuleOrder() cloudprovider.TPriorityOrder {
+	return cloudprovider.PriorityOrderByAsc
+}
+
+func (self *SAliyunRegionDriver) GetDefaultSecurityGroupInRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("in:deny any")}
+}
+
+func (self *SAliyunRegionDriver) GetDefaultSecurityGroupOutRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("out:allow any")}
+}
+
+func (self *SAliyunRegionDriver) GetSecurityGroupRuleMaxPriority() int {
+	return 1
+}
+
+func (self *SAliyunRegionDriver) GetSecurityGroupRuleMinPriority() int {
+	return 100
+}
+
 func (self *SAliyunRegionDriver) GetProvider() string {
 	return api.CLOUD_PROVIDER_ALIYUN
 }

pkg/compute/regiondrivers/aws.go

@@ -24,6 +24,7 @@ import (
 
 	"yunion.io/x/jsonutils"
 	"yunion.io/x/pkg/errors"
+	"yunion.io/x/pkg/util/secrules"
 	"yunion.io/x/pkg/utils"
 	"yunion.io/x/sqlchemy"
 
@@ -50,6 +51,30 @@ func init() {
 	models.RegisterRegionDriver(&driver)
 }
 
+func (self *SAwsRegionDriver) GetSecurityGroupRuleOrder() cloudprovider.TPriorityOrder {
+	return cloudprovider.PriorityOrderByAsc
+}
+
+func (self *SAwsRegionDriver) GetDefaultSecurityGroupInRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("in:deny any")}
+}
+
+func (self *SAwsRegionDriver) GetDefaultSecurityGroupOutRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("out:allow any")}
+}
+
+func (self *SAwsRegionDriver) GetSecurityGroupRuleMaxPriority() int {
+	return 0
+}
+
+func (self *SAwsRegionDriver) GetSecurityGroupRuleMinPriority() int {
+	return 0
+}
+
+func (self *SAwsRegionDriver) IsOnlySupportAllowRules() bool {
+	return true
+}
+
 func (self *SAwsRegionDriver) GetProvider() string {
 	return api.CLOUD_PROVIDER_AWS
 }

pkg/compute/regiondrivers/azure.go

@@ -18,8 +18,10 @@ import (
 	"context"
 
 	"yunion.io/x/jsonutils"
+	"yunion.io/x/pkg/util/secrules"
 
 	api "yunion.io/x/onecloud/pkg/apis/compute"
+	"yunion.io/x/onecloud/pkg/cloudprovider"
 	"yunion.io/x/onecloud/pkg/compute/models"
 	"yunion.io/x/onecloud/pkg/httperrors"
 	"yunion.io/x/onecloud/pkg/mcclient"
@@ -53,3 +55,23 @@ func (self *SAzureRegionDriver) ValidateCreateLoadbalancerCertificateData(ctx co
 func (self *SAzureRegionDriver) IsSupportClassicSecurityGroup() bool {
 	return true
 }
+
+func (self *SAzureRegionDriver) GetSecurityGroupRuleOrder() cloudprovider.TPriorityOrder {
+	return cloudprovider.PriorityOrderByAsc
+}
+
+func (self *SAzureRegionDriver) GetDefaultSecurityGroupInRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("in:deny any")}
+}
+
+func (self *SAzureRegionDriver) GetDefaultSecurityGroupOutRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("out:deny any")}
+}
+
+func (self *SAzureRegionDriver) GetSecurityGroupRuleMaxPriority() int {
+	return 4096
+}
+
+func (self *SAzureRegionDriver) GetSecurityGroupRuleMinPriority() int {
+	return 100
+}

pkg/compute/regiondrivers/base.go

@@ -20,6 +20,7 @@ import (
 	"time"
 
 	"yunion.io/x/jsonutils"
+	"yunion.io/x/pkg/util/secrules"
 
 	api "yunion.io/x/onecloud/pkg/apis/compute"
 	"yunion.io/x/onecloud/pkg/cloudcommon/db/taskman"
@@ -247,6 +248,30 @@ func (self *SBaseRegionDriver) RequestSyncSecurityGroup(ctx context.Context, use
 	return "", fmt.Errorf("Not Implemented RequestSyncSecurityGroup")
 }
 
+func (self *SBaseRegionDriver) GetSecurityGroupRuleOrder() cloudprovider.TPriorityOrder {
+	return cloudprovider.PriorityOrderByDesc
+}
+
+func (self *SBaseRegionDriver) IsOnlySupportAllowRules() bool {
+	return false
+}
+
+func (self *SBaseRegionDriver) GetDefaultSecurityGroupInRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("in:deny any")}
+}
+
+func (self *SBaseRegionDriver) GetDefaultSecurityGroupOutRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("out:allow any")}
+}
+
+func (self *SBaseRegionDriver) GetSecurityGroupRuleMaxPriority() int {
+	return 100
+}
+
+func (self *SBaseRegionDriver) GetSecurityGroupRuleMinPriority() int {
+	return 1
+}
+
 func (self *SBaseRegionDriver) ValidateCreateDBInstanceData(ctx context.Context, userCred mcclient.TokenCredential, ownerId mcclient.IIdentityProvider, input api.DBInstanceCreateInput, skus []models.SDBInstanceSku, network *models.SNetwork) (api.DBInstanceCreateInput, error) {
 	return input, nil
 }

pkg/compute/regiondrivers/ctyun.go

@@ -18,8 +18,10 @@ import (
 	"context"
 
 	"yunion.io/x/jsonutils"
+	"yunion.io/x/pkg/util/secrules"
 
 	api "yunion.io/x/onecloud/pkg/apis/compute"
+	"yunion.io/x/onecloud/pkg/cloudprovider"
 	"yunion.io/x/onecloud/pkg/compute/models"
 	"yunion.io/x/onecloud/pkg/httperrors"
 	"yunion.io/x/onecloud/pkg/mcclient"
@@ -34,6 +36,30 @@ func init() {
 	models.RegisterRegionDriver(&driver)
 }
 
+func (self *SCtyunRegionDriver) GetSecurityGroupRuleOrder() cloudprovider.TPriorityOrder {
+	return cloudprovider.PriorityOrderByAsc
+}
+
+func (self *SCtyunRegionDriver) GetDefaultSecurityGroupInRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("in:deny any")}
+}
+
+func (self *SCtyunRegionDriver) GetDefaultSecurityGroupOutRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("out:deny any")}
+}
+
+func (self *SCtyunRegionDriver) GetSecurityGroupRuleMaxPriority() int {
+	return 0
+}
+
+func (self *SCtyunRegionDriver) GetSecurityGroupRuleMinPriority() int {
+	return 0
+}
+
+func (self *SCtyunRegionDriver) IsOnlySupportAllowRules() bool {
+	return true
+}
+
 func (self *SCtyunRegionDriver) GetProvider() string {
 	return api.CLOUD_PROVIDER_CTYUN
 }

pkg/compute/regiondrivers/google.go

@@ -21,6 +21,7 @@ import (
 	"yunion.io/x/jsonutils"
 	"yunion.io/x/log"
 	"yunion.io/x/pkg/errors"
+	"yunion.io/x/pkg/util/secrules"
 
 	billing_api "yunion.io/x/onecloud/pkg/apis/billing"
 	api "yunion.io/x/onecloud/pkg/apis/compute"
@@ -41,6 +42,26 @@ func init() {
 	models.RegisterRegionDriver(&driver)
 }
 
+func (self *SGoogleRegionDriver) GetSecurityGroupRuleOrder() cloudprovider.TPriorityOrder {
+	return cloudprovider.PriorityOrderByAsc
+}
+
+func (self *SGoogleRegionDriver) GetDefaultSecurityGroupInRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("in:deny any")}
+}
+
+func (self *SGoogleRegionDriver) GetDefaultSecurityGroupOutRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("out:allow any")}
+}
+
+func (self *SGoogleRegionDriver) GetSecurityGroupRuleMaxPriority() int {
+	return 0
+}
+
+func (self *SGoogleRegionDriver) GetSecurityGroupRuleMinPriority() int {
+	return 65535
+}
+
 func (self *SGoogleRegionDriver) GetProvider() string {
 	return api.CLOUD_PROVIDER_GOOGLE
 }

pkg/compute/regiondrivers/huawei.go

@@ -25,6 +25,7 @@ import (
 	"yunion.io/x/jsonutils"
 	"yunion.io/x/log"
 	"yunion.io/x/pkg/errors"
+	"yunion.io/x/pkg/util/secrules"
 	"yunion.io/x/pkg/utils"
 
 	billing_api "yunion.io/x/onecloud/pkg/apis/billing"
@@ -52,6 +53,30 @@ func init() {
 	models.RegisterRegionDriver(&driver)
 }
 
+func (self *SHuaWeiRegionDriver) GetSecurityGroupRuleOrder() cloudprovider.TPriorityOrder {
+	return cloudprovider.PriorityOrderByAsc
+}
+
+func (self *SHuaWeiRegionDriver) GetDefaultSecurityGroupInRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("in:deny any")}
+}
+
+func (self *SHuaWeiRegionDriver) GetDefaultSecurityGroupOutRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("out:allow any")}
+}
+
+func (self *SHuaWeiRegionDriver) GetSecurityGroupRuleMaxPriority() int {
+	return 0
+}
+
+func (self *SHuaWeiRegionDriver) GetSecurityGroupRuleMinPriority() int {
+	return 0
+}
+
+func (self *SHuaWeiRegionDriver) IsOnlySupportAllowRules() bool {
+	return true
+}
+
 func (self *SHuaWeiRegionDriver) GetProvider() string {
 	return api.CLOUD_PROVIDER_HUAWEI
 }

pkg/compute/regiondrivers/managedvirtual.go

@@ -18,7 +18,6 @@ import (
 	"context"
 	"database/sql"
 	"fmt"
-	"sort"
 	"strings"
 	"time"
 
@@ -1532,35 +1531,32 @@ func (self *SManagedVirtualizationRegionDriver) RequestSyncSecurityGroup(ctx con
 		return "", errors.Wrap(err, "db.Update")
 	}
 
-	inAllowList := secgroup.GetInAllowList()
-	outAllowList := secgroup.GetOutAllowList()
-
 	rules, err := iSecgroup.GetRules()
 	if err != nil {
 		return "", errors.Wrap(err, "iSecgroup.GetRules")
 	}
 
-	inRules := secrules.SecurityRuleSet{}
-	outRules := secrules.SecurityRuleSet{}
-	for i := 0; i < len(rules); i++ {
-		if rules[i].Direction == secrules.DIR_IN {
-			inRules = append(inRules, rules[i])
-		} else {
-			outRules = append(outRules, rules[i])
-		}
-	}
-	sort.Sort(inRules)
-	sort.Sort(outRules)
-	_inAllowList := inRules.AllowList()
-	_outAllowList := outRules.AllowList()
-	if inAllowList.Equals(_inAllowList) && outAllowList.Equals(_outAllowList) && (len(_inAllowList) > 0 && len(_outAllowList) > 0) { // 避免单个deny any的allowList为空,导致安全组规则未同步
+	maxPriority := region.GetDriver().GetSecurityGroupRuleMaxPriority()
+	minPriority := region.GetDriver().GetSecurityGroupRuleMinPriority()
+
+	defaultInRule := region.GetDriver().GetDefaultSecurityGroupInRule()
+	defaultOutRule := region.GetDriver().GetDefaultSecurityGroupOutRule()
+	order := region.GetDriver().GetSecurityGroupRuleOrder()
+	onlyAllowRules := region.GetDriver().IsOnlySupportAllowRules()
+
+	localRules := secrules.SecurityRuleSet(secgroup.GetSecRules(""))
+
+	common, inAdds, outAdds, inDels, outDels := cloudprovider.CompareRules(minPriority, maxPriority, order, localRules, rules, defaultInRule, defaultOutRule, onlyAllowRules, false)
+
+	if len(inAdds) == 0 && len(inDels) == 0 && len(outAdds) == 0 && len(outDels) == 0 {
 		return cache.ExternalId, nil
 	}
 
-	err = iSecgroup.SyncRules(secgroup.GetSecRules(""))
+	err = iSecgroup.SyncRules(common, inAdds, outAdds, inDels, outDels)
 	if err != nil {
 		return "", errors.Wrap(err, "iSecgroup.SyncRules")
 	}
+
 	return cache.ExternalId, nil
 }
 

pkg/compute/regiondrivers/openstack.go

@@ -19,8 +19,10 @@ import (
 	"database/sql"
 
 	"yunion.io/x/jsonutils"
+	"yunion.io/x/pkg/util/secrules"
 
 	api "yunion.io/x/onecloud/pkg/apis/compute"
+	"yunion.io/x/onecloud/pkg/cloudprovider"
 	"yunion.io/x/onecloud/pkg/compute/models"
 	"yunion.io/x/onecloud/pkg/httperrors"
 	"yunion.io/x/onecloud/pkg/mcclient"
@@ -35,6 +37,30 @@ func init() {
 	models.RegisterRegionDriver(&driver)
 }
 
+func (self *SOpenStackRegionDriver) GetSecurityGroupRuleOrder() cloudprovider.TPriorityOrder {
+	return cloudprovider.PriorityOrderByDesc
+}
+
+func (self *SOpenStackRegionDriver) GetDefaultSecurityGroupInRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("in:deny any")}
+}
+
+func (self *SOpenStackRegionDriver) GetDefaultSecurityGroupOutRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("out:deny any")}
+}
+
+func (self *SOpenStackRegionDriver) GetSecurityGroupRuleMaxPriority() int {
+	return 0
+}
+
+func (self *SOpenStackRegionDriver) GetSecurityGroupRuleMinPriority() int {
+	return 0
+}
+
+func (self *SOpenStackRegionDriver) IsOnlySupportAllowRules() bool {
+	return true
+}
+
 func (self *SOpenStackRegionDriver) GetProvider() string {
 	return api.CLOUD_PROVIDER_OPENSTACK
 }

pkg/compute/regiondrivers/qcloud.go

@@ -22,6 +22,7 @@ import (
 	"yunion.io/x/jsonutils"
 	"yunion.io/x/log"
 	"yunion.io/x/pkg/errors"
+	"yunion.io/x/pkg/util/secrules"
 	"yunion.io/x/pkg/utils"
 	"yunion.io/x/sqlchemy"
 
@@ -45,6 +46,26 @@ func init() {
 	models.RegisterRegionDriver(&driver)
 }
 
+func (self *SQcloudRegionDriver) GetSecurityGroupRuleOrder() cloudprovider.TPriorityOrder {
+	return cloudprovider.PriorityOrderByAsc
+}
+
+func (self *SQcloudRegionDriver) GetDefaultSecurityGroupInRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("in:deny any")}
+}
+
+func (self *SQcloudRegionDriver) GetDefaultSecurityGroupOutRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("out:deny any")}
+}
+
+func (self *SQcloudRegionDriver) GetSecurityGroupRuleMaxPriority() int {
+	return 0
+}
+
+func (self *SQcloudRegionDriver) GetSecurityGroupRuleMinPriority() int {
+	return 100
+}
+
 func (self *SQcloudRegionDriver) GetProvider() string {
 	return api.CLOUD_PROVIDER_QCLOUD
 }

pkg/compute/regiondrivers/secgroup_aws_test.go

@@ -0,0 +1,63 @@
+// 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 regiondrivers
+
+import (
+	"testing"
+
+	"yunion.io/x/pkg/util/secrules"
+
+	"yunion.io/x/onecloud/pkg/cloudprovider"
+)
+
+func TestAwsRuleSync(t *testing.T) {
+	driver := SAwsRegionDriver{}
+	maxPriority := driver.GetSecurityGroupRuleMaxPriority()
+	minPriority := driver.GetSecurityGroupRuleMinPriority()
+
+	defaultInRule := driver.GetDefaultSecurityGroupInRule()
+	defaultOutRule := driver.GetDefaultSecurityGroupOutRule()
+	order := driver.GetSecurityGroupRuleOrder()
+	isOnlyAllowRules := driver.IsOnlySupportAllowRules()
+
+	data := []TestData{
+		{
+			Name: "Test out deny rules",
+			LocalRules: secrules.SecurityRuleSet{
+				localRuleWithPriority("out:deny any", 1),
+			},
+			RemoteRules: []cloudprovider.SecurityRule{
+				remoteRuleWithName("", "out:allow any", 1),
+			},
+			Common:  []cloudprovider.SecurityRule{},
+			InAdds:  []cloudprovider.SecurityRule{},
+			OutAdds: []cloudprovider.SecurityRule{},
+			InDels:  []cloudprovider.SecurityRule{},
+			OutDels: []cloudprovider.SecurityRule{
+				remoteRuleWithName("", "out:allow any", 1),
+			},
+		},
+	}
+
+	for _, d := range data {
+		t.Logf("check %s", d.Name)
+		common, inAdds, outAdds, inDels, outDels := cloudprovider.CompareRules(minPriority, maxPriority, order, d.LocalRules, d.RemoteRules, defaultInRule, defaultOutRule, isOnlyAllowRules, true)
+		check(t, "common", common, d.Common)
+		check(t, "inAdds", inAdds, d.InAdds)
+		check(t, "outAdds", outAdds, d.OutAdds)
+		check(t, "inDels", inDels, d.InDels)
+		check(t, "outDels", outDels, d.OutDels)
+	}
+}

pkg/compute/regiondrivers/secgroup_azure_test.go

@@ -0,0 +1,235 @@
+// 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 regiondrivers
+
+import (
+	"sort"
+	"testing"
+
+	"yunion.io/x/pkg/util/secrules"
+
+	"yunion.io/x/onecloud/pkg/cloudprovider"
+)
+
+func TestAzureRuleSync(t *testing.T) {
+	driver := SAzureRegionDriver{}
+	maxPriority := driver.GetSecurityGroupRuleMaxPriority()
+	minPriority := driver.GetSecurityGroupRuleMinPriority()
+
+	defaultInRule := driver.GetDefaultSecurityGroupInRule()
+	defaultOutRule := driver.GetDefaultSecurityGroupOutRule()
+	order := driver.GetSecurityGroupRuleOrder()
+	isOnlyAllowRules := driver.IsOnlySupportAllowRules()
+
+	data := []TestData{
+		{
+			Name:        "Test empty rules",
+			LocalRules:  secrules.SecurityRuleSet{},
+			RemoteRules: []cloudprovider.SecurityRule{},
+			Common:      []cloudprovider.SecurityRule{},
+			InAdds:      []cloudprovider.SecurityRule{},
+			OutAdds: []cloudprovider.SecurityRule{
+				remoteRuleWithName("", "out:allow any", 2097),
+			},
+			InDels:  []cloudprovider.SecurityRule{},
+			OutDels: []cloudprovider.SecurityRule{},
+		},
+		{
+			Name:       "Test remove rules",
+			LocalRules: secrules.SecurityRuleSet{},
+			RemoteRules: []cloudprovider.SecurityRule{
+				remoteRuleWithName("test-name", "out:allow any", 1000),
+			},
+			Common: []cloudprovider.SecurityRule{},
+			InAdds: []cloudprovider.SecurityRule{},
+			OutAdds: []cloudprovider.SecurityRule{
+				remoteRuleWithName("", "out:allow any", 2097),
+			},
+			InDels: []cloudprovider.SecurityRule{},
+			OutDels: []cloudprovider.SecurityRule{
+				remoteRuleWithName("test-name", "out:allow any", 1000),
+			},
+		},
+		{
+			Name: "Test diff rules",
+			LocalRules: secrules.SecurityRuleSet{
+				localRuleWithPriority("out:allow tcp 100-200", 99),
+				localRuleWithPriority("out:allow udp 200-300", 98),
+			},
+			RemoteRules: []cloudprovider.SecurityRule{
+				remoteRuleWithName("test-tcp", "out:allow tcp 100-200", 1000),
+				remoteRuleWithName("test-udp", "out:allow udp 200-300", 1002),
+			},
+			Common: []cloudprovider.SecurityRule{
+				remoteRuleWithName("test-tcp", "out:allow tcp 100-200", 1000),
+				remoteRuleWithName("test-udp", "out:allow udp 200-300", 1002),
+			},
+			InAdds: []cloudprovider.SecurityRule{},
+			OutAdds: []cloudprovider.SecurityRule{
+				remoteRuleWithName("", "out:allow any", 2097),
+			},
+			InDels:  []cloudprovider.SecurityRule{},
+			OutDels: []cloudprovider.SecurityRule{},
+		},
+		{
+			Name: "Test add rules",
+			LocalRules: secrules.SecurityRuleSet{
+				localRuleWithPriority("in:allow tcp", 100),
+				localRuleWithPriority("in:allow udp", 99),
+				localRuleWithPriority("out:deny any", 1),
+			},
+			RemoteRules: []cloudprovider.SecurityRule{
+				remoteRuleWithName("allow-ssh", "in:allow tcp 22", 300),
+			},
+			Common: []cloudprovider.SecurityRule{},
+			InAdds: []cloudprovider.SecurityRule{
+				remoteRuleWithName("", "in:allow tcp", 2097),
+				remoteRuleWithName("", "in:allow udp", 2097),
+			},
+			OutAdds: []cloudprovider.SecurityRule{},
+			InDels: []cloudprovider.SecurityRule{
+				remoteRuleWithName("allow-ssh", "in:allow tcp 22", 300),
+			},
+			OutDels: []cloudprovider.SecurityRule{},
+		},
+		{
+			Name: "Test insert rules",
+			LocalRules: secrules.SecurityRuleSet{
+				localRuleWithPriority("in:allow tcp", 100),
+				localRuleWithPriority("in:allow udp", 99),
+				localRuleWithPriority("in:allow icmp", 98),
+				localRuleWithPriority("out:deny any", 1),
+			},
+			RemoteRules: []cloudprovider.SecurityRule{
+				remoteRuleWithName("allow-tcp", "in:allow tcp", 300),
+				remoteRuleWithName("allow-icmp", "in:allow icmp", 400),
+			},
+			Common: []cloudprovider.SecurityRule{
+				remoteRuleWithName("allow-tcp", "in:allow tcp", 300),
+				remoteRuleWithName("allow-icmp", "in:allow icmp", 400),
+			},
+			InAdds: []cloudprovider.SecurityRule{
+				remoteRuleWithName("", "in:allow udp", 2097),
+			},
+			OutAdds: []cloudprovider.SecurityRule{},
+			InDels:  []cloudprovider.SecurityRule{},
+			OutDels: []cloudprovider.SecurityRule{},
+		},
+		{
+			Name: "Test icmp rules",
+			LocalRules: secrules.SecurityRuleSet{
+				localRuleWithPriority("in:allow tcp 33", 10),
+				localRuleWithPriority("in:allow tcp 22", 1),
+				localRuleWithPriority("out:deny any", 1),
+			},
+			RemoteRules: []cloudprovider.SecurityRule{
+				remoteRuleWithName("allow-tcp-22", "in:allow tcp 22", 300),
+			},
+			Common: []cloudprovider.SecurityRule{
+				remoteRuleWithName("allow-tcp-22", "in:allow tcp 22", 300),
+			},
+			InAdds: []cloudprovider.SecurityRule{
+				remoteRuleWithName("", "in:allow tcp 33", 299),
+			},
+			OutAdds: []cloudprovider.SecurityRule{},
+			InDels:  []cloudprovider.SecurityRule{},
+			OutDels: []cloudprovider.SecurityRule{},
+		},
+		{
+			Name: "Test a rules",
+			LocalRules: secrules.SecurityRuleSet{
+				localRuleWithPriority("in:allow tcp 1050", 5),
+				localRuleWithPriority("in:allow tcp 1011", 4),
+				localRuleWithPriority("in:allow tcp 1002", 3),
+				localRuleWithPriority("in:allow tcp 22", 2),
+				localRuleWithPriority("in:allow udp 55", 1),
+				localRuleWithPriority("out:deny any", 1),
+			},
+			RemoteRules: []cloudprovider.SecurityRule{
+				remoteRuleWithName("in_allow_udp_55_4014", "in:allow udp 55", 4014),
+				remoteRuleWithName("in_allow_tcp_22_4013", "in:allow tcp 22", 4013),
+				remoteRuleWithName("in_allow_tcp_1002_4012", "in:allow tcp 1002", 4012),
+				remoteRuleWithName("in_allow_tcp_1010_4011", "in:allow tcp 1010", 4011),
+				remoteRuleWithName("in_allow_tcp_1050_4010", "in:allow tcp 1050", 4010),
+			},
+			Common: []cloudprovider.SecurityRule{
+				remoteRuleWithName("in_allow_tcp_1050_4010", "in:allow tcp 1050", 4010),
+				remoteRuleWithName("in_allow_tcp_1002_4012", "in:allow tcp 1002", 4012),
+				remoteRuleWithName("in_allow_tcp_22_4013", "in:allow tcp 22", 4013),
+				remoteRuleWithName("in_allow_udp_55_4014", "in:allow udp 55", 4014),
+			},
+			InAdds: []cloudprovider.SecurityRule{
+				remoteRuleWithName("", "in:allow tcp 1011", 4011),
+			},
+			OutAdds: []cloudprovider.SecurityRule{},
+			InDels: []cloudprovider.SecurityRule{
+				remoteRuleWithName("in_allow_tcp_1010_4011", "in:allow tcp 1010", 4011),
+			},
+			OutDels: []cloudprovider.SecurityRule{},
+		},
+		{
+			Name: "Test b rules",
+			LocalRules: secrules.SecurityRuleSet{
+				localRuleWithPriority("in:allow udp 1055", 20),
+				localRuleWithPriority("in:allow icmp", 15),
+				localRuleWithPriority("in:allow tcp 1050", 5),
+				localRuleWithPriority("in:allow tcp 1012", 4),
+				localRuleWithPriority("in:allow tcp 1002", 3),
+				localRuleWithPriority("in:allow tcp 22", 2),
+				localRuleWithPriority("in:allow udp 55", 1),
+				localRuleWithPriority("out:deny any", 1),
+			},
+			RemoteRules: []cloudprovider.SecurityRule{
+				remoteRuleWithName("in_allow_udp_55_4014", "in:allow udp 55", 4014),
+				remoteRuleWithName("in_allow_tcp_22_4013", "in:allow tcp 22", 4013),
+				remoteRuleWithName("in_allow_tcp_1002_4012", "in:allow tcp 1002", 4012),
+				remoteRuleWithName("in_allow_tcp_1012_4011", "in:allow tcp 1012", 4011),
+				remoteRuleWithName("in_allow_tcp_1050_4010", "in:allow tcp 1050", 4010),
+				remoteRuleWithName("in_allow_tcp_1055_4009", "in:allow tcp 1055", 4009),
+			},
+			Common: []cloudprovider.SecurityRule{
+				remoteRuleWithName("in_allow_tcp_1050_4010", "in:allow tcp 1050", 4010),
+				remoteRuleWithName("in_allow_tcp_1012_4011", "in:allow tcp 1012", 4011),
+				remoteRuleWithName("in_allow_tcp_1002_4012", "in:allow tcp 1002", 4012),
+				remoteRuleWithName("in_allow_tcp_22_4013", "in:allow tcp 22", 4013),
+				remoteRuleWithName("in_allow_udp_55_4014", "in:allow udp 55", 4014),
+			},
+			InAdds: []cloudprovider.SecurityRule{
+				remoteRuleWithName("", "in:allow icmp", 2097),
+				remoteRuleWithName("", "in:allow udp 1055", 4013),
+			},
+			OutAdds: []cloudprovider.SecurityRule{},
+			InDels: []cloudprovider.SecurityRule{
+				remoteRuleWithName("in_allow_tcp_1055_4009", "in:allow tcp 1055", 4009),
+			},
+			OutDels: []cloudprovider.SecurityRule{},
+		},
+	}
+
+	for _, d := range data {
+		t.Logf("check %s", d.Name)
+		common, inAdds, outAdds, inDels, outDels := cloudprovider.CompareRules(minPriority, maxPriority, order, d.LocalRules, d.RemoteRules, defaultInRule, defaultOutRule, isOnlyAllowRules, true)
+		sort.Sort(cloudprovider.SecurityRuleSet(common))
+		sort.Sort(cloudprovider.SecurityRuleSet(inAdds))
+		sort.Sort(cloudprovider.SecurityRuleSet(outAdds))
+		sort.Sort(cloudprovider.SecurityRuleSet(inDels))
+		sort.Sort(cloudprovider.SecurityRuleSet(outDels))
+		check(t, "common", common, d.Common)
+		check(t, "inAdds", inAdds, d.InAdds)
+		check(t, "outAdds", outAdds, d.OutAdds)
+		check(t, "inDels", inDels, d.InDels)
+		check(t, "outDels", outDels, d.OutDels)
+	}
+}

pkg/compute/regiondrivers/secgroup_ctyun_test.go

@@ -0,0 +1,70 @@
+// 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 regiondrivers
+
+import (
+	"sort"
+	"testing"
+
+	"yunion.io/x/pkg/util/secrules"
+
+	"yunion.io/x/onecloud/pkg/cloudprovider"
+)
+
+func TestCtyunRuleSync(t *testing.T) {
+	driver := SCtyunRegionDriver{}
+	maxPriority := driver.GetSecurityGroupRuleMaxPriority()
+	minPriority := driver.GetSecurityGroupRuleMinPriority()
+
+	defaultInRule := driver.GetDefaultSecurityGroupInRule()
+	defaultOutRule := driver.GetDefaultSecurityGroupOutRule()
+	order := driver.GetSecurityGroupRuleOrder()
+	isOnlyAllowRules := driver.IsOnlySupportAllowRules()
+
+	data := []TestData{
+		{
+			Name: "Test out deny rules",
+			LocalRules: secrules.SecurityRuleSet{
+				localRuleWithPriority("out:deny tcp 200", 1),
+			},
+			RemoteRules: []cloudprovider.SecurityRule{},
+			Common:      []cloudprovider.SecurityRule{},
+			InAdds:      []cloudprovider.SecurityRule{},
+			OutAdds: []cloudprovider.SecurityRule{
+				remoteRuleWithName("", "out:allow icmp", 0),
+				remoteRuleWithName("", "out:allow tcp 1-199", 0),
+				remoteRuleWithName("", "out:allow tcp 201-65535", 0),
+				remoteRuleWithName("", "out:allow udp", 0),
+			},
+			InDels:  []cloudprovider.SecurityRule{},
+			OutDels: []cloudprovider.SecurityRule{},
+		},
+	}
+
+	for _, d := range data {
+		t.Logf("check %s", d.Name)
+		common, inAdds, outAdds, inDels, outDels := cloudprovider.CompareRules(minPriority, maxPriority, order, d.LocalRules, d.RemoteRules, defaultInRule, defaultOutRule, isOnlyAllowRules, true)
+		sort.Sort(cloudprovider.SecurityRuleSet(common))
+		sort.Sort(cloudprovider.SecurityRuleSet(inAdds))
+		sort.Sort(cloudprovider.SecurityRuleSet(outAdds))
+		sort.Sort(cloudprovider.SecurityRuleSet(inDels))
+		sort.Sort(cloudprovider.SecurityRuleSet(outDels))
+		check(t, "common", common, d.Common)
+		check(t, "inAdds", inAdds, d.InAdds)
+		check(t, "outAdds", outAdds, d.OutAdds)
+		check(t, "inDels", inDels, d.InDels)
+		check(t, "outDels", outDels, d.OutDels)
+	}
+}

pkg/compute/regiondrivers/secgroup_openstack_test.go

@@ -0,0 +1,67 @@
+// 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 regiondrivers
+
+import (
+	"testing"
+
+	"yunion.io/x/pkg/util/secrules"
+
+	"yunion.io/x/onecloud/pkg/cloudprovider"
+)
+
+func TestOpenStackRuleSync(t *testing.T) {
+	driver := SOpenStackRegionDriver{}
+	maxPriority := driver.GetSecurityGroupRuleMaxPriority()
+	minPriority := driver.GetSecurityGroupRuleMinPriority()
+
+	defaultInRule := driver.GetDefaultSecurityGroupInRule()
+	defaultOutRule := driver.GetDefaultSecurityGroupOutRule()
+	order := driver.GetSecurityGroupRuleOrder()
+	isOnlyAllowRules := driver.IsOnlySupportAllowRules()
+
+	data := []TestData{
+		{
+			Name: "Test deny rules",
+			LocalRules: secrules.SecurityRuleSet{
+				localRuleWithPriority("in:deny any", 100),
+				localRuleWithPriority("in:allow any", 99),
+				localRuleWithPriority("out:allow any", 100),
+			},
+			RemoteRules: []cloudprovider.SecurityRule{
+				remoteRuleWithName("", "in:allow any", 1),
+			},
+			Common: []cloudprovider.SecurityRule{},
+			InAdds: []cloudprovider.SecurityRule{},
+			OutAdds: []cloudprovider.SecurityRule{
+				remoteRuleWithName("", "out:allow any", 0),
+			},
+			InDels: []cloudprovider.SecurityRule{
+				remoteRuleWithName("", "in:allow any", 1),
+			},
+			OutDels: []cloudprovider.SecurityRule{},
+		},
+	}
+
+	for _, d := range data {
+		t.Logf("check %s", d.Name)
+		common, inAdds, outAdds, inDels, outDels := cloudprovider.CompareRules(minPriority, maxPriority, order, d.LocalRules, d.RemoteRules, defaultInRule, defaultOutRule, isOnlyAllowRules, true)
+		check(t, "common", common, d.Common)
+		check(t, "inAdds", inAdds, d.InAdds)
+		check(t, "outAdds", outAdds, d.OutAdds)
+		check(t, "inDels", inDels, d.InDels)
+		check(t, "outDels", outDels, d.OutDels)
+	}
+}

pkg/compute/regiondrivers/secgroup_test.go

@@ -0,0 +1,84 @@
+// 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 regiondrivers
+
+import (
+	"fmt"
+	"testing"
+
+	"yunion.io/x/log"
+	"yunion.io/x/pkg/util/secrules"
+
+	"yunion.io/x/onecloud/pkg/cloudprovider"
+)
+
+type TestData struct {
+	Name        string
+	LocalRules  secrules.SecurityRuleSet
+	RemoteRules cloudprovider.SecurityRuleSet
+	Common      cloudprovider.SecurityRuleSet
+	InAdds      cloudprovider.SecurityRuleSet
+	OutAdds     cloudprovider.SecurityRuleSet
+	InDels      cloudprovider.SecurityRuleSet
+	OutDels     cloudprovider.SecurityRuleSet
+}
+
+var localRuleWithPriority = func(ruleStr string, priority int) secrules.SecurityRule {
+	rule := secrules.MustParseSecurityRule(ruleStr)
+	if rule == nil {
+		log.Errorf("invalid rule str %s", ruleStr)
+		return secrules.SecurityRule{}
+	}
+	rule.Priority = priority
+	return *rule
+}
+
+var remoteRuleWithName = func(name, ruleStr string, priority int) cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{
+		Name:         name,
+		SecurityRule: localRuleWithPriority(ruleStr, priority),
+	}
+}
+
+var check = func(t *testing.T, name string, ret, expect []cloudprovider.SecurityRule) {
+	var show = func(info string, rules []cloudprovider.SecurityRule) {
+		t.Logf("%s: %d\n", info, len(rules))
+		for _, r := range rules {
+			t.Logf("Name: %s priority: %d %s\n", r.Name, r.Priority, r.String())
+		}
+	}
+	if len(ret) != len(expect) {
+		show(fmt.Sprintf("%s rule", name), ret)
+		show(fmt.Sprintf("%s expect", name), expect)
+		t.Fatalf("invalid rules for %s current is %d expect %d", name, len(ret), len(expect))
+	}
+	for i := range ret {
+		if ret[i].Name != expect[i].Name {
+			show(fmt.Sprintf("%s rule", name), ret)
+			show(fmt.Sprintf("%s expect", name), expect)
+			t.Fatalf("invalid index(%d) %s rule name %s expect %s", i, name, ret[i].Name, expect[i].Name)
+		}
+		if ret[i].Priority != expect[i].Priority {
+			show(fmt.Sprintf("%s rule", name), ret)
+			show(fmt.Sprintf("%s expect", name), expect)
+			t.Fatalf("invalid index(%d) %s rule priority %d expect %d", i, name, ret[i].Priority, expect[i].Priority)
+		}
+		if ret[i].String() != expect[i].String() {
+			show(fmt.Sprintf("%s rule", name), ret)
+			show(fmt.Sprintf("%s expect", name), expect)
+			t.Fatalf("invalid index(%d) %s rules %s expect %s", i, name, ret[i].String(), expect[i].String())
+		}
+	}
+}

pkg/compute/regiondrivers/ucloud.go

@@ -18,8 +18,10 @@ import (
 	"context"
 
 	"yunion.io/x/jsonutils"
+	"yunion.io/x/pkg/util/secrules"
 
 	api "yunion.io/x/onecloud/pkg/apis/compute"
+	"yunion.io/x/onecloud/pkg/cloudprovider"
 	"yunion.io/x/onecloud/pkg/compute/models"
 	"yunion.io/x/onecloud/pkg/httperrors"
 	"yunion.io/x/onecloud/pkg/mcclient"
@@ -34,6 +36,26 @@ func init() {
 	models.RegisterRegionDriver(&driver)
 }
 
+func (self *SUcloudRegionDriver) GetSecurityGroupRuleOrder() cloudprovider.TPriorityOrder {
+	return cloudprovider.PriorityOrderByDesc
+}
+
+func (self *SUcloudRegionDriver) GetDefaultSecurityGroupInRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("in:deny any")}
+}
+
+func (self *SUcloudRegionDriver) GetDefaultSecurityGroupOutRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("out:allow any")}
+}
+
+func (self *SUcloudRegionDriver) GetSecurityGroupRuleMaxPriority() int {
+	return 3
+}
+
+func (self *SUcloudRegionDriver) GetSecurityGroupRuleMinPriority() int {
+	return 1
+}
+
 func (self *SUcloudRegionDriver) GetProvider() string {
 	return api.CLOUD_PROVIDER_UCLOUD
 }

pkg/compute/regiondrivers/zstack.go

@@ -19,8 +19,10 @@ import (
 	"database/sql"
 
 	"yunion.io/x/jsonutils"
+	"yunion.io/x/pkg/util/secrules"
 
 	api "yunion.io/x/onecloud/pkg/apis/compute"
+	"yunion.io/x/onecloud/pkg/cloudprovider"
 	"yunion.io/x/onecloud/pkg/compute/models"
 	"yunion.io/x/onecloud/pkg/httperrors"
 	"yunion.io/x/onecloud/pkg/mcclient"
@@ -35,6 +37,30 @@ func init() {
 	models.RegisterRegionDriver(&driver)
 }
 
+func (self *SZStackRegionDriver) GetSecurityGroupRuleOrder() cloudprovider.TPriorityOrder {
+	return cloudprovider.PriorityOrderByAsc
+}
+
+func (self *SZStackRegionDriver) GetDefaultSecurityGroupInRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("in:deny any")}
+}
+
+func (self *SZStackRegionDriver) GetDefaultSecurityGroupOutRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("out:allow any")}
+}
+
+func (self *SZStackRegionDriver) GetSecurityGroupRuleMaxPriority() int {
+	return 1
+}
+
+func (self *SZStackRegionDriver) GetSecurityGroupRuleMinPriority() int {
+	return 1
+}
+
+func (self *SZStackRegionDriver) IsOnlySupportAllowRules() bool {
+	return true
+}
+
 func (self *SZStackRegionDriver) GetProvider() string {
 	return api.CLOUD_PROVIDER_ZSTACK
 }

pkg/httperrors/consts.go

@@ -84,6 +84,8 @@ const (
 	ErrTooManyAttempts = errors.Error("TooManyFailedAttempts")
 	ErrTooManyRequests = errors.Error("TooManyRequests")
 
+	ErrUnsupportedProtocol = errors.Error("UnsupportedProtocol")
+
 	ErrPolicyDefinition = errors.Error("PolicyDefinitionError")
 )
 

pkg/multicloud/aliyun/region.go

@@ -26,7 +26,6 @@ import (
 	"yunion.io/x/log"
 	"yunion.io/x/pkg/errors"
 	"yunion.io/x/pkg/util/regutils"
-	"yunion.io/x/pkg/util/secrules"
 	"yunion.io/x/pkg/utils"
 
 	api "yunion.io/x/onecloud/pkg/apis/compute"
@@ -786,26 +785,6 @@ func (region *SRegion) CreateISecurityGroup(conf *cloudprovider.SecurityGroupCre
 	return region.GetISecurityGroupById(externalId)
 }
 
-func (region *SRegion) SyncSecurityGroup(secgroupId string, vpcId string, name string, desc string, rules []secrules.SecurityRule) (string, error) {
-	if len(secgroupId) > 0 {
-		_, total, err := region.GetSecurityGroups("", "", []string{secgroupId}, 0, 1)
-		if err != nil {
-			return "", err
-		}
-		if total == 0 {
-			secgroupId = ""
-		}
-	}
-	if len(secgroupId) == 0 {
-		extID, err := region.CreateSecurityGroup(vpcId, name, desc)
-		if err != nil {
-			return "", err
-		}
-		secgroupId = extID
-	}
-	return secgroupId, region.syncSecgroupRules(secgroupId, rules)
-}
-
 func (region *SRegion) GetILoadBalancers() ([]cloudprovider.ICloudLoadbalancer, error) {
 	lbs, err := region.GetLoadbalancers(nil)
 	if err != nil {

pkg/multicloud/aliyun/securitygroup.go

@@ -16,14 +16,18 @@ package aliyun
 
 import (
 	"fmt"
-	"sort"
+	"net"
 	"strings"
 	"time"
 
 	"yunion.io/x/jsonutils"
 	"yunion.io/x/log"
+	"yunion.io/x/pkg/errors"
 	"yunion.io/x/pkg/util/secrules"
 	"yunion.io/x/pkg/utils"
+
+	"yunion.io/x/onecloud/pkg/cloudprovider"
+	"yunion.io/x/onecloud/pkg/multicloud"
 )
 
 // {"CreationTime":"2017-03-19T13:37:48Z","Description":"System created security group.","SecurityGroupId":"sg-j6cannq0xxj2r9z0yxwl","SecurityGroupName":"sg-j6cannq0xxj2r9z0yxwl","Tags":{"Tag":[]},"VpcId":"vpc-j6c86z3sh8ufhgsxwme0q"}
@@ -69,6 +73,8 @@ type Tag struct {
 }
 
 type SSecurityGroup struct {
+	multicloud.SSecurityGroup
+
 	vpc               *SVpc
 	CreationTime      time.Time
 	Description       string
@@ -81,25 +87,6 @@ type SSecurityGroup struct {
 	Tags              Tags
 }
 
-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) GetVpcId() string {
 	return self.VpcId
 }
@@ -127,24 +114,19 @@ func (self *SSecurityGroup) GetDescription() string {
 	return self.Description
 }
 
-func (self *SSecurityGroup) GetRules() ([]secrules.SecurityRule, error) {
-	rules := make([]secrules.SecurityRule, 0)
-	if secgrp, err := self.vpc.region.GetSecurityGroupDetails(self.SecurityGroupId); err != nil {
-		return rules, err
-	} else {
-		for _, permission := range secgrp.Permissions.Permission {
-			if rule, err := secrules.ParseSecurityRule(permission.String()); err != nil {
-				return rules, err
-			} else {
-				priority := permission.Priority
-				if priority > 100 {
-					priority = 100
-				}
-				rule.Priority = 101 - priority
-				rule.Description = permission.Description
-				rules = append(rules, *rule)
-			}
+func (self *SSecurityGroup) GetRules() ([]cloudprovider.SecurityRule, error) {
+	rules := make([]cloudprovider.SecurityRule, 0)
+	secgrp, err := self.vpc.region.GetSecurityGroupDetails(self.SecurityGroupId)
+	if err != nil {
+		return nil, err
+	}
+	for _, permission := range secgrp.Permissions.Permission {
+		rule, err := permission.toRule()
+		if err != nil {
+			log.Errorf("convert rule %s for group %s(%s) error: %v", permission.Description, self.SecurityGroupName, self.SecurityGroupId, err)
+			continue
 		}
+		rules = append(rules, rule)
 	}
 	return rules, nil
 }
@@ -165,11 +147,11 @@ func (self *SSecurityGroup) IsEmulated() bool {
 }
 
 func (self *SSecurityGroup) Refresh() error {
-	if new, err := self.vpc.region.GetSecurityGroupDetails(self.SecurityGroupId); err != nil {
+	group, err := self.vpc.region.GetSecurityGroupDetails(self.SecurityGroupId)
+	if err != nil {
 		return err
-	} else {
-		return jsonutils.Update(self, new)
 	}
+	return jsonutils.Update(self, group)
 }
 
 func (self *SRegion) GetSecurityGroups(vpcId, name string, securityGroupIds []string, offset int, limit int) ([]SSecurityGroup, int, error) {
@@ -214,16 +196,13 @@ func (self *SRegion) GetSecurityGroupDetails(secGroupId string) (*SSecurityGroup
 
 	body, err := self.ecsRequest("DescribeSecurityGroupAttribute", params)
 	if err != nil {
-		log.Errorf("DescribeSecurityGroupAttribute fail %s", err)
-		return nil, err
+		return nil, errors.Wrap(err, "DescribeSecurityGroupAttribute")
 	}
 
-	log.Debugf("%s", body)
 	secgrp := SSecurityGroup{}
 	err = body.Unmarshal(&secgrp)
 	if err != nil {
-		log.Errorf("Unmarshal security group details fail %s", err)
-		return nil, err
+		return nil, errors.Wrap(err, "body.Unmarshal")
 	}
 	return &secgrp, nil
 }
@@ -248,7 +227,7 @@ func (self *SRegion) CreateSecurityGroup(vpcId string, name string, desc string)
 
 	body, err := self.ecsRequest("CreateSecurityGroup", params)
 	if err != nil {
-		return "", err
+		return "", errors.Wrap(err, "CreateSecurityGroup")
 	}
 	return body.GetString("SecurityGroupId")
 }
@@ -311,21 +290,21 @@ func (self *SRegion) modifySecurityGroup(secGrpId string, name string, desc stri
 	return err
 }
 
-func (self *SRegion) addSecurityGroupRules(secGrpId string, rule *secrules.SecurityRule) error {
+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
+			err := self.addSecurityGroupRule(secGrpId, rule)
+			if err != nil {
+				return errors.Wrapf(err, "addSecurityGroupRule %s", rule.String())
 			}
 		}
-	} else {
-		return self.addSecurityGroupRule(secGrpId, rule)
+		return nil
 	}
-	return nil
+	return self.addSecurityGroupRule(secGrpId, rule)
 }
 
-func (self *SRegion) addSecurityGroupRule(secGrpId string, rule *secrules.SecurityRule) error {
+func (self *SRegion) addSecurityGroupRule(secGrpId string, rule secrules.SecurityRule) error {
 	params := make(map[string]string)
 	params["RegionId"] = self.RegionId
 	params["SecurityGroupId"] = secGrpId
@@ -355,7 +334,7 @@ func (self *SRegion) addSecurityGroupRule(secGrpId string, rule *secrules.Securi
 		return nil
 	}
 
-	params["Priority"] = fmt.Sprintf("%d", 101-rule.Priority)
+	params["Priority"] = fmt.Sprintf("%d", rule.Priority)
 	if rule.Direction == secrules.SecurityRuleIngress {
 		if rule.IPNet != nil {
 			params["SourceCidrIp"] = rule.IPNet.String()
@@ -375,7 +354,7 @@ func (self *SRegion) addSecurityGroupRule(secGrpId string, rule *secrules.Securi
 	}
 }
 
-func (self *SRegion) delSecurityGroupRule(secGrpId string, rule *secrules.SecurityRule) error {
+func (self *SRegion) DelSecurityGroupRule(secGrpId string, rule secrules.SecurityRule) error {
 	params := make(map[string]string)
 	params["RegionId"] = self.RegionId
 	params["SecurityGroupId"] = secGrpId
@@ -418,26 +397,38 @@ func (self *SRegion) delSecurityGroupRule(secGrpId string, rule *secrules.Securi
 	}
 }
 
-func (self *SPermission) String() string {
-	action := secrules.SecurityRuleDeny
+func (self *SPermission) toRule() (cloudprovider.SecurityRule, error) {
+	rule := cloudprovider.SecurityRule{
+		SecurityRule: secrules.SecurityRule{
+			Action:      secrules.SecurityRuleDeny,
+			Direction:   secrules.DIR_IN,
+			Priority:    self.Priority,
+			Description: self.Description,
+			PortStart:   -1,
+			PortEnd:     -1,
+		},
+	}
 	if strings.ToLower(self.Policy) == "accept" {
-		action = secrules.SecurityRuleAllow
-	}
-	direction := "in"
-	if self.Direction == "egress" {
-		direction = "out"
+		rule.Action = secrules.SecurityRuleAllow
 	}
+
 	cidr := self.SourceCidrIp
-	if direction == "out" {
+	if self.Direction == "egress" {
+		rule.Direction = secrules.DIR_OUT
 		cidr = self.DestCidrIp
 	}
-	if cidr == "0.0.0.0/0" {
-		cidr = ""
-	}
-	protocol := strings.ToLower(self.IpProtocol)
-	if protocol == "all" {
-		protocol = "any"
+
+	_, rule.IPNet, _ = net.ParseCIDR(cidr)
+
+	switch strings.ToLower(self.IpProtocol) {
+	case "tcp", "udp", "icmp":
+		rule.Protocol = strings.ToLower(self.IpProtocol)
+	case "all":
+		rule.Protocol = secrules.PROTO_ANY
+	default:
+		return rule, fmt.Errorf("unsupported protocal %s", self.IpProtocol)
 	}
+
 	port, ports := "", strings.Split(self.PortRange, "/")
 	if ports[0] == ports[1] {
 		if ports[0] != "-1" {
@@ -446,81 +437,11 @@ func (self *SPermission) String() string {
 	} else if ports[0] != "1" && ports[1] != "65535" {
 		port = fmt.Sprintf("%s-%s", ports[0], ports[1])
 	}
-	result := fmt.Sprintf("%s:%s", direction, string(action))
-	if len(cidr) > 0 {
-		result += fmt.Sprintf(" %s", cidr)
+	err := rule.ParsePorts(port)
+	if err != nil {
+		return rule, errors.Wrapf(err, "ParsePorts(%s)", port)
 	}
-	result += fmt.Sprintf(" %s", protocol)
-	if len(port) > 0 {
-		result += fmt.Sprintf(" %s", port)
-	}
-	return result
-}
-
-func (self *SRegion) syncSecgroupRules(secgroupId string, rules []secrules.SecurityRule) error {
-	if secgroup, err := self.GetSecurityGroupDetails(secgroupId); err != nil {
-		return err
-	} else {
-
-		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
-					}
-				}
-				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
-			}
-		}
-	}
-	return nil
+	return rule, nil
 }
 
 func (self *SRegion) AssignSecurityGroup(secgroupId, instanceId string) error {
@@ -575,6 +496,18 @@ func (self *SSecurityGroup) GetProjectId() string {
 	return ""
 }
 
-func (self *SSecurityGroup) SyncRules(rules []secrules.SecurityRule) error {
-	return self.vpc.region.syncSecgroupRules(self.SecurityGroupId, rules)
+func (self *SSecurityGroup) SyncRules(common, inAdds, outAdds, inDels, outDels []cloudprovider.SecurityRule) error {
+	for _, rule := range append(inDels, outDels...) {
+		err := self.vpc.region.DelSecurityGroupRule(self.SecurityGroupId, rule.SecurityRule)
+		if err != nil {
+			return errors.Wrapf(err, "DelSecurityGroupRule(Name:%s priority: %d %s)", rule.Name, rule.Priority, rule.String())
+		}
+	}
+	for _, rule := range append(inAdds, outAdds...) {
+		err := self.vpc.region.AddSecurityGroupRules(self.SecurityGroupId, rule.SecurityRule)
+		if err != nil {
+			return errors.Wrapf(err, "AddSecurityGroupRules(priority: %d %s)", rule.Priority, rule.String())
+		}
+	}
+	return nil
 }

pkg/multicloud/aws/aws.go

@@ -15,6 +15,7 @@
 package aws
 
 import (
+	"github.com/aws/aws-sdk-go/aws"
 	sdk "github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/client"
 	"github.com/aws/aws-sdk-go/aws/credentials"
@@ -189,7 +190,7 @@ func (self *SAwsClient) fetchRegions() error {
 
 func (client *SAwsClient) getAwsSession(regionId string) (*session.Session, error) {
 	httpClient := client.cpcfg.HttpClient()
-	return session.NewSession(&sdk.Config{
+	s, err := session.NewSession(&sdk.Config{
 		Region: sdk.String(regionId),
 		Credentials: credentials.NewStaticCredentials(
 			client.accessKey, client.accessSecret, "",
@@ -198,6 +199,14 @@ func (client *SAwsClient) getAwsSession(regionId string) (*session.Session, erro
 		DisableParamValidation:        sdk.Bool(true),
 		CredentialsChainVerboseErrors: sdk.Bool(true),
 	})
+	if err != nil {
+		return nil, err
+	}
+	if client.debug {
+		logLevel := aws.LogLevelType(uint(aws.LogDebugWithRequestErrors) + uint(aws.LogDebugWithHTTPBody) + uint(aws.LogDebugWithSigning))
+		s.Config.LogLevel = &logLevel
+	}
+	return s, nil
 }
 
 func (self *SAwsClient) invalidateIBuckets() {

pkg/multicloud/aws/securitygroup.go

@@ -17,7 +17,6 @@ package aws
 import (
 	"fmt"
 	"math/rand"
-	"sort"
 	"strings"
 	"time"
 
@@ -25,9 +24,12 @@ import (
 
 	"yunion.io/x/jsonutils"
 	"yunion.io/x/log"
+	"yunion.io/x/pkg/errors"
 	"yunion.io/x/pkg/util/secrules"
 
+	"yunion.io/x/onecloud/pkg/cloudprovider"
 	"yunion.io/x/onecloud/pkg/httperrors"
+	"yunion.io/x/onecloud/pkg/multicloud"
 )
 
 type Tags struct {
@@ -40,6 +42,7 @@ type Tag struct {
 }
 
 type SSecurityGroup struct {
+	multicloud.SSecurityGroup
 	vpc *SVpc
 
 	RegionId          string
@@ -47,7 +50,7 @@ type SSecurityGroup struct {
 	SecurityGroupId   string
 	Description       string
 	SecurityGroupName string
-	Permissions       []secrules.SecurityRule
+	Permissions       []cloudprovider.SecurityRule
 	Tags              Tags
 
 	// CreationTime      time.Time
@@ -114,33 +117,30 @@ func (self *SSecurityGroup) GetDescription() string {
 	return self.Description
 }
 
-func (self *SSecurityGroup) GetRules() ([]secrules.SecurityRule, error) {
-	rules := make([]secrules.SecurityRule, 0)
-	if secgrp, err := self.vpc.region.GetSecurityGroupDetails(self.SecurityGroupId); err != nil {
-		return rules, err
-	} else {
-		rules = secgrp.Permissions
+func (self *SSecurityGroup) GetRules() ([]cloudprovider.SecurityRule, error) {
+	secgrp, err := self.vpc.region.GetSecurityGroupDetails(self.SecurityGroupId)
+	if err != nil {
+		return nil, err
 	}
-
-	return rules, nil
+	return secgrp.Permissions, nil
 }
 
-func (self *SRegion) addSecurityGroupRules(secGrpId string, rule *secrules.SecurityRule) error {
+func (self *SRegion) addSecurityGroupRules(secGrpId string, rule cloudprovider.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
+			err := self.addSecurityGroupRule(secGrpId, rule)
+			if err != nil {
+				return errors.Wrapf(err, "addSecurityGroupRule(%d %s)", rule.Priority, rule.String())
 			}
 		}
-	} else {
-		return self.addSecurityGroupRule(secGrpId, rule)
+		return nil
 	}
-	return nil
+	return self.addSecurityGroupRule(secGrpId, rule)
 }
 
-func (self *SRegion) addSecurityGroupRule(secGrpId string, rule *secrules.SecurityRule) error {
-	ipPermissions, err := YunionSecRuleToAws(*rule)
+func (self *SRegion) addSecurityGroupRule(secGrpId string, rule cloudprovider.SecurityRule) error {
+	ipPermissions, err := YunionSecRuleToAws(rule)
 	log.Debugf("Aws security group rule: %s", ipPermissions)
 	if err != nil {
 		return err
@@ -168,8 +168,8 @@ func (self *SRegion) addSecurityGroupRule(secGrpId string, rule *secrules.Securi
 	return err
 }
 
-func (self *SRegion) delSecurityGroupRule(secGrpId string, rule *secrules.SecurityRule) error {
-	ipPermissions, err := YunionSecRuleToAws(*rule)
+func (self *SRegion) DelSecurityGroupRule(secGrpId string, rule cloudprovider.SecurityRule) error {
+	ipPermissions, err := YunionSecRuleToAws(rule)
 	if err != nil {
 		return err
 	}
@@ -195,8 +195,8 @@ func (self *SRegion) delSecurityGroupRule(secGrpId string, rule *secrules.Securi
 	return nil
 }
 
-func (self *SRegion) updateSecurityGroupRuleDescription(secGrpId string, rule *secrules.SecurityRule) error {
-	ipPermissions, err := YunionSecRuleToAws(*rule)
+func (self *SRegion) updateSecurityGroupRuleDescription(secGrpId string, rule cloudprovider.SecurityRule) error {
+	ipPermissions, err := YunionSecRuleToAws(rule)
 	if err != nil {
 		return err
 	}
@@ -270,13 +270,15 @@ func (self *SRegion) createDefaultSecurityGroup(vpcId string) (string, error) {
 		return "", err
 	}
 
-	rule := &secrules.SecurityRule{
-		Priority:  1,
-		Action:    secrules.SecurityRuleAllow,
-		Protocol:  "",
-		Direction: secrules.SecurityRuleIngress,
-		PortStart: -1,
-		PortEnd:   -1,
+	rule := cloudprovider.SecurityRule{
+		SecurityRule: secrules.SecurityRule{
+			Priority:  1,
+			Action:    secrules.SecurityRuleAllow,
+			Protocol:  "",
+			Direction: secrules.SecurityRuleIngress,
+			PortStart: -1,
+			PortEnd:   -1,
+		},
 	}
 
 	err = self.addSecurityGroupRule(secId, rule)
@@ -360,68 +362,8 @@ func (self *SRegion) modifySecurityGroup(secGrpId string, name string, desc stri
 	return nil
 }
 
-func (self *SRegion) syncSecgroupRules(secgroupId string, rules []secrules.SecurityRule) error {
-	var DeleteRules []secrules.SecurityRule
-	var AddRules []secrules.SecurityRule
-
-	if secgroup, err := self.GetSecurityGroupDetails(secgroupId); err != nil {
-		return err
-	} else {
-
-		sort.Sort(secrules.SecurityRuleSet(rules))
-		sort.Sort(secrules.SecurityRuleSet(secgroup.Permissions))
-
-		i, j := 0, 0
-		for i < len(rules) || j < len(secgroup.Permissions) {
-			if i < len(rules) && j < len(secgroup.Permissions) {
-				permissionStr := secgroup.Permissions[j].String()
-				ruleStr := rules[i].String()
-				cmp := strings.Compare(permissionStr, ruleStr)
-				if cmp == 0 {
-					DeleteRules = append(DeleteRules, secgroup.Permissions[j])
-					AddRules = append(AddRules, rules[i])
-					i += 1
-					j += 1
-				} else if cmp > 0 {
-					DeleteRules = append(DeleteRules, secgroup.Permissions[j])
-					j += 1
-				} else {
-					AddRules = append(AddRules, rules[i])
-					i += 1
-				}
-			} else if i >= len(rules) {
-				DeleteRules = append(DeleteRules, secgroup.Permissions[j])
-				j += 1
-			} else if j >= len(secgroup.Permissions) {
-				AddRules = append(AddRules, rules[i])
-				i += 1
-			}
-		}
-	}
-
-	for _, r := range DeleteRules {
-		if err := self.delSecurityGroupRule(secgroupId, &r); err != nil {
-			if strings.Contains(err.Error(), "InvalidPermission.NotFound") {
-				continue
-			}
-
-			log.Errorf("delSecurityGroupRule %v error: %s", r, err.Error())
-			return err
-		}
-	}
-
-	for _, r := range AddRules {
-		if err := self.addSecurityGroupRules(secgroupId, &r); err != nil {
-			log.Errorf("addSecurityGroupRule %v error: %s", r, err.Error())
-			return err
-		}
-	}
-
-	return nil
-}
-
-func (self *SRegion) getSecRules(ingress []*ec2.IpPermission, egress []*ec2.IpPermission) []secrules.SecurityRule {
-	rules := []secrules.SecurityRule{}
+func (self *SRegion) getSecRules(ingress []*ec2.IpPermission, egress []*ec2.IpPermission) []cloudprovider.SecurityRule {
+	rules := []cloudprovider.SecurityRule{}
 	for _, p := range ingress {
 		ret, err := AwsIpPermissionToYunion(secrules.SecurityRuleIngress, *p)
 		if err != nil {
@@ -511,9 +453,24 @@ func (self *SSecurityGroup) GetProjectId() string {
 	return ""
 }
 
-func (self *SSecurityGroup) SyncRules(rules []secrules.SecurityRule) error {
-	rules = SecurityRuleSetToAllowSet(rules)
-	return self.vpc.region.syncSecgroupRules(self.SecurityGroupId, rules)
+func (self *SSecurityGroup) SyncRules(common, inAdds, outAdds, inDels, outDels []cloudprovider.SecurityRule) error {
+	for _, r := range append(inDels, outDels...) {
+		err := self.vpc.region.DelSecurityGroupRule(self.SecurityGroupId, r)
+		if err != nil {
+			if strings.Contains(err.Error(), "InvalidPermission.NotFound") {
+				continue
+			}
+			return errors.Wrapf(err, "delSecurityGroupRule %s %d %s", r.Name, r.Priority, r.String())
+		}
+	}
+
+	for _, r := range append(inAdds, outAdds...) {
+		err := self.vpc.region.addSecurityGroupRules(self.SecurityGroupId, r)
+		if err != nil {
+			return errors.Wrapf(err, "addSecurityGroupRules %d %s", r.Priority, r.String())
+		}
+	}
+	return nil
 }
 
 func (self *SSecurityGroup) Delete() error {

pkg/multicloud/aws/shell/secgroup.go

@@ -17,6 +17,10 @@ package shell
 import (
 	"fmt"
 
+	"yunion.io/x/pkg/errors"
+	"yunion.io/x/pkg/util/secrules"
+
+	"yunion.io/x/onecloud/pkg/cloudprovider"
 	"yunion.io/x/onecloud/pkg/multicloud/aws"
 	"yunion.io/x/onecloud/pkg/util/shellutils"
 )
@@ -64,4 +68,17 @@ func init() {
 		return nil
 	})
 
+	type SecurityGroupRuleDeleteOption struct {
+		SECGROUP_ID string
+		RULE        string
+	}
+
+	shellutils.R(&SecurityGroupRuleDeleteOption{}, "security-group-rule-delete", "Delete  security group rule", func(cli *aws.SRegion, args *SecurityGroupRuleDeleteOption) error {
+		rule, err := secrules.ParseSecurityRule(args.RULE)
+		if err != nil {
+			return errors.Wrapf(err, "ParseSecurityRule(%s)", args.RULE)
+		}
+		return cli.DelSecurityGroupRule(args.SECGROUP_ID, cloudprovider.SecurityRule{SecurityRule: *rule})
+	})
+
 }

pkg/multicloud/aws/utils.go

@@ -18,7 +18,6 @@ import (
 	"fmt"
 	"net"
 	"reflect"
-	"regexp"
 	"sort"
 	"strings"
 
@@ -225,7 +224,7 @@ func awsProtocolToYunion(p ec2.IpPermission) string {
 	}
 }
 
-func yunionProtocolToAws(r secrules.SecurityRule) string {
+func yunionProtocolToAws(r cloudprovider.SecurityRule) string {
 	if r.Protocol == secrules.PROTO_ANY {
 		return "-1"
 	} else {
@@ -244,7 +243,7 @@ func isYunionRuleAllPorts(r secrules.SecurityRule) bool {
 	}
 }
 
-func yunionPortRangeToAws(r secrules.SecurityRule) []portRange {
+func yunionPortRangeToAws(r cloudprovider.SecurityRule) []portRange {
 	// port 0 / -1 都代表所有端口
 	portranges := []portRange{}
 	if len(r.Ports) == 0 {
@@ -287,7 +286,7 @@ func yunionPortRangeToAws(r secrules.SecurityRule) []portRange {
 }
 
 // Security Rule Transform
-func AwsIpPermissionToYunion(direction secrules.TSecurityRuleDirection, p ec2.IpPermission) ([]secrules.SecurityRule, error) {
+func AwsIpPermissionToYunion(direction secrules.TSecurityRuleDirection, p ec2.IpPermission) ([]cloudprovider.SecurityRule, error) {
 
 	if len(p.UserIdGroupPairs) > 0 {
 		return nil, fmt.Errorf("AwsIpPermissionToYunion not supported aws rule: UserIdGroupPairs specified")
@@ -301,7 +300,7 @@ func AwsIpPermissionToYunion(direction secrules.TSecurityRuleDirection, p ec2.Ip
 		log.Debugf("AwsIpPermissionToYunion ignored IPV6 rule: %s", p.Ipv6Ranges)
 	}
 
-	rules := []secrules.SecurityRule{}
+	rules := []cloudprovider.SecurityRule{}
 	isAllPorts := isAwsPermissionAllPorts(p)
 	protocol := awsProtocolToYunion(p)
 	for _, ip := range p.IpRanges {
@@ -311,24 +310,28 @@ func AwsIpPermissionToYunion(direction secrules.TSecurityRuleDirection, p ec2.Ip
 			continue
 		}
 
-		var rule secrules.SecurityRule
+		var rule cloudprovider.SecurityRule
 		if isAllPorts {
-			rule = secrules.SecurityRule{
-				Action:      secrules.SecurityRuleAllow,
-				IPNet:       ipNet,
-				Protocol:    protocol,
-				Direction:   direction,
-				Priority:    1,
-				Description: StrVal(ip.Description),
+			rule = cloudprovider.SecurityRule{
+				SecurityRule: secrules.SecurityRule{
+					Action:      secrules.SecurityRuleAllow,
+					IPNet:       ipNet,
+					Protocol:    protocol,
+					Direction:   direction,
+					Priority:    1,
+					Description: StrVal(ip.Description),
+				},
 			}
 		} else {
-			rule = secrules.SecurityRule{
-				Action:      secrules.SecurityRuleAllow,
-				IPNet:       ipNet,
-				Protocol:    protocol,
-				Direction:   direction,
-				Priority:    1,
-				Description: StrVal(ip.Description),
+			rule = cloudprovider.SecurityRule{
+				SecurityRule: secrules.SecurityRule{
+					Action:      secrules.SecurityRuleAllow,
+					IPNet:       ipNet,
+					Protocol:    protocol,
+					Direction:   direction,
+					Priority:    1,
+					Description: StrVal(ip.Description),
+				},
 			}
 
 			if p.FromPort != nil {
@@ -349,36 +352,35 @@ func AwsIpPermissionToYunion(direction secrules.TSecurityRuleDirection, p ec2.Ip
 
 // YunionSecRuleToAws 不能保证无损转换
 // 规则描述如果包含中文等字符，将被丢弃掉
-func YunionSecRuleToAws(rule secrules.SecurityRule) ([]*ec2.IpPermission, error) {
-	if rule.Action == secrules.SecurityRuleDeny {
-		return nil, fmt.Errorf("YunionSecRuleToAws ignored  aws not supported deny rule")
-	}
-
+func YunionSecRuleToAws(rule cloudprovider.SecurityRule) ([]*ec2.IpPermission, error) {
 	iprange := rule.IPNet.String()
 	if iprange == "<nil>" {
 		return nil, fmt.Errorf("YunionSecRuleToAws ignored  ipnet should not be empty")
 	}
 
-	description := ""
-	if match, err := regexp.MatchString("^[\\sa-zA-Z0-9. _:/()#,@\\]\\[+=&;{}!$*-]+$", rule.Description); err == nil && match {
-		description = rule.Description
-	}
 	ipranges := []*ec2.IpRange{}
-	ipranges = append(ipranges, &ec2.IpRange{CidrIp: &iprange, Description: &description})
+	ipranges = append(ipranges, &ec2.IpRange{CidrIp: &iprange})
 
 	portranges := yunionPortRangeToAws(rule)
 	protocol := yunionProtocolToAws(rule)
 	permissions := []*ec2.IpPermission{}
-	for i := range portranges {
-		port := portranges[i]
-		permission := ec2.IpPermission{
-			FromPort:   &port.Start,
+	if rule.Protocol != secrules.PROTO_ANY {
+		for i := range portranges {
+			port := portranges[i]
+			permission := ec2.IpPermission{
+				FromPort:   &port.Start,
+				IpProtocol: &protocol,
+				IpRanges:   ipranges,
+				ToPort:     &port.End,
+			}
+
+			permissions = append(permissions, &permission)
+		}
+	} else {
+		permissions = append(permissions, &ec2.IpPermission{
 			IpProtocol: &protocol,
 			IpRanges:   ipranges,
-			ToPort:     &port.End,
-		}
-
-		permissions = append(permissions, &permission)
+		})
 	}
 
 	return permissions, nil

pkg/multicloud/aws/vpc.go

@@ -21,9 +21,7 @@ import (
 	"github.com/aws/aws-sdk-go/service/ec2"
 
 	"yunion.io/x/jsonutils"
-	"yunion.io/x/log"
 	"yunion.io/x/pkg/errors"
-	"yunion.io/x/pkg/util/secrules"
 
 	api "yunion.io/x/onecloud/pkg/apis/compute"
 	"yunion.io/x/onecloud/pkg/cloudprovider"
@@ -166,43 +164,6 @@ func (self *SVpc) GetIWireById(wireId string) (cloudprovider.ICloudWire, error)
 	return nil, ErrorNotFound()
 }
 
-func (self *SRegion) SyncSecurityGroup(secgroupId string, vpcId string, name string, desc string, rules []secrules.SecurityRule) (string, error) {
-	secgrpId := ""
-	// 名称为default的安全组与aws默认安全组名冲突
-	if strings.ToLower(name) == "default" {
-		name = randomString(fmt.Sprintf("%s-", vpcId), 9)
-	}
-
-	rules = SecurityRuleSetToAllowSet(rules)
-	if secgroup, err := self.getSecurityGroupById(vpcId, secgroupId); err != nil {
-		if len(desc) == 0 {
-			desc = fmt.Sprintf("security group %s for vpc %s", name, vpcId)
-		}
-
-		if secgrpId, err = self.CreateSecurityGroup(vpcId, name, secgroupId, desc); err != nil {
-			return "", err
-		}
-
-		//addRules
-		for _, rule := range rules {
-			if err := self.addSecurityGroupRule(secgrpId, &rule); err != nil {
-				return "", err
-			}
-		}
-	} else {
-		//syncRules
-		secgrpId = secgroup.SecurityGroupId
-		log.Debugf("Sync Rules for %s", secgroup.GetName())
-		if secgroup.GetName() != name {
-			if err := self.modifySecurityGroup(secgrpId, name, ""); err != nil {
-				log.Errorf("Change SecurityGroup name to %s failed: %v", name, err)
-			}
-		}
-		self.syncSecgroupRules(secgrpId, rules)
-	}
-	return secgrpId, nil
-}
-
 func (self *SVpc) getWireByZoneId(zoneId string) *SWire {
 	for i := 0; i < len(self.iwires); i += 1 {
 		wire := self.iwires[i].(*SWire)

pkg/multicloud/azure/classic_secruitygroup.go

@@ -17,19 +17,21 @@ package azure
 import (
 	"fmt"
 	"net"
-	"sort"
 	"strconv"
 	"strings"
+	"unicode"
 
 	"yunion.io/x/jsonutils"
-	"yunion.io/x/log"
+	"yunion.io/x/pkg/errors"
 	"yunion.io/x/pkg/util/secrules"
 	"yunion.io/x/pkg/utils"
 
 	"yunion.io/x/onecloud/pkg/cloudprovider"
+	"yunion.io/x/onecloud/pkg/multicloud"
 )
 
 type SClassicSecurityGroup struct {
+	multicloud.SSecurityGroup
 	region     *SRegion
 	vpc        *SClassicVpc
 	Properties ClassicSecurityGroupProperties `json:"properties,omitempty"`
@@ -65,31 +67,14 @@ type SClassicSecurityGroupRule struct {
 	Type       string
 }
 
-type ClassicSecurityRulesSet []SClassicSecurityGroupRule
-
-func (v ClassicSecurityRulesSet) Len() int {
-	return len(v)
-}
-
-func (v ClassicSecurityRulesSet) Swap(i, j int) {
-	v[i], v[j] = v[j], v[i]
-}
-
-func (v ClassicSecurityRulesSet) Less(i, j int) bool {
-	if v[i].Properties.Priority < v[j].Properties.Priority {
-		return true
-	} else if v[i].Properties.Priority == v[j].Properties.Priority {
-		return strings.Compare(v[i].Properties.String(), v[j].Properties.String()) <= 0
-	}
-	return false
-}
-
-func (self *ClassicSecurityGroupRuleProperties) toRules() []secrules.SecurityRule {
-	rules := []secrules.SecurityRule{}
-	rule := secrules.SecurityRule{
-		Action:    secrules.TSecurityRuleAction(strings.ToLower(self.Action)),
-		Direction: secrules.TSecurityRuleDirection(strings.Replace(strings.ToLower(self.Type), "bound", "", -1)),
-		Protocol:  strings.ToLower(self.Protocol),
+func (self *ClassicSecurityGroupRuleProperties) toRule() *cloudprovider.SecurityRule {
+	rule := cloudprovider.SecurityRule{
+		SecurityRule: secrules.SecurityRule{
+			Action:    secrules.TSecurityRuleAction(strings.ToLower(self.Action)),
+			Direction: secrules.TSecurityRuleDirection(strings.Replace(strings.ToLower(self.Type), "bound", "", -1)),
+			Protocol:  strings.ToLower(self.Protocol),
+			Priority:  int(self.Priority),
+		},
 	}
 	if rule.Protocol == "*" {
 		rule.Protocol = "any"
@@ -102,7 +87,7 @@ func (self *ClassicSecurityGroupRuleProperties) toRules() []secrules.SecurityRul
 	}
 
 	if utils.IsInStringArray(ip, []string{"INTERNET", "VIRTUAL_NETWORK", "AZURE_LOADBALANCER"}) {
-		return rules
+		return nil
 	}
 
 	if ip == "*" {
@@ -127,23 +112,7 @@ func (self *ClassicSecurityGroupRuleProperties) toRules() []secrules.SecurityRul
 		rule.PortStart, _ = strconv.Atoi(ports[0])
 		rule.PortEnd, _ = strconv.Atoi(ports[0])
 	}
-	if rule.PortStart > 0 && rule.Protocol == "any" {
-		rule.Protocol = secrules.PROTO_TCP
-		rules = append(rules, rule)
-		rule.Protocol = secrules.PROTO_UDP
-		rules = append(rules, rule)
-		rule.Protocol = secrules.PROTO_ICMP
-		rules = append(rules, rule)
-	}
-	return rules
-}
-
-func (self *ClassicSecurityGroupRuleProperties) String() string {
-	result := []string{}
-	for _, rule := range self.toRules() {
-		result = append(result, rule.String())
-	}
-	return strings.Join(result, ";")
+	return &rule
 }
 
 func (self *SClassicSecurityGroup) GetVpcId() string {
@@ -174,33 +143,27 @@ func (self *SClassicSecurityGroup) GetName() string {
 	return self.Name
 }
 
-func (self *SClassicSecurityGroup) GetRules() ([]secrules.SecurityRule, error) {
-	rules := make([]secrules.SecurityRule, 0)
+func (self *SClassicSecurityGroup) GetRules() ([]cloudprovider.SecurityRule, error) {
+	rules := make([]cloudprovider.SecurityRule, 0)
 	secgrouprules, err := self.region.getClassicSecurityGroupRules(self.ID)
 	if err != nil {
 		return nil, err
 	}
-	sort.Sort(ClassicSecurityRulesSet(secgrouprules))
-	priority := 100
 
 	for i := 0; i < len(secgrouprules); i++ {
 		if secgrouprules[i].Properties.Priority >= 65000 {
 			continue
 		}
-		_rules := secgrouprules[i].Properties.toRules()
-		for i := 0; i < len(_rules); i++ {
-			rule := _rules[i]
-			rule.Priority = priority
-			rule.Description = secgrouprules[i].Name
-			if err := rule.ValidateRule(); err != nil {
-				log.Errorf("Azure classic secgroup get rules error: %v", err)
-				return nil, err
-			}
-			rules = append(rules, rule)
+		rule := secgrouprules[i].Properties.toRule()
+		if rule == nil {
+			continue
 		}
-		if len(_rules) > 0 {
-			priority--
+		rule.Name = secgrouprules[i].Name
+		rule.ExternalId = secgrouprules[i].ID
+		if err := rule.ValidateRule(); err != nil && err != secrules.ErrInvalidPriority {
+			return nil, errors.Wrap(err, "rule.ValidateRule")
 		}
+		rules = append(rules, *rule)
 	}
 	return rules, nil
 }
@@ -262,18 +225,32 @@ func (self *SClassicSecurityGroup) Refresh() error {
 	return jsonutils.Update(self, sec)
 }
 
-func convertClassicSecurityGroupRules(rule secrules.SecurityRule, priority int32) ([]SClassicSecurityGroupRule, error) {
-	name := strings.Replace(rule.String(), ":", "_", -1)
-	name = strings.Replace(name, " ", "_", -1)
-	name = strings.Replace(name, "-", "_", -1)
-	name = strings.Replace(name, "/", "_", -1)
-	name = fmt.Sprintf("%s_%d", name, rule.Priority)
+func convertClassicSecurityGroupRules(rule cloudprovider.SecurityRule) ([]SClassicSecurityGroupRule, error) {
 	rules := []SClassicSecurityGroupRule{}
+	if len(rule.Name) == 0 {
+		rule.Name = fmt.Sprintf("%s_%d", rule.String(), rule.Priority)
+	}
+	rule.Name = func(name string) string {
+		// 名称必须以字母或数字开头，以字母、数字或下划线结尾，并且只能包含字母、数字、下划线、句点或连字符
+		for _, s := range name {
+			if !(unicode.IsDigit(s) || unicode.IsLetter(s) || s == '.' || s == '-' || s == '_') {
+				name = strings.ReplaceAll(name, string(s), "_")
+			}
+		}
+		if !unicode.IsDigit(rune(name[0])) && !unicode.IsLetter(rune(name[0])) {
+			name = fmt.Sprintf("r_%s", name)
+		}
+		last := len(name) - 1
+		if !unicode.IsDigit(rune(name[last])) && !unicode.IsLetter(rune(name[last])) && name[last] != '_' {
+			name = fmt.Sprintf("%s_", name)
+		}
+		return name
+	}(rule.Name)
 	secRule := SClassicSecurityGroupRule{
-		Name: name,
+		Name: rule.Name,
 		Properties: ClassicSecurityGroupRuleProperties{
 			Action:                   utils.Capitalize(string(rule.Action)),
-			Priority:                 priority,
+			Priority:                 int32(rule.Priority),
 			Type:                     utils.Capitalize(string(rule.Direction)) + "bound",
 			Protocol:                 utils.Capitalize(rule.Protocol),
 			SourcePortRange:          "*",
@@ -286,7 +263,7 @@ func convertClassicSecurityGroupRules(rule secrules.SecurityRule, priority int32
 		secRule.Properties.Protocol = "*"
 	}
 	if rule.Protocol == secrules.PROTO_ICMP {
-		return nil, nil
+		return nil, fmt.Errorf("not support icmp protocol")
 	}
 	ipAddr := "*"
 	if rule.IPNet != nil {
@@ -319,72 +296,42 @@ func (self *SRegion) getClassicSecurityGroupRules(secgroupId string) ([]SClassic
 	return rules, result.Unmarshal(&rules, "value")
 }
 
-func (self *SRegion) syncClassicSecgroupRules(secgroupId string, rules []secrules.SecurityRule) (string, error) {
-	secgrouprules, err := self.getClassicSecurityGroupRules(secgroupId)
-	if err != nil {
-		return "", err
-	}
-	for _, rule := range secgrouprules {
-		if rule.Properties.Priority >= 65000 {
-			continue
-		}
-		if err := self.client.Delete(rule.ID); err != nil {
-			return "", err
-		}
-	}
-	sort.Sort(secrules.SecurityRuleSet(rules))
-	priority := int32(100)
-	ruleStrs := []string{}
-	for i, _rule := range rules {
-		ruleStr := rules[i].String()
-		if !utils.IsInStringArray(ruleStr, ruleStrs) {
-			_rules, err := convertClassicSecurityGroupRules(_rule, priority)
-			if err != nil {
-				return "", err
-			}
-			priority++
-			ruleStrs = append(ruleStrs, ruleStr)
-			for _, rule := range _rules {
-				if err := self.addClassicSecgroupRule(secgroupId, rule); err != nil {
-					return "", err
-				}
-			}
-		}
-	}
-	return secgroupId, nil
-}
-
 func (self *SRegion) addClassicSecgroupRule(secgroupId string, rule SClassicSecurityGroupRule) error {
 	url := fmt.Sprintf("%s/securityRules/%s?api-version=2015-06-01", secgroupId, rule.Name)
 	_, err := self.client.jsonRequest("PUT", url, jsonutils.Marshal(rule).String())
 	return err
 }
 
-func (region *SRegion) syncClassicSecurityGroup(secgroupId, name, desc string, rules []secrules.SecurityRule) (string, error) {
-	if len(secgroupId) > 0 {
-		if _, err := region.GetClassicSecurityGroupDetails(secgroupId); err != nil {
-			if err != cloudprovider.ErrNotFound {
-				return "", err
-			}
-			secgroupId = ""
-		}
-	}
-
-	if len(secgroupId) == 0 {
-		secgroup, err := region.CreateClassicSecurityGroup(name)
-		if err != nil {
-			return "", err
-		}
-		secgroupId = secgroup.ID
-	}
-	return region.syncClassicSecgroupRules(secgroupId, rules)
-}
-
 func (self *SClassicSecurityGroup) GetProjectId() string {
 	return getResourceGroup(self.ID)
 }
 
-func (self *SClassicSecurityGroup) SyncRules(rules []secrules.SecurityRule) error {
-	_, err := self.region.syncClassicSecgroupRules(self.ID, rules)
-	return err
+func (self *SClassicSecurityGroup) SyncRules(common, inAdds, outAdds, inDels, outDels []cloudprovider.SecurityRule) error {
+	for _, r := range append(inDels, outDels...) {
+		err := self.region.client.Delete(r.ExternalId)
+		if err != nil {
+			return errors.Wrapf(err, "Delete(%s)", r.ExternalId)
+		}
+		for _, r := range append(inAdds, outAdds...) {
+			_rules, err := convertClassicSecurityGroupRules(r)
+			if err != nil {
+				return errors.Wrapf(err, "convertClassicSecurityGroupRules(%s)", r.String())
+			}
+			names := []string{}
+			for _, r := range _rules {
+				for {
+					if !utils.IsInStringArray(r.Name, names) {
+						names = append(names, r.Name)
+						break
+					}
+					r.Name = fmt.Sprintf("%s_", r.Name)
+				}
+				err = self.region.addClassicSecgroupRule(self.ID, r)
+				if err != nil {
+					return errors.Wrap(err, "addClassicSecgroupRule")
+				}
+			}
+		}
+	}
+	return nil
 }

pkg/multicloud/azure/classic_vpc.go

@@ -128,6 +128,7 @@ func (self *SClassicVpc) getClassicSecurityGroups() ([]SClassicSecurityGroup, er
 	}
 	for i := 0; i < len(securityGroups); i++ {
 		securityGroups[i].vpc = self
+		securityGroups[i].region = self.region
 	}
 	return securityGroups, nil
 }

pkg/multicloud/azure/region.go

@@ -22,7 +22,6 @@ import (
 	"yunion.io/x/jsonutils"
 	"yunion.io/x/log"
 	"yunion.io/x/pkg/errors"
-	"yunion.io/x/pkg/util/secrules"
 
 	api "yunion.io/x/onecloud/pkg/apis/compute"
 	"yunion.io/x/onecloud/pkg/cloudprovider"
@@ -570,33 +569,11 @@ func (region *SRegion) GetISecurityGroupByName(vpcId string, name string) (cloud
 
 func (region *SRegion) CreateISecurityGroup(conf *cloudprovider.SecurityGroupCreateInput) (cloudprovider.ICloudSecurityGroup, error) {
 	if conf.VpcId == "classic" {
-		return region.CreateClassicSecurityGroup(conf.Desc)
+		return region.CreateClassicSecurityGroup(conf.Name)
 	}
 	return region.CreateSecurityGroup(conf.Name)
 }
 
-func (region *SRegion) SyncSecurityGroup(secgroupId, vpcId, name, desc string, rules []secrules.SecurityRule) (string, error) {
-	if vpcId == "classic" {
-		return region.syncClassicSecurityGroup(secgroupId, name, desc, rules)
-	}
-	if len(secgroupId) > 0 {
-		if _, err := region.GetSecurityGroupDetails(secgroupId); err != nil {
-			if err != cloudprovider.ErrNotFound {
-				return "", err
-			}
-			secgroupId = ""
-		}
-	}
-	if len(secgroupId) == 0 {
-		secgroup, err := region.CreateSecurityGroup(name)
-		if err != nil {
-			return "", err
-		}
-		secgroupId = secgroup.ID
-	}
-	return region.updateSecurityGroupRules(secgroupId, rules)
-}
-
 func (region *SRegion) GetILoadBalancers() ([]cloudprovider.ICloudLoadbalancer, error) {
 	return nil, cloudprovider.ErrNotImplemented
 }

pkg/multicloud/azure/securitygroup.go

@@ -17,15 +17,19 @@ package azure
 import (
 	"fmt"
 	"net"
-	"sort"
 	"strconv"
 	"strings"
+	"unicode"
 
 	"yunion.io/x/jsonutils"
 	"yunion.io/x/log"
+	"yunion.io/x/pkg/errors"
 	"yunion.io/x/pkg/util/regutils"
 	"yunion.io/x/pkg/util/secrules"
 	"yunion.io/x/pkg/utils"
+
+	"yunion.io/x/onecloud/pkg/cloudprovider"
+	"yunion.io/x/onecloud/pkg/multicloud"
 )
 
 type SecurityRulePropertiesFormat struct {
@@ -50,25 +54,6 @@ type SecurityRules struct {
 	ID         string
 }
 
-type SecurityRulesSet []SecurityRules
-
-func (v SecurityRulesSet) Len() int {
-	return len(v)
-}
-
-func (v SecurityRulesSet) Swap(i, j int) {
-	v[i], v[j] = v[j], v[i]
-}
-
-func (v SecurityRulesSet) Less(i, j int) bool {
-	if v[i].Properties.Priority < v[j].Properties.Priority {
-		return true
-	} else if v[i].Properties.Priority == v[j].Properties.Priority {
-		return strings.Compare(v[i].Properties.String(), v[j].Properties.String()) <= 0
-	}
-	return false
-}
-
 type Interface struct {
 	ID string
 }
@@ -81,6 +66,7 @@ type SecurityGroupPropertiesFormat struct {
 	ProvisioningState    string          //Possible values are: 'Updating', 'Deleting', and 'Failed'
 }
 type SSecurityGroup struct {
+	multicloud.SSecurityGroup
 	vpc        *SVpc
 	region     *SRegion
 	Properties *SecurityGroupPropertiesFormat `json:"properties,omitempty"`
@@ -204,15 +190,16 @@ func paresPortsWithIpNet(port string, ports []string, ip string, ips []string) (
 	return portsResult, ipResult, nil
 }
 
-func (self *SecurityRulePropertiesFormat) toRules() ([]secrules.SecurityRule, error) {
-	result := []secrules.SecurityRule{}
-	rule := secrules.SecurityRule{
-		Action:      secrules.TSecurityRuleAction(strings.ToLower(self.Access)),
-		Direction:   secrules.TSecurityRuleDirection(strings.Replace(strings.ToLower(self.Direction), "bound", "", -1)),
-		Protocol:    strings.ToLower(self.Protocol),
-		Priority:    int(self.Priority),
-		Description: self.Description,
-	}
+func (self *SecurityRulePropertiesFormat) toRules() ([]cloudprovider.SecurityRule, error) {
+	result := []cloudprovider.SecurityRule{}
+	rule := cloudprovider.SecurityRule{
+		SecurityRule: secrules.SecurityRule{
+			Action:      secrules.TSecurityRuleAction(strings.ToLower(self.Access)),
+			Direction:   secrules.TSecurityRuleDirection(strings.Replace(strings.ToLower(self.Direction), "bound", "", -1)),
+			Protocol:    strings.ToLower(self.Protocol),
+			Priority:    int(self.Priority),
+			Description: self.Description,
+		}}
 
 	if rule.Protocol == "*" {
 		rule.Protocol = "any"
@@ -240,47 +227,15 @@ func (self *SecurityRulePropertiesFormat) toRules() ([]secrules.SecurityRule, er
 
 	for i := 0; i < len(ips); i++ {
 		rule.IPNet = ips[i]
-		withICMP := false
 		for j := 0; j < len(ports); j++ {
 			rule.Ports = ports[j].ports
 			rule.PortStart = ports[j].portStart
 			rule.PortEnd = ports[j].portEnd
-			if rule.Protocol == secrules.PROTO_ANY && len(rule.Ports) > 0 || (rule.PortStart+rule.PortStart > 0) {
-				tcp := rule
-				tcp.Protocol = secrules.PROTO_TCP
-				err := tcp.ValidateRule()
-				if err != nil {
-					return nil, err
-				}
-				result = append(result, tcp)
-
-				udp := rule
-				udp.Protocol = secrules.PROTO_UDP
-				err = udp.ValidateRule()
-				if err != nil {
-					return nil, err
-				}
-				result = append(result, udp)
-				withICMP = true
-			} else {
-				err := rule.ValidateRule()
-				if err != nil {
-					return nil, err
-				}
-				result = append(result, rule)
-			}
-		}
-		if withICMP {
-			icmp := rule
-			icmp.Protocol = secrules.PROTO_ICMP
-			icmp.PortStart = -1
-			icmp.PortEnd = -1
-			icmp.Ports = []int{}
-			err := icmp.ValidateRule()
-			if err != nil {
+			err := rule.ValidateRule()
+			if err != nil && err != secrules.ErrInvalidPriority {
 				return nil, err
 			}
-			result = append(result, icmp)
+			result = append(result, rule)
 		}
 	}
 
@@ -316,24 +271,20 @@ func (self *SSecurityGroup) GetName() string {
 	return self.Name
 }
 
-func (self *SSecurityGroup) GetRules() ([]secrules.SecurityRule, error) {
-	rules := make([]secrules.SecurityRule, 0)
+func (self *SSecurityGroup) GetRules() ([]cloudprovider.SecurityRule, error) {
+	rules := make([]cloudprovider.SecurityRule, 0)
 	if self.Properties.SecurityRules == nil {
 		return rules, nil
 	}
-	sort.Sort(SecurityRulesSet(self.Properties.SecurityRules))
-	priority := 100
 	for _, _rule := range self.Properties.SecurityRules {
-		_rule.Properties.Priority = int32(priority)
 		secRules, err := _rule.Properties.toRules()
 		if err != nil {
-			log.Errorf("Azure convert rule %v error: %v", _rule, err)
-			return nil, err
+			return nil, errors.Wrap(err, "_rule.Properties.toRules")
 		}
-		if len(secRules) > 0 {
-			priority--
+		for i := range secRules {
+			secRules[i].Name = _rule.Name
+			rules = append(rules, secRules[i])
 		}
-		rules = append(rules, secRules...)
 	}
 	return rules, nil
 }
@@ -392,7 +343,7 @@ func (self *SSecurityGroup) Refresh() error {
 	return jsonutils.Update(self, sec)
 }
 
-func convertRulePort(rule secrules.SecurityRule) []string {
+func convertRulePort(rule cloudprovider.SecurityRule) []string {
 	ports := []string{}
 	if len(rule.Ports) > 0 {
 		for i := 0; i < len(rule.Ports); i++ {
@@ -409,18 +360,31 @@ func convertRulePort(rule secrules.SecurityRule) []string {
 	return ports
 }
 
-func convertSecurityGroupRule(rule secrules.SecurityRule, priority int32) *SecurityRules {
-	name := strings.Replace(rule.String(), ":", "_", -1)
-	name = strings.Replace(name, " ", "_", -1)
-	name = strings.Replace(name, "-", "_", -1)
-	name = strings.Replace(name, "/", "_", -1)
-	name = strings.Replace(name, ",", "_", -1)
-	name = fmt.Sprintf("%s_%d", name, rule.Priority)
+func convertSecurityGroupRule(rule cloudprovider.SecurityRule) *SecurityRules {
+	if len(rule.Name) == 0 {
+		rule.Name = fmt.Sprintf("%s_%d", rule.String(), rule.Priority)
+	}
+	rule.Name = func(name string) string {
+		// 名称必须以字母或数字开头，以字母、数字或下划线结尾，并且只能包含字母、数字、下划线、句点或连字符
+		for _, s := range name {
+			if !(unicode.IsDigit(s) || unicode.IsLetter(s) || s == '.' || s == '-' || s == '_') {
+				name = strings.ReplaceAll(name, string(s), "_")
+			}
+		}
+		if !unicode.IsDigit(rune(name[0])) && !unicode.IsLetter(rune(name[0])) {
+			name = fmt.Sprintf("r_%s", name)
+		}
+		last := len(name) - 1
+		if !unicode.IsDigit(rune(name[last])) && !unicode.IsLetter(rune(name[last])) && name[last] != '_' {
+			name = fmt.Sprintf("%s_", name)
+		}
+		return name
+	}(rule.Name)
 	destRule := SecurityRules{
-		Name: name,
+		Name: rule.Name,
 		Properties: SecurityRulePropertiesFormat{
 			Access:                   utils.Capitalize(string(rule.Action)),
-			Priority:                 priority,
+			Priority:                 int32(rule.Priority),
 			Protocol:                 "*",
 			Direction:                utils.Capitalize((string(rule.Direction) + "bound")),
 			Description:              rule.Description,
@@ -435,10 +399,6 @@ func convertSecurityGroupRule(rule secrules.SecurityRule, priority int32) *Secur
 		destRule.Properties.Protocol = utils.Capitalize(rule.Protocol)
 	}
 
-	if rule.Protocol == secrules.PROTO_ICMP {
-		return nil
-	}
-
 	if len(destRule.Properties.DestinationPortRanges) > 0 {
 		destRule.Properties.DestinationPortRange = ""
 	}
@@ -455,31 +415,6 @@ func convertSecurityGroupRule(rule secrules.SecurityRule, priority int32) *Secur
 	return &destRule
 }
 
-func (region *SRegion) updateSecurityGroupRules(secgroupId string, rules []secrules.SecurityRule) (string, error) {
-	secgroup, err := region.GetSecurityGroupDetails(secgroupId)
-	if err != nil {
-		return "", err
-	}
-	sort.Sort(secrules.SecurityRuleSet(rules))
-	securityRules := []SecurityRules{}
-	priority := int32(100)
-	ruleStrs := []string{}
-	for i := 0; i < len(rules); i++ {
-		ruleStr := rules[i].String()
-		if !utils.IsInStringArray(ruleStr, ruleStrs) {
-			rule := convertSecurityGroupRule(rules[i], priority)
-			if rule != nil {
-				securityRules = append(securityRules, *rule)
-				priority++
-			}
-			ruleStrs = append(ruleStrs, ruleStr)
-		}
-	}
-	secgroup.Properties.SecurityRules = securityRules
-	secgroup.Properties.ProvisioningState = ""
-	return secgroup.ID, region.client.Update(jsonutils.Marshal(secgroup), nil)
-}
-
 func (region *SRegion) AttachSecurityToInterfaces(secgroupId string, nicIds []string) error {
 	for _, nicId := range nicIds {
 		nic, err := region.GetNetworkInterfaceDetail(nicId)
@@ -526,7 +461,28 @@ func (self *SSecurityGroup) Delete() error {
 	return self.region.client.Delete(self.ID)
 }
 
-func (self *SSecurityGroup) SyncRules(rules []secrules.SecurityRule) error {
-	_, err := self.region.updateSecurityGroupRules(self.ID, rules)
-	return err
+func (self *SSecurityGroup) SetRules(rules []cloudprovider.SecurityRule) error {
+	names := []string{}
+	securityRules := []SecurityRules{}
+	for i := 0; i < len(rules); i++ {
+		rule := convertSecurityGroupRule(rules[i])
+		if rule != nil {
+			for {
+				if !utils.IsInStringArray(rule.Name, names) {
+					names = append(names, rule.Name)
+					break
+				}
+				rule.Name = fmt.Sprintf("%s_", rule.Name)
+			}
+			securityRules = append(securityRules, *rule)
+		}
+	}
+	self.Properties.SecurityRules = securityRules
+	self.Properties.ProvisioningState = ""
+	return self.region.client.Update(jsonutils.Marshal(self), nil)
+}
+
+func (self *SSecurityGroup) SyncRules(common, inAdds, outAdds, inDels, outDels []cloudprovider.SecurityRule) error {
+	rules := append(common, append(inAdds, outAdds...)...)
+	return self.SetRules(rules)
 }

pkg/multicloud/ctyun/region.go

@@ -20,7 +20,6 @@ import (
 
 	"yunion.io/x/jsonutils"
 	"yunion.io/x/pkg/errors"
-	"yunion.io/x/pkg/util/secrules"
 
 	api "yunion.io/x/onecloud/pkg/apis/compute"
 	"yunion.io/x/onecloud/pkg/cloudprovider"
@@ -153,11 +152,6 @@ func (self *SRegion) CreateISecurityGroup(conf *cloudprovider.SecurityGroupCreat
 		return nil, errors.Wrap(err, "Region.CreateISecurityGroup")
 	}
 
-	err = self.syncSecgroupRules(secgroup.GetId(), conf.Rules)
-	if err != nil {
-		return nil, errors.Wrap(err, "Region.CreateISecurityGroup.syncSecgroupRules")
-	}
-
 	return secgroup, nil
 }
 
@@ -297,28 +291,6 @@ func (self *SRegion) DeleteSecurityGroup(securityGroupId string) error {
 	return nil
 }
 
-func (self *SRegion) SyncSecurityGroup(secgroupId string, vpcId string, name string, desc string, rules []secrules.SecurityRule) (string, error) {
-	if len(secgroupId) > 0 {
-		_, err := self.GetSecurityGroupDetails(secgroupId)
-		if err == cloudprovider.ErrNotFound {
-			secgroupId = ""
-		} else if err != nil {
-			return "", errors.Wrapf(err, "self.GetSecurityGroupDetails(%s)", secgroupId)
-		}
-	}
-
-	if len(secgroupId) == 0 {
-		secgroup, err := self.CreateSecurityGroup(vpcId, name)
-		if err != nil {
-			return "", errors.Wrap(err, "self.CreateSecurityGroup")
-		}
-		secgroupId = secgroup.GetId()
-	}
-
-	rules = SecurityRuleSetToAllowSet(rules)
-	return secgroupId, self.syncSecgroupRules(secgroupId, rules)
-}
-
 func (self *SRegion) CreateIVpc(name string, desc string, cidr string) (cloudprovider.ICloudVpc, error) {
 	return self.CreateVpc(name, cidr)
 }

pkg/multicloud/ctyun/securitygroup.go

@@ -17,18 +17,18 @@ package ctyun
 import (
 	"fmt"
 	"net"
-	"sort"
-	"strings"
 
 	"yunion.io/x/jsonutils"
-	"yunion.io/x/log"
 	"yunion.io/x/pkg/errors"
 	"yunion.io/x/pkg/util/secrules"
 
 	apis "yunion.io/x/onecloud/pkg/apis/compute"
+	"yunion.io/x/onecloud/pkg/cloudprovider"
+	"yunion.io/x/onecloud/pkg/multicloud"
 )
 
 type SSecurityGroup struct {
+	multicloud.SSecurityGroup
 	region *SRegion
 
 	ResSecurityGroupID string `json:"resSecurityGroupId"`
@@ -42,136 +42,11 @@ type SSecurityGroup struct {
 	Status             int64  `json:"status"`
 }
 
-// 将安全组规则全部转换为等价的allow规则
-func SecurityRuleSetToAllowSet(srs secrules.SecurityRuleSet) secrules.SecurityRuleSet {
-	inRuleSet := secrules.SecurityRuleSet{}
-	outRuleSet := secrules.SecurityRuleSet{}
-
-	for _, rule := range srs {
-		if rule.Direction == secrules.SecurityRuleIngress {
-			inRuleSet = append(inRuleSet, rule)
-		}
-
-		if rule.Direction == secrules.SecurityRuleEgress {
-			outRuleSet = append(outRuleSet, rule)
-		}
-	}
-
-	sort.Sort(inRuleSet)
-	sort.Sort(outRuleSet)
-
-	inRuleSet = inRuleSet.AllowList()
-	// out方向空规则默认全部放行
-	if outRuleSet.Len() == 0 {
-		_, ipNet, _ := net.ParseCIDR("0.0.0.0/0")
-		outRuleSet = append(outRuleSet, secrules.SecurityRule{
-			Priority:  0,
-			Action:    secrules.SecurityRuleAllow,
-			IPNet:     ipNet,
-			Protocol:  secrules.PROTO_ANY,
-			Direction: secrules.SecurityRuleEgress,
-			PortStart: -1,
-			PortEnd:   -1,
-		})
-	}
-	outRuleSet = outRuleSet.AllowList()
-
-	ret := secrules.SecurityRuleSet{}
-	ret = append(ret, inRuleSet...)
-	ret = append(ret, outRuleSet...)
-	return ret
-}
-
-func (self *SSecurityGroup) GetRulesWithExtId() ([]secrules.SecurityRule, error) {
-	_rules, err := self.region.GetSecurityGroupRules(self.GetId())
-	if err != nil {
-		return nil, errors.Wrap(err, "SSecurityGroup.GetRulesWithExtId.GetSecurityGroupRules")
-	}
-
-	rules := make([]secrules.SecurityRule, 0)
-	for _, r := range _rules {
-		if !compatibleSecurityGroupRule(r) {
-			continue
-		}
-
-		rule, err := self.GetSecurityRule(r, true)
-		if err != nil {
-			return rules, err
-		}
-
-		rules = append(rules, rule)
-	}
-
-	return rules, nil
-}
-
-func (self *SRegion) syncSecgroupRules(secgroupId string, srules []secrules.SecurityRule) error {
-	var DeleteRules []secrules.SecurityRule
-	var AddRules []secrules.SecurityRule
-
-	rules := SecurityRuleSetToAllowSet(srules)
-	if secgroup, err := self.GetSecurityGroupDetails(secgroupId); err != nil {
-		return errors.Wrapf(err, "syncSecgroupRules.GetSecurityGroupDetails(%s)", secgroupId)
-	} else {
-		remoteRules, err := secgroup.GetRulesWithExtId()
-		if err != nil {
-			return errors.Wrap(err, "secgroup.GetRulesWithExtId")
-		}
-
-		sort.Sort(secrules.SecurityRuleSet(rules))
-		sort.Sort(secrules.SecurityRuleSet(remoteRules))
-
-		i, j := 0, 0
-		for i < len(rules) || j < len(remoteRules) {
-			if i < len(rules) && j < len(remoteRules) {
-				permissionStr := remoteRules[j].String()
-				ruleStr := rules[i].String()
-				cmp := strings.Compare(permissionStr, ruleStr)
-				if cmp == 0 {
-					// DeleteRules = append(DeleteRules, remoteRules[j])
-					// AddRules = append(AddRules, rules[i])
-					i += 1
-					j += 1
-				} else if cmp > 0 {
-					DeleteRules = append(DeleteRules, remoteRules[j])
-					j += 1
-				} else {
-					AddRules = append(AddRules, rules[i])
-					i += 1
-				}
-			} else if i >= len(rules) {
-				DeleteRules = append(DeleteRules, remoteRules[j])
-				j += 1
-			} else if j >= len(remoteRules) {
-				AddRules = append(AddRules, rules[i])
-				i += 1
-			}
-		}
-	}
-
-	for _, r := range DeleteRules {
-		// r.Description 实际存储的是ruleId
-		if err := self.delSecurityGroupRule(r.Description); err != nil {
-			log.Errorf("delSecurityGroupRule %v error: %s", r, err.Error())
-			return err
-		}
-	}
-
-	for _, r := range AddRules {
-		if err := self.addSecurityGroupRules(secgroupId, &r); err != nil {
-			log.Errorf("addSecurityGroupRule %v error: %s", r, err.Error())
-			return err
-		}
-	}
-
-	return nil
-}
-
 func (self *SRegion) delSecurityGroupRule(secGrpRuleId string) error {
 	return self.DeleteSecurityGroupRule(secGrpRuleId)
 }
 
-func (self *SRegion) addSecurityGroupRules(secGrpId string, rule *secrules.SecurityRule) error {
+func (self *SRegion) AddSecurityGroupRules(secGrpId string, rule cloudprovider.SecurityRule) error {
 	direction := ""
 	if rule.Direction == secrules.SecurityRuleIngress {
 		direction = "ingress"
@@ -209,9 +84,20 @@ func (self *SRegion) addSecurityGroupRules(secGrpId string, rule *secrules.Secur
 	return nil
 }
 
-func (self *SSecurityGroup) SyncRules(rules []secrules.SecurityRule) error {
-	rules = SecurityRuleSetToAllowSet(rules)
-	return self.region.syncSecgroupRules(self.ResSecurityGroupID, rules)
+func (self *SSecurityGroup) SyncRules(common, inAdds, outAdds, inDels, outDels []cloudprovider.SecurityRule) error {
+	for _, r := range append(inDels, outDels...) {
+		err := self.region.delSecurityGroupRule(r.ExternalId)
+		if err != nil {
+			return errors.Wrapf(err, "delSecurityGroupRule(%s)", r.ExternalId)
+		}
+	}
+	for _, r := range append(inAdds, outAdds...) {
+		err := self.region.AddSecurityGroupRules(self.ResSecurityGroupID, r)
+		if err != nil {
+			return errors.Wrapf(err, "addSecurityGroupRule(%d %s)", r.Priority, r.String())
+		}
+	}
+	return nil
 }
 
 func (self *SSecurityGroup) Delete() error {
@@ -272,19 +158,19 @@ func compatibleSecurityGroupRule(r SSecurityGroupRule) bool {
 	return true
 }
 
-func (self *SSecurityGroup) GetRules() ([]secrules.SecurityRule, error) {
+func (self *SSecurityGroup) GetRules() ([]cloudprovider.SecurityRule, error) {
 	_rules, err := self.region.GetSecurityGroupRules(self.GetId())
 	if err != nil {
 		return nil, errors.Wrap(err, "SSecurityGroup.GetRules.GetSecurityGroupRules")
 	}
 
-	rules := make([]secrules.SecurityRule, 0)
+	rules := make([]cloudprovider.SecurityRule, 0)
 	for _, r := range _rules {
 		if !compatibleSecurityGroupRule(r) {
 			continue
 		}
 
-		rule, err := self.GetSecurityRule(r, false)
+		rule, err := self.GetSecurityRule(r)
 		if err != nil {
 			return rules, err
 		}
@@ -295,7 +181,7 @@ func (self *SSecurityGroup) GetRules() ([]secrules.SecurityRule, error) {
 	return rules, nil
 }
 
-func (self *SSecurityGroup) GetSecurityRule(remoteRule SSecurityGroupRule, withRuleId bool) (secrules.SecurityRule, error) {
+func (self *SSecurityGroup) GetSecurityRule(remoteRule SSecurityGroupRule) (cloudprovider.SecurityRule, error) {
 	var err error
 	var direction secrules.TSecurityRuleDirection
 	if remoteRule.Direction == "ingress" {
@@ -327,27 +213,22 @@ func (self *SSecurityGroup) GetSecurityRule(remoteRule SSecurityGroupRule, withR
 	}
 
 	if err != nil {
-		return secrules.SecurityRule{}, err
+		return cloudprovider.SecurityRule{}, err
 	}
 
-	// withRuleId.将ruleId附加到description字段。该hook有特殊目的，仅在同步安全组时使用。
-	desc := ""
-	if withRuleId {
-		desc = remoteRule.ID
-	} else {
-		desc = remoteRule.Description
-	}
-
-	rule := secrules.SecurityRule{
-		Priority:    1,
-		Action:      secrules.SecurityRuleAllow,
-		IPNet:       ipNet,
-		Protocol:    protocol,
-		Direction:   direction,
-		PortStart:   portStart,
-		PortEnd:     portEnd,
-		Ports:       nil,
-		Description: desc,
+	rule := cloudprovider.SecurityRule{
+		ExternalId: remoteRule.ID,
+		SecurityRule: secrules.SecurityRule{
+			Priority:    1,
+			Action:      secrules.SecurityRuleAllow,
+			IPNet:       ipNet,
+			Protocol:    protocol,
+			Direction:   direction,
+			PortStart:   portStart,
+			PortEnd:     portEnd,
+			Ports:       nil,
+			Description: remoteRule.Description,
+		},
 	}
 
 	err = rule.ValidateRule()

pkg/multicloud/ctyun/shell/secgroup.go

@@ -15,6 +15,10 @@
 package shell
 
 import (
+	"yunion.io/x/pkg/errors"
+	"yunion.io/x/pkg/util/secrules"
+
+	"yunion.io/x/onecloud/pkg/cloudprovider"
 	"yunion.io/x/onecloud/pkg/multicloud/ctyun"
 	"yunion.io/x/onecloud/pkg/util/shellutils"
 )
@@ -58,20 +62,14 @@ func init() {
 	})
 
 	type SecurityGroupRuleCreateOptions struct {
-		Group     string `help:"secgroup id"`
-		Direction string `help:"direction"`
-		Ethertype string `help:"ethertype" choice:"IPv4|IPv6"`
-		Protocol  string `help:"protocol,icmp，tcp，udp，and so on "`
-		IpPrefix  string `help:"remote ip prefix"`
-		PortMin   int64  `help:"portRangeMin"`
-		PortMax   int64  `help:"portRangeMax"`
+		GROUP string `help:"secgroup id"`
+		RULE  string
 	}
 	shellutils.R(&SecurityGroupRuleCreateOptions{}, "secrule-create", "Create secgroup rule", func(cli *ctyun.SRegion, args *SecurityGroupRuleCreateOptions) error {
-		e := cli.CreateSecurityGroupRule(args.Group, args.Direction, args.Ethertype, args.Protocol, args.IpPrefix, args.PortMin, args.PortMax)
-		if e != nil {
-			return e
+		rule, err := secrules.ParseSecurityRule(args.RULE)
+		if err != nil {
+			return errors.Wrap(err, "ParseSecurityRule")
 		}
-
-		return nil
+		return cli.AddSecurityGroupRules(args.GROUP, cloudprovider.SecurityRule{SecurityRule: *rule})
 	})
 }

pkg/multicloud/google/securitygroup.go

@@ -17,7 +17,6 @@ package google
 import (
 	"fmt"
 	"net"
-	"sort"
 	"strconv"
 	"strings"
 	"time"
@@ -29,6 +28,7 @@ import (
 	"yunion.io/x/pkg/utils"
 
 	"yunion.io/x/onecloud/pkg/cloudprovider"
+	"yunion.io/x/onecloud/pkg/multicloud"
 )
 
 const (
@@ -60,24 +60,8 @@ type SFirewall struct {
 	Kind                  string
 }
 
-type FirewallSet []SFirewall
-
-func (f FirewallSet) Len() int {
-	return len(f)
-}
-
-func (f FirewallSet) Swap(i, j int) {
-	f[i], f[j] = f[j], f[i]
-}
-
-func (f FirewallSet) Less(i, j int) bool {
-	if f[i].Priority != f[j].Priority {
-		return f[i].Priority < f[j].Priority
-	}
-	return len(f[i].Allowed) < len(f[j].Allowed)
-}
-
 type SSecurityGroup struct {
+	multicloud.SSecurityGroup
 	vpc *SVpc
 
 	ServiceAccount string
@@ -99,18 +83,20 @@ func (region *SRegion) GetFirewall(id string) (*SFirewall, error) {
 	return firewall, region.Get(id, firewall)
 }
 
-func (firewall *SFirewall) _toRules(action secrules.TSecurityRuleAction) ([]secrules.SecurityRule, error) {
-	rules := []secrules.SecurityRule{}
+func (firewall *SFirewall) _toRules(action secrules.TSecurityRuleAction) ([]cloudprovider.SecurityRule, error) {
+	rules := []cloudprovider.SecurityRule{}
 	list := firewall.Allowed
 	if action == secrules.SecurityRuleDeny {
 		list = firewall.Denied
 	}
 	for _, allow := range list {
-		rule := secrules.SecurityRule{
-			Action:      action,
-			Direction:   secrules.DIR_IN,
-			Description: firewall.SelfLink,
-			Priority:    firewall.Priority,
+		rule := cloudprovider.SecurityRule{
+			ExternalId: firewall.SelfLink,
+			SecurityRule: secrules.SecurityRule{
+				Action:    action,
+				Direction: secrules.DIR_IN,
+				Priority:  firewall.Priority,
+			},
 		}
 		if firewall.Direction == "EGRESS" {
 			rule.Direction = secrules.DIR_OUT
@@ -139,6 +125,7 @@ func (firewall *SFirewall) _toRules(action secrules.TSecurityRuleAction) ([]secr
 			ports := []int{}
 			for _, port := range allow.Ports {
 				if strings.Index(port, "-") > 0 {
+					port = strings.Replace(port, "0-", "1-", 1)
 					err := rule.ParsePorts(port)
 					if err != nil {
 						return nil, errors.Wrapf(err, "Parse port %s", port)
@@ -169,8 +156,8 @@ func (firewall *SFirewall) _toRules(action secrules.TSecurityRuleAction) ([]secr
 	return rules, nil
 }
 
-func (firewall *SFirewall) toRules() ([]secrules.SecurityRule, error) {
-	rules := []secrules.SecurityRule{}
+func (firewall *SFirewall) toRules() ([]cloudprovider.SecurityRule, error) {
+	rules := []cloudprovider.SecurityRule{}
 	_rules, err := firewall._toRules(secrules.SecurityRuleAllow)
 	if err != nil {
 		return nil, err
@@ -234,7 +221,7 @@ func (secgroup *SSecurityGroup) Delete() error {
 		return errors.Wrap(err, "GetRules")
 	}
 	for _, rule := range rules {
-		err = secgroup.vpc.region.DeleteSecgroupRule(rule.Description, rule)
+		err = secgroup.vpc.region.DeleteSecgroupRule(rule)
 		if err != nil {
 			return errors.Wrapf(err, "DeleteSecgroupRule(%s)", rule.Description)
 		}
@@ -250,7 +237,7 @@ func (secgroup *SSecurityGroup) GetVpcId() string {
 	return secgroup.vpc.GetGlobalId()
 }
 
-func (self *SSecurityGroup) GetRules() ([]secrules.SecurityRule, error) {
+func (self *SSecurityGroup) GetRules() ([]cloudprovider.SecurityRule, error) {
 	_firewalls, err := self.vpc.region.GetFirewalls(self.vpc.globalnetwork.SelfLink, 0, "")
 	if err != nil {
 		return nil, err
@@ -267,14 +254,8 @@ func (self *SSecurityGroup) GetRules() ([]secrules.SecurityRule, error) {
 			}
 		}
 	}
-	sort.Sort(FirewallSet(firewalls))
-	rules := []secrules.SecurityRule{}
-	priority := 100
+	rules := []cloudprovider.SecurityRule{}
 	for _, firewall := range firewalls {
-		firewall.Priority = priority
-		if priority > 2 {
-			priority--
-		}
 		_rules, err := firewall.toRules()
 		if err != nil {
 			return nil, err
@@ -284,8 +265,8 @@ func (self *SSecurityGroup) GetRules() ([]secrules.SecurityRule, error) {
 	return rules, nil
 }
 
-func (region *SRegion) DeleteSecgroupRule(ruleId string, rule secrules.SecurityRule) error {
-	firwall, err := region.GetFirewall(ruleId)
+func (region *SRegion) DeleteSecgroupRule(rule cloudprovider.SecurityRule) error {
+	firwall, err := region.GetFirewall(rule.ExternalId)
 	if err != nil {
 		return errors.Wrap(err, "region.GetFirewall")
 	}
@@ -320,61 +301,17 @@ func (region *SRegion) DeleteSecgroupRule(ruleId string, rule secrules.SecurityR
 	return region.Delete(firwall.SelfLink)
 }
 
-func (secgroup *SSecurityGroup) SyncRules(rules []secrules.SecurityRule) error {
-	if len(rules) == 0 {
-		rules = append(rules, *secrules.MustParseSecurityRule("in:deny any"))
-	}
-	currentRule, err := secgroup.GetRules()
-	if err != nil {
-		return errors.Wrap(err, "secgroup.GetRules")
-	}
-	sort.Sort(secrules.SecurityRuleSet(rules))
-	region := secgroup.vpc.region
-	deleteRules := map[string]secrules.SecurityRule{}
-	addRules := []secrules.SecurityRule{}
-	i, j := 0, 0
-	for i < len(rules) || j < len(currentRule) {
-		if i < len(rules) && j < len(currentRule) {
-			currentRuleStr := currentRule[j].String()
-			ruleStr := rules[i].String()
-			cmp := strings.Compare(currentRuleStr, ruleStr)
-			if cmp == 0 {
-				i += 1
-				j += 1
-			} else if cmp > 0 {
-				// delete rule
-				deleteRules[currentRule[j].Description] = currentRule[j]
-				j += 1
-			} else {
-				rules[i].Priority = 101 - rules[i].Priority
-				addRules = append(addRules, rules[i])
-				i += 1
-			}
-		} else if i >= len(rules) {
-			// delete rule
-			deleteRules[currentRule[j].Description] = currentRule[j]
-			j += 1
-		} else if j >= len(currentRule) {
-			// add rule
-			rules[i].Priority = 101 - rules[i].Priority
-			addRules = append(addRules, rules[i])
-			err = region.CreateSecurityGroupRule(rules[i], secgroup.vpc.globalnetwork.SelfLink, secgroup.Tag, secgroup.ServiceAccount)
-			if err != nil {
-				return errors.Wrapf(err, "region.CreateSecurityGroupRule(%s)", rules[i].String())
-			}
-			i += 1
+func (secgroup *SSecurityGroup) SyncRules(common, inAdds, outAdds, inDels, outDels []cloudprovider.SecurityRule) error {
+	for _, r := range append(inDels, outDels...) {
+		err := secgroup.vpc.region.DeleteSecgroupRule(r)
+		if err != nil {
+			return errors.Wrap(err, "DeleteSecgroupRule")
 		}
 	}
-	for id, rule := range deleteRules {
-		err = region.DeleteSecgroupRule(id, rule)
+	for _, r := range append(inAdds, outAdds...) {
+		err := secgroup.vpc.region.CreateSecurityGroupRule(r, secgroup.vpc.globalnetwork.SelfLink, secgroup.Tag, secgroup.ServiceAccount)
 		if err != nil {
-			return errors.Wrapf(err, "DeleteSecgroupRule(%s)", id)
-		}
-	}
-	for _, rule := range addRules {
-		err = region.CreateSecurityGroupRule(rule, secgroup.vpc.globalnetwork.SelfLink, secgroup.Tag, secgroup.ServiceAccount)
-		if err != nil {
-			return errors.Wrapf(err, "CreateSecurityGroupRule(%s)", rule.String())
+			return errors.Wrapf(err, "CreateSecurityGroupRule(%s)", r.String())
 		}
 	}
 	return nil
@@ -418,7 +355,7 @@ func (region *SRegion) GetISecurityGroupByName(vpcId string, name string) (cloud
 	return nil, cloudprovider.ErrNotFound
 }
 
-func (region *SRegion) CreateSecurityGroupRule(rule secrules.SecurityRule, vpcId string, tag string, serviceAccount string) error {
+func (region *SRegion) CreateSecurityGroupRule(rule cloudprovider.SecurityRule, vpcId string, tag string, serviceAccount string) error {
 	name := fmt.Sprintf("%s-%d", rule.String(), rule.Priority)
 	if len(tag) > 0 {
 		name = fmt.Sprintf("for-tag-%s-%s", tag, name)
@@ -445,7 +382,7 @@ func (region *SRegion) CreateSecurityGroupRule(rule secrules.SecurityRule, vpcId
 		body["sourceRanges"] = []string{rule.IPNet.String()}
 	}
 
-	protocol := string(rule.Protocol)
+	protocol := strings.ToLower(rule.Protocol)
 	if protocol == secrules.PROTO_ANY {
 		protocol = "all"
 	}

pkg/multicloud/huawei/region.go

@@ -16,7 +16,6 @@ package huawei
 
 import (
 	"fmt"
-	"sort"
 	"strings"
 	"time"
 
@@ -536,29 +535,6 @@ func (self *SRegion) CreateISecurityGroup(conf *cloudprovider.SecurityGroupCreat
 	return self.CreateSecurityGroup(conf.VpcId, conf.Name, conf.Desc)
 }
 
-func (self *SRegion) SyncSecurityGroup(secgroupId string, vpcId string, name string, desc string, rules []secrules.SecurityRule) (string, error) {
-	if len(secgroupId) > 0 {
-		_, err := self.GetSecurityGroupDetails(secgroupId)
-		if err == cloudprovider.ErrNotFound {
-			secgroupId = ""
-		} else if err != nil {
-			return "", errors.Wrapf(err, "self.GetSecurityGroupDetails(%s)", secgroupId)
-		}
-	}
-
-	if len(secgroupId) == 0 {
-		secgroup, err := self.CreateSecurityGroup(vpcId, name, desc)
-		if err != nil {
-			return "", errors.Wrap(err, "self.CreateSecurityGroup")
-		}
-		secgroupId = secgroup.GetId()
-	}
-
-	// 华为云默认deny。不需要显式指定
-	rules = SecurityRuleSetToAllowSet(rules)
-	return secgroupId, self.syncSecgroupRules(secgroupId, rules)
-}
-
 // https://support.huaweicloud.com/api-vpc/zh-cn_topic_0020090608.html
 func (self *SRegion) CreateIVpc(name string, desc string, cidr string) (cloudprovider.ICloudVpc, error) {
 	params := jsonutils.NewDict()
@@ -734,68 +710,6 @@ func (self *SRegion) CreateSecurityGroup(vpcId string, name string, desc string)
 	return &secgroup, err
 }
 
-func (self *SRegion) syncSecgroupRules(secgroupId string, srules []secrules.SecurityRule) error {
-	var DeleteRules []secrules.SecurityRule
-	var AddRules []secrules.SecurityRule
-
-	rules := SecurityRuleSetToAllowSet(srules)
-	if secgroup, err := self.GetSecurityGroupDetails(secgroupId); err != nil {
-		return errors.Wrapf(err, "syncSecgroupRules.GetSecurityGroupDetails(%s)", secgroupId)
-	} else {
-		remoteRules, err := secgroup.GetRulesWithExtId()
-		if err != nil {
-			return errors.Wrap(err, "secgroup.GetRulesWithExtId")
-		}
-
-		sort.Sort(secrules.SecurityRuleSet(rules))
-		sort.Sort(secrules.SecurityRuleSet(remoteRules))
-
-		i, j := 0, 0
-		for i < len(rules) || j < len(remoteRules) {
-			if i < len(rules) && j < len(remoteRules) {
-				permissionStr := remoteRules[j].String()
-				ruleStr := rules[i].String()
-				cmp := strings.Compare(permissionStr, ruleStr)
-				if cmp == 0 {
-					// DeleteRules = append(DeleteRules, remoteRules[j])
-					// AddRules = append(AddRules, rules[i])
-					i += 1
-					j += 1
-				} else if cmp > 0 {
-					DeleteRules = append(DeleteRules, remoteRules[j])
-					j += 1
-				} else {
-					AddRules = append(AddRules, rules[i])
-					i += 1
-				}
-			} else if i >= len(rules) {
-				DeleteRules = append(DeleteRules, remoteRules[j])
-				j += 1
-			} else if j >= len(remoteRules) {
-				AddRules = append(AddRules, rules[i])
-				i += 1
-			}
-		}
-	}
-
-	for _, r := range DeleteRules {
-		// r.Description 实际存储的是ruleId
-		if err := self.delSecurityGroupRule(r.Description); err != nil {
-			log.Errorf("delSecurityGroupRule %v error: %s", r, err.Error())
-			return err
-		}
-	}
-
-	for _, r := range AddRules {
-		if err := self.addSecurityGroupRules(secgroupId, &r); err != nil {
-			log.Errorf("addSecurityGroupRule %v error: %s", r, err.Error())
-			return err
-		}
-	}
-
-	return nil
-}
-
 // https://support.huaweicloud.com/api-vpc/zh-cn_topic_0087467071.html
 func (self *SRegion) delSecurityGroupRule(secGrpRuleId string) error {
 	return DoDelete(self.ecsClient.SecurityGroupRules.Delete, secGrpRuleId, nil, nil)
@@ -803,7 +717,7 @@ func (self *SRegion) delSecurityGroupRule(secGrpRuleId string) error {
 
 // https://support.huaweicloud.com/api-vpc/zh-cn_topic_0087451723.html
 // icmp port对应关系：https://support.huaweicloud.com/api-vpc/zh-cn_topic_0024109590.html
-func (self *SRegion) addSecurityGroupRules(secGrpId string, rule *secrules.SecurityRule) error {
+func (self *SRegion) addSecurityGroupRules(secGrpId string, rule cloudprovider.SecurityRule) error {
 	direction := ""
 	if rule.Direction == secrules.SecurityRuleIngress {
 		direction = "ingress"

pkg/multicloud/huawei/securitygroup.go

@@ -27,12 +27,14 @@ https://support.huaweicloud.com/usermanual-vpc/zh-cn_topic_0073379079.html
 
 import (
 	"net"
-	"sort"
 
 	"yunion.io/x/jsonutils"
+	"yunion.io/x/pkg/errors"
 	"yunion.io/x/pkg/util/secrules"
 
 	api "yunion.io/x/onecloud/pkg/apis/compute"
+	"yunion.io/x/onecloud/pkg/cloudprovider"
+	"yunion.io/x/onecloud/pkg/multicloud"
 )
 
 type SecurityGroupRule struct {
@@ -60,6 +62,7 @@ type SecurityGroupRuleDetail struct {
 
 // https://support.huaweicloud.com/api-vpc/zh-cn_topic_0020090615.html
 type SSecurityGroup struct {
+	multicloud.SSecurityGroup
 	region *SRegion
 
 	ID                  string              `json:"id"`
@@ -85,46 +88,6 @@ func compatibleSecurityGroupRule(r SecurityGroupRule) bool {
 	return true
 }
 
-// 将安全组规则全部转换为等价的allow规则
-func SecurityRuleSetToAllowSet(srs secrules.SecurityRuleSet) secrules.SecurityRuleSet {
-	inRuleSet := secrules.SecurityRuleSet{}
-	outRuleSet := secrules.SecurityRuleSet{}
-
-	for _, rule := range srs {
-		if rule.Direction == secrules.SecurityRuleIngress {
-			inRuleSet = append(inRuleSet, rule)
-		}
-
-		if rule.Direction == secrules.SecurityRuleEgress {
-			outRuleSet = append(outRuleSet, rule)
-		}
-	}
-
-	sort.Sort(inRuleSet)
-	sort.Sort(outRuleSet)
-
-	inRuleSet = inRuleSet.AllowList()
-	// out方向空规则默认全部放行
-	if outRuleSet.Len() == 0 {
-		_, ipNet, _ := net.ParseCIDR("0.0.0.0/0")
-		outRuleSet = append(outRuleSet, secrules.SecurityRule{
-			Priority:  0,
-			Action:    secrules.SecurityRuleAllow,
-			IPNet:     ipNet,
-			Protocol:  secrules.PROTO_ANY,
-			Direction: secrules.SecurityRuleEgress,
-			PortStart: -1,
-			PortEnd:   -1,
-		})
-	}
-	outRuleSet = outRuleSet.AllowList()
-
-	ret := secrules.SecurityRuleSet{}
-	ret = append(ret, inRuleSet...)
-	ret = append(ret, outRuleSet...)
-	return ret
-}
-
 func (self *SSecurityGroup) GetId() string {
 	return self.ID
 }
@@ -173,14 +136,14 @@ func (self *SSecurityGroup) GetDescription() string {
 }
 
 // todo: 这里需要优化查询太多了
-func (self *SSecurityGroup) GetRules() ([]secrules.SecurityRule, error) {
-	rules := make([]secrules.SecurityRule, 0)
+func (self *SSecurityGroup) GetRules() ([]cloudprovider.SecurityRule, error) {
+	rules := make([]cloudprovider.SecurityRule, 0)
 	for _, r := range self.SecurityGroupRules {
 		if !compatibleSecurityGroupRule(r) {
 			continue
 		}
 
-		rule, err := self.GetSecurityRule(r.ID, false)
+		rule, err := self.GetSecurityRule(r.ID)
 		if err != nil {
 			return rules, err
 		}
@@ -191,30 +154,11 @@ func (self *SSecurityGroup) GetRules() ([]secrules.SecurityRule, error) {
 	return rules, nil
 }
 
-func (self *SSecurityGroup) GetRulesWithExtId() ([]secrules.SecurityRule, error) {
-	rules := make([]secrules.SecurityRule, 0)
-	for _, r := range self.SecurityGroupRules {
-		if !compatibleSecurityGroupRule(r) {
-			continue
-		}
-
-		rule, err := self.GetSecurityRule(r.ID, true)
-		if err != nil {
-			return rules, err
-		}
-
-		rules = append(rules, rule)
-	}
-
-	return rules, nil
-}
-
-// withRuleId.
-func (self *SSecurityGroup) GetSecurityRule(ruleId string, withRuleId bool) (secrules.SecurityRule, error) {
+func (self *SSecurityGroup) GetSecurityRule(ruleId string) (cloudprovider.SecurityRule, error) {
 	remoteRule := SecurityGroupRuleDetail{}
 	err := DoGet(self.region.ecsClient.SecurityGroupRules.Get, ruleId, nil, &remoteRule)
 	if err != nil {
-		return secrules.SecurityRule{}, err
+		return cloudprovider.SecurityRule{}, err
 	}
 
 	var direction secrules.TSecurityRuleDirection
@@ -247,27 +191,22 @@ func (self *SSecurityGroup) GetSecurityRule(ruleId string, withRuleId bool) (sec
 	}
 
 	if err != nil {
-		return secrules.SecurityRule{}, err
+		return cloudprovider.SecurityRule{}, err
 	}
 
-	// withRuleId.将ruleId附加到description字段。该hook有特殊目的，仅在同步安全组时使用。
-	desc := ""
-	if withRuleId {
-		desc = ruleId
-	} else {
-		desc = remoteRule.Description
-	}
-
-	rule := secrules.SecurityRule{
-		Priority:    1,
-		Action:      secrules.SecurityRuleAllow,
-		IPNet:       ipNet,
-		Protocol:    protocol,
-		Direction:   direction,
-		PortStart:   portStart,
-		PortEnd:     portEnd,
-		Ports:       nil,
-		Description: desc,
+	rule := cloudprovider.SecurityRule{
+		ExternalId: ruleId,
+		SecurityRule: secrules.SecurityRule{
+			Priority:    1,
+			Action:      secrules.SecurityRuleAllow,
+			IPNet:       ipNet,
+			Protocol:    protocol,
+			Direction:   direction,
+			PortStart:   portStart,
+			PortEnd:     portEnd,
+			Ports:       nil,
+			Description: remoteRule.Description,
+		},
 	}
 
 	err = rule.ValidateRule()
@@ -322,7 +261,18 @@ func (self *SSecurityGroup) Delete() error {
 	return self.region.DeleteSecurityGroup(self.ID)
 }
 
-func (self *SSecurityGroup) SyncRules(rules []secrules.SecurityRule) error {
-	rules = SecurityRuleSetToAllowSet(rules)
-	return self.region.syncSecgroupRules(self.ID, rules)
+func (self *SSecurityGroup) SyncRules(common, inAdds, outAdds, inDels, outDels []cloudprovider.SecurityRule) error {
+	for _, r := range append(inDels, outDels...) {
+		err := self.region.delSecurityGroupRule(r.ExternalId)
+		if err != nil {
+			return errors.Wrapf(err, "delSecurityGroupRule(%s %s)", r.ExternalId, r.String())
+		}
+	}
+	for _, r := range append(inAdds, outAdds...) {
+		err := self.region.addSecurityGroupRules(self.ID, r)
+		if err != nil {
+			return errors.Wrapf(err, "addSecurityGroupRule(%d %s)", r.Priority, r.String())
+		}
+	}
+	return nil
 }

pkg/multicloud/openstack/securitygroup.go

@@ -17,8 +17,6 @@ package openstack
 import (
 	"fmt"
 	"net"
-	"sort"
-	"strings"
 	"time"
 
 	"yunion.io/x/jsonutils"
@@ -28,6 +26,8 @@ import (
 	"yunion.io/x/pkg/utils"
 
 	"yunion.io/x/onecloud/pkg/cloudprovider"
+	"yunion.io/x/onecloud/pkg/httperrors"
+	"yunion.io/x/onecloud/pkg/multicloud"
 	"yunion.io/x/onecloud/pkg/util/httputils"
 )
 
@@ -55,6 +55,7 @@ type SSecurityGroupRule struct {
 }
 
 type SSecurityGroup struct {
+	multicloud.SSecurityGroup
 	region *SRegion
 
 	Description        string
@@ -69,20 +70,6 @@ type SSecurityGroup struct {
 	TenantID           string
 }
 
-type SecurigyGroupRuleSet []SSecurityGroupRule
-
-func (v SecurigyGroupRuleSet) Len() int {
-	return len(v)
-}
-
-func (v SecurigyGroupRuleSet) Swap(i, j int) {
-	v[i], v[j] = v[j], v[i]
-}
-
-func (v SecurigyGroupRuleSet) Less(i, j int) bool {
-	return strings.Compare(v[i].String(), v[j].String()) <= 0
-}
-
 func (region *SRegion) GetSecurityGroup(secgroupId string) (*SSecurityGroup, error) {
 	_, resp, err := region.Get("network", "/v2.0/security-groups/"+secgroupId, "", nil)
 	if err != nil {
@@ -158,26 +145,19 @@ func (secgroup *SSecurityGroup) GetName() string {
 	return secgroup.ID
 }
 
-func (secgroup *SSecurityGroupRule) String() string {
-	rules := secgroup.toRules()
-	result := []string{}
-	for _, rule := range rules {
-		result = append(result, rule.String())
-	}
-	return strings.Join(result, ";")
-}
-
-func (secgrouprule *SSecurityGroupRule) toRules() []secrules.SecurityRule {
-	rules := []secrules.SecurityRule{}
+func (secgrouprule *SSecurityGroupRule) toRules() ([]cloudprovider.SecurityRule, error) {
+	rules := []cloudprovider.SecurityRule{}
 	// 暂时忽略IPv6安全组规则,忽略远端也是安全组的规则
 	if secgrouprule.Ethertype != "IPv4" || len(secgrouprule.RemoteGroupID) > 0 {
-		return rules
+		return rules, fmt.Errorf("ethertype: %s remoteGroupId: %s", secgrouprule.Ethertype, secgrouprule.RemoteGroupID)
 	}
-	rule := secrules.SecurityRule{
-		Direction:   secrules.DIR_IN,
-		Action:      secrules.SecurityRuleAllow,
-		Description: secgrouprule.Description,
-		Priority:    1,
+	rule := cloudprovider.SecurityRule{
+		ExternalId: secgrouprule.ID,
+		SecurityRule: secrules.SecurityRule{
+			Direction:   secrules.DIR_IN,
+			Action:      secrules.SecurityRuleAllow,
+			Description: secgrouprule.Description,
+		},
 	}
 	if utils.IsInStringArray(secgrouprule.Protocol, []string{"any", "-1", ""}) {
 		rule.Protocol = secrules.PROTO_ANY
@@ -188,7 +168,7 @@ func (secgrouprule *SSecurityGroupRule) toRules() []secrules.SecurityRule {
 	} else if utils.IsInStringArray(secgrouprule.Protocol, []string{"1", "icmp"}) {
 		rule.Protocol = secrules.PROTO_ICMP
 	} else {
-		return rules
+		return rules, errors.Wrap(httperrors.ErrUnsupportedProtocol, secgrouprule.Protocol)
 	}
 	if secgrouprule.Direction == "egress" {
 		rule.Direction = secrules.DIR_OUT
@@ -198,7 +178,7 @@ func (secgrouprule *SSecurityGroupRule) toRules() []secrules.SecurityRule {
 	}
 	_, ipnet, err := net.ParseCIDR(secgrouprule.RemoteIpPrefix)
 	if err != nil {
-		return rules
+		return rules, errors.Wrapf(err, "net.ParseCIDR(%s)", secgrouprule.RemoteIpPrefix)
 	}
 	rule.IPNet = ipnet
 	if secgrouprule.PortRangeMax > 0 && secgrouprule.PortRangeMin > 0 {
@@ -209,28 +189,23 @@ func (secgrouprule *SSecurityGroupRule) toRules() []secrules.SecurityRule {
 			rule.PortEnd = secgrouprule.PortRangeMax
 		}
 	}
-	if err := rule.ValidateRule(); err != nil {
-		return rules
+	err = rule.ValidateRule()
+	if err != nil && err != secrules.ErrInvalidPriority {
+		return rules, errors.Wrap(err, "rule.ValidateRule")
 	}
-	return []secrules.SecurityRule{rule}
+	return []cloudprovider.SecurityRule{rule}, nil
 }
 
-func (secgroup *SSecurityGroup) GetRules() ([]secrules.SecurityRule, error) {
-	rules := []secrules.SecurityRule{}
-	priority := 100
+func (secgroup *SSecurityGroup) GetRules() ([]cloudprovider.SecurityRule, error) {
+	rules := []cloudprovider.SecurityRule{}
 	for _, rule := range secgroup.SecurityGroupRules {
-		if priority < 2 {
-			priority = 2
-		}
-		subRules := rule.toRules()
-		for _, subRule := range subRules {
-			subRule.Priority = priority
-			rules = append(rules, subRule)
+		subRules, err := rule.toRules()
+		if err != nil {
+			log.Errorf("failed to convert rule %s for secgroup %s(%s) error: %v", rule.ID, secgroup.Name, secgroup.ID, err)
+			continue
 		}
+		rules = append(rules, subRules...)
 	}
-	defaultDenyRule := secrules.MustParseSecurityRule("out:deny any")
-	defaultDenyRule.Priority = 1
-	rules = append(rules, *defaultDenyRule)
 	return rules, nil
 }
 
@@ -250,145 +225,12 @@ func (secgroup *SSecurityGroup) Refresh() error {
 	return jsonutils.Update(secgroup, new)
 }
 
-func (region *SRegion) SyncSecurityGroup(secgroupId string, vpcId string, name string, desc string, rules []secrules.SecurityRule) (string, error) {
-	if len(secgroupId) > 0 {
-		_, err := region.GetSecurityGroup(secgroupId)
-		if err != nil {
-			if err != cloudprovider.ErrNotFound {
-				return "", err
-			}
-			secgroupId = ""
-		}
-	}
-	if len(secgroupId) == 0 {
-		secgroups, err := region.GetSecurityGroups("")
-		if err != nil {
-			// 若返回 cloudprovider.ErrNotFound, 表明不支持安全组或者未安装安全组相关组件
-			if err == cloudprovider.ErrNotFound {
-				return SECGROUP_NOT_SUPPORT, nil
-			}
-			log.Errorf("failed to get secgroups: %v", err)
-			return "", err
-		}
-
-		secgroupNames := []string{}
-		for _, secgroup := range secgroups {
-			secgroupNames = append(secgroupNames, strings.ToLower(secgroup.Name))
-		}
-
-		uniqName := strings.ToLower(name)
-		if utils.IsInStringArray(uniqName, secgroupNames) {
-			for i := 0; i < 20; i++ {
-				uniqName = fmt.Sprintf("%s-%d", strings.ToLower(name), i)
-				if !utils.IsInStringArray(uniqName, secgroupNames) {
-					break
-				}
-			}
-		}
-		log.Errorf("create secgroup %s", uniqName)
-		secgroup, err := region.CreateSecurityGroup(uniqName, desc)
-		if err != nil {
-			return "", err
-		}
-		secgroupId = secgroup.ID
-	}
-	return region.syncSecgroupRules(secgroupId, rules)
-}
-
-func (region *SRegion) syncSecgroupRules(secgroupId string, rules []secrules.SecurityRule) (string, error) {
-	secgroup, err := region.GetSecurityGroup(secgroupId)
-	if err != nil {
-		return "", err
-	}
-
-	// OpenStack仅支持allow规则添加，需要将规则全转换为allow rules
-	inRules, outRules := secrules.SecurityRuleSet{}, secrules.SecurityRuleSet{}
-	for i := 0; i < len(rules); i++ {
-		if rules[i].Direction == secrules.DIR_IN {
-			inRules = append(inRules, rules[i])
-		} else {
-			outRules = append(outRules, rules[i])
-		}
-	}
-
-	// OpenStack Out方向默认是禁止所有流量，需要给本地安全组规则加一条优先级最低的allow any规则，和OpenStack规则语义保持一致
-	defaultAllow := secrules.MustParseSecurityRule("out:allow any")
-	defaultAllow.Priority = 0
-	outRules = append(outRules, *defaultAllow)
-
-	rules = inRules.AllowList()
-	rules = append(rules, outRules.AllowList()...)
-
-	sort.Sort(secrules.SecurityRuleSet(rules))
-	sort.Sort(SecurigyGroupRuleSet(secgroup.SecurityGroupRules))
-
-	delSecgroupRuleIds := []string{}
-	addSecgroupRules := []secrules.SecurityRule{}
-	addSecgroupRuleStrings := []string{}
-
-	i, j := 0, 0
-	for i < len(rules) || j < len(secgroup.SecurityGroupRules) {
-		if i < len(rules) && j < len(secgroup.SecurityGroupRules) {
-			secruleStr := secgroup.SecurityGroupRules[j].String()
-			ruleStr := rules[i].String()
-			cmp := strings.Compare(secruleStr, ruleStr)
-			if cmp == 0 {
-				i++
-				j++
-			} else if cmp > 0 {
-				delSecgroupRuleIds = append(delSecgroupRuleIds, secgroup.SecurityGroupRules[j].ID)
-				j++
-			} else {
-				if !utils.IsInStringArray(ruleStr, addSecgroupRuleStrings) {
-					addSecgroupRules = append(addSecgroupRules, rules[i])
-					addSecgroupRuleStrings = append(addSecgroupRuleStrings, ruleStr)
-				}
-				i++
-			}
-		} else if i >= len(rules) {
-			delSecgroupRuleIds = append(delSecgroupRuleIds, secgroup.SecurityGroupRules[j].ID)
-			j++
-		} else if j >= len(secgroup.SecurityGroupRules) {
-			ruleStr := rules[i].String()
-			if !utils.IsInStringArray(ruleStr, addSecgroupRuleStrings) {
-				addSecgroupRules = append(addSecgroupRules, rules[i])
-				addSecgroupRuleStrings = append(addSecgroupRuleStrings, ruleStr)
-			}
-			i++
-		}
-	}
-
-	for _, ruleId := range delSecgroupRuleIds {
-		if err := region.delSecurityGroupRule(ruleId); err != nil {
-			log.Errorf("delSecurityGroupRule error %v", err)
-			return "", err
-		}
-	}
-	for i := 0; i < len(addSecgroupRules); i++ {
-		if err := region.addSecurityGroupRules(secgroupId, &addSecgroupRules[i]); err != nil {
-			if jsonError, ok := err.(*httputils.JSONClientError); ok {
-				if jsonError.Class == "SecurityGroupRuleExists" {
-					continue
-				}
-			}
-			log.Errorf("addSecurityGroupRule error %v", rules[i])
-			return "", err
-		}
-	}
-
-	return secgroupId, nil
-}
-
 func (region *SRegion) delSecurityGroupRule(ruleId string) error {
 	_, err := region.Delete("network", "/v2.0/security-group-rules/"+ruleId, "")
 	return err
 }
 
-func (region *SRegion) addSecurityGroupRules(secgroupId string, rule *secrules.SecurityRule) error {
-	if rule.Action == secrules.SecurityRuleDeny {
-		// openstack 不支持deny规则
-		return nil
-	}
+func (region *SRegion) addSecurityGroupRules(secgroupId string, rule cloudprovider.SecurityRule) error {
 	direction := "ingress"
 	if rule.Direction == secrules.SecurityRuleEgress {
 		direction = "egress"
@@ -457,7 +299,23 @@ func (secgroup *SSecurityGroup) GetProjectId() string {
 	return secgroup.TenantID
 }
 
-func (secgroup *SSecurityGroup) SyncRules(rules []secrules.SecurityRule) error {
-	_, err := secgroup.region.syncSecgroupRules(secgroup.ID, rules)
-	return err
+func (secgroup *SSecurityGroup) SyncRules(common, inAdds, outAdds, inDels, outDels []cloudprovider.SecurityRule) error {
+	for _, r := range append(inDels, outDels...) {
+		err := secgroup.region.delSecurityGroupRule(r.ExternalId)
+		if err != nil {
+			return errors.Wrapf(err, "delSecurityGroupRule(%s)", r.ExternalId)
+		}
+	}
+	for _, r := range append(inAdds, outAdds...) {
+		err := secgroup.region.addSecurityGroupRules(secgroup.ID, r)
+		if err != nil {
+			if jsonError, ok := err.(*httputils.JSONClientError); ok {
+				if jsonError.Class == "SecurityGroupRuleExists" {
+					continue
+				}
+			}
+			return errors.Wrapf(err, "addSecgroupRules(%s)", r.String())
+		}
+	}
+	return nil
 }

pkg/multicloud/openstack/shell/securitygroup.go

@@ -33,7 +33,8 @@ func init() {
 	})
 
 	type SecurityGroupShowOptions struct {
-		ID string `help:"ID of security group"`
+		ID        string `help:"ID of security group"`
+		ShowRules bool   `help:"Show rules"`
 	}
 	shellutils.R(&SecurityGroupShowOptions{}, "security-group-show", "Show security group", func(cli *openstack.SRegion, args *SecurityGroupShowOptions) error {
 		secgroup, err := cli.GetSecurityGroup(args.ID)
@@ -41,6 +42,15 @@ func init() {
 			return err
 		}
 		printObject(secgroup)
+		if args.ShowRules {
+			rules, err := secgroup.GetRules()
+			if err != nil {
+				return err
+			}
+			for _, r := range rules {
+				printObject(r)
+			}
+		}
 		return nil
 	})
 

pkg/multicloud/qcloud/securitygroup.go

@@ -17,17 +17,18 @@ package qcloud
 import (
 	"fmt"
 	"net"
-	"sort"
 	"strconv"
 	"strings"
 	"time"
 
 	"yunion.io/x/jsonutils"
 	"yunion.io/x/log"
+	"yunion.io/x/pkg/errors"
 	"yunion.io/x/pkg/util/secrules"
 	"yunion.io/x/pkg/utils"
 
 	"yunion.io/x/onecloud/pkg/cloudprovider"
+	"yunion.io/x/onecloud/pkg/multicloud"
 )
 
 type SecurityGroupPolicy struct {
@@ -61,6 +62,7 @@ type SecurityGroupPolicySet struct {
 }
 
 type SSecurityGroup struct {
+	multicloud.SSecurityGroup
 	region                 *SRegion
 	SecurityGroupId        string    //		安全组实例ID，例如：sg-ohuuioma。
 	SecurityGroupName      string    //		安全组名称，可任意命名，但不得超过60个字符。
@@ -71,25 +73,6 @@ type SSecurityGroup struct {
 	SecurityGroupPolicySet SecurityGroupPolicySet
 }
 
-type SecurityGroupRuleSet []SecurityGroupPolicy
-
-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].PolicyIndex < v[j].PolicyIndex {
-		return true
-	} else if v[i].PolicyIndex == v[j].PolicyIndex {
-		return strings.Compare(v[i].String(), v[j].String()) <= 0
-	}
-	return false
-}
-
 func (self *SRegion) GetSecurityGroups(vpcId string, name string, offset int, limit int) ([]SSecurityGroup, int, error) {
 	if limit > 50 || limit <= 0 {
 		limit = 50
@@ -168,15 +151,19 @@ func parseCIDR(cidr string) (*net.IPNet, error) {
 	return &net.IPNet{IP: ip, Mask: net.CIDRMask(32, 32)}, nil
 }
 
-func (self *SecurityGroupPolicy) toRules() []secrules.SecurityRule {
-	result := []secrules.SecurityRule{}
-	rule := secrules.SecurityRule{
-		Action:    secrules.SecurityRuleAllow,
-		Protocol:  secrules.PROTO_ANY,
-		Direction: secrules.TSecurityRuleDirection(self.direction),
-		Ports:     []int{},
-		PortStart: -1,
-		PortEnd:   -1,
+func (self *SecurityGroupPolicy) toRules() []cloudprovider.SecurityRule {
+	result := []cloudprovider.SecurityRule{}
+	rule := cloudprovider.SecurityRule{
+		ExternalId: fmt.Sprintf("%d", self.PolicyIndex),
+		SecurityRule: secrules.SecurityRule{
+			Action:    secrules.SecurityRuleAllow,
+			Protocol:  secrules.PROTO_ANY,
+			Direction: secrules.TSecurityRuleDirection(self.direction),
+			Priority:  self.PolicyIndex,
+			Ports:     []int{},
+			PortStart: -1,
+			PortEnd:   -1,
+		},
 	}
 	if len(self.SecurityGroupId) != 0 {
 		//安全组关联安全组的规则忽略
@@ -251,8 +238,8 @@ func (self *SecurityGroupPolicy) toRules() []secrules.SecurityRule {
 	return result
 }
 
-func (self *SecurityGroupPolicy) getAddressRules(rule secrules.SecurityRule, addressId string) ([]secrules.SecurityRule, error) {
-	result := []secrules.SecurityRule{}
+func (self *SecurityGroupPolicy) getAddressRules(rule cloudprovider.SecurityRule, addressId string) ([]cloudprovider.SecurityRule, error) {
+	result := []cloudprovider.SecurityRule{}
 	address, total, err := self.region.AddressList(addressId, "", 0, 1)
 	if err != nil {
 		log.Errorf("Get AddressList %s failed %v", self.AddressTemplate.AddressId, err)
@@ -272,7 +259,7 @@ func (self *SecurityGroupPolicy) getAddressRules(rule secrules.SecurityRule, add
 	return result, nil
 }
 
-func (self *SSecurityGroup) GetRules() ([]secrules.SecurityRule, error) {
+func (self *SSecurityGroup) GetRules() ([]cloudprovider.SecurityRule, error) {
 	secgroup, err := self.region.GetSecurityGroupDetails(self.SecurityGroupId)
 	if err != nil {
 		return nil, err
@@ -289,26 +276,11 @@ func (self *SSecurityGroup) GetRules() ([]secrules.SecurityRule, error) {
 	for i := 0; i < len(originRules); i++ {
 		originRules[i].region = self.region
 	}
-	sort.Sort(SecurityGroupRuleSet(originRules))
-	rules := []secrules.SecurityRule{}
-	priority := 100
+	rules := []cloudprovider.SecurityRule{}
 	for _, rule := range originRules {
 		subRules := rule.toRules()
-		for i := 0; i < len(subRules); i++ {
-			subRules[i].Priority = priority
-		}
-		if len(subRules) > 0 {
-			priority--
-		}
 		rules = append(rules, subRules...)
 	}
-	// 腾讯云若出方向规则默认是拒绝所有流量
-	defaultDenyRule, err := secrules.ParseSecurityRule("out:deny any")
-	if err != nil {
-		return nil, err
-	}
-	defaultDenyRule.Priority = 1
-	rules = append(rules, *defaultDenyRule)
 	return rules, nil
 }
 
@@ -321,36 +293,61 @@ func (self *SSecurityGroup) IsEmulated() bool {
 }
 
 func (self *SSecurityGroup) Refresh() error {
-	if new, err := self.region.GetSecurityGroupDetails(self.SecurityGroupId); err != nil {
+	group, err := self.region.GetSecurityGroupDetails(self.SecurityGroupId)
+	if err != nil {
 		return err
-	} else {
-		return jsonutils.Update(self, new)
 	}
+	return jsonutils.Update(self, group)
 }
 
-func (self *SSecurityGroup) SyncRules(rules []secrules.SecurityRule) error {
-	_, err := self.region.syncSecgroupRules(self.SecurityGroupId, rules)
-	return err
+func (self *SSecurityGroup) deleteRules(rules []cloudprovider.SecurityRule, direction string) error {
+	ids := []string{}
+	for _, r := range rules {
+		ids = append(ids, r.ExternalId)
+	}
+	if len(ids) > 0 {
+		err := self.region.DeleteRules(self.SecurityGroupId, direction, ids)
+		if err != nil {
+			return errors.Wrapf(err, "deleteRules(%s)", ids)
+		}
+	}
+	return nil
 }
 
-func (self *SRegion) SyncSecurityGroup(secgroupId string, vpcId string, name string, desc string, rules []secrules.SecurityRule) (string, error) {
-	if len(secgroupId) > 0 {
-		_, err := self.GetSecurityGroupDetails(secgroupId)
+func (self *SSecurityGroup) SyncRules(common, inAdds, outAdds, inDels, outDels []cloudprovider.SecurityRule) error {
+	rules := append(common, append(inAdds, outAdds...)...)
+	return self.region.syncSecgroupRules(self.SecurityGroupId, rules)
+}
+
+func (self *SRegion) syncSecgroupRules(secgroupid string, rules []cloudprovider.SecurityRule) error {
+	err := self.deleteAllRules(secgroupid)
+	if err != nil {
+		return errors.Wrap(err, "deleteAllRules")
+	}
+	egressIndex, ingressIndex := -1, -1
+	for _, rule := range rules {
+		policyIndex := 0
+		switch rule.Direction {
+		case secrules.DIR_IN:
+			ingressIndex++
+			policyIndex = ingressIndex
+		case secrules.DIR_OUT:
+			egressIndex++
+			policyIndex = egressIndex
+		default:
+			return fmt.Errorf("Unknown rule direction %v for secgroup %s", rule, secgroupid)
+		}
+
+		//为什么不一次创建完成?
+		//答: 因为如果只有入方向安全组规则，创建时会提示缺少出方向规则。
+		//为什么不分两次，一次创建入方向规则，一次创建出方向规则?
+		//答: 因为这样就不能设置优先级了，一次性创建的出或入方向的优先级必须一样。
+		err := self.AddRule(secgroupid, policyIndex, rule)
 		if err != nil {
-			if err != cloudprovider.ErrNotFound {
-				return "", err
-			}
-			secgroupId = ""
+			return errors.Wrap(err, "AddRule")
 		}
 	}
-	if len(secgroupId) == 0 {
-		secgroup, err := self.CreateSecurityGroup(name, desc)
-		if err != nil {
-			return "", err
-		}
-		secgroupId = secgroup.SecurityGroupId
-	}
-	return self.syncSecgroupRules(secgroupId, rules)
+	return nil
 }
 
 func (self *SRegion) deleteAllRules(secgroupid string) error {
@@ -359,7 +356,19 @@ func (self *SRegion) deleteAllRules(secgroupid string) error {
 	return err
 }
 
-func (self *SRegion) addRule(secgroupId string, policyIndex int, rule *secrules.SecurityRule) error {
+func (self *SRegion) DeleteRules(secgroupId, direction string, ids []string) error {
+	if len(ids) == 0 {
+		return nil
+	}
+	params := map[string]string{"SecurityGroupId": secgroupId}
+	for idx, id := range ids {
+		params[fmt.Sprintf("SecurityGroupPolicySet.%s.%d.PolicyIndex", direction, idx)] = id
+	}
+	_, err := self.vpcRequest("DeleteSecurityGroupPolicies", params)
+	return err
+}
+
+func (self *SRegion) AddRule(secgroupId string, policyIndex int, rule cloudprovider.SecurityRule) error {
 	params := map[string]string{}
 	params["SecurityGroupId"] = secgroupId
 	direction := "Egress"
@@ -404,47 +413,6 @@ func (self *SRegion) addRule(secgroupId string, policyIndex int, rule *secrules.
 	return nil
 }
 
-func (self *SRegion) syncSecgroupRules(secgroupid string, rules []secrules.SecurityRule) (string, error) {
-	if err := self.deleteAllRules(secgroupid); err != nil {
-		return "", err
-	}
-	egressIndex, ingressIndex := -1, -1
-	for _, rule := range rules {
-		policyIndex := 0
-		switch rule.Direction {
-		case secrules.DIR_IN:
-			ingressIndex++
-			policyIndex = ingressIndex
-		case secrules.DIR_OUT:
-			egressIndex++
-			policyIndex = egressIndex
-		default:
-			return "", fmt.Errorf("Unknown rule direction %v for secgroup %s", rule, secgroupid)
-		}
-
-		//为什么不一次创建完成?
-		//答: 因为如果只有入方向安全组规则，创建时会提示缺少出方向规则。
-		//为什么不分两次，一次创建入方向规则，一次创建出方向规则?
-		//答: 因为这样就不能设置优先级了，一次性创建的出或入方向的优先级必须一样。
-		err := self.addRule(secgroupid, policyIndex, &rule)
-		if err != nil {
-			return "", err
-		}
-	}
-
-	// 需要在云上加上优先级最低的 allow any 规则, 和本地语义保持一致
-	egressIndex++
-	rule, err := secrules.ParseSecurityRule("out:allow any")
-	if err != nil {
-		return "", err
-	}
-	err = self.addRule(secgroupid, egressIndex, rule)
-	if err != nil {
-		return "", err
-	}
-	return secgroupid, nil
-}
-
 func (self *SRegion) GetSecurityGroupDetails(secGroupId string) (*SSecurityGroup, error) {
 	params := make(map[string]string)
 	params["Region"] = self.Region
@@ -558,10 +526,13 @@ func (self *SRegion) CreateSecurityGroup(name, description string) (*SSecurityGr
 		params["GroupDescription"] = "Customize Create"
 	}
 	secgroup := SSecurityGroup{region: self}
-	if body, err := self.vpcRequest("CreateSecurityGroup", params); err != nil {
-		return nil, err
-	} else if err := body.Unmarshal(&secgroup, "SecurityGroup"); err != nil {
-		return nil, err
+	body, err := self.vpcRequest("CreateSecurityGroup", params)
+	if err != nil {
+		return nil, errors.Wrap(err, "CreateSecurityGroup")
+	}
+	err = body.Unmarshal(&secgroup, "SecurityGroup")
+	if err != nil {
+		return nil, errors.Wrap(err, "body.Unmarshal")
 	}
 	return &secgroup, nil
 }

pkg/multicloud/qcloud/shell/securitygroup.go

@@ -15,6 +15,10 @@
 package shell
 
 import (
+	"yunion.io/x/pkg/errors"
+	"yunion.io/x/pkg/util/secrules"
+
+	"yunion.io/x/onecloud/pkg/cloudprovider"
 	"yunion.io/x/onecloud/pkg/multicloud/qcloud"
 	"yunion.io/x/onecloud/pkg/util/shellutils"
 )
@@ -78,4 +82,29 @@ func init() {
 		printList(address, total, args.Offset, args.Limit, []string{})
 		return nil
 	})
+
+	type RuleDeleteOptions struct {
+		SECGROUP_ID string
+		DIRECTION   string `choices:"Egress|Ingress"`
+		IDS         []string
+	}
+
+	shellutils.R(&RuleDeleteOptions{}, "security-group-rule-delete", "Delete rules", func(cli *qcloud.SRegion, args *RuleDeleteOptions) error {
+		return cli.DeleteRules(args.SECGROUP_ID, args.DIRECTION, args.IDS)
+	})
+
+	type RuleCreateOptions struct {
+		SECGROUP_ID string
+		INDEX       int
+		RULE        string
+	}
+
+	shellutils.R(&RuleCreateOptions{}, "security-group-rule-create", "Create rule", func(cli *qcloud.SRegion, args *RuleCreateOptions) error {
+		rule, err := secrules.ParseSecurityRule(args.RULE)
+		if err != nil {
+			return errors.Wrap(err, "secrules.ParseRuleString")
+		}
+		return cli.AddRule(args.SECGROUP_ID, args.INDEX, cloudprovider.SecurityRule{SecurityRule: *rule})
+	})
+
 }

pkg/multicloud/secgroup_base.go

@@ -0,0 +1,19 @@
+// 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 multicloud
+
+type SSecurityGroup struct {
+	SVirtualResourceBase
+}

pkg/multicloud/ucloud/region.go

@@ -16,8 +16,6 @@ package ucloud
 
 import (
 	"fmt"
-	"net"
-	"sort"
 	"strings"
 
 	"yunion.io/x/jsonutils"
@@ -227,41 +225,6 @@ func (self *SRegion) CreateISecurityGroup(conf *cloudprovider.SecurityGroupCreat
 // https://docs.ucloud.cn/api/unet-api/describe_firewall
 // 绑定防火墙组的资源类型，默认为全部资源类型。枚举值为："unatgw"，NAT网关； "uhost"，云主机； "upm"，物理云主机； "hadoophost"，hadoop节点； "fortresshost"，堡垒机； "udhost"，私有专区主机；"udockhost"，容器；"dbaudit"，数据库审计.
 // todo: 是否需要过滤出仅绑定云主机的安全组？
-func (self *SRegion) SyncSecurityGroup(secgroupId string, vpcId string, name string, desc string, rules []secrules.SecurityRule) (string, error) {
-	if len(secgroupId) > 0 {
-		_, err := self.GetSecurityGroupById(secgroupId)
-		if err == cloudprovider.ErrNotFound {
-			secgroupId = ""
-		} else if err != nil {
-			return "", err
-		}
-	}
-
-	if len(secgroupId) == 0 {
-		extID, err := self.CreateDefaultSecurityGroup(name, desc)
-		if err != nil {
-			return "", err
-		}
-		secgroupId = extID
-	}
-
-	// 如果是空规则，onecloud。默认拒绝所有流量
-	if len(rules) == 0 {
-		_, IpNet, _ := net.ParseCIDR("0.0.0.0/0")
-		rules = []secrules.SecurityRule{{
-			Priority:    0,
-			Action:      secrules.SecurityRuleDeny,
-			IPNet:       IpNet,
-			Protocol:    secrules.PROTO_ANY,
-			Direction:   secrules.SecurityRuleIngress,
-			PortStart:   0,
-			PortEnd:     0,
-			Ports:       nil,
-			Description: "",
-		}}
-	}
-	return secgroupId, self.syncSecgroupRules(secgroupId, rules)
-}
 
 func (self *SRegion) CreateIVpc(name string, desc string, cidr string) (cloudprovider.ICloudVpc, error) {
 	params := NewUcloudParams()
@@ -561,49 +524,9 @@ func (self *SRegion) GetInstanceByID(instanceId string) (SInstance, error) {
 	}
 }
 
-func inList(s int, lst []int) bool {
-	for _, e := range lst {
-		if s == e {
-			return true
-		}
-	}
-
-	return false
-}
-
-func toUcloudSecurityRules(rules []secrules.SecurityRule) ([]string, error) {
-	ps := make([]int, 0)
-	for _, rule := range rules {
-		if rule.Direction == secrules.SecurityRuleIngress && !inList(rule.Priority, ps) {
-			ps = append(ps, rule.Priority)
-		}
-	}
-
-	if len(ps) > 3 {
-		return nil, fmt.Errorf("unable map local security group rule priority  %v to LOW/MEDIUM/LOW", ps)
-	}
-
-	sort.Ints(ps)
-	pmap := map[int]string{}
-	for i, p := range ps {
-		pmap[p] = []string{"LOW", "MEDIUM", "HIGH"}[i]
-	}
-
-	ucloudRules := make([]string, 0)
-	for _, rule := range rules {
-		if rule.Direction == secrules.SecurityRuleIngress {
-			// GRE协议被忽略了
-			ucloudRules = append(ucloudRules, toUcloudSecRule(rule, pmap)...)
-		}
-	}
-
-	return ucloudRules, nil
-}
-
 // GRE协议被忽略了
-func toUcloudSecRule(rule secrules.SecurityRule, pmap map[int]string) []string {
+func toUcloudSecRule(rule cloudprovider.SecurityRule) []string {
 	net := rule.IPNet.String()
-	priority := pmap[rule.Priority]
 	action := "DROP"
 	if rule.Action == secrules.SecurityRuleAllow {
 		action = "ACCEPT"
@@ -612,18 +535,18 @@ func toUcloudSecRule(rule secrules.SecurityRule, pmap map[int]string) []string {
 	rules := make([]string, 0)
 	if len(rule.Ports) > 0 {
 		for _, port := range rule.Ports {
-			_rules := generatorRule(rule.Protocol, priority, net, action, port, port)
+			_rules := generatorRule(rule.Protocol, net, action, port, port, rule.Priority)
 			rules = append(rules, _rules...)
 		}
 	} else {
-		_rules := generatorRule(rule.Protocol, priority, net, action, rule.PortStart, rule.PortEnd)
+		_rules := generatorRule(rule.Protocol, net, action, rule.PortStart, rule.PortEnd, rule.Priority)
 		rules = append(rules, _rules...)
 	}
 
 	return rules
 }
 
-func generatorRule(protocol, priority, net, action string, startPort, endPort int) []string {
+func generatorRule(protocol, net, action string, startPort, endPort, priority int) []string {
 	rules := make([]string, 0)
 
 	var ports string
@@ -634,8 +557,17 @@ func generatorRule(protocol, priority, net, action string, startPort, endPort in
 	} else {
 		ports = fmt.Sprintf("%d-%d", startPort, endPort)
 	}
+	prio := "LOW"
+	switch priority {
+	case 1:
+		prio = "LOW"
+	case 2:
+		prio = "MEDIUM"
+	case 3:
+		prio = "HIGH"
+	}
 
-	template := fmt.Sprintf("%s|%s|%s|%s|%s|", "%s", "%s", net, action, priority)
+	template := fmt.Sprintf("%s|%s|%s|%s|%s|", "%s", "%s", net, action, prio)
 	switch protocol {
 	case secrules.PROTO_ANY:
 		rules = append(rules, fmt.Sprintf(template, "TCP", ports))
@@ -653,10 +585,10 @@ func generatorRule(protocol, priority, net, action string, startPort, endPort in
 }
 
 // https://docs.ucloud.cn/api/unet-api/update_firewall
-func (self *SRegion) syncSecgroupRules(secgroupId string, rules []secrules.SecurityRule) error {
-	_rules, err := toUcloudSecurityRules(rules)
-	if err != nil {
-		return err
+func (self *SRegion) syncSecgroupRules(secgroupId string, rules []cloudprovider.SecurityRule) error {
+	_rules := []string{}
+	for _, r := range rules {
+		_rules = append(_rules, toUcloudSecRule(r)...)
 	}
 
 	params := NewUcloudParams()

pkg/multicloud/ucloud/securitygroup.go

@@ -18,20 +18,22 @@ import (
 	"fmt"
 	"math/rand"
 	"net"
-	"strconv"
 	"strings"
 	"time"
 
 	"yunion.io/x/jsonutils"
 	"yunion.io/x/log"
+	"yunion.io/x/pkg/errors"
 	"yunion.io/x/pkg/util/secrules"
 
 	api "yunion.io/x/onecloud/pkg/apis/compute"
 	"yunion.io/x/onecloud/pkg/cloudprovider"
+	"yunion.io/x/onecloud/pkg/multicloud"
 )
 
 // https://docs.ucloud.cn/api/unet-api/describe_firewall
 type SSecurityGroup struct {
+	multicloud.SSecurityGroup
 	region *SRegion
 	vpc    *SVPC // 安全组在UCLOUD实际上与VPC是没有直接关联的。这里的vpc字段只是为了统一，仅仅是标记是哪个VPC在操作该安全组。
 
@@ -99,15 +101,15 @@ func (self *SSecurityGroup) GetDescription() string {
 	return self.Remark
 }
 
-func (self *SSecurityGroup) UcloudSecRuleToOnecloud(rule Rule) secrules.SecurityRule {
-	secrule := secrules.SecurityRule{}
+func (self *SSecurityGroup) UcloudSecRuleToOnecloud(rule Rule) (cloudprovider.SecurityRule, error) {
+	secrule := cloudprovider.SecurityRule{}
 	switch rule.Priority {
 	case "HIGH":
-		secrule.Priority = 90
+		secrule.Priority = 3
 	case "MEDIUM":
-		secrule.Priority = 60
+		secrule.Priority = 2
 	case "LOW":
-		secrule.Priority = 30
+		secrule.Priority = 1
 	default:
 		secrule.Priority = 1
 	}
@@ -123,46 +125,30 @@ func (self *SSecurityGroup) UcloudSecRuleToOnecloud(rule Rule) secrules.Security
 
 	_, ipNet, err := net.ParseCIDR(rule.SrcIP)
 	if err != nil {
-		log.Errorln(err)
+		return secrule, errors.Wrapf(err, "net.ParseCIDR(%s)", rule.SrcIP)
 	}
 
 	secrule.IPNet = ipNet
 	secrule.Protocol = strings.ToLower(rule.ProtocolType)
 	secrule.Direction = secrules.SecurityRuleIngress
-	if rule.DstPort == "" {
-		secrule.PortStart = -1
-		secrule.PortEnd = -1
-	} else if strings.Contains(rule.DstPort, "-") {
-		segs := strings.Split(rule.DstPort, "-")
-		s, err := strconv.Atoi(segs[0])
-		if err != nil {
-			log.Errorln(err)
-		}
-		e, err := strconv.Atoi(segs[1])
-		if err != nil {
-			log.Errorln(err)
-		}
-		secrule.PortStart = s
-		secrule.PortEnd = e
-	} else {
-		port, err := strconv.Atoi(rule.DstPort)
-		if err != nil {
-			log.Errorln(err)
-		}
-
-		secrule.PortStart = port
-		secrule.PortEnd = port
+	err = secrule.ParsePorts(rule.DstPort)
+	if err != nil {
+		return secrule, errors.Wrapf(err, "ParsePorts(%s)", rule.DstPort)
 	}
 
-	return secrule
+	return secrule, nil
 }
 
 // https://docs.ucloud.cn/network/firewall/firewall
 // 只有入方向规则
-func (self *SSecurityGroup) GetRules() ([]secrules.SecurityRule, error) {
-	rules := make([]secrules.SecurityRule, 0)
+func (self *SSecurityGroup) GetRules() ([]cloudprovider.SecurityRule, error) {
+	rules := make([]cloudprovider.SecurityRule, 0)
 	for _, r := range self.Rule {
-		rule := self.UcloudSecRuleToOnecloud(r)
+		rule, err := self.UcloudSecRuleToOnecloud(r)
+		if err != nil {
+			log.Errorf("failed to convert rule for group %s(%s) error: %v", self.Name, self.GroupID, err)
+			continue
+		}
 		rules = append(rules, rule)
 	}
 
@@ -264,22 +250,11 @@ func (self *SRegion) GetSecurityGroups(secGroupId string, resourceId string, nam
 	return result, nil
 }
 
-func (self *SSecurityGroup) SyncRules(rules []secrules.SecurityRule) error {
-	// 如果是空规则，onecloud。默认拒绝所有流量
-	if len(rules) == 0 {
-		_, IpNet, _ := net.ParseCIDR("0.0.0.0/0")
-		rules = []secrules.SecurityRule{{
-			Priority:    0,
-			Action:      secrules.SecurityRuleDeny,
-			IPNet:       IpNet,
-			Protocol:    secrules.PROTO_ANY,
-			Direction:   secrules.SecurityRuleIngress,
-			PortStart:   0,
-			PortEnd:     0,
-			Ports:       nil,
-			Description: "",
-		}}
+func (self *SSecurityGroup) SyncRules(common, inAdds, outAdds, inDels, outDels []cloudprovider.SecurityRule) error {
+	if len(inAdds) == 0 && len(inDels) == 0 {
+		return nil
 	}
+	rules := append(common, inAdds...)
 	return self.region.syncSecgroupRules(self.FWID, rules)
 }
 

pkg/multicloud/zstack/region.go

@@ -22,7 +22,6 @@ import (
 	"yunion.io/x/jsonutils"
 	"yunion.io/x/log"
 	"yunion.io/x/pkg/errors"
-	"yunion.io/x/pkg/util/secrules"
 
 	api "yunion.io/x/onecloud/pkg/apis/compute"
 	"yunion.io/x/onecloud/pkg/cloudprovider"
@@ -381,27 +380,6 @@ func (region *SRegion) CreateISecurityGroup(conf *cloudprovider.SecurityGroupCre
 	return region.CreateSecurityGroup(conf.Name, conf.Desc)
 }
 
-func (region *SRegion) SyncSecurityGroup(secgroupId string, vpcId string, name string, desc string, rules []secrules.SecurityRule) (string, error) {
-	if len(secgroupId) > 0 {
-		_, err := region.GetSecurityGroup(secgroupId)
-		if err != nil {
-			if err == cloudprovider.ErrNotFound {
-				secgroupId = ""
-			} else {
-				return "", err
-			}
-		}
-	}
-	if len(secgroupId) == 0 {
-		secgroup, err := region.CreateSecurityGroup(name, desc)
-		if err != nil {
-			return "", err
-		}
-		secgroupId = secgroup.UUID
-	}
-	return secgroupId, region.syncSecgroupRules(secgroupId, rules)
-}
-
 func (region *SRegion) GetCapabilities() []string {
 	return region.client.GetCapabilities()
 }

pkg/multicloud/zstack/securitygroup.go

@@ -18,13 +18,15 @@ import (
 	"fmt"
 	"net"
 	"net/url"
-	"sort"
 	"strings"
 
 	"yunion.io/x/jsonutils"
+	"yunion.io/x/pkg/errors"
 	"yunion.io/x/pkg/util/secrules"
 
 	api "yunion.io/x/onecloud/pkg/apis/compute"
+	"yunion.io/x/onecloud/pkg/cloudprovider"
+	"yunion.io/x/onecloud/pkg/multicloud"
 )
 
 type SSecurityGroupRule struct {
@@ -41,29 +43,8 @@ type SSecurityGroupRule struct {
 	ZStackTime
 }
 
-type SSecurityGroupRuleSet []SSecurityGroupRule
-
-func (v SSecurityGroupRuleSet) Len() int {
-	return len(v)
-}
-
-func (v SSecurityGroupRuleSet) Swap(i, j int) {
-	v[i], v[j] = v[j], v[i]
-}
-
-func (v SSecurityGroupRuleSet) Less(i, j int) bool {
-	rule, err := v[i].toRule()
-	if err != nil {
-		return false
-	}
-	_rule, err := v[j].toRule()
-	if err != nil {
-		return false
-	}
-	return strings.Compare(rule.String(), _rule.String()) <= 0
-}
-
 type SSecurityGroup struct {
+	multicloud.SSecurityGroup
 	region *SRegion
 
 	ZStackBasic
@@ -123,18 +104,21 @@ func (self *SSecurityGroup) GetDescription() string {
 	return self.Description
 }
 
-func (rule *SSecurityGroupRule) toRule() (*secrules.SecurityRule, error) {
-	r := &secrules.SecurityRule{
-		Direction: secrules.DIR_IN,
-		Action:    secrules.SecurityRuleAllow,
-		Priority:  1,
-		Protocol:  secrules.PROTO_ANY,
-		PortStart: rule.StartPort,
-		PortEnd:   rule.EndPort,
+func (rule *SSecurityGroupRule) toRule() (cloudprovider.SecurityRule, error) {
+	r := cloudprovider.SecurityRule{
+		ExternalId: rule.UUID,
+		SecurityRule: secrules.SecurityRule{
+			Direction: secrules.DIR_IN,
+			Action:    secrules.SecurityRuleAllow,
+			Priority:  1,
+			Protocol:  secrules.PROTO_ANY,
+			PortStart: rule.StartPort,
+			PortEnd:   rule.EndPort,
+		},
 	}
 	_, ipNet, err := net.ParseCIDR(rule.AllowedCIDR)
 	if err != nil {
-		return nil, err
+		return r, err
 	}
 	r.IPNet = ipNet
 	if rule.Type == "Egress" {
@@ -146,29 +130,17 @@ func (rule *SSecurityGroupRule) toRule() (*secrules.SecurityRule, error) {
 	return r, nil
 }
 
-func (self *SSecurityGroup) GetRules() ([]secrules.SecurityRule, error) {
-	rules := []secrules.SecurityRule{}
-	priority := 100
-	outRuleCount := 0
+func (self *SSecurityGroup) GetRules() ([]cloudprovider.SecurityRule, error) {
+	rules := []cloudprovider.SecurityRule{}
 	for i := 0; i < len(self.Rules); i++ {
 		if self.Rules[i].IPVersion == 4 {
 			rule, err := self.Rules[i].toRule()
 			if err != nil {
 				return nil, err
 			}
-			if rule.Direction == secrules.DIR_OUT {
-				outRuleCount++
-			}
-			rule.Priority = priority
-			rules = append(rules, *rule)
-			priority--
+			rules = append(rules, rule)
 		}
 	}
-	if outRuleCount != 0 {
-		rule := secrules.MustParseSecurityRule("out:deny any")
-		rule.Priority = 1
-		rules = append(rules, *rule)
-	}
 	return rules, nil
 }
 
@@ -196,7 +168,7 @@ func (self *SSecurityGroup) GetProjectId() string {
 	return ""
 }
 
-func (region *SRegion) AddSecurityGroupRule(secgroupId string, rules []secrules.SecurityRule) error {
+func (region *SRegion) AddSecurityGroupRule(secgroupId string, rules []cloudprovider.SecurityRule) error {
 	ruleParam := []map[string]interface{}{}
 	for _, rule := range rules {
 		Type := "Ingress"
@@ -280,91 +252,16 @@ func (region *SRegion) CreateSecurityGroup(name, desc string) (*SSecurityGroup,
 	return secgroup, region.client.create("security-groups", jsonutils.Marshal(params), secgroup)
 }
 
-func (region *SRegion) syncSecgroupRules(secgroupId string, rules []secrules.SecurityRule) error {
-	secgroup, err := region.GetSecurityGroup(secgroupId)
+func (self *SSecurityGroup) SyncRules(common, inAdds, outAdds, inDels, outDels []cloudprovider.SecurityRule) error {
+	deleteIds := []string{}
+	for _, r := range append(inDels, outDels...) {
+		deleteIds = append(deleteIds, r.ExternalId)
+	}
+	err := self.region.DeleteSecurityGroupRules(deleteIds)
 	if err != nil {
-		return err
+		return errors.Wrapf(err, "DeleteSecurityGroupRules(%s)", deleteIds)
 	}
-
-	inRules, outRules := secrules.SecurityRuleSet{}, secrules.SecurityRuleSet{}
-	for i := 0; i < len(rules); i++ {
-		if rules[i].Direction == secrules.DIR_IN {
-			inRules = append(inRules, rules[i])
-		} else {
-			outRules = append(outRules, rules[i])
-		}
-	}
-
-	if len(outRules) > 0 {
-		// 避免出现 {"error":{"class":"SYS.1007","code":503,"details":"rule should not be duplicated. rule dump: {\"type\":\"Egress\",\"ipVersion\":4,\"startPort\":-1,\"endPort\":-1,\"protocol\":\"ALL\",\"allowedCidr\":\"0.0.0.0/0\"}"}}
-		find := false
-		for _, _rule := range outRules {
-			if _rule.String() == "out:allow any" {
-				find = true
-				break
-			}
-		}
-		if !find {
-			rule := secrules.MustParseSecurityRule("out:allow any")
-			outRules = append(outRules, *rule)
-		}
-	}
-
-	rules = inRules.AllowList()
-	rules = append(rules, outRules.AllowList()...)
-	for i := 0; i < len(rules); i++ {
-		rules[i].Priority = 1
-	}
-
-	sort.Sort(secrules.SecurityRuleSet(rules))
-	sort.Sort(SSecurityGroupRuleSet(secgroup.Rules))
-
-	delRuleIds := []string{}
-	addRules := []secrules.SecurityRule{}
-
-	i, j := 0, 0
-	for i < len(rules) || j < len(secgroup.Rules) {
-		if i < len(rules) && j < len(secgroup.Rules) {
-			_rule, err := secgroup.Rules[j].toRule()
-			if err != nil {
-				return err
-			}
-			_ruleStr := _rule.String()
-			ruleStr := rules[i].String()
-			cmp := strings.Compare(_ruleStr, ruleStr)
-			if cmp == 0 {
-				if len(secgroup.Rules[j].RemoteSecurityGroupUUID) > 0 {
-					delRuleIds = append(delRuleIds, secgroup.Rules[j].UUID)
-					addRules = append(addRules, rules[i])
-				}
-				i++
-				j++
-			} else if cmp > 0 {
-				delRuleIds = append(delRuleIds, secgroup.Rules[j].UUID)
-				j++
-			} else {
-				addRules = append(addRules, rules[i])
-				i++
-			}
-		} else if i >= len(rules) {
-			delRuleIds = append(delRuleIds, secgroup.Rules[j].UUID)
-			j++
-		} else if j >= len(secgroup.Rules) {
-			addRules = append(addRules, rules[i])
-			i++
-		}
-	}
-	if len(delRuleIds) > 0 {
-		err = region.DeleteSecurityGroupRules(delRuleIds)
-		if err != nil {
-			return err
-		}
-	}
-	return region.AddSecurityGroupRule(secgroupId, addRules)
-}
-
-func (self *SSecurityGroup) SyncRules(rules []secrules.SecurityRule) error {
-	return self.region.syncSecgroupRules(self.UUID, rules)
+	return self.region.AddSecurityGroupRule(self.UUID, append(inAdds, outAdds...))
 }
 
 func (self *SSecurityGroup) Delete() error {