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

Included timeout option for uploading to Rekor #1001

Merged

Conversation

naveensrinivasan
Copy link
Contributor

Included a timeout option to sign-blob because uploading a large binary to rekor was failing with timeout.

#990

@naveensrinivasan naveensrinivasan force-pushed the naveen/feat/sign-blob-timeout branch 3 times, most recently from e710892 to 29c9379 Compare November 5, 2021 02:03
@lukehinds
Copy link
Member

Not sure I understand the logic here. rekor will timeout regardless of a client timing out, whereas with the client having a timeout you will never know the server has timed out in the first place. I would have thought it more useful for the client to know there has been timeout (and then take action accordingly), rather then bail early under the pretence there may timeout?

@naveensrinivasan
Copy link
Contributor Author

Not sure I understand the logic here. rekor will timeout regardless of a client timing out, whereas with the client having a timeout you will never know the server has timed out in the first place. I would have thought it more useful for the client to know there has been timeout (and then take action accordingly), rather then bail early under the pretence there may timeout?

Makes sense, but the default client timeout is only 30 seconds https://github.com/sigstore/rekor/blob/5bf082ac60f873cee7e6de8a97142907ee0134e7/pkg/generated/client/entries/create_log_entry_parameters.go#L45 and I was following the logic of rekor to increase the timeout https://github.com/sigstore/rekor/blob/5bf082ac60f873cee7e6de8a97142907ee0134e7/cmd/rekor-cli/app/upload.go#L82.

I was able to test the uploads with increase timeout and it did work on a larger blob

What if we include this timeout and also validate with max of what the rekor is? What do your thoughts?

@lukehinds
Copy link
Member

interesting, I never knew we had rekor client timeouts.

in regards to your proposal I am not of strong view here, so can let others chime in. There is no harm in having a timeout flag, I guess as long as users don't believe it will have influence on the servers timeout rules.

Signed-off-by: naveen <172697+naveensrinivasan@users.noreply.github.com>
@dlorenc dlorenc merged commit d442592 into sigstore:main Nov 9, 2021
@github-actions github-actions bot added this to the v1.4.0 milestone Nov 9, 2021
@naveensrinivasan naveensrinivasan deleted the naveen/feat/sign-blob-timeout branch November 9, 2021 18:08
@cpanato cpanato modified the milestones: v1.4.0, v1.3.1 Nov 11, 2021
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.

None yet

4 participants