diff --git a/pkg/cli/cmd/workload/render.go b/pkg/cli/cmd/workload/render.go index 57d86cbc6..15b920425 100644 --- a/pkg/cli/cmd/workload/render.go +++ b/pkg/cli/cmd/workload/render.go @@ -143,9 +143,26 @@ func warnDegradations(cmd *cobra.Command, degradations []render.Degradation) { continue } + content := "skipped Helm render for HelmRelease %s (validating the resource as-is): %s" + + switch degradation.Kind { + case render.DegradationPartialValues: + content = "rendered HelmRelease %s with incomplete values — valuesFrom %s " + + "could not be resolved offline (cluster-managed or in another kustomization); " + + "validation/scan may differ from the cluster" + case render.DegradationMissingValuesKey: + // The referent IS in the repo, so pointing at cluster-managed sources + // would misdirect; the actionable cause is the key itself (often a typo). + content = "rendered HelmRelease %s with incomplete values — valuesFrom %s " + + "is in the repo but has no such key (Flux fails reconciliation on a " + + "missing valuesKey, even when the reference is optional); " + + "validation/scan may differ from the cluster" + case render.DegradationSkippedRender: + } + notify.WriteMessage(notify.Message{ Type: notify.WarningType, - Content: "skipped Helm render for HelmRelease %s (validating the resource as-is): %s", + Content: content, Args: []any{degradation.HelmRelease, degradation.Reason}, Writer: cmd.ErrOrStderr(), }) diff --git a/pkg/cli/cmd/workload/validate_render_test.go b/pkg/cli/cmd/workload/validate_render_test.go index 9c7f2dc8c..1d4e1ae39 100644 --- a/pkg/cli/cmd/workload/validate_render_test.go +++ b/pkg/cli/cmd/workload/validate_render_test.go @@ -151,6 +151,69 @@ func TestValidateDegradesWhenChartUnreachable(t *testing.T) { assert.Contains(t, output, "skipped Helm render", "degradation should warn the user") } +// TestValidateWarnsOnUnresolvableValuesFrom verifies the render-fidelity honesty +// contract end-to-end: a HelmRelease whose non-optional valuesFrom references a +// ConfigMap absent from the offline stream (typically cluster-managed) still +// renders and passes, but the user is warned that values were incomplete — not +// the misleading "skipped Helm render" message, since the chart DID render. +func TestValidateWarnsOnUnresolvableValuesFrom(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + chartURL := localChartURL(t, "validchart") + files := map[string]string{ + "kustomization.yaml": `apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +resources: + - ocirepository.yaml + - helmrelease.yaml +`, + "ocirepository.yaml": `apiVersion: source.toolkit.fluxcd.io/v1 +kind: OCIRepository +metadata: + name: chart + namespace: flux-system +spec: + interval: 5m + url: ` + chartURL + ` +`, + "helmrelease.yaml": `apiVersion: helm.toolkit.fluxcd.io/v2 +kind: HelmRelease +metadata: + name: app + namespace: flux-system +spec: + interval: 5m + chartRef: + kind: OCIRepository + name: chart + valuesFrom: + - kind: ConfigMap + name: cluster-settings +`, + } + + for name, content := range files { + require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte(content), 0o600)) + } + + output, err := runValidate(t, dir, "--skip-kinds", "OCIRepository") + require.NoError(t, err, "an unresolvable valuesFrom must warn, not fail the run") + assert.Contains(t, output, "incomplete values", "the user should be warned values were partial") + assert.Contains( + t, + output, + "cluster-settings", + "the warning should name the unresolved reference", + ) + assert.NotContains( + t, + output, + "skipped Helm render", + "the chart did render, so the skipped-render message must not appear", + ) +} + // TestValidateSingleHelmReleaseFileStaysGreen guards the ksail-test-gen-smoke CI // canary: validating a single HelmRelease file (whose source is not present) // must not attempt rendering and must pass as a CR-schema check. diff --git a/pkg/svc/gitops/render/render.go b/pkg/svc/gitops/render/render.go index b2ac120e1..d70dcd08e 100644 --- a/pkg/svc/gitops/render/render.go +++ b/pkg/svc/gitops/render/render.go @@ -5,12 +5,14 @@ import ( "context" "encoding/base64" "errors" + "fmt" "maps" "sort" "strings" "github.com/devantler-tech/ksail/v7/pkg/fsutil" helmv2 "github.com/fluxcd/helm-controller/api/v2" + meta "github.com/fluxcd/pkg/apis/meta" sourcev1 "github.com/fluxcd/source-controller/api/v1" "sigs.k8s.io/yaml" ) @@ -51,12 +53,39 @@ type Document struct { Provenance Provenance } -// Degradation records a HelmRelease that could not be rendered offline. The -// caller falls back to validating/scanning the HelmRelease CR itself. Silent -// degradations are expected in normal repos (e.g. a source object that lives in -// a different Flux Kustomization) and should not be surfaced as warnings. +// DegradationKind classifies why a HelmRelease's offline render is less than +// full-fidelity, so callers can phrase an accurate warning. +type DegradationKind int + +const ( + // DegradationSkippedRender marks a HelmRelease whose chart could not be + // rendered offline at all; the caller falls back to validating/scanning the + // HelmRelease CR itself. This is the zero value, so a Degradation left + // without an explicit Kind keeps this meaning. + DegradationSkippedRender DegradationKind = iota + // DegradationPartialValues marks a HelmRelease that DID render, but with a + // non-optional valuesFrom reference that could not be resolved from the + // offline stream — so the rendered children may differ from what Flux applies + // with the real value present. + DegradationPartialValues + // DegradationMissingValuesKey marks a HelmRelease that DID render, but whose + // valuesFrom referent IS present in the stream while lacking the requested + // valuesKey — typically a valuesKey typo. It is a distinct kind because the + // remedy differs: the resource is in the repo, so the caller must not suggest + // the value is cluster-managed. Flux fails reconciliation on a missing + // valuesKey even when the reference is marked optional. + DegradationMissingValuesKey +) + +// Degradation records a way a HelmRelease's offline render fell short of what +// Flux would apply. For DegradationSkippedRender the caller falls back to +// validating/scanning the HelmRelease CR itself; for DegradationPartialValues +// the children were rendered but with incomplete values. Silent degradations +// are expected in normal repos (e.g. a source object that lives in a different +// Flux Kustomization) and should not be surfaced as warnings. type Degradation struct { HelmRelease string // "namespace/name" + Kind DegradationKind Reason string Err error Silent bool @@ -192,6 +221,68 @@ func expandHelmRelease( newDocument(child, parseObjectMeta(child), provenance), ) } + + // The chart rendered, but a non-optional valuesFrom that could not be resolved + // offline means the children were produced with incomplete values — surface + // that so the shift-left gate is honest about its coverage. + result.Degradations = append(result.Degradations, unresolvedValueRefs(&helmRelease, index)...) +} + +// unresolvedValueRefs returns a DegradationPartialValues for each valuesFrom +// reference on the HelmRelease whose value the offline render could not obtain, +// so the rendered output can diverge from what Flux applies with the real value +// present. It mirrors helm-controller's own tolerance rule: "a not found error +// for the values reference is ignored [when optional], but any ValuesKey, +// TargetPath or transient error will still result in a reconciliation failure". +// So only an ABSENT optional referent is silent (typically cluster-managed, or +// defined in another Flux Kustomization); a referent that IS in the stream but +// lacks the requested valuesKey fails in-cluster even when marked optional — a +// `valuesKey` typo is exactly that case — and is always reported. +func unresolvedValueRefs(helmRelease *helmv2.HelmRelease, sources SourceIndex) []Degradation { + key := helmRelease.Namespace + "/" + helmRelease.Name + + var degradations []Degradation + + for index := range helmRelease.Spec.ValuesFrom { + ref := helmRelease.Spec.ValuesFrom[index] + + data, referentFound := lookupValuesSource(ref, helmRelease.Namespace, sources) + if !referentFound { + // Flux ignores a not-found optional referent; a missing required one + // means the chart rendered with incomplete values. + if ref.Optional { + continue + } + + degradations = append( + degradations, + valuesDegradation(key, ref, DegradationPartialValues), + ) + + continue + } + + // The referent is in the stream, so a missing valuesKey is a hard + // reconciliation failure in Flux regardless of `optional`. + if _, ok := data[ref.GetValuesKey()]; !ok { + degradations = append( + degradations, + valuesDegradation(key, ref, DegradationMissingValuesKey), + ) + } + } + + return degradations +} + +// valuesDegradation builds the degradation entry naming the valuesFrom reference +// whose value could not be obtained offline, under the given kind. +func valuesDegradation(key string, ref meta.ValuesReference, kind DegradationKind) Degradation { + return Degradation{ + HelmRelease: key, + Kind: kind, + Reason: fmt.Sprintf("%s %q (key %q)", ref.Kind, ref.Name, ref.GetValuesKey()), + } } // isSilentDegradation reports whether a degradation is expected in normal repos @@ -370,9 +461,10 @@ func extractStringData(doc []byte, secret bool) map[string]string { maps.Copy(out, obj.StringData) - if len(out) == 0 { - return nil - } - + // An EMPTY (but non-nil) map is returned deliberately for a referent that + // carries no data: the object still exists in the stream, and callers must be + // able to tell "present but has no such key" (a Flux reconciliation failure, + // even for an optional reference) from "absent" (which optional forgives). + // Only a parse failure yields nil, meaning "not a usable referent at all". return out } diff --git a/pkg/svc/gitops/render/render_test.go b/pkg/svc/gitops/render/render_test.go index 92f9b3dda..583f66884 100644 --- a/pkg/svc/gitops/render/render_test.go +++ b/pkg/svc/gitops/render/render_test.go @@ -250,3 +250,160 @@ metadata: func joinDocsString(docs ...string) string { return strings.Join(docs, "\n---\n") } + +// helmReleaseValuesFromYAML references a ConfigMap "app-config" via valuesFrom. +// Whether that ConfigMap is in the stream determines if the offline render is +// full-fidelity. +const ( + helmReleaseValuesFromYAML = `apiVersion: helm.toolkit.fluxcd.io/v2 +kind: HelmRelease +metadata: + name: podinfo + namespace: flux-system +spec: + chartRef: + kind: OCIRepository + name: podinfo + valuesFrom: + - kind: ConfigMap + name: app-config` + + helmReleaseValuesFromOptionalYAML = helmReleaseValuesFromYAML + ` + optional: true` + + // helmReleaseValuesFromOptionalMissingKeyYAML marks the reference optional but + // requests a valuesKey the referenced ConfigMap does not carry (a typo of + // "values.yaml"). helm-controller forgives only a NOT-FOUND optional referent — + // "any ValuesKey, TargetPath or transient error will still result in a + // reconciliation failure" — so this must still be reported. + helmReleaseValuesFromOptionalMissingKeyYAML = helmReleaseValuesFromYAML + ` + optional: true + valuesKey: valuse.yaml` + + appConfigYAML = `apiVersion: v1 +kind: ConfigMap +metadata: + name: app-config + namespace: flux-system +data: + values.yaml: | + replicaCount: 3` + + // appConfigEmptyYAML is present in the stream but carries no data at all. It + // must still count as a PRESENT referent: Flux forgives only a missing object, + // so an optional reference to this one still fails reconciliation on the + // absent valuesKey. + appConfigEmptyYAML = `apiVersion: v1 +kind: ConfigMap +metadata: + name: app-config + namespace: flux-system` +) + +// valuesFromDegradationCase is one valuesFrom resolution scenario for +// TestExpandDegradesOnUnresolvableValuesFrom. +type valuesFromDegradationCase struct { + name string + stream []byte + wantDegraded bool + wantKind render.DegradationKind +} + +// valuesFromDegradationCases enumerates how each valuesFrom resolution outcome +// must be reported. It mirrors helm-controller's tolerance rule: `optional` +// forgives only a NOT-FOUND referent, so a referent that is present — whether it +// lacks the requested key or carries no data at all — still fails reconciliation +// and must be reported, under its own kind so the warning names the real cause. +func valuesFromDegradationCases() []valuesFromDegradationCase { + return []valuesFromDegradationCase{ + { + name: "non-optional valuesFrom absent from the stream degrades", + stream: joinDocs(helmReleaseValuesFromYAML), + wantDegraded: true, + wantKind: render.DegradationPartialValues, + }, + { + name: "optional valuesFrom absent from the stream is tolerated", + stream: joinDocs(helmReleaseValuesFromOptionalYAML), + wantDegraded: false, + }, + { + name: "optional valuesFrom present but missing its valuesKey degrades", + stream: joinDocs(helmReleaseValuesFromOptionalMissingKeyYAML, appConfigYAML), + wantDegraded: true, + wantKind: render.DegradationMissingValuesKey, + }, + { + name: "optional valuesFrom present but empty degrades", + stream: joinDocs(helmReleaseValuesFromOptionalYAML, appConfigEmptyYAML), + wantDegraded: true, + wantKind: render.DegradationMissingValuesKey, + }, + { + name: "optional valuesFrom resolvable from the stream does not degrade", + stream: joinDocs(helmReleaseValuesFromOptionalYAML, appConfigYAML), + wantDegraded: false, + }, + { + name: "resolvable valuesFrom does not degrade", + stream: joinDocs(helmReleaseValuesFromYAML, appConfigYAML), + wantDegraded: false, + }, + } +} + +// TestExpandDegradesOnUnresolvableValuesFrom pins the render-fidelity honesty +// contract: a HelmRelease whose valuesFrom ConfigMap/Secret cannot supply its +// value offline still renders, but records a degradation so the shift-left gate +// does not silently under-report its coverage. +func TestExpandDegradesOnUnresolvableValuesFrom(t *testing.T) { + t.Parallel() + + resolver := fakeResolver{ + render: func(_ *helmv2.HelmRelease, _ render.SourceIndex) (string, error) { + return renderedDeployment, nil + }, + } + + for _, testCase := range valuesFromDegradationCases() { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + result, err := render.Expand( + context.Background(), + testCase.stream, + render.Options{Resolver: resolver}, + ) + require.NoError(t, err) + assertValuesFromDegradation(t, result, testCase.wantDegraded, testCase.wantKind) + }) + } +} + +// assertValuesFromDegradation checks that the chart rendered and that a partial- +// values degradation is present exactly when wantDegraded is true. +func assertValuesFromDegradation( + t *testing.T, + result render.Result, + wantDegraded bool, + wantKind render.DegradationKind, +) { + t.Helper() + + // The chart renders regardless of values coverage. + _, hasDeployment := findDoc(result.Documents, "Deployment") + require.True(t, hasDeployment, "chart should render regardless of values coverage") + + if !wantDegraded { + assert.Empty(t, result.Degradations) + + return + } + + require.Len(t, result.Degradations, 1) + degradation := result.Degradations[0] + assert.Equal(t, "flux-system/podinfo", degradation.HelmRelease) + assert.Equal(t, wantKind, degradation.Kind) + assert.False(t, degradation.Silent, "an unresolvable valuesFrom should warn") + assert.Contains(t, degradation.Reason, "app-config") +} diff --git a/pkg/svc/gitops/render/resolver.go b/pkg/svc/gitops/render/resolver.go index 3bdf1cdc5..1e600c8d5 100644 --- a/pkg/svc/gitops/render/resolver.go +++ b/pkg/svc/gitops/render/resolver.go @@ -302,6 +302,53 @@ func buildValues(helmRelease *helmv2.HelmRelease, sources SourceIndex) (string, return string(raw), nil } +// lookupValuesRef resolves a valuesFrom reference's raw value from the in-stream +// ConfigMap/Secret index, returning it and whether it was found. It is the single +// source of truth for "can this reference be resolved offline?", shared by +// applyValuesFrom (which merges the value) and unresolvedValueRefs (which reports +// when it cannot be resolved). +func lookupValuesRef( + ref meta.ValuesReference, + namespace string, + sources SourceIndex, +) (string, bool) { + data, found := lookupValuesSource(ref, namespace, sources) + if !found { + return "", false + } + + raw, ok := data[ref.GetValuesKey()] + + return raw, ok +} + +// lookupValuesSource resolves the referenced ConfigMap/Secret itself from the +// in-stream index, returning its data and whether the *referent* is present — +// distinct from whether the requested valuesKey exists inside it. Flux forgives +// only a not-found optional referent: per the helm-controller API, "a not found +// error for the values reference is ignored, but any ValuesKey, TargetPath or +// transient error will still result in a reconciliation failure". Reporting the +// two cases apart is what lets unresolvedValueRefs stay silent for a genuinely +// absent optional referent while still flagging a present one whose key is missing. +func lookupValuesSource( + ref meta.ValuesReference, + namespace string, + sources SourceIndex, +) (map[string]string, bool) { + switch ref.Kind { + case "ConfigMap": + data, ok := sources.ConfigMaps[namespace+"/"+ref.Name] + + return data, ok + case "Secret": + data, ok := sources.Secrets[namespace+"/"+ref.Name] + + return data, ok + default: + return nil, false + } +} + // applyValuesFrom merges one valuesFrom reference into values, resolving it from // the in-repo ConfigMap/Secret index. References to objects not present in the // stream (typically cluster-managed) are skipped. A reference with a targetPath @@ -313,18 +360,7 @@ func applyValuesFrom( namespace string, sources SourceIndex, ) { - var data map[string]string - - switch ref.Kind { - case "ConfigMap": - data = sources.ConfigMaps[namespace+"/"+ref.Name] - case "Secret": - data = sources.Secrets[namespace+"/"+ref.Name] - default: - return - } - - raw, ok := data[ref.GetValuesKey()] + raw, ok := lookupValuesRef(ref, namespace, sources) if !ok { return }