press icon indicating copy to clipboard operation
press copied to clipboard

refactor(s3): Don't hardcode endpoint and region

Open cogk opened this issue 1 year ago • 1 comments

Don't hardcode endpoint and region for S3 buckets

There were many references to the ap-south-1 region and a global implicit reference to AWS. This PR contains refactors of the S3 bucket systems for "offsite backups", "dashboard uploads" and "Backup Bucket doctype" buckets, to avoid vendor lock-in.

I tried my best to make it backwards compatible with what I assume is the configuration on Frappe Cloud, but I can't check.

Details:

  • Added region and endpoint fields in Press Settings for the "offsite backups" and "dashboard uploads" buckets
    • Fields are: offsite_backups_endpoint, remote_uploads_region, remote_uploads_endpoint
  • Dropped unused fields: offsite_backups_provider (replaced by endpoint), data_40 (typo probably)
  • There might be some agent stuff left that is not handled correctly (e.g. retrieve the ENDPOINT_URL param here)
  • I did not add unit tests

~~cc: @gavindsouza :wave:~~ re: https://github.com/frappe/press/pull/19 https://github.com/frappe/press/pull/22


For the Frappe team

  • There was a hardcoded uploads.frappe.cloud URL somewhere, I just kept it unchanged, but the refactor won't be truly finished until a migration path can be found for you.
  • I'm not so sure about the other buckets (Backup Bucket doctype), they already had the Endpoint and Region fields, but these were not used everywhere (e.g. not in delete_s3_file)

Typical configuration values for the fields

AWS S3 (Fallback/Legacy)

  • Region: leave empty for ap-south-1
  • Endpoint: leave empty for automatic endpoint generation by boto3 = AWS S3 (I believe https://s3.amazonaws.com)

AWS S3

  • Region: ap-south-1
  • Endpoint: https://s3.ap-south-1.amazonaws.com (possibly a custom DNS name such as uploads.example.com)

OVH

  • Region: de
  • Endpoint: https://storage.de.cloud.ovh.net

cogk avatar Sep 17 '24 13:09 cogk

A patch could be written to set the value back in Press Settings if migration needs to be automatic.

NagariaHussain avatar Sep 17 '24 14:09 NagariaHussain

  • [ ] Need to migrate boto3_offsite_backup_session for audit

cogk avatar Oct 17 '24 16:10 cogk

Codecov Report

Attention: Patch coverage is 16.66667% with 35 lines in your changes missing coverage. Please review.

Project coverage is 39.90%. Comparing base (ee3fe50) to head (72b56a8).

Files with missing lines Patch % Lines
press/press/doctype/remote_file/remote_file.py 3.33% 29 Missing :warning:
press/api/site.py 0.00% 6 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2159      +/-   ##
==========================================
- Coverage   39.93%   39.90%   -0.03%     
==========================================
  Files         372      372              
  Lines       28210    28232      +22     
==========================================
+ Hits        11266    11267       +1     
- Misses      16944    16965      +21     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Nov 14 '24 13:11 codecov[bot]