-
Notifications
You must be signed in to change notification settings - Fork 409
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
[Feature] Updated AWS UC storage credential to include permissions for file events #4406
[Feature] Updated AWS UC storage credential to include permissions for file events #4406
Conversation
3ae3e94
to
9085086
Compare
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.
@mgyucht thanks for the review. Ideally we would not make these changes opt-in/out as we're moving towards making file events mandatory (see PRD: Maximizing coverage of managed file events) |
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 think we can make the requested resources slightly narrower, which would be marginally better from a security stance. Otherwise, seems OK to me.
aws/data_aws_unity_catalog_policy.go
Outdated
fmt.Sprintf("arn:%s:sqs:*:*:csms-*", awsPartition), | ||
fmt.Sprintf("arn:%s:sns:*:*:csms-*", awsPartition), |
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.
In the other policies, we restrict the resource templates to a single account. Can we do that here as well for consistency, or do we need to allow access to all accounts? (Understanding that this would also require cross-account policy on the target account, but with defence in depth in mind, we shouldn't force people to give broader access than necessary.) Ditto for the other resources. We already accept awsAccountId
as a parameter.
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.
@mgyucht I considered scoping down to the account, but this is not the case for the S3 permissions in this specific file and I believe we should align the new permissions because they're targeting resources for the same use case.
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
Changes
Databricks documentation for storage credentials contains instructions to add permissions for file events, but as of yet these are missing from the terraform provider. This PR adds them for AWS. PRs for Azure and GCP will follow soon
Tests
Updated test:
aws/data_aws_unity_catalog_policy_test.go
make test
run locallydocs/
folderinternal/acceptance