Skip to content

Commit f35989c

Browse files
rayaisaiahrayaisaiah
committed
updated with match expressions edgecase
1 parent 1689071 commit f35989c

File tree

2 files changed

+141
-15
lines changed

2 files changed

+141
-15
lines changed

‎tools/azure-npm-to-cilium-validator/azure-npm-to-cilium-validator.go‎

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -232,37 +232,36 @@ func checkNoServiceRisk(service *corev1.Service, policiesListAtNamespace []*netw
232232
// Check if there is an allow all ingress policy that matches labels the service is safe
233233
if len(ingress.From) == 0 && len(ingress.Ports) == 0 {
234234
// Check if there is an allow all ingress policy with empty selectors or matching service labels as the policy allows all services in the namespace
235-
if checkPolicySelectorsAreEmpty(policy.Spec.PodSelector) || checkPolicyMatchServiceLabels(service.Spec.Selector, policy.Spec.PodSelector.MatchLabels) {
235+
if checkPolicyMatchServiceLabels(service.Spec.Selector, policy.Spec.PodSelector) {
236236
return true
237237
}
238238
}
239239
// If there are no ingress from but there are ports in the policy; check if the service is safe
240240
if len(ingress.From) == 0 && len(ingress.Ports) > 0 {
241241
// If the policy targets all pods (allow all) or only pods that are in the service selector, check if traffic is allowed to all the service's target ports
242-
if checkPolicySelectorsAreEmpty(policy.Spec.PodSelector) || checkPolicyMatchServiceLabels(service.Spec.Selector, policy.Spec.PodSelector.MatchLabels) {
243-
if checkServiceTargetPortMatchPolicyPorts(service.Spec.Ports, ingress.Ports) {
244-
return true
245-
}
242+
if checkPolicyMatchServiceLabels(service.Spec.Selector, policy.Spec.PodSelector) && checkServiceTargetPortMatchPolicyPorts(service.Spec.Ports, ingress.Ports) {
243+
return true
246244
}
247245
}
248246
}
249247
}
250248
return false
251249
}
252250

253-
func checkPolicySelectorsAreEmpty(podSelector metav1.LabelSelector) bool {
254-
return len(podSelector.MatchLabels) == 0 && len(podSelector.MatchExpressions) == 0
255-
}
251+
func checkPolicyMatchServiceLabels(serviceLabels map[string]string, podSelector metav1.LabelSelector) bool {
252+
// Check if there is an allow all ingress policy with empty selectors if so the service is safe
253+
if len(podSelector.MatchLabels) == 0 && len(podSelector.MatchExpressions) == 0 {
254+
return true
255+
}
256256

257-
func checkPolicyMatchServiceLabels(serviceLabels, policyLabels map[string]string) bool {
258-
// Return false if the policy has more labels than the service
259-
if len(policyLabels) > len(serviceLabels) {
257+
// Return false if the policy has no labels or more labels than the service
258+
if len(podSelector.MatchLabels) == 0 || len(podSelector.MatchLabels) > len(serviceLabels) {
260259
return false
261260
}
262261

263262
// Check for each policy label that that label is present in the service labels
264263
// Note: does not check matchExpressions
265-
for policyKey, policyValue := range policyLabels {
264+
for policyKey, policyValue := range podSelector.MatchLabels {
266265
matchedPolicyLabelToServiceLabel := false
267266
for serviceKey, serviceValue := range serviceLabels {
268267
if policyKey == serviceKey && policyValue == serviceValue {

‎tools/azure-npm-to-cilium-validator/azure-npm-to-cilium-validator_test.go‎

Lines changed: 130 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1353,6 +1353,44 @@ func TestGetExternalTrafficPolicyClusterServices(t *testing.T) {
13531353
expectedUnsafeRiskServices: []string{"namespace1/service-with-selector"},
13541354
expectedUnsafeNoSelectorServices: []string{},
13551355
},
1356+
{
1357+
name: "LoadBalancer service with externalTrafficPolicy=Cluster with a selector and an allow all ingress policy with a selector that has more labels",
1358+
namespaces: &corev1.NamespaceList{
1359+
Items: []corev1.Namespace{
1360+
{ObjectMeta: metav1.ObjectMeta{Name: "namespace1"}},
1361+
},
1362+
},
1363+
servicesByNamespace: map[string][]*corev1.Service{
1364+
"namespace1": {
1365+
{
1366+
ObjectMeta: metav1.ObjectMeta{Name: "service-with-selector"},
1367+
Spec: corev1.ServiceSpec{
1368+
Type: corev1.ServiceTypeLoadBalancer,
1369+
Selector: map[string]string{"app": "test"},
1370+
ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyTypeCluster,
1371+
},
1372+
},
1373+
},
1374+
},
1375+
policiesByNamespace: map[string][]*networkingv1.NetworkPolicy{
1376+
"namespace1": {
1377+
{
1378+
ObjectMeta: metav1.ObjectMeta{Name: "allow-all-ingress-policy-with-selector"},
1379+
Spec: networkingv1.NetworkPolicySpec{
1380+
PodSelector: metav1.LabelSelector{
1381+
MatchLabels: map[string]string{"app": "test", "app2": "test2", "app3": "test3"},
1382+
},
1383+
PolicyTypes: []networkingv1.PolicyType{"Ingress"},
1384+
Ingress: []networkingv1.NetworkPolicyIngressRule{
1385+
{},
1386+
},
1387+
},
1388+
},
1389+
},
1390+
},
1391+
expectedUnsafeRiskServices: []string{"namespace1/service-with-selector"},
1392+
expectedUnsafeNoSelectorServices: []string{},
1393+
},
13561394
{
13571395
name: "LoadBalancer service with externalTrafficPolicy=Cluster with a selector and an allow all ingress policy with a matching selector but ports dont match",
13581396
namespaces: &corev1.NamespaceList{
@@ -1549,7 +1587,7 @@ func TestGetExternalTrafficPolicyClusterServices(t *testing.T) {
15491587
},
15501588
},
15511589
{
1552-
ObjectMeta: metav1.ObjectMeta{Name: "allow-all-ingress-policy-with-no-selector"},
1590+
ObjectMeta: metav1.ObjectMeta{Name: "allow-all-ingress-policy-with-a-selector"},
15531591
Spec: networkingv1.NetworkPolicySpec{
15541592
PodSelector: metav1.LabelSelector{
15551593
MatchLabels: map[string]string{"app": "test"},
@@ -1565,10 +1603,99 @@ func TestGetExternalTrafficPolicyClusterServices(t *testing.T) {
15651603
expectedUnsafeRiskServices: []string{},
15661604
expectedUnsafeNoSelectorServices: []string{},
15671605
},
1568-
// add ut where target port is nil
1606+
{
1607+
name: "LoadBalancer service with externalTrafficPolicy=Cluster with a selector and no ports and an allow all ingress policy with a matching selector and ports",
1608+
namespaces: &corev1.NamespaceList{
1609+
Items: []corev1.Namespace{
1610+
{ObjectMeta: metav1.ObjectMeta{Name: "namespace1"}},
1611+
},
1612+
},
1613+
servicesByNamespace: map[string][]*corev1.Service{
1614+
"namespace1": {
1615+
{
1616+
ObjectMeta: metav1.ObjectMeta{Name: "service-with-selector"},
1617+
Spec: corev1.ServiceSpec{
1618+
Type: corev1.ServiceTypeLoadBalancer,
1619+
Selector: map[string]string{"app": "test"},
1620+
ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyTypeCluster,
1621+
},
1622+
},
1623+
},
1624+
},
1625+
policiesByNamespace: map[string][]*networkingv1.NetworkPolicy{
1626+
"namespace1": {
1627+
{
1628+
ObjectMeta: metav1.ObjectMeta{Name: "allow-all-ingress-policy-with-selector-and-ports"},
1629+
Spec: networkingv1.NetworkPolicySpec{
1630+
PodSelector: metav1.LabelSelector{
1631+
MatchLabels: map[string]string{"app": "test"},
1632+
},
1633+
PolicyTypes: []networkingv1.PolicyType{"Ingress"},
1634+
Ingress: []networkingv1.NetworkPolicyIngressRule{
1635+
{
1636+
Ports: []networkingv1.NetworkPolicyPort{
1637+
{
1638+
Port: intstrPtr(intstr.FromInt(80)),
1639+
Protocol: func() *corev1.Protocol {
1640+
protocol := corev1.ProtocolTCP
1641+
return &protocol
1642+
}(),
1643+
},
1644+
},
1645+
},
1646+
},
1647+
},
1648+
},
1649+
},
1650+
},
1651+
expectedUnsafeRiskServices: []string{"namespace1/service-with-selector"},
1652+
expectedUnsafeNoSelectorServices: []string{},
1653+
},
1654+
{
1655+
name: "LoadBalancer service with externalTrafficPolicy=Cluster with no selector and an allow all ingress policy with a matchExpressions selector",
1656+
namespaces: &corev1.NamespaceList{
1657+
Items: []corev1.Namespace{
1658+
{ObjectMeta: metav1.ObjectMeta{Name: "namespace1"}},
1659+
},
1660+
},
1661+
servicesByNamespace: map[string][]*corev1.Service{
1662+
"namespace1": {
1663+
{
1664+
ObjectMeta: metav1.ObjectMeta{Name: "service-with-no-selector"},
1665+
Spec: corev1.ServiceSpec{
1666+
Type: corev1.ServiceTypeLoadBalancer,
1667+
ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyTypeCluster,
1668+
},
1669+
},
1670+
},
1671+
},
1672+
policiesByNamespace: map[string][]*networkingv1.NetworkPolicy{
1673+
"namespace1": {
1674+
{
1675+
ObjectMeta: metav1.ObjectMeta{Name: "allow-all-ingress-policy-with-matchexpressins-selector"},
1676+
Spec: networkingv1.NetworkPolicySpec{
1677+
PodSelector: metav1.LabelSelector{
1678+
MatchExpressions: []metav1.LabelSelectorRequirement{
1679+
{
1680+
Key: "app",
1681+
Operator: metav1.LabelSelectorOpIn,
1682+
Values: []string{"test"},
1683+
},
1684+
},
1685+
},
1686+
PolicyTypes: []networkingv1.PolicyType{"Ingress"},
1687+
Ingress: []networkingv1.NetworkPolicyIngressRule{
1688+
{},
1689+
},
1690+
},
1691+
},
1692+
},
1693+
},
1694+
expectedUnsafeRiskServices: []string{"namespace1/service-with-no-selector"},
1695+
expectedUnsafeNoSelectorServices: []string{"namespace1/service-with-no-selector"},
1696+
},
15691697
// add a ut where target port matches to protocol
15701698
// add a ut where the port is 0
1571-
// add a ut testing for match expressions
15721699

15731700
// Scenarios where there are LoadBalancer or NodePort services with externalTrafficPolicy=Cluster and there are multiple policies
15741701

0 commit comments

Comments
 (0)