BCDA-7565: Support S3 file downloads for opt-out files #890
Conversation
| @@ -1,120 +1,122 @@ | |||
| $ANSIBLE_VAULT;1.1;AES256 | |||
There was a problem hiding this comment.
Updated this to include BFD_S3_ENDPOINT=http://localhost:4566
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #890 +/- ##
==========================================
- Coverage 79.63% 79.62% -0.02%
==========================================
Files 93 97 +4
Lines 10671 10852 +181
==========================================
+ Hits 8498 8641 +143
- Misses 1637 1663 +26
- Partials 536 548 +12
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
kyeah
requested review from
karinamzalez,
alex-dzeda and
laurenkrugen-navapbc
|
@kyeah It looks like this is the first use of localstack in the bcda-app project; would this be something that needs to trigger one of the "security implications" checkboxes? |
Thanks! I'll check the "new dependency" box but add a note that it's only installed for local development/testing. It shouldn't need to wait for approval (cc. @StewGoin @bhodges-navapbc) |
| return sc, func() {}, err | ||
| } | ||
|
|
||
| func (handler *S3FileHandler) CleanupOptOutFiles(suppresslist []*OptOutFilenameMetadata) error { |
There was a problem hiding this comment.
Are these opt-out files per-consumer, or shared between them?
There was a problem hiding this comment.
Per consumer (each API team has their own S3 folders that only they have access to.)
| sess := session.Must(session.NewSession()) | ||
|
|
||
| config := aws.Config{ | ||
| Region: aws.String("us-east-1"), |
There was a problem hiding this comment.
Do we see a scenario where a file may not be in us-east-1?
There was a problem hiding this comment.
Ah thanks, I said on our call earlier that it would automatically use whatever is configured in the AWS profile, but I forgot that this is hardcoded.
The BFD S3 buckets will live in us-east-1 so I think we can leave them hardcoded for now to avoid the "too many options/parameters/environment variables" fatigue, but open to alternative thoughts!
I did note that this region is hardcoded in several other places in the codebase (e.g. the insights pipeline and a looot of Jenkins workflows)
🎫 Ticket
https://jira.cms.gov/browse/BCDA-7565
🛠 Changes
db.Close()statements (switch to sqlmock)ℹ️ Context for reviewers
We run a daily Jenkins job that sshes onto an NFS server and reads files from local disk. In the future, the BFD team will be serving files through their S3 bucket; this is an interim solution that switches our file handling to read from their S3 bucket when given the
--filesource s3option.✅ Acceptance Validation
I added files locally to dev S3 and then manually ran the import in dev.
This job imported files successfully.
This job correctly did nothing after seeing nothing in S3.
This job failed due to an invalid file header (which was expected.)
This job showed one skipped file due to an invalid filename (also expected.)
See the ops PR for more details.
🔒 Security Implications
This adds a new local-development-only dependency for Localstack for testing against a fake S3 server.