Merge pull request #10018 from ioito/hotfix/qx-secgroup-fix

fix(region): optimized secgroup rule sync

pkg/cloudprovider/securitygroup.go

@@ -18,12 +18,64 @@ import (
 	"sort"
 	"strings"
 
+	"gopkg.in/fatih/set.v0"
+
 	"yunion.io/x/log"
 	"yunion.io/x/pkg/util/secrules"
+	"yunion.io/x/pkg/utils"
 )
 
-const DEFAULT_CLOUD_RULE_ID = "default_cloud_rule_id"
-const DEFAULT_LOCAL_RULE_ID = "default_local_rule_id"
+type SecDriver interface {
+	GetDefaultSecurityGroupInRule() SecurityRule
+	GetDefaultSecurityGroupOutRule() SecurityRule
+	GetSecurityGroupRuleMaxPriority() int
+	GetSecurityGroupRuleMinPriority() int
+	IsOnlySupportAllowRules() bool
+}
+
+func NewSecRuleInfo(driver SecDriver) SecRuleInfo {
+	return SecRuleInfo{
+		InDefaultRule:           driver.GetDefaultSecurityGroupInRule(),
+		OutDefaultRule:          driver.GetDefaultSecurityGroupOutRule(),
+		MinPriority:             driver.GetSecurityGroupRuleMinPriority(),
+		MaxPriority:             driver.GetSecurityGroupRuleMaxPriority(),
+		IsOnlySupportAllowRules: driver.IsOnlySupportAllowRules(),
+	}
+}
+
+const DEFAULT_DEST_RULE_ID = "default_dest_rule_id"
+const DEFAULT_SRC_RULE_ID = "default_src_rule_id"
+
+type SecRuleInfo struct {
+	InDefaultRule           SecurityRule
+	OutDefaultRule          SecurityRule
+	Rules                   SecurityRuleSet
+	MinPriority             int
+	MaxPriority             int
+	IsOnlySupportAllowRules bool
+}
+
+func (r SecRuleInfo) AddDefaultRule(d SecRuleInfo, inRules, outRules []SecurityRule, isSrc bool) ([]SecurityRule, []SecurityRule) {
+	min, max := r.MinPriority, r.MaxPriority
+	r.InDefaultRule.Priority = min + 1
+	r.OutDefaultRule.Priority = min + 1
+	if max >= min {
+		r.InDefaultRule.Priority = min - 1
+		r.OutDefaultRule.Priority = min - 1
+	}
+
+	if isSrc {
+		r.InDefaultRule.Id = DEFAULT_SRC_RULE_ID
+		r.OutDefaultRule.Id = DEFAULT_SRC_RULE_ID
+	} else {
+		r.InDefaultRule.ExternalId = DEFAULT_DEST_RULE_ID
+		r.OutDefaultRule.ExternalId = DEFAULT_DEST_RULE_ID
+	}
+
+	inRules = append(inRules, r.InDefaultRule)
+	outRules = append(outRules, r.OutDefaultRule)
+	return inRules, outRules
+}
 
 type SecurityGroupFilterOptions struct {
 	VpcId     string
@@ -41,45 +93,10 @@ type SecurityGroupCreateInput struct {
 
 type SecurityRule struct {
 	secrules.SecurityRule
-	Name             string
-	ExternalId       string
-	LocalRulePrority int
-}
-
-type LocalSecurityRule struct {
-	secrules.SecurityRule
+	Name       string
 	ExternalId string
-}
-
-func (r LocalSecurityRule) String() string {
-	return r.SecurityRule.String()
-}
-
-type LocalSecurityRuleSet []LocalSecurityRule
-
-func (srs LocalSecurityRuleSet) Len() int {
-	return len(srs)
-}
-
-func (srs LocalSecurityRuleSet) Swap(i, j int) {
-	srs[i], srs[j] = srs[j], srs[i]
-}
-
-func (srs LocalSecurityRuleSet) Less(i, j int) bool {
-	if srs[i].Priority > srs[j].Priority {
-		return true
-	} else if srs[i].Priority == srs[j].Priority {
-		return srs[i].String() < srs[j].String()
-	}
-	return false
-}
-
-func (srs LocalSecurityRuleSet) AllowList() secrules.SecurityRuleSet {
-	rules := secrules.SecurityRuleSet{}
-	for _, r := range srs {
-		rules = append(rules, r.SecurityRule)
-	}
-	return rules.AllowList()
+	Id         string
+	SrcPrority int
 }
 
 func (r SecurityRule) String() string {
@@ -88,6 +105,17 @@ func (r SecurityRule) String() string {
 
 type SecurityRuleSet []SecurityRule
 
+func (rules SecurityRuleSet) Split() (in, out SecurityRuleSet) {
+	for i := 0; i < len(rules); i++ {
+		if rules[i].Direction == secrules.DIR_IN {
+			in = append(in, rules[i])
+		} else {
+			out = append(out, rules[i])
+		}
+	}
+	return
+}
+
 func (srs SecurityRuleSet) Len() int {
 	return len(srs)
 }
@@ -108,129 +136,119 @@ func (srs SecurityRuleSet) AllowList() secrules.SecurityRuleSet {
 	return rules.AllowList()
 }
 
-func AddDefaultRule(rules []SecurityRule, defaultRule SecurityRule, localRuleStr string, min, max int, onlyAllowRules bool) []SecurityRule {
-	if defaultRule.String() == localRuleStr {
-		return rules
+func (srs SecurityRuleSet) Debug() {
+	for i := 0; i < len(srs); i++ {
+		log.Debugf("Name: %s id: %s external_id: %s priority: %d %s", srs[i].Name, srs[i].Id, srs[i].ExternalId, srs[i].Priority, srs[i].String())
 	}
-	defaultRule.ExternalId = DEFAULT_CLOUD_RULE_ID
-	if max > min {
-		defaultRule.Priority = min - 1
-	} else {
-		defaultRule.Priority = max + 1
-	}
-	return append(rules, defaultRule)
 }
 
-func SortSecurityRule(rules SecurityRuleSet, max, min int, onlyAllowRules bool) {
-	if onlyAllowRules {
-		return
-	}
-	if max < min {
+func SortSecurityRule(rules SecurityRuleSet, max, min int, isAsc, onlyAllowRules bool) {
+	if (max >= min || onlyAllowRules) && !isAsc {
 		sort.Sort(sort.Reverse(rules))
 		return
 	}
 	sort.Sort(rules)
+	return
 }
 
-func CompareRules(
-	minPriority, maxPriority int,
-	localRules LocalSecurityRuleSet, remoteRules []SecurityRule,
-	defaultInRule, defaultOutRule SecurityRule,
-	onlyAllowRules bool, debug bool, refreshLocalRule bool,
-) (common, inAdds, outAdds, inDels, outDels []SecurityRule) {
-	localInRules := LocalSecurityRuleSet{}
-	localOutRules := LocalSecurityRuleSet{}
-	for i := range localRules {
-		if localRules[i].Direction == secrules.DIR_IN {
-			localInRules = append(localInRules, localRules[i])
-		} else {
-			localOutRules = append(localOutRules, localRules[i])
-		}
+func isAllowListEqual(src, dest secrules.SecurityRuleSet) bool {
+	if len(src) != len(dest) {
+		return false
 	}
-	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])
-		}
+	s1, s2 := set.New(set.ThreadSafe), set.New(set.ThreadSafe)
+	for i := 0; i < len(src); i++ {
+		s1.Add(src[i].String())
+		s2.Add(dest[i].String())
 	}
-	var inCommon, outCommon = inRules, outRules
+	return s1.IsEqual(s2)
+}
 
-	defaultLocalInRule := LocalSecurityRule{ExternalId: DEFAULT_LOCAL_RULE_ID}
-	defaultLocalInRule.SecurityRule = *secrules.MustParseSecurityRule("in:deny any")
-	defaultLocalOutRule := LocalSecurityRule{ExternalId: DEFAULT_LOCAL_RULE_ID}
-	defaultLocalOutRule.SecurityRule = *secrules.MustParseSecurityRule("out:allow any")
+func CompareRules(src, dest SecRuleInfo, debug bool) (common, inAdds, outAdds, inDels, outDels SecurityRuleSet) {
+	srcInRules, srcOutRules := src.Rules.Split()
+	destInRules, destOutRules := dest.Rules.Split()
 
-	inRules = AddDefaultRule(inRules, defaultInRule, defaultLocalInRule.String(), minPriority, maxPriority, onlyAllowRules)
-	outRules = AddDefaultRule(outRules, defaultOutRule, defaultLocalOutRule.String(), minPriority, maxPriority, onlyAllowRules)
+	srcInRules, srcOutRules = src.AddDefaultRule(dest, srcInRules, srcOutRules, true)
+	destInRules, destOutRules = dest.AddDefaultRule(src, destInRules, destOutRules, false)
 
-	defaultInEquals, defaultOutEquals := true, true
-	if defaultLocalInRule.String() != defaultInRule.String() {
-		localInRules = append(localInRules, defaultLocalInRule)
-		defaultInEquals = false
-	}
-	if defaultLocalOutRule.String() != defaultOutRule.String() {
-		localOutRules = append(localOutRules, defaultLocalOutRule)
-		defaultOutEquals = false
+	if debug {
+		log.Debugf("src in rules: ")
+		srcInRules.Debug()
 	}
 
-	sort.Sort(localInRules)
-	sort.Sort(localOutRules)
+	// AllowList 需要优先级从高到低排序
+	SortSecurityRule(srcInRules, src.MaxPriority, src.MinPriority, false, src.IsOnlySupportAllowRules)
+	SortSecurityRule(srcOutRules, src.MaxPriority, src.MinPriority, false, src.IsOnlySupportAllowRules)
 
-	localInAllowList := localInRules.AllowList()
-	localOutAllowList := localOutRules.AllowList()
-	_localInRules := LocalSecurityRuleSet{}
-	for i := range localInAllowList {
-		rule := LocalSecurityRule{}
-		rule.SecurityRule = localInAllowList[i]
-		_localInRules = append(_localInRules, rule)
-	}
-	_localOutRules := LocalSecurityRuleSet{}
-	for i := range localOutAllowList {
-		rule := LocalSecurityRule{}
-		rule.SecurityRule = localOutAllowList[i]
-		_localOutRules = append(_localOutRules, rule)
-	}
-	if !refreshLocalRule && onlyAllowRules {
-		localOutRules, localInRules = _localOutRules, _localInRules
-	}
-	if !refreshLocalRule && len(_localInRules) < len(localInRules) {
-		localInRules = _localInRules
-	}
-	if !refreshLocalRule && len(_localOutRules) < len(localOutRules) {
-		localOutRules = _localOutRules
-	}
+	SortSecurityRule(destInRules, dest.MaxPriority, dest.MinPriority, false, dest.IsOnlySupportAllowRules)
+	SortSecurityRule(destOutRules, dest.MaxPriority, dest.MinPriority, false, dest.IsOnlySupportAllowRules)
 
-	SortSecurityRule(inRules, maxPriority, minPriority, onlyAllowRules)
-	SortSecurityRule(outRules, maxPriority, minPriority, onlyAllowRules)
+	srcInAllowList := srcInRules.AllowList()
+	srcOutAllowList := srcOutRules.AllowList()
+
+	destInAllowList := destInRules.AllowList()
+	destOutAllowList := destOutRules.AllowList()
+	inEquals, outEquals := isAllowListEqual(srcInAllowList, destInAllowList), isAllowListEqual(srcOutAllowList, destOutAllowList)
 
-	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 maxPriority < minPriority {
-		startPriority = maxPriority + 1
+	if debug {
+		log.Debugf("In: src: %s dest: %s result: %v", srcInAllowList.String(), destInAllowList.String(), inEquals)
+		log.Debugf("Out: src: %s dest: %s result: %v", srcOutAllowList.String(), destOutAllowList.String(), outEquals)
 	}
 
-	var addPriority = func(priority int, inc int, min, max int, onlyAllowRules bool) int {
+	var tryUseAllowList = func(defaultRule SecurityRule, allowList secrules.SecurityRuleSet, rules SecurityRuleSet, isOnlyAllowList bool) SecurityRuleSet {
+		if len(allowList) < len(rules) || isOnlyAllowList {
+			rules = SecurityRuleSet{}
+			for i := range allowList {
+				rule := SecurityRule{}
+				rule.SecurityRule = allowList[i]
+				rules = append(rules, rule)
+			}
+
+			if !utils.IsInStringArray(allowList.String(), []string{
+				"",
+				"in:allow any",
+				"out:allow any",
+				"in:deny any",
+				"out:deny any",
+			}) && strings.HasSuffix(defaultRule.SecurityRule.String(), "deny any") {
+				rules = append(rules, defaultRule)
+			}
+		}
+		return rules
+	}
+
+	srcInRules = tryUseAllowList(src.InDefaultRule, srcInAllowList, srcInRules, dest.IsOnlySupportAllowRules)
+	srcOutRules = tryUseAllowList(src.OutDefaultRule, srcOutAllowList, srcOutRules, dest.IsOnlySupportAllowRules)
+
+	if inEquals {
+		srcInRules, destInRules = []SecurityRule{}, []SecurityRule{}
+	}
+	if outEquals {
+		srcOutRules, destOutRules = []SecurityRule{}, []SecurityRule{}
+	}
+
+	if debug {
+		log.Debugf("src in rules: ")
+		srcInRules.Debug()
+	}
+
+	// 默认从优先级低到高比较
+	SortSecurityRule(srcInRules, src.MaxPriority, src.MinPriority, true, src.IsOnlySupportAllowRules)
+	SortSecurityRule(srcOutRules, src.MaxPriority, src.MinPriority, true, src.IsOnlySupportAllowRules)
+
+	SortSecurityRule(destInRules, dest.MaxPriority, dest.MinPriority, true, dest.IsOnlySupportAllowRules)
+	SortSecurityRule(destOutRules, dest.MaxPriority, dest.MinPriority, true, dest.IsOnlySupportAllowRules)
+
+	var addPriority = func(priority int, min, max int, onlyAllowRules bool) int {
 		if onlyAllowRules {
 			return priority
 		}
+		inc := 1
 		if max < min {
-			max, min, inc = min, max, inc*-1
+			max, min, inc = min, max, -1
 		}
 		if priority >= max || priority <= min {
 			return priority
@@ -238,103 +256,77 @@ func CompareRules(
 		return priority + inc
 	}
 
-	var getInitPriority = func(init, min, max int) int {
-		if init < min || init > max {
-			return (min + max) / 2
-		}
-		return init
-	}
-
-	var compare = func(localRules LocalSecurityRuleSet, remoteRules SecurityRuleSet) (common, add, del []SecurityRule) {
-		i, j, inc, prePriority, localPrority := 0, 0, 1, 0, 50
-		for i < len(localRules) || j < len(remoteRules) {
-			if i < len(localRules) && j < len(remoteRules) {
-				ruleStr := remoteRules[j].String()
-				localRuleStr := localRules[i].String()
+	var _compare = func(srcRules SecurityRuleSet, destRules SecurityRuleSet) (common, add, del SecurityRuleSet) {
+		i, j, destPriority, srcPrority := 0, 0, (dest.MinPriority-1+dest.MaxPriority)/2, (src.MinPriority-1+src.MaxPriority)/2
+		for i < len(srcRules) || j < len(destRules) {
+			if i < len(srcRules) && j < len(destRules) {
+				destRuleStr := destRules[j].String()
+				srcRuleStr := srcRules[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())
+					log.Debugf("compare src %s(%s) priority(%d) %s -> dest name(%s) %s(%s) priority(%d) %s\n",
+						srcRules[i].Id, srcRules[i].ExternalId, srcRules[i].Priority, srcRules[i].String(),
+						destRules[j].Name, destRules[j].ExternalId, destRules[j].Id, destRules[j].Priority, destRules[j].String())
 				}
-				cmp := strings.Compare(ruleStr, localRuleStr)
+				cmp := strings.Compare(destRuleStr, srcRuleStr)
 				if cmp == 0 {
-					prePriority = remoteRules[j].Priority
-					if remoteRules[j].ExternalId == DEFAULT_CLOUD_RULE_ID {
-						remoteRules[j].Priority = addPriority(remoteRules[j].Priority, 1, minPriority, maxPriority, onlyAllowRules)
+					destRules[j].SrcPrority = srcRules[i].Priority
+					destRules[j].Id = srcRules[i].Id
+					common = append(common, destRules[j])
+					if srcRules[i].Id != DEFAULT_SRC_RULE_ID {
+						srcPrority = srcRules[i].Priority
 					}
-					if localRules[i].ExternalId != DEFAULT_LOCAL_RULE_ID ||
-						(localRules[i].Direction == secrules.DIR_IN && !defaultInEquals) ||
-						(localRules[i].Direction == secrules.DIR_OUT && !defaultOutEquals) {
-						common = append(common, remoteRules[j])
+					if destRules[j].ExternalId != DEFAULT_DEST_RULE_ID {
+						destPriority = destRules[j].Priority
 					}
-					localPrority = localRules[i].Priority
 					i++
 					j++
 				} else if cmp < 0 {
-					if remoteRules[j].ExternalId != DEFAULT_CLOUD_RULE_ID {
-						remoteRules[j].LocalRulePrority = localPrority
-						localPrority = addPriority(localPrority, 1, 1, 100, false)
-						del = append(del, remoteRules[j])
-					}
+					destRules[j].SrcPrority = srcPrority
+					srcPrority = addPriority(srcPrority, src.MinPriority, src.MaxPriority, src.IsOnlySupportAllowRules)
+					del = append(del, destRules[j])
 					j++
 				} else {
-					initPriority := getInitPriority(prePriority, minPriority, maxPriority)
-					localRules[i].Priority = addPriority(initPriority, inc, minPriority, maxPriority, onlyAllowRules)
-					if localRules[i].ExternalId != DEFAULT_LOCAL_RULE_ID ||
-						(localRules[i].Direction == secrules.DIR_IN && !defaultInEquals) ||
-						(localRules[i].Direction == secrules.DIR_OUT && !defaultOutEquals) {
-						add = append(add, SecurityRule{SecurityRule: localRules[i].SecurityRule, ExternalId: localRules[i].ExternalId})
-					}
+					srcRules[i].Priority = addPriority(destPriority, dest.MinPriority, dest.MaxPriority, dest.IsOnlySupportAllowRules)
+					add = append(add, srcRules[i])
 					i++
-					inc++
-				}
-			} else if i >= len(localRules) {
-				if remoteRules[j].ExternalId != DEFAULT_CLOUD_RULE_ID {
-					remoteRules[j].LocalRulePrority = localPrority
-					localPrority = addPriority(localPrority, 1, 1, 100, false)
-					del = append(del, remoteRules[j])
 				}
+			} else if i >= len(srcRules) {
+				destRules[j].SrcPrority = srcPrority
+				srcPrority = addPriority(srcPrority, src.MinPriority, src.MaxPriority, false)
+				del = append(del, destRules[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, inc, minPriority, maxPriority, onlyAllowRules)
-				if localRules[i].ExternalId != DEFAULT_LOCAL_RULE_ID ||
-					(localRules[i].Direction == secrules.DIR_IN && !defaultInEquals) ||
-					(localRules[i].Direction == secrules.DIR_OUT && !defaultOutEquals) {
-					add = append(add, SecurityRule{SecurityRule: localRules[i].SecurityRule, ExternalId: localRules[i].ExternalId})
-				}
+			} else if j >= len(destRules) {
+				srcRules[i].Priority = addPriority(destPriority, dest.MinPriority, dest.MaxPriority, dest.IsOnlySupportAllowRules)
+				add = append(add, srcRules[i])
 				i++
-				inc++
 			}
 		}
 		return
 	}
 
 	type rulePair struct {
-		localRules  LocalSecurityRuleSet
-		remoteRules []SecurityRule
-		protocol    string
+		srcRules  SecurityRuleSet
+		destRules SecurityRuleSet
+		protocol  string
 	}
 
-	var splitRules = func(localRules LocalSecurityRuleSet, remoteRules []SecurityRule) []rulePair {
+	var splitRules = func(src, dest SecurityRuleSet) []rulePair {
 		rules := map[string]rulePair{}
-		for _, r := range localRules {
+		for _, r := range src {
 			pair, ok := rules[r.Protocol]
 			if !ok {
-				pair = rulePair{localRules: LocalSecurityRuleSet{}, remoteRules: []SecurityRule{}, protocol: r.Protocol}
+				pair = rulePair{srcRules: SecurityRuleSet{}, destRules: SecurityRuleSet{}, protocol: r.Protocol}
 			}
-			pair.localRules = append(pair.localRules, r)
+			pair.srcRules = append(pair.srcRules, r)
 			rules[r.Protocol] = pair
 		}
 
-		for _, r := range remoteRules {
+		for _, r := range dest {
 			pair, ok := rules[r.Protocol]
 			if !ok {
-				pair = rulePair{localRules: LocalSecurityRuleSet{}, remoteRules: []SecurityRule{}, protocol: r.Protocol}
+				pair = rulePair{srcRules: SecurityRuleSet{}, destRules: SecurityRuleSet{}, protocol: r.Protocol}
 			}
-			pair.remoteRules = append(pair.remoteRules, r)
+			pair.destRules = append(pair.destRules, r)
 			rules[r.Protocol] = pair
 		}
 
@@ -345,24 +337,62 @@ func CompareRules(
 		return ret
 	}
 
-	var compareRules = func(localRules LocalSecurityRuleSet, remoteRules []SecurityRule) (common, add, dels []SecurityRule) {
-		pairs := splitRules(localRules, remoteRules)
+	var compare = func(src, dest SecurityRuleSet) (common, added, dels SecurityRuleSet) {
+		pairs := splitRules(src, dest)
 		for _, r := range pairs {
-			_common, _add, _dels := compare(r.localRules, r.remoteRules)
+			_common, _add, _dels := _compare(r.srcRules, r.destRules)
 			common = append(common, _common...)
-			add = append(add, _add...)
+			added = append(added, _add...)
 			dels = append(dels, _dels...)
 		}
 		return
 	}
 
-	if !inEquals {
-		inCommon, inAdds, inDels = compareRules(localInRules, inRules)
+	var inCommon, outCommon SecurityRuleSet
+	inCommon, inAdds, inDels = compare(srcInRules, destInRules)
+	outCommon, outAdds, outDels = compare(srcOutRules, destOutRules)
+
+	var handleDefaultRules = func(removed, added []SecurityRule, isOnlyAllowList bool) ([]SecurityRule, []SecurityRule) {
+		ret := []SecurityRule{}
+		for _, rule := range removed {
+			if rule.ExternalId == DEFAULT_DEST_RULE_ID {
+				if debug {
+					log.Debugf("remove dest default rule: %s external id %s priority: %d", rule.String(), rule.ExternalId, rule.Priority)
+				}
+				if rule.Action == secrules.SecurityRuleDeny && isOnlyAllowList {
+					continue
+				}
+				switch rule.Action {
+				case secrules.SecurityRuleDeny:
+					rule.Action = secrules.SecurityRuleAllow
+				case secrules.SecurityRuleAllow:
+					rule.Action = secrules.SecurityRuleDeny
+				}
+				rule.Priority = dest.MinPriority
+
+				find := false
+				for i := range added {
+					if added[i].String() == rule.String() {
+						find = true
+						break
+					}
+				}
+				if !find {
+					if debug {
+						log.Debugf("add new default rule: %s external id %s priority: %d", rule.String(), rule.ExternalId, rule.Priority)
+					}
+					added = append(added, rule)
+				}
+			} else {
+				ret = append(ret, rule)
+			}
+		}
+		return ret, added
 	}
-	if !outEquals {
-		outCommon, outAdds, outDels = compareRules(localOutRules, outRules)
-	}
-	common = append(inCommon, outCommon...)
+
+	inDels, inAdds = handleDefaultRules(inDels, inAdds, dest.IsOnlySupportAllowRules)
+	outDels, outAdds = handleDefaultRules(outDels, outAdds, dest.IsOnlySupportAllowRules)
+	common, _ = handleDefaultRules(append(inCommon, outCommon...), []SecurityRule{}, dest.IsOnlySupportAllowRules)
 	return
 }
 

pkg/compute/models/secgroupcache.go

@@ -404,11 +404,12 @@ func (self *SSecurityGroupCache) syncWithCloudSecurityGroup(ctx context.Context,
 	if cacheCount > 1 {
 		return nil
 	}
-	info, err := SecurityGroupManager.getRuleInfo(provider, ext)
+	dest := cloudprovider.NewSecRuleInfo(GetRegionDriver(provider.Provider))
+	dest.Rules, err = ext.GetRules()
 	if err != nil {
-		return errors.Wrapf(err, "getRuleInfo")
+		return errors.Wrapf(err, "GetRules")
 	}
-	secgroup.SyncSecurityGroupRules(ctx, userCred, info)
+	secgroup.SyncSecurityGroupRules(ctx, userCred, dest)
 	return nil
 }
 
@@ -647,19 +648,18 @@ func (self *SSecurityGroupCache) SyncRules() error {
 		return errors.Wrapf(err, "iSecgroup.GetRules")
 	}
 
-	maxPriority := region.GetDriver().GetSecurityGroupRuleMaxPriority()
-	minPriority := region.GetDriver().GetSecurityGroupRuleMinPriority()
-
-	defaultInRule := region.GetDriver().GetDefaultSecurityGroupInRule()
-	defaultOutRule := region.GetDriver().GetDefaultSecurityGroupOutRule()
-	onlyAllowRules := region.GetDriver().IsOnlySupportAllowRules()
-
 	localRules, err := secgroup.GetSecuritRuleSet()
 	if err != nil {
 		return errors.Wrapf(err, "GetSecuritRuleSet")
 	}
 
-	common, inAdds, outAdds, inDels, outDels := cloudprovider.CompareRules(minPriority, maxPriority, localRules, rules, defaultInRule, defaultOutRule, onlyAllowRules, false, false)
+	src := cloudprovider.NewSecRuleInfo(GetRegionDriver(api.CLOUD_PROVIDER_ONECLOUD))
+	src.Rules = localRules
+
+	dest := cloudprovider.NewSecRuleInfo(GetRegionDriver(region.Provider))
+	dest.Rules = rules
+
+	common, inAdds, outAdds, inDels, outDels := cloudprovider.CompareRules(src, dest, false)
 
 	if len(inAdds) == 0 && len(inDels) == 0 && len(outAdds) == 0 && len(outDels) == 0 {
 		return nil

pkg/compute/models/secgrouprules.go

@@ -469,7 +469,7 @@ func (self *SSecurityGroup) newFromCloudSecurityGroupRule(ctx context.Context, u
 		cidr = rule.IPNet.String()
 	}
 
-	rule.Priority = rule.LocalRulePrority
+	rule.Priority = rule.SrcPrority
 
 	err := rule.ValidateRule()
 	if err != nil {
@@ -477,7 +477,7 @@ func (self *SSecurityGroup) newFromCloudSecurityGroupRule(ctx context.Context, u
 	}
 
 	secrule := &SSecurityGroupRule{
-		Priority:    int64(rule.LocalRulePrority),
+		Priority:    int64(rule.Priority),
 		Protocol:    protocol,
 		Ports:       rule.GetPortsString(),
 		Direction:   string(rule.Direction),

pkg/compute/models/secgroups.go

@@ -560,8 +560,8 @@ func (self *SSecurityGroup) getSecurityRules() ([]SSecurityGroupRule, error) {
 	return rules, nil
 }
 
-func (self *SSecurityGroup) GetSecuritRuleSet() (cloudprovider.LocalSecurityRuleSet, error) {
-	ruleSet := cloudprovider.LocalSecurityRuleSet{}
+func (self *SSecurityGroup) GetSecuritRuleSet() (cloudprovider.SecurityRuleSet, error) {
+	ruleSet := cloudprovider.SecurityRuleSet{}
 	rules, err := self.getSecurityRules()
 	if err != nil {
 		return ruleSet, errors.Wrapf(err, "getSecurityRules")
@@ -572,7 +572,7 @@ func (self *SSecurityGroup) GetSecuritRuleSet() (cloudprovider.LocalSecurityRule
 		if err != nil {
 			return nil, errors.Wrapf(err, "toRule")
 		}
-		ruleSet = append(ruleSet, cloudprovider.LocalSecurityRule{SecurityRule: *rule, ExternalId: rules[i].Id})
+		ruleSet = append(ruleSet, cloudprovider.SecurityRule{SecurityRule: *rule, ExternalId: rules[i].Id})
 	}
 	return ruleSet, nil
 }
@@ -944,14 +944,18 @@ func (self *SSecurityGroup) removeRules(ruleIds []string, result *compare.SyncRe
 	}
 }
 
-func (self *SSecurityGroup) SyncSecurityGroupRules(ctx context.Context, userCred mcclient.TokenCredential, info *sRuleInfo) compare.SyncResult {
+func (self *SSecurityGroup) SyncSecurityGroupRules(ctx context.Context, userCred mcclient.TokenCredential, src cloudprovider.SecRuleInfo) compare.SyncResult {
 	result := compare.SyncResult{}
 	localRules, err := self.GetSecuritRuleSet()
 	if err != nil {
 		result.Error(errors.Wrapf(err, "GetSecuritRuleSet"))
 		return result
 	}
-	_, inDels, outDels, inAdds, outAdds := cloudprovider.CompareRules(info.minPriority, info.maxPriority, localRules, info.rules, info.defaultInRule, info.defaultOutRule, info.onlyAllowRules, false, true)
+
+	dest := cloudprovider.NewSecRuleInfo(GetRegionDriver(api.CLOUD_PROVIDER_ONECLOUD))
+	dest.Rules = localRules
+
+	_, inAdds, outAdds, inDels, outDels := cloudprovider.CompareRules(src, dest, false)
 	if len(inAdds)+len(inDels)+len(outAdds)+len(outDels) == 0 {
 		return result
 	}
@@ -982,54 +986,14 @@ func (self *SSecurityGroup) SyncSecurityGroupRules(ctx context.Context, userCred
 	return result
 }
 
-type sRuleInfo struct {
-	rules          []cloudprovider.SecurityRule
-	inRules        []cloudprovider.SecurityRule
-	outRules       []cloudprovider.SecurityRule
-	defaultInRule  cloudprovider.SecurityRule
-	defaultOutRule cloudprovider.SecurityRule
-	onlyAllowRules bool
-	maxPriority    int
-	minPriority    int
-}
-
-func (manager *SSecurityGroupManager) getRuleInfo(provider *SCloudprovider, extSec cloudprovider.ICloudSecurityGroup) (*sRuleInfo, error) {
-	regionDriver, err := provider.GetRegionDriver()
-	if err != nil {
-		return nil, errors.Wrap(err, "provider.GetRegionDriver")
-	}
-
-	rules, err := extSec.GetRules()
-	if err != nil {
-		return nil, errors.Wrap(err, "extSec.GetRules")
-	}
-
-	info := &sRuleInfo{
-		rules:          rules,
-		inRules:        []cloudprovider.SecurityRule{},
-		outRules:       []cloudprovider.SecurityRule{},
-		defaultInRule:  regionDriver.GetDefaultSecurityGroupInRule(),
-		defaultOutRule: regionDriver.GetDefaultSecurityGroupOutRule(),
-		onlyAllowRules: regionDriver.IsOnlySupportAllowRules(),
-		maxPriority:    regionDriver.GetSecurityGroupRuleMaxPriority(),
-		minPriority:    regionDriver.GetSecurityGroupRuleMinPriority(),
-	}
-
-	for i := range rules {
-		if rules[i].Direction == secrules.DIR_IN {
-			info.inRules = append(info.inRules, rules[i])
-		} else {
-			info.outRules = append(info.outRules, rules[i])
-		}
-	}
-	return info, nil
-}
-
 func (manager *SSecurityGroupManager) newFromCloudSecgroup(ctx context.Context, userCred mcclient.TokenCredential, provider *SCloudprovider, extSec cloudprovider.ICloudSecurityGroup) (*SSecurityGroup, error) {
-	info, err := manager.getRuleInfo(provider, extSec)
+	dest := cloudprovider.NewSecRuleInfo(GetRegionDriver(provider.Provider))
+	var err error
+	dest.Rules, err = extSec.GetRules()
 	if err != nil {
-		return nil, errors.Wrapf(err, "getRuleInfo")
+		return nil, errors.Wrapf(err, "extSec.GetRules")
 	}
+	src := cloudprovider.NewSecRuleInfo(GetRegionDriver(api.CLOUD_PROVIDER_ONECLOUD))
 
 	if options.Options.EnableAutoMergeSecurityGroup {
 		// 查询与provider在同域的安全组，比对寻找一个与云上安全组规则相同的安全组
@@ -1040,12 +1004,12 @@ func (manager *SSecurityGroupManager) newFromCloudSecgroup(ctx context.Context,
 			return nil, errors.Wrap(err, "db.FetchModelObjects")
 		}
 		for i := range secgroups {
-			localRules, err := secgroups[i].GetSecuritRuleSet()
+			src.Rules, err = secgroups[i].GetSecuritRuleSet()
 			if err != nil {
 				log.Warningf("GetSecuritRuleSet %s(%s) error: %v", secgroups[i].Name, secgroups[i].Id, err)
 				continue
 			}
-			_, inAdds, outAdds, inDels, outDels := cloudprovider.CompareRules(info.minPriority, info.maxPriority, localRules, info.rules, info.defaultInRule, info.defaultOutRule, info.onlyAllowRules, false, false)
+			_, inAdds, outAdds, inDels, outDels := cloudprovider.CompareRules(src, dest, false)
 			if len(inAdds) == 0 && len(outAdds) == 0 && len(inDels) == 0 && len(outDels) == 0 {
 				return &secgroups[i], nil
 			}
@@ -1072,7 +1036,7 @@ func (manager *SSecurityGroupManager) newFromCloudSecgroup(ctx context.Context,
 		return nil, errors.Wrapf(err, "Insert")
 	}
 
-	secgroup.SyncSecurityGroupRules(ctx, userCred, info)
+	secgroup.SyncSecurityGroupRules(ctx, userCred, dest)
 
 	db.OpsLog.LogEvent(&secgroup, db.ACT_CREATE, secgroup.GetShortDesc(ctx), userCred)
 	return &secgroup, nil

pkg/compute/regiondrivers/azure.go

@@ -66,11 +66,11 @@ func (self *SAzureRegionDriver) GetDefaultSecurityGroupOutRule() cloudprovider.S
 }
 
 func (self *SAzureRegionDriver) GetSecurityGroupRuleMaxPriority() int {
-	return 4096
+	return 100
 }
 
 func (self *SAzureRegionDriver) GetSecurityGroupRuleMinPriority() int {
-	return 100
+	return 4096
 }
 
 func (self *SAzureRegionDriver) ValidateCreateVpcData(ctx context.Context, userCred mcclient.TokenCredential, input api.VpcCreateInput) (api.VpcCreateInput, error) {

pkg/compute/regiondrivers/kvm.go

@@ -24,6 +24,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"
 
 	api "yunion.io/x/onecloud/pkg/apis/compute"
@@ -67,6 +68,26 @@ func (self *SKVMRegionDriver) GetProvider() string {
 	return api.CLOUD_PROVIDER_ONECLOUD
 }
 
+func GetDefaultSecurityGroupInRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("in:deny any")}
+}
+
+func GetDefaultSecurityGroupOutRule() cloudprovider.SecurityRule {
+	return cloudprovider.SecurityRule{SecurityRule: *secrules.MustParseSecurityRule("out:allow any")}
+}
+
+func GetSecurityGroupRuleMaxPriority() int {
+	return 100
+}
+
+func GetSecurityGroupRuleMinPriority() int {
+	return 1
+}
+
+func IsOnlySupportAllowRules() bool {
+	return false
+}
+
 func (self *SKVMRegionDriver) ValidateCreateLoadbalancerData(ctx context.Context, userCred mcclient.TokenCredential, ownerId mcclient.IIdentityProvider, data *jsonutils.JSONDict) (*jsonutils.JSONDict, error) {
 	networkV := validators.NewModelIdOrNameValidator("network", "network", ownerId)
 	addressV := validators.NewIPv4AddrValidator("address")

pkg/compute/regiondrivers/managedvirtual.go

@@ -1682,19 +1682,18 @@ func (self *SManagedVirtualizationRegionDriver) RequestSyncSecurityGroup(ctx con
 				return errors.Wrapf(err, "iSecgroup.GetRules")
 			}
 
-			maxPriority := region.GetDriver().GetSecurityGroupRuleMaxPriority()
-			minPriority := region.GetDriver().GetSecurityGroupRuleMinPriority()
-
-			defaultInRule := region.GetDriver().GetDefaultSecurityGroupInRule()
-			defaultOutRule := region.GetDriver().GetDefaultSecurityGroupOutRule()
-			onlyAllowRules := region.GetDriver().IsOnlySupportAllowRules()
-
 			localRules, err := secgroup.GetSecuritRuleSet()
 			if err != nil {
 				return errors.Wrapf(err, "GetSecuritRuleSet")
 			}
 
-			common, inAdds, outAdds, inDels, outDels := cloudprovider.CompareRules(minPriority, maxPriority, localRules, rules, defaultInRule, defaultOutRule, onlyAllowRules, false, false)
+			src := cloudprovider.NewSecRuleInfo(&SKVMRegionDriver{})
+			src.Rules = localRules
+
+			dest := cloudprovider.NewSecRuleInfo(region.GetDriver())
+			dest.Rules = rules
+
+			common, inAdds, outAdds, inDels, outDels := cloudprovider.CompareRules(src, dest, false)
 
 			if len(inAdds) == 0 && len(inDels) == 0 && len(outAdds) == 0 && len(outDels) == 0 {
 				return nil

pkg/compute/regiondrivers/secgroup_aliyun_test.go

@@ -15,37 +15,28 @@
 package regiondrivers
 
 import (
-	"sort"
 	"testing"
 
 	"yunion.io/x/onecloud/pkg/cloudprovider"
 )
 
 func TestAliyunRuleSync(t *testing.T) {
-	driver := SAliyunRegionDriver{}
-	maxPriority := driver.GetSecurityGroupRuleMaxPriority()
-	minPriority := driver.GetSecurityGroupRuleMinPriority()
-
-	defaultInRule := driver.GetDefaultSecurityGroupInRule()
-	defaultOutRule := driver.GetDefaultSecurityGroupOutRule()
-	isOnlyAllowRules := driver.IsOnlySupportAllowRules()
-
 	data := []TestData{
 		{
 			Name: "Test out rules",
-			LocalRules: cloudprovider.LocalSecurityRuleSet{
-				localRuleWithPriority("in:allow tcp 1212", 52),
-				localRuleWithPriority("in:allow tcp 22", 51),
-				localRuleWithPriority("in:allow tcp 3389", 50),
-				localRuleWithPriority("in:allow udp 1231", 49),
-				localRuleWithPriority("in:deny tcp 443", 48),
+			SrcRules: cloudprovider.SecurityRuleSet{
+				ruleWithPriority("in:allow tcp 1212", 52),
+				ruleWithPriority("in:allow tcp 22", 51),
+				ruleWithPriority("in:allow tcp 3389", 50),
+				ruleWithPriority("in:allow udp 1231", 49),
+				ruleWithPriority("in:deny tcp 443", 48),
 			},
-			RemoteRules: []cloudprovider.SecurityRule{
-				remoteRuleWithName("", "in:deny tcp 443", 1),
-				remoteRuleWithName("", "in:allow udp 1231", 1),
-				remoteRuleWithName("", "in:allow tcp 3389", 100),
-				remoteRuleWithName("", "in:allow tcp 22", 100),
-				remoteRuleWithName("", "in:allow tcp 1212", 100),
+			DestRules: []cloudprovider.SecurityRule{
+				ruleWithName("", "in:deny tcp 443", 1),
+				ruleWithName("", "in:allow udp 1231", 1),
+				ruleWithName("", "in:allow tcp 3389", 100),
+				ruleWithName("", "in:allow tcp 22", 100),
+				ruleWithName("", "in:allow tcp 1212", 100),
 			},
 			Common:  []cloudprovider.SecurityRule{},
 			InAdds:  []cloudprovider.SecurityRule{},
@@ -53,20 +44,23 @@ func TestAliyunRuleSync(t *testing.T) {
 			InDels:  []cloudprovider.SecurityRule{},
 			OutDels: []cloudprovider.SecurityRule{},
 		},
+		{
+			Name: "Test tcp rules",
+			SrcRules: cloudprovider.SecurityRuleSet{
+				ruleWithPriority("out:deny tcp 443", 48),
+			},
+			DestRules: []cloudprovider.SecurityRule{},
+			Common:    []cloudprovider.SecurityRule{},
+			InAdds:    []cloudprovider.SecurityRule{},
+			OutAdds: []cloudprovider.SecurityRule{
+				ruleWithName("", "out:deny tcp 443", 49),
+			},
+			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, d.LocalRules, d.RemoteRules, defaultInRule, defaultOutRule, isOnlyAllowRules, true, 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)
+		d.Test(t, &SKVMRegionDriver{}, &SAliyunRegionDriver{})
 	}
 }

pkg/compute/regiondrivers/secgroup_aws_test.go

@@ -21,51 +21,43 @@ import (
 )
 
 func TestAwsRuleSync(t *testing.T) {
-	driver := SAwsRegionDriver{}
-	maxPriority := driver.GetSecurityGroupRuleMaxPriority()
-	minPriority := driver.GetSecurityGroupRuleMinPriority()
-
-	defaultInRule := driver.GetDefaultSecurityGroupInRule()
-	defaultOutRule := driver.GetDefaultSecurityGroupOutRule()
-	isOnlyAllowRules := driver.IsOnlySupportAllowRules()
-
 	data := []TestData{
 		{
 			Name: "Test remove out allow rules",
-			LocalRules: cloudprovider.LocalSecurityRuleSet{
-				localRuleWithPriority("out:deny any", 1),
+			SrcRules: cloudprovider.SecurityRuleSet{
+				ruleWithPriority("out:deny any", 1),
 			},
-			RemoteRules: []cloudprovider.SecurityRule{
-				remoteRuleWithName("", "out:allow any", 1),
+			DestRules: []cloudprovider.SecurityRule{
+				ruleWithName("test-allow any", "out:allow any", 1),
 			},
 			Common:  []cloudprovider.SecurityRule{},
 			InAdds:  []cloudprovider.SecurityRule{},
 			OutAdds: []cloudprovider.SecurityRule{},
 			InDels:  []cloudprovider.SecurityRule{},
 			OutDels: []cloudprovider.SecurityRule{
-				remoteRuleWithName("", "out:allow any", 1),
+				ruleWithName("test-allow any", "out:allow any", 1),
 			},
 		},
 		{
 			Name: "Test out deny rules",
-			LocalRules: cloudprovider.LocalSecurityRuleSet{
-				localRuleWithPriority("out:deny any", 1),
+			SrcRules: cloudprovider.SecurityRuleSet{
+				ruleWithPriority("out:deny any", 1),
 			},
-			RemoteRules: []cloudprovider.SecurityRule{},
-			Common:      []cloudprovider.SecurityRule{},
-			InAdds:      []cloudprovider.SecurityRule{},
-			OutAdds:     []cloudprovider.SecurityRule{},
-			InDels:      []cloudprovider.SecurityRule{},
-			OutDels:     []cloudprovider.SecurityRule{},
+			DestRules: []cloudprovider.SecurityRule{},
+			Common:    []cloudprovider.SecurityRule{},
+			InAdds:    []cloudprovider.SecurityRule{},
+			OutAdds:   []cloudprovider.SecurityRule{},
+			InDels:    []cloudprovider.SecurityRule{},
+			OutDels:   []cloudprovider.SecurityRule{},
 		},
 		{
-			Name:        "Test out allow rules",
-			LocalRules:  cloudprovider.LocalSecurityRuleSet{},
-			RemoteRules: []cloudprovider.SecurityRule{},
-			Common:      []cloudprovider.SecurityRule{},
-			InAdds:      []cloudprovider.SecurityRule{},
+			Name:      "Test out allow rules",
+			SrcRules:  cloudprovider.SecurityRuleSet{},
+			DestRules: []cloudprovider.SecurityRule{},
+			Common:    []cloudprovider.SecurityRule{},
+			InAdds:    []cloudprovider.SecurityRule{},
 			OutAdds: []cloudprovider.SecurityRule{
-				remoteRuleWithName("", "out:allow any", 0),
+				ruleWithName("", "out:allow any", 0),
 			},
 			InDels:  []cloudprovider.SecurityRule{},
 			OutDels: []cloudprovider.SecurityRule{},
@@ -73,12 +65,6 @@ func TestAwsRuleSync(t *testing.T) {
 	}
 
 	for _, d := range data {
-		t.Logf("check %s", d.Name)
-		common, inAdds, outAdds, inDels, outDels := cloudprovider.CompareRules(minPriority, maxPriority, d.LocalRules, d.RemoteRules, defaultInRule, defaultOutRule, isOnlyAllowRules, true, false)
-		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)
+		d.Test(t, &SKVMRegionDriver{}, &SAwsRegionDriver{})
 	}
 }

pkg/compute/regiondrivers/secgroup_azure_test.go

@@ -15,94 +15,85 @@
 package regiondrivers
 
 import (
-	"sort"
 	"testing"
 
 	"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()
-	isOnlyAllowRules := driver.IsOnlySupportAllowRules()
-
 	data := []TestData{
 		{
-			Name:        "Test empty rules",
-			LocalRules:  cloudprovider.LocalSecurityRuleSet{},
-			RemoteRules: []cloudprovider.SecurityRule{},
-			Common:      []cloudprovider.SecurityRule{},
-			InAdds:      []cloudprovider.SecurityRule{},
+			Name:      "Test empty rules",
+			SrcRules:  cloudprovider.SecurityRuleSet{},
+			DestRules: []cloudprovider.SecurityRule{},
+			Common:    []cloudprovider.SecurityRule{},
+			InAdds:    []cloudprovider.SecurityRule{},
 			OutAdds: []cloudprovider.SecurityRule{
-				remoteRuleWithName("", "out:allow any", 2099),
+				ruleWithName("", "out:allow any", 2096),
 			},
 			InDels:  []cloudprovider.SecurityRule{},
 			OutDels: []cloudprovider.SecurityRule{},
 		},
 		{
 			Name: "Test diff rules",
-			LocalRules: cloudprovider.LocalSecurityRuleSet{
-				localRuleWithPriority("out:allow tcp 100-200", 99),
-				localRuleWithPriority("out:allow udp 200-300", 98),
+			SrcRules: cloudprovider.SecurityRuleSet{
+				ruleWithPriority("out:allow tcp 100-200", 99),
+				ruleWithPriority("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),
+			DestRules: []cloudprovider.SecurityRule{
+				ruleWithName("test-tcp", "out:allow tcp 100-200", 1000),
+				ruleWithName("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),
+				ruleWithName("test-tcp", "out:allow tcp 100-200", 1000),
+				ruleWithName("test-udp", "out:allow udp 200-300", 1002),
 			},
 			InAdds: []cloudprovider.SecurityRule{},
 			OutAdds: []cloudprovider.SecurityRule{
-				remoteRuleWithName("", "out:allow any", 2099),
+				ruleWithName("", "out:allow any", 2096),
 			},
 			InDels:  []cloudprovider.SecurityRule{},
 			OutDels: []cloudprovider.SecurityRule{},
 		},
 		{
 			Name: "Test add rules",
-			LocalRules: cloudprovider.LocalSecurityRuleSet{
-				localRuleWithPriority("in:allow tcp", 100),
-				localRuleWithPriority("in:allow udp", 99),
-				localRuleWithPriority("out:deny any", 1),
+			SrcRules: cloudprovider.SecurityRuleSet{
+				ruleWithPriority("in:allow tcp", 100),
+				ruleWithPriority("in:allow udp", 99),
+				ruleWithPriority("out:deny any", 1),
 			},
-			RemoteRules: []cloudprovider.SecurityRule{
-				remoteRuleWithName("allow-ssh", "in:allow tcp 22", 300),
+			DestRules: []cloudprovider.SecurityRule{
+				ruleWithName("allow-ssh", "in:allow tcp 22", 300),
 			},
 			Common: []cloudprovider.SecurityRule{},
 			InAdds: []cloudprovider.SecurityRule{
-				remoteRuleWithName("", "in:allow tcp", 2099),
-				remoteRuleWithName("", "in:allow udp", 2099),
+				ruleWithName("", "in:allow tcp", 2098),
+				ruleWithName("", "in:allow udp", 2098),
 			},
 			OutAdds: []cloudprovider.SecurityRule{},
 			InDels: []cloudprovider.SecurityRule{
-				remoteRuleWithName("allow-ssh", "in:allow tcp 22", 300),
+				ruleWithName("allow-ssh", "in:allow tcp 22", 300),
 			},
 			OutDels: []cloudprovider.SecurityRule{},
 		},
 		{
 			Name: "Test insert rules",
-			LocalRules: cloudprovider.LocalSecurityRuleSet{
-				localRuleWithPriority("in:allow tcp", 100),
-				localRuleWithPriority("in:allow udp", 99),
-				localRuleWithPriority("in:allow icmp", 98),
-				localRuleWithPriority("out:deny any", 1),
+			SrcRules: cloudprovider.SecurityRuleSet{
+				ruleWithPriority("in:allow tcp", 100),
+				ruleWithPriority("in:allow udp", 99),
+				ruleWithPriority("in:allow icmp", 98),
+				ruleWithPriority("out:deny any", 1),
 			},
-			RemoteRules: []cloudprovider.SecurityRule{
-				remoteRuleWithName("allow-tcp", "in:allow tcp", 300),
-				remoteRuleWithName("allow-icmp", "in:allow icmp", 400),
+			DestRules: []cloudprovider.SecurityRule{
+				ruleWithName("allow-tcp", "in:allow tcp", 300),
+				ruleWithName("allow-icmp", "in:allow icmp", 400),
 			},
 			Common: []cloudprovider.SecurityRule{
-				remoteRuleWithName("allow-tcp", "in:allow tcp", 300),
-				remoteRuleWithName("allow-icmp", "in:allow icmp", 400),
+				ruleWithName("allow-tcp", "in:allow tcp", 300),
+				ruleWithName("allow-icmp", "in:allow icmp", 400),
 			},
 			InAdds: []cloudprovider.SecurityRule{
-				remoteRuleWithName("", "in:allow udp", 2099),
+				ruleWithName("", "in:allow udp", 2098),
 			},
 			OutAdds: []cloudprovider.SecurityRule{},
 			InDels:  []cloudprovider.SecurityRule{},
@@ -110,19 +101,19 @@ func TestAzureRuleSync(t *testing.T) {
 		},
 		{
 			Name: "Test icmp rules",
-			LocalRules: cloudprovider.LocalSecurityRuleSet{
-				localRuleWithPriority("in:allow tcp 33", 10),
-				localRuleWithPriority("in:allow tcp 22", 1),
-				localRuleWithPriority("out:deny any", 1),
+			SrcRules: cloudprovider.SecurityRuleSet{
+				ruleWithPriority("in:allow tcp 33", 10),
+				ruleWithPriority("in:allow tcp 22", 1),
+				ruleWithPriority("out:deny any", 1),
 			},
-			RemoteRules: []cloudprovider.SecurityRule{
-				remoteRuleWithName("allow-tcp-22", "in:allow tcp 22", 300),
+			DestRules: []cloudprovider.SecurityRule{
+				ruleWithName("allow-tcp-22", "in:allow tcp 22", 300),
 			},
 			Common: []cloudprovider.SecurityRule{
-				remoteRuleWithName("allow-tcp-22", "in:allow tcp 22", 300),
+				ruleWithName("allow-tcp-22", "in:allow tcp 22", 300),
 			},
 			InAdds: []cloudprovider.SecurityRule{
-				remoteRuleWithName("", "in:allow tcp 33", 301),
+				ruleWithName("", "in:allow tcp 33", 301),
 			},
 			OutAdds: []cloudprovider.SecurityRule{},
 			InDels:  []cloudprovider.SecurityRule{},
@@ -130,88 +121,82 @@ func TestAzureRuleSync(t *testing.T) {
 		},
 		{
 			Name: "Test a rules",
-			LocalRules: cloudprovider.LocalSecurityRuleSet{
-				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),
+			SrcRules: cloudprovider.SecurityRuleSet{
+				ruleWithPriority("in:allow tcp 1050", 5),
+				ruleWithPriority("in:allow tcp 1011", 4),
+				ruleWithPriority("in:allow tcp 1002", 3),
+				ruleWithPriority("in:allow tcp 22", 2),
+				ruleWithPriority("in:allow udp 55", 1),
+				ruleWithPriority("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),
+			DestRules: []cloudprovider.SecurityRule{
+				ruleWithName("in_allow_udp_55_4014", "in:allow udp 55", 4014),
+				ruleWithName("in_allow_tcp_22_4013", "in:allow tcp 22", 4013),
+				ruleWithName("in_allow_tcp_1002_4012", "in:allow tcp 1002", 4012),
+				ruleWithName("in_allow_tcp_1010_4011", "in:allow tcp 1010", 4011),
+				ruleWithName("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),
+				ruleWithName("in_allow_tcp_22_4013", "in:allow tcp 22", 4013),
+				ruleWithName("in_allow_udp_55_4014", "in:allow udp 55", 4014),
 			},
 			InAdds: []cloudprovider.SecurityRule{
-				remoteRuleWithName("", "in:allow tcp 1011", 4013),
+				ruleWithName("", "in:allow tcp 1011", 4014),
+				ruleWithName("", "in:allow tcp 1050", 4014),
+				ruleWithName("", "in:allow tcp 1002", 4014),
 			},
 			OutAdds: []cloudprovider.SecurityRule{},
 			InDels: []cloudprovider.SecurityRule{
-				remoteRuleWithName("in_allow_tcp_1010_4011", "in:allow tcp 1010", 4011),
+				ruleWithName("in_allow_tcp_1050_4010", "in:allow tcp 1050", 4010),
+				ruleWithName("in_allow_tcp_1010_4011", "in:allow tcp 1010", 4011),
+				ruleWithName("in_allow_tcp_1002_4012", "in:allow tcp 1002", 4012),
 			},
 			OutDels: []cloudprovider.SecurityRule{},
 		},
 		{
 			Name: "Test b rules",
-			LocalRules: cloudprovider.LocalSecurityRuleSet{
-				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),
+			SrcRules: cloudprovider.SecurityRuleSet{
+				ruleWithPriority("in:allow udp 1055", 20),
+				ruleWithPriority("in:allow icmp", 15),
+				ruleWithPriority("in:allow tcp 1050", 5),
+				ruleWithPriority("in:allow tcp 1012", 4),
+				ruleWithPriority("in:allow tcp 1002", 3),
+				ruleWithPriority("in:allow tcp 22", 2),
+				ruleWithPriority("in:allow udp 55", 1),
+				ruleWithPriority("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),
+			DestRules: []cloudprovider.SecurityRule{
+				ruleWithName("in_allow_udp_55_4014", "in:allow udp 55", 4014),
+				ruleWithName("in_allow_tcp_22_4013", "in:allow tcp 22", 4013),
+				ruleWithName("in_allow_tcp_1002_4012", "in:allow tcp 1002", 4012),
+				ruleWithName("in_allow_tcp_1012_4011", "in:allow tcp 1012", 4011),
+				ruleWithName("in_allow_tcp_1050_4010", "in:allow tcp 1050", 4010),
+				ruleWithName("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),
+				ruleWithName("in_allow_tcp_22_4013", "in:allow tcp 22", 4013),
+				ruleWithName("in_allow_udp_55_4014", "in:allow udp 55", 4014),
 			},
 			InAdds: []cloudprovider.SecurityRule{
-				remoteRuleWithName("", "in:allow icmp", 2099),
-				remoteRuleWithName("", "in:allow udp 1055", 4015),
+				ruleWithName("", "in:allow icmp", 2098),
+				ruleWithName("", "in:allow udp 1055", 4015),
+				ruleWithName("", "in:allow tcp 1050", 4014),
+				ruleWithName("", "in:allow tcp 1012", 4014),
+				ruleWithName("", "in:allow tcp 1002", 4014),
 			},
 			OutAdds: []cloudprovider.SecurityRule{},
 			InDels: []cloudprovider.SecurityRule{
-				remoteRuleWithName("in_allow_tcp_1055_4009", "in:allow tcp 1055", 4009),
+				ruleWithName("in_allow_tcp_1055_4009", "in:allow tcp 1055", 4009),
+				ruleWithName("in_allow_tcp_1050_4010", "in:allow tcp 1050", 4010),
+				ruleWithName("in_allow_tcp_1012_4011", "in:allow tcp 1012", 4011),
+				ruleWithName("in_allow_tcp_1002_4012", "in:allow tcp 1002", 4012),
 			},
 			OutDels: []cloudprovider.SecurityRule{},
 		},
 	}
 
 	for _, d := range data {
-		t.Logf("check %s", d.Name)
-		common, inAdds, outAdds, inDels, outDels := cloudprovider.CompareRules(minPriority, maxPriority, d.LocalRules, d.RemoteRules, defaultInRule, defaultOutRule, isOnlyAllowRules, true, false)
-		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)
+		d.Test(t, &SKVMRegionDriver{}, &SAzureRegionDriver{})
 	}
 }
 
@@ -222,14 +207,14 @@ func TestAzureUniqPriorityRuleSync(t *testing.T) {
 	}{
 		{
 			in: cloudprovider.SecurityRuleSet{
-				remoteRuleWithName("", "out:allow icmp", 99),
-				remoteRuleWithName("", "out:allow tcp 100-200", 99),
-				remoteRuleWithName("", "out:allow udp 200-300", 99),
+				ruleWithName("", "out:allow icmp", 99),
+				ruleWithName("", "out:allow tcp 100-200", 99),
+				ruleWithName("", "out:allow udp 200-300", 99),
 			},
 			out: cloudprovider.SecurityRuleSet{
-				remoteRuleWithName("", "out:allow icmp", 99),
-				remoteRuleWithName("", "out:allow tcp 100-200", 98),
-				remoteRuleWithName("", "out:allow udp 200-300", 97),
+				ruleWithName("", "out:allow icmp", 99),
+				ruleWithName("", "out:allow tcp 100-200", 98),
+				ruleWithName("", "out:allow udp 200-300", 97),
 			},
 		},
 	}

pkg/compute/regiondrivers/secgroup_ctyun_test.go

@@ -15,35 +15,27 @@
 package regiondrivers
 
 import (
-	"sort"
 	"testing"
 
 	"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()
-	isOnlyAllowRules := driver.IsOnlySupportAllowRules()
 
 	data := []TestData{
 		{
 			Name: "Test out deny rules",
-			LocalRules: cloudprovider.LocalSecurityRuleSet{
-				localRuleWithPriority("out:deny tcp 200", 1),
+			SrcRules: cloudprovider.SecurityRuleSet{
+				ruleWithPriority("out:deny tcp 200", 1),
 			},
-			RemoteRules: []cloudprovider.SecurityRule{},
-			Common:      []cloudprovider.SecurityRule{},
-			InAdds:      []cloudprovider.SecurityRule{},
+			DestRules: []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),
+				ruleWithName("", "out:allow icmp", 0),
+				ruleWithName("", "out:allow tcp 1-199", 0),
+				ruleWithName("", "out:allow tcp 201-65535", 0),
+				ruleWithName("", "out:allow udp", 0),
 			},
 			InDels:  []cloudprovider.SecurityRule{},
 			OutDels: []cloudprovider.SecurityRule{},
@@ -51,17 +43,6 @@ func TestCtyunRuleSync(t *testing.T) {
 	}
 
 	for _, d := range data {
-		t.Logf("check %s", d.Name)
-		common, inAdds, outAdds, inDels, outDels := cloudprovider.CompareRules(minPriority, maxPriority, d.LocalRules, d.RemoteRules, defaultInRule, defaultOutRule, isOnlyAllowRules, true, false)
-		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)
+		d.Test(t, &SKVMRegionDriver{}, &SCtyunRegionDriver{})
 	}
 }

pkg/compute/regiondrivers/secgroup_kvm_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 (
+	"testing"
+
+	"yunion.io/x/onecloud/pkg/cloudprovider"
+)
+
+func TestKvmRuleSync(t *testing.T) {
+	data := []TestData{
+		{
+			Name:      "Test kvm deny rules",
+			SrcRules:  cloudprovider.SecurityRuleSet{},
+			DestRules: []cloudprovider.SecurityRule{},
+			Common:    []cloudprovider.SecurityRule{},
+			InAdds:    []cloudprovider.SecurityRule{},
+			OutAdds: []cloudprovider.SecurityRule{
+				ruleWithName("", "out:deny any", 1),
+			},
+			InDels:  []cloudprovider.SecurityRule{},
+			OutDels: []cloudprovider.SecurityRule{},
+		},
+	}
+
+	for _, d := range data {
+		d.Test(t, &SAzureRegionDriver{}, &SKVMRegionDriver{})
+	}
+
+	aliyun := []TestData{
+		{
+			Name: "Test aliyun rules",
+			SrcRules: cloudprovider.SecurityRuleSet{
+				ruleWithName("allow tcp 443", "in:allow tcp 443", 1),
+				ruleWithName("allow tcp 6379", "in:allow tcp 6379", 1),
+				ruleWithName("allow tcp 3389", "in:allow tcp 3389", 1),
+				ruleWithName("allow tcp 1521", "in:allow tcp 1521", 1),
+				ruleWithName("allow tcp 80", "in:allow tcp 80", 1),
+				ruleWithName("deny tcp 1521", "in:deny tcp 1521", 12),
+			},
+			DestRules: []cloudprovider.SecurityRule{
+				ruleWithName("allow tcp", "in:allow tcp", 51),
+				ruleWithName("allow tcp 1521", "in:allow tcp 1521", 50),
+				ruleWithName("allow tcp 3389", "in:allow tcp 3389", 50),
+				ruleWithName("allow tcp 443", "in:allow tcp 443", 50),
+				ruleWithName("allow tcp 6379", "in:allow tcp 6379", 50),
+				ruleWithName("allow tcp 80", "in:allow tcp 80", 50),
+				ruleWithName("allow tcp", "in:allow tcp", 1),
+			},
+			Common: []cloudprovider.SecurityRule{
+				ruleWithName("allow tcp 1521", "in:allow tcp 1521", 50),
+				ruleWithName("allow tcp 3389", "in:allow tcp 3389", 50),
+				ruleWithName("allow tcp 443", "in:allow tcp 443", 50),
+				ruleWithName("allow tcp 6379", "in:allow tcp 6379", 50),
+				ruleWithName("allow tcp 80", "in:allow tcp 80", 50),
+			},
+			InAdds:  []cloudprovider.SecurityRule{},
+			OutAdds: []cloudprovider.SecurityRule{},
+			InDels: []cloudprovider.SecurityRule{
+				ruleWithName("allow tcp", "in:allow tcp", 51),
+				ruleWithName("allow tcp", "in:allow tcp", 1),
+			},
+			OutDels: []cloudprovider.SecurityRule{},
+		},
+	}
+
+	for _, d := range aliyun {
+		d.Test(t, &SAliyunRegionDriver{}, &SKVMRegionDriver{})
+	}
+
+}

pkg/compute/regiondrivers/secgroup_openstack_test.go

@@ -21,44 +21,49 @@ import (
 )
 
 func TestOpenStackRuleSync(t *testing.T) {
-	driver := SOpenStackRegionDriver{}
-	maxPriority := driver.GetSecurityGroupRuleMaxPriority()
-	minPriority := driver.GetSecurityGroupRuleMinPriority()
-
-	defaultInRule := driver.GetDefaultSecurityGroupInRule()
-	defaultOutRule := driver.GetDefaultSecurityGroupOutRule()
-	isOnlyAllowRules := driver.IsOnlySupportAllowRules()
-
 	data := []TestData{
 		{
 			Name: "Test deny rules",
-			LocalRules: cloudprovider.LocalSecurityRuleSet{
-				localRuleWithPriority("in:deny any", 100),
-				localRuleWithPriority("in:allow any", 99),
-				localRuleWithPriority("out:allow any", 100),
+			SrcRules: cloudprovider.SecurityRuleSet{
+				ruleWithPriority("in:deny any", 100),
+				ruleWithPriority("in:allow any", 99),
+				ruleWithPriority("out:allow any", 100),
 			},
-			RemoteRules: []cloudprovider.SecurityRule{
-				remoteRuleWithName("", "in:allow any", 1),
+			DestRules: []cloudprovider.SecurityRule{
+				ruleWithName("", "in:allow any", 1),
 			},
 			Common: []cloudprovider.SecurityRule{},
 			InAdds: []cloudprovider.SecurityRule{},
 			OutAdds: []cloudprovider.SecurityRule{
-				remoteRuleWithName("", "out:allow any", 0),
+				ruleWithName("", "out:allow any", 0),
 			},
 			InDels: []cloudprovider.SecurityRule{
-				remoteRuleWithName("", "in:allow any", 1),
+				ruleWithName("", "in:allow any", 1),
+			},
+			OutDels: []cloudprovider.SecurityRule{},
+		},
+		{
+			Name: "Test deny rules",
+			SrcRules: cloudprovider.SecurityRuleSet{
+				ruleWithPriority("in:deny any", 100),
+				ruleWithPriority("in:allow any", 99),
+				ruleWithPriority("out:allow any", 100),
+			},
+			DestRules: []cloudprovider.SecurityRule{
+				ruleWithName("", "in:allow any", 0),
+				ruleWithName("", "out:allow any", 0),
+			},
+			Common:  []cloudprovider.SecurityRule{},
+			InAdds:  []cloudprovider.SecurityRule{},
+			OutAdds: []cloudprovider.SecurityRule{},
+			InDels: []cloudprovider.SecurityRule{
+				ruleWithName("", "in:allow any", 0),
 			},
 			OutDels: []cloudprovider.SecurityRule{},
 		},
 	}
 
 	for _, d := range data {
-		t.Logf("check %s", d.Name)
-		common, inAdds, outAdds, inDels, outDels := cloudprovider.CompareRules(minPriority, maxPriority, d.LocalRules, d.RemoteRules, defaultInRule, defaultOutRule, isOnlyAllowRules, true, false)
-		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)
+		d.Test(t, &SKVMRegionDriver{}, &SOpenStackRegionDriver{})
 	}
 }

pkg/compute/regiondrivers/secgroup_qcloud_test.go

@@ -15,33 +15,24 @@
 package regiondrivers
 
 import (
-	"sort"
 	"testing"
 
 	"yunion.io/x/onecloud/pkg/cloudprovider"
 )
 
 func TestQcloudRuleSync(t *testing.T) {
-	driver := SQcloudRegionDriver{}
-	maxPriority := driver.GetSecurityGroupRuleMaxPriority()
-	minPriority := driver.GetSecurityGroupRuleMinPriority()
-
-	defaultInRule := driver.GetDefaultSecurityGroupInRule()
-	defaultOutRule := driver.GetDefaultSecurityGroupOutRule()
-	isOnlyAllowRules := driver.IsOnlySupportAllowRules()
-
 	data := []TestData{
 		{
 			Name: "Test out rules",
-			LocalRules: cloudprovider.LocalSecurityRuleSet{
-				localRuleWithPriority("out:allow any", 11),
-				localRuleWithPriority("out:deny any", 10),
+			SrcRules: cloudprovider.SecurityRuleSet{
+				ruleWithPriority("out:allow any", 11),
+				ruleWithPriority("out:deny any", 10),
 			},
-			RemoteRules: []cloudprovider.SecurityRule{},
-			Common:      []cloudprovider.SecurityRule{},
-			InAdds:      []cloudprovider.SecurityRule{},
+			DestRules: []cloudprovider.SecurityRule{},
+			Common:    []cloudprovider.SecurityRule{},
+			InAdds:    []cloudprovider.SecurityRule{},
 			OutAdds: []cloudprovider.SecurityRule{
-				remoteRuleWithName("", "out:allow any", 49),
+				ruleWithName("", "out:allow any", 48),
 			},
 			InDels:  []cloudprovider.SecurityRule{},
 			OutDels: []cloudprovider.SecurityRule{},
@@ -49,17 +40,6 @@ func TestQcloudRuleSync(t *testing.T) {
 	}
 
 	for _, d := range data {
-		t.Logf("check %s", d.Name)
-		common, inAdds, outAdds, inDels, outDels := cloudprovider.CompareRules(minPriority, maxPriority, d.LocalRules, d.RemoteRules, defaultInRule, defaultOutRule, isOnlyAllowRules, true, false)
-		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)
+		d.Test(t, &SKVMRegionDriver{}, &SQcloudRegionDriver{})
 	}
 }

pkg/compute/regiondrivers/secgroup_test.go

@@ -16,47 +16,110 @@ package regiondrivers
 
 import (
 	"fmt"
+	"sort"
 	"testing"
 
-	"yunion.io/x/log"
 	"yunion.io/x/pkg/util/secrules"
+	"yunion.io/x/pkg/util/stringutils"
+	"yunion.io/x/pkg/utils"
 
 	"yunion.io/x/onecloud/pkg/cloudprovider"
 )
 
 type TestData struct {
-	Name        string
-	LocalRules  cloudprovider.LocalSecurityRuleSet
-	RemoteRules cloudprovider.SecurityRuleSet
-	Common      cloudprovider.SecurityRuleSet
-	InAdds      cloudprovider.SecurityRuleSet
-	OutAdds     cloudprovider.SecurityRuleSet
-	InDels      cloudprovider.SecurityRuleSet
-	OutDels     cloudprovider.SecurityRuleSet
+	Name      string
+	SrcRules  cloudprovider.SecurityRuleSet
+	DestRules cloudprovider.SecurityRuleSet
+	Common    cloudprovider.SecurityRuleSet
+	InAdds    cloudprovider.SecurityRuleSet
+	OutAdds   cloudprovider.SecurityRuleSet
+	InDels    cloudprovider.SecurityRuleSet
+	OutDels   cloudprovider.SecurityRuleSet
 }
 
-var localRuleWithPriority = func(ruleStr string, priority int) cloudprovider.LocalSecurityRule {
+func (d TestData) Clone() TestData {
+	return TestData{
+		Name:      d.Name,
+		SrcRules:  d.SrcRules,
+		DestRules: d.DestRules,
+		Common:    cloudprovider.SecurityRuleSet{},
+		InAdds:    cloudprovider.SecurityRuleSet{},
+		OutAdds:   cloudprovider.SecurityRuleSet{},
+		InDels:    cloudprovider.SecurityRuleSet{},
+		OutDels:   cloudprovider.SecurityRuleSet{},
+	}
+}
+
+func (d TestData) Test(t *testing.T, srcD, destD cloudprovider.SecDriver) {
+	t.Logf("check %s", d.Name)
+	src, dest := cloudprovider.NewSecRuleInfo(srcD), cloudprovider.NewSecRuleInfo(destD)
+	src.Rules, dest.Rules = d.SrcRules, d.DestRules
+	common, inAdds, outAdds, inDels, outDels := cloudprovider.CompareRules(src, dest, true)
+	check(t, "common", common, d.Common, 0, 0)
+	check(t, "inAdds", inAdds, d.InAdds, dest.MinPriority, dest.MaxPriority)
+	check(t, "outAdds", outAdds, d.OutAdds, dest.MinPriority, dest.MaxPriority)
+	check(t, "inDels", inDels, d.InDels, 0, 0)
+	check(t, "outDels", outDels, d.OutDels, 0, 0)
+
+	t.Logf("check %s reverse", d.Name)
+
+	rd := d.Clone()
+	dest.Rules = append(dest.Rules, inAdds...)
+	dest.Rules = append(dest.Rules, outAdds...)
+	externalIds := []string{}
+	for i := range inDels {
+		if len(inDels[i].ExternalId) > 0 {
+			externalIds = append(externalIds, inDels[i].ExternalId)
+		} else {
+			externalIds = append(externalIds, fmt.Sprintf("%s-%d", inDels[i].String(), inDels[i].Priority))
+		}
+	}
+	for i := range outDels {
+		if len(outDels[i].ExternalId) > 0 {
+			externalIds = append(externalIds, outDels[i].ExternalId)
+		} else {
+			externalIds = append(externalIds, fmt.Sprintf("%s-%d", outDels[i].String(), outDels[i].Priority))
+		}
+	}
+	destRules := cloudprovider.SecurityRuleSet{}
+	for i := range dest.Rules {
+		if utils.IsInStringArray(dest.Rules[i].ExternalId, externalIds) || utils.IsInStringArray(fmt.Sprintf("%s-%d", dest.Rules[i].String(), dest.Rules[i].Priority), externalIds) {
+			continue
+		}
+		destRules = append(destRules, dest.Rules[i])
+	}
+	dest.Rules = destRules
+	_, inAdds, outAdds, inDels, outDels = cloudprovider.CompareRules(src, dest, true)
+	//check(t, "common", common, rd.Common)
+	check(t, "inAdds", inAdds, rd.InAdds, dest.MinPriority, dest.MaxPriority)
+	check(t, "outAdds", outAdds, rd.OutAdds, dest.MinPriority, dest.MaxPriority)
+	check(t, "inDels", inDels, rd.InDels, 0, 0)
+	check(t, "outDels", outDels, rd.OutDels, 0, 0)
+}
+
+var ruleWithPriority = func(ruleStr string, priority int) cloudprovider.SecurityRule {
 	rule := secrules.MustParseSecurityRule(ruleStr)
 	if rule == nil {
-		log.Errorf("invalid rule str %s", ruleStr)
-		return cloudprovider.LocalSecurityRule{}
+		panic(fmt.Sprintf("invalid rule str %s", ruleStr))
+		return cloudprovider.SecurityRule{}
 	}
 	rule.Priority = priority
-	return cloudprovider.LocalSecurityRule{SecurityRule: *rule}
+	return cloudprovider.SecurityRule{SecurityRule: *rule, Id: stringutils.UUID4()}
 }
 
-var remoteRuleWithName = func(name, ruleStr string, priority int) cloudprovider.SecurityRule {
+var ruleWithName = func(name, ruleStr string, priority int) cloudprovider.SecurityRule {
 	return cloudprovider.SecurityRule{
 		Name:         name,
-		SecurityRule: localRuleWithPriority(ruleStr, priority).SecurityRule,
+		ExternalId:   name,
+		SecurityRule: ruleWithPriority(ruleStr, priority).SecurityRule,
 	}
 }
 
-var check = func(t *testing.T, name string, ret, expect []cloudprovider.SecurityRule) {
+var check = func(t *testing.T, name string, ret, expect []cloudprovider.SecurityRule, min, max int) {
 	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())
+			t.Logf("Name: %s id: %s external id: %s priority: %d %s\n", r.Name, r.Id, r.ExternalId, r.Priority, r.String())
 		}
 	}
 	if len(ret) != len(expect) {
@@ -64,21 +127,31 @@ var check = func(t *testing.T, name string, ret, expect []cloudprovider.Security
 		show(fmt.Sprintf("%s expect", name), expect)
 		t.Fatalf("invalid rules for %s current is %d expect %d", name, len(ret), len(expect))
 	}
+	sort.Sort(cloudprovider.SecurityRuleSet(ret))
+	sort.Sort(cloudprovider.SecurityRuleSet(expect))
+	if max < min {
+		max, min = min, max
+	}
 	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].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 max != min && (ret[i].Priority < min || ret[i].Priority > max) {
+			t.Fatalf("invalid index(%d) %s rules %s priority should be in [%d, %d] current is %d", i, name, ret[i].String(), min, max, ret[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())
 		}
+
 	}
 }