-
Notifications
You must be signed in to change notification settings - Fork 332
DOCS: OPA as External Policy Decision Point #3030
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: main
Are you sure you want to change the base?
Conversation
snazy
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.
@sungwy This is a really neat piece of documentation and PR work! Thanks a lot!
Just a few minor comments, nothing serious.
|
|
||
| Create a policy file (e.g., `policies/polaris.rego`): | ||
|
|
||
| ```rego |
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.
Too sad - guess we need to bump the Hugo version to get syntax highlighting for Rego (not for this PR tho).
site/content/in-dev/unreleased/managing-security/external-pdp/opa.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Robert Stupp <[email protected]>
Co-authored-by: Robert Stupp <[email protected]>
| linkTitle: Managing Security | ||
| type: docs | ||
| weight: 550 | ||
| --- |
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.
The front-matter ending is missing now. That's probably why CI fails.
| {{< alert warning "Experimental Feature" >}} | ||
| **OPA integration is currently an experimental feature** and may undergo breaking changes in future versions. Use with caution in production environments. | ||
| {{< /alert >}} |
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βd suggest using βPreview Featureβ instead of βExperimental Feature.β
βPreviewβ typically signals that the feature is available for early adoption and feedback, but not finalized, while sounding more stable and user-friendly than βexperimental.β This may set better expectations for users. For example, we marked the event as a preview feature, https://polaris.apache.org/downloads/#120.
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.
This is a great suggestion - thank you @flyrain !
|
|
||
| The `action` field contains the operation being attempted as a string value from the `PolarisAuthorizableOperation` enum. | ||
|
|
||
| For the complete list of available operations, see the [PolarisAuthorizableOperation enum](https://github.com/apache/polaris/blob/main/polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizableOperation.java) in the source code. |
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.
Should we recommend to handle all operations in OPA so that it makes sure that an operation won't accidentally fail? Even say, defaulting operations you don't care to deny would be helpful.
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.
Should we also mention that the certain operations(e.g. grant privilges) should be set to deny always?
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.
Should we recommend to handle all operations in OPA so that it makes sure that an operation won't accidentally fail? Even say, defaulting operations you don't care to deny would be helpful.
Sounds good π
Should we also mention that the certain operations(e.g. grant privilges) should be set to deny always?
I think that makes sense. I was just wondering if we'd want to do that explicitly in the OpaPolarisAuthorizer, but I think relying on the default behavior of default allow := false would be an easier way of achieving than maintaining a list in the opa extension
| For the complete list of available operations, see the [PolarisAuthorizableOperation enum](https://github.com/apache/polaris/blob/main/polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizableOperation.java) in the source code. | ||
|
|
||
| Common examples include: | ||
| - Table operations: `LOAD_TABLE_WITH_READ_DELEGATION`, `LOAD_TABLE_WITH_WRITE_DELEGATION`, `CREATE_TABLE_DIRECT`, `UPDATE_TABLE`, `DROP_TABLE_WITHOUT_PURGE` |
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.
Thinking more about it, would it make sense to provide a rego example that demonstrate how it would work with all kinds of operations? It's a blocker for this PR. We could file an issue for that if that makes sense.
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.
Did you mean to say that it's not a blocker for this PR? :)
I'm +1 for creating a follow up issue for that! Happy to create it once you clarify above
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.
Sorry for the typo. It's NOT a blocker.
flyrain
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 doc! Thanks a lot for working on it, @sungwy! Left some comments.
Doc PR following up the introduction of
OpaPolarisAuthorizer: #2680Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)