Filter pods that have out of range IP #1963

sawsa307 · 2023-02-21T21:04:00Z

Filter pods have IPs outside of the corresponding nodes' IP ranges.

k8s-ci-robot · 2023-02-21T21:04:10Z

Hi @sawsa307. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sawsa307 · 2023-02-21T21:33:14Z

/assign @swetharepakula

aojea · 2023-02-21T23:37:45Z

pkg/neg/syncers/transaction.go

+	if !isNode {
+		return false
+	}
+	_, podCIDR, err := netset.ParseCIDRSloppy(node.Spec.PodCIDR)


you can omit the custom parsers here and use the std library ones, net.ParseCIDR()

Hi Antonio, i found in the comment section for ParseCIDRSloppy, it says We're choosing to keep it for compat with potential stored values., where the values refer to CIDR with leading '0' characters on numbers. Is there a specific reason that we can be sure that there is no need to consider CIDR like this?

yeah, that is a good question, we had to do it in kubernetes to keep compatibility, imagine someone created a service with the ip 10.001.1.1, if he upgrades that Service will be invalid, and we don't want to break it.

For components that use the kubernetes API they have more freedom to make their choice and say, since version X if we found that IP we are going to reject it, so please update your manifests.

I prefer for the ecosystem to "force" this evolution and not keep the old parsers, it should be a problem only for Kubernetes/kubernetes not for the others.

Got it. That makes sense. Thank you!

aojea · 2023-02-21T23:40:39Z

pkg/neg/syncers/transaction.go

+		return false
+	}
+	_, podCIDR, err := netset.ParseCIDRSloppy(node.Spec.PodCIDR)
+	podIP := net.ParseIP(pod.Status.PodIP)


the CNI not necessarily have to use node.Spec.PodCIDR , that means that this relation may not be always true, also, if the CNI is supposed to use the CIDR this double check should not be necessary, because this can only happen if the CNI is wrong.

In addition, there is a plan to support multiple PodCIDRs, so node.Spec.PodCIDRs will contain a slice of networks.

The checks is related to the problem of preemptible node with the buggy 1.22 node eviction logic. We are verify this because it might cause the NEG not to be sync completely. We are operation it on the assumption that there might be a bug somewhere else.
Currently since we are only supporting single network ipv4, we only do check for ipv4. As we add support for the other feature, this will be updated.
@swetharepakula

then it is better if you iterate over the node.Spec.PodCIDRs list too , and returns true if there is a match, and false if there is no match

Sounds good. I'll change it. Thank you!

I commented in the other PR you sent with a sketch of a solution

aojea · 2023-02-22T23:43:16Z

/ok-to-test

pkg/neg/syncers/utils_test.go

swetharepakula

The pod IP validation needs to consider a few more edge cases

swetharepakula · 2023-04-21T21:42:58Z

pkg/neg/syncers/utils.go

@@ -527,10 +535,13 @@ func validatePod(pod *apiv1.Pod, nodeLister cache.Indexer) error {
 	if err != nil || !exists {
 		return negtypes.ErrEPNodeNotFound
 	}
-	_, isNode := obj.(*apiv1.Node)
+	node, isNode := obj.(*apiv1.Node)


nit: by convention we use ok instead of isNode

swetharepakula · 2023-04-21T22:00:11Z

pkg/neg/syncers/utils.go

+// If dual stack is enabled, it would return error if either ipv4 or ipv6 address does not match to the pod IPs
+func podContainsEndpointAddress(networkEndpoint negtypes.NetworkEndpoint, pod *apiv1.Pod, enableDualStackNEG bool) error {
+	endpointIPs := []string{networkEndpoint.IP}
+	if enableDualStackNEG {


check that there is a dual stack endpoint. It is possible dual stack is enabled but this service is not a dual stack service.

swetharepakula · 2023-04-21T22:04:29Z

pkg/neg/syncers/utils.go

+		for _, podIP := range pod.Status.PodIPs {
+			podIPs = append(podIPs, net.ParseIP(podIP.IP))
+		}


since the pod ips have already been verified, use the network endpoint we formed instead. This might be a dual stack cluster, but it might not be a dual stack service so both endpoints may not need to be verified.

swetharepakula

few small comments, otherwise this is ready to merge

pkg/neg/syncers/utils.go

pkg/neg/syncers/utils_test.go

Filter pods have IPs outside of the corresponding nodes' IP ranges.

sawsa307 · 2023-04-27T03:21:36Z

/retest

swetharepakula

/lgtm
/approve

k8s-ci-robot · 2023-04-27T20:24:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sawsa307, swetharepakula

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [swetharepakula]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 21, 2023

k8s-ci-robot requested review from aojea and kl52752 February 21, 2023 21:04

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 21, 2023

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 21, 2023

k8s-ci-robot assigned swetharepakula Feb 21, 2023

sawsa307 force-pushed the filter-pod-using-podcidr branch 2 times, most recently from fe88f4d to 963f048 Compare February 21, 2023 21:38

aojea reviewed Feb 21, 2023

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 22, 2023

sawsa307 mentioned this pull request Feb 23, 2023

Filter pods that don't belong to the service in question #1966

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2023

sawsa307 force-pushed the filter-pod-using-podcidr branch from 963f048 to 47edd1e Compare March 10, 2023 19:50

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 10, 2023

sawsa307 force-pushed the filter-pod-using-podcidr branch 2 times, most recently from 2dd2c7f to fc1e477 Compare March 10, 2023 22:46

sawsa307 mentioned this pull request Mar 11, 2023

Filter terminal pods in degraded mode #1958

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 14, 2023

sawsa307 force-pushed the filter-pod-using-podcidr branch from fc1e477 to ef87f23 Compare March 14, 2023 14:49

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 14, 2023

sawsa307 force-pushed the filter-pod-using-podcidr branch from ef87f23 to d7384ff Compare March 14, 2023 15:09

sawsa307 commented Mar 15, 2023

View reviewed changes

pkg/neg/syncers/utils_test.go Outdated Show resolved Hide resolved

sawsa307 force-pushed the filter-pod-using-podcidr branch 3 times, most recently from e0a1233 to b37cf85 Compare March 15, 2023 23:27

sawsa307 force-pushed the filter-pod-using-podcidr branch 5 times, most recently from b8c4612 to 25a99ce Compare April 18, 2023 03:06

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 18, 2023

swetharepakula reviewed Apr 21, 2023

View reviewed changes

sawsa307 force-pushed the filter-pod-using-podcidr branch from 25a99ce to 316a74d Compare April 21, 2023 23:41

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 21, 2023

sawsa307 force-pushed the filter-pod-using-podcidr branch from 316a74d to 68bccdc Compare April 22, 2023 00:11

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 22, 2023

sawsa307 force-pushed the filter-pod-using-podcidr branch 3 times, most recently from d202d16 to d87e0fd Compare April 26, 2023 22:00

include dual stack for degraded mode

5c389bf

sawsa307 force-pushed the filter-pod-using-podcidr branch from d87e0fd to 352c729 Compare April 27, 2023 00:07

swetharepakula reviewed Apr 27, 2023

View reviewed changes

pkg/neg/syncers/utils.go Show resolved Hide resolved

pkg/neg/syncers/utils_test.go Outdated Show resolved Hide resolved

sawsa307 force-pushed the filter-pod-using-podcidr branch from 352c729 to b0d6d5f Compare April 27, 2023 03:05

Filter pods that have out of range IP

e00401e

Filter pods have IPs outside of the corresponding nodes' IP ranges.

sawsa307 force-pushed the filter-pod-using-podcidr branch from b0d6d5f to e00401e Compare April 27, 2023 03:07

sawsa307 requested a review from swetharepakula April 27, 2023 18:03

swetharepakula reviewed Apr 27, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 27, 2023

k8s-ci-robot merged commit 743ff5b into kubernetes:master Apr 27, 2023
5 checks passed

sawsa307 deleted the filter-pod-using-podcidr branch September 2, 2023 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter pods that have out of range IP #1963

Filter pods that have out of range IP #1963

sawsa307 commented Feb 21, 2023

k8s-ci-robot commented Feb 21, 2023

sawsa307 commented Feb 21, 2023

aojea Feb 21, 2023

sawsa307 Mar 10, 2023

aojea Mar 10, 2023

sawsa307 Mar 10, 2023

aojea Feb 21, 2023

sawsa307 Feb 22, 2023

aojea Feb 22, 2023

sawsa307 Feb 23, 2023

aojea Feb 23, 2023

aojea commented Feb 22, 2023

swetharepakula left a comment

swetharepakula Apr 21, 2023

swetharepakula Apr 21, 2023

swetharepakula Apr 21, 2023

swetharepakula left a comment

sawsa307 commented Apr 27, 2023

swetharepakula left a comment

k8s-ci-robot commented Apr 27, 2023

Filter pods that have out of range IP #1963

Filter pods that have out of range IP #1963

Conversation

sawsa307 commented Feb 21, 2023

k8s-ci-robot commented Feb 21, 2023

sawsa307 commented Feb 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Feb 22, 2023

swetharepakula left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swetharepakula left a comment

Choose a reason for hiding this comment

sawsa307 commented Apr 27, 2023

swetharepakula left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 27, 2023