Merge pull request #609 from viccuad/fix-webhook-upgrade

fix: Initialize nil maps when updating webhooks
kubewarden · Jan 16, 2024 · bcb6e76 · bcb6e76
2 parents e9e3446 + 38067af
commit bcb6e76
Show file tree

Hide file tree

Showing 5 changed files with 67 additions and 6 deletions.
diff --git a/controllers/admissionpolicy_controller.go b/controllers/admissionpolicy_controller.go
@@ -164,7 +164,7 @@ func (r *AdmissionPolicyReconciler) findAdmissionPolicyForWebhookConfiguration(_
 
 	policyScope, found := webhookConfiguration.GetLabels()[constants.WebhookConfigurationPolicyScopeLabelKey]
 	if !found {
-		r.Log.Error(nil, "Found a webhook configuration without a scope label", "name", webhookConfiguration.GetName())
+		r.Log.Info("Found a webhook configuration without a scope label, reconciling it", "name", webhookConfiguration.GetName())
 		return []reconcile.Request{}
 	}
 
@@ -175,13 +175,13 @@ func (r *AdmissionPolicyReconciler) findAdmissionPolicyForWebhookConfiguration(_
 
 	policyNamespace, found := webhookConfiguration.GetAnnotations()[constants.WebhookConfigurationPolicyNamespaceAnnotationKey]
 	if !found {
-		r.Log.Error(nil, "Found a webhook configuration without a namespace annotation", "name", webhookConfiguration.GetName())
+		r.Log.Info("Found a webhook configuration without a namespace annotation, reconciling it", "name", webhookConfiguration.GetName())
 		return []reconcile.Request{}
 	}
 
 	policyName, found := webhookConfiguration.GetAnnotations()[constants.WebhookConfigurationPolicyNameAnnotationKey]
 	if !found {
-		r.Log.Error(nil, "Found webhook configuration without a policy name annotation", "name", webhookConfiguration.GetName())
+		r.Log.Info("Found a webhook configuration without a policy name annotation, reconciling it", "name", webhookConfiguration.GetName())
 		return []reconcile.Request{}
 	}
 

diff --git a/controllers/admissionpolicy_controller_test.go b/controllers/admissionpolicy_controller_test.go
@@ -120,6 +120,30 @@ var _ = Describe("AdmissionPolicy controller", func() {
 					),
 				)
 			})
+
+			It("should reconcile unitialized label and annotation maps (behavior of Kubewarden <= 1.9.0))", func() {
+				By("changing the ValidatingWebhookConfiguration")
+				validatingWebhookConfiguration, err := getTestValidatingWebhookConfiguration(fmt.Sprintf("namespaced-%s-%s", policyNamespace, policyName))
+				Expect(err).ToNot(HaveOccurred())
+				originalValidatingWebhookConfiguration := validatingWebhookConfiguration.DeepCopy()
+				// simulate unitialized labels and annotation maps (behaviour of Kubewarden <= 1.9.0), or user change
+				validatingWebhookConfiguration.Labels = nil
+				validatingWebhookConfiguration.Annotations = nil
+				Expect(
+					k8sClient.Update(ctx, validatingWebhookConfiguration),
+				).To(Succeed())
+
+				By("reconciling the ValidatingWebhookConfiguration to its original state")
+				Eventually(func(g Gomega) (*admissionregistrationv1.ValidatingWebhookConfiguration, error) {
+					return getTestValidatingWebhookConfiguration(fmt.Sprintf("namespaced-%s-%s", policyNamespace, policyName))
+				}, timeout, pollInterval).Should(
+					And(
+						HaveField("Labels", Equal(originalValidatingWebhookConfiguration.Labels)),
+						HaveField("Annotations", Equal(originalValidatingWebhookConfiguration.Annotations)),
+						HaveField("Webhooks", Equal(originalValidatingWebhookConfiguration.Webhooks)),
+					),
+				)
+			})
 		})
 	})
 
@@ -199,6 +223,30 @@ var _ = Describe("AdmissionPolicy controller", func() {
 					),
 				)
 			})
+
+			It("should reconcile unitialized label and annotation maps (behavior of Kubewarden <= 1.9.0))", func() {
+				By("changing the MutatingWebhookConfiguration")
+				mutatingWebhookConfiguration, err := getTestMutatingWebhookConfiguration(fmt.Sprintf("namespaced-%s-%s", policyNamespace, policyName))
+				Expect(err).ToNot(HaveOccurred())
+				originalMutatingWebhookConfiguration := mutatingWebhookConfiguration.DeepCopy()
+				// simulate unitialized labels and annotation maps (behaviour of Kubewarden <= 1.9.0), or user change
+				mutatingWebhookConfiguration.Labels = nil
+				mutatingWebhookConfiguration.Annotations = nil
+				Expect(
+					k8sClient.Update(ctx, mutatingWebhookConfiguration),
+				).To(Succeed())
+
+				By("reconciling the MutatingWebhookConfiguration to its original state")
+				Eventually(func(g Gomega) (*admissionregistrationv1.MutatingWebhookConfiguration, error) {
+					return getTestMutatingWebhookConfiguration(fmt.Sprintf("namespaced-%s-%s", policyNamespace, policyName))
+				}, timeout, pollInterval).Should(
+					And(
+						HaveField("Labels", Equal(originalMutatingWebhookConfiguration.Labels)),
+						HaveField("Annotations", Equal(originalMutatingWebhookConfiguration.Annotations)),
+						HaveField("Webhooks", Equal(originalMutatingWebhookConfiguration.Webhooks)),
+					),
+				)
+			})
 		})
 	})
 

diff --git a/controllers/clusteradmissionpolicy_controller.go b/controllers/clusteradmissionpolicy_controller.go
@@ -161,7 +161,7 @@ func (r *ClusterAdmissionPolicyReconciler) findClusterAdmissionPolicyForWebhookC
 
 	policyScope, found := webhookConfiguration.GetLabels()[constants.WebhookConfigurationPolicyScopeLabelKey]
 	if !found {
-		r.Log.Error(nil, "Found a webhook configuration without a scope label", "name", webhookConfiguration.GetName())
+		r.Log.Info("Found a webhook configuration without a scope label, reconciling it", "name", webhookConfiguration.GetName())
 		return []reconcile.Request{}
 	}
 
@@ -172,7 +172,7 @@ func (r *ClusterAdmissionPolicyReconciler) findClusterAdmissionPolicyForWebhookC
 
 	policyName, found := webhookConfiguration.GetAnnotations()[constants.WebhookConfigurationPolicyNameAnnotationKey]
 	if !found {
-		r.Log.Error(nil, "Found webhook configuration without a policy name annotation", "name", webhookConfiguration.GetName())
+		r.Log.Info("Found a webhook configuration without a policy name annotation, reconciling it", "name", webhookConfiguration.GetName())
 		return []reconcile.Request{}
 	}
 

diff --git a/internal/pkg/admission/mutating-webhook.go b/internal/pkg/admission/mutating-webhook.go
@@ -51,10 +51,16 @@ func (r *Reconciler) updateMutatingWebhook(ctx context.Context,
 
 	patch := originalWebhook.DeepCopy()
 
+	if patch.ObjectMeta.Labels == nil {
+		patch.ObjectMeta.Labels = make(map[string]string)
+	}
 	for key, value := range newWebhook.ObjectMeta.Labels {
 		patch.ObjectMeta.Labels[key] = value
 	}
 
+	if patch.ObjectMeta.Annotations == nil {
+		patch.ObjectMeta.Annotations = make(map[string]string)
+	}
 	for key, value := range newWebhook.ObjectMeta.Annotations {
 		patch.ObjectMeta.Annotations[key] = value
 	}

diff --git a/internal/pkg/admission/validating-webhook.go b/internal/pkg/admission/validating-webhook.go
@@ -46,14 +46,21 @@ func (r *Reconciler) updateValidatingWebhook(ctx context.Context,
 		Name: policy.GetUniqueName(),
 	}, &originalWebhook)
 	if err != nil && apierrors.IsNotFound(err) {
-		return fmt.Errorf("cannot retrieve mutating webhook: %w", err)
+		return fmt.Errorf("cannot retrieve validating webhook: %w", err)
 	}
 
 	patch := originalWebhook.DeepCopy()
 
+	if patch.ObjectMeta.Labels == nil {
+		patch.ObjectMeta.Labels = make(map[string]string)
+	}
 	for key, value := range newWebhook.ObjectMeta.Labels {
 		patch.ObjectMeta.Labels[key] = value
 	}
+
+	if patch.ObjectMeta.Annotations == nil {
+		patch.ObjectMeta.Annotations = make(map[string]string)
+	}
 	for key, value := range newWebhook.ObjectMeta.Annotations {
 		patch.ObjectMeta.Annotations[key] = value
 	}