diff --git a/pkg/cloudprovider/securitygroup.go b/pkg/cloudprovider/securitygroup.go index c63554b521..a3503950fe 100644 --- a/pkg/cloudprovider/securitygroup.go +++ b/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 } diff --git a/pkg/compute/regiondrivers/secgroup_aliyun_test.go b/pkg/compute/regiondrivers/secgroup_aliyun_test.go index 9ced085fc7..b603286590 100644 --- a/pkg/compute/regiondrivers/secgroup_aliyun_test.go +++ b/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{}, diff --git a/pkg/compute/regiondrivers/secgroup_azure_test.go b/pkg/compute/regiondrivers/secgroup_azure_test.go index 09acd02b1d..07671364b1 100644 --- a/pkg/compute/regiondrivers/secgroup_azure_test.go +++ b/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{}, }, diff --git a/pkg/compute/regiondrivers/secgroup_kvm_test.go b/pkg/compute/regiondrivers/secgroup_kvm_test.go index 6f77f5e051..3f219045e0 100644 --- a/pkg/compute/regiondrivers/secgroup_kvm_test.go +++ b/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{}, diff --git a/pkg/compute/regiondrivers/secgroup_openstack_test.go b/pkg/compute/regiondrivers/secgroup_openstack_test.go index a42b331e57..d5de68a42f 100644 --- a/pkg/compute/regiondrivers/secgroup_openstack_test.go +++ b/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{ diff --git a/pkg/compute/regiondrivers/secgroup_qcloud_test.go b/pkg/compute/regiondrivers/secgroup_qcloud_test.go index 42c69180b2..5c6017a923 100644 --- a/pkg/compute/regiondrivers/secgroup_qcloud_test.go +++ b/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 { diff --git a/pkg/compute/regiondrivers/secgroup_test.go b/pkg/compute/regiondrivers/secgroup_test.go index 44130c1d40..88c8af5d09 100644 --- a/pkg/compute/regiondrivers/secgroup_test.go +++ b/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) } diff --git a/pkg/multicloud/aliyun/securitygroup.go b/pkg/multicloud/aliyun/securitygroup.go index edf953c6ee..04789974f6 100644 --- a/pkg/multicloud/aliyun/securitygroup.go +++ b/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) diff --git a/pkg/multicloud/qcloud/securitygroup.go b/pkg/multicloud/qcloud/securitygroup.go index 57a825d3de..0bc9a93dba 100644 --- a/pkg/multicloud/qcloud/securitygroup.go +++ b/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) }