From 8a1b6c00a83443a06b09a6e26f8f87f7b631275a Mon Sep 17 00:00:00 2001 From: Xavi Garcia Date: Wed, 2 Sep 2026 15:06:21 +0200 Subject: [PATCH] Deploy charts that define CRDs and use lookup (#5689) * Deploy charts that define CRDs and use lookup Charts that both declare CRDs in `crds/` and instantiate custom resources of those kinds cannot be deployed when they also use the `lookup` function. `lookup` forces a server-side dry run, and a server-side dry run does not install `crds/`, so the rendered custom resources have no REST mapping yet and Helm fails with `no matches for kind`. The dry run is now skipped when the failure consists only of REST mapping failures for kinds the chart itself declares in `crds/` (including its dependencies'). The real install installs `crds/` before building the manifest, so it validates the chart without hitting the chicken-and-egg problem. Any other dry run failure is still reported as before, and template mode is unaffected. Refers to: https://github.com/rancher/fleet/issues/5630 Signed-off-by: Xavi Garcia * Fix linter findings Signed-off-by: Xavi Garcia * return all leaf errors Signed-off-by: Xavi Garcia --------- Signed-off-by: Xavi Garcia --- .../agent/assets/crd-chart/Chart.yaml | 6 + .../assets/crd-chart/crds/appconfig.yaml | 25 ++ .../assets/crd-chart/templates/appconfig.yaml | 6 + .../assets/crd-chart/templates/configmap.yaml | 6 + .../agent/assets/missing-crd-chart/Chart.yaml | 6 + .../templates/configmap.yaml | 6 + .../templates/unknownconfig.yaml | 6 + .../agent/bundle_deployment_crds_test.go | 105 ++++++++ integrationtests/agent/suite_test.go | 41 +++ internal/helmdeployer/crds.go | 94 +++++++ internal/helmdeployer/crds_test.go | 248 ++++++++++++++++++ internal/helmdeployer/install.go | 16 +- 12 files changed, 563 insertions(+), 2 deletions(-) create mode 100644 integrationtests/agent/assets/crd-chart/Chart.yaml create mode 100644 integrationtests/agent/assets/crd-chart/crds/appconfig.yaml create mode 100644 integrationtests/agent/assets/crd-chart/templates/appconfig.yaml create mode 100644 integrationtests/agent/assets/crd-chart/templates/configmap.yaml create mode 100644 integrationtests/agent/assets/missing-crd-chart/Chart.yaml create mode 100644 integrationtests/agent/assets/missing-crd-chart/templates/configmap.yaml create mode 100644 integrationtests/agent/assets/missing-crd-chart/templates/unknownconfig.yaml create mode 100644 integrationtests/agent/bundle_deployment_crds_test.go create mode 100644 internal/helmdeployer/crds.go create mode 100644 internal/helmdeployer/crds_test.go diff --git a/integrationtests/agent/assets/crd-chart/Chart.yaml b/integrationtests/agent/assets/crd-chart/Chart.yaml new file mode 100644 index 0000000000..64dc46a8e1 --- /dev/null +++ b/integrationtests/agent/assets/crd-chart/Chart.yaml @@ -0,0 +1,6 @@ +apiVersion: v2 +name: crd-chart +description: A test chart defining a CRD, a custom resource and a template using lookup +type: application +version: 0.1.0 +appVersion: "1.16.0" diff --git a/integrationtests/agent/assets/crd-chart/crds/appconfig.yaml b/integrationtests/agent/assets/crd-chart/crds/appconfig.yaml new file mode 100644 index 0000000000..20f3e0ede5 --- /dev/null +++ b/integrationtests/agent/assets/crd-chart/crds/appconfig.yaml @@ -0,0 +1,25 @@ +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: appconfigs.stable.example.com +spec: + group: stable.example.com + names: + kind: AppConfig + listKind: AppConfigList + plural: appconfigs + singular: appconfig + scope: Namespaced + versions: + - name: v1 + served: true + storage: true + schema: + openAPIV3Schema: + type: object + properties: + spec: + type: object + properties: + value: + type: string diff --git a/integrationtests/agent/assets/crd-chart/templates/appconfig.yaml b/integrationtests/agent/assets/crd-chart/templates/appconfig.yaml new file mode 100644 index 0000000000..470c6ab592 --- /dev/null +++ b/integrationtests/agent/assets/crd-chart/templates/appconfig.yaml @@ -0,0 +1,6 @@ +apiVersion: stable.example.com/v1 +kind: AppConfig +metadata: + name: frontend-config +spec: + value: {{ .Values.value | quote }} diff --git a/integrationtests/agent/assets/crd-chart/templates/configmap.yaml b/integrationtests/agent/assets/crd-chart/templates/configmap.yaml new file mode 100644 index 0000000000..eb9dc316b8 --- /dev/null +++ b/integrationtests/agent/assets/crd-chart/templates/configmap.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: lookup-chart-config +data: + namespace: {{ (lookup "v1" "Namespace" "" .Release.Namespace).metadata.name }} diff --git a/integrationtests/agent/assets/missing-crd-chart/Chart.yaml b/integrationtests/agent/assets/missing-crd-chart/Chart.yaml new file mode 100644 index 0000000000..7bb0eb5b22 --- /dev/null +++ b/integrationtests/agent/assets/missing-crd-chart/Chart.yaml @@ -0,0 +1,6 @@ +apiVersion: v2 +name: missing-crd-chart +description: A test chart using lookup and a custom resource whose CRD is not part of the chart +type: application +version: 0.1.0 +appVersion: "1.16.0" diff --git a/integrationtests/agent/assets/missing-crd-chart/templates/configmap.yaml b/integrationtests/agent/assets/missing-crd-chart/templates/configmap.yaml new file mode 100644 index 0000000000..912a92a77c --- /dev/null +++ b/integrationtests/agent/assets/missing-crd-chart/templates/configmap.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: missing-crd-chart-config +data: + namespace: {{ (lookup "v1" "Namespace" "" .Release.Namespace).metadata.name }} diff --git a/integrationtests/agent/assets/missing-crd-chart/templates/unknownconfig.yaml b/integrationtests/agent/assets/missing-crd-chart/templates/unknownconfig.yaml new file mode 100644 index 0000000000..886e5c9096 --- /dev/null +++ b/integrationtests/agent/assets/missing-crd-chart/templates/unknownconfig.yaml @@ -0,0 +1,6 @@ +apiVersion: stable.example.com/v1 +kind: UnknownConfig +metadata: + name: unknown-config +spec: + value: {{ .Values.value | quote }} diff --git a/integrationtests/agent/bundle_deployment_crds_test.go b/integrationtests/agent/bundle_deployment_crds_test.go new file mode 100644 index 0000000000..e4699c7a82 --- /dev/null +++ b/integrationtests/agent/bundle_deployment_crds_test.go @@ -0,0 +1,105 @@ +package agent_test + +import ( + "context" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" +) + +var _ = Describe("Helm chart defining CRDs and using lookup", Ordered, func() { + var ( + env *specEnv + name string + ) + + createBundleDeployment := func(name, id, chart string) { + bundled := v1alpha1.BundleDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: clusterNS, + }, + Spec: v1alpha1.BundleDeploymentSpec{ + DeploymentID: id, + Options: v1alpha1.BundleDeploymentOptions{ + DefaultNamespace: env.namespace, + Helm: &v1alpha1.HelmOptions{ + Chart: chart, + Values: &v1alpha1.GenericMap{ + Data: map[string]any{"value": "example-value"}, + }, + }, + }, + }, + } + + Expect(k8sClient.Create(ctx, &bundled)).ToNot(HaveOccurred()) + + DeferCleanup(func() { + Expect(k8sClient.Delete(context.TODO(), &v1alpha1.BundleDeployment{ + ObjectMeta: metav1.ObjectMeta{Namespace: clusterNS, Name: name}, + })).ToNot(HaveOccurred()) + }) + } + + BeforeAll(func() { + env = &specEnv{namespace: createNamespace()} + }) + + When("the chart provides the CRDs for its custom resources", func() { + BeforeAll(func() { + name = "crds-and-lookup" + createBundleDeployment(name, "crdsAndLookup", "crd-chart") + }) + + It("deploys the chart, although its CRDs cannot be installed during the dry run", func() { + By("Making the BundleDeployment ready") + Eventually(env.isBundleDeploymentReadyAndNotModified).WithArguments(name).Should(BeTrue()) + + By("Deploying the custom resource defined by the chart's CRD") + appConfig := &unstructured.Unstructured{} + appConfig.SetGroupVersionKind(schema.GroupVersionKind{ + Group: "stable.example.com", + Version: "v1", + Kind: "AppConfig", + }) + err := k8sClient.Get( + ctx, + types.NamespacedName{Namespace: env.namespace, Name: "frontend-config"}, + appConfig, + ) + Expect(err).ToNot(HaveOccurred()) + + By("Resolving the lookup function against the cluster") + cm, err := env.getConfigMap("lookup-chart-config") + Expect(err).ToNot(HaveOccurred()) + Expect(cm.Data["namespace"]).To(Equal(env.namespace)) + }) + }) + + When("the chart does not provide the CRDs for its custom resources", func() { + BeforeAll(func() { + name = "missing-crd-and-lookup" + createBundleDeployment(name, "missingCRDAndLookup", "missing-crd-chart") + }) + + It("reports the missing CRD in the BundleDeployment status", func() { + Eventually(func(g Gomega) { + bd := &v1alpha1.BundleDeployment{} + err := k8sClient.Get(ctx, types.NamespacedName{Namespace: clusterNS, Name: name}, bd) + g.Expect(err).ToNot(HaveOccurred()) + + checkCondition(g, bd.Status.Conditions, "Deployed", "False", `no matches for kind "UnknownConfig"`) + }).Should(Succeed()) + + Expect(env.isBundleDeploymentReadyAndNotModified(name)).To(BeFalse()) + }) + }) +}) diff --git a/integrationtests/agent/suite_test.go b/integrationtests/agent/suite_test.go index f572eff9da..11f5624079 100644 --- a/integrationtests/agent/suite_test.go +++ b/integrationtests/agent/suite_test.go @@ -3,7 +3,9 @@ package agent_test import ( "context" "fmt" + "io/fs" "os" + "path/filepath" "sync" "testing" "time" @@ -448,6 +450,12 @@ data: Name: "fleet.yaml", }, }, + // A chart which defines a CRD in crds/, deploys a custom resource of + // that kind and uses the lookup function in one of its templates. + "crdsAndLookup": chartResources("crd-chart"), + // The same, but the CRD for the deployed custom resource is missing + // from the chart. + "missingCRDAndLookup": chartResources("missing-crd-chart"), "capabilitiesv2": []v1alpha1.BundleResource{ { Content: "apiVersion: v2\nname: config-chart\ndescription: A test chart that verifies its config\ntype: application\nversion: 0.1.0\nappVersion: \"1.16.0\"\nkubeVersion: '>= 920.920.0-0'\n", @@ -464,3 +472,36 @@ data: }, } } + +// chartResources reads a chart from the assets directory and turns it into +// bundle resources, named after their path relative to the assets directory. +func chartResources(chartDir string) []v1alpha1.BundleResource { + var resources []v1alpha1.BundleResource + + root := filepath.Join(assetsPath, chartDir) + err := filepath.WalkDir(root, func(path string, entry fs.DirEntry, err error) error { + if err != nil || entry.IsDir() { + return err + } + + content, err := os.ReadFile(path) + if err != nil { + return err + } + + name, err := filepath.Rel(assetsPath, path) + if err != nil { + return err + } + + resources = append(resources, v1alpha1.BundleResource{ + Name: name, + Content: string(content), + }) + + return nil + }) + Expect(err).ToNot(HaveOccurred()) + + return resources +} diff --git a/internal/helmdeployer/crds.go b/internal/helmdeployer/crds.go new file mode 100644 index 0000000000..74c6628739 --- /dev/null +++ b/internal/helmdeployer/crds.go @@ -0,0 +1,94 @@ +package helmdeployer + +import ( + "bytes" + "errors" + + chartv2 "helm.sh/helm/v4/pkg/chart/v2" + + apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/runtime/schema" + utilerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apimachinery/pkg/util/yaml" +) + +// isMissingOwnCRDsError reports whether err consists solely of REST mapping +// failures for kinds which the chart itself declares in its crds/ directory. +// +// A server-side dry run cannot install those CRDs, hence custom resources +// defined by the chart have no REST mapping yet and Helm fails to build the +// Kubernetes objects for the rendered manifest. The actual install installs +// the contents of crds/ first, so it does not suffer from this limitation. +func isMissingOwnCRDsError(err error, chart *chartv2.Chart) bool { + leaves := leafErrors(err) + if len(leaves) == 0 { + return false + } + + declared := chartCRDGroupKinds(chart) + for _, leaf := range leaves { + noKindMatch, ok := errors.AsType[*meta.NoKindMatchError](leaf) + if !ok || !declared.Has(noKindMatch.GroupKind) { + return false + } + } + + return true +} + +// leafErrors returns the leaf errors contained in err, expanding aggregates +// recursively. Errors reported while building a resource list are wrapped into +// an aggregate when more than one resource fails, and aggregates cannot be +// traversed by errors.AsType, hence the explicit recursion. +func leafErrors(err error) []error { + if err == nil { + return nil + } + + aggregate, ok := errors.AsType[utilerrors.Aggregate](err) + if !ok { + return []error{err} + } + + var result []error + for _, e := range aggregate.Errors() { + result = append(result, leafErrors(e)...) + } + + return result +} + +// chartCRDGroupKinds returns the group kinds defined by the CRDs found in the +// crds/ directory of the chart and of its dependencies. +func chartCRDGroupKinds(chart *chartv2.Chart) sets.Set[schema.GroupKind] { + groupKinds := sets.New[schema.GroupKind]() + if chart == nil { + return groupKinds + } + + for _, crd := range chart.CRDObjects() { + if crd.File == nil { + continue + } + + // CRD files may contain multiple documents. + decoder := yaml.NewYAMLToJSONDecoder(bytes.NewReader(crd.File.Data)) + for { + var def apiextv1.CustomResourceDefinition + if err := decoder.Decode(&def); err != nil { + // Stop at the first unreadable document: an incomplete list of + // group kinds can only make the caller stricter, and Helm + // reports invalid CRDs when it installs them for real. + break + } + if def.Spec.Group == "" || def.Spec.Names.Kind == "" { + continue + } + groupKinds.Insert(schema.GroupKind{Group: def.Spec.Group, Kind: def.Spec.Names.Kind}) + } + } + + return groupKinds +} diff --git a/internal/helmdeployer/crds_test.go b/internal/helmdeployer/crds_test.go new file mode 100644 index 0000000000..36aa83bc91 --- /dev/null +++ b/internal/helmdeployer/crds_test.go @@ -0,0 +1,248 @@ +package helmdeployer + +import ( + "errors" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "helm.sh/helm/v4/pkg/chart/common" + chartv2 "helm.sh/helm/v4/pkg/chart/v2" + + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/runtime/schema" + utilerrors "k8s.io/apimachinery/pkg/util/errors" +) + +const appConfigCRD = `apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: appconfigs.stable.example.com +spec: + group: stable.example.com + names: + kind: AppConfig + plural: appconfigs + scope: Namespaced + versions: + - name: v1 + served: true + storage: true +` + +const multiDocCRDs = appConfigCRD + `--- +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: backends.stable.example.com +spec: + group: stable.example.com + names: + kind: Backend + plural: backends + scope: Namespaced + versions: + - name: v1 + served: true + storage: true +` + +// noKindMatchFor builds the error which Helm returns when the REST mapping for +// a rendered resource cannot be found. +func noKindMatchFor(group, kind, name string) error { + err := &meta.NoKindMatchError{ + GroupKind: schema.GroupKind{Group: group, Kind: kind}, + SearchedVersions: []string{"v1"}, + } + + return fmt.Errorf( + "resource mapping not found for name: %q namespace: %q from %q: %w\nensure CRDs are installed first", + name, "", "", err, + ) +} + +// buildError wraps err the way Helm does when building the Kubernetes objects +// for a release manifest fails. +func buildError(errs ...error) error { + var err error + if len(errs) == 1 { + err = errs[0] + } else { + err = utilerrors.NewAggregate(errs) + } + + return fmt.Errorf("unable to build kubernetes objects from release manifest: %w", err) +} + +func TestLeafErrors(t *testing.T) { + appConfig := noKindMatchFor("stable.example.com", "AppConfig", "frontend-config") + backend := noKindMatchFor("stable.example.com", "Backend", "frontend-backend") + unrelated := errors.New("something else went wrong") + + testCases := []struct { + name string + err error + expectedLeaves []string + }{ + { + name: "nil error", + err: nil, + expectedLeaves: nil, + }, + { + name: "unrelated error", + err: unrelated, + expectedLeaves: []string{unrelated.Error()}, + }, + { + name: "single wrapped error", + err: buildError(appConfig), + expectedLeaves: []string{buildError(appConfig).Error()}, + }, + { + name: "aggregated errors", + err: buildError(appConfig, backend), + expectedLeaves: []string{appConfig.Error(), backend.Error()}, + }, + { + name: "aggregate mixing unrelated errors", + err: buildError(unrelated, appConfig), + expectedLeaves: []string{unrelated.Error(), appConfig.Error()}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + assert := assert.New(t) + + var leaves []string + for _, err := range leafErrors(tc.err) { + leaves = append(leaves, err.Error()) + } + + assert.Equal(tc.expectedLeaves, leaves) + }) + } +} + +func TestIsMissingOwnCRDsError(t *testing.T) { + testCases := []struct { + name string + crds []*common.File + dependencyCRDs []*common.File + err error + expectedResult bool + }{ + { + name: "no error", + crds: []*common.File{{Name: "crds/appconfig.yaml", Data: []byte(appConfigCRD)}}, + err: nil, + expectedResult: false, + }, + { + name: "unrelated error", + crds: []*common.File{{Name: "crds/appconfig.yaml", Data: []byte(appConfigCRD)}}, + err: buildError(errors.New("connection refused")), + expectedResult: false, + }, + { + name: "kind defined by the chart", + crds: []*common.File{{Name: "crds/appconfig.yaml", Data: []byte(appConfigCRD)}}, + err: buildError(noKindMatchFor("stable.example.com", "AppConfig", "frontend-config")), + expectedResult: true, + }, + { + name: "kinds from a multi document CRD file", + crds: []*common.File{{Name: "crds/crds.yaml", Data: []byte(multiDocCRDs)}}, + err: buildError( + noKindMatchFor("stable.example.com", "AppConfig", "frontend-config"), + noKindMatchFor("stable.example.com", "Backend", "frontend-backend"), + ), + expectedResult: true, + }, + { + name: "kind not defined by the chart", + crds: []*common.File{{Name: "crds/appconfig.yaml", Data: []byte(appConfigCRD)}}, + err: buildError(noKindMatchFor("stable.example.com", "AppConfigs", "frontend-config")), + expectedResult: false, + }, + { + name: "kind from another group", + crds: []*common.File{{Name: "crds/appconfig.yaml", Data: []byte(appConfigCRD)}}, + err: buildError(noKindMatchFor("other.example.com", "AppConfig", "frontend-config")), + expectedResult: false, + }, + { + name: "one kind out of two not defined by the chart", + crds: []*common.File{{Name: "crds/appconfig.yaml", Data: []byte(appConfigCRD)}}, + err: buildError( + noKindMatchFor("stable.example.com", "AppConfig", "frontend-config"), + noKindMatchFor("stable.example.com", "Backend", "frontend-backend"), + ), + expectedResult: false, + }, + { + name: "kind defined by the chart mixed with another error", + crds: []*common.File{{Name: "crds/appconfig.yaml", Data: []byte(appConfigCRD)}}, + err: buildError( + noKindMatchFor("stable.example.com", "AppConfig", "frontend-config"), + errors.New("connection refused"), + ), + expectedResult: false, + }, + { + name: "chart without CRDs", + crds: nil, + err: buildError(noKindMatchFor("stable.example.com", "AppConfig", "frontend-config")), + expectedResult: false, + }, + { + name: "CRD file not in the crds directory", + crds: []*common.File{{Name: "templates/appconfig.yaml", Data: []byte(appConfigCRD)}}, + err: buildError(noKindMatchFor("stable.example.com", "AppConfig", "frontend-config")), + expectedResult: false, + }, + { + name: "kind defined by a dependency", + crds: nil, + dependencyCRDs: []*common.File{{Name: "crds/appconfig.yaml", Data: []byte(appConfigCRD)}}, + err: buildError(noKindMatchFor("stable.example.com", "AppConfig", "frontend-config")), + expectedResult: true, + }, + { + name: "invalid CRD file", + crds: []*common.File{{Name: "crds/appconfig.yaml", Data: []byte("this is: not: a CRD")}}, + err: buildError(noKindMatchFor("stable.example.com", "AppConfig", "frontend-config")), + expectedResult: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + assert := assert.New(t) + + chart := newChartWithCRDs("test-chart", tc.crds) + if tc.dependencyCRDs != nil { + chart.AddDependency(newChartWithCRDs("dependency", tc.dependencyCRDs)) + } + + assert.Equal(tc.expectedResult, isMissingOwnCRDsError(tc.err, chart)) + }) + } +} + +func TestIsMissingOwnCRDsErrorNilChart(t *testing.T) { + err := buildError(noKindMatchFor("stable.example.com", "AppConfig", "frontend-config")) + + assert.False(t, isMissingOwnCRDsError(err, nil)) +} + +func newChartWithCRDs(name string, crds []*common.File) *chartv2.Chart { + return &chartv2.Chart{ + Metadata: &chartv2.Metadata{ + Name: name, + Version: "0.1.0", + }, + Files: crds, + } +} diff --git a/internal/helmdeployer/install.go b/internal/helmdeployer/install.go index b48e6b9c10..9479387396 100644 --- a/internal/helmdeployer/install.go +++ b/internal/helmdeployer/install.go @@ -66,8 +66,20 @@ func (h *Helm) Deploy(ctx context.Context, bundleID string, manifest *manifest.M chart.Metadata.Annotations[CommitAnnotation] = manifest.Commit } - if release, err := h.install(ctx, bundleID, manifest, chart, options, getDryRunConfig(chart, true)); err != nil { - return nil, err + release, err := h.install(ctx, bundleID, manifest, chart, options, getDryRunConfig(chart, true)) + if err != nil { + // A server-side dry run, which is needed for charts using the lookup + // function, cannot install the CRDs contained in the chart. Custom + // resources defined by those CRDs then have no REST mapping yet and the + // dry run fails. The install below installs crds/ first, hence it + // validates the manifest without hitting this chicken and egg problem. + if h.template || !isMissingOwnCRDsError(err, chart) { + return nil, err + } + log.FromContext(ctx).WithName("helm-deployer").Info( + "Skipping dry run validation, the CRDs defining resources from this chart are not installed yet", + "error", err, + ) } else if h.template { return release, nil }