Skip to content

Commit 14e4ff8

Browse files
authoredFeb 1, 2024
ingress doesn't need to check src ports (#72)
<!-- Thanks for sending a pull request! Here are some tips for you: 1. Ensure you have added the unit tests for your changes. 2. Ensure you have included output of manual testing done in the Testing section. 3. Ensure number of lines of code for new or existing methods are within the reasonable limit. 4. Ensure your change works on existing clusters after upgrade. --> **What type of PR is this?** fix a bug <!-- Add one of the following: bug cleanup documentation feature --> bug **Which issue does this PR fix**: #71 **What does this PR do / Why do we need it**: The controller shouldn't loop through src pods to check if their ports matching policies'. **If an issue # is not available please add steps to reproduce and the controller logs**: **Testing done on this change**: <!-- output of manual testing/integration tests results and also attach logs showing the fix being resolved --> Unit tests are updated and functional tests were done policy applied pod (dst) ``` ports: - containerPort: 80 name: http protocol: TCP - containerPort: 443 name: https protocol: TCP ``` src pod ``` ports: - name: src containerPort: 9999 protocol: TCP ``` svc ``` spec: ports: - port: 80 name: http targetPort: http - port: 443 name: https targetPort: https ``` policyendpoint ``` spec: ingress: - cidr: 192.168.xx.xxx ports: - port: 80 protocol: TCP - port: 443 protocol: TCP podIsolation: - Ingress ``` **Automation added to e2e**: <!-- List the e2e tests you added as part of this PR. If no, create an issue with enhancement/testing label --> no **Will this PR introduce any new dependencies?**: <!-- e.g. new K8s API --> no **Will this break upgrades or downgrades. Has updating a running cluster been tested?**: no **Does this PR introduce any user-facing change?**: <!-- If yes, a release note update is required: Enter your extended release note in the block below. If the PR requires additional actions from users switching to the new release, include the string "action required". --> no ```release-note ``` By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
2 parents f013674 + 31ff67f commit 14e4ff8

File tree

2 files changed

+48
-26
lines changed

2 files changed

+48
-26
lines changed
 

‎pkg/resolvers/endpoints.go

+26-20
Original file line numberDiff line numberDiff line change
@@ -191,15 +191,8 @@ func (r *defaultEndpointsResolver) resolveNetworkPeers(ctx context.Context, poli
191191
namespaces = []string{policy.Namespace}
192192
}
193193

194-
var portsToApply []policyinfo.Port
195-
// populate the policy applied targets' ports
196-
// only populate ports for Ingress and from network policy namespaces as destination ports
197-
if policyType == networking.PolicyTypeIngress {
198-
portsToApply = r.getIngressRulesPorts(ctx, policy.Namespace, &policy.Spec.PodSelector, ports)
199-
}
200-
201194
for _, ns := range namespaces {
202-
networkPeers = append(networkPeers, r.getMatchingPodAddresses(ctx, peer.PodSelector, ns, portsToApply, ports, policyType)...)
195+
networkPeers = append(networkPeers, r.getMatchingPodAddresses(ctx, peer.PodSelector, ns, policy, ports, policyType)...)
203196
}
204197

205198
}
@@ -317,9 +310,22 @@ func (r *defaultEndpointsResolver) resolveNamespaces(ctx context.Context, ls *me
317310
}
318311

319312
func (r *defaultEndpointsResolver) getMatchingPodAddresses(ctx context.Context, ls *metav1.LabelSelector, namespace string,
320-
policyPorts []policyinfo.Port, ports []networking.NetworkPolicyPort, rule networking.PolicyType) []policyinfo.EndpointInfo {
313+
policy *networking.NetworkPolicy, rulePorts []networking.NetworkPolicyPort, policyType networking.PolicyType) []policyinfo.EndpointInfo {
321314
var addresses []policyinfo.EndpointInfo
322315

316+
var portList []policyinfo.Port
317+
// populate the policy applied targets' ports
318+
// only populate ports for Ingress and from network policy namespaces as destination ports
319+
if policyType == networking.PolicyTypeIngress {
320+
portList = r.getIngressRulesPorts(ctx, policy.Namespace, &policy.Spec.PodSelector, rulePorts)
321+
if len(rulePorts) != len(portList) && len(portList) == 0 {
322+
r.logger.Info("Couldn't get matched port list from ingress of policy", "policy", types.NamespacedName{Name: policy.Name, Namespace: policy.Namespace}.String(),
323+
"ingressPorts", rulePorts, "derivedPorts", portList)
324+
return nil
325+
}
326+
}
327+
328+
// populate src pods for ingress and dst pods for egress
323329
podList := &corev1.PodList{}
324330
if err := r.k8sClient.List(ctx, podList, &client.ListOptions{
325331
LabelSelector: r.createPodLabelSelector(ls),
@@ -329,25 +335,25 @@ func (r *defaultEndpointsResolver) getMatchingPodAddresses(ctx context.Context,
329335
return nil
330336
}
331337
r.logger.V(1).Info("Got pods for label selector", "count", len(podList.Items), "selector", ls.String())
338+
332339
for _, pod := range podList.Items {
333340
podIP := k8s.GetPodIP(&pod)
334341
if len(podIP) == 0 {
335342
r.logger.Info("pod IP not assigned yet", "pod", k8s.NamespacedName(&pod))
336343
continue
337344
}
338-
portList := r.getPortList(pod, ports)
339-
if len(ports) != len(portList) && len(portList) == 0 {
340-
r.logger.Info("Couldn't get matched port list from the pod", "pod", k8s.NamespacedName(&pod), "expectedPorts", ports)
341-
continue
345+
346+
if policyType == networking.PolicyTypeEgress {
347+
portList = r.getPortList(pod, rulePorts)
348+
if len(rulePorts) != len(portList) && len(portList) == 0 {
349+
r.logger.Info("Couldn't get matched port list from the pod", "pod", k8s.NamespacedName(&pod), "expectedPorts", rulePorts)
350+
continue
351+
}
342352
}
353+
343354
addresses = append(addresses, policyinfo.EndpointInfo{
344-
CIDR: policyinfo.NetworkAddress(podIP),
345-
Ports: func(policyType networking.PolicyType) []policyinfo.Port {
346-
if policyType == networking.PolicyTypeIngress {
347-
return policyPorts
348-
}
349-
return portList
350-
}(rule),
355+
CIDR: policyinfo.NetworkAddress(podIP),
356+
Ports: portList,
351357
})
352358
}
353359

‎pkg/resolvers/endpoints_test.go

+22-6
Original file line numberDiff line numberDiff line change
@@ -658,7 +658,7 @@ func TestEndpointsResolver_ResolveNetworkPeers(t *testing.T) {
658658
{
659659
ContainerPort: port80,
660660
Protocol: corev1.ProtocolTCP,
661-
Name: "test-port",
661+
Name: "src-port",
662662
},
663663
},
664664
},
@@ -668,6 +668,7 @@ func TestEndpointsResolver_ResolveNetworkPeers(t *testing.T) {
668668
PodIP: "1.0.0.1",
669669
},
670670
}
671+
671672
dstPodOne := corev1.Pod{
672673
ObjectMeta: metav1.ObjectMeta{
673674
Name: "pod2",
@@ -681,7 +682,7 @@ func TestEndpointsResolver_ResolveNetworkPeers(t *testing.T) {
681682
{
682683
ContainerPort: port8080,
683684
Protocol: corev1.ProtocolTCP,
684-
Name: "test-port",
685+
Name: "dst-port",
685686
},
686687
},
687688
},
@@ -715,6 +716,12 @@ func TestEndpointsResolver_ResolveNetworkPeers(t *testing.T) {
715716
},
716717
}
717718

719+
portsMap := map[string]int32{
720+
"src-port": port80,
721+
"dst-port": port8080,
722+
}
723+
724+
// the policy is applied to dst namespace on dst pod
718725
policy := &networking.NetworkPolicy{
719726
ObjectMeta: metav1.ObjectMeta{
720727
Name: "netpol",
@@ -737,7 +744,7 @@ func TestEndpointsResolver_ResolveNetworkPeers(t *testing.T) {
737744
Ports: []networking.NetworkPolicyPort{
738745
{
739746
Protocol: &protocolTCP,
740-
Port: &intstr.IntOrString{Type: intstr.String, StrVal: "test-port"},
747+
Port: &intstr.IntOrString{Type: intstr.String, StrVal: "dst-port"},
741748
},
742749
},
743750
},
@@ -756,7 +763,7 @@ func TestEndpointsResolver_ResolveNetworkPeers(t *testing.T) {
756763
Ports: []networking.NetworkPolicyPort{
757764
{
758765
Protocol: &protocolTCP,
759-
Port: &intstr.IntOrString{Type: intstr.Int, IntVal: port8080},
766+
Port: &intstr.IntOrString{Type: intstr.String, StrVal: "src-port"},
760767
EndPort: &port9090,
761768
},
762769
},
@@ -798,6 +805,7 @@ func TestEndpointsResolver_ResolveNetworkPeers(t *testing.T) {
798805
// getting ingress endpoint calls listing pods with dst NS first
799806
mockClient.EXPECT().List(gomock.Any(), podList, gomock.Any()).DoAndReturn(
800807
func(ctx context.Context, podList *corev1.PodList, opts ...client.ListOption) error {
808+
podList.Items = []corev1.Pod{dstPodOne, dstPodTwo}
801809
podList.Items = []corev1.Pod{dstPodOne, dstPodTwo}
802810
return nil
803811
},
@@ -820,7 +828,7 @@ func TestEndpointsResolver_ResolveNetworkPeers(t *testing.T) {
820828

821829
dstNS := corev1.Namespace{
822830
ObjectMeta: metav1.ObjectMeta{
823-
Name: "dst",
831+
Name: "src",
824832
},
825833
}
826834

@@ -834,6 +842,7 @@ func TestEndpointsResolver_ResolveNetworkPeers(t *testing.T) {
834842
),
835843
mockClient.EXPECT().List(gomock.Any(), podList, gomock.Any()).DoAndReturn(
836844
func(ctx context.Context, podList *corev1.PodList, opts ...client.ListOption) error {
845+
podList.Items = []corev1.Pod{dstPodOne, dstPodTwo}
837846
podList.Items = []corev1.Pod{dstPodOne, dstPodTwo}
838847
return nil
839848
},
@@ -866,16 +875,23 @@ func TestEndpointsResolver_ResolveNetworkPeers(t *testing.T) {
866875
}
867876
}
868877

878+
// the policy is applied to dst namespace
879+
// the ingress should have cidr from src pod and ports from dst pod
880+
// the egress should have cidr from src pod and ports from src pod
869881
for _, ingPE := range ingressEndpoints {
870882
assert.Equal(t, srcPod.Status.PodIP, string(ingPE.CIDR))
871883
assert.Equal(t, dstPodOne.Spec.Containers[0].Ports[0].ContainerPort, *ingPE.Ports[0].Port)
872884
assert.Equal(t, 1, len(ingPE.Ports))
885+
assert.Equal(t, dstPodOne.Spec.Containers[0].Ports[0].ContainerPort, *ingPE.Ports[0].Port)
886+
assert.Equal(t, 1, len(ingPE.Ports))
873887
}
874888

875889
for _, egPE := range egressEndpoints {
876890
assert.True(t, string(egPE.CIDR) == dstPodOne.Status.PodIP || string(egPE.CIDR) == dstPodTwo.Status.PodIP)
877891
assert.Equal(t, dstPodOne.Spec.Containers[0].Ports[0].ContainerPort, *egPE.Ports[0].Port)
878-
assert.Equal(t, policy.Spec.Egress[0].Ports[0].Port.IntVal, *egPE.Ports[0].Port)
892+
assert.Equal(t, srcPod.Status.PodIP, string(egPE.CIDR))
893+
assert.Equal(t, srcPod.Spec.Containers[0].Ports[0].ContainerPort, *egPE.Ports[0].Port)
894+
assert.Equal(t, portsMap[policy.Spec.Egress[0].Ports[0].Port.StrVal], *egPE.Ports[0].Port)
879895
assert.Equal(t, *policy.Spec.Egress[0].Ports[0].EndPort, *egPE.Ports[0].EndPort)
880896
}
881897
}

0 commit comments

Comments
 (0)
Failed to load comments.