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

[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

Conversation

pierresouchay
Copy link
Contributor

Tnis is an alternative to #9494

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

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.

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

@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

This ensures that hashicorp#9474 will
not reproduce.
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
[Streaming][bugfix] handle TLS signalisation when TLS is disabled on client side (alternative to #9494)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/streaming Related to Streaming connections between server and client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants