fix: Add a safety net in adhoc's defaultRunnerFactory (#1268)

This is trying to test for possible mistakes in setting up ad-hoc checks
in the future.

Signed-off-by: Marcelo E. Magallon <[email protected]>

Author: mem

Diff for: internal/adhoc/adhoc.go

@@ -51,10 +51,13 @@ type Error string
 func (e Error) Error() string { return string(e) }
 
 const (
-	errNotAuthorized     = Error("probe not authorized")
-	errTransportClosing  = Error("transport closing")
-	errProbeUnregistered = Error("probe no longer registered")
-	errIncompatibleApi   = Error("API does not support required features")
+	errNotAuthorized       = Error("probe not authorized")
+	errTransportClosing    = Error("transport closing")
+	errProbeUnregistered   = Error("probe no longer registered")
+	errIncompatibleApi     = Error("API does not support required features")
+	errInvalidAdHocRequest = Error("invalid ad-hoc request")
+
+	k6AdhocGraceTime = 20 * time.Second
 )
 
 type runner struct {
@@ -358,6 +361,10 @@ func (h *Handler) handleAdHocCheck(ctx context.Context, ahReq *sm.AdHocRequest)
 
 	runner, err := h.runnerFactory(ctx, ahReq)
 	if err != nil {
+		// TODO(mem): if the runner factory returns an error, we should
+		// create a result that reflects that and publish it, so that
+		// the frontend is able to communicate to the user that
+		// something went wrong.
 		return err
 	}
 
@@ -382,12 +389,31 @@ func defaultGrpcAdhocChecksClientFactory(conn ClientConn) (sm.AdHocChecksClient,
 }
 
 func (h *Handler) defaultRunnerFactory(ctx context.Context, req *sm.AdHocRequest) (*runner, error) {
+	// This should never happen. If we hit this, it's a bug in the code
+	// that is handling the request, or the API sent us an invalid request.
+	if req == nil || req.AdHocCheck.TenantId == 0 {
+		return nil, errInvalidAdHocRequest
+	}
+
 	check := model.Check{
 		Check: sm.Check{
 			TenantId: req.AdHocCheck.TenantId,
 			Target:   req.AdHocCheck.Target,
 			Timeout:  req.AdHocCheck.Timeout,
 			Settings: req.AdHocCheck.Settings,
+
+			// All the following fields are not used for ad-hoc checks.
+			Id:               0, // ad-hoc checks don't have an ID in this sense.
+			Job:              "",
+			Frequency:        0,
+			Offset:           0,
+			Enabled:          true,
+			Labels:           nil,
+			Probes:           nil,
+			BasicMetricsOnly: false,
+			AlertSensitivity: "",
+			Created:          0,
+			Modified:         0,
 		},
 	}
 
@@ -402,7 +428,6 @@ func (h *Handler) defaultRunnerFactory(ctx context.Context, req *sm.AdHocRequest
 	timeout := time.Duration(check.Timeout) * time.Millisecond
 	switch check.Type() {
 	case sm.CheckTypeMultiHttp, sm.CheckTypeScripted, sm.CheckTypeBrowser:
-		const k6AdhocGraceTime = 20 * time.Second
 		timeout += k6AdhocGraceTime
 	}
 

Diff for: internal/adhoc/adhoc_test.go

@@ -2,6 +2,7 @@ package adhoc
 
 import (
 	"context"
+	"errors"
 	"io"
 	"os"
 	"testing"
@@ -17,6 +18,9 @@ import (
 	"google.golang.org/grpc/status"
 
 	"github.com/grafana/synthetic-monitoring-agent/internal/feature"
+	"github.com/grafana/synthetic-monitoring-agent/internal/k6runner"
+	"github.com/grafana/synthetic-monitoring-agent/internal/model"
+	"github.com/grafana/synthetic-monitoring-agent/internal/prober"
 	"github.com/grafana/synthetic-monitoring-agent/internal/prober/logger"
 	"github.com/grafana/synthetic-monitoring-agent/internal/pusher"
 	sm "github.com/grafana/synthetic-monitoring-agent/pkg/pb/synthetic_monitoring"
@@ -347,3 +351,255 @@ func (p *testProber) Probe(ctx context.Context, target string, registry *prometh
 	_ = logger.Log("msg", "test")
 	return true, 1
 }
+
+// Add mock secrets store
+type testSecretStore struct{}
+
+func (s testSecretStore) GetSecretCredentials(ctx context.Context, tenantId model.GlobalID) (*sm.SecretStore, error) {
+	if tenantId == 0 {
+		return nil, errors.New("invalid tenant ID")
+	}
+
+	return &sm.SecretStore{
+		Url:   "http://example.com",
+		Token: "test-token",
+	}, nil
+}
+
+func TestDefaultRunnerFactory(t *testing.T) {
+	t.Parallel()
+
+	features := feature.NewCollection()
+	require.NoError(t, features.Set(feature.K6))
+
+	logger := zerolog.New(io.Discard)
+	if testing.Verbose() {
+		logger = zerolog.New(os.Stdout)
+	}
+
+	// Initialize the mockRunner and secretStore
+	mockRunner := &testK6Runner{}
+	secretStore := &testSecretStore{}
+
+	testcases := map[string]struct {
+		request     *sm.AdHocRequest
+		expectError bool
+		errCheck    func(error) bool
+		shouldPanic bool
+	}{
+		"ping check": {
+			request: &sm.AdHocRequest{
+				AdHocCheck: sm.AdHocCheck{
+					Id:       "test-ping",
+					TenantId: 1000,
+					Target:   "example.com",
+					Timeout:  1000,
+					Settings: sm.CheckSettings{
+						Ping: &sm.PingSettings{},
+					},
+				},
+			},
+		},
+		"http check": {
+			request: &sm.AdHocRequest{
+				AdHocCheck: sm.AdHocCheck{
+					Id:       "test-http",
+					TenantId: 1000,
+					Target:   "http://example.com",
+					Timeout:  1000,
+					Settings: sm.CheckSettings{
+						Http: &sm.HttpSettings{},
+					},
+				},
+			},
+		},
+		"dns check": {
+			request: &sm.AdHocRequest{
+				AdHocCheck: sm.AdHocCheck{
+					Id:       "test-dns",
+					TenantId: 1000,
+					Target:   "example.com",
+					Timeout:  1000,
+					Settings: sm.CheckSettings{
+						Dns: &sm.DnsSettings{},
+					},
+				},
+			},
+		},
+		"tcp check": {
+			request: &sm.AdHocRequest{
+				AdHocCheck: sm.AdHocCheck{
+					Id:       "test-tcp",
+					TenantId: 1000,
+					Target:   "example.com:80",
+					Timeout:  1000,
+					Settings: sm.CheckSettings{
+						Tcp: &sm.TcpSettings{},
+					},
+				},
+			},
+		},
+		"k6 scripted check": {
+			request: &sm.AdHocRequest{
+				AdHocCheck: sm.AdHocCheck{
+					Id:       "test-scripted",
+					TenantId: 1000,
+					Target:   "test-target",
+					Timeout:  1000,
+					Settings: sm.CheckSettings{
+						Scripted: &sm.ScriptedSettings{},
+					},
+				},
+			},
+		},
+		"k6 multihttp check": {
+			request: &sm.AdHocRequest{
+				AdHocCheck: sm.AdHocCheck{
+					Id:       "test-multihttp",
+					TenantId: 1000,
+					Target:   "test-target",
+					Timeout:  1000,
+					Settings: sm.CheckSettings{
+						Multihttp: &sm.MultiHttpSettings{
+							Entries: []*sm.MultiHttpEntry{
+								{
+									Request: &sm.MultiHttpEntryRequest{
+										Url: "http://example.com",
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+		"k6 browser check": {
+			request: &sm.AdHocRequest{
+				AdHocCheck: sm.AdHocCheck{
+					Id:       "test-browser",
+					TenantId: 1000,
+					Target:   "test-target",
+					Timeout:  1000,
+					Settings: sm.CheckSettings{
+						Browser: &sm.BrowserSettings{},
+					},
+				},
+			},
+		},
+		"zero timeout": {
+			request: &sm.AdHocRequest{
+				AdHocCheck: sm.AdHocCheck{
+					Id:       "test-zero-timeout",
+					TenantId: 1000,
+					Target:   "example.com",
+					Timeout:  0,
+					Settings: sm.CheckSettings{
+						Ping: &sm.PingSettings{},
+					},
+				},
+			},
+		},
+		"empty settings": {
+			request: &sm.AdHocRequest{
+				AdHocCheck: sm.AdHocCheck{
+					Id:       "test-empty-settings",
+					TenantId: 1000,
+					Target:   "example.com",
+					Timeout:  1000,
+					Settings: sm.CheckSettings{},
+				},
+			},
+			expectError: true,
+			shouldPanic: true,
+		},
+		"nil request": {
+			request:     nil,
+			expectError: true,
+			errCheck: func(err error) bool {
+				return errors.Is(err, errInvalidAdHocRequest)
+			},
+		},
+		"zero tenant": {
+			request: &sm.AdHocRequest{
+				AdHocCheck: sm.AdHocCheck{
+					Id:       "test-zero-tenant",
+					TenantId: 0,
+					Target:   "example.com",
+					Timeout:  1000,
+					Settings: sm.CheckSettings{},
+				},
+			},
+			expectError: true,
+			errCheck: func(err error) bool {
+				return errors.Is(err, errInvalidAdHocRequest)
+			},
+		},
+	}
+
+	for name, tc := range testcases {
+		tc := tc
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			h := &Handler{
+				logger:   logger,
+				features: features,
+				probe: &sm.Probe{
+					Name: "test-probe",
+				},
+				proberFactory: prober.NewProberFactory(mockRunner, 0, features, secretStore),
+			}
+
+			ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+			defer cancel()
+
+			if tc.shouldPanic {
+				require.Panics(t, func() {
+					_, _ = h.defaultRunnerFactory(ctx, tc.request)
+				})
+				return
+			}
+
+			runner, err := h.defaultRunnerFactory(ctx, tc.request)
+			if tc.expectError {
+				require.Error(t, err)
+				if tc.errCheck != nil {
+					require.True(t, tc.errCheck(err), "unexpected error: %v", err)
+				}
+				require.Nil(t, runner)
+				return
+			}
+
+			require.NoError(t, err)
+			require.NotNil(t, runner)
+
+			// Verify runner fields
+			require.Equal(t, logger, runner.logger)
+			require.NotNil(t, runner.prober)
+			require.Equal(t, tc.request.AdHocCheck.Id, runner.id)
+			require.Equal(t, "test-probe", runner.probe)
+
+			// For k6-based checks, verify the grace period is added
+			switch tc.request.AdHocCheck.Type() {
+			case sm.CheckTypeMultiHttp, sm.CheckTypeScripted, sm.CheckTypeBrowser:
+				expectedTimeout := time.Duration(tc.request.AdHocCheck.Timeout)*time.Millisecond + k6AdhocGraceTime
+				require.Equal(t, expectedTimeout, runner.timeout)
+
+			default:
+				expectedTimeout := time.Duration(tc.request.AdHocCheck.Timeout) * time.Millisecond
+				require.Equal(t, expectedTimeout, runner.timeout)
+			}
+		})
+	}
+}
+
+// Add mock k6runner
+type testK6Runner struct{}
+
+func (r *testK6Runner) WithLogger(logger *zerolog.Logger) k6runner.Runner {
+	return r
+}
+
+func (r *testK6Runner) Run(ctx context.Context, script k6runner.Script) (*k6runner.RunResponse, error) {
+	return &k6runner.RunResponse{}, nil
+}