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

[Streaming][bugfix] handle TLS signalisation when TLS is disabled on client side (alternative to #9494) #9512

Merged
merged 2 commits into from
Jan 6, 2021
Merged

[Streaming][bugfix] handle TLS signalisation when TLS is disabled on client side (alternative to #9494) #9512

merged 2 commits into from
Jan 6, 2021

Conversation

pierresouchay
Copy link
Contributor

Tnis is an alternative to #9494

We just patch the GRPC side, not everything as suggested by @dnephin

@pierresouchay pierresouchay force-pushed the streaming_fix_grpc_tls2 branch from 8a03283 to e2f2d4b Compare January 6, 2021 16:25
@pierresouchay pierresouchay mentioned this pull request Jan 6, 2021
Closed
dnephin
dnephin reviewed Jan 6, 2021
View reviewed changes
Copy link
Contributor

@dnephin dnephin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice! looks good! Do you think it would be possible to write a test for newDialer that fails without this change? I think there is an integration test somewhere in the agent/grpc package that uses fakeRPCListener that might work.

@pierresouchay
Copy link
Contributor Author

pierresouchay commented Jan 6, 2021 •
edited
Loading

@dnephin Done!

Added unit test in new commit on this PR: c5a2e3a

Putting this file on current master fails, succeeds with this patch.

Edit: I did server tests as I fought too much on trying to deal properly with TLS Dialers

@pierresouchay
Added testing of GRPC with TLS combinations
994fe80 
This ensures that hashicorp#9474 will
not reproduce.
@pierresouchay pierresouchay force-pushed the streaming_fix_grpc_tls2 branch from c5a2e3a to 994fe80 Compare January 6, 2021 21:21
dnephin
dnephin approved these changes Jan 6, 2021
View reviewed changes
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM!

@dnephin dnephin added backport/1.9 theme/streaming Related to Streaming connections between server and client labels Jan 6, 2021
@dnephin
Copy link
Contributor

dnephin commented Jan 6, 2021

Those integration tests seem to be failing on master, so not related to this change.

@dnephin dnephin merged commit 326a312 into hashicorp:master Jan 6, 2021
@hashicorp-ci
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/305585.

@hashicorp-ci
Copy link
Contributor

🍒✅ Cherry pick of commit 326a312 onto release/1.9.x succeeded!

hashicorp-ci pushed a commit that referenced this pull request Jan 6, 2021
@dnephin @hashicorp-ci
Merge pull request #9512 from pierresouchay/streaming_fix_grpc_tls2
84a9ac1 
[Streaming][bugfix] handle TLS signalisation when TLS is disabled on client side (alternative to #9494)
@dnephin dnephin mentioned this pull request Jan 6, 2021
Closed
@dnephin dnephin mentioned this pull request Oct 27, 2021
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnephin dnephin dnephin approved these changes

Assignees
No one assigned
Labels
theme/streaming Related to Streaming connections between server and client
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pierresouchay @dnephin @hashicorp-ci