From 2823ff756185ee6f3d46e97726a336a587a7f533 Mon Sep 17 00:00:00 2001 From: Pablo Chacin Date: Tue, 20 Dec 2022 16:03:58 +0100 Subject: [PATCH 01/15] Add unit tests for PodDisruptor constructor in api Signed-off-by: Pablo Chacin --- pkg/api/api.go | 4 ++ pkg/api/api_test.go | 124 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+) create mode 100644 pkg/api/api_test.go diff --git a/pkg/api/api.go b/pkg/api/api.go index d4c819c4..1d344cee 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -12,6 +12,10 @@ import ( // NewPodDisruptor creates an instance of a PodDisruptor func NewPodDisruptor(rt *goja.Runtime, c goja.ConstructorCall, k8s kubernetes.Kubernetes) (*goja.Object, error) { + if c.Argument(0).Equals(goja.Null()) { + return nil, fmt.Errorf("PodDisruptor constructor expects a non null PodSelector argument") + } + selector := disruptors.PodSelector{} err := rt.ExportTo(c.Argument(0), &selector) if err != nil { diff --git a/pkg/api/api_test.go b/pkg/api/api_test.go new file mode 100644 index 00000000..8139fb40 --- /dev/null +++ b/pkg/api/api_test.go @@ -0,0 +1,124 @@ +package api + +import ( + "fmt" + "testing" + + "github.com/dop251/goja" + "github.com/grafana/xk6-disruptor/pkg/disruptors" + "github.com/grafana/xk6-disruptor/pkg/kubernetes" + "go.k6.io/k6/js/common" + "k8s.io/client-go/kubernetes/fake" +) + +func testRuntime() (*goja.Runtime, error) { + rt := goja.New() + rt.SetFieldNameMapper(common.FieldNameMapper{}) + + k8s, err := kubernetes.NewFakeKubernetes(fake.NewSimpleClientset()) + if err != nil { + return nil, err + } + + var disruptor *goja.Object + err = rt.Set("PodDisruptor", func(c goja.ConstructorCall) *goja.Object { + disruptor, err = NewPodDisruptor(rt, c, k8s) + if err != nil { + common.Throw(rt, fmt.Errorf("error creating PodDisruptor: %w", err)) + } + return disruptor + }) + if err != nil { + return nil, err + } + + return rt, nil +} + +func Test_PodDisruptorConstructor(t *testing.T) { + t.Parallel() + + testCases := []struct { + description string + script string + expectError bool + }{ + { + description: "valid constructor", + script: ` + const selector = { + namespace: "namespace", + select: { + labels: { + app: "app" + } + } + } + const opts = { + injectTimeout: 0 + } + new PodDisruptor(selector, opts) + `, + expectError: false, + }, + { + description: "valid constructor without options", + script: ` + const selector = { + namespace: "namespace", + select: { + labels: { + app: "app" + } + } + } + new PodDisruptor(selector) + `, + expectError: false, + }, + { + description: "invalid constructor without selector", + script: ` + new PodDisruptor() + `, + expectError: true, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.description, func(t *testing.T) { + t.Parallel() + + rt, err := testRuntime() + if err != nil { + t.Errorf("error in test setup %v", err) + return + } + + value, err := rt.RunString(tc.script) + + if !tc.expectError && err != nil { + t.Errorf("failed %v", err) + return + } + + if tc.expectError && err == nil { + t.Errorf("should had failed") + return + } + + // failed and it was expected, it is ok + if tc.expectError && err != nil { + return + } + + var pd disruptors.PodDisruptor + err = rt.ExportTo(value, &pd) + if err != nil { + t.Errorf("returned valued cannot be converted to PodDisruptor: %v", err) + return + } + }) + } +} From 9b16468f7283728464f9dea6abb83743adeddda6 Mon Sep 17 00:00:00 2001 From: Pablo Chacin Date: Tue, 20 Dec 2022 19:10:51 +0100 Subject: [PATCH 02/15] Validate PodSelector Signed-off-by: Pablo Chacin --- pkg/disruptors/pod.go | 8 +++ pkg/disruptors/pod_test.go | 134 ++++++++++++++++++++++++++----------- 2 files changed, 103 insertions(+), 39 deletions(-) diff --git a/pkg/disruptors/pod.go b/pkg/disruptors/pod.go index 21afe2c7..7c2e3b2f 100644 --- a/pkg/disruptors/pod.go +++ b/pkg/disruptors/pod.go @@ -3,6 +3,7 @@ package disruptors import ( "fmt" + "reflect" "sync" "time" @@ -85,6 +86,13 @@ func (s *PodSelector) buildLabelSelector() (labels.Selector, error) { // GetTargets retrieves the names of the targets of the disruptor func (s *PodSelector) GetTargets(k8s kubernetes.Kubernetes) ([]string, error) { + // validate selector + emptySelect := reflect.DeepEqual(s.Select, PodAttributes{}) + emptyExclude := reflect.DeepEqual(s.Exclude, PodAttributes{}) + if s.Namespace == "" && emptySelect && emptyExclude { + return nil, fmt.Errorf("namespace, select and exclude attributes in pod selector cannot all be empty") + } + namespace := s.Namespace if namespace == "" { namespace = metav1.NamespaceDefault diff --git a/pkg/disruptors/pod_test.go b/pkg/disruptors/pod_test.go index 04b19770..9d5d1f2f 100644 --- a/pkg/disruptors/pod_test.go +++ b/pkg/disruptors/pod_test.go @@ -15,14 +15,13 @@ import ( "github.com/grafana/xk6-disruptor/pkg/testutils/kubernetes/builders" ) -func Test_PodSelectorWithLabels(t *testing.T) { +func Test_PodSelector(t *testing.T) { t.Parallel() testCases := []struct { title string pods []podDesc - labels map[string]string - exclude map[string]string + selector PodSelector expectError bool expectedPods []string }{ @@ -35,8 +34,13 @@ func Test_PodSelectorWithLabels(t *testing.T) { labels: map[string]string{}, }, }, - labels: map[string]string{ - "app": "test", + selector: PodSelector{ + Namespace: "test-ns", + Select: PodAttributes{ + Labels: map[string]string{ + "app": "test", + }, + }, }, expectError: false, expectedPods: []string{}, @@ -52,8 +56,13 @@ func Test_PodSelectorWithLabels(t *testing.T) { }, }, }, - labels: map[string]string{ - "app": "test", + selector: PodSelector{ + Namespace: "test-ns", + Select: PodAttributes{ + Labels: map[string]string{ + "app": "test", + }, + }, }, expectError: false, expectedPods: []string{}, @@ -63,14 +72,19 @@ func Test_PodSelectorWithLabels(t *testing.T) { pods: []podDesc{ { name: "pod-with-app-label", - namespace: testNamespace, + namespace: "test-ns", labels: map[string]string{ "app": "test", }, }, }, - labels: map[string]string{ - "app": "test", + selector: PodSelector{ + Namespace: "test-ns", + Select: PodAttributes{ + Labels: map[string]string{ + "app": "test", + }, + }, }, expectError: false, expectedPods: []string{ @@ -82,21 +96,26 @@ func Test_PodSelectorWithLabels(t *testing.T) { pods: []podDesc{ { name: "pod-with-app-label", - namespace: testNamespace, + namespace: "test-ns", labels: map[string]string{ "app": "test", }, }, { name: "another-pod-with-app-label", - namespace: testNamespace, + namespace: "test-ns", labels: map[string]string{ "app": "test", }, }, }, - labels: map[string]string{ - "app": "test", + selector: PodSelector{ + Namespace: "test-ns", + Select: PodAttributes{ + Labels: map[string]string{ + "app": "test", + }, + }, }, expectError: false, expectedPods: []string{ @@ -109,14 +128,14 @@ func Test_PodSelectorWithLabels(t *testing.T) { pods: []podDesc{ { name: "pod-with-app-label", - namespace: testNamespace, + namespace: "test-ns", labels: map[string]string{ "app": "test", }, }, { name: "pod-with-dev-label", - namespace: testNamespace, + namespace: "test-ns", labels: map[string]string{ "app": "test", "env": "dev", @@ -124,16 +143,21 @@ func Test_PodSelectorWithLabels(t *testing.T) { }, { name: "pod-with-prod-label", - namespace: testNamespace, + namespace: "test-ns", labels: map[string]string{ "app": "test", "env": "prod", }, }, }, - labels: map[string]string{ - "app": "test", - "env": "dev", + selector: PodSelector{ + Namespace: "test-ns", + Select: PodAttributes{ + Labels: map[string]string{ + "app": "test", + "env": "dev", + }, + }, }, expectError: false, expectedPods: []string{ @@ -141,11 +165,11 @@ func Test_PodSelectorWithLabels(t *testing.T) { }, }, { - title: "exclude environment", + title: "exclude labels", pods: []podDesc{ { name: "pod-with-dev-label", - namespace: testNamespace, + namespace: "test-ns", labels: map[string]string{ "app": "test", "env": "dev", @@ -153,24 +177,66 @@ func Test_PodSelectorWithLabels(t *testing.T) { }, { name: "pod-with-prod-label", - namespace: testNamespace, + namespace: "test-ns", labels: map[string]string{ "app": "test", "env": "prod", }, }, }, - labels: map[string]string{ - "app": "test", - }, - exclude: map[string]string{ - "env": "prod", + selector: PodSelector{ + Namespace: "test-ns", + Select: PodAttributes{ + Labels: map[string]string{ + "app": "test", + }, + }, + Exclude: PodAttributes{ + Labels: map[string]string{ + "env": "prod", + }, + }, }, expectError: false, expectedPods: []string{ "pod-with-dev-label", }, }, + { + title: "Namespace selector", + pods: []podDesc{ + { + name: "pod-in-test-ns", + namespace: "test-ns", + labels: map[string]string{}, + }, + { + name: "another-pod-in-test-ns", + namespace: "test-ns", + labels: map[string]string{}, + }, + { + name: "pod-in-another-namespace", + namespace: "other-ns", + labels: map[string]string{}, + }, + }, + selector: PodSelector{ + Namespace: "test-ns", + }, + expectError: false, + expectedPods: []string{ + "pod-in-test-ns", + "another-pod-in-test-ns", + }, + }, + { + title: "Empty selector", + pods: []podDesc{}, + selector: PodSelector{}, + expectError: true, + expectedPods: []string{}, + }, } for _, tc := range testCases { @@ -189,17 +255,7 @@ func Test_PodSelectorWithLabels(t *testing.T) { } client := fake.NewSimpleClientset(pods...) k, _ := kubernetes.NewFakeKubernetes(client) - selector := PodSelector{ - Namespace: testNamespace, - Select: PodAttributes{ - Labels: tc.labels, - }, - Exclude: PodAttributes{ - Labels: tc.exclude, - }, - } - - targets, err := selector.GetTargets(k) + targets, err := tc.selector.GetTargets(k) if tc.expectError && err == nil { t.Errorf("should had failed") return From 342a402cb7e9af8d8721b2f9d3722370d2988db5 Mon Sep 17 00:00:00 2001 From: Pablo Chacin Date: Tue, 20 Dec 2022 19:57:58 +0100 Subject: [PATCH 03/15] Add service disruptor unit tests Signed-off-by: Pablo Chacin --- pkg/disruptors/service.go | 3 ++ pkg/disruptors/service_test.go | 90 ++++++++++++++++++++++++++++++---- 2 files changed, 83 insertions(+), 10 deletions(-) diff --git a/pkg/disruptors/service.go b/pkg/disruptors/service.go index 6e6927a0..354c9d6c 100644 --- a/pkg/disruptors/service.go +++ b/pkg/disruptors/service.go @@ -61,6 +61,9 @@ func NewServiceDisruptor( namespace string, options ServiceDisruptorOptions, ) (ServiceDisruptor, error) { + if service == "" { + return nil, fmt.Errorf("must specify a service name") + } svc, err := k8s.CoreV1(). Services(namespace). Get(k8s.Context(), service, metav1.GetOptions{}) diff --git a/pkg/disruptors/service_test.go b/pkg/disruptors/service_test.go index 4dc1a618..1f41430e 100644 --- a/pkg/disruptors/service_test.go +++ b/pkg/disruptors/service_test.go @@ -25,6 +25,8 @@ func Test_NewServiceDisruptor(t *testing.T) { testCases := []struct { title string + serviceName string + namespace string service serviceDesc options ServiceDisruptorOptions pods []podDesc @@ -32,10 +34,12 @@ func Test_NewServiceDisruptor(t *testing.T) { expectedPods []string }{ { - title: "one matching pod", + title: "one matching pod", + serviceName: "test-svc", + namespace: "test-ns", service: serviceDesc{ name: "test-svc", - namespace: testNamespace, + namespace: "test-ns", ports: []corev1.ServicePort{ { Port: 80, @@ -60,10 +64,12 @@ func Test_NewServiceDisruptor(t *testing.T) { expectedPods: []string{"pod-1"}, }, { - title: "no matching pod", + title: "no matching pod", + serviceName: "test-svc", + namespace: "test-ns", service: serviceDesc{ name: "test-svc", - namespace: testNamespace, + namespace: "test-ns", ports: []corev1.ServicePort{ { Port: 80, @@ -88,10 +94,12 @@ func Test_NewServiceDisruptor(t *testing.T) { expectedPods: []string{}, }, { - title: "no pods", + title: "no pods", + serviceName: "test-svc", + namespace: "test-ns", service: serviceDesc{ name: "test-svc", - namespace: testNamespace, + namespace: "test-ns", ports: []corev1.ServicePort{ { Port: 80, @@ -108,10 +116,12 @@ func Test_NewServiceDisruptor(t *testing.T) { expectedPods: []string{}, }, { - title: "pods in another namespace", + title: "pods in another namespace", + serviceName: "test-svc", + namespace: "test-ns", service: serviceDesc{ name: "test-svc", - namespace: testNamespace, + namespace: "test-ns", ports: []corev1.ServicePort{ { Port: 80, @@ -135,6 +145,66 @@ func Test_NewServiceDisruptor(t *testing.T) { expectError: false, expectedPods: []string{}, }, + { + title: "service does not exist", + serviceName: "other-svc", + namespace: "test-ns", + service: serviceDesc{ + name: "test-svc", + namespace: "test-ns", + ports: []corev1.ServicePort{ + { + Port: 80, + TargetPort: intstr.FromInt(80), + }, + }, + selector: map[string]string{ + "app": "test", + }, + }, + options: ServiceDisruptorOptions{}, + pods: []podDesc{ + { + name: "pod-1", + namespace: "another-ns", + labels: map[string]string{ + "app": "test", + }, + }, + }, + expectError: true, + expectedPods: []string{}, + }, + { + title: "empty service name", + serviceName: "", + namespace: "test-ns", + service: serviceDesc{ + name: "test-svc", + namespace: "test-ns", + ports: []corev1.ServicePort{ + { + Port: 80, + TargetPort: intstr.FromInt(80), + }, + }, + selector: map[string]string{ + "app": "test", + }, + }, + options: ServiceDisruptorOptions{}, + pods: []podDesc{ + { + name: "pod-1", + namespace: "another-ns", + labels: map[string]string{ + "app": "test", + }, + }, + }, + expectError: true, + expectedPods: []string{}, + }, } for _, tc := range testCases { @@ -164,8 +234,8 @@ func Test_NewServiceDisruptor(t *testing.T) { tc.options.InjectTimeout = -1 d, err := NewServiceDisruptor( k, - tc.service.name, - tc.service.namespace, + tc.serviceName, + tc.namespace, tc.options, ) From 0e4106c7fe73a53bb35ec5d51e2578072b42033f Mon Sep 17 00:00:00 2001 From: Pablo Chacin Date: Tue, 20 Dec 2022 19:58:52 +0100 Subject: [PATCH 04/15] Relax lint for tests (table tests can be long) Signed-off-by: Pablo Chacin --- .golangci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.golangci.yml b/.golangci.yml index cf01e780..0cf5f8dd 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -25,6 +25,7 @@ issues: - gocognit - lll - bodyclose + - maintidx - linters: - paralleltest # false positive: https://github.com/kunwardeep/paralleltest/issues/8. text: "does not use range value in test Run" From 7e748e63c867ec80bb20bb98828f88e20a2f9642 Mon Sep 17 00:00:00 2001 From: Pablo Chacin Date: Wed, 21 Dec 2022 10:55:48 +0100 Subject: [PATCH 05/15] Refactor to support multiple constructors Signed-off-by: Pablo Chacin --- pkg/api/api_test.go | 56 +++++++++++++++++++++++++++++++-------------- 1 file changed, 39 insertions(+), 17 deletions(-) diff --git a/pkg/api/api_test.go b/pkg/api/api_test.go index 8139fb40..70b9dc92 100644 --- a/pkg/api/api_test.go +++ b/pkg/api/api_test.go @@ -1,6 +1,7 @@ package api import ( + "errors" "fmt" "testing" @@ -11,28 +12,41 @@ import ( "k8s.io/client-go/kubernetes/fake" ) -func testRuntime() (*goja.Runtime, error) { - rt := goja.New() - rt.SetFieldNameMapper(common.FieldNameMapper{}) +// test environment +type testEnv struct { + rt *goja.Runtime + k8s kubernetes.Kubernetes +} - k8s, err := kubernetes.NewFakeKubernetes(fake.NewSimpleClientset()) - if err != nil { - return nil, err - } +// a function that constructs an object +type constructor func(*testEnv, goja.ConstructorCall) (*goja.Object, error) - var disruptor *goja.Object - err = rt.Set("PodDisruptor", func(c goja.ConstructorCall) *goja.Object { - disruptor, err = NewPodDisruptor(rt, c, k8s) +// registers a constructor with a name in the environment's runtime +func (env *testEnv) registerConstructor(name string, constructor constructor) error { + var object *goja.Object + var err error + err = env.rt.Set(name, func(c goja.ConstructorCall) *goja.Object { + object, err = constructor(env, c) if err != nil { - common.Throw(rt, fmt.Errorf("error creating PodDisruptor: %w", err)) + common.Throw(env.rt, fmt.Errorf("error creating %s: %w", name, err)) } - return disruptor + return object }) + return err +} + +func testSetup() (*testEnv, error) { + rt := goja.New() + rt.SetFieldNameMapper(common.FieldNameMapper{}) + + k8s, err := kubernetes.NewFakeKubernetes(fake.NewSimpleClientset()) if err != nil { return nil, err } - - return rt, nil + return &testEnv{ + rt: rt, + k8s: k8s, + }, nil } func Test_PodDisruptorConstructor(t *testing.T) { @@ -90,13 +104,21 @@ func Test_PodDisruptorConstructor(t *testing.T) { t.Run(tc.description, func(t *testing.T) { t.Parallel() - rt, err := testRuntime() + env, err := testSetup() + if err != nil { + t.Errorf("error in test setup %v", err) + return + } + + err = env.registerConstructor("PodDisruptor", func(e *testEnv, c goja.ConstructorCall) (*goja.Object, error) { + return NewPodDisruptor(e.rt, c, e.k8s) + }) if err != nil { t.Errorf("error in test setup %v", err) return } - value, err := rt.RunString(tc.script) + value, err := env.rt.RunString(tc.script) if !tc.expectError && err != nil { t.Errorf("failed %v", err) @@ -114,7 +136,7 @@ func Test_PodDisruptorConstructor(t *testing.T) { } var pd disruptors.PodDisruptor - err = rt.ExportTo(value, &pd) + err = env.rt.ExportTo(value, &pd) if err != nil { t.Errorf("returned valued cannot be converted to PodDisruptor: %v", err) return From 64dfe379c350cded574a1b48e8c6edd26159508f Mon Sep 17 00:00:00 2001 From: Pablo Chacin Date: Wed, 21 Dec 2022 11:07:48 +0100 Subject: [PATCH 06/15] Add tests for ServiceDisruptor constructor Signed-off-by: Pablo Chacin --- pkg/api/api_test.go | 94 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 93 insertions(+), 1 deletion(-) diff --git a/pkg/api/api_test.go b/pkg/api/api_test.go index 70b9dc92..5a0cbe71 100644 --- a/pkg/api/api_test.go +++ b/pkg/api/api_test.go @@ -1,14 +1,16 @@ package api import ( - "errors" + "context" "fmt" "testing" "github.com/dop251/goja" "github.com/grafana/xk6-disruptor/pkg/disruptors" "github.com/grafana/xk6-disruptor/pkg/kubernetes" + "github.com/grafana/xk6-disruptor/pkg/testutils/kubernetes/builders" "go.k6.io/k6/js/common" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/fake" ) @@ -144,3 +146,93 @@ func Test_PodDisruptorConstructor(t *testing.T) { }) } } + +func Test_ServiceDisruptorConstructor(t *testing.T) { + t.Parallel() + + testCases := []struct { + description string + script string + expectError bool + }{ + { + description: "valid constructor", + script: ` + const opts = { + injectTimeout: 0 + } + new ServiceDisruptor("service", "default", opts) + `, + expectError: false, + }, + { + description: "valid constructor without options", + script: ` + new ServiceDisruptor("service", "default") + `, + expectError: false, + }, + { + description: "invalid constructor without namespace", + script: ` + new ServiceDisruptor("service") + `, + expectError: true, + }, + { + description: "invalid constructor without arguments", + script: ` + new ServiceDisruptor() + `, + expectError: true, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.description, func(t *testing.T) { + t.Parallel() + env, err := testSetup() + if err != nil { + t.Errorf("error in test setup %v", err) + return + } + + err = env.registerConstructor("ServiceDisruptor", func(e *testEnv, c goja.ConstructorCall) (*goja.Object, error) { + return NewServiceDisruptor(e.rt, c, e.k8s) + }) + if err != nil { + t.Errorf("error in test setup %v", err) + return + } + + // create a service because the ServiceDisruptor's constructor expects it to exist + svc := builders.NewServiceBuilder("service").Build() + _, _ = env.k8s.CoreV1().Services("default").Create(context.TODO(), svc, v1.CreateOptions{}) + + value, err := env.rt.RunString(tc.script) + + if !tc.expectError && err != nil { + t.Errorf("failed %v", err) + return + } + + if tc.expectError && err == nil { + t.Errorf("should had failed") + return + } + + // failed and it was expected, it is ok + if tc.expectError && err != nil { + return + } + + var sd disruptors.ServiceDisruptor + err = env.rt.ExportTo(value, &sd) + if err != nil { + t.Errorf("returned valued cannot be converted to ServiceDisruptor: %v", err) + return + } + }) + } +} From ad7803314ed8717b18059cdca1920237870240b2 Mon Sep 17 00:00:00 2001 From: Pablo Chacin Date: Wed, 21 Dec 2022 11:22:06 +0100 Subject: [PATCH 07/15] Check for required arguments Signed-off-by: Pablo Chacin --- pkg/api/api.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/api/api.go b/pkg/api/api.go index 1d344cee..344913cf 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -38,6 +38,9 @@ func NewPodDisruptor(rt *goja.Runtime, c goja.ConstructorCall, k8s kubernetes.Ku // NewServiceDisruptor creates an instance of a ServiceDisruptor func NewServiceDisruptor(rt *goja.Runtime, c goja.ConstructorCall, k8s kubernetes.Kubernetes) (*goja.Object, error) { + if len(c.Arguments) < 2 { + return nil, fmt.Errorf("ServiceDisruptor constructor requires service and namespace parameters") + } var service string err := rt.ExportTo(c.Argument(0), &service) if err != nil { From ce302689e760802cf89834031497122d860ee050 Mon Sep 17 00:00:00 2001 From: Pablo Chacin Date: Wed, 21 Dec 2022 10:50:26 +0100 Subject: [PATCH 08/15] Fix error message Signed-off-by: Pablo Chacin --- pkg/api/api.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/api/api.go b/pkg/api/api.go index 344913cf..55b2ea13 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -58,7 +58,7 @@ func NewServiceDisruptor(rt *goja.Runtime, c goja.ConstructorCall, k8s kubernete if len(c.Arguments) > 2 { err = rt.ExportTo(c.Argument(2), &options) if err != nil { - return nil, fmt.Errorf("PodDisruptor constructor expects PodDisruptorOptions as third argument: %w", err) + return nil, fmt.Errorf("ServiceDisruptor constructor expects ServiceDisruptorOptions as third argument: %w", err) } } From 4b200b18400ba684f7ce4484bbfad3d39b0ecbc6 Mon Sep 17 00:00:00 2001 From: Pablo Chacin Date: Wed, 21 Dec 2022 14:02:44 +0100 Subject: [PATCH 09/15] Add type validations to ServiceDisruptor constructor Signed-off-by: Pablo Chacin --- pkg/api/api.go | 12 ++++++++++-- pkg/api/api_test.go | 14 ++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/pkg/api/api.go b/pkg/api/api.go index 55b2ea13..382575d7 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -4,6 +4,7 @@ package api import ( "fmt" + "reflect" "github.com/dop251/goja" "github.com/grafana/xk6-disruptor/pkg/disruptors" @@ -41,16 +42,23 @@ func NewServiceDisruptor(rt *goja.Runtime, c goja.ConstructorCall, k8s kubernete if len(c.Arguments) < 2 { return nil, fmt.Errorf("ServiceDisruptor constructor requires service and namespace parameters") } + + if c.Argument(0).ExportType().Kind() != reflect.String { + return nil, fmt.Errorf("ServiceDisruptor constructor expects service name to be a string") + } var service string err := rt.ExportTo(c.Argument(0), &service) if err != nil { - return nil, fmt.Errorf("ServiceDisruptor constructor expects service name (string) as first argument: %w", err) + return nil, fmt.Errorf("invalid service name argument for ServiceDisruptor constructor: %w", err) } + if c.Argument(1).ExportType().Kind() != reflect.String { + return nil, fmt.Errorf("ServiceDisruptor constructor expects namespace to be a string") + } var namespace string err = rt.ExportTo(c.Argument(1), &namespace) if err != nil { - return nil, fmt.Errorf("ServiceDisruptor constructor expects namespace (string) as second argument: %w", err) + return nil, fmt.Errorf("invalid namespace argument for ServiceDisruptor constructor: %w", err) } options := disruptors.ServiceDisruptorOptions{} diff --git a/pkg/api/api_test.go b/pkg/api/api_test.go index 5a0cbe71..3b00e762 100644 --- a/pkg/api/api_test.go +++ b/pkg/api/api_test.go @@ -179,6 +179,20 @@ func Test_ServiceDisruptorConstructor(t *testing.T) { `, expectError: true, }, + { + description: "invalid constructor service name is not string", + script: ` + new ServiceDisruptor(1, "default") + `, + expectError: true, + }, + { + description: "invalid constructor namespace is not string", + script: ` + new ServiceDisruptor("service", {}) + `, + expectError: true, + }, { description: "invalid constructor without arguments", script: ` From 5af2ed57409a54bfe3c7c47691247998d41d2147 Mon Sep 17 00:00:00 2001 From: Pablo Chacin Date: Thu, 22 Dec 2022 16:04:08 +0100 Subject: [PATCH 10/15] Implement API validations helpers Signed-off-by: Pablo Chacin --- pkg/api/validation.go | 88 ++++++++++++++++++ pkg/api/validation_test.go | 185 +++++++++++++++++++++++++++++++++++++ 2 files changed, 273 insertions(+) create mode 100644 pkg/api/validation.go create mode 100644 pkg/api/validation_test.go diff --git a/pkg/api/validation.go b/pkg/api/validation.go new file mode 100644 index 00000000..66b25142 --- /dev/null +++ b/pkg/api/validation.go @@ -0,0 +1,88 @@ +package api + +import ( + "fmt" + "reflect" + "strings" +) + +// transforms an identifier to its camel case +// maps 'fieldName' and 'field_name' to 'FieldName' +func toGoCase(name string) string { + goCase := "" + for _, world := range strings.Split(name, "_") { + runes := []rune(world) + first := strings.ToUpper(string(runes[0])) + goCase = goCase + first + string(runes[1:]) + } + + return goCase +} + +// IsCompatible checks if the actual value can be assigned to a variable of the expected type +// For Slices it expects []interface{} and therefore cannot check the type of the elements. +// For Maps it expects map[string]interface{} and therefore cannot check the type of the elements +func IsCompatible(actual interface{}, expected interface{}) error { + actualType := reflect.TypeOf(actual) + expectedType := reflect.TypeOf(expected) + compatible := false + switch expectedType.Kind() { + case reflect.Map: + compatible = actualType.Kind() == reflect.Map + case reflect.Slice: + compatible = actualType.Kind() == reflect.Slice + case reflect.String: + compatible = actualType.Kind() == reflect.String + case reflect.Struct: + return ValidateStruct(actual, expected) + default: + compatible = actualType.ConvertibleTo(expectedType) + } + + if !compatible { + return fmt.Errorf("expected %s got %s", expectedType, actualType) + } + return nil +} + +// ValidateStruct validates that the value of a generic map[string]interface{} can +// be assigned to a expected Struct using the compatibility rules defined in IsCompatible. +// Note that the field names are expected to match except for the case of the initial letter +// e.g 'fieldName' will match 'FieldName' in the struct, but 'field_name' will not. +// +// TODO: use the tags in the struct to find out any field name mapping. This wil require +// iterating from the struct to the actual value. This will not detect spurious fields in the +// actual value +func ValidateStruct(actual interface{}, expected interface{}) error { + actualValue, ok := actual.(map[string]interface{}) + if !ok { + return fmt.Errorf("expected actual value map[string]interface{} got %s", reflect.TypeOf(actual)) + } + expectedType := reflect.TypeOf(expected) + if expectedType.Kind() != reflect.Struct { + return fmt.Errorf("expected value must be a Struct") + } + expectedValue := reflect.ValueOf(expected) + + for field, value := range actualValue { + sf, found := expectedType.FieldByName(toGoCase(field)) + if !found { + return fmt.Errorf("unknown field %s in struct %s", field, expectedType.Name()) + } + + err := IsCompatible(value, expectedValue.FieldByIndex(sf.Index).Interface()) + if err != nil { + return fmt.Errorf("invalid type for field %s: %w", field, err) + } + + if sf.Type.Kind() == reflect.Struct { + sfv := expectedValue.FieldByName(sf.Name).Interface() + err := ValidateStruct(value, sfv) + if err != nil { + return err + } + } + } + + return nil +} diff --git a/pkg/api/validation_test.go b/pkg/api/validation_test.go new file mode 100644 index 00000000..0faa37a3 --- /dev/null +++ b/pkg/api/validation_test.go @@ -0,0 +1,185 @@ +package api + +import ( + "testing" +) + +func Test_Validations(t *testing.T) { + t.Parallel() + type prototype struct { + FieldString string + FieldInt int64 + FieldFloat float64 + FieldStruct struct { + SubfieldInt int + SubfieldStruct struct { + SubSubfieldString string + } + } + FieldMap map[string]string + FieldArray []string + } + + testCases := []struct { + description string + actual interface{} + expected interface{} + expectError bool + }{ + { + description: "Valid mapping", + expected: prototype{}, + actual: map[string]interface{}{ + "fieldString": "string", + "fieldInt": int64(1), + "fieldFloat": float64(1), + "fieldStruct": map[string]interface{}{ + "subfieldInt": int64(0), + "subfieldStruct": map[string]interface{}{ + "subSubfieldString": "string", + }, + }, + "fieldArray": []interface{}{}, + }, + expectError: false, + }, + { + description: "Valid mapping with snake_case", + expected: prototype{}, + actual: map[string]interface{}{ + "field_string": "string", + "field_int": int64(1), + "field_float": float64(1), + "field_struct": map[string]interface{}{ + "subfield_int": int64(0), + "subfield_struct": map[string]interface{}{ + "sub_subfield_string": "string", + }, + }, + "field_array": []interface{}{}, + }, + expectError: false, + }, + { + description: "Valid mapping int to float", + expected: prototype{}, + actual: map[string]interface{}{ + "field_string": "string", + "field_int": int64(1), + "fieldFloat": int64(1), + "field_struct": map[string]interface{}{ + "subfield_int": int64(0), + "subfield_struct": map[string]interface{}{ + "sub_subfield_string": "string", + }, + }, + "field_array": []interface{}{}, + }, + expectError: false, + }, + { + description: "Valid mapping float to int", + expected: prototype{}, + actual: map[string]interface{}{ + "field_string": "string", + "field_int": float64(1), + "fieldFloat": float64(1), + "field_struct": map[string]interface{}{ + "subfield_int": int64(0), + "subfield_struct": map[string]interface{}{ + "sub_subfield_string": "string", + }, + }, + "field_array": []interface{}{}, + }, + expectError: false, + }, + { + description: "Invalid mapping (unknown field)", + expected: prototype{}, + actual: map[string]interface{}{ + "unknownField": "string", + "fieldString": "string", + "fieldInt": int64(1), + "fieldFloat": float64(1), + "fieldStruct": map[string]interface{}{ + "subfieldInt": int64(0), + "subfieldStruct": map[string]interface{}{ + "subSubfieldString": "string", + }, + }, + "fieldArray": []interface{}{}, + }, + expectError: true, + }, + { + description: "Invalid mapping (string to int)", + expected: prototype{}, + actual: map[string]interface{}{ + "fieldString": "string", + "fieldInt": "1", + "fieldFloat": float64(1), + "fieldStruct": map[string]interface{}{ + "subfieldInt": int64(0), + "subfieldStruct": map[string]interface{}{ + "subSubfieldString": "string", + }, + }, + "fieldArray": []interface{}{}, + }, + expectError: true, + }, + { + description: "Invalid mapping (int to string)", + expected: prototype{}, + actual: map[string]interface{}{ + "fieldString": int64(1), + "fieldInt": int64(1), + "fieldFloat": float64(1), + "fieldStruct": map[string]interface{}{ + "subfieldInt": int64(0), + "subfieldStruct": map[string]interface{}{ + "subSubfieldString": "string", + }, + }, + "fieldArray": []interface{}{}, + }, + expectError: true, + }, + { + description: "Invalid mapping (struct to string)", + expected: prototype{}, + actual: map[string]interface{}{ + "fieldString": struct{}{}, + "fieldInt": int64(1), + "fieldFloat": float64(1), + "fieldStruct": map[string]interface{}{ + "subfieldInt": int64(0), + "subfieldStruct": map[string]interface{}{ + "subSubfieldString": "string", + }, + }, + "fieldArray": []interface{}{}, + }, + expectError: true, + }, + { + description: "Invalid mapping (string to struct)", + expected: prototype{}, + actual: "string", + expectError: true, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.description, func(t *testing.T) { + t.Parallel() + + err := ValidateStruct(tc.actual, tc.expected) + if !tc.expectError && err != nil { + t.Errorf("failed: %v", err) + } + }) + } +} From 8f0a1d0c6787ac092ac41915eef9498088e54a9d Mon Sep 17 00:00:00 2001 From: Pablo Chacin Date: Thu, 22 Dec 2022 17:45:33 +0100 Subject: [PATCH 11/15] Add validations to PodDisruptor constructor Signed-off-by: Pablo Chacin --- pkg/api/api.go | 23 +++++++++++++++++------ pkg/api/api_test.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 6 deletions(-) diff --git a/pkg/api/api.go b/pkg/api/api.go index 382575d7..42b8db10 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -18,15 +18,26 @@ func NewPodDisruptor(rt *goja.Runtime, c goja.ConstructorCall, k8s kubernetes.Ku } selector := disruptors.PodSelector{} - err := rt.ExportTo(c.Argument(0), &selector) + err := IsCompatible(c.Argument(0).Export(), selector) if err != nil { - return nil, fmt.Errorf("PodDisruptor constructor expects PodSelector as argument: %w", err) + return nil, fmt.Errorf("invalid PodSelector: %w", err) + } + err = rt.ExportTo(c.Argument(0), &selector) + if err != nil { + return nil, fmt.Errorf("invalid PodSelector: %w", err) } options := disruptors.PodDisruptorOptions{} - err = rt.ExportTo(c.Argument(1), &options) - if err != nil { - return nil, fmt.Errorf("PodDisruptor constructor expects PodDisruptorOptions as second argument: %w", err) + // options argument is optional + if len(c.Arguments) > 1 { + err = IsCompatible(c.Argument(1).Export(), options) + if err != nil { + return nil, fmt.Errorf("invalid PodDisruptorOptions: %w", err) + } + err = rt.ExportTo(c.Argument(1), &options) + if err != nil { + return nil, fmt.Errorf("invalid PodDisruptorOptions: %w", err) + } } disruptor, err := disruptors.NewPodDisruptor(k8s, selector, options) @@ -66,7 +77,7 @@ func NewServiceDisruptor(rt *goja.Runtime, c goja.ConstructorCall, k8s kubernete if len(c.Arguments) > 2 { err = rt.ExportTo(c.Argument(2), &options) if err != nil { - return nil, fmt.Errorf("ServiceDisruptor constructor expects ServiceDisruptorOptions as third argument: %w", err) + return nil, fmt.Errorf("invalid ServiceDisruptorOptions: %w", err) } } diff --git a/pkg/api/api_test.go b/pkg/api/api_test.go index 3b00e762..4d65fe8c 100644 --- a/pkg/api/api_test.go +++ b/pkg/api/api_test.go @@ -99,6 +99,37 @@ func Test_PodDisruptorConstructor(t *testing.T) { `, expectError: true, }, + { + description: "invalid constructor with malformed selector", + script: ` + const selector = { + namespace: "namespace", + labels: { + app: "app" + } + } + new PodDisruptor(selector) + `, + expectError: true, + }, + { + description: "invalid constructor with malformed options", + script: ` + const selector = { + namespace: "namespace", + select: { + labels: { + app: "app" + } + } + } + const opts = { + timeout: 0 + } + new PodDisruptor(selector, opts) + `, + expectError: true, + }, } for _, tc := range testCases { From 5800677fed0007198eb96cc0b374432df2a6c271 Mon Sep 17 00:00:00 2001 From: Pablo Chacin Date: Thu, 22 Dec 2022 17:50:42 +0100 Subject: [PATCH 12/15] Add validations to ServiceDisruptor constructor Signed-off-by: Pablo Chacin --- pkg/api/api.go | 4 ++++ pkg/api/api_test.go | 10 ++++++++++ 2 files changed, 14 insertions(+) diff --git a/pkg/api/api.go b/pkg/api/api.go index 42b8db10..dd35bac3 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -75,6 +75,10 @@ func NewServiceDisruptor(rt *goja.Runtime, c goja.ConstructorCall, k8s kubernete options := disruptors.ServiceDisruptorOptions{} // options argument is optional if len(c.Arguments) > 2 { + err = IsCompatible(c.Argument(2).Export(), options) + if err != nil { + return nil, fmt.Errorf("invalid ServiceDisruptorOptions: %w", err) + } err = rt.ExportTo(c.Argument(2), &options) if err != nil { return nil, fmt.Errorf("invalid ServiceDisruptorOptions: %w", err) diff --git a/pkg/api/api_test.go b/pkg/api/api_test.go index 4d65fe8c..8ba2330b 100644 --- a/pkg/api/api_test.go +++ b/pkg/api/api_test.go @@ -231,6 +231,16 @@ func Test_ServiceDisruptorConstructor(t *testing.T) { `, expectError: true, }, + { + description: "valid constructor malformed options", + script: ` + const opts = { + timeout: 0 + } + new ServiceDisruptor("service", "default", opts) + `, + expectError: true, + }, } for _, tc := range testCases { From 0b9e69b64f980fe1c585837516cc531b0f91d035 Mon Sep 17 00:00:00 2001 From: Pablo Chacin Date: Fri, 23 Dec 2022 15:47:39 +0100 Subject: [PATCH 13/15] Add validations to PodDisruptor methods Signed-off-by: Pablo Chacin --- pkg/api/api.go | 99 +++++++++++++++++++++++++++---- pkg/api/api_test.go | 140 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 222 insertions(+), 17 deletions(-) diff --git a/pkg/api/api.go b/pkg/api/api.go index dd35bac3..753bbfca 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -9,8 +9,88 @@ import ( "github.com/dop251/goja" "github.com/grafana/xk6-disruptor/pkg/disruptors" "github.com/grafana/xk6-disruptor/pkg/kubernetes" + "go.k6.io/k6/js/common" ) +// converts a goja.Value into a object. target is expected to be a pointer +func convertValue(rt *goja.Runtime, value goja.Value, target interface{}) error { + // get the value pointed to by the target and check for compatibility + err := IsCompatible(value.Export(), reflect.ValueOf(target).Elem().Interface()) + if err != nil { + return err + } + + err = rt.ExportTo(value, target) + return err +} + +// JsPodDisruptor implements the JS interface for PodDisruptor +type JsPodDisruptor struct { + rt *goja.Runtime + disruptor disruptors.PodDisruptor +} + +// Targets is a proxy method. Validates parameters and delegates to the PodDisruptor method +func (p *JsPodDisruptor) Targets() goja.Value { + targets, err := p.disruptor.Targets() + if err != nil { + common.Throw(p.rt, fmt.Errorf("error getting kubernetes config path: %w", err)) + } + + return p.rt.ToValue(targets) +} + +// InjectHTTPFaults is a proxy method. Validates parameters and delegates to the PodDisruptor method +func (p *JsPodDisruptor) InjectHTTPFaults(args ...goja.Value) { + if len(args) < 2 { + common.Throw(p.rt, fmt.Errorf("HTTPFault and duration are required")) + } + + fault := disruptors.HTTPFault{} + err := convertValue(p.rt, args[0], &fault) + if err != nil { + common.Throw(p.rt, fmt.Errorf("invalid fault argument: %w", err)) + } + + duration := args[1].ToInteger() + if duration < 0 { + common.Throw(p.rt, fmt.Errorf("duration must be non-negative")) + } + + opts := disruptors.HTTPDisruptionOptions{} + if len(args) > 2 { + err = convertValue(p.rt, args[2], &opts) + if err != nil { + common.Throw(p.rt, fmt.Errorf("invalid options argument: %w", err)) + } + } + + err = p.disruptor.InjectHTTPFaults(fault, uint(duration), opts) + if err != nil { + common.Throw(p.rt, fmt.Errorf("error injecting fault: %w", err)) + } +} + +func buildJsPodDisruptor(rt *goja.Runtime, disruptor disruptors.PodDisruptor) (*goja.Object, error) { + jsDisruptor := JsPodDisruptor{ + rt: rt, + disruptor: disruptor, + } + + obj := rt.NewObject() + err := obj.Set("targets", jsDisruptor.Targets) + if err != nil { + return nil, err + } + + err = obj.Set("injectHTTPFaults", jsDisruptor.InjectHTTPFaults) + if err != nil { + return nil, err + } + + return obj, nil +} + // NewPodDisruptor creates an instance of a PodDisruptor func NewPodDisruptor(rt *goja.Runtime, c goja.ConstructorCall, k8s kubernetes.Kubernetes) (*goja.Object, error) { if c.Argument(0).Equals(goja.Null()) { @@ -18,11 +98,7 @@ func NewPodDisruptor(rt *goja.Runtime, c goja.ConstructorCall, k8s kubernetes.Ku } selector := disruptors.PodSelector{} - err := IsCompatible(c.Argument(0).Export(), selector) - if err != nil { - return nil, fmt.Errorf("invalid PodSelector: %w", err) - } - err = rt.ExportTo(c.Argument(0), &selector) + err := convertValue(rt, c.Argument(0), &selector) if err != nil { return nil, fmt.Errorf("invalid PodSelector: %w", err) } @@ -30,11 +106,7 @@ func NewPodDisruptor(rt *goja.Runtime, c goja.ConstructorCall, k8s kubernetes.Ku options := disruptors.PodDisruptorOptions{} // options argument is optional if len(c.Arguments) > 1 { - err = IsCompatible(c.Argument(1).Export(), options) - if err != nil { - return nil, fmt.Errorf("invalid PodDisruptorOptions: %w", err) - } - err = rt.ExportTo(c.Argument(1), &options) + err = convertValue(rt, c.Argument(1), &options) if err != nil { return nil, fmt.Errorf("invalid PodDisruptorOptions: %w", err) } @@ -45,7 +117,12 @@ func NewPodDisruptor(rt *goja.Runtime, c goja.ConstructorCall, k8s kubernetes.Ku return nil, fmt.Errorf("error creating PodDisruptor: %w", err) } - return rt.ToValue(disruptor).ToObject(rt), nil + obj, err := buildJsPodDisruptor(rt, disruptor) + if err != nil { + return nil, fmt.Errorf("error creating PodDisruptor: %w", err) + } + + return obj, nil } // NewServiceDisruptor creates an instance of a ServiceDisruptor diff --git a/pkg/api/api_test.go b/pkg/api/api_test.go index 8ba2330b..a9cd8795 100644 --- a/pkg/api/api_test.go +++ b/pkg/api/api_test.go @@ -151,7 +151,7 @@ func Test_PodDisruptorConstructor(t *testing.T) { return } - value, err := env.rt.RunString(tc.script) + _, err = env.rt.RunString(tc.script) if !tc.expectError && err != nil { t.Errorf("failed %v", err) @@ -162,16 +162,144 @@ func Test_PodDisruptorConstructor(t *testing.T) { t.Errorf("should had failed") return } + }) + } +} - // failed and it was expected, it is ok - if tc.expectError && err != nil { +const setupPodDisruptor = ` + const selector = { + namespace: "namespace", + select: { + labels: { + app: "app" + } + } +} +const opts = { + injectTimeout: 0 +} +const d = new PodDisruptor(selector, opts) +` + +func Test_JsPodDisruptor(t *testing.T) { + t.Parallel() + + testCases := []struct { + description string + script string + expectError bool + }{ + { + description: "get targets", + script: ` + d.targets() + `, + expectError: false, + }, + { + description: "inject HTTP Fault with full arguments", + script: ` + const fault = { + errorRate: 1.0, + errorCode: 500, + averageDelay: 100, + delayVariation: 10, + errorBody: '', + exclude: "", + port: 80 + } + + const faultOpts = { + proxyPort: 8080, + iface: "eth0" + } + + d.injectHTTPFaults(fault, 1, faultOpts) + `, + expectError: false, + }, + { + description: "inject HTTP Fault without options", + script: ` + const fault = { + errorRate: 1.0, + errorCode: 500, + averageDelay: 100, + delayVariation: 10, + errorBody: '', + exclude: "", + port: 80 + } + + d.injectHTTPFaults(fault, 1) + `, + expectError: false, + }, + { + description: "inject HTTP Fault without duration", + script: ` + const fault = { + errorRate: 1.0, + errorCode: 500, + averageDelay: 100, + delayVariation: 10, + errorBody: '', + exclude: "", + port: 80 + } + + d.injectHTTPFaults(fault) + `, + expectError: true, + }, + { + description: "inject HTTP Fault with malformed fault (misspelled field)", + script: ` + const fault = { + errorRate: 1.0, + error: 500, // this is should be 'errorCode' + } + + d.injectHTTPFaults(fault, 1) + `, + expectError: true, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.description, func(t *testing.T) { + t.Parallel() + + env, err := testSetup() + if err != nil { + t.Errorf("error in test setup %v", err) return } - var pd disruptors.PodDisruptor - err = env.rt.ExportTo(value, &pd) + err = env.registerConstructor("PodDisruptor", func(e *testEnv, c goja.ConstructorCall) (*goja.Object, error) { + return NewPodDisruptor(e.rt, c, e.k8s) + }) + if err != nil { + t.Errorf("error in test setup %v", err) + return + } + + _, err = env.rt.RunString(setupPodDisruptor) if err != nil { - t.Errorf("returned valued cannot be converted to PodDisruptor: %v", err) + t.Errorf("error in test setup %v", err) + return + } + + _, err = env.rt.RunString(tc.script) + + if !tc.expectError && err != nil { + t.Errorf("failed %v", err) + return + } + + if tc.expectError && err == nil { + t.Errorf("should had failed") return } }) From 2eb9f720d1e0d361da4047101e66fda50843ff6a Mon Sep 17 00:00:00 2001 From: Pablo Chacin Date: Fri, 23 Dec 2022 16:26:41 +0100 Subject: [PATCH 14/15] Eliminate duplicated method definitions Signed-off-by: Pablo Chacin --- pkg/disruptors/service.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/pkg/disruptors/service.go b/pkg/disruptors/service.go index 354c9d6c..5cbc9210 100644 --- a/pkg/disruptors/service.go +++ b/pkg/disruptors/service.go @@ -11,11 +11,7 @@ import ( // ServiceDisruptor defines operations for injecting faults in services type ServiceDisruptor interface { - // InjectHTTPFault injects faults in the http requests sent to the disruptor's target - // for the specified duration (in seconds) - InjectHTTPFaults(fault HTTPFault, duration uint, options HTTPDisruptionOptions) error - // Targets returns the list of targets for the disruptor - Targets() ([]string, error) + PodDisruptor } // ServiceDisruptorOptions defines options that controls the behavior of the ServiceDisruptor From 8ff506f716257f9790907839f03a394127d87f06 Mon Sep 17 00:00:00 2001 From: Pablo Chacin Date: Fri, 23 Dec 2022 17:35:01 +0100 Subject: [PATCH 15/15] Add validations to ServiceDisruptor methods Signed-off-by: Pablo Chacin --- pkg/api/api.go | 27 +++++++++++++-------------- pkg/api/api_test.go | 19 ++++--------------- 2 files changed, 17 insertions(+), 29 deletions(-) diff --git a/pkg/api/api.go b/pkg/api/api.go index 753bbfca..adfd7a6b 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -131,20 +131,14 @@ func NewServiceDisruptor(rt *goja.Runtime, c goja.ConstructorCall, k8s kubernete return nil, fmt.Errorf("ServiceDisruptor constructor requires service and namespace parameters") } - if c.Argument(0).ExportType().Kind() != reflect.String { - return nil, fmt.Errorf("ServiceDisruptor constructor expects service name to be a string") - } var service string - err := rt.ExportTo(c.Argument(0), &service) + err := convertValue(rt, c.Argument(0), &service) if err != nil { return nil, fmt.Errorf("invalid service name argument for ServiceDisruptor constructor: %w", err) } - if c.Argument(1).ExportType().Kind() != reflect.String { - return nil, fmt.Errorf("ServiceDisruptor constructor expects namespace to be a string") - } var namespace string - err = rt.ExportTo(c.Argument(1), &namespace) + err = convertValue(rt, c.Argument(1), &namespace) if err != nil { return nil, fmt.Errorf("invalid namespace argument for ServiceDisruptor constructor: %w", err) } @@ -152,11 +146,7 @@ func NewServiceDisruptor(rt *goja.Runtime, c goja.ConstructorCall, k8s kubernete options := disruptors.ServiceDisruptorOptions{} // options argument is optional if len(c.Arguments) > 2 { - err = IsCompatible(c.Argument(2).Export(), options) - if err != nil { - return nil, fmt.Errorf("invalid ServiceDisruptorOptions: %w", err) - } - err = rt.ExportTo(c.Argument(2), &options) + err = convertValue(rt, c.Argument(2), &options) if err != nil { return nil, fmt.Errorf("invalid ServiceDisruptorOptions: %w", err) } @@ -167,5 +157,14 @@ func NewServiceDisruptor(rt *goja.Runtime, c goja.ConstructorCall, k8s kubernete return nil, fmt.Errorf("error creating ServiceDisruptor: %w", err) } - return rt.ToValue(disruptor).ToObject(rt), nil + // ServiceDisruptor is a wrapper to PodDisruptor, so we can use it for building a JsPodDisruptor. + // Notice that when [1] is implemented, this will make even more sense because there will be only + // a Disruptor interface. + // [1] https://github.com/grafana/xk6-disruptor/issues/60 + obj, err := buildJsPodDisruptor(rt, disruptor) + if err != nil { + return nil, fmt.Errorf("error creating ServiceDisruptor: %w", err) + } + + return obj, nil } diff --git a/pkg/api/api_test.go b/pkg/api/api_test.go index a9cd8795..cebbde7a 100644 --- a/pkg/api/api_test.go +++ b/pkg/api/api_test.go @@ -6,7 +6,6 @@ import ( "testing" "github.com/dop251/goja" - "github.com/grafana/xk6-disruptor/pkg/disruptors" "github.com/grafana/xk6-disruptor/pkg/kubernetes" "github.com/grafana/xk6-disruptor/pkg/testutils/kubernetes/builders" "go.k6.io/k6/js/common" @@ -181,6 +180,8 @@ const opts = { const d = new PodDisruptor(selector, opts) ` +// This function tests covers both PodDisruptor and ServiceDisruptor because +// they are wrapped by the same methods in the API. func Test_JsPodDisruptor(t *testing.T) { t.Parallel() @@ -259,7 +260,7 @@ func Test_JsPodDisruptor(t *testing.T) { errorRate: 1.0, error: 500, // this is should be 'errorCode' } - + d.injectHTTPFaults(fault, 1) `, expectError: true, @@ -393,7 +394,7 @@ func Test_ServiceDisruptorConstructor(t *testing.T) { svc := builders.NewServiceBuilder("service").Build() _, _ = env.k8s.CoreV1().Services("default").Create(context.TODO(), svc, v1.CreateOptions{}) - value, err := env.rt.RunString(tc.script) + _, err = env.rt.RunString(tc.script) if !tc.expectError && err != nil { t.Errorf("failed %v", err) @@ -404,18 +405,6 @@ func Test_ServiceDisruptorConstructor(t *testing.T) { t.Errorf("should had failed") return } - - // failed and it was expected, it is ok - if tc.expectError && err != nil { - return - } - - var sd disruptors.ServiceDisruptor - err = env.rt.ExportTo(value, &sd) - if err != nil { - t.Errorf("returned valued cannot be converted to ServiceDisruptor: %v", err) - return - } }) } }