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

34 remove need for direct auth with s3 #36

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

phwissmann
Copy link
Collaborator

@phwissmann phwissmann commented Nov 19, 2024

This PR changes how S3 uploads are authenticated: instead of using credentials, it will get presigned urls from a server (using the scicat jwt token to authenticate). This allows to not have to distribute any S3 credentials.
Additionally, there are some tuning parameter for multipart-s3 upload in order to optimally use the available bandwidth

@phwissmann phwissmann linked an issue Nov 19, 2024 that may be closed by this pull request
9 tasks
@phwissmann phwissmann force-pushed the 34-remove-need-for-direct-auth-with-s3 branch from d92db01 to 549f285 Compare November 22, 2024 15:28
@phwissmann phwissmann self-assigned this Dec 12, 2024
@phwissmann phwissmann force-pushed the 34-remove-need-for-direct-auth-with-s3 branch 6 times, most recently from ae043ed to 3110222 Compare December 19, 2024 13:46
@phwissmann phwissmann force-pushed the 34-remove-need-for-direct-auth-with-s3 branch from 3110222 to cec7c99 Compare January 6, 2025 15:59
@phwissmann phwissmann force-pushed the 34-remove-need-for-direct-auth-with-s3 branch from 8c964a4 to fc5aa15 Compare January 8, 2025 11:30
@phwissmann phwissmann marked this pull request as ready for review January 8, 2025 11:43
case task.TransferGlobus:
err = GlobusTransfer(config.Transfer.Globus, ingestionTask, task_context, ingestionTask.DatasetFolder.Id, datasetFolder, fullFileArray, notifier)
if err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why add the error handling separately for each transfer case when at line 219 (line 210 before changes), we have a common error handler for both cases? If there's a good reason then the common one should also be removed

@@ -147,12 +144,15 @@ func (w *TaskQueue) RemoveTask(id uuid.UUID) error {

func (w *TaskQueue) ScheduleTask(id uuid.UUID) {
w.taskListLock.RLock()
ingestionTask, found := w.datasetUploadTasks.Get(id)
ingestionTask := w.datasetUploadTasks.GetElement(id)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the a specific reason to use GetElement over Get?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove need for direct auth with S3
2 participants