istio / old_pilot_repo

Deprecated home of Istio's Pilot, now in istio/istio's pilot dir
Apache License 2.0
137 stars 91 forks source link

Adding alpn_protocols for listener ssl context #1664

Closed mdhume closed 6 years ago

mdhume commented 6 years ago

What this PR does / why we need it: @rshriram 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.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer: Let me know if any additional tests need to be added. I ran the e2etests which passed successfully. I also functionally validated it against the grpc-java client server where I had initially seen the issue and it worked successfully this time.

Release note:

NONE
istio-testing commented 6 years ago

@mdhume: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none". Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed. Instructions for interacting with me using PR comments are available [here](https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
istio-testing commented 6 years ago

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://github.com/kubernetes/test-infra/blob/master/commands.md).
rshriram commented 6 years ago

/ok-to-test

rshriram commented 6 years ago

This is pretty neat. I am impressed that you picked up the code base and fixed this nicely :).

rshriram commented 6 years ago

/test pilot-e2e-smoketest

rshriram commented 6 years ago

/lgtm

istio-testing commented 6 years ago

@mdhume: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
prow/pilot-e2e-smoketest.sh 3ebb51244bead1cd00b20e6a4f7005fe1e8e29ab link /test pilot-e2e-smoketest
prow/pilot-presubmit.sh 3ebb51244bead1cd00b20e6a4f7005fe1e8e29ab link /test pilot-presubmit
Instructions for interacting with me using PR comments are available [here](https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://github.com/kubernetes/test-infra/blob/master/commands.md).
rshriram commented 6 years ago

@mdhume please send this PR to istio/istio. We have moved all our code there. I am closing this in the interim.