feat: Remove filtering from executor#429
feat: Remove filtering from executor#429bbernays merged 35 commits intocloudquery:mainfrom bbernays:Remove-filtering-from-executor
Conversation
…bernays/cloudquery into Remove-filtering-from-executor
roneli
left a comment
There was a problem hiding this comment.
Had some suggestions, also in retrospect, feels like hack that the filter returns a Policy with one policy from the selector, not sure if its better to add a policy.FilterCheck. I would maybe suggest Filter only returns the last policy level i.e policy/subpolicy/subsubpolicy/check so it will return subsubpolicy and then in the execute u will do subsubPolicy.GetCheck(lastPart) ? maybe less hacky but must pass check to execute.
You give me a path lets say Px is a policy and C is a check. So by passsing p//p1/p2/p3/C or p//p1/p2/p3 the function always returns p3 + C in the latter case it will return p3 + "". The Execute function gets P3 + C if C != "" it runs C in P3 if it exists, otherwise runs all P3 checks. |
While I see that your suggestion works in our current codebase but I am wary about tying policy filtering execution together. I implemented it this way so that any function that needs to interact with a policy never needs any more information than the policy that is passed to it. This fully decouples filtering from execution/evaluation/anything other action we implement. I don't think that actions should have to know any information about the selector or any other processing that might go on. |
Yes that makes sense, maybe what bugs me is the fact that if you ask for a control you get a copy of that policy only with that control. But I can see that the my initial suggestion remove the decoupling. |
roneli
left a comment
There was a problem hiding this comment.
I think this looks good, lets wrap up our conversation and we can merge.
roneli
left a comment
There was a problem hiding this comment.
Looks good, a few questions nits.
pkg/policy/manager.go
Outdated
| if request.Policy.meta.subPolicy != "" { | ||
| selector = strings.Split(request.Policy.meta.subPolicy, "/") | ||
| filteredPolicy := request.Policy.Filter(request.Policy.meta.subPolicy) | ||
| if len(filteredPolicy.Policies.All()) == 0 && len(filteredPolicy.Checks) == 0 { |
There was a problem hiding this comment.
isn't this checked in the .Execute function?
There was a problem hiding this comment.
The check needs to be in both places in order to maintain backwards compatibility... We need to sanitize any policies being passed into the function and in order to maintain the same error message we have to return the error from within the manager Run() function otherwise we would have to pass the selector to the function which we are trying to avoid.
* upstream/main: (25 commits) fix: Relax tfstate version check (cloudquery#460) fix: Use provider source (cloudquery#458) feat: Configurable data folder (cloudquery#456) chore: remove duplicate lint workflow (cloudquery#454) chore: Synced local '.github/workflows/' with remote 'workflows/common' (cloudquery#453) docs: Small fixes (cloudquery#452) chore: Remove extra linter workflow (cloudquery#451) fix: Getter implementation (cloudquery#450) fix: DB version validation shouldn't block on vanilla PG (cloudquery#447) fix: Validate provider semantic version (cloudquery#445) feat: Remove filtering from executor (cloudquery#429) fix: Panic when canceled (cloudquery#443) fix: Nil connection (cloudquery#442) feat: Update drift for AWS IOT resources (cloudquery#434) fix: Typo in error message (cloudquery#441) feat: Report on multiple aliased providers (cloudquery#437) fix: Refer to cloudquery as cloudquery (cloudquery#440) fix: Panic On Policy Describe (cloudquery#438) feat: Update SDK v0.7.0 (cloudquery#435) fix: Don't connect on cloudquery init (cloudquery#436) ...
…14.3 (#429) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/cloudquery/cq-provider-sdk](https://togithub.com/cloudquery/cq-provider-sdk) | require | patch | `v0.14.2` -> `v0.14.3` | --- ### Release Notes <details> <summary>cloudquery/cq-provider-sdk</summary> ### [`v0.14.3`](https://togithub.com/cloudquery/cq-provider-sdk/releases/tag/v0.14.3) [Compare Source](https://togithub.com/cloudquery/cq-provider-sdk/compare/v0.14.2...v0.14.3) ##### Bug Fixes - Fix bug in ValueTypeFromString ([#​430](https://togithub.com/cloudquery/cq-provider-sdk/issues/430)) ([6ae324e](https://togithub.com/cloudquery/cq-provider-sdk/commit/6ae324ea2adeb9e0f83c523ab73c6f827db17bec)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate).
Implements #428
A few comments:
rootPolicy/policy1/subpolicy/checkError message has changed, need to understand if we need to put the old error message back in