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

Fix S3 and add generic S3 compatible server support #2376

Merged
merged 1 commit into from
Apr 6, 2022
Merged

Fix S3 and add generic S3 compatible server support #2376

merged 1 commit into from
Apr 6, 2022

Conversation

MichaIng
Copy link
Member

@MichaIng MichaIng commented Mar 21, 2022

WIP: AFAIK this still cannot work.

@jmichault
You merged #1063 and finished the class, but it seems you didn't test it?

  • False syntax in main.js prevented secret key and bucket inputs from being used, leading to a failure in boto3.
  • I aligned show/hide method with how upload_authorization_key is handled, initialising it as empty string and then using markHideIfNull to hide it if null. I'm not sure about the rational behind this, but makes sense to follow existing logics.
  • It still does not work. If I'm not mistaken, the authorization key is a one time access key for Google and Dropbox uploads, to authorize the client against the server, not required anymore after done, hence not saved to camera config. For S3 access however it is required on every upload, i.e. needs to be stored in the config, at least bucket and one of the other keys?
  • Also there is this 'upload_bucket_region': $('#uploadBucketRegionEntry').val(), in the code, which from what I see is never used, but instead you re-used the existing location input, which seems reasonable. Generally I'd suggest to re-use username and password configs for the two keys and just show them with different text, so we have no redundant/empty settings stored in camera configs while only one upload service can be selected anyway.

What I'm most unhappy with is that this does not work with other S3 compatible servers, just with AWS. There is another PR for adding MinIO support, but again MinIO only: #1490
This PR may btw help to actually fix the AWS S3 implementation, e.g. see how the configs are handled and persistently stored in camera config.

I wonder whether there is a generic S3 module which works with AWS and MinIO and other S3 compatible servers. From a drop-down we could add a provider selection which translates to a URL (for AWS at least), combined with a "Custom" entry which, when selected, shows an additional custom URL input field.
EDIT: Ah, boto3 seems to be able to upload to other S3 compatible servers via endpoint_url: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/core/session.html#boto3.session.Session.client

Also, I suggest to not make boto3 (or whatever it will be) a hard dependency. Instead, when AWS S3 (or any other S3 server) is selected, and the related module is not found, a notification could be shown about how to install the missing module.

@MichaIng MichaIng added the bug label Mar 21, 2022
@MichaIng MichaIng requested a review from jmichault March 21, 2022 19:44
@MichaIng
Copy link
Member Author

MichaIng commented Mar 21, 2022

@amosshapira
Linking you as well here, since you offered some time ago to finish the original PR. I can grant you access to help fixing/finishing it here, if you are willing to?

@MichaIng
Copy link
Member Author

MichaIng commented Mar 21, 2022

I'm one step further. I hardcoded keys and bucket stings and manually added endpoint_url to point to my MinIO S3 server:

  • At first uploading failed since s3.upload_file requires an absolute file path while upload_data passes a relative path only, and even with os.path.abspath(filename) or prepending with camera_name the absolute path cannot be derived. I guess the CWD is root /, in Enable uploading of media files to minio(s3 compatible) #1490 solves it via camera_config.get('target_dir'). I went a different approach to get the upload working: I replaced the service class' upload_file method instead of defining the upload_data method. The first gets the absolute file path, the second seems more for uploading a data stream while everything else is intended as meta data. Since boto3's upload_file does not take a data stream but a file path indeed, using upload_data seems to be an unnecessary overhead.
    • upload_data is btw only called from the service's upload_file class, only called from upload_media_file which is the actual method called from event handlers. So there is no way that upload_data is missing. It makes sense to have if for upload methods with take data streams, but otherwise I see no reason to not skip it.
  • Without hardcoding keys and buckets, it fails, i.e. it is definitely required to store them persistently in camera config. I'll try to enable this the way done in Enable uploading of media files to minio(s3 compatible) #1490.
  • Additionally, it would be great to have an endpoint URL input field, which defaults to the AWS S3 endpoint but allows to enter any other S3 compatible server. Shouldn't be to hard either. Better to have a generic upload service.

@MichaIng MichaIng force-pushed the s3 branch 3 times, most recently from 95bc099 to 699d892 Compare March 22, 2022 00:29
@MichaIng
Copy link
Member Author

MichaIng commented Mar 22, 2022

Okay, works now with MinIO:

  • Made access key, secret key and bucket persistent options, stored to camera config. They cannot really be merged with other settings, at least it becomes difficult then to show correct label and tooltip in GUI.
  • I added an endpoint URL input field and setting. Leaving it empty leads to AWS being used automatically, setting it to an absolute URL allows to use the upload service with any other S3 compatible server.
  • The unused upload_bucket_region has been removed and the location field is hidden for S3 upload service: S3 allows to set a region indeed, but it is not required, at least not for MinIO. In Enable uploading of media files to minio(s3 compatible) #1490 it was just hardcoded, in current dev the "location" field, which takes a path, not a region, is wrong. So if this is beneficial somehow, we can re-add it as dedicated region setting.
  • Secret Access Key => Secret Key, to better differentiate from Access Key.

The question remains whether we want to make boto3 optional and show instructions in case for installing the module. I'm just not sure how to implement this, how to show the install instructions where the input fields would be shown instead. Any help on this is highly welcome.

@MichaIng MichaIng marked this pull request as ready for review March 22, 2022 00:58
@MichaIng MichaIng changed the title Fix S3 upload service Fix S3 and add generic S3 compatible server support Mar 22, 2022
@MichaIng
Copy link
Member Author

@urandu
Pinging you as well for review and testing, as an alternative to your MinIO only approach.

Signed-off-by: MichaIng <[email protected]>
@MichaIng
Copy link
Member Author

MichaIng commented Apr 6, 2022

Merging this now, it fixes the currently non-functional S3 upload service and fine tuning can be done based on beta testing/feedback.

@MichaIng MichaIng merged commit 6a5953b into dev Apr 6, 2022
@MichaIng MichaIng deleted the s3 branch April 6, 2022 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Question re: Uploading to Amazon S3 Feature request: S3 Upload Functionality
1 participant