-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[Feature] Pre job run OPA policy enforcement #15947
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
base: devel
Are you sure you want to change the base?
[Feature] Pre job run OPA policy enforcement #15947
Conversation
7fe36b2
to
a18e7c7
Compare
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. 🚀 New features to boost your workflow:
|
28b09c9
to
01ce8a3
Compare
Co-Authored-By: Jiří Jeřábek (Jiri Jerabek) <[email protected]> Co-Authored-By: Alexander Saprykin <[email protected]> Co-Authored-By: Tina Tien <[email protected]>
b4fe26a
to
f33266c
Compare
|
super().__init__(*args, **kwargs) | ||
|
||
if not flag_enabled("FEATURE_POLICY_AS_CODE_ENABLED") and 'opa_query_path' in self.fields: | ||
self.fields.pop('opa_query_path') |
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.
I'm still so incredibly worried about this from a performance and consistency standpoint, and I hope we are able to make this non-conditional soon.
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.
kinda agree...
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.
but it's the pattern for APIs behind feature flag ain't it ?
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.
Like RBAC, there may be a good argument to have OPA API structure at arms-length (having its own section of the API) from the rest of the API. Inter-mingling the things causes problems like these. A flag that makes an entire view accessible or not accessible could be more reasonably turned off by a feature flag, safely tucked away at a view-level override, and honest documentation and OpenAPI specs.
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.
To put views/APIs behind feature flags, I'd recommend we check out these options -
URLS - https://cfpb.github.io/django-flags/api/urls/
Views - https://cfpb.github.io/django-flags/api/decorators/
This code was removed a while ago, but initially this is how we gated an API endpoint behind feature flags - https://github.com/ansible/eda-server/pull/1132/files#diff-2970751c57bcb2f4906a6353ed7d8f16c396995e16c41b35bf6c84a780c5b4cfR110-R120
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.
Good to know. The flagged_path
is the reference implementation we should look to.
That uses a decorator, so the view is still the view.
https://github.com/cfpb/django-flags/blob/main/flags/decorators.py#L46
But as a runtime thing it does raise Http404
So this explains how it does something that appears would check a flag at import and delays it to runtime, as we would want.
SUMMARY
See https://github.com/TheRealHaoLiu/example-opa-policy-for-aap for detail how to and example
ISSUE TYPE
COMPONENT NAME
AWX VERSION
ADDITIONAL INFORMATION