Update Envoy SHA to cherry-pick fix for idle timer segfault.

[PiotrSikora]

Cherry-pick envoyproxy/envoy#3970.

Signed-off-by: Piotr Sikora [email protected]

[istio-testing]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: PiotrSikora
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: jimmycyj

If they are not already assigned, you can assign the PR to them by writing /assign @jimmycyj in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[PiotrSikora]

Cherry-picked instead of rolling forward to minimize untested changes and to avoid picking envoyproxy/envoy#3975, since it could require last minute changes in Pilot.

[rshriram]

Please wait. I have already merged the latest changes in istio (for latest Envoy sha). We can’t cherry pick because we need the gRPC bug fix.

[rshriram]

Sorry I spoke to soon. So we don’t need that fix because we don’t set the idle timer (ie disabled always). We set the stream_idle_timeout.

[rshriram]

Also with regard to 3975, it doesn’t affect pilot as we don’t issue any mutating commands to Envoy.

[rkpagadala]

@rshriram @PiotrSikora Whats the plan for this PR, do we need this for 1.0? We are planning to cut 1.0 today.

[PiotrSikora]

I've got mixed feelings about not cherry-picking this, since if we don't set idle timer somewhere and/or allow users to configure it in the future, then we're risking Envoy crashing instead of just prematurely closing connections... I agree that this probably isn't "critical fix", but also I don't see any cost associated with merging it, especially considering that we just bumped API.

cc @lizan @costinm Thoughts?

[rkpagadala]

@PiotrSikora I bumped the API because it was merged in release-1.0 branch, we need all the code to be consistent when shipped. Also it was a doc fix only. I would prefer not to merge this.

[PiotrSikora]

cc @htuch any thoughts?

[lizan]

Sorry I spoke to soon. So we don’t need that fix because we don’t set the idle timer (ie disabled always). We set the stream_idle_timeout.

@rshriram If we set stream_idle_timeout, then it could trigger the segfault which fixed in envoyproxy/envoy#3970, when stream is created but envoy didn't get full request header, and idle timeout kicked in. @htuch correct me if I am wrong.

[costinm]

We were supposed to cut 1.0 30 min ago.

I think it is too late. If it crashes we'll need to cut 1.0.1 next week

[htuch]

If you set it to zero, you are probably safe, otherwise you need to cherry pick all follow-up PRs.

…

On Mon, Jul 30, 2018 at 6:32 PM Costin Manolache ***@***.***> wrote: We were supposed to cut 1.0 30 min ago. I think it is too late. If it crashes we'll need to cut 1.0.1 next week — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1890 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKaLv6SbX3JsbFPlPBZVlgGcVqMzfvXSks5uL4lpgaJpZM4VmNPo> .

[rshriram]

See pilot code. It’s always set to 0 (stream idle timeout). But we don’t set idle timeout.

And when we allow users to pick idle timeouts in future, we will be upgrading Envoy as well. Ie pull in bug fix automatically.

[PiotrSikora]

Superseded by #1894.