From 4a83a273cc9763b1269dcec1eeac6141b7f8b70d Mon Sep 17 00:00:00 2001 From: Evan Anderson Date: Thu, 16 Jan 2025 14:58:11 -0800 Subject: [PATCH 1/2] Datasource cleanup: introduce some types and avoid pass-by-context --- internal/datasources/rest/handler.go | 25 +++++-------- internal/datasources/rest/handler_test.go | 32 ++--------------- internal/datasources/structured/handler.go | 18 ++++------ .../datasources/structured/handler_test.go | 35 +++++++------------ internal/engine/eval/rego/datasources.go | 13 ++----- internal/engine/eval/rego/rego_test.go | 4 +-- pkg/datasources/v1/datasources.go | 7 ++-- pkg/datasources/v1/mock/datasources.go | 16 +++++---- 8 files changed, 46 insertions(+), 104 deletions(-) diff --git a/internal/datasources/rest/handler.go b/internal/datasources/rest/handler.go index 06a9045283..ce3edb2bdf 100644 --- a/internal/datasources/rest/handler.go +++ b/internal/datasources/rest/handler.go @@ -22,6 +22,7 @@ import ( "github.com/mindersec/minder/internal/util/schemaupdate" "github.com/mindersec/minder/internal/util/schemavalidate" minderv1 "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1" + "github.com/mindersec/minder/pkg/engine/v1/interfaces" ) const ( @@ -69,7 +70,7 @@ func newHandlerFromDef(def *minderv1.RestDataSource_Def) (*restHandler, error) { }, nil } -func (h *restHandler) GetArgsSchema() any { +func (h *restHandler) GetArgsSchema() *structpb.Struct { return h.rawInputSchema } @@ -86,28 +87,18 @@ func (h *restHandler) ValidateArgs(args any) error { return schemavalidate.ValidateAgainstSchema(h.inputSchema, mapobj) } -func (h *restHandler) ValidateUpdate(obj any) error { - if obj == nil { +func (h *restHandler) ValidateUpdate(argsSchema *structpb.Struct) error { + if argsSchema == nil { return errors.New("update schema cannot be nil") } - switch castedobj := obj.(type) { - case *structpb.Struct: - if _, err := schemavalidate.CompileSchemaFromPB(castedobj); err != nil { - return fmt.Errorf("update validation failed due to invalid schema: %w", err) - } - return schemaupdate.ValidateSchemaUpdate(h.rawInputSchema, castedobj) - case map[string]any: - if _, err := schemavalidate.CompileSchemaFromMap(castedobj); err != nil { - return fmt.Errorf("update validation failed due to invalid schema: %w", err) - } - return schemaupdate.ValidateSchemaUpdateMap(h.rawInputSchema.AsMap(), castedobj) - default: - return errors.New("invalid type") + if _, err := schemavalidate.CompileSchemaFromPB(argsSchema); err != nil { + return fmt.Errorf("update validation failed due to invalid schema: %w", err) } + return schemaupdate.ValidateSchemaUpdate(h.rawInputSchema, argsSchema) } -func (h *restHandler) Call(ctx context.Context, args any) (any, error) { +func (h *restHandler) Call(ctx context.Context, _ *interfaces.Result, args any) (any, error) { argsMap, ok := args.(map[string]any) if !ok { return nil, errors.New("args is not a map") diff --git a/internal/datasources/rest/handler_test.go b/internal/datasources/rest/handler_test.go index a9616816e6..fdea1eb7d6 100644 --- a/internal/datasources/rest/handler_test.go +++ b/internal/datasources/rest/handler_test.go @@ -252,7 +252,7 @@ func Test_restHandler_Call(t *testing.T) { headers: tt.fields.headers, parse: tt.fields.parse, } - got, err := h.Call(context.Background(), tt.args.args) + got, err := h.Call(context.Background(), nil, tt.args.args) if tt.wantErr { assert.Error(t, err) } else { @@ -367,7 +367,7 @@ func Test_restHandler_ValidateUpdate(t *testing.T) { t.Parallel() type args struct { - updateSchema any + updateSchema *structpb.Struct } tests := []struct { name string @@ -408,34 +408,6 @@ func Test_restHandler_ValidateUpdate(t *testing.T) { }, wantErr: false, }, - { - name: "Valid map[string]any", - inputSchema: map[string]any{ - "type": "object", - "properties": map[string]any{"key": map[string]any{"type": "string"}}, - }, - args: args{ - updateSchema: map[string]any{ - "type": "object", - "properties": map[string]any{ - "key": map[string]any{"type": "string"}, - "new_key": map[string]any{"type": "number"}, - }, - }, - }, - wantErr: false, - }, - { - name: "Invalid type", - inputSchema: map[string]any{ - "type": "object", - "properties": map[string]any{"key": map[string]any{"type": "string"}}, - }, - args: args{ - updateSchema: "invalid_type", - }, - wantErr: true, - }, { name: "nil update schema", inputSchema: map[string]any{ diff --git a/internal/datasources/structured/handler.go b/internal/datasources/structured/handler.go index aeb7cb6afe..ba84751346 100644 --- a/internal/datasources/structured/handler.go +++ b/internal/datasources/structured/handler.go @@ -15,9 +15,11 @@ import ( "github.com/go-git/go-billy/v5" "github.com/rs/zerolog/log" + "google.golang.org/protobuf/types/known/structpb" minderv1 "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1" v1datasources "github.com/mindersec/minder/pkg/datasources/v1" + "github.com/mindersec/minder/pkg/engine/v1/interfaces" ) const ( @@ -148,21 +150,15 @@ func parseFile(f billy.File) (any, error) { } // Call parses the structured data from the billy filesystem in the context -func (sh *structHandler) Call(ctx context.Context, _ any) (any, error) { - var ctxData v1datasources.Context - var ok bool - if ctxData, ok = ctx.Value(v1datasources.ContextKey{}).(v1datasources.Context); !ok { - return nil, fmt.Errorf("unable to read execution context") - } - - if ctxData.Ingest == nil || ctxData.Ingest.Fs == nil { +func (sh *structHandler) Call(ctx context.Context, ingest *interfaces.Result, _ any) (any, error) { + if ingest == nil || ingest.Fs == nil { return nil, fmt.Errorf("filesystem not found in execution context") } - return parseFileAlternatives(ctxData.Ingest.Fs, sh.Path.GetFileName(), sh.Path.GetAlternatives()) + return parseFileAlternatives(ingest.Fs, sh.Path.GetFileName(), sh.Path.GetAlternatives()) } -func (*structHandler) GetArgsSchema() any { +func (*structHandler) GetArgsSchema() *structpb.Struct { return nil } @@ -172,6 +168,6 @@ func (_ *structHandler) ValidateArgs(any) error { } // ValidateUpdate -func (_ *structHandler) ValidateUpdate(any) error { +func (_ *structHandler) ValidateUpdate(*structpb.Struct) error { return nil } diff --git a/internal/datasources/structured/handler_test.go b/internal/datasources/structured/handler_test.go index c20f4e1765..5fb3109550 100644 --- a/internal/datasources/structured/handler_test.go +++ b/internal/datasources/structured/handler_test.go @@ -14,7 +14,6 @@ import ( "github.com/stretchr/testify/require" minderv1 "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1" - v1datasources "github.com/mindersec/minder/pkg/datasources/v1" "github.com/mindersec/minder/pkg/engine/v1/interfaces" ) @@ -166,25 +165,19 @@ func TestNew(t *testing.T) { func TestCall(t *testing.T) { t.Parallel() for _, tc := range []struct { - name string - buildContext func(t *testing.T) context.Context - def *minderv1.StructDataSource_Def - mustErr bool + name string + ingest func(t *testing.T) *interfaces.Result + def *minderv1.StructDataSource_Def + mustErr bool }{ { "success", - func(t *testing.T) context.Context { + func(t *testing.T) *interfaces.Result { t.Helper() fs := memfs.New() writeFSFile(t, fs, "./test1.json", []byte("{ \"a\": \"b\"}")) - return context.WithValue( - context.Background(), - v1datasources.ContextKey{}, - v1datasources.Context{ - Ingest: &interfaces.Result{Fs: fs}, - }, - ) + return &interfaces.Result{Fs: fs} }, &minderv1.StructDataSource_Def{ Path: &minderv1.StructDataSource_Def_Path{ @@ -195,31 +188,27 @@ func TestCall(t *testing.T) { }, { "no-datasource-context", - func(t *testing.T) context.Context { + func(t *testing.T) *interfaces.Result { t.Helper() - return context.Background() + return nil }, &minderv1.StructDataSource_Def{}, true, }, {"ctx-no-fs", - func(t *testing.T) context.Context { + func(t *testing.T) *interfaces.Result { t.Helper() - return context.WithValue( - context.Background(), - v1datasources.ContextKey{}, - v1datasources.Context{}, - ) + return &interfaces.Result{} }, &minderv1.StructDataSource_Def{}, true}, } { t.Run(tc.name, func(t *testing.T) { t.Parallel() - ctx := tc.buildContext(t) + ingest := tc.ingest(t) handler, err := newHandlerFromDef(tc.def) require.NoError(t, err) - _, err = handler.Call(ctx, []string{}) + _, err = handler.Call(context.Background(), ingest, []string{}) if tc.mustErr { require.Error(t, err) return diff --git a/internal/engine/eval/rego/datasources.go b/internal/engine/eval/rego/datasources.go index 8e9398b925..4054bc8099 100644 --- a/internal/engine/eval/rego/datasources.go +++ b/internal/engine/eval/rego/datasources.go @@ -4,7 +4,6 @@ package rego import ( - "context" "fmt" "strings" @@ -48,7 +47,7 @@ func buildFromDataSource( Name: k, Decl: types.NewFunction(types.Args(types.A), types.A), }, - func(_ rego.BuiltinContext, obj *ast.Term) (*ast.Term, error) { + func(bctx rego.BuiltinContext, obj *ast.Term) (*ast.Term, error) { // Convert the AST value back to a Go interface{} jsonObj, err := ast.JSON(obj.Value) if err != nil { @@ -59,15 +58,7 @@ func buildFromDataSource( return nil, err } - // Call the data source function - ctx := context.WithValue( - context.Background(), - v1datasources.ContextKey{}, - v1datasources.Context{ - Ingest: res, - }, - ) - ret, err := dsf.Call(ctx, jsonObj) + ret, err := dsf.Call(bctx.Context, res, jsonObj) if err != nil { return nil, err } diff --git a/internal/engine/eval/rego/rego_test.go b/internal/engine/eval/rego/rego_test.go index 3954f0b5d8..c38d72d596 100644 --- a/internal/engine/eval/rego/rego_test.go +++ b/internal/engine/eval/rego/rego_test.go @@ -522,7 +522,7 @@ allow { emptyPol := map[string]any{} // Matches - fdsf.EXPECT().Call(gomock.Any(), gomock.Any()).Return("foo", nil) + fdsf.EXPECT().Call(gomock.Any(), gomock.Any(), gomock.Any()).Return("foo", nil) _, err = e.Eval(context.Background(), emptyPol, nil, &interfaces.Result{ Object: map[string]any{ "data": "foo", @@ -531,7 +531,7 @@ allow { require.NoError(t, err, "could not evaluate") // Doesn't match - fdsf.EXPECT().Call(gomock.Any(), gomock.Any()).Return("bar", nil) + fdsf.EXPECT().Call(gomock.Any(), gomock.Any(), gomock.Any()).Return("bar", nil) _, err = e.Eval(context.Background(), emptyPol, nil, &interfaces.Result{ Object: map[string]any{ "data": "bar", diff --git a/pkg/datasources/v1/datasources.go b/pkg/datasources/v1/datasources.go index c5bbc6c5cb..0ddf22a47d 100644 --- a/pkg/datasources/v1/datasources.go +++ b/pkg/datasources/v1/datasources.go @@ -8,6 +8,7 @@ import ( "context" "github.com/mindersec/minder/pkg/engine/v1/interfaces" + "google.golang.org/protobuf/types/known/structpb" ) //go:generate go run go.uber.org/mock/mockgen -package mock_$GOPACKAGE -destination=./mock/$GOFILE -source=./$GOFILE @@ -36,14 +37,14 @@ type DataSourceFuncDef interface { // ValidateUpdate validates the update to the data source. // The data source implementation should respect the update and return an error // if the update is invalid. - ValidateUpdate(obj any) error + ValidateUpdate(obj *structpb.Struct) error // Call calls the function with the given arguments. // It is the responsibility of the data source implementation to handle the call. // It is also the responsibility of the caller to validate the arguments // before calling the function. - Call(ctx context.Context, args any) (any, error) + Call(ctx context.Context, ingest *interfaces.Result, args any) (any, error) // GetArgsSchema returns the schema of the arguments. - GetArgsSchema() any + GetArgsSchema() *structpb.Struct } // DataSource is the interface that a data source must implement. diff --git a/pkg/datasources/v1/mock/datasources.go b/pkg/datasources/v1/mock/datasources.go index 596e851c76..1992c1667e 100644 --- a/pkg/datasources/v1/mock/datasources.go +++ b/pkg/datasources/v1/mock/datasources.go @@ -14,7 +14,9 @@ import ( reflect "reflect" v1 "github.com/mindersec/minder/pkg/datasources/v1" + interfaces "github.com/mindersec/minder/pkg/engine/v1/interfaces" gomock "go.uber.org/mock/gomock" + structpb "google.golang.org/protobuf/types/known/structpb" ) // MockDataSourceFuncDef is a mock of DataSourceFuncDef interface. @@ -42,25 +44,25 @@ func (m *MockDataSourceFuncDef) EXPECT() *MockDataSourceFuncDefMockRecorder { } // Call mocks base method. -func (m *MockDataSourceFuncDef) Call(ctx context.Context, args any) (any, error) { +func (m *MockDataSourceFuncDef) Call(ctx context.Context, ingest *interfaces.Result, args any) (any, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Call", ctx, args) + ret := m.ctrl.Call(m, "Call", ctx, ingest, args) ret0, _ := ret[0].(any) ret1, _ := ret[1].(error) return ret0, ret1 } // Call indicates an expected call of Call. -func (mr *MockDataSourceFuncDefMockRecorder) Call(ctx, args any) *gomock.Call { +func (mr *MockDataSourceFuncDefMockRecorder) Call(ctx, ingest, args any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Call", reflect.TypeOf((*MockDataSourceFuncDef)(nil).Call), ctx, args) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Call", reflect.TypeOf((*MockDataSourceFuncDef)(nil).Call), ctx, ingest, args) } // GetArgsSchema mocks base method. -func (m *MockDataSourceFuncDef) GetArgsSchema() any { +func (m *MockDataSourceFuncDef) GetArgsSchema() *structpb.Struct { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetArgsSchema") - ret0, _ := ret[0].(any) + ret0, _ := ret[0].(*structpb.Struct) return ret0 } @@ -85,7 +87,7 @@ func (mr *MockDataSourceFuncDefMockRecorder) ValidateArgs(obj any) *gomock.Call } // ValidateUpdate mocks base method. -func (m *MockDataSourceFuncDef) ValidateUpdate(obj any) error { +func (m *MockDataSourceFuncDef) ValidateUpdate(obj *structpb.Struct) error { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ValidateUpdate", obj) ret0, _ := ret[0].(error) From 87aaa5fa87531a60a3dda3c0797b731e3212e1ca Mon Sep 17 00:00:00 2001 From: Evan Anderson Date: Fri, 17 Jan 2025 06:37:16 -0800 Subject: [PATCH 2/2] Fix lint --- internal/datasources/structured/handler.go | 2 +- pkg/datasources/v1/datasources.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/datasources/structured/handler.go b/internal/datasources/structured/handler.go index ba84751346..d89f8f204e 100644 --- a/internal/datasources/structured/handler.go +++ b/internal/datasources/structured/handler.go @@ -150,7 +150,7 @@ func parseFile(f billy.File) (any, error) { } // Call parses the structured data from the billy filesystem in the context -func (sh *structHandler) Call(ctx context.Context, ingest *interfaces.Result, _ any) (any, error) { +func (sh *structHandler) Call(_ context.Context, ingest *interfaces.Result, _ any) (any, error) { if ingest == nil || ingest.Fs == nil { return nil, fmt.Errorf("filesystem not found in execution context") } diff --git a/pkg/datasources/v1/datasources.go b/pkg/datasources/v1/datasources.go index 0ddf22a47d..d45d43b595 100644 --- a/pkg/datasources/v1/datasources.go +++ b/pkg/datasources/v1/datasources.go @@ -7,8 +7,9 @@ package v1 import ( "context" - "github.com/mindersec/minder/pkg/engine/v1/interfaces" "google.golang.org/protobuf/types/known/structpb" + + "github.com/mindersec/minder/pkg/engine/v1/interfaces" ) //go:generate go run go.uber.org/mock/mockgen -package mock_$GOPACKAGE -destination=./mock/$GOFILE -source=./$GOFILE