add support for ALPN protocols in listener ssl context#1294
add support for ALPN protocols in listener ssl context#1294rshriram merged 3 commits intoistio:masterfrom
Conversation
|
Hi @mdhume. Thanks for your PR. I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/ok-to-test |
|
Thanks for resubmitting the PR @mdhume .. We just merged a lot of repos. So there is some churn. But we should be good to go by EOD. |
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
Hi, could you tweak the e2e tests in pilot/test/gRPC or something to have the Alpn case covered? |
|
@rshriram sure will look into it...will see if I can add a ccheck for the presence of the alpn protocols. |
|
Or just use a client that sends alpn |
|
@rshriram The go grpc client does send ALPN protocols by default for TLS - https://github.com/grpc/grpc-go/blob/master/credentials/credentials.go#L38 and https://github.com/grpc/grpc-go/blob/master/credentials/credentials.go#L174-L178 . It does not have the additional check to see whether the server has returned a specific ALPN protocol like grpc-java does. Is that sufficient or do we need to add a check to see if envoy is returning a selected ALPN protocol? I did do some tcpdump tests and I could see a protocol being returned in the Server Hello step. |
|
I think that is fine.. |
|
@mdhume: The following test failed, say
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
@rshriram Thanks for helping getting this merged. Can you share which Istio release version this fix will be part of? |
|
This should be in the 0.3 for sure. I am not sure if people cherry picked
it for 0.2.11. You can compile from master. Just follow the steps in
.circleci/config.yml
…On Tue, Nov 7, 2017 at 7:47 PM Mrunmayi Dhume ***@***.***> wrote:
@rshriram <https://github.com/rshriram> Thanks for helping getting this
merged. Can you share which Istio release version this fix will be part of?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1294 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AH0qd4Hv7OzihvZCFu7xpwxiPWjq4vJCks5s0PodgaJpZM4QN2Uc>
.
|
|
@rshriram Sure for local testing we will compile from master and test it out. Would it be possible to get this cherry picked for a version prior to the 0.3 release? I checked out 0.2.11 and 0.2.12 and neither seems to have it yet. We have a team waiting on us for this fix and we were hoping to roll this out by next week preferably. |
|
I don't own the release process. @linsun ?? |
|
@mdhume if this fix is important for you to include it in 0.2, please submit a PR for the release-0.2 branch with a justification on user's impact. Once it is merged, it will be included in the next 0.2.x release. |
|
@linsun sure will do that, thanks! |
|
@linsun sorry I'm a little confused. It seems like the release-0.2 branch in istio/istio repo does not have the pilot folder. Should I be submitting the PR to the old pilot repo instead? |
|
@rshriram @linsun Sorry to bother. Should I be sending the PR to the old pilot repo? The new one does not seem to have a pilot folder in the branch release-0.2 - https://github.com/istio/istio/tree/release-0.2 |
|
@rkpagadala could you pls advise since you are oncall this week? |
|
Yes. Please submit against the old pilot repo |
What this PR does / why we need it:
Copying over the PR changes from istio/pilot git repo - istio/old_pilot_repo#1664
Also addressed the PR comments left in the previous PR
@rshriram @ijsnellf This PR adds the param "alpn_protocols" for listener ssl context as documented here - https://www.envoyproxy.io/envoy/configuration/listeners/ssl.html#config-listener-ssl-context
Currently grpc-java clients require that the proxy return a selected alpn_protocol else it errors out.
The discussion on this issue is detailed here - https://groups.google.com/forum/#!topic/istio-users/MYRflKPQRkA
I am intentionally leaving the cluster ssl context untouched here.