-
Notifications
You must be signed in to change notification settings - Fork 140
Update Key-value-storage and policy-engine #1131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
fitzthum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. A few comments.
The change in the abstraction is a little confusing. I guess the properties of the Rego backend (setting extensions, getting data and input), etc) are too specific to implement in one generic policy trait.
Still, I feel that things could be clarified slightly. Maybe policy isn't the right name for the directory or the trait could be renamed or comments added.
Generally this seems fine though.
Yes. Especially extensions. |
|
note: this PR does not trigger |
- Add SetResult enum with Inserted and AlreadyExists variants - Update KeyValueStorage::set() to return Result<SetResult> instead of Result<()> - Change behavior: return AlreadyExists instead of error when key exists and overwrite=false - Update all storage implementations (LocalFs, LocalJson, Memory, Postgres) - Update PolicyEngine::set_policy() to handle new return type This change allows callers to distinguish between successful insertion and existing key scenarios without treating 'already exists' as an error. Signed-off-by: Xynnn007 <[email protected]>
…pport This commit removes the Engine trait abstraction in favor of concrete, scenario-specific implementations. The rationale is: 1. Different policy engines may have different entry parameters and APIs, making a unified trait interface impractical and overly complex. 2. Policy engines are tightly coupled to their specific use cases. For example, the AS (Attestation Service) for EAR token issuance uses Rego policies, and there's no foreseeable need to support other policy languages in the near future. 3. By creating concrete engine instances at their usage sites and encapsulating the implementation details, we achieve better type safety and clearer code organization without unnecessary abstraction. To achieve these goals, this commit - Refactor PolicyEngine from trait-based to generic design (PolicyEngine<T>) - Remove Engine trait and PolicyType enum - Enhance Regorus engine with extension support - Add ExtensionWrapper struct for Regorus extensions - Make data parameter optional in Regorus::evaluate - Add AddRegorusExtensionFailed error variant - Update error comments from OPA to Regorus terminology - Move engine-specific methods to PolicyEngine<Regorus> implementation Signed-off-by: Xynnn007 <[email protected]>
fitzthum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks
This PR mainly lays the groundwork for the dependency in #1092; please see the specific commit message.
Pre of #1129