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

satellite/vouchers: deprecate voucher endpoint, return 'please upgrade' error #2940

Merged
merged 8 commits into from
Sep 4, 2019

Conversation

cam-a
Copy link
Contributor

@cam-a cam-a commented Sep 3, 2019

What: Since we are removing kademlia, we no longer need vouchers. We have already removed the voucher request service on the storage node. This PR removes the logic inside the satellite vouchers endpoint and instead returns a custom error asking the storage node to upgrade to the latest version

Why: We have already removed the storage node voucher request service, but not all storage node operators will have updated to this version. If we remove the satellite vouchers endpoint entirely, those who have not upgraded will see errors and ask the community for help. Returning a custom error asking the storage node to upgrade should remedy the situation

Please describe the tests:

  • Test 1: A test to verify the correct error is returned to the client
  • Test 2:

Please describe the performance impact:

Code Review Checklist (to be filled out by reviewer)

  • Does the PR describe what changes are being made?
  • Does the PR describe why the changes are being made?
  • Does the code follow our style guide?
  • Does the code follow our testing guide?
  • Is the PR appropriately sized? (If it could be broken into smaller PRs it should be)
  • Does the new code have enough tests? (every PR should have tests or justification otherwise. Bug-fix PRs especially)
  • Does the new code have enough documentation that answers "how do I use it?" and "what does it do?"? (both source documentation and higher level, diagrams?)
  • Does any documentation need updating?
  • Do the database access patterns make sense?

@cam-a cam-a requested a review from a team September 3, 2019 16:52
@cla-bot cla-bot bot added the cla-signed label Sep 3, 2019
@ghost ghost requested review from kaloyan-raev and rikysya and removed request for a team September 3, 2019 16:52
@cam-a cam-a added the Request Code Review Code review requested label Sep 3, 2019
littleskunk
littleskunk previously approved these changes Sep 4, 2019
JessicaGreben
JessicaGreben previously approved these changes Sep 4, 2019
Copy link
Contributor

@JessicaGreben JessicaGreben left a comment

Choose a reason for hiding this comment

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

yay less code

expiration: expiration,
}
}
type Endpoint struct{}

// Request receives a voucher request and returns a voucher and an error
Copy link
Contributor

Choose a reason for hiding this comment

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

update comment, do we want to say that this is kept in for backwards compatibility?

type Config struct {
Expiration time.Duration `help:"length of time before a voucher expires" default:"720h0m0s"`
}

// Endpoint for issuing signed vouchers
Copy link
Contributor

Choose a reason for hiding this comment

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

could we update this comment and say this is deprecated

Voucher: voucher,
Status: pb.VoucherResponse_ACCEPTED,
}, nil
return nil, errs.New("Vouchers endpoint is deprecated. Please upgrade your storage node to the latest version.")
Copy link
Contributor

Choose a reason for hiding this comment

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

will this cause any crashing downstream?

Copy link
Contributor Author

@cam-a cam-a Sep 4, 2019

Choose a reason for hiding this comment

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

It shouldn't. The old storage node voucher service just logs errors and continues running and I have a test for checking that the client receives the correct error message

Copy link
Contributor

Choose a reason for hiding this comment

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

ok great

@cam-a cam-a merged commit af5fb8e into master Sep 4, 2019
@cam-a cam-a deleted the purple/remove-sat-vouchers branch September 4, 2019 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants