Skip to content

Commit

Permalink
Filter pods that don't belong to the service in question
Browse files Browse the repository at this point in the history 
Filter pods that don't have labels match to its service label selector.
  • Loading branch information
@sawsa307
sawsa307 committed Feb 23, 2023
1 parent 963f048 commit 6bb7f21
Show file tree
Hide file tree
Showing 2 changed files with 112 additions and 10 deletions.
18 changes: 17 additions & 1 deletion pkg/neg/syncers/transaction.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import (
"k8s.io/ingress-gce/pkg/neg/readiness"
negtypes "k8s.io/ingress-gce/pkg/neg/types"
svcnegclient "k8s.io/ingress-gce/pkg/svcneg/client/clientset/versioned"
"k8s.io/ingress-gce/pkg/utils"
"k8s.io/ingress-gce/pkg/utils/patch"
"k8s.io/klog/v2"
netset "k8s.io/utils/net"
Expand Down Expand Up @@ -333,7 +334,7 @@ func (s *transactionSyncer) syncInternalImpl() error {
// 2. is in terminal state
// 3. corresponds to a non-existent node
// 4. have an IP that is outside of the node's allocated IP range
func (s *transactionSyncer) isValidPod(pod *apiv1.Pod) bool {
func (s *transactionSyncer) isValidPod(pod *apiv1.Pod, serviceName string) bool {
// Terminal Pod means a pod is in PodFailed or PodSucceeded phase
if pod.Status.Phase == corev1.PodFailed || pod.Status.Phase == corev1.PodSucceeded {
return false
Expand All @@ -351,6 +352,21 @@ func (s *transactionSyncer) isValidPod(pod *apiv1.Pod) bool {
if !podCIDR.Contains(podIP) {
return false
}
podLabels := pod.ObjectMeta.Labels
obj, exists, err = s.serviceLister.GetByKey(utils.ServiceKeyFunc(pod.ObjectMeta.Namespace, serviceName))
if err != nil || !exists {
return false
}
service, isService := obj.(*apiv1.Service)
if !isService {
return false
}
serviceLabels := service.Spec.Selector
for key, val1 := range serviceLabels {
if val2, contains := podLabels[key]; !contains || val1 != val2 {
return false
}
}
return true
}

Expand Down
104 changes: 95 additions & 9 deletions pkg/neg/syncers/transaction_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2357,6 +2357,14 @@ func TestIsValidPod(t *testing.T) {
podCIDR := "192.0.2.0/24"
validIP := "192.0.2.1"
invalidIP := "192.0.3.1"
testLabels1 := map[string]string{
"run": "foo",
}
testLabels2 := map[string]string{
"run": "bar",
}
testServiceNameNotFound := "foo"

ts.nodeLister.Add(&corev1.Node{
ObjectMeta: metav1.ObjectMeta{
Name: testNode1,
Expand All @@ -2366,17 +2374,29 @@ func TestIsValidPod(t *testing.T) {
},
})

ts.serviceLister.Add(&corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: testService,
},
Spec: corev1.ServiceSpec{
Selector: testLabels1,
},
})

testCases := []struct {
desc string
pod *corev1.Pod
expect bool
desc string
pod *corev1.Pod
serviceName string
expect bool
}{
{
desc: "valid pod",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: "pod1",
Labels: testLabels1,
},
Status: corev1.PodStatus{
Phase: corev1.PodRunning,
Expand All @@ -2386,14 +2406,16 @@ func TestIsValidPod(t *testing.T) {
NodeName: testNode1,
},
},
expect: true,
serviceName: testService,
expect: true,
},
{
desc: "terminal pod with phase failed",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: "pod2",
Labels: testLabels1,
},
Status: corev1.PodStatus{
Phase: corev1.PodFailed,
Expand All @@ -2403,14 +2425,16 @@ func TestIsValidPod(t *testing.T) {
NodeName: testNode1,
},
},
expect: false,
serviceName: testService,
expect: false,
},
{
desc: "terminal pod with phase succeeded",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: "pod3",
Labels: testLabels1,
},
Status: corev1.PodStatus{
Phase: corev1.PodSucceeded,
Expand All @@ -2420,14 +2444,16 @@ func TestIsValidPod(t *testing.T) {
NodeName: testNode1,
},
},
expect: false,
serviceName: testService,
expect: false,
},
{
desc: "pod from non-existent pod",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: "pod4",
Labels: testLabels1,
},
Status: corev1.PodStatus{
Phase: corev1.PodSucceeded,
Expand All @@ -2437,14 +2463,16 @@ func TestIsValidPod(t *testing.T) {
NodeName: testNodeNonExistent,
},
},
expect: false,
serviceName: testService,
expect: false,
},
{
desc: "pod with IP out of node's PodCIDR range",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: "pod5",
Labels: testLabels1,
},
Status: corev1.PodStatus{
Phase: corev1.PodRunning,
Expand All @@ -2454,12 +2482,70 @@ func TestIsValidPod(t *testing.T) {
NodeName: testNode1,
},
},
expect: false,
serviceName: testService,
expect: false,
},
{
desc: "pod with IP out of node's PodCIDR range",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: "pod5",
Labels: testLabels1,
},
Status: corev1.PodStatus{
Phase: corev1.PodRunning,
PodIP: invalidIP,
},
Spec: corev1.PodSpec{
NodeName: testNode1,
},
},
serviceName: testService,
expect: false,
},
{
desc: "pod with non existing service name",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: "pod6",
Labels: testLabels1,
},
Status: corev1.PodStatus{
Phase: corev1.PodRunning,
PodIP: invalidIP,
},
Spec: corev1.PodSpec{
NodeName: testNode1,
},
},
serviceName: testServiceNameNotFound,
expect: false,
},
{
desc: "pod with labels not matching to service label selector",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: "pod7",
Labels: testLabels2,
},
Status: corev1.PodStatus{
Phase: corev1.PodRunning,
PodIP: validIP,
},
Spec: corev1.PodSpec{
NodeName: testNode1,
},
},
serviceName: testService,
expect: false,
},
}
for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
if got := ts.isValidPod(tc.pod); got != tc.expect {
if got := ts.isValidPod(tc.pod, tc.serviceName); got != tc.expect {
t.Errorf("isValidPod() = %t, expected %t", got, tc.expect)
}
})
Expand Down

0 comments on commit 6bb7f21

Please sign in to comment.