Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: optimized rules allowList count #36

Merged
merged 1 commit into from
Nov 16, 2019

Conversation

ioito
Copy link
Contributor

@ioito ioito commented Nov 13, 2019

优化AllowList输出数量

/cc @yousong @swordqiu

@@ -59,6 +62,8 @@ func (srs SecurityRuleSet) equals(srs1 SecurityRuleSet) bool {
if len(srs) != len(srs1) {
return false
}
sort.Sort(srs)
sort.Sort(srs1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

equals()不应该变更输入参数内容

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已删除

@@ -190,6 +195,120 @@ func (srs SecurityRuleSet) collapse() SecurityRuleSet {
}
// save that contains, intersects
}
if srs1[i].PortStart <= 1 && srs1[i].PortEnd >= 65535 && len(srs1[i].Ports) == 0 {
srs1[i].PortStart = -1
srs1[i].PortEnd = -1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

应该是s < 1 && e > 65535

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已去掉端口转换

if sr0.GetPortsString() != sr1.GetPortsString() {
return sr0.GetPortsString() < sr1.GetPortsString()
}
return sr0.Priority < sr1.Priority
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里可以再以startip, endip排序

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已更新

for j := range addrRanges {
_merged = compareRange(addrRanges[i], addrRanges[j])
if _merged != nil {
addrRanges[i] = *_merged
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个复杂度太高,可以简化。相邻rule若protocol, port相同,只需要考虑range.IsOverlap()的情况,直接merge就可以。考虑给IPV4AddrRange添加一个Merge方法,返回指针

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已优化

@ioito ioito force-pushed the hotfix/qx-secgroup-optimized branch 2 times, most recently from dafef6b to b3bddc8 Compare November 14, 2019 06:09
@ioito ioito force-pushed the hotfix/qx-secgroup-optimized branch from b3bddc8 to 3aa6b83 Compare November 14, 2019 06:19
}
range0 := netutils.NewIPV4AddrRangeFromIPNet(sr0.IPNet)
range1 := netutils.NewIPV4AddrRangeFromIPNet(sr1.IPNet)
return range0.EndIp() < range1.StartIp()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

startip同endip比无法保证full order,是鸡同鸭比

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已更新

@ioito ioito force-pushed the hotfix/qx-secgroup-optimized branch 2 times, most recently from c15bf24 to 5b17d5d Compare November 14, 2019 06:46
preNet := ranges[len(ranges)-1]
nextNet := netutils.NewIPV4AddrRangeFromIPNet(srs[i].IPNet)
if preNet.IsOverlap(nextNet) || preNet.EndIp()+1 == nextNet.StartIp() {
ranges[len(ranges)-1] = netutils.NewIPV4AddrRange(preNet.StartIp(), nextNet.EndIp())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

考虑把这段封装成range.Merge()方法

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已封装

@ioito ioito force-pushed the hotfix/qx-secgroup-optimized branch 2 times, most recently from dd751ca to e6b64d6 Compare November 15, 2019 09:18
}
rules := srs0.AllowList()
for _, rule := range rules {
t.Logf("rule: %s", rule.String())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test用Logf写没有意义

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已更新

@yousong
Copy link
Contributor

yousong commented Nov 16, 2019

/approve
/lgtm

@yunion-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yousong

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@yunion-ci-robot yunion-ci-robot merged commit 1ad9417 into yunionio:master Nov 16, 2019
wanyaoqi pushed a commit to wanyaoqi/pkg that referenced this pull request Jan 2, 2020
…/add-signal-handler to master

* commit '557c3fe63902e89b894a3c6dd95a1932d9cc7883':
  add profiler
  add signal utils
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants