fix(region): optimized peer secgroup

pkg/cloudprovider/securitygroup.go

@@ -57,6 +57,9 @@ const DEFAULT_SRC_RULE_ID = "default_src_rule_id"
 type SecRuleInfo struct {
 	InDefaultRule           SecurityRule
 	OutDefaultRule          SecurityRule
+	in                      SecurityRuleSet
+	out                     SecurityRuleSet
+	rules                   SecurityRuleSet
 	Rules                   SecurityRuleSet
 	MinPriority             int
 	MaxPriority             int
@@ -64,10 +67,26 @@ type SecRuleInfo struct {
 	IsSupportPeerSecgroup   bool
 }
 
+func (r SecRuleInfo) getOffest(priority int) (int, int) {
+	if r.MinPriority < r.MaxPriority {
+		if priority >= r.MaxPriority {
+			return priority, -1
+		}
+		return priority + 1, 0
+	} else {
+		if priority <= r.MaxPriority {
+			return priority, 1
+		}
+		return priority - 1, 0
+	}
+}
+
 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
+	r.InDefaultRule.minPriority, r.InDefaultRule.maxPriority = r.MinPriority, r.MaxPriority
+	r.OutDefaultRule.minPriority, r.OutDefaultRule.maxPriority = r.MinPriority, r.MaxPriority
 	if max >= min {
 		r.InDefaultRule.Priority = min - 1
 		r.OutDefaultRule.Priority = min - 1
@@ -81,8 +100,21 @@ func (r SecRuleInfo) AddDefaultRule(d SecRuleInfo, inRules, outRules []SecurityR
 		r.OutDefaultRule.ExternalId = DEFAULT_DEST_RULE_ID
 	}
 
-	inRules = append(inRules, r.InDefaultRule)
-	outRules = append(outRules, r.OutDefaultRule)
+	var isWideRule = func(rules []SecurityRule) bool {
+		for _, rule := range rules {
+			if strings.HasSuffix(rule.String(), "allow any") || strings.HasSuffix(rule.String(), "deny any") {
+				return true
+			}
+		}
+		return false
+	}
+
+	if !isWideRule(inRules) {
+		inRules = append(inRules, r.InDefaultRule)
+	}
+	if !isWideRule(outRules) {
+		outRules = append(outRules, r.OutDefaultRule)
+	}
 	return inRules, outRules
 }
 
@@ -101,6 +133,10 @@ type SecurityGroupCreateInput struct {
 }
 
 type SecurityRule struct {
+	minPriority int
+	maxPriority int
+	offset      int
+
 	secrules.SecurityRule
 	Name       string
 	ExternalId string
@@ -109,6 +145,11 @@ type SecurityRule struct {
 	PeerSecgroupId string
 }
 
+func (self *SecurityRule) addPriority(offset int) {
+	self.Priority += offset
+	self.offset += offset
+}
+
 func (r SecurityRule) String() string {
 	if len(r.PeerSecgroupId) == 0 {
 		return r.SecurityRule.String()
@@ -118,19 +159,25 @@ func (r SecurityRule) String() string {
 
 type SecurityRuleSet []SecurityRule
 
-func (rules SecurityRuleSet) Split(isSupportPeerSecgroup bool) (in, out SecurityRuleSet, isStandardRules bool) {
-	isStandardRules = true
-	for i := 0; i < len(rules); i++ {
-		if len(rules[i].PeerSecgroupId) > 0 {
-			isStandardRules = false
+func (self *SecRuleInfo) Split() (in, out SecurityRuleSet) {
+	self.rules = SecurityRuleSet{}
+	self.in = SecurityRuleSet{}
+	self.out = SecurityRuleSet{}
+	for i := 0; i < len(self.Rules); i++ {
+		self.rules = append(self.rules, self.Rules[i])
+		if self.Rules[i].Direction == secrules.DIR_IN {
+			self.in = append(self.in, self.Rules[i])
+		} else {
+			self.out = append(self.out, self.Rules[i])
 		}
-		if !isSupportPeerSecgroup && len(rules[i].PeerSecgroupId) > 0 {
+		self.Rules[i].minPriority, self.Rules[i].maxPriority = self.MinPriority, self.MaxPriority
+		if !self.IsSupportPeerSecgroup && len(self.Rules[i].PeerSecgroupId) > 0 {
 			continue
 		}
-		if rules[i].Direction == secrules.DIR_IN {
-			in = append(in, rules[i])
+		if self.Rules[i].Direction == secrules.DIR_IN {
+			in = append(in, self.Rules[i])
 		} else {
-			out = append(out, rules[i])
+			out = append(out, self.Rules[i])
 		}
 	}
 	return
@@ -145,29 +192,63 @@ func (srs SecurityRuleSet) Swap(i, j int) {
 }
 
 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())
+	if srs[i].Priority < srs[j].Priority {
+		return true
+	}
+	if srs[i].Priority > srs[j].Priority {
+		return false
+	}
+	if len(srs) > 0 {
+		if (srs[0].minPriority <= srs[0].maxPriority && srs[i].String() < srs[j].String()) ||
+			(srs[0].minPriority > srs[0].maxPriority && srs[i].String() > srs[j].String()) {
+			return true
+		}
+	}
+	return false
 }
 
-func (srs SecurityRuleSet) AllowList() secrules.SecurityRuleSet {
+func (srs SecurityRuleSet) CanBeSplitByProtocol() bool {
+	firstNormalRuleIndex, find := 0, false
+	for idx, r := range srs {
+		if !(strings.HasSuffix(r.String(), "allow any") || strings.HasSuffix(r.String(), "deny any")) && !find {
+			firstNormalRuleIndex = idx
+			find = true
+		} else {
+			if r.ExternalId == DEFAULT_SRC_RULE_ID || r.ExternalId == DEFAULT_DEST_RULE_ID {
+				return true
+			}
+			if idx > firstNormalRuleIndex {
+				return false
+			}
+		}
+	}
+	return true
+}
+
+func (srs SecurityRuleSet) AllowList() (secrules.SecurityRuleSet, bool) {
 	rules := secrules.SecurityRuleSet{}
+	isOk := true
 	for _, r := range srs {
+		if len(r.PeerSecgroupId) > 0 {
+			isOk = false
+		}
 		rules = append(rules, r.SecurityRule)
 	}
-	return rules.AllowList()
+	return rules.AllowList(), isOk
 }
 
 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())
+		log.Debugf("Name: %s id: %s external_id: %s min: %d max: %d priority: %d %s", srs[i].Name, srs[i].Id, srs[i].ExternalId, srs[i].minPriority, srs[i].maxPriority, srs[i].Priority, srs[i].String())
 	}
 }
 
-func SortSecurityRule(rules SecurityRuleSet, max, min int, isAsc, onlyAllowRules bool) {
-	if (max >= min || onlyAllowRules) && !isAsc {
-		sort.Sort(sort.Reverse(rules))
+func SortSecurityRule(rules SecurityRuleSet, isAsc bool, info SecRuleInfo) {
+	if (info.MaxPriority > info.MinPriority && isAsc) || (info.MaxPriority < info.MinPriority && !isAsc) || (info.IsOnlySupportAllowRules && isAsc) {
+		sort.Sort(rules)
 		return
 	}
-	sort.Sort(rules)
+	sort.Sort(sort.Reverse(rules))
 	return
 }
 
@@ -183,103 +264,125 @@ func isAllowListEqual(src, dest secrules.SecurityRuleSet) bool {
 	return s1.IsEqual(s2)
 }
 
+func isPeerListEqual(src, dest SecurityRuleSet) bool {
+	if len(src) != len(dest) {
+		return false
+	}
+	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())
+	}
+	return s1.IsEqual(s2)
+}
+
 func CompareRules(src, dest SecRuleInfo, debug bool) (common, inAdds, outAdds, inDels, outDels SecurityRuleSet) {
-	srcInRules, srcOutRules, isSrcStandardRules := src.Rules.Split(src.IsSupportPeerSecgroup)
-	destInRules, destOutRules, isDestStandardRules := dest.Rules.Split(dest.IsSupportPeerSecgroup)
+	srcInRules, srcOutRules := src.Split()
+	destInRules, destOutRules := dest.Split()
 
 	srcInRules, srcOutRules = src.AddDefaultRule(dest, srcInRules, srcOutRules, true)
 	destInRules, destOutRules = dest.AddDefaultRule(src, destInRules, destOutRules, false)
 
-	if debug {
-		log.Debugf("src in rules: ")
-		srcInRules.Debug()
+	// AllowList 需要优先级从高到低排序
+	SortSecurityRule(srcInRules, false, src)
+	SortSecurityRule(srcOutRules, false, src)
+
+	SortSecurityRule(destInRules, false, dest)
+	SortSecurityRule(destOutRules, false, dest)
+
+	isInAllowSplit := srcInRules.CanBeSplitByProtocol() && destInRules.CanBeSplitByProtocol()
+	isOutAllowSplit := srcOutRules.CanBeSplitByProtocol() && destOutRules.CanBeSplitByProtocol()
+
+	srcInAllowList, srcInOk := srcInRules.AllowList()
+	srcOutAllowList, srcOutOk := srcOutRules.AllowList()
+
+	destInAllowList, destInOk := destInRules.AllowList()
+	destOutAllowList, destOutOk := destOutRules.AllowList()
+	inEquals, outEquals, inOk, outOk := isAllowListEqual(srcInAllowList, destInAllowList), isAllowListEqual(srcOutAllowList, destOutAllowList), srcInOk && destInOk, srcOutOk && destOutOk
+
+	if inEquals && outEquals && inOk && outOk {
+		common = dest.rules
+		return
 	}
 
-	if (isSrcStandardRules && isDestStandardRules) || (!src.IsSupportPeerSecgroup && !dest.IsSupportPeerSecgroup) {
-		// AllowList 需要优先级从高到低排序
-		SortSecurityRule(srcInRules, src.MaxPriority, src.MinPriority, false, src.IsOnlySupportAllowRules)
-		SortSecurityRule(srcOutRules, src.MaxPriority, src.MinPriority, false, src.IsOnlySupportAllowRules)
+	if debug {
+		log.Debugf("====desc sort====")
+		log.Debugf("src in rules: ")
+		srcInRules.Debug()
+		log.Debugf("src out rules: ")
+		srcOutRules.Debug()
+		log.Debugf("dest in rules: ")
+		destInRules.Debug()
+		log.Debugf("dest out rules: ")
+		destOutRules.Debug()
+		log.Debugf("====desc sort end====")
 
-		SortSecurityRule(destInRules, dest.MaxPriority, dest.MinPriority, false, dest.IsOnlySupportAllowRules)
-		SortSecurityRule(destOutRules, dest.MaxPriority, dest.MinPriority, false, dest.IsOnlySupportAllowRules)
+		log.Debugf("isInAllowSplit: %v isOutAllowSplit: %v", isInAllowSplit, isOutAllowSplit)
 
-		srcInAllowList := srcInRules.AllowList()
-		srcOutAllowList := srcOutRules.AllowList()
+		log.Debugf("AllowList:")
+		log.Debugf("In: src: %s dest: %s isEquals: %v", srcInAllowList.String(), destInAllowList.String(), inEquals)
+		log.Debugf("Out: src: %s dest: %s isEquals: %v", srcOutAllowList.String(), destOutAllowList.String(), outEquals)
+	}
 
-		destInAllowList := destInRules.AllowList()
-		destOutAllowList := destOutRules.AllowList()
-		inEquals, outEquals := isAllowListEqual(srcInAllowList, destInAllowList), isAllowListEqual(srcOutAllowList, destOutAllowList)
-
-		if inEquals && outEquals {
-			return
-		}
-
-		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 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)
-				}
+	var tryUseAllowList = func(defaultRule SecurityRule, allowList secrules.SecurityRuleSet, rules SecurityRuleSet, isOnlyAllowList bool, isOk bool) SecurityRuleSet {
+		if isOk && (len(allowList) < len(rules) || isOnlyAllowList) {
+			rules = SecurityRuleSet{}
+			for i := range allowList {
+				rule := SecurityRule{}
+				rule.SecurityRule = allowList[i]
+				rules = append(rules, rule)
 			}
-			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 !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
 	}
 
-	if debug {
-		log.Debugf("src in rules: ")
-		srcInRules.Debug()
+	srcInRules = tryUseAllowList(src.InDefaultRule, srcInAllowList, srcInRules, dest.IsOnlySupportAllowRules, inOk)
+	srcOutRules = tryUseAllowList(src.OutDefaultRule, srcOutAllowList, srcOutRules, dest.IsOnlySupportAllowRules, outOk)
+
+	if inEquals && inOk {
+		common = append(common, dest.in...)
+		srcInRules, destInRules = []SecurityRule{}, []SecurityRule{}
+	}
+	if outEquals && outOk {
+		common = append(common, dest.out...)
+		srcOutRules, destOutRules = []SecurityRule{}, []SecurityRule{}
 	}
 
 	// 默认从优先级低到高比较
-	SortSecurityRule(srcInRules, src.MaxPriority, src.MinPriority, true, src.IsOnlySupportAllowRules)
-	SortSecurityRule(srcOutRules, src.MaxPriority, src.MinPriority, true, src.IsOnlySupportAllowRules)
+	SortSecurityRule(srcInRules, true, src)
+	SortSecurityRule(srcOutRules, true, src)
 
-	SortSecurityRule(destInRules, dest.MaxPriority, dest.MinPriority, true, dest.IsOnlySupportAllowRules)
-	SortSecurityRule(destOutRules, dest.MaxPriority, dest.MinPriority, true, dest.IsOnlySupportAllowRules)
+	SortSecurityRule(destInRules, true, dest)
+	SortSecurityRule(destOutRules, true, dest)
 
-	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, -1
-		}
-		if priority >= max || priority <= min {
-			return priority
-		}
-		return priority + inc
+	if debug {
+		log.Debugf("====asc sort====")
+		log.Debugf("src in rules: ")
+		srcInRules.Debug()
+		log.Debugf("src out rules: ")
+		srcOutRules.Debug()
+		log.Debugf("dest in rules: ")
+		destInRules.Debug()
+		log.Debugf("dest out rules: ")
+		destOutRules.Debug()
+		log.Debugf("====asc sort end====")
 	}
 
 	var _compare = func(srcRules SecurityRuleSet, destRules SecurityRuleSet) (common, add, del SecurityRuleSet) {
-		i, j, priority := 0, 0, (dest.MinPriority-1+dest.MaxPriority)/2
+		i, j, priority := 0, 0, dest.MinPriority
+		if len(destRules) > 0 && (destRules[i].ExternalId != DEFAULT_DEST_RULE_ID) {
+			priority = destRules[0].Priority
+		}
 		for i < len(srcRules) || j < len(destRules) {
 			if i < len(srcRules) && j < len(destRules) {
 				destRuleStr := destRules[j].String()
@@ -302,8 +405,20 @@ func CompareRules(src, dest SecRuleInfo, debug bool) (common, inAdds, outAdds, i
 					del = append(del, destRules[j])
 					j++
 				} else {
-					priority = addPriority(priority, dest.MinPriority, dest.MaxPriority, dest.IsOnlySupportAllowRules)
+					offset := 0
+					if !dest.IsOnlySupportAllowRules && i-1 >= 0 && srcRules[i-1].Priority != srcRules[i].Priority {
+						priority, offset = dest.getOffest(priority)
+					}
+					if offset != 0 {
+						for r := range add {
+							add[r].addPriority(offset)
+						}
+						for r := range common {
+							common[r].addPriority(offset)
+						}
+					}
 					srcRules[i].Priority = priority
+					srcRules[i].minPriority, srcRules[i].maxPriority = dest.MinPriority, dest.MaxPriority
 					add = append(add, srcRules[i])
 					i++
 				}
@@ -311,8 +426,20 @@ func CompareRules(src, dest SecRuleInfo, debug bool) (common, inAdds, outAdds, i
 				del = append(del, destRules[j])
 				j++
 			} else if j >= len(destRules) {
-				priority = addPriority(priority, dest.MinPriority, dest.MaxPriority, dest.IsOnlySupportAllowRules)
+				offset := 0
+				if !dest.IsOnlySupportAllowRules && i-1 >= 0 && srcRules[i-1].Priority != srcRules[i].Priority {
+					priority, offset = dest.getOffest(priority)
+				}
 				srcRules[i].Priority = priority
+				srcRules[i].minPriority, srcRules[i].maxPriority = dest.MinPriority, dest.MaxPriority
+				if offset != 0 {
+					for r := range add {
+						add[r].addPriority(offset)
+					}
+					for r := range common {
+						common[r].addPriority(offset)
+					}
+				}
 				add = append(add, srcRules[i])
 				i++
 			}
@@ -365,8 +492,16 @@ func CompareRules(src, dest SecRuleInfo, debug bool) (common, inAdds, outAdds, i
 	}
 
 	var inCommon, outCommon SecurityRuleSet
-	inCommon, inAdds, inDels = compare(srcInRules, destInRules)
-	outCommon, outAdds, outDels = compare(srcOutRules, destOutRules)
+	if isInAllowSplit {
+		inCommon, inAdds, inDels = compare(srcInRules, destInRules)
+	} else {
+		inCommon, inAdds, inDels = _compare(srcInRules, destInRules)
+	}
+	if isOutAllowSplit {
+		outCommon, outAdds, outDels = compare(srcOutRules, destOutRules)
+	} else {
+		outCommon, outAdds, outDels = _compare(srcOutRules, destOutRules)
+	}
 
 	var handleDefaultRules = func(removed, added []SecurityRule, isOnlyAllowList bool) ([]SecurityRule, []SecurityRule) {
 		ret := []SecurityRule{}
@@ -408,7 +543,8 @@ func CompareRules(src, dest SecRuleInfo, debug bool) (common, inAdds, outAdds, i
 
 	inDels, inAdds = handleDefaultRules(inDels, inAdds, dest.IsOnlySupportAllowRules)
 	outDels, outAdds = handleDefaultRules(outDels, outAdds, dest.IsOnlySupportAllowRules)
-	common, _ = handleDefaultRules(append(inCommon, outCommon...), []SecurityRule{}, dest.IsOnlySupportAllowRules)
+	_common, _ := handleDefaultRules(append(inCommon, outCommon...), []SecurityRule{}, dest.IsOnlySupportAllowRules)
+	common = append(common, _common...)
 	return
 }
 

pkg/compute/regiondrivers/secgroup_aliyun_test.go

@@ -38,7 +38,13 @@ func TestAliyunRuleSync(t *testing.T) {
 				ruleWithName("", "in:allow tcp 22", 100),
 				ruleWithName("", "in:allow tcp 1212", 100),
 			},
-			Common:  []cloudprovider.SecurityRule{},
+			Common: []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),
+			},
 			InAdds:  []cloudprovider.SecurityRule{},
 			OutAdds: []cloudprovider.SecurityRule{},
 			InDels:  []cloudprovider.SecurityRule{},
@@ -53,7 +59,7 @@ func TestAliyunRuleSync(t *testing.T) {
 			Common:    []cloudprovider.SecurityRule{},
 			InAdds:    []cloudprovider.SecurityRule{},
 			OutAdds: []cloudprovider.SecurityRule{
-				ruleWithName("", "out:deny tcp 443", 49),
+				ruleWithName("", "out:deny tcp 443", 99),
 			},
 			InDels:  []cloudprovider.SecurityRule{},
 			OutDels: []cloudprovider.SecurityRule{},

pkg/compute/regiondrivers/secgroup_azure_test.go

@@ -29,7 +29,7 @@ func TestAzureRuleSync(t *testing.T) {
 			Common:    []cloudprovider.SecurityRule{},
 			InAdds:    []cloudprovider.SecurityRule{},
 			OutAdds: []cloudprovider.SecurityRule{
-				ruleWithName("", "out:allow any", 2096),
+				ruleWithName("", "out:allow any", 4096),
 			},
 			InDels:  []cloudprovider.SecurityRule{},
 			OutDels: []cloudprovider.SecurityRule{},
@@ -44,16 +44,18 @@ func TestAzureRuleSync(t *testing.T) {
 				ruleWithName("test-tcp", "out:allow tcp 100-200", 1000),
 				ruleWithName("test-udp", "out:allow udp 200-300", 1002),
 			},
-			Common: []cloudprovider.SecurityRule{
+			Common: []cloudprovider.SecurityRule{},
+			InAdds: []cloudprovider.SecurityRule{},
+			OutAdds: []cloudprovider.SecurityRule{
+				ruleWithName("", "out:allow tcp 100-200", 4094),
+				ruleWithName("", "out:allow udp 200-300", 4095),
+				ruleWithName("", "out:allow any", 4096),
+			},
+			InDels: []cloudprovider.SecurityRule{},
+			OutDels: []cloudprovider.SecurityRule{
 				ruleWithName("test-tcp", "out:allow tcp 100-200", 1000),
 				ruleWithName("test-udp", "out:allow udp 200-300", 1002),
 			},
-			InAdds: []cloudprovider.SecurityRule{},
-			OutAdds: []cloudprovider.SecurityRule{
-				ruleWithName("", "out:allow any", 2096),
-			},
-			InDels:  []cloudprovider.SecurityRule{},
-			OutDels: []cloudprovider.SecurityRule{},
 		},
 		{
 			Name: "Test add rules",
@@ -67,8 +69,8 @@ func TestAzureRuleSync(t *testing.T) {
 			},
 			Common: []cloudprovider.SecurityRule{},
 			InAdds: []cloudprovider.SecurityRule{
-				ruleWithName("", "in:allow tcp", 2098),
-				ruleWithName("", "in:allow udp", 2098),
+				ruleWithName("", "in:allow tcp", 4094),
+				ruleWithName("", "in:allow udp", 4095),
 			},
 			OutAdds: []cloudprovider.SecurityRule{},
 			InDels: []cloudprovider.SecurityRule{
@@ -89,14 +91,16 @@ func TestAzureRuleSync(t *testing.T) {
 				ruleWithName("allow-icmp", "in:allow icmp", 400),
 			},
 			Common: []cloudprovider.SecurityRule{
-				ruleWithName("allow-tcp", "in:allow tcp", 300),
 				ruleWithName("allow-icmp", "in:allow icmp", 400),
 			},
 			InAdds: []cloudprovider.SecurityRule{
-				ruleWithName("", "in:allow udp", 2098),
+				ruleWithName("", "in:allow tcp", 398),
+				ruleWithName("", "in:allow udp", 399),
 			},
 			OutAdds: []cloudprovider.SecurityRule{},
-			InDels:  []cloudprovider.SecurityRule{},
+			InDels: []cloudprovider.SecurityRule{
+				ruleWithName("allow-tcp", "in:allow tcp", 300),
+			},
 			OutDels: []cloudprovider.SecurityRule{},
 		},
 		{
@@ -113,7 +117,7 @@ func TestAzureRuleSync(t *testing.T) {
 				ruleWithName("allow-tcp-22", "in:allow tcp 22", 300),
 			},
 			InAdds: []cloudprovider.SecurityRule{
-				ruleWithName("", "in:allow tcp 33", 301),
+				ruleWithName("", "in:allow tcp 33", 299),
 			},
 			OutAdds: []cloudprovider.SecurityRule{},
 			InDels:  []cloudprovider.SecurityRule{},
@@ -139,17 +143,15 @@ func TestAzureRuleSync(t *testing.T) {
 			Common: []cloudprovider.SecurityRule{
 				ruleWithName("in_allow_tcp_22_4013", "in:allow tcp 22", 4013),
 				ruleWithName("in_allow_udp_55_4014", "in:allow udp 55", 4014),
+				ruleWithName("in_allow_tcp_1002_4012", "in:allow tcp 1002", 4012),
+				ruleWithName("in_allow_tcp_1050_4010", "in:allow tcp 1050", 4010),
 			},
 			InAdds: []cloudprovider.SecurityRule{
-				ruleWithName("", "in:allow tcp 1050", 4010),
 				ruleWithName("", "in:allow tcp 1011", 4011),
-				ruleWithName("", "in:allow tcp 1002", 4012),
 			},
 			OutAdds: []cloudprovider.SecurityRule{},
 			InDels: []cloudprovider.SecurityRule{
-				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{},
 		},
@@ -176,20 +178,17 @@ func TestAzureRuleSync(t *testing.T) {
 			Common: []cloudprovider.SecurityRule{
 				ruleWithName("in_allow_tcp_22_4013", "in:allow tcp 22", 4013),
 				ruleWithName("in_allow_udp_55_4014", "in:allow udp 55", 4014),
+				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),
 			},
 			InAdds: []cloudprovider.SecurityRule{
-				ruleWithName("", "in:allow icmp", 2096),
-				ruleWithName("", "in:allow tcp 1050", 4010),
-				ruleWithName("", "in:allow tcp 1012", 4011),
-				ruleWithName("", "in:allow tcp 1002", 4012),
-				ruleWithName("", "in:allow udp 1055", 4013),
+				ruleWithName("", "in:allow icmp", 4009),
+				ruleWithName("", "in:allow udp 1055", 4008),
 			},
 			OutAdds: []cloudprovider.SecurityRule{},
 			InDels: []cloudprovider.SecurityRule{
 				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{},
 		},

pkg/compute/regiondrivers/secgroup_kvm_test.go

@@ -41,6 +41,7 @@ func TestKvmRuleSync(t *testing.T) {
 	}
 
 	aliyun := []TestData{
+
 		{
 			Name: "Test aliyun rules",
 			SrcRules: cloudprovider.SecurityRuleSet{
@@ -60,19 +61,24 @@ func TestKvmRuleSync(t *testing.T) {
 				ruleWithName("allow tcp 80", "in:allow tcp 80", 50),
 				ruleWithName("allow tcp", "in:allow tcp", 1),
 			},
-			Common: []cloudprovider.SecurityRule{
+			Common: []cloudprovider.SecurityRule{},
+			InAdds: []cloudprovider.SecurityRule{
+				ruleWithName("", "in:allow tcp 1521", 1),
+				ruleWithName("", "in:allow tcp 3389", 1),
+				ruleWithName("", "in:allow tcp 443", 1),
+				ruleWithName("", "in:allow tcp 6379", 1),
+				ruleWithName("", "in:allow tcp 80", 1),
+			},
+			OutAdds: []cloudprovider.SecurityRule{},
+			InDels: []cloudprovider.SecurityRule{
+				ruleWithName("allow tcp", "in:allow tcp", 51),
+				ruleWithName("allow tcp", "in:allow tcp", 1),
 				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{},
 		},
 		{
@@ -84,8 +90,8 @@ func TestKvmRuleSync(t *testing.T) {
 			DestRules: []cloudprovider.SecurityRule{},
 			Common:    []cloudprovider.SecurityRule{},
 			InAdds: []cloudprovider.SecurityRule{
-				ruleWithPeerSecgroup("allow tcp 443", "in:allow tcp 443", 51, "peer1"),
-				ruleWithName("deny tcp 1521", "in:deny tcp 1521", 51),
+				ruleWithPeerSecgroup("allow tcp 443", "in:allow tcp 443", 3, "peer1"),
+				ruleWithName("deny tcp 1521", "in:deny tcp 1521", 2),
 			},
 			OutAdds: []cloudprovider.SecurityRule{},
 			InDels:  []cloudprovider.SecurityRule{},

pkg/compute/regiondrivers/secgroup_openstack_test.go

@@ -43,7 +43,7 @@ func TestOpenStackRuleSync(t *testing.T) {
 			OutDels: []cloudprovider.SecurityRule{},
 		},
 		{
-			Name: "Test deny rules",
+			Name: "Test deny rules 2",
 			SrcRules: cloudprovider.SecurityRuleSet{
 				ruleWithPriority("in:deny any", 100),
 				ruleWithPriority("in:allow any", 99),
@@ -53,7 +53,9 @@ func TestOpenStackRuleSync(t *testing.T) {
 				ruleWithName("", "in:allow any", 0),
 				ruleWithName("", "out:allow any", 0),
 			},
-			Common:  []cloudprovider.SecurityRule{},
+			Common: []cloudprovider.SecurityRule{
+				ruleWithName("", "out:allow any", 0),
+			},
 			InAdds:  []cloudprovider.SecurityRule{},
 			OutAdds: []cloudprovider.SecurityRule{},
 			InDels: []cloudprovider.SecurityRule{

pkg/compute/regiondrivers/secgroup_qcloud_test.go

@@ -22,6 +22,7 @@ import (
 
 func TestQcloudRuleSync(t *testing.T) {
 	data := []TestData{
+
 		{
 			Name: "Test out rules",
 			SrcRules: cloudprovider.SecurityRuleSet{
@@ -32,11 +33,98 @@ func TestQcloudRuleSync(t *testing.T) {
 			Common:    []cloudprovider.SecurityRule{},
 			InAdds:    []cloudprovider.SecurityRule{},
 			OutAdds: []cloudprovider.SecurityRule{
-				ruleWithName("", "out:allow any", 48),
+				ruleWithName("", "out:allow any", 100),
 			},
 			InDels:  []cloudprovider.SecurityRule{},
 			OutDels: []cloudprovider.SecurityRule{},
 		},
+		{
+			Name: "Test peer out rules",
+			SrcRules: cloudprovider.SecurityRuleSet{
+				ruleWithPeerSecgroup("", "out:allow tcp", 1, "sec2"),
+				ruleWithPriority("out:deny any", 1),
+			},
+			DestRules: []cloudprovider.SecurityRule{
+				ruleWithPeerSecgroup("", "out:allow tcp", 2, "sec2"),
+				ruleWithPriority("out:deny any", 1),
+			},
+			Common: []cloudprovider.SecurityRule{
+				ruleWithPeerSecgroup("", "out:allow tcp", 2, "sec2"),
+				ruleWithPriority("out:deny any", 1),
+			},
+			InAdds:  []cloudprovider.SecurityRule{},
+			OutAdds: []cloudprovider.SecurityRule{},
+			InDels:  []cloudprovider.SecurityRule{},
+			OutDels: []cloudprovider.SecurityRule{},
+		},
+		{
+			Name: "Test peer out rules priority",
+			SrcRules: cloudprovider.SecurityRuleSet{
+				ruleWithPeerSecgroup("", "out:allow tcp", 2, "sec2"),
+				ruleWithPriority("out:deny any", 1),
+			},
+			DestRules: []cloudprovider.SecurityRule{
+				ruleWithPeerSecgroup("", "out:allow tcp", 2, "sec2"),
+				ruleWithPriority("out:deny any", 1),
+			},
+			Common: []cloudprovider.SecurityRule{
+				ruleWithPriority("out:deny any", 1),
+			},
+			InAdds: []cloudprovider.SecurityRule{},
+			OutAdds: []cloudprovider.SecurityRule{
+				ruleWithPeerSecgroup("", "out:allow tcp", 0, "sec2"),
+			},
+			InDels: []cloudprovider.SecurityRule{},
+			OutDels: []cloudprovider.SecurityRule{
+				ruleWithPeerSecgroup("", "out:allow tcp", 2, "sec2"),
+			},
+		},
+		{
+			Name: "Test peer out rules priority 2",
+			SrcRules: cloudprovider.SecurityRuleSet{
+				ruleWithPeerSecgroup("", "out:deny tcp", 4, "sec2"),
+				ruleWithPriority("out:allow any", 5),
+			},
+			DestRules: []cloudprovider.SecurityRule{
+				ruleWithPeerSecgroup("", "out:deny tcp", 0, "sec2"),
+				ruleWithPriority("out:allow any", 3),
+			},
+			Common: []cloudprovider.SecurityRule{
+				ruleWithPeerSecgroup("", "out:deny tcp", 1, "sec2"),
+			},
+			InAdds: []cloudprovider.SecurityRule{},
+			OutAdds: []cloudprovider.SecurityRule{
+				ruleWithPriority("out:allow any", 0),
+			},
+			InDels: []cloudprovider.SecurityRule{},
+			OutDels: []cloudprovider.SecurityRule{
+				ruleWithPriority("out:allow any", 3),
+			},
+		},
+		{
+			Name: "Test peer out rules 1",
+			SrcRules: cloudprovider.SecurityRuleSet{
+				ruleWithPeerSecgroup("", "out:deny tcp 22", 60, "Sys-Default"),
+				ruleWithPriority("out:allow 10.0.0.0/8 udp", 10),
+			},
+			DestRules: []cloudprovider.SecurityRule{
+				ruleWithPriority("out:allow any", 1),
+				ruleWithPeerSecgroup("", "out:deny tcp 22", 2, "Sys-Default"),
+				ruleWithPriority("out:allow 10.0.0.0/8 udp", 3),
+				ruleWithPriority("out:allow any", 4),
+			},
+			Common: []cloudprovider.SecurityRule{
+				ruleWithPeerSecgroup("", "out:deny tcp 22", 2, "Sys-Default"),
+				ruleWithPriority("out:allow 10.0.0.0/8 udp", 3),
+				ruleWithPriority("out:allow any", 4),
+			},
+			InAdds:  []cloudprovider.SecurityRule{},
+			OutAdds: []cloudprovider.SecurityRule{},
+			InDels:  []cloudprovider.SecurityRule{},
+			OutDels: []cloudprovider.SecurityRule{
+				ruleWithPriority("out:allow any", 1),
+			},
+		},
 	}
 
 	for _, d := range data {

pkg/compute/regiondrivers/secgroup_test.go

@@ -21,7 +21,6 @@ import (
 
 	"yunion.io/x/pkg/util/secrules"
 	"yunion.io/x/pkg/util/stringutils"
-	"yunion.io/x/pkg/utils"
 
 	"yunion.io/x/onecloud/pkg/cloudprovider"
 )
@@ -81,15 +80,8 @@ func (d TestData) Test(t *testing.T, srcD, destD cloudprovider.SecDriver) {
 			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)
+	dest.Rules = append(append(common, inAdds...), outAdds...)
+	_, inAdds, outAdds, inDels, outDels = cloudprovider.CompareRules(dest, src, 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)
@@ -146,11 +138,11 @@ var check = func(t *testing.T, name string, ret, expect []cloudprovider.Security
 			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)
 		}

pkg/multicloud/aliyun/securitygroup.go

@@ -475,7 +475,10 @@ func (self *SPermission) toRule() (cloudprovider.SecurityRule, error) {
 	if self.Direction == "egress" {
 		rule.Direction = secrules.DIR_OUT
 		cidr = self.DestCidrIp
-		rule.PeerSecgroupId = self.DestGroupId
+	}
+	if len(self.SourceGroupId) > 0 || len(self.DestGroupId) > 0 {
+		cidr = "0.0.0.0/0"
+		rule.PeerSecgroupId = self.SourceGroupId + self.DestGroupId
 	}
 
 	rule.ParseCIDR(cidr)

pkg/multicloud/qcloud/securitygroup.go

@@ -16,6 +16,7 @@ package qcloud
 
 import (
 	"fmt"
+	"sort"
 	"strconv"
 	"strings"
 	"time"
@@ -191,8 +192,8 @@ func (self *SecurityGroupPolicy) toRules() []cloudprovider.SecurityRule {
 		},
 	}
 	if len(self.SecurityGroupId) != 0 {
-		//安全组关联安全组的规则忽略
-		return nil
+		rule.ParseCIDR("0.0.0.0/0")
+		rule.PeerSecgroupId = self.SecurityGroupId
 	}
 	if strings.ToLower(self.Action) == "drop" {
 		rule.Action = secrules.SecurityRuleDeny
@@ -339,6 +340,7 @@ func (self *SSecurityGroup) deleteRules(rules []cloudprovider.SecurityRule, dire
 
 func (self *SSecurityGroup) SyncRules(common, inAdds, outAdds, inDels, outDels []cloudprovider.SecurityRule) error {
 	rules := append(common, append(inAdds, outAdds...)...)
+	sort.Sort(cloudprovider.SecurityRuleSet(rules))
 	return self.region.syncSecgroupRules(self.SecurityGroupId, rules)
 }