Skip to content

feat: Remove filtering from executor#429

Merged
bbernays merged 35 commits intocloudquery:mainfrom
bbernays:Remove-filtering-from-executor
Jan 27, 2022
Merged

feat: Remove filtering from executor#429
bbernays merged 35 commits intocloudquery:mainfrom
bbernays:Remove-filtering-from-executor

Conversation

@bbernays
Copy link
Copy Markdown
Collaborator

@bbernays bbernays commented Jan 20, 2022

Implements #428

A few comments:

  1. selector path needs to be of the form rootPolicy/policy1/subpolicy/check
  2. Error message has changed, need to understand if we need to put the old error message back in
  3. Change: If a policy has no Checks or sub-policies then no views will be invoked

@bbernays bbernays changed the title Remove filtering from executor [WIP] Remove filtering from executor Jan 20, 2022
@bbernays bbernays changed the title [WIP] Remove filtering from executor Remove filtering from executor Jan 20, 2022
@bbernays bbernays changed the title Remove filtering from executor feat: Remove filtering from executor Jan 20, 2022
@github-actions github-actions bot added the feat label Jan 20, 2022
@github-actions github-actions bot added feat and removed feat labels Jan 20, 2022
Copy link
Copy Markdown
Contributor

@roneli roneli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@roneli
Copy link
Copy Markdown
Contributor

roneli commented Jan 23, 2022

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.

With that way of doing it, how do you tell if the last part is a check or a policy?

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.

@bbernays
Copy link
Copy Markdown
Collaborator Author

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.

@roneli
Copy link
Copy Markdown
Contributor

roneli commented Jan 24, 2022

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.

Copy link
Copy Markdown
Contributor

@roneli roneli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good, lets wrap up our conversation and we can merge.

Copy link
Copy Markdown
Contributor

@roneli roneli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, a few questions nits.

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 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this checked in the .Execute function?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@roneli roneli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown
Contributor

@roneli roneli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@bbernays bbernays merged commit 0317672 into cloudquery:main Jan 27, 2022
@bbernays bbernays deleted the Remove-filtering-from-executor branch January 27, 2022 15:13
TinLe pushed a commit to TinLe/cloudquery that referenced this pull request Feb 2, 2022
* 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)
  ...
@cq-bot cq-bot mentioned this pull request Aug 9, 2022
erezrokah pushed a commit that referenced this pull request Aug 14, 2022
erezrokah pushed a commit that referenced this pull request Aug 14, 2022
…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 ([#&#8203;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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants