[backport] Add more robust CRD conversion patching (#5303)

Watch CRDs over a span of 15s to make sure that the CRD conversion patch
is not overwritten by another reconciler

Signed-off-by: Keith Mattix II <[email protected]>
Co-authored-by: Keith Mattix II <[email protected]>

Author: jaellio

cmd/osm-bootstrap/osm-bootstrap.go

@@ -12,7 +12,6 @@ import (
 	"net/http"
 	"os"
 	"path/filepath"
-	"strconv"
 
 	"github.com/spf13/pflag"
 	admissionv1 "k8s.io/api/admissionregistration/v1"
@@ -248,7 +247,7 @@ func applyOrUpdateCRDs(crdClient *apiclient.ApiextensionsV1Client) {
 			log.Fatal().Err(err).Msgf("Error decoding CRD file %s", file)
 		}
 
-		crd.Labels[constants.ReconcileLabel] = strconv.FormatBool(enableReconciler)
+		crd.Labels[constants.ReconcileLabel] = osmVersion
 
 		crdExisting, err := crdClient.CustomResourceDefinitions().Get(context.Background(), crd.Name, metav1.GetOptions{})
 		if err != nil && !apierrors.IsNotFound(err) {
@@ -267,7 +266,7 @@ func applyOrUpdateCRDs(crdClient *apiclient.ApiextensionsV1Client) {
 		} else {
 			log.Info().Msgf("Patching conversion webhook configuration for crd: %s, setting to \"None\"", crd.Name)
 
-			crdExisting.Labels[constants.ReconcileLabel] = strconv.FormatBool(enableReconciler)
+			crdExisting.Labels[constants.ReconcileLabel] = osmVersion
 			crdExisting.Spec = crd.Spec
 			crdExisting.Spec.Conversion = &apiv1.CustomResourceConversion{
 				Strategy: apiv1.NoneConverter,

dockerfiles/Dockerfile.init

@@ -1,2 +1,2 @@
 FROM alpine:3.17.3
-RUN apk add --no-cache iptables
+RUN apk add --no-cache iptables

pkg/reconciler/client.go

@@ -60,7 +60,8 @@ func NewReconcilerClient(kubeClient kubernetes.Interface, apiServerClient client
 
 // Initializes CustomResourceDefinition monitoring
 func (c *client) initCustomResourceDefinitionMonitor() {
-	osmCrdsLabel := map[string]string{constants.OSMAppNameLabelKey: constants.OSMAppNameLabelValue, constants.ReconcileLabel: strconv.FormatBool(true)}
+	// Use the OSM version as the selector for reconciliation
+	osmCrdsLabel := map[string]string{constants.OSMAppNameLabelKey: constants.OSMAppNameLabelValue, constants.ReconcileLabel: c.osmVersion}
 
 	labelSelector := fields.SelectorFromSet(osmCrdsLabel).String()
 	options := internalinterfaces.TweakListOptionsFunc(func(opt *metav1.ListOptions) {

tests/e2e/e2e_reconciler_test.go

@@ -2,13 +2,24 @@ package e2e
 
 import (
 	"context"
+	"os"
+	"path/filepath"
+	"strconv"
 	"time"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
-
+	apiv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/serializer"
+	"k8s.io/kubectl/pkg/util"
+	"k8s.io/utils/pointer"
+
+	"github.com/openservicemesh/osm/pkg/constants"
+	"github.com/openservicemesh/osm/pkg/reconciler"
 	. "github.com/openservicemesh/osm/tests/framework"
 )
 
@@ -106,7 +117,7 @@ var _ = OSMDescribe("Test OSM Reconciler",
 				Eventually(func() (string, error) {
 					updatedVwhc, err := Td.Client.AdmissionregistrationV1().ValidatingWebhookConfigurations().Get(context.Background(), "osm-validator-mesh-osm", metav1.GetOptions{})
 					return updatedVwhc.Webhooks[0].ClientConfig.Service.Name, err
-				}, 3*time.Second).Should(Equal(originalWebhookServiceName))
+				}, 30*time.Second).Should(Equal(originalWebhookServiceName))
 
 				// delete the validating webhook
 				err = Td.Client.AdmissionregistrationV1().ValidatingWebhookConfigurations().Delete(context.Background(), "osm-validator-mesh-osm", metav1.DeleteOptions{})
@@ -116,7 +127,107 @@ var _ = OSMDescribe("Test OSM Reconciler",
 				Eventually(func() error {
 					_, err = Td.Client.AdmissionregistrationV1().ValidatingWebhookConfigurations().Get(context.Background(), "osm-validator-mesh-osm", metav1.GetOptions{})
 					return err
-				}, 3*time.Second).Should(BeNil())
+				}, 30*time.Second).Should(BeNil())
+			})
+		})
+
+		Context("Pre-existing reconciler with CRD conversion webhook change", func() {
+			It("Retries until CRD change is detected 3 times, waiting for 5 seconds each time", func() {
+				// Install CRDs with a conversion webhook
+				crdFiles, err := filepath.Glob("../../cmd/osm-bootstrap/crds/*.yaml")
+
+				Expect(err).NotTo(HaveOccurred())
+
+				scheme := runtime.NewScheme()
+				codecs := serializer.NewCodecFactory(scheme)
+				decode := codecs.UniversalDeserializer().Decode
+
+				for _, file := range crdFiles {
+					yaml, err := os.ReadFile(filepath.Clean(file))
+					if err != nil {
+						Expect(err).NotTo(HaveOccurred())
+					}
+
+					crd := &apiv1.CustomResourceDefinition{}
+					_, _, err = decode(yaml, nil, crd)
+					if err != nil {
+						Expect(err).NotTo(HaveOccurred())
+					}
+
+					// imitate older version of OSM who set the reconcile label to a (stringified) bool
+					crd.Labels[constants.ReconcileLabel] = strconv.FormatBool(true)
+
+					crd.Spec.Conversion = &apiv1.CustomResourceConversion{
+						Strategy: apiv1.WebhookConverter,
+						Webhook: &apiv1.WebhookConversion{
+							ClientConfig: &apiv1.WebhookClientConfig{
+								Service: &apiv1.ServiceReference{
+									Namespace: "osm-system",
+									Name:      "osm-bootstrap",
+									Path:      pointer.StringPtr("/convert"),
+									Port:      pointer.Int32Ptr(9443),
+								},
+							},
+							ConversionReviewVersions: []string{"v1alpha1", "v1alpha2", "v1alpha3", "v1beta1", "v1"},
+						},
+					}
+
+					err = util.CreateApplyAnnotation(crd, unstructured.UnstructuredJSONScheme)
+					Expect(err).NotTo(HaveOccurred())
+
+					_, err = Td.APIServerClient.ApiextensionsV1().CustomResourceDefinitions().Create(context.Background(), crd, metav1.CreateOptions{})
+					if errors.IsAlreadyExists(err) {
+						existingCRD, err := Td.APIServerClient.ApiextensionsV1().CustomResourceDefinitions().Get(context.Background(), "meshconfigs.config.openservicemesh.io", metav1.GetOptions{})
+						Expect(err).NotTo(HaveOccurred())
+						existingCRD.Spec.Conversion = crd.Spec.Conversion
+						existingCRD.Labels[constants.ReconcileLabel] = strconv.FormatBool(true)
+						_, err = Td.APIServerClient.ApiextensionsV1().CustomResourceDefinitions().Update(context.Background(), existingCRD, metav1.UpdateOptions{})
+						Expect(err).NotTo(HaveOccurred())
+					} else {
+						Expect(err).NotTo(HaveOccurred())
+					}
+				}
+				// Add existing reconciler to watch the cluster
+				_, cancel := context.WithCancel(context.Background())
+				defer cancel()
+				stop := make(chan struct{})
+				Expect(err).NotTo(HaveOccurred())
+
+				// The current reconciler code matches the version to the label value, which is a stringified bool
+				err = reconciler.NewReconcilerClient(Td.Client, Td.APIServerClient, "osm", "true", stop, reconciler.CrdInformerKey)
+				Expect(err).NotTo(HaveOccurred())
+
+				// Confirm that the CRD conversion webhook is set to WebhookConverter
+				crd, err := Td.APIServerClient.ApiextensionsV1().CustomResourceDefinitions().Get(context.Background(), "meshconfigs.config.openservicemesh.io", metav1.GetOptions{})
+				Expect(err).NotTo(HaveOccurred())
+				Expect(crd.Spec.Conversion.Strategy).To(Equal(apiv1.WebhookConverter))
+
+				// Install OSM with reconciler enabled
+				installOpts := Td.GetOSMInstallOpts()
+				installOpts.EnableReconciler = true
+				Expect(Td.InstallOSM(installOpts)).To(Succeed())
+
+				Eventually(func() int {
+					// Make sure the CRD conversion webhook is set to NoneConverter 3 times in a row
+					var attempts int
+					for i := 0; i < 3; attempts++ {
+						if attempts >= 30 { // Gomega doesn't stop the function at the timeout, so prevent an infinite loop
+							break
+						}
+						crd, err := Td.APIServerClient.ApiextensionsV1().CustomResourceDefinitions().Get(context.Background(), "meshconfigs.config.openservicemesh.io", metav1.GetOptions{})
+						Expect(err).NotTo(HaveOccurred())
+						if crd.Spec.Conversion.Strategy == apiv1.NoneConverter {
+							i++
+						} else {
+							i = 0
+						}
+						time.Sleep(500 * time.Millisecond)
+					}
+					return attempts
+				}, 15*time.Second).Should(BeNumerically("<", 30))
+
+				// Stop the first reconciler (simulate the OSM upgrade completion)
+				close(stop)
 			})
 		})
 	})