CHAOSPLT-248: Respect annotation filters (#835)

* retry with logs

* try a fix

* run an e2e test surprise

* Put logger in the suite_test file

* remove the dontexpects

* review feedback

Author: ptnapoleon

controllers/disruption_controller_test.go

@@ -95,8 +95,23 @@ var _ = Describe("Disruption Controller", func() {
 	Context("annotation filters should limit selected targets", func() {
 		BeforeEach(func() {
 			disruption.Spec.Filter = &chaosv1beta1.DisruptionFilter{
-				Annotations: targetPod.Annotations,
+				Annotations: map[string]string{"foo": "baz"},
 			}
+			disruption.Spec.Count = &intstr.IntOrString{Type: intstr.String, StrVal: "100%"}
+		})
+
+		It("should only select half of all pods", func(ctx SpecContext) {
+			ExpectChaosPods(ctx, disruption, 4)
+		})
+	})
+
+	Context("annotation filters should limit selected targets", func() {
+		BeforeEach(func() {
+			disruption.Spec.Filter = &chaosv1beta1.DisruptionFilter{
+				Annotations: map[string]string{"fob": "baf"},
+			}
+			disruption.Spec.Selector = map[string]string{"second": "true"}
+			disruption.Spec.Count = &intstr.IntOrString{Type: intstr.String, StrVal: "100%"}
 		})
 
 		It("should only select half of all pods", func(ctx SpecContext) {

controllers/suite_toolsfor_test.go

@@ -166,6 +166,7 @@ func InjectPodsAndDisruption(ctx SpecContext, wantDisruption chaosv1beta1.Disrup
 		anotherTargetPod = *targetPod.DeepCopy()
 		// we want some small differences from the previous pod, a single container, and other annotation value for foo
 		anotherTargetPod.Annotations["foo"] = "qux"
+		anotherTargetPod.Labels["second"] = "true"
 		anotherTargetPod.Spec.Containers = anotherTargetPod.Spec.Containers[0:1]
 		anotherTargetPod.Spec.Volumes = anotherTargetPod.Spec.Volumes[0:1] // second volume is used by second container, hence not needed as we removed it
 		anotherTargetPodCreated = CreateRunningPod(ctx, anotherTargetPod)
@@ -440,6 +441,7 @@ func uniquePod() corev1.Pod {
 			},
 			Annotations: map[string]string{
 				"foo": "baz",
+				"fob": "baf",
 				"uid": uuid,
 			},
 		},

main.go

@@ -141,7 +141,7 @@ func main() {
 	}
 
 	// target selector
-	targetSelector := targetselector.NewRunningTargetSelector(cfg.Controller.EnableSafeguards, controllerNodeName)
+	targetSelector := targetselector.NewRunningTargetSelector(cfg.Controller.EnableSafeguards, controllerNodeName, logger)
 
 	var gcPtr *time.Duration
 	if cfg.Controller.ExpiredDisruptionGCDelay >= 0 {

targetselector/running_target_selector.go

@@ -12,6 +12,7 @@ import (
 
 	chaosv1beta1 "github.com/DataDog/chaos-controller/api/v1beta1"
 	chaostypes "github.com/DataDog/chaos-controller/types"
+	"go.uber.org/zap"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/selection"
@@ -23,12 +24,14 @@ import (
 type runningTargetSelector struct {
 	controllerEnableSafeguards bool
 	controllerNodeName         string
+	logger                     *zap.SugaredLogger
 }
 
-func NewRunningTargetSelector(controllerEnableSafeguards bool, controllerNodeName string) TargetSelector {
+func NewRunningTargetSelector(controllerEnableSafeguards bool, controllerNodeName string, logger *zap.SugaredLogger) TargetSelector {
 	return runningTargetSelector{
 		controllerEnableSafeguards: controllerEnableSafeguards,
 		controllerNodeName:         controllerNodeName,
+		logger:                     logger,
 	}
 }
 
@@ -54,6 +57,7 @@ func (r runningTargetSelector) GetMatchingPodsOverTotalPods(c client.Client, ins
 
 	runningPods := &corev1.PodList{}
 
+podLoop:
 	for _, pod := range pods.Items {
 		// check the pod is already a disruption target
 		isAlreadyATarget := false
@@ -75,10 +79,12 @@ func (r runningTargetSelector) GetMatchingPodsOverTotalPods(c client.Client, ins
 		}
 
 		if instance.Spec.Filter != nil {
+			r.logger.Debugw("checking if pod has filtered annotations", "pod", pod.Name, "pod.Annotations", pod.Annotations, "spec.Filter", instance.Spec.Filter.Annotations)
 			for k, v := range instance.Spec.Filter.Annotations {
 				podAnno, ok := pod.Annotations[k]
 				if !ok || podAnno != v {
-					continue
+					// This pod doesn't have the annotation specified in our filter, we don't want to include it as a target
+					continue podLoop
 				}
 			}
 		}
@@ -133,6 +139,7 @@ func (r runningTargetSelector) GetMatchingNodesOverTotalNodes(c client.Client, i
 
 	runningNodes := &corev1.NodeList{}
 
+nodeLoop:
 	for _, node := range nodes.Items {
 		// apply controller safeguards if enabled
 		if r.controllerEnableSafeguards {
@@ -153,10 +160,12 @@ func (r runningTargetSelector) GetMatchingNodesOverTotalNodes(c client.Client, i
 		}
 
 		if instance.Spec.Filter != nil {
+			r.logger.Debugw("checking if node has filtered annotations", "node", node.Name, "node.Annotations", node.Annotations, "spec.Filter", instance.Spec.Filter.Annotations)
 			for k, v := range instance.Spec.Filter.Annotations {
 				nodeAnno, ok := node.Annotations[k]
 				if !ok || nodeAnno != v {
-					continue
+					// This node doesn't have the annotation specified in our filter, we don't want to include it as a target
+					continue nodeLoop
 				}
 			}
 		}

targetselector/running_target_selector_test.go

@@ -137,7 +137,7 @@ var _ = Describe("Helpers", func() {
 	var targetSelector TargetSelector
 
 	BeforeEach(func() {
-		targetSelector = NewRunningTargetSelector(false, "foo")
+		targetSelector = NewRunningTargetSelector(false, "foo", logger)
 
 		c = fakeClient{}
 
@@ -375,7 +375,7 @@ var _ = Describe("Helpers", func() {
 
 		Context("with controller safeguards enabled", func() {
 			BeforeEach(func() {
-				targetSelector = NewRunningTargetSelector(true, "runningNode")
+				targetSelector = NewRunningTargetSelector(true, "runningNode", logger)
 			})
 
 			It("should exclude the pods running on the same node as the controller from targets", func() {
@@ -440,7 +440,7 @@ var _ = Describe("Helpers", func() {
 
 		Context("with controller safeguards enabled", func() {
 			BeforeEach(func() {
-				targetSelector = NewRunningTargetSelector(true, "runningNode")
+				targetSelector = NewRunningTargetSelector(true, "runningNode", logger)
 			})
 
 			It("should exclude the controller node from targets", func() {

targetselector/targetselector_suite_test.go

@@ -3,16 +3,24 @@
 // This product includes software developed at Datadog (https://www.datadoghq.com/).
 // Copyright 2024 Datadog, Inc.
 
-package targetselector_test
+package targetselector
 
 import (
 	"testing"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"go.uber.org/zap"
+	"go.uber.org/zap/zaptest"
 )
 
+var logger *zap.SugaredLogger
+
 func TestTargetselector(t *testing.T) {
 	RegisterFailHandler(Fail)
 	RunSpecs(t, "Targetselector Suite")
 }
+
+var _ = BeforeSuite(func(ctx SpecContext) {
+	logger = zaptest.NewLogger(GinkgoT()).Sugar()
+})