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 support for alpn protocols in listener ssl context #1683

Closed mdhume closed 6 years ago

mdhume commented 6 years ago

What this PR does / why we need it: @rkpagadala @linsun @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: These changes have been merged into master branch in the istio/istio repo - https://github.com/istio/istio/pull/1294 Without this PR we are currently blocked on using any grpc-java service over TLS with Istio, hence we would be grateful if this gets merged into the next 0.2 release.

Release note:

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).
mdhume commented 6 years ago

@linsun @rkpagadala Any updates on when the next 0.2 release will be? Also when will 0.3 be out?

istio-testing commented 6 years ago

@mdhume: you can't request testing unless you are a istio member.

In response to [this](https://github.com/istio/old_pilot_repo/pull/1683#issuecomment-344276728): >@linsun @rkpagadala Any updates on when the next 0.2 release will be? Also when will 0.3 be out? 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.
rkpagadala commented 6 years ago

@mdhume We are currently not planning on a .2 release. We expect to release a .3 in a couple of weeks.

codecov[bot] commented 6 years ago

Codecov Report

Merging #1683 into release-0.2 will increase coverage by <.01%. The diff coverage is 100%.

Impacted file tree graph

@@               Coverage Diff               @@
##           release-0.2    #1683      +/-   ##
===============================================
+ Coverage        82.69%   82.69%   +<.01%     
===============================================
  Files               52       52              
  Lines             6426     6427       +1     
===============================================
+ Hits              5314     5315       +1     
  Misses             909      909              
  Partials           203      203
Impacted Files Coverage Δ
proxy/envoy/resources.go 85.08% <ø> (ø) :arrow_up:
proxy/envoy/ingress.go 77.27% <100%> (+0.17%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 998e0e0...00550ca. Read the comment docs.

istio-testing commented 6 years ago

@codecov[bot]: you can't request testing unless you are a istio member.

In response to [this](https://github.com/istio/old_pilot_repo/pull/1683#issuecomment-344371689): ># [Codecov](https://codecov.io/gh/istio/old_pilot_repo/pull/1683?src=pr&el=h1) Report >> Merging [#1683](https://codecov.io/gh/istio/old_pilot_repo/pull/1683?src=pr&el=desc) into [release-0.2](https://codecov.io/gh/istio/old_pilot_repo/commit/998e0e00d375688bcb2af042fc81a60ce5264009?src=pr&el=desc) will **increase** coverage by `<.01%`. >> The diff coverage is `100%`. > >[![Impacted file tree graph](https://codecov.io/gh/istio/old_pilot_repo/pull/1683/graphs/tree.svg?height=150&width=650&token=tMOrVsUuql&src=pr)](https://codecov.io/gh/istio/old_pilot_repo/pull/1683?src=pr&el=tree) > >```diff >@@ Coverage Diff @@ >## release-0.2 #1683 +/- ## >=============================================== >+ Coverage 82.69% 82.69% +<.01% >=============================================== > Files 52 52 > Lines 6426 6427 +1 >=============================================== >+ Hits 5314 5315 +1 > Misses 909 909 > Partials 203 203 >``` > > >| [Impacted Files](https://codecov.io/gh/istio/old_pilot_repo/pull/1683?src=pr&el=tree) | Coverage Δ | | >|---|---|---| >| [proxy/envoy/resources.go](https://codecov.io/gh/istio/old_pilot_repo/pull/1683?src=pr&el=tree#diff-cHJveHkvZW52b3kvcmVzb3VyY2VzLmdv) | `85.08% <ø> (ø)` | :arrow_up: | >| [proxy/envoy/ingress.go](https://codecov.io/gh/istio/old_pilot_repo/pull/1683?src=pr&el=tree#diff-cHJveHkvZW52b3kvaW5ncmVzcy5nbw==) | `77.27% <100%> (+0.17%)` | :arrow_up: | > >------ > >[Continue to review full report at Codecov](https://codecov.io/gh/istio/old_pilot_repo/pull/1683?src=pr&el=continue). >> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) >> `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` >> Powered by [Codecov](https://codecov.io/gh/istio/old_pilot_repo/pull/1683?src=pr&el=footer). Last update [998e0e0...00550ca](https://codecov.io/gh/istio/old_pilot_repo/pull/1683?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments). > 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.
ldemailly commented 6 years ago

please make PRs in istio/istio/pilot

ldemailly commented 6 years ago

I did not notice at first this was for the release branch, my bad, but is it done on master already - which PR ?

also I don’t think we will be making a 0.2 patch after 0.2.12 - this should go on master/0.3.0

mdhume commented 6 years ago

@ldemailly Yes it was merged here - https://github.com/istio/istio/pull/1294 . Is there any ETA on 0.3? We really need this fix to get grpc-java working for us.

ldemailly commented 6 years ago

we're supposed to be doing daily and weekly builds... the mono repo and other changes have delayed this so far but it should be imminent cc @guptasu , @mattdelco, @jasminejaksic

mattdelco commented 6 years ago

The goal/expectation is to have a release this week. The remaining tasks are mostly about navigating certain infrastructure and procedural issues/barriers (some of which are harder to workaround during a holiday week) so I don't have a more precise ETA besides Mon-Wed.

mattdelco commented 6 years ago

New ETA is Monday. The build process was running fine but we had difficulties locating a version of the code base that would pass all the required test suites. A viable candidate was identified towards the end of the day but the actual release won't happen until after the coming holiday weekend.

mdhume commented 6 years ago

@mattdelco Sounds good, thanks for the update.