diff --git a/e2e/disruptors/pod_e2e_test.go b/e2e/disruptors/pod_e2e_test.go index 66f1682b..86d5a828 100644 --- a/e2e/disruptors/pod_e2e_test.go +++ b/e2e/disruptors/pod_e2e_test.go @@ -152,7 +152,7 @@ func Test_PodDisruptor(t *testing.T) { } // create pod disruptor that will select the service's pods - selector := disruptors.PodSelector{ + selector := disruptors.PodSelectorSpec{ Namespace: namespace, Select: disruptors.PodAttributes{ Labels: tc.service.Spec.Selector, @@ -242,7 +242,7 @@ func Test_PodDisruptor(t *testing.T) { } // create pod disruptor that will select the service's pods - selector := disruptors.PodSelector{ + selector := disruptors.PodSelectorSpec{ Namespace: namespace, Select: disruptors.PodAttributes{ Labels: service.Spec.Selector, @@ -299,7 +299,7 @@ func Test_PodDisruptor(t *testing.T) { } // create pod disruptor that will select all pods - selector := disruptors.PodSelector{ + selector := disruptors.PodSelectorSpec{ Namespace: namespace, } options := disruptors.PodDisruptorOptions{} diff --git a/pkg/api/api.go b/pkg/api/api.go index 69147bb1..2f42bfc5 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -232,7 +232,7 @@ func NewPodDisruptor( return nil, fmt.Errorf("PodDisruptor constructor expects a non null PodSelector argument") } - selector := disruptors.PodSelector{} + selector := disruptors.PodSelectorSpec{} err := convertValue(rt, c.Argument(0), &selector) if err != nil { return nil, fmt.Errorf("invalid PodSelector: %w", err) diff --git a/pkg/api/api_test.go b/pkg/api/api_test.go index f1c8bdf6..72caa174 100644 --- a/pkg/api/api_test.go +++ b/pkg/api/api_test.go @@ -49,8 +49,10 @@ func testSetup() (*testEnv, error) { return nil, err } - // Constructors for ServiceDisruptor and PodDisruptor will error if they cannot find any target for the supplied - // parameters. For this reason, we need to add to the fake k8s client a service and a pod backing it. + // Create a Service and a backing pod that match the targets in fault injection methods + // We are not testing the fault injection logic, only the arguments passed to the API but + // those methods would fail if they don't find a target pod. + // Note: the ServiceDisruptor constructor also will fail if the target service doesn't exist ns := &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{Name: "namespace"}, } @@ -71,7 +73,7 @@ func testSetup() (*testEnv, error) { WithIP("192.0.2.6"). Build() - // Constructors for ServiceDisruptor and PodDisruptor will also attempt to inject the disruptor agent into a target + // ServiceDisruptor and PodDisruptor will also attempt to inject the disruptor agent into a target // pod once it's discovered, and then wait for that container to be Running. Flagging this pod as ready is hard to // do with the k8s fake client, so we take advantage of the fact that both injection and check are skipped if the // agent container already exists by creating the fake pod with the sidecar already added. diff --git a/pkg/disruptors/pod.go b/pkg/disruptors/pod.go index 7d250fcd..fdd05c87 100644 --- a/pkg/disruptors/pod.go +++ b/pkg/disruptors/pod.go @@ -3,25 +3,14 @@ package disruptors import ( "context" - "errors" - "fmt" - "reflect" - "strings" "time" "github.com/grafana/xk6-disruptor/pkg/kubernetes" "github.com/grafana/xk6-disruptor/pkg/kubernetes/helpers" "github.com/grafana/xk6-disruptor/pkg/types/intstr" "github.com/grafana/xk6-disruptor/pkg/utils" - - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -// ErrSelectorNoPods is returned by NewPodDisruptor when the selector passed to it does not match any pod in the -// cluster. -var ErrSelectorNoPods = errors.New("no pods found matching selector") - // DefaultTargetPort defines the default value for a target HTTP var DefaultTargetPort = intstr.FromInt32(80) //nolint:gochecknoglobals @@ -41,13 +30,13 @@ type PodDisruptorOptions struct { // podDisruptor is an instance of a PodDisruptor that uses a PodController to interact with target pods type podDisruptor struct { - helper helpers.PodHelper - options PodDisruptorOptions - targets []corev1.Pod + helper helpers.PodHelper + selector *PodSelector + options PodDisruptorOptions } -// PodSelector defines the criteria for selecting a pod for disruption -type PodSelector struct { +// PodSelectorSpec defines the criteria for selecting a pod for disruption +type PodSelectorSpec struct { Namespace string // Select Pods that match these PodAttributes Select PodAttributes @@ -60,94 +49,38 @@ type PodAttributes struct { Labels map[string]string } -// NamespaceOrDefault returns the configured namespace for this selector, and the name of the default namespace if it -// is not configured. -func (p PodSelector) NamespaceOrDefault() string { - if p.Namespace != "" { - return p.Namespace - } - - return metav1.NamespaceDefault -} - -// String returns a human-readable explanation of the pods matched by a PodSelector. -func (p PodSelector) String() string { - var str string - - if len(p.Select.Labels) == 0 && len(p.Exclude.Labels) == 0 { - str = "all pods" - } else { - str = "pods " - str += p.groupLabels("including", p.Select.Labels) - str += p.groupLabels("excluding", p.Exclude.Labels) - str = strings.TrimSuffix(str, ", ") - } - - str += fmt.Sprintf(" in ns %q", p.NamespaceOrDefault()) - - return str -} - -// groupLabels returns a group of labels as a string, giving that group a name. The returned string has the form of: -// `groupName(foo=bar, boo=baz), `, including the trailing space and comma. -// An empty group of labels produces an empty string. -func (PodSelector) groupLabels(groupName string, labels map[string]string) string { - if len(labels) == 0 { - return "" - } - - group := groupName + "(" - for k, v := range labels { - group += fmt.Sprintf("%s=%s, ", k, v) - } - group = strings.TrimSuffix(group, ", ") - group += "), " - - return group -} - // NewPodDisruptor creates a new instance of a PodDisruptor that acts on the pods // that match the given PodSelector func NewPodDisruptor( - ctx context.Context, + _ context.Context, k8s kubernetes.Kubernetes, - selector PodSelector, + spec PodSelectorSpec, options PodDisruptorOptions, ) (PodDisruptor, error) { - // validate selector - emptySelect := reflect.DeepEqual(selector.Select, PodAttributes{}) - emptyExclude := reflect.DeepEqual(selector.Exclude, PodAttributes{}) - if selector.Namespace == "" && emptySelect && emptyExclude { - return nil, fmt.Errorf("namespace, select and exclude attributes in pod selector cannot all be empty") - } - // ensure selector and controller use default namespace if none specified - namespace := selector.NamespaceOrDefault() - helper := k8s.PodHelper(namespace) + namespace := spec.NamespaceOrDefault() - filter := helpers.PodFilter{ - Select: selector.Select.Labels, - Exclude: selector.Exclude.Labels, - } + helper := k8s.PodHelper(namespace) - targets, err := helper.List(ctx, filter) + selector, err := NewPodSelector(spec, helper) if err != nil { return nil, err } - if len(targets) == 0 { - return nil, fmt.Errorf("finding pods matching '%s': %w", selector, ErrSelectorNoPods) - } - return &podDisruptor{ - helper: helper, - options: options, - targets: targets, + helper: helper, + options: options, + selector: selector, }, nil } -func (d *podDisruptor) Targets(_ context.Context) ([]string, error) { - return utils.PodNames(d.targets), nil +func (d *podDisruptor) Targets(ctx context.Context) ([]string, error) { + targets, err := d.selector.Targets(ctx) + if err != nil { + return nil, err + } + + return utils.PodNames(targets), nil } // InjectHTTPFault injects faults in the http requests sent to the disruptor's targets @@ -175,7 +108,12 @@ func (d *podDisruptor) InjectHTTPFaults( command, ) - controller := NewPodController(d.targets) + targets, err := d.selector.Targets(ctx) + if err != nil { + return err + } + + controller := NewPodController(targets) return controller.Visit(ctx, visitor) } @@ -199,7 +137,12 @@ func (d *podDisruptor) InjectGrpcFaults( command, ) - controller := NewPodController(d.targets) + targets, err := d.selector.Targets(ctx) + if err != nil { + return err + } + + controller := NewPodController(targets) return controller.Visit(ctx, visitor) } @@ -209,7 +152,12 @@ func (d *podDisruptor) TerminatePods( ctx context.Context, fault PodTerminationFault, ) ([]string, error) { - targets, err := utils.Sample(d.targets, fault.Count) + targets, err := d.selector.Targets(ctx) + if err != nil { + return nil, err + } + + targets, err = utils.Sample(targets, fault.Count) if err != nil { return nil, err } diff --git a/pkg/disruptors/pod_test.go b/pkg/disruptors/pod_test.go deleted file mode 100644 index 2c936a98..00000000 --- a/pkg/disruptors/pod_test.go +++ /dev/null @@ -1,151 +0,0 @@ -package disruptors - -import ( - "context" - "sort" - "testing" - - "github.com/google/go-cmp/cmp" - "github.com/grafana/xk6-disruptor/pkg/kubernetes" - "github.com/grafana/xk6-disruptor/pkg/testutils/kubernetes/builders" - - corev1 "k8s.io/api/core/v1" - k8sruntime "k8s.io/apimachinery/pkg/runtime" - "k8s.io/client-go/kubernetes/fake" -) - -func Test_NewPodDisruptor(t *testing.T) { - t.Parallel() - - testCases := []struct { - title string - pods []corev1.Pod - selector PodSelector - expectError bool - expected []string - }{ - { - title: "matching pods", - pods: []corev1.Pod{ - builders.NewPodBuilder("pod-1"). - WithNamespace("test-ns"). - WithLabel("app", "test"). - WithIP("192.0.2.6"). - Build(), - }, - selector: PodSelector{ - Namespace: "test-ns", - Select: PodAttributes{Labels: map[string]string{ - "app": "test", - }}, - }, - expectError: false, - expected: []string{"pod-1"}, - }, - { - title: "no matching pods", - pods: []corev1.Pod{}, - selector: PodSelector{ - Namespace: "test-ns", - Select: PodAttributes{Labels: map[string]string{ - "app": "test", - }}, - }, - expectError: true, - }, - } - - for _, tc := range testCases { - tc := tc - - t.Run(tc.title, func(t *testing.T) { - t.Parallel() - - var objs []k8sruntime.Object - for p := range tc.pods { - objs = append(objs, &tc.pods[p]) - } - - client := fake.NewSimpleClientset(objs...) - k, _ := kubernetes.NewFakeKubernetes(client) - - d, err := NewPodDisruptor( - context.TODO(), - k, - tc.selector, - PodDisruptorOptions{InjectTimeout: -1}, // Disable waiting for injected container to become Running. - ) - - if tc.expectError && err != nil { - return - } - - if !tc.expectError && err != nil { - t.Errorf("unexpected error creating pod disruptor: %v", err) - return - } - - if tc.expectError && err == nil { - t.Errorf("should had failed creating service disruptor") - return - } - - targets, _ := d.Targets(context.TODO()) - sort.Strings(targets) - if diff := cmp.Diff(targets, tc.expected); diff != "" { - t.Errorf("expected targets dot not match returned\n%s", diff) - return - } - }) - } -} - -func Test_PodSelectorString(t *testing.T) { - t.Parallel() - - for _, tc := range []struct { - name string - selector PodSelector - expected string - }{ - { - name: "Empty selector", - expected: `all pods in ns "default"`, - }, - { - name: "Only inclusions", - selector: PodSelector{ - Namespace: "testns", - Select: PodAttributes{map[string]string{"foo": "bar"}}, - }, - expected: `pods including(foo=bar) in ns "testns"`, - }, - { - name: "Only exclusions", - selector: PodSelector{ - Namespace: "testns", - Exclude: PodAttributes{map[string]string{"foo": "bar"}}, - }, - expected: `pods excluding(foo=bar) in ns "testns"`, - }, - { - name: "Both inclusions and exclusions", - selector: PodSelector{ - Namespace: "testns", - Select: PodAttributes{map[string]string{"foo": "bar"}}, - Exclude: PodAttributes{map[string]string{"boo": "baa"}}, - }, - expected: `pods including(foo=bar), excluding(boo=baa) in ns "testns"`, - }, - } { - tc := tc - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - output := tc.selector.String() - if tc.expected != output { - t.Errorf("expected string does not match output string:\n%s\n%s", tc.expected, output) - } - }) - } -} diff --git a/pkg/disruptors/selector.go b/pkg/disruptors/selector.go new file mode 100644 index 00000000..2f7d8430 --- /dev/null +++ b/pkg/disruptors/selector.go @@ -0,0 +1,141 @@ +package disruptors + +import ( + "context" + "errors" + "fmt" + "reflect" + "strings" + + "github.com/grafana/xk6-disruptor/pkg/kubernetes/helpers" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// ErrSelectorNoPods is returned by NewPodDisruptor when the selector passed to it does not match any pod in the +// cluster. +var ErrSelectorNoPods = errors.New("no pods found matching selector") + +// ErrServiceNoTargets is returned by NewServiceDisruptor when passed a service without any pod matching its selector. +var ErrServiceNoTargets = errors.New("service does not have any backing pods") + +// PodSelector returns the target of a PodSelectorSpec +type PodSelector struct { + helper helpers.PodHelper + spec PodSelectorSpec +} + +// NewPodSelector creates a new PodSelector +func NewPodSelector(spec PodSelectorSpec, helper helpers.PodHelper) (*PodSelector, error) { + // validate selector + emptySelect := reflect.DeepEqual(spec.Select, PodAttributes{}) + emptyExclude := reflect.DeepEqual(spec.Exclude, PodAttributes{}) + if spec.Namespace == "" && emptySelect && emptyExclude { + return nil, fmt.Errorf("namespace, select and exclude attributes in pod selector cannot all be empty") + } + + return &PodSelector{ + spec: spec, + helper: helper, + }, nil +} + +// Targets returns the list of target pods +func (s *PodSelector) Targets(ctx context.Context) ([]corev1.Pod, error) { + filter := helpers.PodFilter{ + Select: s.spec.Select.Labels, + Exclude: s.spec.Exclude.Labels, + } + + targets, err := s.helper.List(ctx, filter) + if err != nil { + return nil, err + } + + if len(targets) == 0 { + return nil, fmt.Errorf("finding pods matching '%s': %w", s.spec, ErrSelectorNoPods) + } + + return targets, nil +} + +// NamespaceOrDefault returns the configured namespace for this selector, and the name of the default namespace if it +// is not configured. +func (p PodSelectorSpec) NamespaceOrDefault() string { + if p.Namespace != "" { + return p.Namespace + } + + return metav1.NamespaceDefault +} + +// String returns a human-readable explanation of the pods matched by a PodSelector. +func (p PodSelectorSpec) String() string { + var str string + + if len(p.Select.Labels) == 0 && len(p.Exclude.Labels) == 0 { + str = "all pods" + } else { + str = "pods " + str += p.groupLabels("including", p.Select.Labels) + str += p.groupLabels("excluding", p.Exclude.Labels) + str = strings.TrimSuffix(str, ", ") + } + + str += fmt.Sprintf(" in ns %q", p.NamespaceOrDefault()) + + return str +} + +// groupLabels returns a group of labels as a string, giving that group a name. The returned string has the form of: +// `groupName(foo=bar, boo=baz), `, including the trailing space and comma. +// An empty group of labels produces an empty string. +func (PodSelectorSpec) groupLabels(groupName string, labels map[string]string) string { + if len(labels) == 0 { + return "" + } + + group := groupName + "(" + for k, v := range labels { + group += fmt.Sprintf("%s=%s, ", k, v) + } + group = strings.TrimSuffix(group, ", ") + group += "), " + + return group +} + +// ServicePodSelector returns the targets of a Service +type ServicePodSelector struct { + service string + namespace string + helper helpers.ServiceHelper +} + +// NewServicePodSelector returns a new ServicePodSelector +func NewServicePodSelector( + service string, + namespace string, + helper helpers.ServiceHelper, +) (*ServicePodSelector, error) { + return &ServicePodSelector{ + service: service, + namespace: namespace, + helper: helper, + }, nil +} + +// Targets returns the list of target pods +func (s *ServicePodSelector) Targets(ctx context.Context) ([]corev1.Pod, error) { + targets, err := s.helper.GetTargets(ctx, s.service) + if err != nil { + return nil, err + } + + if len(targets) == 0 { + return nil, fmt.Errorf("finding pods matching%s/%s: %w", s.service, s.namespace, ErrServiceNoTargets) + } + + return targets, nil +} diff --git a/pkg/disruptors/selector_test.go b/pkg/disruptors/selector_test.go new file mode 100644 index 00000000..becc364c --- /dev/null +++ b/pkg/disruptors/selector_test.go @@ -0,0 +1,332 @@ +package disruptors + +import ( + "context" + "sort" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/grafana/xk6-disruptor/pkg/kubernetes" + "github.com/grafana/xk6-disruptor/pkg/testutils/kubernetes/builders" + "github.com/grafana/xk6-disruptor/pkg/utils" + + corev1 "k8s.io/api/core/v1" + runtime "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/intstr" + + "k8s.io/client-go/kubernetes/fake" +) + +func Test_NewPodSelector(t *testing.T) { + t.Parallel() + + testCases := []struct { + title string + spec PodSelectorSpec + expectError bool + expected []string + }{ + { + title: "valid specs", + spec: PodSelectorSpec{ + Namespace: "test-ns", + Select: PodAttributes{Labels: map[string]string{ + "app": "test", + }}, + }, + expectError: false, + }, + { + title: "empty specs", + spec: PodSelectorSpec{}, + expectError: true, + }, + } + + for _, tc := range testCases { + tc := tc + + t.Run(tc.title, func(t *testing.T) { + t.Parallel() + + client := fake.NewSimpleClientset() + k, _ := kubernetes.NewFakeKubernetes(client) + helper := k.PodHelper(tc.spec.Namespace) + + _, err := NewPodSelector(tc.spec, helper) + + if tc.expectError && err != nil { + return + } + + if !tc.expectError && err != nil { + t.Fatalf("unexpected error creating pod selector: %v", err) + } + + if tc.expectError && err == nil { + t.Fatalf("should had failed creating pod selector") + } + }) + } +} + +func Test_PodSelectorString(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + selector PodSelectorSpec + expected string + }{ + { + name: "Empty selector", + expected: `all pods in ns "default"`, + }, + { + name: "Only inclusions", + selector: PodSelectorSpec{ + Namespace: "testns", + Select: PodAttributes{map[string]string{"foo": "bar"}}, + }, + expected: `pods including(foo=bar) in ns "testns"`, + }, + { + name: "Only exclusions", + selector: PodSelectorSpec{ + Namespace: "testns", + Exclude: PodAttributes{map[string]string{"foo": "bar"}}, + }, + expected: `pods excluding(foo=bar) in ns "testns"`, + }, + { + name: "Both inclusions and exclusions", + selector: PodSelectorSpec{ + Namespace: "testns", + Select: PodAttributes{map[string]string{"foo": "bar"}}, + Exclude: PodAttributes{map[string]string{"boo": "baa"}}, + }, + expected: `pods including(foo=bar), excluding(boo=baa) in ns "testns"`, + }, + } { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + output := tc.selector.String() + if tc.expected != output { + t.Fatalf("expected string does not match output string:\n%s\n%s", tc.expected, output) + } + }) + } +} + +func Test_PodSelectorTargets(t *testing.T) { + t.Parallel() + + testCases := []struct { + title string + namespace string + pods []corev1.Pod + spec PodSelectorSpec + expectError bool + expected []string + }{ + { + title: "matching pods", + namespace: "test-ns", + pods: []corev1.Pod{ + builders.NewPodBuilder("pod-1"). + WithNamespace("test-ns"). + WithLabel("app", "test"). + Build(), + }, + spec: PodSelectorSpec{ + Namespace: "test-ns", + Select: PodAttributes{Labels: map[string]string{ + "app": "test", + }}, + }, + expectError: false, + expected: []string{"pod-1"}, + }, + { + title: "no matching pods", + namespace: "test-ns", + pods: []corev1.Pod{}, + spec: PodSelectorSpec{ + Namespace: "test-ns", + Select: PodAttributes{Labels: map[string]string{ + "app": "test", + }}, + }, + expected: nil, + expectError: true, + }, + } + + for _, tc := range testCases { + tc := tc + + t.Run(tc.title, func(t *testing.T) { + t.Parallel() + + var objs []runtime.Object + for p := range tc.pods { + objs = append(objs, &tc.pods[p]) + } + + client := fake.NewSimpleClientset(objs...) + k, _ := kubernetes.NewFakeKubernetes(client) + + s, err := NewPodSelector(tc.spec, k.PodHelper(tc.namespace)) + if err != nil { + t.Fatalf("failed%v", err) + } + + targets, err := s.Targets(context.TODO()) + if tc.expectError && err != nil { + return + } + + if !tc.expectError && err != nil { + t.Fatalf("failed%v", err) + } + + if tc.expectError && err == nil { + t.Fatalf("should had failed") + } + + targetNames := utils.PodNames(targets) + sort.Strings(targetNames) + if diff := cmp.Diff(targetNames, tc.expected); diff != "" { + t.Fatalf("expected targets dot not match returned\n%s", diff) + } + }) + } +} + +func Test_ServicePodSelectorTargets(t *testing.T) { + t.Parallel() + + testCases := []struct { + title string + name string + namespace string + service *corev1.Service + pods []corev1.Pod + expectError bool + expected []string + }{ + { + title: "one endpoint", + name: "test-svc", + namespace: "test-ns", + service: builders.NewServiceBuilder("test-svc"). + WithNamespace("test-ns"). + WithSelectorLabel("app", "test"). + WithPort("http", 80, intstr.FromInt(80)). + BuildAsPtr(), + pods: []corev1.Pod{ + builders.NewPodBuilder("pod-1"). + WithNamespace("test-ns"). + WithLabel("app", "test"). + Build(), + }, + expectError: false, + expected: []string{"pod-1"}, + }, + { + title: "multiple endpoints", + name: "test-svc", + namespace: "test-ns", + service: builders.NewServiceBuilder("test-svc"). + WithNamespace("test-ns"). + WithSelectorLabel("app", "test"). + WithPort("http", 80, intstr.FromInt(80)). + BuildAsPtr(), + pods: []corev1.Pod{ + builders.NewPodBuilder("pod-1"). + WithNamespace("test-ns"). + WithLabel("app", "test"). + Build(), + builders.NewPodBuilder("pod-2"). + WithNamespace("test-ns"). + WithLabel("app", "test"). + Build(), + }, + expectError: false, + expected: []string{"pod-1", "pod-2"}, + }, + { + title: "no endpoints", + name: "test-svc", + namespace: "test-ns", + service: builders.NewServiceBuilder("test-svc"). + WithNamespace("test-ns"). + WithSelectorLabel("app", "test"). + WithPort("http", 80, intstr.FromInt(80)). + BuildAsPtr(), + pods: nil, + expectError: true, + }, + { + title: "service does not exist", + name: "test-svc", + namespace: "test-ns", + service: nil, + pods: nil, + expectError: true, + }, + } + + for _, tc := range testCases { + tc := tc + + t.Run(tc.title, func(t *testing.T) { + t.Parallel() + + objs := []runtime.Object{} + if tc.service != nil { + objs = append(objs, tc.service) + } + for p := range tc.pods { + objs = append(objs, &tc.pods[p]) + } + + client := fake.NewSimpleClientset(objs...) + k, _ := kubernetes.NewFakeKubernetes(client) + + d, err := NewServicePodSelector( + tc.name, + tc.namespace, + k.ServiceHelper(tc.namespace), + ) + if err != nil { + t.Fatalf("failed%v", err) + } + + targets, err := d.Targets(context.TODO()) + + if tc.expectError && err != nil { + return + } + + if !tc.expectError && err != nil { + t.Errorf("failed: %v", err) + return + } + + if tc.expectError && err == nil { + t.Errorf("should had failed") + return + } + + targetNames := utils.PodNames(targets) + sort.Strings(targetNames) + if diff := cmp.Diff(targetNames, tc.expected); diff != "" { + t.Errorf("expected targets dot not match returned\n%s", diff) + return + } + }) + } +} diff --git a/pkg/disruptors/service.go b/pkg/disruptors/service.go index b492c830..3eb744fd 100644 --- a/pkg/disruptors/service.go +++ b/pkg/disruptors/service.go @@ -2,7 +2,6 @@ package disruptors import ( "context" - "errors" "fmt" "time" @@ -14,9 +13,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -// ErrServiceNoTargets is returned by NewServiceDisruptor when passed a service without any pod matching its selector. -var ErrServiceNoTargets = errors.New("service does not have any backing pods") - // ServiceDisruptor defines operations for injecting faults in services type ServiceDisruptor interface { Disruptor @@ -33,10 +29,10 @@ type ServiceDisruptorOptions struct { // serviceDisruptor is an instance of a ServiceDisruptor type serviceDisruptor struct { - service corev1.Service - helper helpers.PodHelper - options ServiceDisruptorOptions - targets []corev1.Pod + service corev1.Service + helper helpers.PodHelper + selector *ServicePodSelector + options ServiceDisruptorOptions } // NewServiceDisruptor creates a new instance of a ServiceDisruptor that targets the given service @@ -51,29 +47,25 @@ func NewServiceDisruptor( return nil, fmt.Errorf("must specify a service name") } + if namespace == "" { + return nil, fmt.Errorf("must specify a namespace") + } + svc, err := k8s.Client().CoreV1().Services(namespace).Get(ctx, service, metav1.GetOptions{}) if err != nil { return nil, err } - sh := k8s.ServiceHelper(namespace) - - targets, err := sh.GetTargets(ctx, service) + selector, err := NewServicePodSelector(service, namespace, k8s.ServiceHelper(namespace)) if err != nil { return nil, err } - if len(targets) == 0 { - return nil, fmt.Errorf("creating disruptor for service %s/%s: %w", service, namespace, ErrServiceNoTargets) - } - - helper := k8s.PodHelper(namespace) - return &serviceDisruptor{ - service: *svc, - helper: helper, - options: options, - targets: targets, + service: *svc, + helper: k8s.PodHelper(namespace), + selector: selector, + options: options, }, nil } @@ -103,7 +95,12 @@ func (d *serviceDisruptor) InjectHTTPFaults( command, ) - controller := NewPodController(d.targets) + targets, err := d.selector.Targets(ctx) + if err != nil { + return err + } + + controller := NewPodController(targets) return controller.Visit(ctx, visitor) } @@ -134,13 +131,23 @@ func (d *serviceDisruptor) InjectGrpcFaults( command, ) - controller := NewPodController(d.targets) + targets, err := d.selector.Targets(ctx) + if err != nil { + return err + } + + controller := NewPodController(targets) return controller.Visit(ctx, visitor) } -func (d *serviceDisruptor) Targets(_ context.Context) ([]string, error) { - return utils.PodNames(d.targets), nil +func (d *serviceDisruptor) Targets(ctx context.Context) ([]string, error) { + targets, err := d.selector.Targets(ctx) + if err != nil { + return nil, err + } + + return utils.PodNames(targets), nil } // TerminatePods terminates a subset of the target pods of the disruptor @@ -148,7 +155,12 @@ func (d *serviceDisruptor) TerminatePods( ctx context.Context, fault PodTerminationFault, ) ([]string, error) { - targets, err := utils.Sample(d.targets, fault.Count) + targets, err := d.selector.Targets(ctx) + if err != nil { + return nil, err + } + + targets, err = utils.Sample(targets, fault.Count) if err != nil { return nil, err } diff --git a/pkg/disruptors/service_test.go b/pkg/disruptors/service_test.go index 52240cdd..7a663d0f 100644 --- a/pkg/disruptors/service_test.go +++ b/pkg/disruptors/service_test.go @@ -2,7 +2,6 @@ package disruptors import ( "context" - "sort" "testing" corev1 "k8s.io/api/core/v1" @@ -11,13 +10,10 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/client-go/kubernetes/fake" - "github.com/google/go-cmp/cmp" "github.com/grafana/xk6-disruptor/pkg/kubernetes" "github.com/grafana/xk6-disruptor/pkg/testutils/kubernetes/builders" ) -// TODO: Refactor tests so they include the generated command. -// Currently we do not have tests covering command generation logic for ServiceDisruptor. func Test_NewServiceDisruptor(t *testing.T) { t.Parallel() @@ -26,13 +22,11 @@ func Test_NewServiceDisruptor(t *testing.T) { name string namespace string service *corev1.Service - pods []corev1.Pod options ServiceDisruptorOptions expectError bool - expected []string }{ { - title: "one endpoint", + title: "service exists", name: "test-svc", namespace: "test-ns", service: builders.NewServiceBuilder("test-svc"). @@ -40,78 +34,41 @@ func Test_NewServiceDisruptor(t *testing.T) { WithSelectorLabel("app", "test"). WithPort("http", 80, intstr.FromInt(80)). BuildAsPtr(), - pods: []corev1.Pod{ - builders.NewPodBuilder("pod-1"). - WithNamespace("test-ns"). - WithLabel("app", "test"). - WithIP("192.0.2.6"). - Build(), - }, + options: ServiceDisruptorOptions{ InjectTimeout: -1, }, expectError: false, - expected: []string{"pod-1"}, }, { - title: "multiple endpoints", - name: "test-svc", - namespace: "test-ns", - service: builders.NewServiceBuilder("test-svc"). - WithNamespace("test-ns"). - WithSelectorLabel("app", "test"). - WithPort("http", 80, intstr.FromInt(80)). - BuildAsPtr(), - pods: []corev1.Pod{ - builders.NewPodBuilder("pod-1"). - WithNamespace("test-ns"). - WithLabel("app", "test"). - WithIP("192.0.2.6"). - Build(), - builders.NewPodBuilder("pod-2"). - WithNamespace("test-ns"). - WithLabel("app", "test"). - WithIP("192.0.2.7"). - Build(), - }, - options: ServiceDisruptorOptions{ - InjectTimeout: -1, - }, - expectError: false, - expected: []string{"pod-1", "pod-2"}, + title: "service does not exist", + name: "test-svc", + namespace: "test-ns", + service: nil, + options: ServiceDisruptorOptions{}, + expectError: true, }, { - title: "no endpoints", - name: "test-svc", + title: "empty service name", + name: "", namespace: "test-ns", service: builders.NewServiceBuilder("test-svc"). WithNamespace("test-ns"). WithSelectorLabel("app", "test"). WithPort("http", 80, intstr.FromInt(80)). BuildAsPtr(), - pods: []corev1.Pod{}, options: ServiceDisruptorOptions{}, expectError: true, }, { - title: "service does not exist", - name: "test-svc", - namespace: "test-ns", - service: nil, - pods: []corev1.Pod{}, - options: ServiceDisruptorOptions{}, - expectError: true, - }, - { - title: "empty service name", - name: "", - namespace: "test-ns", + title: "empty namespace", + name: "test-svc", + namespace: "", service: builders.NewServiceBuilder("test-svc"). WithNamespace("test-ns"). WithSelectorLabel("app", "test"). WithPort("http", 80, intstr.FromInt(80)). BuildAsPtr(), - pods: []corev1.Pod{}, options: ServiceDisruptorOptions{}, expectError: true, }, @@ -127,14 +84,11 @@ func Test_NewServiceDisruptor(t *testing.T) { if tc.service != nil { objs = append(objs, tc.service) } - for p := range tc.pods { - objs = append(objs, &tc.pods[p]) - } client := fake.NewSimpleClientset(objs...) k, _ := kubernetes.NewFakeKubernetes(client) - d, err := NewServiceDisruptor( + _, err := NewServiceDisruptor( context.TODO(), k, tc.name, @@ -147,19 +101,12 @@ func Test_NewServiceDisruptor(t *testing.T) { } if !tc.expectError && err != nil { - t.Errorf(" unexpected error creating service disruptor: %v", err) + t.Errorf("failed: %v", err) return } if tc.expectError && err == nil { - t.Errorf("should had failed creating service disruptor") - return - } - - targets, _ := d.Targets(context.TODO()) - sort.Strings(targets) - if diff := cmp.Diff(targets, tc.expected); diff != "" { - t.Errorf("expected targets dot not match returned\n%s", diff) + t.Errorf("should had failed ") return } })