🐛 Fix Issues with Missing Gateway Objects on Hibernated Clusters (#37)

* Fix issues with hibernated clusters
* Use istio namespace from extension state as fallback

pkg/controller/actuator.go

@@ -134,14 +134,17 @@ func (a *actuator) Reconcile(ctx context.Context, log logr.Logger, ex *extension
 
 	istioNamespace, istioLabels, err := a.findIstioNamespaceForExtension(ctx, ex)
 	if err != nil {
+		// we ignore errors for hibernated clusters if they don't have a Gateway
+		// resource for the extension to get the istio namespace from
+		if controller.IsHibernated(cluster) {
+			return client.IgnoreNotFound(err)
+		}
 		return err
 	}
 
-	extState := &ExtensionState{}
-	if ex.Status.State != nil && ex.Status.State.Raw != nil {
-		if err := json.Unmarshal(ex.Status.State.Raw, &extState); err != nil {
-			return err
-		}
+	extState, err := getExtensionState(ex)
+	if err != nil {
+		return err
 	}
 
 	if err := a.triggerWebhook(ctx, ex.GetNamespace(), istioNamespace); err != nil {
@@ -261,16 +264,33 @@ func (a *actuator) Delete(ctx context.Context, log logr.Logger, ex *extensionsv1
 	namespace := ex.GetNamespace()
 	log.Info("Component is being deleted", "component", "", "namespace", namespace)
 
-	istioNamespace, _, err := a.findIstioNamespaceForExtension(ctx, ex)
-	if err != nil {
+	if err := a.deleteSeedResources(ctx, log, namespace); err != nil {
 		return err
 	}
 
-	if err := a.triggerWebhook(ctx, namespace, istioNamespace); err != nil {
+	var istioNamespace string
+
+	istioNamespace, _, err := a.findIstioNamespaceForExtension(ctx, ex)
+	if client.IgnoreNotFound(err) != nil {
 		return err
 	}
+	if apierrors.IsNotFound(err) {
+		exState, err := getExtensionState(ex)
+		if err != nil {
+			return err
+		}
+		if exState.IstioNamespace == nil {
+			// we have never reconciled this cluster completely, therefore no
+			// cleanup needs to be performed
+			return nil
+		}
 
-	return a.deleteSeedResources(ctx, log, namespace)
+		// the cluster has no Gateway object, but we can get the information
+		// from the extension state
+		istioNamespace = *exState.IstioNamespace
+	}
+
+	return a.triggerWebhook(ctx, namespace, istioNamespace)
 }
 
 // Restore the Extension resource.
@@ -437,6 +457,17 @@ func (a *actuator) updateStatus(
 	return a.client.Status().Patch(ctx, ex, patch)
 }
 
+func getExtensionState(ex *extensionsv1alpha1.Extension) (*ExtensionState, error) {
+	extState := &ExtensionState{}
+	if ex.Status.State != nil && ex.Status.State.Raw != nil {
+		if err := json.Unmarshal(ex.Status.State.Raw, &extState); err != nil {
+			return nil, err
+		}
+	}
+
+	return extState, nil
+}
+
 // triggerWebhook allows us to "reconcile" the existing EnvoyFilter which we
 // need to modify using a mutating webhook. This is achieved by sending an empty
 // patch for this EnvoyFilter, which invokes the ACL webhook component. This
@@ -500,9 +531,26 @@ func (a *actuator) getAllShootsWithACLExtension(
 		var shootIstioLabels map[string]string
 
 		shootIstioNamespace, shootIstioLabels, err = a.findIstioNamespaceForExtension(ctx, &extensions.Items[i])
-		if err != nil {
+		if client.IgnoreNotFound(err) != nil {
 			return nil, nil, err
 		}
+		// If we don't find a Gateway object to get the Istio Namespace from, we
+		// try the extension status as a fallback. If both aren't available, we
+		// ignore the Shoot's ACL rules entirely in this pass. This can only
+		// occur when the ACL extension for the Shoot in question itself has
+		// never been reconciled before.
+		if apierrors.IsNotFound(err) {
+			extState, err := getExtensionState(ex)
+			if err != nil {
+				return nil, nil, err
+			}
+			if extState.IstioNamespace == nil {
+				continue
+			}
+
+			shootIstioNamespace = *extState.IstioNamespace
+		}
+
 		if istioNamespace != shootIstioNamespace {
 			continue
 		}
@@ -568,11 +616,14 @@ func HashData(data interface{}) (string, error) {
 	return strings.ToLower(base32.StdEncoding.EncodeToString(bytes[:]))[:16], nil
 }
 
-// findIstioNamepsaceForExtension finds the Istio namespace by the Istio Gateway
+// findIstioNamespaceForExtension finds the Istio namespace by the Istio Gateway
 // object named "kube-apiserver", which is expected to be present in every
-// Shoot namespace. This Gateway object has a Selector field that selects a
-// Deployment in the namespace we need. We list Deployments filtered by the
-// labelSelector and return the namespace of the returned Deployment.
+// Shoot namespace (except when the Shoot is hibernated - in this case, the
+// function returns a NotFoundError which the caller should handle).
+//
+// The Gateway object has a Selector field that selects a Deployment in the
+// namespace we need. We list Deployments filtered by the labelSelector and
+// return the namespace of the returned Deployment.
 func (a *actuator) findIstioNamespaceForExtension(
 	ctx context.Context, ex *extensionsv1alpha1.Extension,
 ) (

pkg/controller/actuator_test.go

@@ -181,6 +181,52 @@ var _ = Describe("actuator test", func() {
 				ContainSubstring("5.6.7.8"),
 			))
 		})
+
+		It("should not fail when the Gateway resource can't be found for an extension other than the one being reconciled (e.g. for hibernated clusters)", func() {
+			// arrange
+			extSpec1 := ExtensionSpec{
+				Rule: &envoyfilters.ACLRule{
+					Cidrs:  []string{"1.2.3.4/24"},
+					Action: "ALLOW",
+					Type:   "remote_ip",
+				},
+			}
+			extSpecJSON1, err := json.Marshal(extSpec1)
+			Expect(err).To(BeNil())
+			ext1 := createNewExtension(shootNamespace1, extSpecJSON1)
+			Expect(ext1).To(Not(BeNil()))
+
+			// contents of the seconds extension don't matter, it just needs to exist
+			ext2 := createNewExtension(shootNamespace2, []byte("{}"))
+			Expect(ext2).To(Not(BeNil()))
+
+			// simulate a hibernated cluster by deleting the Gateway object
+			gw2 := &istionetworkingv1beta1.Gateway{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "kube-apiserver",
+					Namespace: shootNamespace2,
+				},
+			}
+			Expect(k8sClient.Delete(ctx, gw2)).To(Succeed())
+
+			// act (reconcile the existing extension)
+			Expect(a.Reconcile(ctx, logger, ext1)).To(Succeed())
+
+			// assert
+			envoyFilter := &istionetworkingClientGo.EnvoyFilter{}
+			Expect(k8sClient.Get(
+				ctx, types.NamespacedName{
+					Name:      "acl-vpn",
+					Namespace: istioNamespace1,
+				},
+				envoyFilter,
+			)).To(Succeed())
+
+			// we expect the filter to contain the settings from the first extension
+			Expect(envoyFilter.Spec.MarshalJSON()).To(And(
+				ContainSubstring("1.2.3.4"),
+			))
+		})
 	})
 
 	Describe("a shoot switching the istio namespace (e.g. when being migrated to HA)", func() {
@@ -248,6 +294,47 @@ var _ = Describe("actuator test", func() {
 			Expect(apierrors.IsNotFound(err)).To(BeTrue())
 		})
 	})
+
+	Describe("deletion of a hibernated cluster (no Gateway resource exists)", func() {
+		It("should properly clean up according ManagedResource", func() {
+			// arrange
+			extSpec := ExtensionSpec{
+				Rule: &envoyfilters.ACLRule{
+					Cidrs:  []string{"1.2.3.4/24"},
+					Action: "ALLOW",
+					Type:   "remote_ip",
+				},
+			}
+			extSpecJSON, err := json.Marshal(extSpec)
+			Expect(err).To(BeNil())
+			ext := createNewExtension(shootNamespace1, extSpecJSON)
+			Expect(ext).To(Not(BeNil()))
+
+			Expect(a.Reconcile(ctx, logger, ext)).To(Succeed())
+
+			mr := &v1alpha1.ManagedResource{}
+			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: ResourceNameSeed, Namespace: shootNamespace1}, mr)).To(Succeed())
+
+			// simulate a hibernated cluster by deleting the Gateway object
+			gw := &istionetworkingv1beta1.Gateway{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "kube-apiserver",
+					Namespace: shootNamespace1,
+				},
+			}
+
+			Expect(k8sClient.Delete(ctx, gw)).To(Succeed())
+
+			// act
+			Expect(a.Delete(ctx, logger, ext)).To(Succeed())
+
+			// assert
+			mr = &v1alpha1.ManagedResource{}
+			err = k8sClient.Get(ctx, types.NamespacedName{Name: ResourceNameSeed, Namespace: shootNamespace1}, mr)
+			Expect(err).ToNot(BeNil())
+			Expect(apierrors.IsNotFound(err)).To(BeTrue())
+		})
+	})
 })
 
 var _ = Describe("actuator unit test", func() {