[Issue #3094] Use CDN URLs for attachments instead of S3 URLs #3659

mikehgrantsgov · 2025-01-28T17:00:04Z

Summary

Time to review: 10 mins

Changes proposed

Branch logic for attachment URLs whether CDN_URL is set or not. If set, use CDN_URL to replace s3:///<path_to_file>

Context for reviewers

Instead of changing actual values in the DB, evaluate at run-time if the CDN_URL is set and use that value instead of presigning S3 URLS.

Additional information

One thing to figure out: AttachmentConfig relies on *.env files being set, but doesn't appear to work when setting ENV vars via Monkeypatch.

api/src/util/file_util.py

chouinar · 2025-01-28T17:18:29Z

api/tests/src/api/opportunities_v1/test_opportunity_route_get.py

+def test_get_opportunity_returns_cdn_urls(
+    client, api_auth_token, monkeypatch_session, enable_factory_create, db_session, mock_s3_bucket
+):
+    monkeypatch_session.setenv("CDN_URL", "https://cdn.example.com")


Could we set this in some autouse conftest value? Otherwise we'll need to set it for every API test with attachments. Fine to override it here.

Also we should set CDN_URL in local.env to be localhost:4566 and test that this works locally as well?

Tried this locally and receive:

This would be the correct path is <s3_path> was replaced with <cdn_path>

If you go to that path, does the file download work? I think for local, it also needs the bucket name (ie. CDN_URL should include the bucket for our local dev purposes).

I added the bucket name to CDN_URL in local.env and it works. Checking that in shortly.

…ents

coilysiren · 2025-01-28T21:56:59Z

api/src/services/opportunities_v1/get_opportunity.py

+
+class AttachmentConfig(PydanticBaseEnvConfig):
+    # If the CDN URL is set, we'll use it instead of pre-signing the file locations
+    cdn_url: str | None = None


This doesn't need to be in capslock?

Pydantic will look for CDN_URL as an env var. We sometimes also give values an alias if we want a cleaner python name, but doesn't matter too much here.

coilysiren · 2025-01-28T21:58:14Z

api/src/util/file_util.py

+    if not is_s3_path(file_path):
+        raise ValueError(f"Expected s3:// path, got: {file_path}")
+
+    return file_path.replace(os.environ["PUBLIC_FILES_BUCKET"], cdn_url)


Have you testing that this URL is exactly right?

Yes we added tests to cover a few scenarios: https://github.com/HHS/simpler-grants-gov/pull/3659/files#diff-e284f21c2216feb0f0612d0cbed5d770d98a4fccb1591714ce50bb9dc47910d0R214

Also the only scenarios where S3 -> CDN replacement happens is explicitly for files in the public files bucket.

chouinar · 2025-01-29T14:31:30Z

api/src/services/opportunities_v1/get_opportunity.py

+
+class AttachmentConfig(PydanticBaseEnvConfig):
+    # If the CDN URL is set, we'll use it instead of pre-signing the file locations
+    cdn_url: str | None = None


Pydantic will look for CDN_URL as an env var. We sometimes also give values an alias if we want a cleaner python name, but doesn't matter too much here.

chouinar · 2025-01-29T14:32:13Z

api/src/services/opportunities_v1/get_opportunity.py

+    else:
+        pre_sign_opportunity_file_location(opportunity.opportunity_attachments)


I'm not sure there will be a case where it's not set once we merge this. (NOTE - we should first verify with @coilysiren that the CDN is fully working before we merge this - I think it's still in progress)

chouinar · 2025-01-29T14:33:02Z

api/src/util/file_util.py

+    if not is_s3_path(file_path):
+        raise ValueError(f"Expected s3:// path, got: {file_path}")
+
+    return file_path.replace(os.environ["PUBLIC_FILES_BUCKET"], cdn_url)


Can you use the s3 config for this instead of the os module? Just so we only define the env vars in a single place

chouinar · 2025-01-29T14:34:03Z

api/tests/src/api/opportunities_v1/test_opportunity_route_get.py

+def test_get_opportunity_returns_cdn_urls(
+    client, api_auth_token, monkeypatch_session, enable_factory_create, db_session, mock_s3_bucket
+):
+    monkeypatch_session.setenv("CDN_URL", "https://cdn.example.com")


If you go to that path, does the file download work? I think for local, it also needs the bucket name (ie. CDN_URL should include the bucket for our local dev purposes).

chouinar · 2025-01-31T14:36:42Z

@mikehgrantsgov - FYI - the change looks good, but holding approval until we can confirm the CDN itself is actually working.

chouinar

LGTM - I think we're safe to merge this now.

After this is deployed, can you check that dev is using the CDN URL instead of presigned URLs?

Here's an opportunity with two attachments already on it: http://frontend-dev-1739892538.us-east-1.elb.amazonaws.com/opportunity/70111

## Summary Fixes #3094 ### Time to review: 10 mins ## Changes proposed Branch logic for attachment URLs whether CDN_URL is set or not. If set, use CDN_URL to replace s3://<foo>/<path_to_file> ## Context for reviewers Instead of changing actual values in the DB, evaluate at run-time if the CDN_URL is set and use that value instead of presigning S3 URLS. ## Additional information One thing to figure out: `AttachmentConfig` relies on *.env files being set, but doesn't appear to work when setting ENV vars via Monkeypatch.

mikehgrantsgov added 3 commits January 28, 2025 11:57

Add AttachmentConfig, branch logic for attachment URLs if CDN_URL is set

371ca7f

Format / lint

3bc548e

Load via config

de4bd08

chouinar reviewed Jan 28, 2025

View reviewed changes

mikehgrantsgov added 5 commits January 28, 2025 12:29

Add autouse fixture, use join

57ee4f8

Use an explicit str replace to make sure we only replace public paths

90ab8a1

Update file tests

d5caae0

Rename method / lint

b90c413

Format

40fe954

mikehgrantsgov requested a review from chouinar January 28, 2025 19:53

Merge branch 'main' into mikehgrantsgov/3094-use-cdn-urls-for-attachm…

985a0a0

…ents

mikehgrantsgov marked this pull request as ready for review January 28, 2025 19:54

mikehgrantsgov requested review from mdragon, acouch, coilysiren and widal001 as code owners January 28, 2025 19:54

coilysiren reviewed Jan 28, 2025

View reviewed changes

chouinar reviewed Jan 29, 2025

View reviewed changes

Use S3Config vs. os path

dcfe46f

mikehgrantsgov requested a review from chouinar January 29, 2025 16:47

mdragon changed the title ~~[Issue #3094] Update database records to use CDN URLs for attachments instead of S3 URLs~~ [Issue #3094] Use CDN URLs for attachments instead of S3 URLs Feb 4, 2025

chouinar approved these changes Feb 5, 2025

View reviewed changes

mikehgrantsgov merged commit 971b38f into main Feb 5, 2025
3 checks passed

mikehgrantsgov deleted the mikehgrantsgov/3094-use-cdn-urls-for-attachments branch February 5, 2025 21:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue #3094] Use CDN URLs for attachments instead of S3 URLs #3659

[Issue #3094] Use CDN URLs for attachments instead of S3 URLs #3659

mikehgrantsgov commented Jan 28, 2025

chouinar Jan 28, 2025

mikehgrantsgov Jan 28, 2025

chouinar Jan 29, 2025

mikehgrantsgov Jan 29, 2025

coilysiren Jan 28, 2025

chouinar Jan 29, 2025

coilysiren Jan 28, 2025

mikehgrantsgov Jan 29, 2025

chouinar Jan 29, 2025

chouinar Jan 29, 2025

chouinar Jan 29, 2025

chouinar Jan 29, 2025

chouinar commented Jan 31, 2025

chouinar left a comment

		else:
		pre_sign_opportunity_file_location(opportunity.opportunity_attachments)

[Issue #3094] Use CDN URLs for attachments instead of S3 URLs #3659

[Issue #3094] Use CDN URLs for attachments instead of S3 URLs #3659

Conversation

mikehgrantsgov commented Jan 28, 2025

Summary

Time to review: 10 mins

Changes proposed

Context for reviewers

Additional information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chouinar commented Jan 31, 2025

chouinar left a comment

Choose a reason for hiding this comment