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

[v2] sync command doesn't fail immediately when SSO token expires #4863

Closed
borrell opened this issue Jan 21, 2020 · 5 comments
Closed

[v2] sync command doesn't fail immediately when SSO token expires #4863

borrell opened this issue Jan 21, 2020 · 5 comments
Labels
closed-for-staleness feature-request A feature should be added or improved. needs-review This issue or pull request needs review from a core team member. s3sync

Comments

@borrell
Copy link

borrell commented Jan 21, 2020

When executing an S3 sync with an SSO profile, if the SSO token expires during the sync (e.g. expires via timeout), then the sync will continue, trying each file and displaying an error noting that the token has expired.

aws2 sync --profile SSOProfileName docs/ s3://example.bucket/backups

upload: ../docs/file1.txt to s3://example.bucket/backups/file1.txt 
upload failed: ../docs/file2.txt to s3://example.bucket/backups/file2.txt The SSO session associated with this profile has expired or is otherwise invalid. To refresh this SSO session run aws2 sso login with the corresponding profile.
upload failed: ../docs/file3.txt to s3://example.bucket/backups/file3.txt The SSO session associated with this profile has expired or is otherwise invalid. To refresh this SSO session run aws2 sso login with the corresponding profile.
upload failed: ../docs/file4.txt to s3://example.bucket/backups/file4.txt The SSO session associated with this profile has expired or is otherwise invalid. To refresh this SSO session run aws2 sso login with the corresponding profile.
...

I would imagine (without entirely thinking through any possible side-effects) that the desired behaviour would be for the sync to fail immediately, rather than continue retrying.

@borrell borrell changed the title sync command doesn't fail when SSO token expires [v2] sync command doesn't fail when SSO token expires Jan 21, 2020
@borrell borrell changed the title [v2] sync command doesn't fail when SSO token expires [v2] sync command doesn't fail immediately when SSO token expires Jan 21, 2020
@klaytaybai
Copy link

Hi @borrell, thanks for your feedback. Instead of failing immediately, do you think retrying the current file and not attempting all others would be a preferred behavior?

@klaytaybai klaytaybai added feature-request A feature should be added or improved. s3sync labels Jan 23, 2020
@borrell
Copy link
Author

borrell commented Jan 24, 2020

Retrying the current file would be fine, to a point (see #3 below!)

I've got a few more thoughts, after ruminating on it for a day:

  1. The main issue I see is that it leaves the action in a potentially inconsistent state. For example, If I kick of a sync command, and part-way through the SSO token times out, without a separate reconciliation process to determine which files have been synchronized, there's no easy way to identify which files failed to be synchronized (particularly if you run the command as part of a script, and/or have a log file a million lines long).

  2. I initially tagged this as a V2 issue, however I don't know what the behavior is for V1 (or V2 for that matter) if you are using temporary IAM credentials, that could also expire partway through the command. It would be worth investigating if it fails in the same manner, or if it behaves differently.

  3. As noted above, retrying the file is acceptable, however I'm not sure of the intention for this. It is possible that the token may be refreshed (i.e. a user using a separate terminal to run an 'aws2 sso login' to refresh the token), but outside of that it's not a transient error - it won't recover without some sort of user intervention. In that case, retrying the file won't have any chance of succeeding. The retry would need to have an upper limit (so as not to get stuck in a locked state), and unless the user is sitting there ready to refresh the token, retrying the file once or ten times won't have a difference.

  4. If the command aborted with an indication that the credentials were expired, then it would allow error handling that condition in a script or tool (e.g. detecting that the command aborted due to failed credentials, refreshing them, and retrying).

@klaytaybai
Copy link

Good points. Thanks for the feedback

@mlopezantequera
Copy link

Perhaps these should be options for the client.

In my application, a wrapper script can request a new token and retry the sync.

Sadly the sync only returns after attempting the whole sync (even though the token has expired). This means a lot of time is wasted scanning the source files.

@stobrien89 stobrien89 added the needs-review This issue or pull request needs review from a core team member. label Nov 12, 2021
@github-actions
Copy link

Greetings! It looks like this issue hasn’t been active in longer than one year. We encourage you to check if this is still an issue in the latest release. In the absence of more information, we will be closing this issue soon. If you find that this is still a problem, please feel free to provide a comment or upvote with a reaction on the initial post to prevent automatic closure. If the issue is already closed, please feel free to open a new one.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Nov 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-for-staleness feature-request A feature should be added or improved. needs-review This issue or pull request needs review from a core team member. s3sync
Projects
None yet
Development

No branches or pull requests

4 participants