search

runfinch / finch-daemon

Apache License 2.0
7 stars 10 forks source link

feat: add rego + YAML parsing capabilities #32

Closed sondavidb closed 2 months ago

sondavidb commented 2 months ago

Issue #, if available:

Description of changes: Add parsing capabilities for rego files in the CLI. Currently only allowlisting/denylisting is supported.

I also attempted to add an option to configure this via a YAML file in ~/.finch, but was having issues trying to get the default home directory. You can see the branch here. If I figure this out I can add this to the PR as well, and then this would allow me to add e2e testing.

Testing done:

License Acceptance

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

sondavidb commented 2 months ago

It's also worth noting the OPA library adds a ton of dependencies. We might want to explore if this is alright with us.

sondavidb commented 2 months ago

Had to address linting issues with the upgrade, LMK if we should include the upgrade in a separate PR

sondavidb commented 2 months ago

PR ready for review. Still have concerns with increased dependency tree but unsure how to evaluate it.

I also think maybe we should allow the package name to be specified to make it more general use. This is meant so that docker OPA AuthZ plugins port over nicely but we can change it to be more flexible I suppose.

pendo324 commented 2 months ago

It's also worth noting the OPA library adds a ton of dependencies. We might want to explore if this is alright with us.

Still have concerns with increased dependency tree but unsure how to evaluate it.

Seems like we're increasing the amount of direct+indirect dependencies by 13%. Do we have an alternative method? I don't think we are experts on writing an OPA-compliant rego parser

sondavidb commented 2 months ago

Addressed all feedback, PTAL when you can @Shubhranshu153

sondavidb commented 2 months ago

We are discussing internally if we want to go through with this. Converting to draft to avoid ambiguity, and will close in future if deemed unnecessary.

sondavidb commented 2 months ago

Ultimately we have decided that this feature is not a necessity and can be done more thoroughly via a plugin implementation. Additionally, the REGO libraries used are very heavy, and for the simple use case we are trying to implement it isn't worth the extra bloat.

Closing this.