Gate rule type engine construction on provider_traits - #6702
Gate rule type engine construction on provider_traits#6702intelligent-ears wants to merge 12 commits into
Conversation
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.
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).
|
coverage: 61.708% (+0.005%) from 61.703% — intelligent-ears:feat/rtengine-provider-traits into mindersec:main |
evankanderson
left a comment
evankanderson
left a comment
There was a problem hiding this comment.
Thanks for adding support in TestKit! Can you add some provider_traits to the tests in pkg/ruletest/testdata, as that's where the mindev test integration tests live.
| // Note: this interface intentionally does not depend on | ||
| // github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1 (minderv1). That | ||
| // package transitively imports pkg/datasources/v1, which imports this | ||
| // package for interfaces.Ingested — so importing minderv1 here would create | ||
| // an import cycle. Callers that need a minderv1-typed method (e.g. | ||
| // CanImplement) should type-assert against a package-local interface | ||
| // instead, the way rtengine does. |
There was a problem hiding this comment.
Hmm -- I feel like I'd rather have the protobuf package not depend on the pkg/datasources/v1, but have the dependency go the other way.
What do you think about moving the two constants to the protobuf package to avoid the loop (and then add the appropriate methods to this interface)?
There was a problem hiding this comment.
Agreed, and it turned out to be a one-line dependency — minderv1's hand-written datasources.go imported pkg/datasources/v1 only so GetDriverType() could return those two constants. Moved them into minderv1 (d16e01e) with deprecated aliases left behind in pkg/datasources/v1, since that's a public package. With the cycle gone, interfaces.Provider now declares CanImplement(minderv1.ProviderType) bool directly and the package-local traitProvider interface plus its explanatory comment are deleted (da95a73).
One thing to flag: interfaces.Provider was previously empty, so this is its first method and technically a breaking change for any out-of-tree implementer. Everything in-tree already satisfies it, including TestKit.
There was a problem hiding this comment.
We don't guarantee API compatibility on interfaces.Provider, so anyone out of tree needs to be able to deal with this sort of API breakage. Thanks for thinking of it, though!
| return nil, fmt.Errorf("rule type context must have a project") | ||
| } | ||
|
|
||
| if traits := ruletype.GetDef().GetProviderTraits(); len(traits) > 0 { |
There was a problem hiding this comment.
I don't think you need the check for len(traits) > 0; if you simply write range ruletype.GetDef().GetProviderTraits(), the loop will execute 0 times if no traits were declared (and then will be no CanImplement checks).
There was a problem hiding this comment.
Removed in da95a73 — you're right, the getters are nil-safe and range over a nil slice is a no-op, so the guard wasn't buying anything.
| return nil, fmt.Errorf( | ||
| "provider does not implement required trait %s for rule type %s", | ||
| trait.String(), ruletype.Name) |
There was a problem hiding this comment.
What happens in a running Minder instance if this returns error repeatedly? I suspect that's not a common case that we expect, and might cause looping or other evaluation problems.
Can you test the following:
A profile with three ruletypes:
- The first declares no
provider_traits - The second declares
provider_traitsthat aren't satisfied by any provider (e.g. add a "GitLab" trait, and require both GitHub and GitLab) - The third declares a
provider_traitsatisfied by the provider.
And then register at least one repository with e.g. the GitHub or GitLab provider that should evaluate the profile.
What should happen as I understand it is that we should get rule evaluation results for the first and third ruletypes, and no evaluation results (not listed, not "skipped" or "error") for the second ruletype.
evankanderson
left a comment
evankanderson
left a comment
There was a problem hiding this comment.
Thanks for updating this!
I know that we use special error status to return different types of evaluation results, but this feels like a place where we can simplify to regular methods that return a boolean, rather than needing the additional complexity.
| # RuleType uses plain encoding/json (not protojson) for these fields, so | ||
| # provider_traits must be the numeric enum value, not the string name: | ||
| # 1 = PROVIDER_TYPE_GITHUB. | ||
| provider_traits: [1] |
There was a problem hiding this comment.
Hmm, this doesn't feel very good. I feel like I'd rather have this be a list of string than numeric enums.
There was a problem hiding this comment.
I think we still have time to go back and change the proto definition (even though it will technically be incompatible), since this feature hasn't really been documented or used.
There was a problem hiding this comment.
Done — provider_traits is now repeated string (SHA1). Field 8 is reserved and the name is reused for field 9, so the number can never come back with a different meaning. Testdata reads provider_traits: [github] and the comment apologising for numeric enums is gone.
Two consequences worth flagging.
buf breaking needed a narrow FIELD_NO_DELETE_UNLESS_NAME_RESERVED exception scoped to minder/v1/minder.proto. WIRE_JSON wants both the number and the name reserved on deletion, which conflicts with reusing provider_traits for field 9. Reserving the number only seemed right, and the alternative — renaming the field — seemed worse. Happy to do it differently if you'd rather.
Dropping the enum also means the compiler stops catching typos, and under the boolean check in the other thread a misspelled trait would silently make a rule type never evaluate: no error, no skipped status, no row. That's the failure mode the enum was implicitly protecting against, so SHA3 adds validation at rule type creation that rejects unknown trait names and lists the valid ones, plus a warning log in NewRuleTypeEngine as a fallback for rule types already stored or any path that bypasses validation.
There was a problem hiding this comment.
Since we haven't released a client with this definition, I feel good about disabling or ignoring the buf check for this particular incompatibility.
I'll take a look at the implementation -- I think we should be able to (on ingest) flag typo'ed provider traits based on the server's knowledge of the trait universe. This also fixes a problem where someone using a ruletype which requires a recent trait would need a recent client in order to upload the ruletype to the server. IMO, it's better for the client to be able to treat this as opaque data, and people to be able to expect that e.g. a client from 6 months ago can work reasonably as long as the server is fairly up to date. (This matches, for example, my usage of the gh CLI and other cloud clients.)
| "profile?", &profileDict, "params?", ¶msDict, "mock_http?", &mockHttpDict, | ||
| "mock_fs?", &mockFSDict, "data_sources?", &datasourcesList) | ||
| "mock_fs?", &mockFSDict, "data_sources?", &datasourcesList, | ||
| "provider_missing_traits?", &providerMissingTraitsList) |
There was a problem hiding this comment.
I think I'd rather prefer to see a positive list of "provider traits present" in the test, with the default behavior still being to have all traits present if the argument is not set.
There was a problem hiding this comment.
Renamed to provider_traits_present with the logic inverted (SHA4). Default is unchanged: when the argument isn't passed, every trait is present.
While inverting it I noticed the argument was taking full enum names (PROVIDER_TYPE_GITHUB) while rule type YAML takes short trait names, so it now takes the same short names as the YAML field — provider_traits_present=["git", "rest"]. An unrecognised name errors out and lists the valid ones rather than silently parsing as absent.
One divergence to call out: the harness reports status: "skip" for a rule type the test provider doesn't support, whereas the executor produces nothing at all for that case. A Starlark test needs eval() to return something assertable, so reproducing the executor's silent no-op didn't seem workable. There's a comment on skippedResult explaining it, but say the word if you'd rather it surfaced differently.
| // skip record, nothing — stricter than the SkipSilently | ||
| // path, which still logs at info level. | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Rather than going through the engine cache and back with an error status, what about adding the provider check right above GetRuleEngine, or even in EvalEntityEvent before calling evaluateRule?
There was a problem hiding this comment.
Agreed, this is much simpler. The sentinel is gone entirely (SHA2).
NewRuleTypeEngine now computes trait satisfaction once at construction and exposes SupportedByProvider() bool, so it no longer returns an error for something that isn't really an error. The cache's errors.Is/continue is gone and every rule type gets cached again; evaluateRule checks the boolean and returns before createOrUpdateEvalStatus, so there's still no eval status row for an inapplicable rule — not listed, not skipped, not error.
On placement: I put it on the engine rather than above GetRuleEngine or in EvalEntityEvent because the check needs the rule type's provider_traits, so doing it earlier means either an extra DB lookup or reaching for the engine we'd be trying to avoid building. The trade-off is that engines now get constructed for rule types that will never evaluate, so we pay validator/ingester/evaluator setup for them once per cache build. That seemed like the cheaper side of the trade, but if you'd rather pay the lookup and skip construction entirely I'm happy to move it.
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.
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.
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.
That's the right call, and it collapsed a lot of machinery. The sentinel is gone — no error wrapping, no It also removes the failure mode you were originally asking about more cleanly than my previous version did. Construction can't fail on traits at all now, so the case where one unsatisfiable rule type took down Also in this round: One thing I owe you: I couldn't complete the three-ruletype manual run against a live instance. PAT enrollment fails on my local stack at |
I can do a manual run if you want; I already have a GitHub app set up for my docker-compose setup, so I shouldn't be hitting the token case at all. |
evankanderson
left a comment
evankanderson
left a comment
There was a problem hiding this comment.
This is looking really good, by the way! A few more comments, but only because I'm excited to see this working. I'll also be running an integration test for you and have results probably tomorrow morning.
| # RuleType.Definition.provider_traits (field 8) was removed in favor of a | ||
| # new field 9 of the same name but a different type (repeated ProviderType | ||
| # -> repeated string). Field 8 is reserved so its number can never be | ||
| # reused, but WIRE_JSON also requires the *name* to be reserved on | ||
| # deletion, which would conflict with reusing "provider_traits" for field | ||
| # 9. This is a deliberate, sanctioned exception: provider_traits has never | ||
| # been documented or used, so no real caller depends on the old field 8 | ||
| # wire/JSON shape. | ||
| ignore_only: | ||
| FIELD_NO_DELETE_UNLESS_NAME_RESERVED: | ||
| - minder/v1/minder.proto |
There was a problem hiding this comment.
We should be able to delete this after this PR is merged, as the buf incompatibility check only compares between the PR base and current (as it should).
There was a problem hiding this comment.
Agreed. Updated the comment in 82b6b46 to say so explicitly rather than removing it now, since deleting it before merge turns the check red. Happy to open the follow-up to drop it once this lands, or leave it to whoever's next in that file.
| // validProviderTraitNames lists the trait names a rule type's | ||
| // provider_traits may declare: the (name) option of every defined | ||
| // ProviderType value. | ||
| var validProviderTraitNames = func() []string { |
There was a problem hiding this comment.
I'd prefer to explicitly use an init method to initialize this at startup, rather than implicitly through an anonymous function evaluation. It seems a little more clear to me.
There was a problem hiding this comment.
Done in c0a0f1a. It's an explicit init() now, and it moved to the minderv1 package — between your comment here and the one on pkg/ruletest/eval.go, it was clear this list had no business existing in three places. So this local copy is gone entirely and both callers use minderv1.ValidProviderTraitNames().
| // This validation is the only safeguard against a typo in provider_traits: | ||
| // an unrecognized string parses fine but matches no trait, silently making | ||
| // the rule type never evaluate (see rtengine.RuleTypeEngine.SupportedByProvider), | ||
| // with no error, skipped status, or eval status row to reveal the mistake. |
There was a problem hiding this comment.
In particular, I think this means that any stored ruletype with a provider_trait name that is later renamed would stop evaluating? (Because we're using the JSON-serialized proto definition stored in the definition row.)
I think that's acceptable, but if we do another set of commits, you might want to add that comment to the proto definition for the provider types.
There was a problem hiding this comment.
Changing the name of the provider traits would also break all the clients using provider_trait, so I think it's fair to consider that a fixed-ish interface.
There was a problem hiding this comment.
Correct — the definition is stored as serialized proto, so a renamed trait name would stop matching for anything already in the database, and your second point is the sharper one: it'd break every client constructing or parsing those names too. That makes them a public interface whether or not we intended them as one.
Added that to the ProviderType enum in 82b6b46: the (name) values are a stable interface, renaming one silently stops it matching stored rule types as well as breaking clients, so add new values rather than renaming existing ones.
The comment you're commenting on is also now out of date — this validation is no longer the only safeguard. c2dd129 catches an unrecognized name at evaluation time too and reports it as an eval status error, so a rule type stored before this validation existed no longer fails silently. Updated the comment to say that instead.
| // ProviderTypeFromString returns the ProviderType whose (name) option | ||
| // matches s (e.g. "github", "git"), and true if one was found. It is the | ||
| // inverse of ProviderType.ToString. |
There was a problem hiding this comment.
Given the cost of iterating, I'd prefer to have this use a map generated at init time, rather than called in a loop (like we do in pkg/ruletypes/service.go).
There was a problem hiding this comment.
Done in c0a0f1a — providerTypeByName is built once in init() and this is a map lookup now. enumFromStringViaDescriptor is deleted since it had no other callers; enumToStringViaDescriptor stays, it's still used by ToString() on several enums.
The same init() also builds the sorted valid-name list, exported as ValidProviderTraitNames() for error messages, which let me delete the duplicate copies in pkg/ruletypes and pkg/ruletest you flagged elsewhere. It returns a copy rather than the package slice, so a caller sorting or mutating it can't affect anyone else.
| zerolog.Ctx(ctx).Warn(). | ||
| Str("rule_type", ruletype.GetName()). | ||
| Str("provider_trait", trait). | ||
| Msg("rule type declares an unknown provider trait; treating as unsupported") | ||
| supportedByProvider = false | ||
| break |
There was a problem hiding this comment.
If we mismatch here, can we make this return an error to the user? I'd need to think about the mechanics, but it "feels" right, since the user can fix it by adjusting the ruletype definition.
There was a problem hiding this comment.
Agreed, and done in c2dd129 using the mechanics you sketched in the executor thread — the engine caches the unknown names rather than trying to report from here.
NewRuleTypeEngine now collects every unrecognized trait name into unknownProviderTraits and exposes it via UnknownProviderTraits(). The loop no longer breaks on the first one, so someone with two typos sees both rather than fixing one and rediscovering the other on the next run. The two conditions stay distinct instead of both collapsing into supportedByProvider = false, since only one of them is something the user can act on.
Dropped the warning log — it was a consolation prize for not being able to tell the user, and now we can.
| if !ruleEngine.SupportedByProvider() { | ||
| // This rule type doesn't apply to this entity's provider. | ||
| // Produce zero evaluation-status footprint: no error, no | ||
| // skip record, nothing — stricter than the SkipSilently | ||
| // path, which still logs at info level. | ||
| return nil | ||
| } |
There was a problem hiding this comment.
If you can cache the "unknown provider trait" in the ruleEngine, you can use it here to record the error only for ruletypes that have unrecognized provider traits.
(Sorry to move the goals on you -- your observation that a completely unknown provider_trait has a silent failure mode made me look to make that loud.)
There was a problem hiding this comment.
No apology needed — I flagged the silent failure and then left it silent, so this is the better end of that. Done in c2dd129, exactly as you described.
There are three states now rather than two:
- Unknown trait name →
evalErris set to an error naming the rule type and every offending trait, which flows through the existingSetEvalErr→DoActions→createOrUpdateEvalStatuspath, so it lands as a normal eval status error. No parallel reporting mechanism. - Known trait the provider doesn't implement → unchanged, silent early return, no row at all.
- Everything satisfied → evaluates as before.
The silent return is guarded on len(unknownTraits) == 0 && !SupportedByProvider(), so the two can't collapse into each other. Worth noting the error is deliberately not wrapped in ErrEvaluationFailed or ErrEvaluationSkipped, so it classifies as EvalStatusTypesError rather than a rule failure — the rule didn't fail, the rule type is malformed.
| // DataSourceDriverStruct is the driver type for the structured data source. | ||
| // | ||
| // Deprecated: use minderv1.DataSourceDriverStruct. Kept here as an alias so | ||
| // existing callers of this package don't break; the canonical definition | ||
| // lives in minderv1 to avoid an import cycle (minderv1 -> this package -> | ||
| // pkg/engine/v1/interfaces). | ||
| DataSourceDriverStruct = minderv1.DataSourceDriverStruct | ||
| // DataSourceDriverRest is the driver type for a REST data source. | ||
| DataSourceDriverRest = "rest" | ||
| // | ||
| // Deprecated: use minderv1.DataSourceDriverRest. See DataSourceDriverStruct. | ||
| DataSourceDriverRest = minderv1.DataSourceDriverRest |
There was a problem hiding this comment.
IMO, since the package is pre-1.0, it would be okay to remove these entirely.
There was a problem hiding this comment.
Removed entirely in 82b6b46. Turned out to be free — nothing referenced the aliases at all; every caller was already using the minderv1 constants directly, so there was no migration to do and the minderv1 import in this file went with them.
| // validProviderTraitNames lists the trait names accepted by | ||
| // provider_traits_present: the (name) option of every defined ProviderType | ||
| // value. | ||
| func validProviderTraitNames() []string { | ||
| names := make([]string, 0, len(minderv1.ProviderType_name)) | ||
| for v := range minderv1.ProviderType_name { | ||
| t := minderv1.ProviderType(v) | ||
| if t == minderv1.ProviderType_PROVIDER_TYPE_UNSPECIFIED { | ||
| continue | ||
| } | ||
| names = append(names, t.ToString()) | ||
| } | ||
| sort.Strings(names) | ||
| return names | ||
| } |
There was a problem hiding this comment.
Given that we use this a few different places, what do you think about creating this as a map in the proto package?
There was a problem hiding this comment.
Yes — this was the third copy, which is two too many. c0a0f1a builds providerTypeByName once in an init() in minderv1 and exports ValidProviderTraitNames() alongside it; this function and the one in pkg/ruletypes/service.go are both gone, and ProviderTypeFromString is a map lookup rather than a descriptor walk.
The accessor returns a copy rather than the package slice, so a caller sorting or mutating it can't affect anyone else.
Replaces per-call descriptor iteration in ProviderTypeFromString and the duplicated valid-name lists in ruletypes and ruletest.
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.
Also removes the pkg/datasources/v1 driver constants outright rather than leaving deprecated aliases, and notes the buf exception is removable once this merges.
That'd be great, thank you — yes please. A few things that changed under you since I first described the setup:
A fourth case worth adding to your three, if it's cheap: a rule type with a deliberately misspelled trait, e.g. Also worth pushing on: register the unsatisfiable rule type but leave it out of every profile. That was the case I was most worried about — the engine cache builds an engine for every rule type in the project hierarchy regardless of profile membership, so an unused broken one used to be able to take down evaluation for everything. |
Thanks — and no complaints about the comment volume, the PR is meaningfully better for them. Pushed in three commits:
The unknown-trait change is the one worth a second look, since it's the only behavioural change of the three and it's new since you last read this. Taken you up on the integration test in the other thread — there are a couple of things worth knowing before you run it, including a fourth case that didn't exist when you offered. |
3 participants
Summary
Widens the gating logic in
NewRuleTypeEngineso a rule type carryingprovider_traits(added in #6669) is rejected up front — at engine construction time — if the provider doesn't implement every listed trait. This mirrors the existing pattern used for entity-context mismatches (reject at construction, not via a runtimeErrEvaluationSkipped), per the issue.The trait check is a package-local interface in
pkg/engine/v1/rtengine/engine.go:rather than a new method on pkg/engine/v1/interfaces.Provider. The original design called for widening interfaces.Provider directly, but that creates an import cycle: pkg/engine/v1/interfaces can't import minderv1 because minderv1 (via its hand-written datasources.go) imports pkg/datasources/v1, which imports pkg/engine/v1/interfaces. Declaring traitProvider locally in
rtengine and type-asserting against it avoids the cycle; every concrete provider already implements CanImplement with this exact signature, so they satisfy it structurally with zero provider-side changes.
pkg/testkit/v1.TestKit.CanImplement was hardcoded to always return true; added a WithCanImplement functional option so tests can control it per case, defaulting to true so existing callers (including pkg/ruletest/eval.go) are unaffected.
Part of #6650, closes #6652.
Test plan