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

sawsa307 · 2023-02-23T00:12:43Z

Filter pods that don't have labels match to its service label selector.

k8s-ci-robot · 2023-02-23T00:12:52Z

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-23T01:12:49Z

/assign @swetharepakula

aojea · 2023-02-23T11:51:49Z

pkg/neg/syncers/transaction.go

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


you don't need to use the custom parsers, and is more complete if you iterate over all the PodCIDRs, you can add a helper for that

func nodeContainsPodIP(node *v1.Node, pod *v1.Pod) bool { if node == nil || pod == nil { return false } ipnets := []*net.IPNet{} for _, podCIDR := range node.Spec.PodCIDRs { podCIDR = strings.TrimSpace(podCIDR) _, ipnet, err := net.ParseCIDR(podCIDR) if err != nil { // swallow errors for CIDRs that are invalid continue } ipnets = append(ipnets, ipnet) } _, ipnet, err := net.ParseCIDR(node.Spec.PodCIDR) if err == nil { // swallow errors for CIDRs that are invalid ipnets = append(ipnets, ipnet) } podIP := net.ParseIP(pod.Status.PodIP) for _, net := range ipnets { if net.Contains(podIP) { return true } } return false }

Yes I saw your comment in the other PR #1963, and I was about to address it there, but thank you for writing this out!

since node.Spec.PodCIDR is guaranteed to be the zeroth entry in node.Spec.PodCIDRs, I think we can skip the check outside the loop

since node.Spec.PodCIDR is guaranteed to be the zeroth entry in node.Spec.PodCIDRs

good catch

aojea · 2023-02-23T11:53:26Z

pkg/neg/syncers/transaction.go

@@ -351,6 +352,21 @@ func (s *transactionSyncer) isValidPod(pod *apiv1.Pod) bool {
 	if !podCIDR.Contains(podIP) {
 		return false
 	}
+	podLabels := pod.ObjectMeta.Labels


hmm, how can we end in this situation?

Hi Antonio, could you elaborate on which situation you are referring to?
If you are talking about a service has a pod, and this pod's label doesn't match the label selector of the service, it is indeed a case that is unlikely to happen, but we are doing this check to make sure if in the future, anything goes wrong in other part of code, NEG controller is still able to make correct decisions.

fair enough, this should be like a panic kind of thing but we don't want to crash ...then just log it but I suggest to be verbose

log. "this can not happen, pod %s/%s labels %v doesn't match service %s/%s selector %v", pod.Namespace, pod.Name, pod.Labels, service.Namespace, service.Name, service.spec.Selector)

Definitely! I'll add logging to this. Thank you!

aojea · 2023-02-23T11:53:41Z

/ok-to-test

sawsa307 · 2023-03-14T15:53:26Z

/retest

sawsa307 · 2023-03-15T21:57:46Z

pkg/neg/syncers/utils.go

+		return false
+	}
+	// for custom endpoint slice, we won't check the pod's labels
+	if !isCustomEPS && !podBelongsToService(pod, service) {


refactor so continue if custom EPS

sawsa307 · 2023-04-17T23:59:29Z

/retest

pkg/neg/syncers/transaction.go

swetharepakula · 2023-04-27T20:40:38Z

pkg/neg/syncers/utils.go

@@ -387,6 +387,8 @@ func toZoneNetworkEndpointMapDegradedMode(eds []negtypes.EndpointsData, zoneGett
 		if len(matchPort) == 0 {
 			continue
 		}
+		serviceName := ed.Meta.Labels[discovery.LabelServiceName]
+		isCustomEPS := ed.Meta.Labels[discovery.LabelManagedBy] != "endpointslice-controller.k8s.io"


is there a constant we can use? otherwise define this as a constant

I check and the only place we define this const is in endpoint slice controller repo as an not exported const, so I create one in this file.

swetharepakula · 2023-04-27T20:44:34Z

pkg/neg/syncers/utils.go

+	serviceLabels := service.Spec.Selector
+	for key, val1 := range serviceLabels {
+		if val2, contains := podLabels[key]; !contains || val1 != val2 {
+			return fmt.Errorf("%w: pod %s has labels not match to its service %s's label selector", negtypes.ErrEPPodLabelMismatch, pod.Name, service.Name)


Perhaps this as the msg instead: "%w: pod %s/%s has labels that do not match the service %s/%s's label selector"

Include the namespace for both the pod and service

Updated. Thanks!

swetharepakula · 2023-04-27T20:47:25Z

pkg/neg/syncers/utils_test.go

@@ -2354,13 +2499,15 @@ func getTestEndpointSlices(name, namespace string) []*discovery.EndpointSlice {
 	port81 := int32(81)
 	port8081 := int32(8081)
 	protocolTCP := v1.ProtocolTCP
+	managedByController := "endpointslice-controller.k8s.io"


use a constant instead

Updated. Thanks!

pkg/neg/syncers/transaction.go

swetharepakula · 2023-04-27T21:24:03Z

pkg/neg/syncers/utils.go

+
+	// controllerName is a unique value used with LabelManagedBy to indicated
+	// the EndpointSlice is managed by the endpoint slice controller.
+	controllerName = "endpointslice-controller.k8s.io"


maybe managedByEPSControllerValue

Updated. Thanks!

swetharepakula · 2023-04-27T21:28:45Z

pkg/neg/syncers/utils.go

@@ -48,6 +48,10 @@ const (
 	minRetryDelay = 5 * time.Second
 	maxRetryDelay = 600 * time.Second
 	separator     = "||"
+
+	// managedByEPSControllerValue is a unique value used with LabelManagedBy to indicated


typo: to indicate

Filter pods that don't have labels match to its service label selector.

swetharepakula

/lgtm
/approve

k8s-ci-robot · 2023-04-27T21:31:18Z

[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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 23, 2023

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

k8s-ci-robot requested review from aojea and MrHohn February 23, 2023 00:12

k8s-ci-robot assigned swetharepakula Feb 23, 2023

aojea reviewed Feb 23, 2023

View reviewed changes

sawsa307 force-pushed the filter-pod-using-labels branch from 6bb7f21 to a4605aa Compare March 10, 2023 23:08

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 10, 2023

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-labels branch from a4605aa to e252bf1 Compare March 14, 2023 15:26

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 14, 2023

sawsa307 commented Mar 15, 2023

View reviewed changes

sawsa307 force-pushed the filter-pod-using-labels branch from e252bf1 to 4d1964a Compare March 15, 2023 23:20

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 Mar 15, 2023

sawsa307 force-pushed the filter-pod-using-labels branch from 4d1964a to 903ba31 Compare March 15, 2023 23:48

sawsa307 force-pushed the filter-pod-using-labels branch from 97c1776 to b96225c Compare April 17, 2023 22:05

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 17, 2023

sawsa307 force-pushed the filter-pod-using-labels branch from b96225c to b473908 Compare April 17, 2023 23:52

sawsa307 force-pushed the filter-pod-using-labels branch 3 times, most recently from 646798b to 3faf446 Compare April 18, 2023 18:05

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

sawsa307 force-pushed the filter-pod-using-labels branch from 3faf446 to 08cba5e Compare April 22, 2023 00:00

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

sawsa307 force-pushed the filter-pod-using-labels branch 2 times, most recently from daf13f4 to f559eb4 Compare April 27, 2023 03:09

swetharepakula reviewed Apr 27, 2023

View reviewed changes

sawsa307 force-pushed the filter-pod-using-labels branch from f559eb4 to 4ae2ec2 Compare April 27, 2023 21:07

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 27, 2023

sawsa307 force-pushed the filter-pod-using-labels branch from 4ae2ec2 to 1e085ca Compare April 27, 2023 21:14

sawsa307 requested a review from swetharepakula April 27, 2023 21:16

swetharepakula reviewed Apr 27, 2023

View reviewed changes

sawsa307 force-pushed the filter-pod-using-labels branch from 1e085ca to 1228926 Compare April 27, 2023 21:27

swetharepakula reviewed Apr 27, 2023

View reviewed changes

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

9c75049

Filter pods that don't have labels match to its service label selector.

sawsa307 force-pushed the filter-pod-using-labels branch from 1228926 to 9c75049 Compare April 27, 2023 21:30

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 27, 2023

k8s-ci-robot merged commit 136d422 into kubernetes:master Apr 27, 2023

sawsa307 deleted the filter-pod-using-labels branch September 2, 2023 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sawsa307 commented Feb 23, 2023

k8s-ci-robot commented Feb 23, 2023

sawsa307 commented Feb 23, 2023

aojea Feb 23, 2023

sawsa307 Feb 23, 2023

sawsa307 Mar 10, 2023

aojea Mar 10, 2023

aojea Feb 23, 2023

sawsa307 Mar 10, 2023 •

edited

Loading

aojea Mar 10, 2023

sawsa307 Mar 10, 2023

aojea commented Feb 23, 2023

sawsa307 commented Mar 14, 2023

sawsa307 Mar 15, 2023 •

edited

Loading

sawsa307 commented Apr 17, 2023

swetharepakula Apr 27, 2023

sawsa307 Apr 27, 2023

swetharepakula Apr 27, 2023

sawsa307 Apr 27, 2023

swetharepakula Apr 27, 2023

sawsa307 Apr 27, 2023

swetharepakula Apr 27, 2023

sawsa307 Apr 27, 2023

swetharepakula Apr 27, 2023

sawsa307 Apr 27, 2023

swetharepakula left a comment

k8s-ci-robot commented Apr 27, 2023

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

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

Conversation

sawsa307 commented Feb 23, 2023

k8s-ci-robot commented Feb 23, 2023

sawsa307 commented Feb 23, 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

sawsa307 Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Feb 23, 2023

sawsa307 commented Mar 14, 2023

sawsa307 Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

sawsa307 commented Apr 17, 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

Choose a reason for hiding this comment

swetharepakula left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 27, 2023

sawsa307 Mar 10, 2023 •

edited

Loading

sawsa307 Mar 15, 2023 •

edited

Loading