remove init fetch timeout env var

[ramaraochavali]

We already use default fetch time out for clusters and listeners via bootstrap and 0s for SDS. Only EDS and RDS use this flag which I think should behave similar to clusters and listeners. So there is no need for this env var.
[ ] Configuration Infrastructure
[ ] Docs
[ ] Installation
[ X] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure

Pull Request Attributes

Please check any characteristics that apply to this pull request.

[X ] Does not have any changes that may affect Istio users.

[ramaraochavali]

/retest

[howardjohn]

This removes the ability to set InitialFetchTimeout=0 and not timeout?

[ramaraochavali]

Yes. But do we need that for EDS and RDS when we do not give that ability for CDS and LDS? It looked inconsistent to me.

[howardjohn]

I am not sure this is a safe change to make, see comments inline

[howardjohn]

This will cause a blocked DNS cluster to stop envoy permanently rather than for 15s, which I think is undesired?

[ramaraochavali]

This is just for TLS setting. The DNS cluster block may happen because of resolution not happening which is not impacted by this change and we do not have control over that behaviour. This envoyproxy/envoy#10728 fixed that in Envoy. So we should be good - unless you are thinking about some thing different than this?

[howardjohn]

This means a missing cert (which is possible for Gateways) can block envoy forever

[ramaraochavali]

Ok. So we do want this SDS? If yes, I would say we default to 15s here instead of forever because that is sensible default?. But current default is to block forever

[ramaraochavali]

There is some thing odd about this though. When I made this change, tests are failing even though features.InitialFetchTimeout is same as what I changed here and sdsAdsConfig. Let me debug this bit more

[howardjohn]

I think current behavior is unset which defaults to 15s. New behavior is 0s (forever)

…

On Fri, Feb 19, 2021, 9:18 PM Istio Automation ***@***.***> wrote: @ramaraochavali <https://github.com/ramaraochavali>: The following tests *failed*, say /retest to rerun all failed tests: Test name Commit Details Rerun command integ-security-multicluster-tests_istio 518f3b1 <518f3b1> link <https://prow.istio.io/view/gs/istio-prow/pr-logs/pull/istio_istio/30948/integ-security-multicluster-tests_istio/1362771307636199424> /test integ-security-multicluster-tests_istio unit-tests_istio 164867f <164867f> link <https://prow.istio.io/view/gs/istio-prow/pr-logs/pull/istio_istio/30948/unit-tests_istio/1362993668922806272> /test unit-tests_istio Instructions for interacting with me using PR comments are available here <https://git.k8s.io/community/contributors/guide/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://go.k8s.io/bot-commands>. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#30948 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXJSSRMKR5XV5NO4BZ3S75AZRANCNFSM4X4FK6DQ> .

[ramaraochavali]

I think current behavior is unset which defaults to 15s. New behavior is 0s
(forever)

Sorry I did not follow. This is for SDS?

[ramaraochavali]

I see what you mean. I did not fully look at this method

	timeout, f := testTimeoutVar.Lookup()
		if !f {
			return nil
		}
		return ptypes.DurationProto(timeout)

[ramaraochavali]

@howardjohn I cleanup all the stuff but still removed the env var because

• I think EDS and RDS should be consistent with CDS and LDS (We default it to 15s and do not provide ability to override for them) and I think EDS and RDS do not need the ability. Let me know if you think there are any special cases in them that need that behaviour
• Defaulted the SdsAdsConfig and ConstructSdsSecretConfig to same 15s

Do you have any other concerns on why we need that control? If you think it is still safe to leave it - I am fine to close this.