Skip to content

Commit f3d022d

Browse files
authored
fix a duplicated ingress port issue (#69)
<!-- 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?** <!-- Add one of the following: bug cleanup documentation feature --> bug **Which issue does this PR fix**: This PR is fixing the duplicated ingress ports issue when scale dst pods. **What does this PR do / Why do we need it**: The bug can cause duplicated ports breaching the limit and fails ports beyond the limit. **If an issue # is not available please add steps to reproduce and the controller logs**: 1, create two deployments (simple nginx) with replicas = 1 2, create NPs for one deployment pods with ingress from the other deployment pods 3, scale the dst pods to a larger number 4, print out the policyendpoint and check if the ingress has duplicated ports with a same cidr in one entry **Testing done on this change**: <!-- output of manual testing/integration tests results and also attach logs showing the fix being resolved --> Before this fix ``` ingress: - cidr: 192.168.163.178 ports: - port: 80 protocol: TCP - port: 80 protocol: TCP - port: 80 protocol: TCP podIsolation: - Ingress - Egress ``` after this fix ``` ingress: - cidr: 192.168.51.76 ports: - port: 80 protocol: TCP podIsolation: - Ingress - Egress ``` with more than one port ``` ingress: - cidr: 192.168.7.80 ports: - port: 80 protocol: TCP - port: 8080 protocol: TCP podIsolation: - Ingress - Egress ``` Updated Unit Tests Before the fix ``` Error Trace: /local/home/zhuhz/go/src/amazon-network-policy-controller-k8s/pkg/resolvers/endpoints_test.go:872 Error: Not equal: expected: 1 actual : 2 Test: TestEndpointsResolver_ResolveNetworkPeers ``` After the fix ``` === RUN TestEndpointsResolver_ResolveNetworkPeers --- PASS: TestEndpointsResolver_ResolveNetworkPeers (0.00s) PASS ok github.com/aws/amazon-network-policy-controller-k8s/pkg/resolvers (cached) ``` **Automation added to e2e**: <!-- List the e2e tests you added as part of this PR. If no, create an issue with enhancement/testing label --> **Will this PR introduce any new dependencies?**: <!-- e.g. new K8s API --> **Will this break upgrades or downgrades. Has updating a running cluster been tested?**: **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". --> ```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 0499f4c + 804eaea commit f3d022d

File tree

2 files changed

+62
-8
lines changed

2 files changed

+62
-8
lines changed

pkg/resolvers/endpoints.go

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,19 @@ package resolvers
22

33
import (
44
"context"
5+
"fmt"
6+
"strconv"
57

68
policyinfo "github.com/aws/amazon-network-policy-controller-k8s/api/v1alpha1"
79
"github.com/aws/amazon-network-policy-controller-k8s/pkg/k8s"
810
"github.com/go-logr/logr"
911
"github.com/pkg/errors"
12+
"golang.org/x/exp/maps"
1013
corev1 "k8s.io/api/core/v1"
1114
networking "k8s.io/api/networking/v1"
1215
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1316
"k8s.io/apimachinery/pkg/labels"
17+
"k8s.io/apimachinery/pkg/types"
1418
"k8s.io/apimachinery/pkg/util/intstr"
1519
"sigs.k8s.io/controller-runtime/pkg/client"
1620
)
@@ -216,10 +220,14 @@ func (r *defaultEndpointsResolver) getIngressRulesPorts(ctx context.Context, pol
216220
var portList []policyinfo.Port
217221
for _, pod := range podList.Items {
218222
portList = append(portList, r.getPortList(pod, ports)...)
219-
r.logger.Info("Got ingress port", "port", portList, "pod", pod)
223+
r.logger.Info("Got ingress port from pod", "pod", types.NamespacedName{Namespace: pod.Namespace, Name: pod.Name}.String())
220224
}
221225

222-
return portList
226+
// since we pull ports from dst pods, we should deduplicate them
227+
dedupedPorts := dedupPorts(portList)
228+
r.logger.Info("Got ingress ports from dst pods", "port", dedupedPorts)
229+
230+
return dedupedPorts
223231
}
224232

225233
func (r *defaultEndpointsResolver) getPortList(pod corev1.Pod, ports []networking.NetworkPolicyPort) []policyinfo.Port {
@@ -455,3 +463,25 @@ func (r *defaultEndpointsResolver) getMatchingServicePort(ctx context.Context, s
455463
}
456464
return 0, errors.Errorf("unable to find matching service listen port %s for service %s", port.String(), k8s.NamespacedName(svc))
457465
}
466+
467+
func dedupPorts(policyPorts []policyinfo.Port) []policyinfo.Port {
468+
ports := make(map[string]policyinfo.Port)
469+
for _, port := range policyPorts {
470+
prot, p, ep := "", "", ""
471+
if port.Protocol != nil {
472+
prot = string(*port.Protocol)
473+
}
474+
if port.Port != nil {
475+
p = strconv.FormatInt(int64(*port.Port), 10)
476+
}
477+
if port.EndPort != nil {
478+
ep = strconv.FormatInt(int64(*port.EndPort), 10)
479+
}
480+
481+
ports[fmt.Sprintf("%s@%s@%s", prot, p, ep)] = port
482+
}
483+
if len(ports) > 0 {
484+
return maps.Values(ports)
485+
}
486+
return nil
487+
}

pkg/resolvers/endpoints_test.go

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -668,7 +668,7 @@ func TestEndpointsResolver_ResolveNetworkPeers(t *testing.T) {
668668
PodIP: "1.0.0.1",
669669
},
670670
}
671-
dstPod := corev1.Pod{
671+
dstPodOne := corev1.Pod{
672672
ObjectMeta: metav1.ObjectMeta{
673673
Name: "pod2",
674674
Namespace: "dst",
@@ -691,6 +691,29 @@ func TestEndpointsResolver_ResolveNetworkPeers(t *testing.T) {
691691
PodIP: "1.0.0.2",
692692
},
693693
}
694+
dstPodTwo := corev1.Pod{
695+
ObjectMeta: metav1.ObjectMeta{
696+
Name: "pod3",
697+
Namespace: "dst",
698+
},
699+
Spec: corev1.PodSpec{
700+
Containers: []corev1.Container{
701+
{
702+
Name: "pod3",
703+
Ports: []corev1.ContainerPort{
704+
{
705+
ContainerPort: port8080,
706+
Protocol: corev1.ProtocolTCP,
707+
Name: "test-port",
708+
},
709+
},
710+
},
711+
},
712+
},
713+
Status: corev1.PodStatus{
714+
PodIP: "1.0.0.3",
715+
},
716+
}
694717

695718
policy := &networking.NetworkPolicy{
696719
ObjectMeta: metav1.ObjectMeta{
@@ -775,7 +798,7 @@ func TestEndpointsResolver_ResolveNetworkPeers(t *testing.T) {
775798
// getting ingress endpoint calls listing pods with dst NS first
776799
mockClient.EXPECT().List(gomock.Any(), podList, gomock.Any()).DoAndReturn(
777800
func(ctx context.Context, podList *corev1.PodList, opts ...client.ListOption) error {
778-
podList.Items = []corev1.Pod{dstPod}
801+
podList.Items = []corev1.Pod{dstPodOne, dstPodTwo}
779802
return nil
780803
},
781804
),
@@ -811,7 +834,7 @@ func TestEndpointsResolver_ResolveNetworkPeers(t *testing.T) {
811834
),
812835
mockClient.EXPECT().List(gomock.Any(), podList, gomock.Any()).DoAndReturn(
813836
func(ctx context.Context, podList *corev1.PodList, opts ...client.ListOption) error {
814-
podList.Items = []corev1.Pod{dstPod}
837+
podList.Items = []corev1.Pod{dstPodOne, dstPodTwo}
815838
return nil
816839
},
817840
),
@@ -845,12 +868,13 @@ func TestEndpointsResolver_ResolveNetworkPeers(t *testing.T) {
845868

846869
for _, ingPE := range ingressEndpoints {
847870
assert.Equal(t, srcPod.Status.PodIP, string(ingPE.CIDR))
848-
assert.Equal(t, dstPod.Spec.Containers[0].Ports[0].ContainerPort, *ingPE.Ports[0].Port)
871+
assert.Equal(t, dstPodOne.Spec.Containers[0].Ports[0].ContainerPort, *ingPE.Ports[0].Port)
872+
assert.Equal(t, 1, len(ingPE.Ports))
849873
}
850874

851875
for _, egPE := range egressEndpoints {
852-
assert.Equal(t, dstPod.Status.PodIP, string(egPE.CIDR))
853-
assert.Equal(t, dstPod.Spec.Containers[0].Ports[0].ContainerPort, *egPE.Ports[0].Port)
876+
assert.True(t, string(egPE.CIDR) == dstPodOne.Status.PodIP || string(egPE.CIDR) == dstPodTwo.Status.PodIP)
877+
assert.Equal(t, dstPodOne.Spec.Containers[0].Ports[0].ContainerPort, *egPE.Ports[0].Port)
854878
assert.Equal(t, policy.Spec.Egress[0].Ports[0].Port.IntVal, *egPE.Ports[0].Port)
855879
assert.Equal(t, *policy.Spec.Egress[0].Ports[0].EndPort, *egPE.Ports[0].EndPort)
856880
}

0 commit comments

Comments
 (0)