Skip to content
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

DWX-19990: Add Log Bucket and its Access for Managed ARN Restricted Policies #21

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

architlatkar27
Copy link

Based on QE test changes, we got S3 Access denied when a different log bucket was used. This is because we only specify permissions to work on the DL bucket. To mitigate this issue, we can add another bucket for Logs in the Sid: S3PutGetObject.


Pull Request checklist:

Description:

  • Description of the change is added
  • Jira ID is added to the title

Changes to any files in generated folder(not to be touched)

  • No
  • Yes

Is the DWX JIRA PR using a new AWS SDK API which needs new permission(s), not included in either of the files under
https://github.com/cloudera/cdw-cloud-policies/tree/main/aws-iam-policies/docs then create a PR to include the new action

Is the dwx-cf-template.yaml being modified, that is specifically any new resources are added or if new permissions/actions are needed.

Are new permissions added in the policies section of "NodeInstanceRole" section of dwx-cf-template.yaml

Is this a bug fix for an old release, with missing permission, example DWX-15473

  • Create a TSB about the missing permission

Backward compatibility

  • No breaking changes for upgrades
  • Yes, then create a TSB,

Testing:

  • Manual tests - QE team
  • Control Plane integrated
  • mow-priv

@architlatkar27
Copy link
Author

@roohisyeda this issue affects R41C so could you please review and merge this as soon as possible?
Thanks

],
"Resource": [
"arn:aws:s3:::${DATALAKE_BUCKET}/cf-templates/*",
"arn:aws:s3:::${DATALAKE_BUCKET}/backup/*"
"arn:aws:s3:::${DATALAKE_BUCKET}/backup/*",
"arn:aws:s3:::${LOG_BUCKET}/cdw/*"
Copy link

Choose a reason for hiding this comment

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

backup bucket can be defined as well, see backupStorageLocationBase in https://cloudera.github.io/cdp-dev-docs/cli-docs/environments/create-aws-environment.html

When you create an environment all 3 properties can be configured: data bucket and location, log bucket and loction and backup bucket and location; these 3 can point to different or the same bucktes

Copy link
Author

Choose a reason for hiding this comment

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

yes, good catch. Added it here in the latest commit

@roohisyeda
Copy link
Collaborator

Aren't we supposed to fix here https://github.com/cloudera/cdw-cloud-policies/blob/main/aws-iam-policies/managedArn-node-inline-policy.json#L266 for this issue or may be both?

Copy link

@tevesz tevesz 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, thank you!

],
"Resource": [
"arn:aws:s3:::${DATALAKE_BUCKET}/cf-templates/*",
"arn:aws:s3:::${DATALAKE_BUCKET}/backup/*"
"arn:aws:s3:::${DATALAKE_BUCKET}/backup/*",
"arn:aws:s3:::${LOG_BUCKET}/cdw/*",
Copy link

Choose a reason for hiding this comment

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

oh sorry, we need access to LOG_LOCATION, not really the bucket and we should not restrict it under /cdw. The location is bucket + custom_path like: s3://my-bucket/my/custom/path/ This is exactly what the user can configure when they create a data lake and an environment.

Same for the backup bucket

@@ -121,7 +121,9 @@
],
"Resource": [
"arn:aws:s3:::${DATALAKE_BUCKET}/cf-templates/*",
"arn:aws:s3:::${DATALAKE_BUCKET}/backup/*"
"arn:aws:s3:::${DATALAKE_BUCKET}/backup/*",
"arn:aws:s3:::${LOG_BUCKET}/cdw/*",
Copy link

Choose a reason for hiding this comment

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

we should drop the /cdw

Copy link

@tevesz tevesz left a comment

Choose a reason for hiding this comment

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

sorry, we should remove the /cdw

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.

3 participants