xds: fix issue when skipping first request

[howardjohn]

The bug (example failure:
https://prow.istio.io/view/gs/istio-prow/pr-logs/pull/istio_istio/39896/integ-cni_istio/1546984256612339712)

• ingressgateway has 1 Gateway
• Gateway is removed
• envoy disconnects
• Envoy reconnects, requests RDS. This hits the INIT/RECONNECT flow.
• RDS hits 'Gateway missing for route' path and gives no response at all
• Next RDS request, we get a "stale nonce" since we have no previously
  sent nonce
• Envoy stuck forever

The fix:

• Remove code path to return empty route instead of no route (matching
  other paths)
• Add assertions to ensure that we don't send empty response to requests
  and that we never count a "stale nonce" if we somehow have no
  previously sent nonce, to ensure there aren't any other issues

Please provide a description of this PR:

[istio-policy-bot]

🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test?

Courtesy of your friendly test nag.

[howardjohn]

/test integ-assertion_istio

[howardjohn]

/retest

[hzxuzhonghu]

When met Gateway missing for route, isn't the listener will be removed too?

[howardjohn]

No, the issue happens because listener is still present for 45s (draining) after removal of the Gateway

/test integ-assertion_istio

(assertion failed due to unrelated issue, fixed in another pr)

[hzxuzhonghu]

IC, should we fix envoy which should not send rds request for a listener in draining state?

[howardjohn]

It seems reasonable to me for envoy to do that otherwise it cannot serve traffic on the draining listener appropriately. We only set drain to 45s but some may do it for hours even where it makes more sense

[ramaraochavali]

LGTM. Would be good if we can add a test case that covers this. But if you want to add later, I am OK to approve this. Let me know.

[howardjohn]

will follow up with more tests, right now I want to make sure we can unblock all the failing e2e tests

…

On Thu, Jul 14, 2022, 4:56 AM Rama Chavali ***@***.***> wrote: LGTM. Would be good if we can add a test case that covers this. But if you want to add later, I am OK to approve this. Let me know. — Reply to this email directly, view it on GitHub <#39916 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXLE523YSOL6KL52RJTVT755RANCNFSM53PWNDLQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[istio-testing]

@howardjohn: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
integ-security-multicluster_istio	aadf93e	link	unknown	/test integ-security-multicluster_istio

Instructions 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.

[howardjohn]

/retest

[SpecialYang]

@howardjohn When we met the case Gateway missing for route, our gateway respond 404 for a while. We think this pr is the reason.

Is it right that we push empty RDS?

istio/pilot/pkg/networking/core/v1alpha3/gateway.go

Lines 354 to 360 in ecb615d

	// This can happen when a gateway has recently been deleted. Envoy will still request route
	// information due to the draining of listeners, so we should not return an error.
	return &route.RouteConfiguration{
	Name: routeName,
	VirtualHosts: []*route.VirtualHost{},
	ValidateClusters: proto.BoolFalse,
	}

[howardjohn]

Its hard to say without more info. what did you actually do in terms of creating/deleting GW/VS?