Skip to content

Commit 161ae18

Browse files
Gate rule type engine construction on provider_traits (#6702)
* Gate rule type engine construction on provider_traits Reject rule types at construction time when the provider does not implement all traits listed in the rule type's provider_traits, the same way entity-type mismatches are already handled up front rather than via a runtime ErrEvaluationSkipped. The trait check is declared as a package-local interface in rtengine rather than added to interfaces.Provider: that package cannot import minderv1 without creating an import cycle (minderv1 transitively imports pkg/datasources/v1, which imports pkg/engine/v1/interfaces). Every concrete provider already implements CanImplement with this exact signature, so they satisfy the interface structurally with no provider-side changes. * Add tests for provider_traits gating in rtengine Extend TestKit with a configurable CanImplement (via WithCanImplement), defaulting to true so existing callers are unaffected, and add cases to engine_test.go covering: no provider_traits (unaffected), all required traits satisfied (succeeds), and a missing trait (fails with a clear error). * Skip (not abort) rule types with unsatisfied provider_traits * Move DataSourceDriver constants to minderv1 to break import cycle * Widen interfaces.Provider with CanImplement, simplify provider_traits loop * Add provider_traits coverage to mindev integration tests * Replace provider_traits sentinel error with SupportedByProvider RuleTypeEngine now computes whether the provider satisfies the rule type's provider_traits once at construction time and exposes it via SupportedByProvider(), instead of NewRuleTypeEngine returning a wrapped sentinel error that callers had to unwrap with errors.Is. The rule engine cache no longer excludes unsatisfied rule types from its population loop, so every rule type in the hierarchy is cached regardless of applicability. The executor checks SupportedByProvider() before evaluating a rule and, if false, returns early ahead of createOrUpdateEvalStatus so no eval status row is written for a rule type that isn't applicable to the entity's provider. An unrecognized trait name (e.g. a typo, or a rule type stored before provider_traits validation existed) is treated as unsupported and logs a warning, distinguishing it from a provider that simply doesn't implement a valid trait. * Change provider_traits to a list of strings in the proto RuleType.Definition.provider_traits was `repeated ProviderType`, requiring the numeric enum value in JSON/YAML rule type definitions. Change it to `repeated string`, using the same short trait names ("github", "git", "rest", "oci", "repo-lister", "image-lister") already used elsewhere for provider class definitions. Field 8 (the old enum field) is reserved rather than reused, since the two field types have different wire encodings and reusing the number would let old serialized data misparse instead of failing loudly. The new field is 9. proto/buf.yaml gains a narrowly scoped buf breaking exception: WIRE_JSON requires the field *name* to also be reserved on deletion, which conflicts with intentionally reusing "provider_traits" for the new field. This is called out as a deliberate, sanctioned break, since the field has never been documented or used. ProviderTypeFromString, backed by the same (name) option lookup ToString already uses, converts a trait string to its ProviderType. Rule type validation now rejects unknown trait strings at creation/update time, naming the valid values in the error - this is the only safeguard against a typo silently making a rule type never evaluate. * Use a positive provider trait list in the ruletest harness Rename eval()'s provider_missing_traits argument to provider_traits_present and invert its semantics: it now lists the traits the test provider does implement, rather than the ones it doesn't. The default when the argument is omitted is unchanged - all traits are present. parseProviderTraitsList now accepts the same short trait names used in rule type YAML (e.g. "github") via ProviderTypeFromString, instead of full enum names like "PROVIDER_TYPE_GITHUB", and its error on an unrecognized value lists the valid trait names. eval() now checks the rule type engine's SupportedByProvider() before evaluating and reports a "skip" result if the provider doesn't implement a required trait, mirroring - but distinct from - the production executor's zero-footprint no-op for the same case: a test still needs eval() to return something a test author can assert on. * Build provider trait name lookup as a map at init Replaces per-call descriptor iteration in ProviderTypeFromString and the duplicated valid-name lists in ruletypes and ruletest. * Report unknown provider_traits entries as an evaluation error An unrecognized trait name is a rule type authoring problem the user can fix, so it now surfaces as an eval status error naming every offending trait. A known trait the provider doesn't implement still produces no eval status row at all. * Document ProviderType names as a stable interface Also removes the pkg/datasources/v1 driver constants outright rather than leaving deprecated aliases, and notes the buf exception is removable once this merges. * Regenerate GitHub provider mocks for CanImplement * Simplify ProviderTypeFromString to return a bare ProviderType PROVIDER_TYPE_UNSPECIFIED is the zero value and absent from the lookup table, so it already signals a miss and the bool was redundant. * Add a gitlab provider trait Lets a rule type declare [rest, gitlab] for the GitLab API, the counterpart to [rest, github]. The GitLab provider client implements it; there is deliberately no matching provider_type database enum value, since trait gating happens through CanImplement rather than the providers table.
1 parent e5901e1 commit 161ae18

29 files changed

Lines changed: 1088 additions & 285 deletions

File tree

docs/docs/ref/proto.mdx

Lines changed: 11 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

internal/datasources/service/convert.go

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import (
1313

1414
"github.com/mindersec/minder/internal/db"
1515
minderv1 "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1"
16-
v1datasources "github.com/mindersec/minder/pkg/datasources/v1"
1716
)
1817

1918
// DataSourceMetadata is used to serialize additional datasource-level fields
@@ -47,12 +46,12 @@ func dataSourceDBToProtobuf(ds db.DataSource, dsfuncs []db.DataSourcesFunction)
4746
// If we didn't record the type in metadata, use the first function to guess.
4847
dsfType := cmp.Or(metadata.Type, dsfuncs[0].Type)
4948
switch dsfType {
50-
case v1datasources.DataSourceDriverStruct:
49+
case minderv1.DataSourceDriverStruct:
5150
outds.Driver = &minderv1.DataSource_Structured{
5251
Structured: &minderv1.StructDataSource{},
5352
}
5453
return dataSourceStructDBToProtobuf(outds, dsfuncs)
55-
case v1datasources.DataSourceDriverRest:
54+
case minderv1.DataSourceDriverRest:
5655
outds.Driver = &minderv1.DataSource_Rest{
5756
Rest: &minderv1.RestDataSource{},
5857
}
@@ -100,14 +99,14 @@ func dataSourceStructDBToProtobuf(ds *minderv1.DataSource, dsfuncs []db.DataSour
10099

101100
func metadataForDataSource(ds *minderv1.DataSource) (json.RawMessage, error) {
102101
metadata := DataSourceMetadata{
103-
Type: v1datasources.DataSourceDriverStruct,
102+
Type: minderv1.DataSourceDriverStruct,
104103
}
105104
switch ds.Driver.(type) {
106105
case *minderv1.DataSource_Rest:
107-
metadata.Type = v1datasources.DataSourceDriverRest
106+
metadata.Type = minderv1.DataSourceDriverRest
108107
metadata.ProviderAuth = ds.GetRest().GetProviderAuth()
109108
case *minderv1.DataSource_Structured:
110-
metadata.Type = v1datasources.DataSourceDriverStruct
109+
metadata.Type = minderv1.DataSourceDriverStruct
111110
default:
112111
return nil, fmt.Errorf("unknown datasource driver %T", ds.Driver)
113112
}

internal/datasources/service/service.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -538,7 +538,7 @@ func addDataSourceFunctions(
538538
DataSourceID: dsID,
539539
ProjectID: projectID,
540540
Name: name,
541-
Type: v1datasources.DataSourceDriverStruct,
541+
Type: minderv1.DataSourceDriverStruct,
542542
Definition: defBytes,
543543
}); err != nil {
544544
return fmt.Errorf("failed to create data source function: %w", err)
@@ -555,7 +555,7 @@ func addDataSourceFunctions(
555555
DataSourceID: dsID,
556556
ProjectID: projectID,
557557
Name: name,
558-
Type: v1datasources.DataSourceDriverRest,
558+
Type: minderv1.DataSourceDriverRest,
559559
Definition: defBytes,
560560
}); err != nil {
561561
return fmt.Errorf("failed to create data source function: %w", err)

internal/datasources/service/service_test.go

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ import (
2121
"github.com/mindersec/minder/internal/db"
2222
"github.com/mindersec/minder/internal/util/ptr"
2323
minderv1 "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1"
24-
v1 "github.com/mindersec/minder/pkg/datasources/v1"
2524
)
2625

2726
var (
@@ -93,7 +92,7 @@ func TestGetByName(t *testing.T) {
9392
ID: uuid.New(),
9493
DataSourceID: dsID,
9594
Name: "test_function",
96-
Type: string(v1.DataSourceDriverRest),
95+
Type: string(minderv1.DataSourceDriverRest),
9796
Definition: restDriverToJson(t, &minderv1.RestDataSource_Def{
9897
Endpoint: "http://example.com",
9998
InputSchema: is,
@@ -230,7 +229,7 @@ func TestGetByID(t *testing.T) {
230229
ID: uuid.New(),
231230
DataSourceID: id,
232231
Name: "test_function",
233-
Type: string(v1.DataSourceDriverRest),
232+
Type: string(minderv1.DataSourceDriverRest),
234233
Definition: restDriverToJson(t, &minderv1.RestDataSource_Def{
235234
Endpoint: "http://example.com",
236235
InputSchema: is,
@@ -348,7 +347,7 @@ func TestList(t *testing.T) {
348347
ID: uuid.New(),
349348
DataSourceID: dsID,
350349
Name: "test_function",
351-
Type: string(v1.DataSourceDriverRest),
350+
Type: string(minderv1.DataSourceDriverRest),
352351
Definition: restDriverToJson(t, &minderv1.RestDataSource_Def{
353352
Endpoint: "http://example.com",
354353
InputSchema: is,
@@ -628,7 +627,7 @@ func TestBuildDataSourceRegistry(t *testing.T) {
628627
DataSourceID: dsID,
629628
ProjectID: projectID,
630629
Name: "test_function",
631-
Type: string(v1.DataSourceDriverRest),
630+
Type: string(minderv1.DataSourceDriverRest),
632631
Definition: restDriverToJson(t, &minderv1.RestDataSource_Def{
633632
Endpoint: "http://example.com",
634633
InputSchema: is,
@@ -1147,7 +1146,7 @@ func TestUpdate(t *testing.T) {
11471146
ID: uuid.New(),
11481147
DataSourceID: uuid.New(),
11491148
Name: "test_function",
1150-
Type: string(v1.DataSourceDriverRest),
1149+
Type: string(minderv1.DataSourceDriverRest),
11511150
Definition: restDriverToJson(t, &minderv1.RestDataSource_Def{
11521151
Endpoint: "http://example.com",
11531152
InputSchema: func() *structpb.Struct {
@@ -1221,7 +1220,7 @@ func TestUpdate(t *testing.T) {
12211220
ID: uuid.New(),
12221221
DataSourceID: uuid.New(),
12231222
Name: "test_function",
1224-
Type: v1.DataSourceDriverRest,
1223+
Type: minderv1.DataSourceDriverRest,
12251224
Definition: restDriverToJson(t, &minderv1.RestDataSource_Def{}),
12261225
},
12271226
}, nil)
@@ -1357,7 +1356,7 @@ func TestUpdate(t *testing.T) {
13571356
ID: uuid.New(),
13581357
DataSourceID: uuid.New(),
13591358
Name: "test_function",
1360-
Type: string(v1.DataSourceDriverRest),
1359+
Type: string(minderv1.DataSourceDriverRest),
13611360
Definition: restDriverToJson(t, &minderv1.RestDataSource_Def{
13621361
Endpoint: "http://example.com",
13631362
InputSchema: func() *structpb.Struct {
@@ -1402,7 +1401,7 @@ func TestUpdate(t *testing.T) {
14021401
ID: uuid.New(),
14031402
DataSourceID: uuid.New(),
14041403
Name: "test_function",
1405-
Type: string(v1.DataSourceDriverRest),
1404+
Type: string(minderv1.DataSourceDriverRest),
14061405
Definition: restDriverToJson(t, &minderv1.RestDataSource_Def{
14071406
Endpoint: "http://example.com",
14081407
InputSchema: func() *structpb.Struct {
@@ -1496,7 +1495,7 @@ func TestUpdate(t *testing.T) {
14961495
ID: uuid.New(),
14971496
DataSourceID: uuid.New(),
14981497
Name: "test_function",
1499-
Type: string(v1.DataSourceDriverRest),
1498+
Type: string(minderv1.DataSourceDriverRest),
15001499
Definition: restDriverToJson(t, &minderv1.RestDataSource_Def{
15011500
Endpoint: "http://example.com",
15021501
InputSchema: func() *structpb.Struct {
@@ -1624,7 +1623,7 @@ func TestUpsert(t *testing.T) {
16241623
ID: uuid.New(),
16251624
DataSourceID: dsID,
16261625
Name: "test_function",
1627-
Type: v1.DataSourceDriverRest,
1626+
Type: minderv1.DataSourceDriverRest,
16281627
Definition: restDriverToJson(t, &minderv1.RestDataSource_Def{
16291628
Endpoint: "http://example.com/updated",
16301629
InputSchema: func() *structpb.Struct {

internal/engine/executor.go

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ package engine
66
import (
77
"context"
88
"fmt"
9+
"strings"
910
"time"
1011

1112
"github.com/google/uuid"
@@ -179,6 +180,21 @@ func (e *executor) evaluateRule(
179180
return fmt.Errorf("error creating rule type engine: %w", err)
180181
}
181182

183+
// An unknown provider_traits entry (typo, or a trait renamed since the
184+
// rule type was stored) is a rule type authoring problem the user can
185+
// fix, so it must surface as a loud evaluation error rather than the
186+
// silent skip below. Only when every declared trait name is known, but
187+
// the provider doesn't implement one of them, do we take the silent
188+
// path: that's expected and not something the user can act on.
189+
unknownTraits := ruleEngine.UnknownProviderTraits()
190+
if len(unknownTraits) == 0 && !ruleEngine.SupportedByProvider() {
191+
// This rule type doesn't apply to this entity's provider.
192+
// Produce zero evaluation-status footprint: no error, no
193+
// skip record, nothing — stricter than the SkipSilently
194+
// path, which still logs at info level.
195+
return nil
196+
}
197+
182198
// create the action engine for this rule instance
183199
// unlike the rule type engine, this cannot be cached
184200
actionEngine, err := actions.NewRuleActions(ctx, ruleEngine.GetRuleType(), provider, &profile.ActionConfig)
@@ -192,9 +208,14 @@ func (e *executor) evaluateRule(
192208
// Evaluate the rule
193209
var evalErr error
194210
var result *interfaces.EvaluationResult
195-
if profileEvalStatus != nil {
211+
switch {
212+
case len(unknownTraits) > 0:
213+
evalErr = fmt.Errorf("rule type %q declares unknown provider trait(s) %s; "+
214+
"check provider_traits for a typo or a trait renamed since this rule type was stored",
215+
ruleEngine.GetRuleType().Name, strings.Join(unknownTraits, ", "))
216+
case profileEvalStatus != nil:
196217
evalErr = profileEvalStatus
197-
} else {
218+
default:
198219
// enrich the logger with the entity type and execution ID
199220
ctx := zerolog.Ctx(ctx).With().
200221
Str("entity_type", inf.Type.ToString()).

internal/engine/rtengine/cache_test.go

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,59 @@ func TestGetRuleEngine(t *testing.T) {
248248
}
249249
}
250250

251+
// TestNewRuleEngineCache_ProviderTraits verifies that NewRuleEngineCache
252+
// caches every rule type in the project hierarchy regardless of whether its
253+
// provider_traits are satisfied: unsatisfied rule types are still
254+
// constructed and cached, just marked as unsupported, rather than being
255+
// excluded from the cache or aborting the whole population loop.
256+
func TestNewRuleEngineCache_ProviderTraits(t *testing.T) {
257+
t.Parallel()
258+
259+
ctrl := gomock.NewController(t)
260+
defer ctrl.Finish()
261+
ctx := context.Background()
262+
263+
noTraitsID := uuid.New()
264+
satisfiedTraitsID := uuid.New()
265+
unsatisfiedTraitsID := uuid.New()
266+
projectID := uuid.New()
267+
268+
store := dbf.NewDBMock(func(mock dbf.DBMock) {
269+
mock.EXPECT().GetParentProjects(gomock.Any(), gomock.Any()).
270+
Return([]uuid.UUID{projectID}, nil)
271+
mock.EXPECT().GetRuleTypesByEntityInHierarchy(gomock.Any(), gomock.Any()).
272+
Return([]db.RuleType{
273+
{ID: noTraitsID, ProjectID: projectID, Definition: []byte(ruleDefJSON)},
274+
{ID: satisfiedTraitsID, ProjectID: projectID, Definition: []byte(ruleDefJSONWithGitTrait)},
275+
{ID: unsatisfiedTraitsID, ProjectID: projectID, Definition: []byte(ruleDefJSONWithGithubTrait)},
276+
}, nil)
277+
})(ctrl)
278+
279+
dssvc := mockdssvc.NewMockDataSourcesService(ctrl)
280+
dssvc.EXPECT().BuildDataSourceRegistry(gomock.Any(), gomock.Any(), gomock.Any()).
281+
Return(v1datasources.NewDataSourceRegistry(), nil).Times(3)
282+
283+
cache, err := NewRuleEngineCache(
284+
ctx, store, db.EntitiesRepository, projectID,
285+
testproviders.NewGitProvider(nil), nil, ingestcache.NewNoopCache(),
286+
dssvc)
287+
require.NoError(t, err)
288+
require.NotNil(t, cache)
289+
290+
impl, ok := cache.(*ruleEngineCache)
291+
require.True(t, ok)
292+
require.Len(t, impl.engines, 3)
293+
294+
require.Contains(t, impl.engines, noTraitsID)
295+
require.True(t, impl.engines[noTraitsID].SupportedByProvider())
296+
297+
require.Contains(t, impl.engines, satisfiedTraitsID)
298+
require.True(t, impl.engines[satisfiedTraitsID].SupportedByProvider())
299+
300+
require.Contains(t, impl.engines, unsatisfiedTraitsID)
301+
require.False(t, impl.engines[unsatisfiedTraitsID].SupportedByProvider())
302+
}
303+
251304
var (
252305
ruleTypeID = uuid.New()
253306
errTest = errors.New("error in rule type engine cache test")
@@ -305,3 +358,42 @@ const brokenRuleDef = `
305358
}
306359
}
307360
`
361+
362+
// same as ruleDefJSON, but requires the "git" provider trait
363+
const ruleDefJSONWithGitTrait = `
364+
{
365+
"rule_schema": {},
366+
"provider_traits": ["git"],
367+
"ingest": {
368+
"type": "git",
369+
"git": {}
370+
},
371+
"eval": {
372+
"type": "jq",
373+
"jq": [{
374+
"ingested": {"def": ".abc"},
375+
"profile": {"def": ".xyz"}
376+
}]
377+
}
378+
}
379+
`
380+
381+
// same as ruleDefJSON, but requires the "github" provider trait, which
382+
// testproviders.GitProvider does not implement
383+
const ruleDefJSONWithGithubTrait = `
384+
{
385+
"rule_schema": {},
386+
"provider_traits": ["github"],
387+
"ingest": {
388+
"type": "git",
389+
"git": {}
390+
},
391+
"eval": {
392+
"type": "jq",
393+
"jq": [{
394+
"ingested": {"def": ".abc"},
395+
"profile": {"def": ".xyz"}
396+
}]
397+
}
398+
}
399+
`

0 commit comments

Comments
 (0)