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

credentials/tls: default GRPC_ENFORCE_ALPN_ENABLED to true #7535

Merged
merged 3 commits into from
Sep 4, 2024
Merged

credentials/tls: default GRPC_ENFORCE_ALPN_ENABLED to true #7535

merged 3 commits into from
Sep 4, 2024

Conversation

arjan-bal
Copy link
Contributor

@arjan-bal arjan-bal commented Aug 19, 2024 •
edited
Loading

Related issue: #434

RELEASE NOTES:

  • Clients and servers will reject TLS connections that don't support ALPN. This can be disabled by setting environment variable GRPC_ENFORCE_ALPN_ENABLED to false (case insensitive). Please file a bug if you encounter any issues with this behaviour. The environment variable to revert this behaviour will be removed in an upcoming release.

@arjan-bal arjan-bal added Type: Bug Type: Behavior Change Behavior changes not categorized as bugs and removed Type: Bug labels Aug 19, 2024
@arjan-bal arjan-bal added this to the 1.66 Release milestone Aug 19, 2024
@arjan-bal arjan-bal requested a review from dfawley August 19, 2024 09:55
@arjan-bal arjan-bal assigned dfawley and arjan-bal and unassigned dfawley Aug 19, 2024
@arjan-bal arjan-bal assigned dfawley and unassigned arjan-bal Aug 19, 2024
@codecov Codecov
Copy link

codecov bot commented Aug 19, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.75%. Comparing base (005b092) to head (1b94310).
Report is 12 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #7535      +/-   ##
==========================================
- Coverage   81.91%   81.75%   -0.17%     
==========================================
  Files         361      361              
  Lines       27825    27825              
==========================================
- Hits        22794    22748      -46     
- Misses       3841     3869      +28     
- Partials     1190     1208      +18
Files with missing lines Coverage Δ
internal/envconfig/envconfig.go 100.00% <ø> (ø)

... and 19 files with indirect coverage changes

@dfawley
Copy link
Member

dfawley commented Aug 20, 2024

Let's wait a few weeks for this since we have a lot of other semi-risky things in flight right now.

@arjan-bal arjan-bal modified the milestones: 1.66 Release, 1.67 Release Aug 20, 2024
@dfawley dfawley assigned arjan-bal and unassigned dfawley Aug 29, 2024
@arjan-bal
Copy link
Contributor Author

arjan-bal commented Aug 30, 2024 •
edited
Loading

Waiting to fix internal users before submitting this PR.

@arjan-bal arjan-bal merged commit 70f19ee into grpc:master Sep 4, 2024
14 checks passed
@arjan-bal arjan-bal deleted the alpn branch September 4, 2024 11:25
@atollena
Copy link
Collaborator

atollena commented Sep 5, 2024

Small note on the release note:

Clients and servers will reject TLS connections that don't support ALPN. This can be disabled by setting environment variable GRPC_ENFORCE_ALPN_ENABLED to false (case insensitive). Please file a bug if you encounter any issues with this behaviour. This will become the default behaviour in an upcoming release.

Is the last sentence correct? Shouldn't it just read "The environment variable to revert this behavior will be removed in an upcoming release."?

@arjan-bal
Copy link
Contributor Author

Small note on the release note:

Clients and servers will reject TLS connections that don't support ALPN. This can be disabled by setting environment variable GRPC_ENFORCE_ALPN_ENABLED to false (case insensitive). Please file a bug if you encounter any issues with this behaviour. This will become the default behaviour in an upcoming release.

Is the last sentence correct? Shouldn't it just read "The environment variable to revert this behavior will be removed in an upcoming release."?

You're correct. It was a copy/paste error. Updated the release note

@skonto skonto mentioned this pull request Sep 24, 2024
Merged
This was referenced Oct 5, 2024
Closed
Merged
Merged
@DmitriyMV DmitriyMV mentioned this pull request Oct 22, 2024
Closed
@nr-tdeluna nr-tdeluna mentioned this pull request Nov 22, 2024
Open
3 tasks
@tbg tbg mentioned this pull request Nov 27, 2024
Draft
16 tasks
tbg added a commit to cockroachdb/grpc-go that referenced this pull request Nov 28, 2024
@tbg
envconfig: default GRPC_ENFORCE_ALPN_ENABLED to false
3093b97 
CRDB at v1.68.0 fails to communicate with CRDB at v1.56.3 due to this
check. This is strange, since CRDB uses gRPC throughout, and in both
v1.56.3 and v1.68.0 uses `tls.NewConfig` which ensures that `NextProtos`
always contains `h2`.

gRPC introduced this check in grpc#7535.

See: cockroachdb/cockroach#136278 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dfawley dfawley dfawley approved these changes

Assignees

@arjan-bal arjan-bal

Labels
Type: Behavior Change Behavior changes not categorized as bugs
Projects
None yet
Milestone
1.67 Release
Development

Successfully merging this pull request may close these issues.
3 participants
@arjan-bal @dfawley @atollena