fix ingress dest ports issue #58
Conversation
|
lgtm. Maybe, let's add an UT case for this scenario here - https://github.com/aws/amazon-network-policy-controller-k8s/blob/main/pkg/resolvers/endpoints_test.go#L185 |
Added unit tests and make some minor changes to the endpoints.go file as well. |
| r.logger.V(1).Info("Namespaces for network peers resolution", "list", namespaces, "policy", k8s.NamespacedName(policy)) | ||
|
|
||
| var portsToApply []policyinfo.Port | ||
| // populate the policy applied targets' ports |
There was a problem hiding this comment.
Nit: Please add few comments as to why this is done only for ingress...
| for _, ns := range namespaces { | ||
| networkPeers = append(networkPeers, r.getMatchingPodAddresses(ctx, peer.PodSelector, ns, ports)...) | ||
| networkPeers = append(networkPeers, r.getMatchingPodAddresses(ctx, peer.PodSelector, ns, portsToApply, ports, policyType)...) |
There was a problem hiding this comment.
If portsToApply is nil say for instance we hit the "Unable to List Pods" then why do even we need to go in the loop? Because eventually portList := r.getPortList(pod, ports) would be 0 and we just continue to the next pod isn't it? And we will loop thru all pods and just continue since ports are nil...Am I missing anything?
|
go vulns check failed on go version update. we will do a follow-up to update go version in action and also build script. |
What type of PR is this?
Fixing an issue for ingress port in policyendpoints
bug
Which issue does this PR fix:
#52
What does this PR do / Why do we need it:
The controller shouldn't use src port in PE's ingress endpoint.
If an issue # is not available please add steps to reproduce and the controller logs:
Testing done on this change:
Automation added to e2e:
Will this PR introduce any new dependencies?:
Will this break upgrades or downgrades. Has updating a running cluster been tested?:
Does this PR introduce any user-facing change?:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.