Skip to content

Commit d880731

Browse files
abhishek-pandey-1abhishek-pandey-1
committed
comply with code-style and improve coverage
Signed-off-by: Abhishek Pandey <abhpandey@microsoft.com>
1 parent a0d7e78 commit d880731

3 files changed

Lines changed: 221 additions & 8 deletions

File tree

‎pkg/controller/security_group_test.go‎

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,12 @@ import (
55

66
"github.com/stretchr/testify/require"
77
"go.uber.org/mock/gomock"
8+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
89

10+
kubeovnv1 "github.com/kubeovn/kube-ovn/pkg/apis/kubeovn/v1"
911
"github.com/kubeovn/kube-ovn/pkg/ovs"
1012
"github.com/kubeovn/kube-ovn/pkg/ovsdb/ovnnb"
13+
"github.com/kubeovn/kube-ovn/pkg/util"
1114
)
1215

1316
func mockLsp() *ovnnb.LogicalSwitchPort {
@@ -72,3 +75,142 @@ func Test_securityGroupALLNotExist(t *testing.T) {
7275
require.True(t, exist)
7376
})
7477
}
78+
79+
func Test_validateSgRule(t *testing.T) {
80+
t.Parallel()
81+
82+
ctrl := &Controller{}
83+
baseSG := func(rules ...kubeovnv1.SecurityGroupRule) *kubeovnv1.SecurityGroup {
84+
return &kubeovnv1.SecurityGroup{
85+
ObjectMeta: metav1.ObjectMeta{Name: "test-sg"},
86+
Spec: kubeovnv1.SecurityGroupSpec{
87+
SecurityGroupTier: util.SecurityGroupTierMinimum,
88+
IngressRules: rules,
89+
},
90+
}
91+
}
92+
93+
t.Run("valid local address as CIDR", func(t *testing.T) {
94+
t.Parallel()
95+
96+
sg := baseSG(kubeovnv1.SecurityGroupRule{
97+
IPVersion: "ipv4",
98+
Priority: 1,
99+
RemoteType: kubeovnv1.SgRemoteTypeAddress,
100+
RemoteAddress: "10.0.0.0/8",
101+
LocalAddress: "192.168.1.0/24",
102+
Protocol: "all",
103+
Policy: kubeovnv1.SgPolicy(ovnnb.ACLActionAllow),
104+
})
105+
err := ctrl.validateSgRule(sg)
106+
require.NoError(t, err)
107+
})
108+
109+
t.Run("valid local address as IP", func(t *testing.T) {
110+
t.Parallel()
111+
112+
sg := baseSG(kubeovnv1.SecurityGroupRule{
113+
IPVersion: "ipv4",
114+
Priority: 1,
115+
RemoteType: kubeovnv1.SgRemoteTypeAddress,
116+
RemoteAddress: "10.0.0.1",
117+
LocalAddress: "192.168.1.100",
118+
Protocol: "all",
119+
Policy: kubeovnv1.SgPolicy(ovnnb.ACLActionAllow),
120+
})
121+
err := ctrl.validateSgRule(sg)
122+
require.NoError(t, err)
123+
})
124+
125+
t.Run("invalid local address CIDR", func(t *testing.T) {
126+
t.Parallel()
127+
128+
sg := baseSG(kubeovnv1.SecurityGroupRule{
129+
IPVersion: "ipv4",
130+
Priority: 1,
131+
RemoteType: kubeovnv1.SgRemoteTypeAddress,
132+
RemoteAddress: "10.0.0.1",
133+
LocalAddress: "999.999.999.0/24",
134+
Protocol: "all",
135+
Policy: kubeovnv1.SgPolicy(ovnnb.ACLActionAllow),
136+
})
137+
err := ctrl.validateSgRule(sg)
138+
require.ErrorContains(t, err, "invalid CIDR")
139+
})
140+
141+
t.Run("invalid local address IP", func(t *testing.T) {
142+
t.Parallel()
143+
144+
sg := baseSG(kubeovnv1.SecurityGroupRule{
145+
IPVersion: "ipv4",
146+
Priority: 1,
147+
RemoteType: kubeovnv1.SgRemoteTypeAddress,
148+
RemoteAddress: "10.0.0.1",
149+
LocalAddress: "not-an-ip",
150+
Protocol: "all",
151+
Policy: kubeovnv1.SgPolicy(ovnnb.ACLActionAllow),
152+
})
153+
err := ctrl.validateSgRule(sg)
154+
require.ErrorContains(t, err, "invalid ip address")
155+
})
156+
157+
t.Run("valid local port range with TCP and local address", func(t *testing.T) {
158+
t.Parallel()
159+
160+
sg := baseSG(kubeovnv1.SecurityGroupRule{
161+
IPVersion: "ipv4",
162+
Priority: 1,
163+
RemoteType: kubeovnv1.SgRemoteTypeAddress,
164+
RemoteAddress: "10.0.0.1",
165+
LocalAddress: "192.168.1.100",
166+
Protocol: "tcp",
167+
Policy: kubeovnv1.SgPolicy(ovnnb.ACLActionAllow),
168+
PortRangeMin: 80,
169+
PortRangeMax: 443,
170+
LocalPortRangeMin: 1024,
171+
LocalPortRangeMax: 65535,
172+
})
173+
err := ctrl.validateSgRule(sg)
174+
require.NoError(t, err)
175+
})
176+
177+
t.Run("invalid local port range out of bounds", func(t *testing.T) {
178+
t.Parallel()
179+
180+
sg := baseSG(kubeovnv1.SecurityGroupRule{
181+
IPVersion: "ipv4",
182+
Priority: 1,
183+
RemoteType: kubeovnv1.SgRemoteTypeAddress,
184+
RemoteAddress: "10.0.0.1",
185+
LocalAddress: "192.168.1.100",
186+
Protocol: "tcp",
187+
Policy: kubeovnv1.SgPolicy(ovnnb.ACLActionAllow),
188+
PortRangeMin: 80,
189+
PortRangeMax: 443,
190+
LocalPortRangeMin: 0,
191+
LocalPortRangeMax: 65535,
192+
})
193+
err := ctrl.validateSgRule(sg)
194+
require.ErrorContains(t, err, "portRange is out of range")
195+
})
196+
197+
t.Run("invalid local port range min greater than max", func(t *testing.T) {
198+
t.Parallel()
199+
200+
sg := baseSG(kubeovnv1.SecurityGroupRule{
201+
IPVersion: "ipv4",
202+
Priority: 1,
203+
RemoteType: kubeovnv1.SgRemoteTypeAddress,
204+
RemoteAddress: "10.0.0.1",
205+
LocalAddress: "192.168.1.100",
206+
Protocol: "udp",
207+
Policy: kubeovnv1.SgPolicy(ovnnb.ACLActionAllow),
208+
PortRangeMin: 80,
209+
PortRangeMax: 443,
210+
LocalPortRangeMin: 9000,
211+
LocalPortRangeMax: 8000,
212+
})
213+
err := ctrl.validateSgRule(sg)
214+
require.ErrorContains(t, err, "range Minimum value greater than maximum value")
215+
})
216+
}

‎pkg/ovs/ovn-nb-acl.go‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1066,8 +1066,8 @@ func (c *OVNNbClient) newSgRuleACL(sgName, direction string, rule kubeovnv1.Secu
10661066
portDirection = "inport"
10671067
}
10681068

1069-
remoteipKey := ipSuffix + "." + remoteSrcOrDst
1070-
localipKey := ipSuffix + "." + localSrcOrDst
1069+
remoteIPKey := ipSuffix + "." + remoteSrcOrDst
1070+
localIPKey := ipSuffix + "." + localSrcOrDst
10711071

10721072
/* match all traffic to or from pgName */
10731073
allIPMatch := NewAndACLMatch(
@@ -1077,10 +1077,9 @@ func (c *OVNNbClient) newSgRuleACL(sgName, direction string, rule kubeovnv1.Secu
10771077

10781078
/* allow allowed ip traffic */
10791079
// type address
1080-
10811080
allowedIPMatch := NewAndACLMatch(
10821081
allIPMatch,
1083-
NewACLMatch(remoteipKey, "==", rule.RemoteAddress, ""),
1082+
NewACLMatch(remoteIPKey, "==", rule.RemoteAddress, ""),
10841083
)
10851084

10861085
// type securityGroup
@@ -1091,15 +1090,15 @@ func (c *OVNNbClient) newSgRuleACL(sgName, direction string, rule kubeovnv1.Secu
10911090
if rule.RemoteType == kubeovnv1.SgRemoteTypeSg {
10921091
allowedIPMatch = NewAndACLMatch(
10931092
allIPMatch,
1094-
NewACLMatch(remoteipKey, "==", "$"+remotePgName, ""),
1093+
NewACLMatch(remoteIPKey, "==", "$"+remotePgName, ""),
10951094
)
10961095
}
10971096

10981097
// Add a rule to match local address only if it is set
10991098
if rule.LocalAddress != "" {
11001099
allowedIPMatch = NewAndACLMatch(
11011100
allowedIPMatch,
1102-
NewACLMatch(localipKey, "==", rule.LocalAddress, ""),
1101+
NewACLMatch(localIPKey, "==", rule.LocalAddress, ""),
11031102
)
11041103
}
11051104

@@ -1137,8 +1136,7 @@ func (c *OVNNbClient) newSgRuleACL(sgName, direction string, rule kubeovnv1.Secu
11371136
action := ovnnb.ACLActionDrop
11381137
if rule.Policy == kubeovnv1.SgPolicyAllow {
11391138
action = ovnnb.ACLActionAllowRelated
1140-
}
1141-
if rule.Policy == kubeovnv1.SgPolicyPass {
1139+
} else if rule.Policy == kubeovnv1.SgPolicyPass {
11421140
action = ovnnb.ACLActionPass
11431141
}
11441142

‎pkg/ovs/ovn-nb-acl_test.go‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1380,6 +1380,79 @@ func (suite *OvnClientTestSuite) testNewSgRuleACL() {
13801380
expect.UUID = acl.UUID
13811381
require.Equal(t, expect, acl)
13821382
})
1383+
1384+
t.Run("create sg acl with local address", func(t *testing.T) {
1385+
t.Parallel()
1386+
1387+
sgRule := kubeovnv1.SecurityGroupRule{
1388+
IPVersion: "ipv4",
1389+
RemoteType: kubeovnv1.SgRemoteTypeAddress,
1390+
RemoteAddress: "10.10.10.0/24",
1391+
LocalAddress: "192.168.1.0/24",
1392+
Protocol: "all",
1393+
Priority: 5,
1394+
Policy: "allow",
1395+
}
1396+
priority := strconv.Itoa(highestPriority - sgRule.Priority)
1397+
1398+
acl, err := nbClient.newSgRuleACL(sgName, ovnnb.ACLDirectionToLport, sgRule, util.SecurityGroupTierMinimum)
1399+
require.NoError(t, err)
1400+
1401+
match := fmt.Sprintf("outport == @%s && ip4 && ip4.src == %s && ip4.dst == %s", pgName, sgRule.RemoteAddress, sgRule.LocalAddress)
1402+
expect := newACL(pgName, ovnnb.ACLDirectionToLport, priority, match, ovnnb.ACLActionAllowRelated, util.NetpolACLTier)
1403+
expect.UUID = acl.UUID
1404+
require.Equal(t, expect, acl)
1405+
})
1406+
1407+
t.Run("create tcp sg acl with local address and source port", func(t *testing.T) {
1408+
t.Parallel()
1409+
1410+
sgRule := kubeovnv1.SecurityGroupRule{
1411+
IPVersion: "ipv4",
1412+
RemoteType: kubeovnv1.SgRemoteTypeAddress,
1413+
RemoteAddress: "10.10.10.0/24",
1414+
LocalAddress: "192.168.1.100",
1415+
Protocol: "tcp",
1416+
Priority: 8,
1417+
Policy: "allow",
1418+
PortRangeMin: 80,
1419+
PortRangeMax: 443,
1420+
LocalPortRangeMin: 1024,
1421+
LocalPortRangeMax: 65535,
1422+
}
1423+
priority := strconv.Itoa(highestPriority - sgRule.Priority)
1424+
1425+
acl, err := nbClient.newSgRuleACL(sgName, ovnnb.ACLDirectionToLport, sgRule, util.SecurityGroupTierMinimum)
1426+
require.NoError(t, err)
1427+
1428+
match := fmt.Sprintf("outport == @%s && ip4 && ip4.src == %s && ip4.dst == %s && %d <= tcp.dst <= %d && %d <= tcp.src <= %d",
1429+
pgName, sgRule.RemoteAddress, sgRule.LocalAddress, sgRule.PortRangeMin, sgRule.PortRangeMax, sgRule.LocalPortRangeMin, sgRule.LocalPortRangeMax)
1430+
expect := newACL(pgName, ovnnb.ACLDirectionToLport, priority, match, ovnnb.ACLActionAllowRelated, util.NetpolACLTier)
1431+
expect.UUID = acl.UUID
1432+
require.Equal(t, expect, acl)
1433+
})
1434+
1435+
t.Run("create pass policy sg acl", func(t *testing.T) {
1436+
t.Parallel()
1437+
1438+
sgRule := kubeovnv1.SecurityGroupRule{
1439+
IPVersion: "ipv4",
1440+
RemoteType: kubeovnv1.SgRemoteTypeAddress,
1441+
RemoteAddress: "10.10.10.0/24",
1442+
Protocol: "icmp",
1443+
Priority: 10,
1444+
Policy: kubeovnv1.SgPolicy(ovnnb.ACLActionPass),
1445+
}
1446+
priority := strconv.Itoa(highestPriority - sgRule.Priority)
1447+
1448+
acl, err := nbClient.newSgRuleACL(sgName, ovnnb.ACLDirectionToLport, sgRule, util.SecurityGroupTierMinimum)
1449+
require.NoError(t, err)
1450+
1451+
match := fmt.Sprintf("outport == @%s && ip4 && ip4.src == %s && icmp4", pgName, sgRule.RemoteAddress)
1452+
expect := newACL(pgName, ovnnb.ACLDirectionToLport, priority, match, ovnnb.ACLActionPass, util.NetpolACLTier)
1453+
expect.UUID = acl.UUID
1454+
require.Equal(t, expect, acl)
1455+
})
13831456
}
13841457

13851458
func (suite *OvnClientTestSuite) testCreateAcls() {

0 commit comments

Comments
 (0)