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

Add note about the status of the legacy HTTP API. #531

Merged
merged 2 commits into from May 1, 2022

Conversation

znewman01
Copy link
Contributor

Summary

Small doc clarification about the legacy HTTP API.

Ticket Link

No ticket, but this came up in the 2022-04-19 community meeting.

Release Note

NONE

@znewman01
Copy link
Contributor Author

CC @bobcallaway @haydentherapper

Let me know if my understanding isn't quite right; I'm happy to revise the docs.

@haydentherapper
Copy link
Contributor

I think I'd like a stronger statement, something like "All features should be ported to legacy HTTP API. If there's a feature that you think would negatively impact the HTTP API, open an issue to discuss."

The new gRPC API does support calls over HTTP too.

I'd propose that we keep both APIs around through the V1 launch, and consider removing the legacy API closer to a V2 launch, unless maintenance of both is trivial.

@znewman01
Copy link
Contributor Author

Roger. I cribbed that word-for-word, and added it in both fulcio.proto and fulcio_legacy.proto.

@haydentherapper
Copy link
Contributor

Looks good! I'll let Bob do the approval for the PR, in case he had a different plan in mind for the legacy HTTP API.

dlorenc
dlorenc previously approved these changes Apr 20, 2022
@bobcallaway
Copy link
Member

My intent in naming the v1 endpoint as "legacy" and marking all of the fields in fulcio_legacy.proto as deprecated was to actually drive the opposite outcome than what is written in this PR - that the v1 endpoint should be closed to all new feature development and all additions should be made to v2 only going forward (with appropriate exceptions made for any security-related issues).

I'm not supportive of having two functionally equivalent HTTP API endpoints with semantic differences that introduce technical debt and client confusion.

+1 to making the above intent more explicit in the code comments, as well as making explicit that all additions/changes to the v2 API need to include HTTP support so as to not make gRPC a requirement for clients.

@znewman01
Copy link
Contributor Author

+1 to the sentiment, @bobcallaway; that's what I had initially assumed and then some confusion in that regard was what prompted this. What I found when I asked at the community meeting last week was that there was general support for maintaining both, though I'm not sure whether any of those opinions were strongly held.

I'm happy to revise but want to double check whether there's any reason to keep updating the old one? If we're ensuring that the new API works over HTTP I can't think of any.

CC @haydentherapper, do you remember who else was saying we should backport stuff?

@haydentherapper
Copy link
Contributor

There was a thought that clients may want to keep the legacy API around longer, but given that the V2 endpoint supports both gRPC and HTTP, just keeping V2 should be fine. +1 to what's been said - Let's close V1 to further feature development, and encourage clients to move off V1.

@bobcallaway
Copy link
Member

@znewman01 do you want to change this PR to update the wording or do you want me to do that in a separate PR?

@znewman01
Copy link
Contributor Author

@znewman01 do you want to change this PR to update the wording or do you want me to do that in a separate PR?

@bobcallaway I'll do it!

Signed-off-by: Zachary Newman <z@znewman.net>
@znewman01
Copy link
Contributor Author

Just pushed; should be ready for a look.

Copy link
Member

@bobcallaway bobcallaway left a comment

Choose a reason for hiding this comment

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

1 nit but the intent is clearer, thanks for revising this!

fulcio_legacy.proto Outdated Show resolved Hide resolved
Co-authored-by: Bob Callaway <bobcallaway@users.noreply.github.com>
Signed-off-by: Zachary Newman <z@znewman.net>
@znewman01
Copy link
Contributor Author

1 nit but the intent is clearer, thanks for revising this!

Done

@codecov-commenter
Copy link

codecov-commenter commented Apr 29, 2022

Codecov Report

Merging #531 (54c1dd7) into main (38798fe) will increase coverage by 1.93%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #531      +/-   ##
==========================================
+ Coverage   33.75%   35.68%   +1.93%     
==========================================
  Files          18       18              
  Lines        1357     1415      +58     
==========================================
+ Hits          458      505      +47     
- Misses        836      851      +15     
+ Partials       63       59       -4     
Impacted Files Coverage Δ
pkg/api/error.go 100.00% <0.00%> (ø)
pkg/api/client.go 0.00% <0.00%> (ø)
pkg/api/legacy_server.go 0.00% <0.00%> (ø)
pkg/api/grpc_server.go 52.97% <0.00%> (+7.24%) ⬆️
pkg/challenges/challenges.go 28.78% <0.00%> (+7.30%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38798fe...54c1dd7. Read the comment docs.

@bobcallaway bobcallaway merged commit 3729384 into sigstore:main May 1, 2022
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

5 participants