Explicitly destroy kube-apiserver-sni resources before the kube-apiserver-service is destroyed.

[ScheererJ]

In case the envoy filter, which is part of kube-apiserver-sni, is not destroyed before the kube-apiserver-service the envoy configuration might become inconsistent. An inconsistent envoy configuration might affect other clusters as no new configuration changes can be applied. The resolution is to destroy the sni resources earlier.

How to categorize this PR?

/area networking
/kind bug

What this PR does / why we need it:
Explicitly destroys the kube-apiserver-sni resources during the shoot cluster deletion to prevent other cluster configuration changes to be affected by a race during the deletion.

Which issue(s) this PR fixes:
None.

Special notes for your reviewer:

Release note:

Fixed a race in shoot cluster deletion, which could affect other clusters as the envoy filter (as part of the kube-apiserver-sni) was not deleted before the kube-apiserver-service. This is now explicitly ensured.

[gardener-robot]

@ScheererJ Thank you for your contribution.

[ialidzhikov]

@ScheererJ , can you give more details about the issue and how to reproduce it?

/assign @mvladev

[mvladev]

@ScheererJ , can you give more details about the issue and how to reproduce it?

I think the problem is that the Service for the kube-apiserver and other components is removed before the EnvoyFilters are removed. The EnvoyFilters reference the service (cluster in envoy).

This causes problems for Istio / Envoy.

[ScheererJ]

@ScheererJ , can you give more details about the issue and how to reproduce it?

/assign @mvladev

@ialidzhikov we had a cluster two weeks ago were deletion was stuck. The remaining envoy filter caused others cluster's envoy configurations to not be applied anymore as the envoy configuration was inconsistent. We did not try to reproduce it, though.

[ialidzhikov]

@ialidzhikov we had a cluster two weeks ago were deletion was stuck. The remaining envoy filter caused others cluster's envoy configurations to not be applied anymore as the envoy configuration was inconsistent. We did not try to reproduce it, though.

Could you please add appropriate release note of type bugfix describing the issue this PR is fixing?

[ScheererJ]

@ialidzhikov we had a cluster two weeks ago were deletion was stuck. The remaining envoy filter caused others cluster's envoy configurations to not be applied anymore as the envoy configuration was inconsistent. We did not try to reproduce it, though.

Could you please add appropriate release note of type bugfix describing the issue this PR is fixing?

@ialidzhikov I added a release note. Any feedback?

[rfranzke]

@ScheererJ Still, why is the destruction task of the extensions.gardener.cloud/v1alpha1.ControlPlane resource depending on the SNI deletion?

[ScheererJ]

@ScheererJ Still, why is the destruction task of the extensions.gardener.cloud/v1alpha1.ControlPlane resource depending on the SNI deletion?

@rfranzke In discussion with @mvladev, he suggested that kube-apiserver-sni should be deleted before kube-apiserver-service to reverse the creation order. From his perspective, kube-apiserver-service should be deleted as part of the controlplane deletion. This is why I added the dependency.

@ialidzhikov I am still trying to figure out where kube-apiserver-service is actually being deleted. It does not seem completely obvious.

[rfranzke]

I don't think we explicitly delete the kube-apiserver service yet. You'd need to introduce this. However, the destruction task of the extensions.gardener.cloud/v1alpha1.ControlPlane resource should be completely independent/unrelated.

[ScheererJ]

I don't think we explicitly delete the kube-apiserver service yet. You'd need to introduce this. However, the destruction task of the extensions.gardener.cloud/v1alpha1.ControlPlane resource should be completely independent/unrelated.

@mvladev How should we proceed then? Should I also introduce the explicit destruction of the kube-apiserver-service or is it sufficient just explicitly delete the kube-apiserver-sni late in the process, but without the dependency to the controlplane?
@rfranzke / @ialidzhikov Your feedback is appreciated as well.

[rfranzke]

I don't know if deletion of the kube-apiserver service would be necessary, but it for sure doesn't harm. Hence, my recommendation: Introduce two new tasks which are both dependent on the deleteKubeAPIServer task: One for deleting the SNI resources, one for deleting the service.

[ScheererJ]

Ok, but so that we are on the same page: The task for the kube-apiserver-service would depend on the kube-apiserver-sni task as we want to ensure that the envoy filter (included in the sni resources) is deleted before the service.

[rfranzke]

OK, sure :) I guess @ialidzhikov were just confused how the other task (deletion of ControlPlane) is related to the SNI/service.

[ialidzhikov]

Hence, my recommendation: Introduce two new tasks which are both dependent on the deleteKubeAPIServer task: One for deleting the SNI resources, one for deleting the service.

This sounds good to me.

Going one step back, as we now know that the kube-apiserver Service is not deleted explicitly, the following statement from the release note does not sound good:

[...] as the envoy filter (as part of the kube-apiserver-sni) was not deleted before the kube-apiserver-service.

We know that both envoy filter and kube-apiserver are deleted as part of the Namespace deletion at the end of the Shoot deletion flow. So what is the real issue? Envoy filter is not deleted when the apiserver Deployment is deleted and this is causing issues? So we now need to delete envoy filter when we delete the apiserver Deployment?

[ScheererJ]

The envoy filter is not located in the shoot namespace on the seed, but in the istio-ingress namespace. Therefore, there is a race.
I changed the order as discussed.

[ialidzhikov]

The envoy filter is not located in the shoot namespace on the seed, but in the istio-ingress namespace. Therefore, there is a race.

You are right, I missed that the envoyfilter is in the istio-ingress ns. I see that the envoyfilter has ownerReference to the Shoot namespace in the Seed and that's how it is deleted.

[ialidzhikov]

/lgtm

[rfranzke]

/lgtm