Validate iam for liveramp S3 #3209
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3209 +/- ##
=======================================
Coverage 79.88% 79.88%
=======================================
Files 1190 1190
Lines 21874 21894 +20
Branches 4262 4270 +8
=======================================
+ Hits 17474 17490 +16
- Misses 3648 3650 +2
- Partials 752 754 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
nit pick: in the test cases |
varadarajan-tw
left a comment
varadarajan-tw
left a comment
There was a problem hiding this comment.
Looks good. Left a couple of questions
| if ( | ||
| !payload.s3_aws_access_key || | ||
| !payload.s3_aws_secret_key || | ||
| !payload.s3_aws_bucket_name || | ||
| !payload.s3_aws_region | ||
| ) { | ||
| throw new PayloadValidationError('Missing required S3 credentials.') | ||
| } |
There was a problem hiding this comment.
Can't we mark all these as required? aren't these marked as required 🤔 ?
There was a problem hiding this comment.
Only the s3_aws_region is a required property as of now. We'll have to introduce another change to mark the other properties as required.
|
|
||
| // skip for legacy flow to avoid snapshot issues | ||
| if (!(input.features && input.features[LIVERAMP_LEGACY_FLOW_FLAG_NAME] === true)) { | ||
| await validateS3Permissions(input.payloads[0], input.request) |
There was a problem hiding this comment.
Should we put this behind another feature flag for rollout?
There was a problem hiding this comment.
Right now i have avoided the validation for the legacy_flag due to snapshot issues, but yeah we can introduce a flag for rollout
| if (!(input.features && input.features[LIVERAMP_LEGACY_FLOW_FLAG_NAME] === true)) { | ||
| // only validate S3 permissions when the validation flag is enabled | ||
| if (input.features && input.features[LIVERAMP_S3_IAM_VALIDATION_FLAG_NAME] === true) { | ||
| await validateS3Permissions(input.payloads[0], input.request) | ||
| } | ||
| } |
There was a problem hiding this comment.
Could this work?
| if (!(input.features && input.features[LIVERAMP_LEGACY_FLOW_FLAG_NAME] === true)) { | |
| // only validate S3 permissions when the validation flag is enabled | |
| if (input.features && input.features[LIVERAMP_S3_IAM_VALIDATION_FLAG_NAME] === true) { | |
| await validateS3Permissions(input.payloads[0], input.request) | |
| } | |
| } | |
| if (input?.features[LIVERAMP_LEGACY_FLOW_FLAG_NAME)!==true && input?.features[LIVERAMP_S3_IAM_VALIDATION_FLAG_NAME] == true){ | |
| await validateS3Permissions(input.payloads[0], input.request) | |
| } |
There was a problem hiding this comment.
added the recommended change
JIRA - https://twilio-engineering.atlassian.net/browse/STRATCONN-6015?atlOrigin=eyJpIjoiMzg0OGZlYWY3MDhjNDE5YWJlNjc1NDA1MjBhMjQ3YjYiLCJwIjoiaiJ9
HeadBucket - https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadBucket.html
This change adds a validation logic to validate the IAM role for the liveramp audienceEnteredS3 action. This is done using HEAD request to the S3 bucket with the IAM credentials in the mappings.
Number of batch events processed by liveramp in 1 day
Testing
Include any additional information about the testing you have completed to
ensure your changes behave as expected. For a speedy review, please check
any of the tasks you completed below during your testing.
Rollout Strategy
Going with the flagon based rollout with following stages