From dcaa3ab54e4c8d40161c00c590394adac8c0a66a Mon Sep 17 00:00:00 2001 From: Gianluca Mardente Date: Sat, 17 Oct 2026 16:27:21 +0200 Subject: [PATCH 1/2] feat: installOptions.existingCRDs controls CRDs already present on helm install (#2041) On a fresh install the chart's crds/ were applied with server-side apply and force conflicts, silently overwriting (and possibly downgrading) CRDs that already existed in the cluster. This contradicts the SkipCRDs documentation ("installed if not already present") and Helm's own behavior for crds/. Before helm install, create the CRDs of the chart (subcharts included) that are missing and tell Helm to skip its own CRD step. An existing CRD is never modified. Whether it differs from the chart's is decided with a server-side apply dry run. The new installOptions.existingCRDs field selects what to do with a differing one: - Skip (default): leave it as it is, log it and carry on - Fail: fail the installation naming the CRDs, creating nothing - Overwrite: previous behavior, Helm applies the chart's CRD skipCRDs, DryRun and template-only runs are unchanged. Co-Authored-By: Claude Sonnet 5.5 --- api/v1beta1/spec.go | 29 ++ ...fig.projectsveltos.io_clusterprofiles.yaml | 15 + ...g.projectsveltos.io_clusterpromotions.yaml | 15 + ...ig.projectsveltos.io_clustersummaries.yaml | 15 + .../config.projectsveltos.io_profiles.yaml | 15 + controllers/export_test.go | 10 +- controllers/handlers_helm.go | 237 +++++++++++++- controllers/handlers_helm_test.go | 306 ++++++++++++++++++ lib/crd/clusterprofiles.go | 15 + lib/crd/clusterpromotions.go | 15 + lib/crd/clustersummaries.go | 15 + lib/crd/profiles.go | 15 + manifest/manifest.yaml | 60 ++++ test/fv/helm_crds_test.go | 202 ++++++++++++ test/fv/test_constants_test.go | 18 ++ 15 files changed, 969 insertions(+), 13 deletions(-) create mode 100644 test/fv/helm_crds_test.go diff --git a/api/v1beta1/spec.go b/api/v1beta1/spec.go index 7bcd78ff..e9098a06 100644 --- a/api/v1beta1/spec.go +++ b/api/v1beta1/spec.go @@ -180,6 +180,24 @@ const ( PostRenderStrategyNoHooks = PostRenderStrategy("nohooks") ) +// ExistingCRDsPolicy controls what happens when a Helm chart is installed and one of the +// CRDs in the chart's crds/ directory already exists in the cluster with a different definition. +// +kubebuilder:validation:Enum:=Skip;Fail;Overwrite +type ExistingCRDsPolicy string + +const ( + // ExistingCRDsPolicySkip leaves the existing CRD as it is and carries on with the + // remaining CRDs of the chart. CRDs not yet in the cluster are created. + ExistingCRDsPolicySkip = ExistingCRDsPolicy("Skip") + + // ExistingCRDsPolicyFail fails the installation, reporting the CRDs that differ. + // No CRD of the chart is created or changed. + ExistingCRDsPolicyFail = ExistingCRDsPolicy("Fail") + + // ExistingCRDsPolicyOverwrite applies the CRD of the chart over the existing one. + ExistingCRDsPolicyOverwrite = ExistingCRDsPolicy("Overwrite") +) + type HelmOptions struct { // SkipCRDs controls whether CRDs should be installed during install/upgrade operation. // By default, CRDs are installed if not already present. @@ -307,6 +325,17 @@ type HelmInstallOptions struct { // +optional TakeOwnership bool `json:"takeOwnership,omitempty"` + // ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts + // included) already exists in the cluster with a different definition. CRDs not yet in the + // cluster are always created. Ignored when SkipCRDs is true. + // Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. + // Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + // Overwrite applies the CRD of the chart over the existing one. + // Default to Skip + // +kubebuilder:default:=Skip + // +optional + ExistingCRDs ExistingCRDsPolicy `json:"existingCRDs,omitempty"` + // RecoverAfterConsecutiveFailures is the number of consecutive install failures for this // chart after which Sveltos uninstalls any existing release under this name before // retrying, to clear potentially stale Helm release history that would otherwise keep diff --git a/config/crd/bases/config.projectsveltos.io_clusterprofiles.yaml b/config/crd/bases/config.projectsveltos.io_clusterprofiles.yaml index 38228a56..664edf59 100644 --- a/config/crd/bases/config.projectsveltos.io_clusterprofiles.yaml +++ b/config/crd/bases/config.projectsveltos.io_clusterprofiles.yaml @@ -367,6 +367,21 @@ spec: disable hooks on install Default to false type: boolean + existingCRDs: + default: Skip + description: |- + ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts + included) already exists in the cluster with a different definition. CRDs not yet in the + cluster are always created. Ignored when SkipCRDs is true. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. + Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Overwrite applies the CRD of the chart over the existing one. + Default to Skip + enum: + - Skip + - Fail + - Overwrite + type: string recoverAfterConsecutiveFailures: default: 5 description: |- diff --git a/config/crd/bases/config.projectsveltos.io_clusterpromotions.yaml b/config/crd/bases/config.projectsveltos.io_clusterpromotions.yaml index 7c2d0d5f..198a426e 100644 --- a/config/crd/bases/config.projectsveltos.io_clusterpromotions.yaml +++ b/config/crd/bases/config.projectsveltos.io_clusterpromotions.yaml @@ -267,6 +267,21 @@ spec: disable hooks on install Default to false type: boolean + existingCRDs: + default: Skip + description: |- + ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts + included) already exists in the cluster with a different definition. CRDs not yet in the + cluster are always created. Ignored when SkipCRDs is true. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. + Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Overwrite applies the CRD of the chart over the existing one. + Default to Skip + enum: + - Skip + - Fail + - Overwrite + type: string recoverAfterConsecutiveFailures: default: 5 description: |- diff --git a/config/crd/bases/config.projectsveltos.io_clustersummaries.yaml b/config/crd/bases/config.projectsveltos.io_clustersummaries.yaml index a17b9724..e6e73991 100644 --- a/config/crd/bases/config.projectsveltos.io_clustersummaries.yaml +++ b/config/crd/bases/config.projectsveltos.io_clustersummaries.yaml @@ -404,6 +404,21 @@ spec: disable hooks on install Default to false type: boolean + existingCRDs: + default: Skip + description: |- + ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts + included) already exists in the cluster with a different definition. CRDs not yet in the + cluster are always created. Ignored when SkipCRDs is true. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. + Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Overwrite applies the CRD of the chart over the existing one. + Default to Skip + enum: + - Skip + - Fail + - Overwrite + type: string recoverAfterConsecutiveFailures: default: 5 description: |- diff --git a/config/crd/bases/config.projectsveltos.io_profiles.yaml b/config/crd/bases/config.projectsveltos.io_profiles.yaml index f357ecc8..364119aa 100644 --- a/config/crd/bases/config.projectsveltos.io_profiles.yaml +++ b/config/crd/bases/config.projectsveltos.io_profiles.yaml @@ -367,6 +367,21 @@ spec: disable hooks on install Default to false type: boolean + existingCRDs: + default: Skip + description: |- + ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts + included) already exists in the cluster with a different definition. CRDs not yet in the + cluster are always created. Ignored when SkipCRDs is true. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. + Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Overwrite applies the CRD of the chart over the existing one. + Default to Skip + enum: + - Skip + - Fail + - Overwrite + type: string recoverAfterConsecutiveFailures: default: 5 description: |- diff --git a/controllers/export_test.go b/controllers/export_test.go index 800f26a3..46ab9283 100644 --- a/controllers/export_test.go +++ b/controllers/export_test.go @@ -320,9 +320,13 @@ var ( ) var ( - IsCRDEstablished = isCRDEstablished - GetCRDObjects = getCRDObjects - GetHelmUpgradeClient = getHelmUpgradeClient + IsCRDEstablished = isCRDEstablished + GetCRDObjects = getCRDObjects + GetExistingCRDsPolicy = getExistingCRDsPolicy + InstallMissingCRDs = installMissingCRDs + InstallChartCRDs = installChartCRDs + WithRestConfig = withRestConfig + GetHelmUpgradeClient = getHelmUpgradeClient ) type ( diff --git a/controllers/handlers_helm.go b/controllers/handlers_helm.go index f163e404..83e7c857 100644 --- a/controllers/handlers_helm.go +++ b/controllers/handlers_helm.go @@ -66,6 +66,7 @@ import ( "helm.sh/helm/v4/pkg/storage/driver" corev1 "k8s.io/api/core/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + apiequality "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -2059,6 +2060,11 @@ func installRelease(ctx context.Context, clusterSummary *configv1beta1.ClusterSu ctxWithTimeout, cancel := context.WithTimeout(ctx, getTimeoutValue(requestedChart.Options).Duration) defer cancel() + if err := prepareCRDsForInstall(ctxWithTimeout, clusterSummary, requestedChart, installClient, + chartRequested, kubeconfig, templateOnly, logger); err != nil { + return nil, err + } + r, err := installClient.RunWithContext(ctxWithTimeout, chartRequested, values) if err != nil { logger.V(logs.LogInfo).Info(fmt.Sprintf("failed to install: %v", err)) @@ -2599,38 +2605,241 @@ func upgradeCRDs(ctx context.Context, requestedChart *configv1beta1.HelmChart, k logger.V(logs.LogDebug).Info("upgrade crds") + dr, err := getCRDResourceInterface(ctx, kubeconfig) + if err != nil { + return err + } + + var appliedCRDs []string + + for _, obj := range crds { + names, err := upgradeCRDsInFile(ctx, dr, obj.File, logger) + if err != nil { + return err + } + appliedCRDs = append(appliedCRDs, names...) + } + + if len(appliedCRDs) > 0 { + if err := waitForCRDsEstablished(ctx, dr, appliedCRDs, logger); err != nil { + return err + } + } + logger.V(logs.LogDebug).Info(fmt.Sprintf("CRDs upgraded: %d", len(appliedCRDs))) + return nil +} + +// getCRDResourceInterface returns a dynamic client for the CustomResourceDefinitions of the managed cluster. +func getCRDResourceInterface(ctx context.Context, kubeconfig string) (dynamic.ResourceInterface, error) { destConfig := restConfigFromContext(ctx) if destConfig == nil { var err error destConfig, err = clientcmd.BuildConfigFromFlags("", kubeconfig) if err != nil { - return err + return nil, err } } - dr, err := k8s_utils.GetDynamicResourceInterface(destConfig, + return k8s_utils.GetDynamicResourceInterface(destConfig, apiextensionsv1.SchemeGroupVersion.WithKind(customResourceDefinitionKind), "") +} + +// prepareCRDsForInstall installs the CRDs of the chart ahead of the Helm release, honoring the +// ExistingCRDs policy, and tells Helm to skip them when they have been dealt with. +// No action in DryRun mode or when only templating. +func prepareCRDsForInstall(ctx context.Context, clusterSummary *configv1beta1.ClusterSummary, + requestedChart *configv1beta1.HelmChart, installClient *action.Install, chartRequested chartbase.Charter, + kubeconfig string, templateOnly bool, logger logr.Logger) error { + + if templateOnly || clusterSummary.Spec.ClusterProfileSpec.SyncMode == configv1beta1.SyncModeDryRun { + return nil + } + + crdsInstalled, err := installChartCRDs(ctx, requestedChart, kubeconfig, getCRDObjects(chartRequested), logger) if err != nil { + logger.V(logs.LogInfo).Info(fmt.Sprintf("failed to install crds: %v", err)) return err } - var appliedCRDs []string + // Helm must not apply again the CRDs already dealt with + if crdsInstalled { + installClient.SkipCRDs = true + } - for _, obj := range crds { - names, err := upgradeCRDsInFile(ctx, dr, obj.File, logger) + return nil +} + +// installChartCRDs installs the CRDs of a chart before the Helm release is installed, honoring the +// ExistingCRDs policy. It returns true when the CRDs have been dealt with and Helm must skip them. +// It returns false when Helm has to deal with the CRDs itself: SkipCRDs is set (Helm skips them +// anyway), the policy is Overwrite, or the chart has no CRDs. +func installChartCRDs(ctx context.Context, requestedChart *configv1beta1.HelmChart, kubeconfig string, + crds []chart.CRD, logger logr.Logger) (bool, error) { + + if getSkipCRDsHelmValue(requestedChart.Options) || len(crds) == 0 { + return false, nil + } + + policy := getExistingCRDsPolicy(requestedChart.Options) + if policy == configv1beta1.ExistingCRDsPolicyOverwrite { + return false, nil + } + + dr, err := getCRDResourceInterface(ctx, kubeconfig) + if err != nil { + return false, err + } + + if err := installMissingCRDs(ctx, dr, policy, crds, logger); err != nil { + return false, err + } + + return true, nil +} + +// installMissingCRDs creates the CRDs of the chart that do not exist in the cluster. +// A CRD that already exists is never modified. If it differs from the one in the chart, +// it is logged (Skip policy) or the whole operation fails before anything is created (Fail policy). +func installMissingCRDs(ctx context.Context, dr dynamic.ResourceInterface, + policy configv1beta1.ExistingCRDsPolicy, crds []chart.CRD, logger logr.Logger) error { + + chartCRDs, err := getChartCRDs(crds, logger) + if err != nil { + return err + } + + var missing []*unstructured.Unstructured + var differing []string + + for _, chartCRD := range chartCRDs { + existing, err := dr.Get(ctx, chartCRD.GetName(), metav1.GetOptions{}) + if apierrors.IsNotFound(err) { + missing = append(missing, chartCRD) + continue + } if err != nil { return err } - appliedCRDs = append(appliedCRDs, names...) + + changed, err := wouldChangeCRD(ctx, dr, existing, chartCRD) + if err != nil { + return err + } + if changed { + differing = append(differing, chartCRD.GetName()) + } } - if len(appliedCRDs) > 0 { - if err := waitForCRDsEstablished(ctx, dr, appliedCRDs, logger); err != nil { + if len(differing) > 0 { + if policy == configv1beta1.ExistingCRDsPolicyFail { + return fmt.Errorf("CRDs already exist with a different definition: %s. "+ + "Set installOptions.existingCRDs to Skip to keep them or to Overwrite to replace them", + strings.Join(differing, ", ")) + } + for _, name := range differing { + logger.V(logs.LogInfo).Info(fmt.Sprintf( + "CRD %s already exists with a different definition. Skipping it", name)) + } + } + + return createCRDs(ctx, dr, missing, logger) +} + +// getChartCRDs returns the CRDs defined in the crds/ files of a chart. +// Like Helm, only files with a manifest extension (.yaml, .yml, .json) are considered. +func getChartCRDs(crds []chart.CRD, logger logr.Logger) ([]*unstructured.Unstructured, error) { + var chartCRDs []*unstructured.Unstructured + + for i := range crds { + file := crds[i].File + if file == nil || !hasManifestExtension(file.Name) { + continue + } + + objects, err := deployer.GetUnstructured(file.Data, logger) + if err != nil { + return nil, fmt.Errorf("failed to parse %s: %w", file.Name, err) + } + + for _, object := range objects { + if object.GetKind() != customResourceDefinitionKind { + return nil, fmt.Errorf("%s contains a %s. Only CustomResourceDefinitions are expected in crds/", + file.Name, object.GetKind()) + } + chartCRDs = append(chartCRDs, object) + } + } + + return chartCRDs, nil +} + +func hasManifestExtension(fileName string) bool { + extension := filepath.Ext(fileName) + return strings.EqualFold(extension, ".yaml") || strings.EqualFold(extension, ".yml") || + strings.EqualFold(extension, ".json") +} + +// wouldChangeCRD returns true when applying the CRD of the chart would change the existing CRD. +// The apply is a server side dry run, so defaulting done by the API server is taken into account. +func wouldChangeCRD(ctx context.Context, dr dynamic.ResourceInterface, + existing, chartCRD *unstructured.Unstructured) (bool, error) { + + data, err := runtime.Encode(unstructured.UnstructuredJSONScheme, chartCRD) + if err != nil { + return false, err + } + + forceConflict := true + options := metav1.PatchOptions{ + FieldManager: applyPatchFieldManager, + Force: &forceConflict, + DryRun: []string{metav1.DryRunAll}, + } + + result, err := dr.Patch(ctx, chartCRD.GetName(), types.ApplyPatchType, data, options) + if err != nil { + // The API server refuses the change (for instance a change to an immutable field) + if apierrors.IsInvalid(err) { + return true, nil + } + return false, err + } + + return !haveSameDefinition(existing, result), nil +} + +// haveSameDefinition compares the parts of a CRD that an apply can change. +func haveSameDefinition(first, second *unstructured.Unstructured) bool { + return apiequality.Semantic.DeepEqual(first.Object["spec"], second.Object["spec"]) && + apiequality.Semantic.DeepEqual(first.GetLabels(), second.GetLabels()) && + apiequality.Semantic.DeepEqual(first.GetAnnotations(), second.GetAnnotations()) +} + +// createCRDs creates the CRDs and waits for them to be established. +func createCRDs(ctx context.Context, dr dynamic.ResourceInterface, crds []*unstructured.Unstructured, + logger logr.Logger) error { + + var createdCRDs []string + + for _, crd := range crds { + _, err := dr.Create(ctx, crd, metav1.CreateOptions{FieldManager: applyPatchFieldManager}) + if apierrors.IsAlreadyExists(err) { + // Created by somebody else after the check was done. Never modify it. + continue + } + if err != nil { return err } + + createdCRDs = append(createdCRDs, crd.GetName()) } - logger.V(logs.LogDebug).Info(fmt.Sprintf("CRDs upgraded: %d", len(appliedCRDs))) - return nil + + if len(createdCRDs) == 0 { + return nil + } + + logger.V(logs.LogDebug).Info(fmt.Sprintf("CRDs created: %d", len(createdCRDs))) + return waitForCRDsEstablished(ctx, dr, createdCRDs, logger) } func getRegistryClient(namespace string, registryOptions *registryClientOptions, enableClientCache bool, @@ -4184,6 +4393,14 @@ func getTakeOwnershipHelmValue(options *configv1beta1.HelmOptions, isUpgrade boo return options.InstallOptions.TakeOwnership } +func getExistingCRDsPolicy(options *configv1beta1.HelmOptions) configv1beta1.ExistingCRDsPolicy { + if options == nil || options.InstallOptions.ExistingCRDs == "" { + return configv1beta1.ExistingCRDsPolicySkip + } + + return options.InstallOptions.ExistingCRDs +} + func getCreateNamespaceHelmValue(options *configv1beta1.HelmOptions) bool { if options != nil && options.InstallOptions.CreateNamespace != nil { return *options.InstallOptions.CreateNamespace diff --git a/controllers/handlers_helm_test.go b/controllers/handlers_helm_test.go index 6add66e0..6a17995d 100644 --- a/controllers/handlers_helm_test.go +++ b/controllers/handlers_helm_test.go @@ -26,6 +26,7 @@ import ( "os" "reflect" "sort" + "strings" "github.com/go-logr/logr" . "github.com/onsi/ginkgo/v2" @@ -37,18 +38,21 @@ import ( "helm.sh/helm/v4/pkg/cli" releasecommon "helm.sh/helm/v4/pkg/release/common" corev1 "k8s.io/api/core/v1" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apierrors "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/labels" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/dynamic" "k8s.io/klog/v2/textlogger" "k8s.io/utils/ptr" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + "sigs.k8s.io/yaml" configv1beta1 "github.com/projectsveltos/addon-controller/api/v1beta1" "github.com/projectsveltos/addon-controller/controllers" @@ -57,6 +61,7 @@ import ( "github.com/projectsveltos/addon-controller/pkg/scope" libsveltosv1beta1 "github.com/projectsveltos/libsveltos/api/v1beta1" "github.com/projectsveltos/libsveltos/lib/clusterproxy" + "github.com/projectsveltos/libsveltos/lib/k8s_utils" "github.com/projectsveltos/libsveltos/lib/pullmode" ) @@ -3293,6 +3298,307 @@ var _ = Describe("getCRDObjects", func() { }) }) +var _ = Describe("getExistingCRDsPolicy", func() { + It("defaults to Skip when no options are set", func() { + Expect(controllers.GetExistingCRDsPolicy(nil)).To(Equal(configv1beta1.ExistingCRDsPolicySkip)) + }) + + It("defaults to Skip when the policy is not set", func() { + Expect(controllers.GetExistingCRDsPolicy(&configv1beta1.HelmOptions{})).To( + Equal(configv1beta1.ExistingCRDsPolicySkip)) + }) + + It("returns the configured policy", func() { + options := &configv1beta1.HelmOptions{ + InstallOptions: configv1beta1.HelmInstallOptions{ExistingCRDs: configv1beta1.ExistingCRDsPolicyFail}, + } + Expect(controllers.GetExistingCRDsPolicy(options)).To(Equal(configv1beta1.ExistingCRDsPolicyFail)) + }) +}) + +const topCRDFile = "crds/top.yaml" + +func crdResourceInterface() dynamic.ResourceInterface { + dr, err := k8s_utils.GetDynamicResourceInterface(testEnv.Config, + apiextensionsv1.SchemeGroupVersion.WithKind("CustomResourceDefinition"), "") + Expect(err).To(BeNil()) + return dr +} + +var _ = Describe("installMissingCRDs", func() { + const ( + topCRD = "tops" + subCRD = "subs" + ) + + var ( + dr dynamic.ResourceInterface + group string + logger = textlogger.NewLogger(textlogger.NewConfig()) + ) + + crdName := func(plural string) string { + return plural + "." + group + } + + // crdManifest returns a CRD whose spec has one string property for each of the given properties + crdManifest := func(plural string, properties ...string) string { + manifest := fmt.Sprintf(`apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: %s +spec: + group: %s + scope: Namespaced + names: + kind: %s + plural: %s + versions: + - name: v1 + served: true + storage: true + schema: + openAPIV3Schema: + type: object + properties: + spec: + type: object + properties: +`, crdName(plural), group, strings.ToUpper(plural[:1])+plural[1:], plural) + for _, property := range properties { + manifest += fmt.Sprintf(" %s: {type: string}\n", property) + } + return manifest + } + + chartCRD := func(fileName, manifest string) chart.CRD { + file := &common.File{Name: fileName, Data: []byte(manifest)} + return chart.CRD{Name: fileName, File: file} + } + + createExistingCRD := func(plural string, properties ...string) { + crd := &unstructured.Unstructured{} + Expect(yaml.Unmarshal([]byte(crdManifest(plural, properties...)), &crd.Object)).To(Succeed()) + _, err := dr.Create(ctx, crd, metav1.CreateOptions{}) + Expect(err).To(BeNil()) + } + + // crdProperties returns the names of the properties of the CRD spec found in the cluster + crdProperties := func(plural string) []string { + crd, err := dr.Get(ctx, crdName(plural), metav1.GetOptions{}) + Expect(err).To(BeNil()) + versions, found, err := unstructured.NestedSlice(crd.Object, "spec", "versions") + Expect(err).To(BeNil()) + Expect(found).To(BeTrue()) + properties, _, err := unstructured.NestedMap(versions[0].(map[string]interface{}), + "schema", "openAPIV3Schema", "properties", "spec", "properties") + Expect(err).To(BeNil()) + names := make([]string, 0, len(properties)) + for name := range properties { + names = append(names, name) + } + return names + } + + crdExists := func(plural string) bool { + _, err := dr.Get(ctx, crdName(plural), metav1.GetOptions{}) + if apierrors.IsNotFound(err) { + return false + } + Expect(err).To(BeNil()) + return true + } + + BeforeEach(func() { + dr = crdResourceInterface() + group = strings.ToLower(randomString()) + ".io" + }) + + AfterEach(func() { + for _, plural := range []string{topCRD, subCRD} { + err := dr.Delete(ctx, crdName(plural), metav1.DeleteOptions{}) + if err != nil && !apierrors.IsNotFound(err) { + Expect(err).To(BeNil()) + } + } + }) + + It("creates the CRDs missing from the cluster", func() { + crds := []chart.CRD{ + chartCRD(topCRDFile, crdManifest(topCRD, "a")), + chartCRD("crds/sub.yaml", crdManifest(subCRD, "a")), + } + + Expect(controllers.InstallMissingCRDs(ctx, dr, configv1beta1.ExistingCRDsPolicySkip, crds, logger)).To(Succeed()) + + Expect(crdProperties(topCRD)).To(ConsistOf("a")) + Expect(crdProperties(subCRD)).To(ConsistOf("a")) + }) + + It("leaves an existing identical CRD alone", func() { + createExistingCRD(topCRD, "a") + crds := []chart.CRD{chartCRD(topCRDFile, crdManifest(topCRD, "a"))} + + for _, policy := range []configv1beta1.ExistingCRDsPolicy{ + configv1beta1.ExistingCRDsPolicySkip, configv1beta1.ExistingCRDsPolicyFail} { + + Expect(controllers.InstallMissingCRDs(ctx, dr, policy, crds, logger)).To(Succeed()) + } + + Expect(crdProperties(topCRD)).To(ConsistOf("a")) + }) + + It("with Skip leaves an existing CRD newer than the chart's alone and creates the missing ones", func() { + createExistingCRD(topCRD, "a", "b") + crds := []chart.CRD{ + chartCRD(topCRDFile, crdManifest(topCRD, "a")), + chartCRD("crds/sub.yaml", crdManifest(subCRD, "a")), + } + + Expect(controllers.InstallMissingCRDs(ctx, dr, configv1beta1.ExistingCRDsPolicySkip, crds, logger)).To(Succeed()) + + Expect(crdProperties(topCRD)).To(ConsistOf("a", "b")) + Expect(crdProperties(subCRD)).To(ConsistOf("a")) + }) + + It("with Skip leaves an existing CRD older than the chart's alone", func() { + createExistingCRD(topCRD, "a") + crds := []chart.CRD{chartCRD(topCRDFile, crdManifest(topCRD, "a", "b"))} + + Expect(controllers.InstallMissingCRDs(ctx, dr, configv1beta1.ExistingCRDsPolicySkip, crds, logger)).To(Succeed()) + + Expect(crdProperties(topCRD)).To(ConsistOf("a")) + }) + + It("with Fail returns an error naming the CRDs that differ and creates nothing", func() { + createExistingCRD(topCRD, "a", "b") + crds := []chart.CRD{ + chartCRD(topCRDFile, crdManifest(topCRD, "a")), + chartCRD("crds/sub.yaml", crdManifest(subCRD, "a")), + } + + err := controllers.InstallMissingCRDs(ctx, dr, configv1beta1.ExistingCRDsPolicyFail, crds, logger) + Expect(err).ToNot(BeNil()) + Expect(err.Error()).To(ContainSubstring(crdName(topCRD))) + Expect(err.Error()).ToNot(ContainSubstring(crdName(subCRD))) + + Expect(crdProperties(topCRD)).To(ConsistOf("a", "b")) + Expect(crdExists(subCRD)).To(BeFalse()) + }) + + It("with Fail creates the missing CRDs when no existing CRD differs", func() { + createExistingCRD(topCRD, "a") + crds := []chart.CRD{ + chartCRD(topCRDFile, crdManifest(topCRD, "a")), + chartCRD("crds/sub.yaml", crdManifest(subCRD, "a")), + } + + Expect(controllers.InstallMissingCRDs(ctx, dr, configv1beta1.ExistingCRDsPolicyFail, crds, logger)).To(Succeed()) + + Expect(crdProperties(subCRD)).To(ConsistOf("a")) + }) + + It("reads every CRD of a file with several documents", func() { + crds := []chart.CRD{ + chartCRD("crds/all.yaml", crdManifest(topCRD, "a")+"---\n"+crdManifest(subCRD, "a")), + } + + Expect(controllers.InstallMissingCRDs(ctx, dr, configv1beta1.ExistingCRDsPolicySkip, crds, logger)).To(Succeed()) + + Expect(crdExists(topCRD)).To(BeTrue()) + Expect(crdExists(subCRD)).To(BeTrue()) + }) + + It("ignores files without a manifest extension", func() { + crds := []chart.CRD{ + chartCRD("crds/README.md", "# not a manifest: {"), + chartCRD(topCRDFile, crdManifest(topCRD, "a")), + } + + Expect(controllers.InstallMissingCRDs(ctx, dr, configv1beta1.ExistingCRDsPolicySkip, crds, logger)).To(Succeed()) + + Expect(crdExists(topCRD)).To(BeTrue()) + }) + + It("returns an error when crds/ contains something other than a CRD", func() { + crds := []chart.CRD{chartCRD("crds/cm.yaml", "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test\n")} + + err := controllers.InstallMissingCRDs(ctx, dr, configv1beta1.ExistingCRDsPolicySkip, crds, logger) + Expect(err).ToNot(BeNil()) + Expect(err.Error()).To(ContainSubstring("ConfigMap")) + }) +}) + +var _ = Describe("installChartCRDs", func() { + logger := textlogger.NewLogger(textlogger.NewConfig()) + + newHelmChart := func(options *configv1beta1.HelmOptions) *configv1beta1.HelmChart { + return &configv1beta1.HelmChart{ReleaseName: randomString(), ReleaseNamespace: randomString(), Options: options} + } + + crds := []chart.CRD{{ + Name: topCRDFile, + File: &common.File{Name: topCRDFile, Data: []byte("not used: the cluster is never reached")}, + }} + + It("leaves the CRDs to Helm when skipCRDs is set", func() { + helmChart := newHelmChart(&configv1beta1.HelmOptions{SkipCRDs: true}) + + handled, err := controllers.InstallChartCRDs(ctx, helmChart, "no-such-kubeconfig", crds, logger) + Expect(err).To(BeNil()) + Expect(handled).To(BeFalse()) + }) + + It("leaves the CRDs to Helm when the policy is Overwrite", func() { + helmChart := newHelmChart(&configv1beta1.HelmOptions{ + InstallOptions: configv1beta1.HelmInstallOptions{ExistingCRDs: configv1beta1.ExistingCRDsPolicyOverwrite}, + }) + + handled, err := controllers.InstallChartCRDs(ctx, helmChart, "no-such-kubeconfig", crds, logger) + Expect(err).To(BeNil()) + Expect(handled).To(BeFalse()) + }) + + It("does nothing when the chart has no CRDs", func() { + handled, err := controllers.InstallChartCRDs(ctx, newHelmChart(nil), "no-such-kubeconfig", nil, logger) + Expect(err).To(BeNil()) + Expect(handled).To(BeFalse()) + }) + + It("installs the CRDs itself and tells Helm to skip them by default", func() { + group := strings.ToLower(randomString()) + ".io" + manifest := fmt.Sprintf(`apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: tops.%s +spec: + group: %s + scope: Namespaced + names: + kind: Top + plural: tops + versions: + - name: v1 + served: true + storage: true + schema: + openAPIV3Schema: + type: object +`, group, group) + topCRD := []chart.CRD{{Name: topCRDFile, File: &common.File{Name: topCRDFile, Data: []byte(manifest)}}} + + restConfigCtx := controllers.WithRestConfig(ctx, testEnv.Config) + handled, err := controllers.InstallChartCRDs(restConfigCtx, newHelmChart(nil), "", topCRD, logger) + Expect(err).To(BeNil()) + Expect(handled).To(BeTrue()) + + dr := crdResourceInterface() + _, err = dr.Get(ctx, "tops."+group, metav1.GetOptions{}) + Expect(err).To(BeNil()) + Expect(dr.Delete(ctx, "tops."+group, metav1.DeleteOptions{})).To(Succeed()) + }) +}) + var _ = Describe("prepareBundleSettersWithHelmInfo", func() { It("sets SkipNamespaceCreation when createNamespace is false", func() { currentChart := &configv1beta1.HelmChart{ diff --git a/lib/crd/clusterprofiles.go b/lib/crd/clusterprofiles.go index 7e32a1bc..8f305089 100644 --- a/lib/crd/clusterprofiles.go +++ b/lib/crd/clusterprofiles.go @@ -386,6 +386,21 @@ spec: disable hooks on install Default to false type: boolean + existingCRDs: + default: Skip + description: |- + ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts + included) already exists in the cluster with a different definition. CRDs not yet in the + cluster are always created. Ignored when SkipCRDs is true. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. + Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Overwrite applies the CRD of the chart over the existing one. + Default to Skip + enum: + - Skip + - Fail + - Overwrite + type: string recoverAfterConsecutiveFailures: default: 5 description: |- diff --git a/lib/crd/clusterpromotions.go b/lib/crd/clusterpromotions.go index 09c24c74..eb79b35d 100644 --- a/lib/crd/clusterpromotions.go +++ b/lib/crd/clusterpromotions.go @@ -286,6 +286,21 @@ spec: disable hooks on install Default to false type: boolean + existingCRDs: + default: Skip + description: |- + ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts + included) already exists in the cluster with a different definition. CRDs not yet in the + cluster are always created. Ignored when SkipCRDs is true. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. + Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Overwrite applies the CRD of the chart over the existing one. + Default to Skip + enum: + - Skip + - Fail + - Overwrite + type: string recoverAfterConsecutiveFailures: default: 5 description: |- diff --git a/lib/crd/clustersummaries.go b/lib/crd/clustersummaries.go index 145251b9..7341a0dd 100644 --- a/lib/crd/clustersummaries.go +++ b/lib/crd/clustersummaries.go @@ -423,6 +423,21 @@ spec: disable hooks on install Default to false type: boolean + existingCRDs: + default: Skip + description: |- + ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts + included) already exists in the cluster with a different definition. CRDs not yet in the + cluster are always created. Ignored when SkipCRDs is true. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. + Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Overwrite applies the CRD of the chart over the existing one. + Default to Skip + enum: + - Skip + - Fail + - Overwrite + type: string recoverAfterConsecutiveFailures: default: 5 description: |- diff --git a/lib/crd/profiles.go b/lib/crd/profiles.go index e8012088..9af24cd6 100644 --- a/lib/crd/profiles.go +++ b/lib/crd/profiles.go @@ -386,6 +386,21 @@ spec: disable hooks on install Default to false type: boolean + existingCRDs: + default: Skip + description: |- + ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts + included) already exists in the cluster with a different definition. CRDs not yet in the + cluster are always created. Ignored when SkipCRDs is true. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. + Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Overwrite applies the CRD of the chart over the existing one. + Default to Skip + enum: + - Skip + - Fail + - Overwrite + type: string recoverAfterConsecutiveFailures: default: 5 description: |- diff --git a/manifest/manifest.yaml b/manifest/manifest.yaml index 41151bc5..5677f6b2 100644 --- a/manifest/manifest.yaml +++ b/manifest/manifest.yaml @@ -676,6 +676,21 @@ spec: disable hooks on install Default to false type: boolean + existingCRDs: + default: Skip + description: |- + ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts + included) already exists in the cluster with a different definition. CRDs not yet in the + cluster are always created. Ignored when SkipCRDs is true. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. + Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Overwrite applies the CRD of the chart over the existing one. + Default to Skip + enum: + - Skip + - Fail + - Overwrite + type: string recoverAfterConsecutiveFailures: default: 5 description: |- @@ -3221,6 +3236,21 @@ spec: disable hooks on install Default to false type: boolean + existingCRDs: + default: Skip + description: |- + ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts + included) already exists in the cluster with a different definition. CRDs not yet in the + cluster are always created. Ignored when SkipCRDs is true. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. + Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Overwrite applies the CRD of the chart over the existing one. + Default to Skip + enum: + - Skip + - Fail + - Overwrite + type: string recoverAfterConsecutiveFailures: default: 5 description: |- @@ -6941,6 +6971,21 @@ spec: disable hooks on install Default to false type: boolean + existingCRDs: + default: Skip + description: |- + ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts + included) already exists in the cluster with a different definition. CRDs not yet in the + cluster are always created. Ignored when SkipCRDs is true. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. + Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Overwrite applies the CRD of the chart over the existing one. + Default to Skip + enum: + - Skip + - Fail + - Overwrite + type: string recoverAfterConsecutiveFailures: default: 5 description: |- @@ -9655,6 +9700,21 @@ spec: disable hooks on install Default to false type: boolean + existingCRDs: + default: Skip + description: |- + ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts + included) already exists in the cluster with a different definition. CRDs not yet in the + cluster are always created. Ignored when SkipCRDs is true. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. + Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Overwrite applies the CRD of the chart over the existing one. + Default to Skip + enum: + - Skip + - Fail + - Overwrite + type: string recoverAfterConsecutiveFailures: default: 5 description: |- diff --git a/test/fv/helm_crds_test.go b/test/fv/helm_crds_test.go new file mode 100644 index 00000000..32b2d114 --- /dev/null +++ b/test/fv/helm_crds_test.go @@ -0,0 +1,202 @@ +/* +Copyright 2026. projectsveltos.io. All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package fv_test + +import ( + "context" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/util/retry" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + + configv1beta1 "github.com/projectsveltos/addon-controller/api/v1beta1" + "github.com/projectsveltos/addon-controller/lib/clusterops" + libsveltosv1beta1 "github.com/projectsveltos/libsveltos/api/v1beta1" +) + +var _ = Describe("Helm CRDs", func() { + const ( + namePrefix = "helm-crds-" + ) + + // Envoy Gateway v1.7.x ships its CRDs (Gateway API included) in the chart's crds/ directory. + // Starting with v1.9.x all of them are in the crds subchart. Upgrading from v1.7.4 to v1.9.2 + // with upgradeCRDs set must therefore update CRDs that only exist in a subchart. + It("Upgrades the CRDs shipped in a subchart", Label("FV", "EXTENDED"), func() { + Byf("Getting client to access the workload cluster") + workloadClient, err := getKindWorkloadClusterKubeconfig() + Expect(err).To(BeNil()) + Expect(workloadClient).ToNot(BeNil()) + + // Helm never removes CRDs. Start from, and leave behind, a cluster without them. + deleteEnvoyGatewayCRDs(workloadClient) + DeferCleanup(func() { deleteEnvoyGatewayCRDs(workloadClient) }) + + Byf("Create a ClusterProfile matching Cluster %s/%s", kindWorkloadCluster.GetNamespace(), kindWorkloadCluster.GetName()) + clusterProfile := getClusterProfile(namePrefix, map[string]string{key: value}) + clusterProfile.Spec.SyncMode = configv1beta1.SyncModeContinuous + clusterProfile.Spec.HelmCharts = []configv1beta1.HelmChart{ + newEnvoyGatewayChart(envoyGatewayVersion174), + } + Expect(k8sClient.Create(context.TODO(), clusterProfile)).To(Succeed()) + + verifyClusterProfileMatches(clusterProfile) + + clusterSummary := verifyClusterSummary(clusterops.ClusterProfileLabelName, clusterProfile.Name, + &clusterProfile.Spec, kindWorkloadCluster.GetNamespace(), kindWorkloadCluster.GetName(), getClusterType()) + + Byf("Verifying ClusterSummary %s status is set to Deployed for Helm feature", clusterSummary.Name) + verifyFeatureStatusIsProvisioned(kindWorkloadCluster.GetNamespace(), clusterSummary.Name, libsveltosv1beta1.FeatureHelm) + + Byf("Verifying the Gateway API CRDs are at bundle version %s", gatewayAPIBundleVersion141) + Eventually(func() string { + return getCRDBundleVersion(workloadClient, gatewayClassesCRDName) + }, timeout, pollingInterval).Should(Equal(gatewayAPIBundleVersion141)) + + Byf("Verifying the CRD only present in newer Gateway API bundles does not exist yet") + Expect(crdExists(workloadClient, gatewayListenerSetsCRDName)).To(BeFalse()) + + Byf("Changing ClusterProfile %s to require Envoy Gateway %s", clusterProfile.Name, envoyGatewayVersion192) + currentClusterProfile := &configv1beta1.ClusterProfile{} + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { + Expect(k8sClient.Get(context.TODO(), + types.NamespacedName{Name: clusterProfile.Name}, currentClusterProfile)).To(Succeed()) + currentClusterProfile.Spec.HelmCharts = []configv1beta1.HelmChart{ + newEnvoyGatewayChart(envoyGatewayVersion192), + } + return k8sClient.Update(context.TODO(), currentClusterProfile) + }) + Expect(err).To(BeNil()) + + Expect(k8sClient.Get(context.TODO(), + types.NamespacedName{Name: clusterProfile.Name}, currentClusterProfile)).To(Succeed()) + + clusterSummary = verifyClusterSummary(clusterops.ClusterProfileLabelName, + currentClusterProfile.Name, ¤tClusterProfile.Spec, + kindWorkloadCluster.GetNamespace(), kindWorkloadCluster.GetName(), getClusterType()) + + Byf("Verifying ClusterSummary %s status is set to Deployed for Helm feature", clusterSummary.Name) + verifyFeatureStatusIsProvisioned(kindWorkloadCluster.GetNamespace(), clusterSummary.Name, libsveltosv1beta1.FeatureHelm) + + charts := []configv1beta1.Chart{ + {ReleaseName: envoyGatewayRelease, ChartVersion: envoyGatewayVersion192, Namespace: envoyGatewayNamespace}, + } + verifyClusterConfiguration(configv1beta1.ClusterProfileKind, clusterProfile.Name, + clusterSummary.Spec.ClusterNamespace, clusterSummary.Spec.ClusterName, libsveltosv1beta1.FeatureHelm, + nil, charts) + + Byf("Verifying the Gateway API CRDs, now only in the crds subchart, are at bundle version %s", + gatewayAPIBundleVersion161) + Eventually(func() string { + return getCRDBundleVersion(workloadClient, gatewayClassesCRDName) + }, timeout, pollingInterval).Should(Equal(gatewayAPIBundleVersion161)) + + Byf("Verifying the CRD only present in newer Gateway API bundles has been created") + Eventually(func() bool { + return crdExists(workloadClient, gatewayListenerSetsCRDName) + }, timeout, pollingInterval).Should(BeTrue()) + + deleteClusterProfile(clusterProfile) + }) +}) + +// newEnvoyGatewayChart returns the Envoy Gateway chart at the given version, with CRDs upgraded. +// Hooks are disabled: the certgen job is not needed to verify CRDs and would require the +// Envoy Gateway image to be pulled in the workload cluster. +func newEnvoyGatewayChart(version string) configv1beta1.HelmChart { + return configv1beta1.HelmChart{ + RepositoryURL: envoyGatewayURL, + RepositoryName: envoyGatewayRepoName, + ChartName: envoyGatewayChartName, + ChartVersion: version, + ReleaseName: envoyGatewayRelease, + ReleaseNamespace: envoyGatewayNamespace, + HelmChartAction: configv1beta1.HelmChartActionInstall, + Options: &configv1beta1.HelmOptions{ + DisableHooks: true, + InstallOptions: configv1beta1.HelmInstallOptions{ + CreateNamespace: ptr.To(true), + }, + UpgradeOptions: configv1beta1.HelmUpgradeOptions{ + UpgradeCRDs: true, + }, + }, + } +} + +// getCRDBundleVersion returns the Gateway API bundle version annotation of the CRD, +// or an empty string when the CRD cannot be read. +func getCRDBundleVersion(workloadClient client.Client, crdName string) string { + crd := &apiextensionsv1.CustomResourceDefinition{} + if err := workloadClient.Get(context.TODO(), types.NamespacedName{Name: crdName}, crd); err != nil { + return "" + } + + return crd.Annotations[gatewayAPIBundleVersionKey] +} + +func crdExists(workloadClient client.Client, crdName string) bool { + crd := &apiextensionsv1.CustomResourceDefinition{} + err := workloadClient.Get(context.TODO(), types.NamespacedName{Name: crdName}, crd) + if apierrors.IsNotFound(err) { + return false + } + Expect(err).To(BeNil()) + + return true +} + +// isEnvoyGatewayCRD returns true for the CRDs installed by the Envoy Gateway chart, Gateway API ones included. +func isEnvoyGatewayCRD(crd *apiextensionsv1.CustomResourceDefinition) bool { + switch crd.Spec.Group { + case gatewayAPIGroup, gatewayAPIExperimentalGroup, envoyGatewayGroup: + return true + default: + return false + } +} + +// deleteEnvoyGatewayCRDs removes the CRDs installed by the Envoy Gateway chart and waits for them to be gone. +// Helm does not remove CRDs when a release is uninstalled. +func deleteEnvoyGatewayCRDs(workloadClient client.Client) { + crds := &apiextensionsv1.CustomResourceDefinitionList{} + Expect(workloadClient.List(context.TODO(), crds)).To(Succeed()) + + for i := range crds.Items { + if isEnvoyGatewayCRD(&crds.Items[i]) { + Expect(client.IgnoreNotFound(workloadClient.Delete(context.TODO(), &crds.Items[i]))).To(Succeed()) + } + } + + Eventually(func() int { + current := &apiextensionsv1.CustomResourceDefinitionList{} + Expect(workloadClient.List(context.TODO(), current)).To(Succeed()) + remaining := 0 + for i := range current.Items { + if isEnvoyGatewayCRD(¤t.Items[i]) { + remaining++ + } + } + return remaining + }, timeout, pollingInterval).Should(BeZero()) +} diff --git a/test/fv/test_constants_test.go b/test/fv/test_constants_test.go index 3ab9792b..913c80a5 100644 --- a/test/fv/test_constants_test.go +++ b/test/fv/test_constants_test.go @@ -105,6 +105,24 @@ const ( // Resource types resourceTypeSecrets = "secrets" + // Envoy Gateway chart details. Up to v1.7.x the CRDs are in the chart's crds/ directory, + // from v1.9.x they are all in the crds subchart (charts/crds/crds/) + envoyGatewayURL = "oci://docker.io/envoyproxy" + envoyGatewayRepoName = "envoyproxy" + envoyGatewayChartName = "gateway-helm" + envoyGatewayRelease = "eg" + envoyGatewayNamespace = "envoy-gateway-system" + envoyGatewayVersion174 = "v1.7.4" + envoyGatewayVersion192 = "v1.9.2" + gatewayAPIBundleVersionKey = "gateway.networking.k8s.io/bundle-version" + gatewayAPIBundleVersion141 = "v1.4.1" + gatewayAPIBundleVersion161 = "v1.6.1" + gatewayClassesCRDName = "gatewayclasses.gateway.networking.k8s.io" + gatewayListenerSetsCRDName = "listenersets.gateway.networking.k8s.io" + gatewayAPIGroup = "gateway.networking.k8s.io" + gatewayAPIExperimentalGroup = "gateway.networking.x-k8s.io" + envoyGatewayGroup = "gateway.envoyproxy.io" + // MariaDB chart details mariadbOperatorURL = "https://helm.mariadb.com/mariadb-operator" mariadbOperatorName = "mariadb-operator" From f3be8da4f4867b1d3f8de8a2890b8e4cff3359c2 Mon Sep 17 00:00:00 2001 From: Gianluca Mardente Date: Sun, 18 Oct 2026 14:13:24 +0200 Subject: [PATCH 2/2] feat: make Fail the default for installOptions.existingCRDs (#2041) A fresh install of a chart whose crds/ already exist in the cluster with a different definition now fails by default and reports the CRDs that differ, so Sveltos never decides on behalf of the user whether to keep or replace a CRD that may be owned by another tool. It matches what Helm 4 does by default when the existing CRD has conflicting fields. Users choose explicitly with installOptions.existingCRDs: - Skip: leave the existing CRDs as they are and carry on - Overwrite: apply the chart's CRDs over the existing ones Releases that are already deployed are not affected, only fresh installs go through this step. --- api/v1beta1/spec.go | 6 +- ...fig.projectsveltos.io_clusterprofiles.yaml | 6 +- ...g.projectsveltos.io_clusterpromotions.yaml | 6 +- ...ig.projectsveltos.io_clustersummaries.yaml | 6 +- .../config.projectsveltos.io_profiles.yaml | 6 +- controllers/handlers_helm.go | 2 +- controllers/handlers_helm_test.go | 59 ++++++++++++++++--- lib/crd/clusterprofiles.go | 6 +- lib/crd/clusterpromotions.go | 6 +- lib/crd/clustersummaries.go | 6 +- lib/crd/profiles.go | 6 +- manifest/manifest.yaml | 24 ++++---- 12 files changed, 91 insertions(+), 48 deletions(-) diff --git a/api/v1beta1/spec.go b/api/v1beta1/spec.go index e9098a06..26d53541 100644 --- a/api/v1beta1/spec.go +++ b/api/v1beta1/spec.go @@ -328,11 +328,11 @@ type HelmInstallOptions struct { // ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts // included) already exists in the cluster with a different definition. CRDs not yet in the // cluster are always created. Ignored when SkipCRDs is true. - // Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. // Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + // Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. // Overwrite applies the CRD of the chart over the existing one. - // Default to Skip - // +kubebuilder:default:=Skip + // Default to Fail + // +kubebuilder:default:=Fail // +optional ExistingCRDs ExistingCRDsPolicy `json:"existingCRDs,omitempty"` diff --git a/config/crd/bases/config.projectsveltos.io_clusterprofiles.yaml b/config/crd/bases/config.projectsveltos.io_clusterprofiles.yaml index 664edf59..5f306f90 100644 --- a/config/crd/bases/config.projectsveltos.io_clusterprofiles.yaml +++ b/config/crd/bases/config.projectsveltos.io_clusterprofiles.yaml @@ -368,15 +368,15 @@ spec: Default to false type: boolean existingCRDs: - default: Skip + default: Fail description: |- ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts included) already exists in the cluster with a different definition. CRDs not yet in the cluster are always created. Ignored when SkipCRDs is true. - Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Overwrite applies the CRD of the chart over the existing one. - Default to Skip + Default to Fail enum: - Skip - Fail diff --git a/config/crd/bases/config.projectsveltos.io_clusterpromotions.yaml b/config/crd/bases/config.projectsveltos.io_clusterpromotions.yaml index 198a426e..ba189626 100644 --- a/config/crd/bases/config.projectsveltos.io_clusterpromotions.yaml +++ b/config/crd/bases/config.projectsveltos.io_clusterpromotions.yaml @@ -268,15 +268,15 @@ spec: Default to false type: boolean existingCRDs: - default: Skip + default: Fail description: |- ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts included) already exists in the cluster with a different definition. CRDs not yet in the cluster are always created. Ignored when SkipCRDs is true. - Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Overwrite applies the CRD of the chart over the existing one. - Default to Skip + Default to Fail enum: - Skip - Fail diff --git a/config/crd/bases/config.projectsveltos.io_clustersummaries.yaml b/config/crd/bases/config.projectsveltos.io_clustersummaries.yaml index e6e73991..9e7d5dae 100644 --- a/config/crd/bases/config.projectsveltos.io_clustersummaries.yaml +++ b/config/crd/bases/config.projectsveltos.io_clustersummaries.yaml @@ -405,15 +405,15 @@ spec: Default to false type: boolean existingCRDs: - default: Skip + default: Fail description: |- ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts included) already exists in the cluster with a different definition. CRDs not yet in the cluster are always created. Ignored when SkipCRDs is true. - Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Overwrite applies the CRD of the chart over the existing one. - Default to Skip + Default to Fail enum: - Skip - Fail diff --git a/config/crd/bases/config.projectsveltos.io_profiles.yaml b/config/crd/bases/config.projectsveltos.io_profiles.yaml index 364119aa..f913f9e8 100644 --- a/config/crd/bases/config.projectsveltos.io_profiles.yaml +++ b/config/crd/bases/config.projectsveltos.io_profiles.yaml @@ -368,15 +368,15 @@ spec: Default to false type: boolean existingCRDs: - default: Skip + default: Fail description: |- ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts included) already exists in the cluster with a different definition. CRDs not yet in the cluster are always created. Ignored when SkipCRDs is true. - Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Overwrite applies the CRD of the chart over the existing one. - Default to Skip + Default to Fail enum: - Skip - Fail diff --git a/controllers/handlers_helm.go b/controllers/handlers_helm.go index 83e7c857..3f403cb8 100644 --- a/controllers/handlers_helm.go +++ b/controllers/handlers_helm.go @@ -4395,7 +4395,7 @@ func getTakeOwnershipHelmValue(options *configv1beta1.HelmOptions, isUpgrade boo func getExistingCRDsPolicy(options *configv1beta1.HelmOptions) configv1beta1.ExistingCRDsPolicy { if options == nil || options.InstallOptions.ExistingCRDs == "" { - return configv1beta1.ExistingCRDsPolicySkip + return configv1beta1.ExistingCRDsPolicyFail } return options.InstallOptions.ExistingCRDs diff --git a/controllers/handlers_helm_test.go b/controllers/handlers_helm_test.go index 6a17995d..3d3fde20 100644 --- a/controllers/handlers_helm_test.go +++ b/controllers/handlers_helm_test.go @@ -3299,20 +3299,20 @@ var _ = Describe("getCRDObjects", func() { }) var _ = Describe("getExistingCRDsPolicy", func() { - It("defaults to Skip when no options are set", func() { - Expect(controllers.GetExistingCRDsPolicy(nil)).To(Equal(configv1beta1.ExistingCRDsPolicySkip)) + It("defaults to Fail when no options are set", func() { + Expect(controllers.GetExistingCRDsPolicy(nil)).To(Equal(configv1beta1.ExistingCRDsPolicyFail)) }) - It("defaults to Skip when the policy is not set", func() { + It("defaults to Fail when the policy is not set", func() { Expect(controllers.GetExistingCRDsPolicy(&configv1beta1.HelmOptions{})).To( - Equal(configv1beta1.ExistingCRDsPolicySkip)) + Equal(configv1beta1.ExistingCRDsPolicyFail)) }) It("returns the configured policy", func() { options := &configv1beta1.HelmOptions{ - InstallOptions: configv1beta1.HelmInstallOptions{ExistingCRDs: configv1beta1.ExistingCRDsPolicyFail}, + InstallOptions: configv1beta1.HelmInstallOptions{ExistingCRDs: configv1beta1.ExistingCRDsPolicySkip}, } - Expect(controllers.GetExistingCRDsPolicy(options)).To(Equal(configv1beta1.ExistingCRDsPolicyFail)) + Expect(controllers.GetExistingCRDsPolicy(options)).To(Equal(configv1beta1.ExistingCRDsPolicySkip)) }) }) @@ -3565,8 +3565,7 @@ var _ = Describe("installChartCRDs", func() { Expect(handled).To(BeFalse()) }) - It("installs the CRDs itself and tells Helm to skip them by default", func() { - group := strings.ToLower(randomString()) + ".io" + crdManifest := func(group string, properties ...string) string { manifest := fmt.Sprintf(`apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: @@ -3584,7 +3583,51 @@ spec: schema: openAPIV3Schema: type: object + properties: + spec: + type: object + properties: `, group, group) + for _, property := range properties { + manifest += fmt.Sprintf(" %s: {type: string}\n", property) + } + return manifest + } + + It("fails by default when an existing CRD differs from the chart's", func() { + group := strings.ToLower(randomString()) + ".io" + restConfigCtx := controllers.WithRestConfig(ctx, testEnv.Config) + + existing := []chart.CRD{{Name: topCRDFile, File: &common.File{Name: topCRDFile, + Data: []byte(crdManifest(group, "a", "b"))}}} + handled, err := controllers.InstallChartCRDs(restConfigCtx, newHelmChart(nil), "", existing, logger) + Expect(err).To(BeNil()) + Expect(handled).To(BeTrue()) + + dr := crdResourceInterface() + DeferCleanup(func() { _ = dr.Delete(ctx, "tops."+group, metav1.DeleteOptions{}) }) + + older := []chart.CRD{{Name: topCRDFile, File: &common.File{Name: topCRDFile, + Data: []byte(crdManifest(group, "a"))}}} + handled, err = controllers.InstallChartCRDs(restConfigCtx, newHelmChart(nil), "", older, logger) + Expect(err).ToNot(BeNil()) + Expect(err.Error()).To(ContainSubstring("tops." + group)) + Expect(handled).To(BeFalse()) + + By("Verifying the existing CRD has not been changed") + current, err := dr.Get(ctx, "tops."+group, metav1.GetOptions{}) + Expect(err).To(BeNil()) + versions, _, err := unstructured.NestedSlice(current.Object, "spec", "versions") + Expect(err).To(BeNil()) + properties, _, err := unstructured.NestedMap(versions[0].(map[string]interface{}), + "schema", "openAPIV3Schema", "properties", "spec", "properties") + Expect(err).To(BeNil()) + Expect(properties).To(HaveKey("b")) + }) + + It("installs the CRDs itself and tells Helm to skip them by default", func() { + group := strings.ToLower(randomString()) + ".io" + manifest := crdManifest(group, "a") topCRD := []chart.CRD{{Name: topCRDFile, File: &common.File{Name: topCRDFile, Data: []byte(manifest)}}} restConfigCtx := controllers.WithRestConfig(ctx, testEnv.Config) diff --git a/lib/crd/clusterprofiles.go b/lib/crd/clusterprofiles.go index 8f305089..a3a7ba79 100644 --- a/lib/crd/clusterprofiles.go +++ b/lib/crd/clusterprofiles.go @@ -387,15 +387,15 @@ spec: Default to false type: boolean existingCRDs: - default: Skip + default: Fail description: |- ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts included) already exists in the cluster with a different definition. CRDs not yet in the cluster are always created. Ignored when SkipCRDs is true. - Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Overwrite applies the CRD of the chart over the existing one. - Default to Skip + Default to Fail enum: - Skip - Fail diff --git a/lib/crd/clusterpromotions.go b/lib/crd/clusterpromotions.go index eb79b35d..313c46e1 100644 --- a/lib/crd/clusterpromotions.go +++ b/lib/crd/clusterpromotions.go @@ -287,15 +287,15 @@ spec: Default to false type: boolean existingCRDs: - default: Skip + default: Fail description: |- ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts included) already exists in the cluster with a different definition. CRDs not yet in the cluster are always created. Ignored when SkipCRDs is true. - Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Overwrite applies the CRD of the chart over the existing one. - Default to Skip + Default to Fail enum: - Skip - Fail diff --git a/lib/crd/clustersummaries.go b/lib/crd/clustersummaries.go index 7341a0dd..1fda6f0e 100644 --- a/lib/crd/clustersummaries.go +++ b/lib/crd/clustersummaries.go @@ -424,15 +424,15 @@ spec: Default to false type: boolean existingCRDs: - default: Skip + default: Fail description: |- ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts included) already exists in the cluster with a different definition. CRDs not yet in the cluster are always created. Ignored when SkipCRDs is true. - Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Overwrite applies the CRD of the chart over the existing one. - Default to Skip + Default to Fail enum: - Skip - Fail diff --git a/lib/crd/profiles.go b/lib/crd/profiles.go index 9af24cd6..87c72dde 100644 --- a/lib/crd/profiles.go +++ b/lib/crd/profiles.go @@ -387,15 +387,15 @@ spec: Default to false type: boolean existingCRDs: - default: Skip + default: Fail description: |- ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts included) already exists in the cluster with a different definition. CRDs not yet in the cluster are always created. Ignored when SkipCRDs is true. - Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Overwrite applies the CRD of the chart over the existing one. - Default to Skip + Default to Fail enum: - Skip - Fail diff --git a/manifest/manifest.yaml b/manifest/manifest.yaml index 5677f6b2..ccd4157a 100644 --- a/manifest/manifest.yaml +++ b/manifest/manifest.yaml @@ -677,15 +677,15 @@ spec: Default to false type: boolean existingCRDs: - default: Skip + default: Fail description: |- ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts included) already exists in the cluster with a different definition. CRDs not yet in the cluster are always created. Ignored when SkipCRDs is true. - Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Overwrite applies the CRD of the chart over the existing one. - Default to Skip + Default to Fail enum: - Skip - Fail @@ -3237,15 +3237,15 @@ spec: Default to false type: boolean existingCRDs: - default: Skip + default: Fail description: |- ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts included) already exists in the cluster with a different definition. CRDs not yet in the cluster are always created. Ignored when SkipCRDs is true. - Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Overwrite applies the CRD of the chart over the existing one. - Default to Skip + Default to Fail enum: - Skip - Fail @@ -6972,15 +6972,15 @@ spec: Default to false type: boolean existingCRDs: - default: Skip + default: Fail description: |- ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts included) already exists in the cluster with a different definition. CRDs not yet in the cluster are always created. Ignored when SkipCRDs is true. - Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Overwrite applies the CRD of the chart over the existing one. - Default to Skip + Default to Fail enum: - Skip - Fail @@ -9701,15 +9701,15 @@ spec: Default to false type: boolean existingCRDs: - default: Skip + default: Fail description: |- ExistingCRDs controls what happens when a CRD in the chart's crds/ directory (subcharts included) already exists in the cluster with a different definition. CRDs not yet in the cluster are always created. Ignored when SkipCRDs is true. - Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Fail fails the installation and reports the CRDs that differ. No CRD is created or changed. + Skip leaves the existing CRD as it is, logs it and carries on with the rest of the chart. Overwrite applies the CRD of the chart over the existing one. - Default to Skip + Default to Fail enum: - Skip - Fail