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.

Sign up for GitHub

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

Jump to bottom

[alpn] Remove grpc-exp experimental ALPN protocol. #34876

Closed
wants to merge 3 commits into from
Closed

[alpn] Remove grpc-exp experimental ALPN protocol. #34876

wants to merge 3 commits into from

Conversation

matthewstevenson88
Copy link
Contributor

This fixes #21619. This experimental ALPN protocol has already been removed from the other gRPC stacks.

@matthewstevenson88 matthewstevenson88 added area/security release notes: yes Indicates if PR needs to be in release notes labels Nov 3, 2023
@matthewstevenson88 matthewstevenson88 self-assigned this Nov 3, 2023
@matthewstevenson88 matthewstevenson88 marked this pull request as ready for review November 3, 2023 20:45
gtcooke94
gtcooke94 approved these changes Nov 3, 2023
View reviewed changes
Copy link
Contributor

@gtcooke94 gtcooke94 left a comment

Choose a reason for hiding this comment

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

LGTM, one comment if you want to make the change bigger

src/core/ext/transport/chttp2/alpn/alpn.cc
@@ -25,7 +25,7 @@
#include "src/core/lib/gpr/useful.h"

// in order of preference
static const char* const supported_versions[] = {"grpc-exp", "h2"};
static const char* const supported_versions[] = {"h2"};
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we plan to have any other supported versions? Otherwise I wonder if it's worth it to just remove this complexity altogether and have a non-arr const value for h2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm planning a follow-up PR to clean up this file (some of the APIs, e.g. grpc_chttp2_get_alpn_version_index, are now unused). Do you mind if we defer this to the cleanup PR?

@matthewstevenson88
Copy link
Contributor Author

@markdroth @ejona86 Could one of you confirm that this should be safe? The grpc-exp has already been removed from Go and Java, but I'd like to confirm that there are no edge cases for the C-core or wrapped languages that are still dependent on this ALPN protocol.

@ctiller
Copy link
Member

ctiller commented Nov 3, 2023 via email

@matthewstevenson88
Copy link
Contributor Author

@markdroth or @ejona86 Could one of you confirm that this should be safe, as far as you're aware?

ejona86
ejona86 reviewed Dec 18, 2023
View reviewed changes
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

I believe this to be safe

@matthewstevenson88
Copy link
Contributor Author

Thanks Eric! Will merge when the repo is unlocked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ejona86 ejona86 ejona86 left review comments

@rockspore rockspore rockspore approved these changes

@gtcooke94 gtcooke94 gtcooke94 approved these changes

@markdroth markdroth Awaiting requested review from markdroth

Assignees

@matthewstevenson88 matthewstevenson88

Labels
area/security bloat/none lang/core per-call-memory/decrease per-channel-memory/neutral release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove grpc-exp from ALPN
5 participants
@matthewstevenson88 @ctiller @ejona86 @rockspore @gtcooke94