Repository navigation
Feature: krmignore refactor - #4829
Catalin-Stratulat-Ericsson wants to merge 20 commits into
Conversation
Signed-off-by: Catalin-Stratulat-Ericsson <catalin.stratulat@ericsson.com>
… in Kptfile status Signed-off-by: Catalin-Stratulat-Ericsson <catalin.stratulat@ericsson.com>
Signed-off-by: Catalin-Stratulat-Ericsson <catalin.stratulat@ericsson.com>
Signed-off-by: Catalin-Stratulat-Ericsson <catalin.stratulat@ericsson.com>
Signed-off-by: Catalin-Stratulat-Ericsson <catalin.stratulat@ericsson.com>
Signed-off-by: Catalin-Stratulat-Ericsson <catalin.stratulat@ericsson.com>
Signed-off-by: Catalin-Stratulat-Ericsson <catalin.stratulat@ericsson.com>
Signed-off-by: Catalin-Stratulat-Ericsson <catalin.stratulat@ericsson.com>
Signed-off-by: Catalin-Stratulat-Ericsson <catalin.stratulat@ericsson.com>
✅ Deploy Preview for kptdocs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved file-content loss paths and inconsistent exclusion and validation behavior prevent safe approval.
9 open issues
What changed in this PR
This PR centralizes non-KRM validation across kpt commands to improve diagnostics for packages containing files such as Helm values, addressing #4790 and the proposal in #4824.
Changes:
- Adds shared validation and pre-scanning, plus an opt-in annotation for automatic exclusion.
- Integrates scanning into function and live commands, records render skips, and classifies update inputs by content.
- Adds regression tests and documents command behavior and annotation scope.
| File | Description |
|---|---|
thirdparty/kyaml/runfn/runfn.go |
Adds eval pre-scanning and file skipping. |
thirdparty/kyaml/runfn/runfn_test.go |
Makes the function fixture valid KRM. |
thirdparty/cmdconfig/commands/cmdsource/cmdsource.go |
Adds source pre-scanning and exclusions. |
thirdparty/cmdconfig/commands/cmdsource/cmdsource_test.go |
Updates fixtures and tests non-KRM handling. |
pkg/live/rgpath.go |
Passes file exclusions to manifest reading. |
pkg/live/load.go |
Adds context-aware live pre-scanning. |
pkg/live/load_test.go |
Tests live loading and exclusion behavior. |
pkg/lib/update/update_test.go |
Updates non-KRM raw-copy expectations. |
pkg/lib/update/resource-merge.go |
Classifies merge inputs by content. |
pkg/lib/update/resource-merge_test.go |
Tests non-KRM values-file updates. |
pkg/lib/update/pruninglocalpackagereader.go |
Filters non-KRM merge inputs. |
pkg/lib/krmvalidate/prescan.go |
Implements scanning, decisions, and skip helpers. |
pkg/lib/krmvalidate/prescan_test.go |
Tests partitioning and consolidated diagnostics. |
pkg/lib/krmvalidate/iskrm.go |
Defines shared KRM validation and exceptions. |
pkg/lib/krmvalidate/iskrm_test.go |
Tests validation rules and exceptions. |
pkg/lib/kptops/render_executor.go |
Integrates scanning and records ignored files. |
pkg/lib/kptops/render_executor_test.go |
Tests skipped-file status messages. |
pkg/lib/kptops/autoignore.go |
Exposes render-facing scan helpers. |
pkg/lib/kptops/autoignore_test.go |
Tests render blocking, exclusions, and scope. |
documentation/content/en/reference/cli/pkg/update/_index.md |
Describes non-KRM update handling. |
documentation/content/en/reference/cli/live/status/_index.md |
Documents validation and auto-ignore. |
documentation/content/en/reference/cli/live/migrate/_index.md |
Documents scanning without annotation support. |
documentation/content/en/reference/cli/live/init/_index.md |
Documents validation and auto-ignore. |
documentation/content/en/reference/cli/live/destroy/_index.md |
Documents validation before cluster changes. |
documentation/content/en/reference/cli/live/apply/_index.md |
Documents validation before cluster changes. |
documentation/content/en/reference/cli/fn/source/_index.md |
Documents source validation and exclusions. |
documentation/content/en/reference/cli/fn/render/_index.md |
Documents scanning and skipped-file status. |
documentation/content/en/reference/cli/fn/eval/_index.md |
Documents validation before function execution. |
documentation/content/en/reference/cli/alpha/live/plan/_index.md |
Documents validation and auto-ignore. |
documentation/content/en/reference/annotations/auto-ignore-non-krm/_index.md |
Adds annotation behavior, scope, and examples. |
documentation/content/en/reference/annotations/_index.md |
Links the new annotation reference. |
documentation/content/en/book/03-packages/_index.md |
Explains manual and automatic exclusions. |
commands/live/status/fake-loader.go |
Passes context to live loading. |
commands/live/status/cmdstatus.go |
Passes context to live loading. |
commands/live/migrate/migratecmd.go |
Adds pre-scanning before CRD installation. |
commands/live/migrate/migratecmd_test.go |
Tests blocking and manual exclusions. |
commands/live/init/cmdliveinit.go |
Adds scanning before namespace discovery. |
commands/live/init/cmdliveinit_test.go |
Adds non-KRM initialization cases. |
commands/live/destroy/cmddestroy.go |
Passes context to live loading. |
commands/live/apply/cmdapply.go |
Passes context to live loading. |
commands/alpha/live/plan/command.go |
Passes context to live loading. |
Review effort: Balanced
…les containing offending documents' Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Catalin-Stratulat-Ericsson <catalin.stratulat@ericsson.com>
003a749 to
5896630
Compare
… the root package) Signed-off-by: Catalin-Stratulat-Ericsson <catalin.stratulat@ericsson.com>
…ring out-of-place renders) Signed-off-by: Catalin-Stratulat-Ericsson <catalin.stratulat@ericsson.com>
…en classifying parsed files) Signed-off-by: Catalin-Stratulat-Ericsson <catalin.stratulat@ericsson.com>
…consistent across merge paths) Signed-off-by: Catalin-Stratulat-Ericsson <catalin.stratulat@ericsson.com>
… without inheritance) Signed-off-by: Catalin-Stratulat-Ericsson <catalin.stratulat@ericsson.com>
… discovery and attribution) Signed-off-by: Catalin-Stratulat-Ericsson <catalin.stratulat@ericsson.com>
… during destination writes) Signed-off-by: Catalin-Stratulat-Ericsson <catalin.stratulat@ericsson.com>
…on input and output checks) Signed-off-by: Catalin-Stratulat-Ericsson <catalin.stratulat@ericsson.com>
Signed-off-by: Catalin-Stratulat-Ericsson <catalin.stratulat@ericsson.com>
Signed-off-by: Catalin-Stratulat-Ericsson <catalin.stratulat@ericsson.com>
|
|
will address sonarqube report on return |
Nikshay1
left a comment
Nikshay1
left a comment
There was a problem hiding this comment.
hey @Catalin-Stratulat-Ericsson, I went through this and tried a bunch of small packages on the merge base and on this branch, and for some of them also on this branch with main merged in. left a few things inline, each with what I ran. the first few change things for people who don't set the annotation, so those are probably worth looking at first. happy to share the scripts too!
| if err != nil { | ||
| return false, errors.E(op, errors.IO, err) | ||
| } | ||
| nodes, err := kio.FromBytes(content) |
There was a problem hiding this comment.
I think this one can quietly delete a file during pkg update. I had a kind: List file with no top-level apiVersion, just items: with one ConfigMap. it was the same in upstream and local, and only another file changed upstream:
kind: List
items:
- apiVersion: v1
kind: ConfigMap
metadata:
name: abase: Updated 1 package(s). list.yaml is still there
PR: Updated 1 package(s). list.yaml is gone, no message
I think the two readers disagree about this file. kio.FromBytes here unwraps the List, so the file counts as KRM. the merge reader keeps the wrapper, so PruningLocalPackageReader drops it for having no apiVersion. the destination writer still tracks the file, and since nothing in the merged output comes from it, the writer deletes it. the non-KRM copy pass doesn't bring it back because this check still says it's KRM. happens with main merged in too.
| } | ||
| return false, nil | ||
| for _, n := range nodes { | ||
| if krmvalidate.IsKRM(n) != nil { |
There was a problem hiding this comment.
IsKRM needs metadata.name, so a Job that uses generateName now goes down the non-KRM path and stops getting the 3-way merge. upstream changed backoffLimit from 1 to 5, and local changed parallelism from 1 to 3:
base: backoffLimit: 5, parallelism: 3 both changes kept
PR: backoffLimit: 1, parallelism: 3 upstream change lost, no warning
it seems the file is treated as locally modified, so it's kept as is and the upstream side never comes in. on base the same Job goes through the 3-way merge fine. maybe pkg update could use a looser check here than the one for function input?
|
|
||
| namespace, err := config.FindNamespace(c.Factory.ToRawKubeConfigLoader(), c.Pkg.UniquePath.String()) | ||
| // Pre-scan for non-KRM files before config.FindNamespace() reads them. | ||
| scanResult, err := krmvalidate.ScanFlatAndDecide(filesys.FileSystemOrOnDisk{}, c.Pkg.UniquePath.String(), pr.Printf) |
There was a problem hiding this comment.
this runs before the namespace step, so --namespace doesn't get you past non-KRM files anymore. before, FindNamespace returned early when the namespace was given, without reading the package, and the old error message told people to use that flag:
$ kpt live init --namespace bar pkg # pkg has a JSON6902 bare-list patch.yaml
base: initializing "resourcegroup.yaml" data (namespace: bar)...success
PR: Error: input resource list must contain only KRM resources (1 file(s)):
patch.yaml: resource must have `apiVersion`
a malformed yaml file fails the same way, with non-krm pre-scan: ... MalformedYAMLError. this is also the bare-list case from #4795, so it hits that path too. maybe skip the pre-scan when the namespace is set, or only warn there?
| } | ||
|
|
||
| // Non-KRM pre-scan, before any pipeline starts. See autoignore.go. | ||
| preScanResult, preScanErr := PreScanNonKRM(e.FileSystem, e.PkgPath, pr.Printf) |
There was a problem hiding this comment.
I think the scan here covers the whole package, so files a function would never see now block it. before, SelectInput ran first and the function's input check only looked at the selected resources:
# Kptfile mutator with selectors: [{kind: ConfigMap}], plus a values.yaml
base: [PASS] "..." / Successfully executed 1 function(s) in 1 package(s).
PR: Error: input resource list must contain only KRM resources (1 file(s)):
values.yaml: resource must have `apiVersion`
fn eval --match-kind ConfigMap does the same thing through thirdparty/kyaml/runfn/runfn.go:130. maybe the check could run after SelectInput?
| return nil, kptfilev1.Inventory{}, err | ||
| } | ||
| pr := printer.FromContextOrDie(ctx) | ||
| scanResult, err := krmvalidate.ScanFlatAndDecide(filesys.FileSystemOrOnDisk{}, absPkgPath, pr.Printf) |
There was a problem hiding this comment.
two cases where the scan here rejects things the live reader below never complained about:
- the scan reads
*.jsonbecause it usesMatchAllKRM, butResourceGroupPathManifestReaderonly reads*.yamland*.yml. so an unrelatedpackage.jsonnow breaks thelivecommands:
base: live init succeeds, Load returns 1 object
PR: package.json: resource must have `apiVersion`
filterLocalConfigused to drop aconfig.kubernetes.io/local-config: "true"object without a name, and now the scan rejects it first:
base: live init succeeds, Load returns 1 object
PR: local.yaml: resource must have `metadata.name`
maybe the scan could use the same glob as the real reader and skip local-config objects?
| } | ||
| found = append(found, nonKRM...) | ||
|
|
||
| subpkgs, subErr := p.DirectSubpackages() |
There was a problem hiding this comment.
the scan and the live reader seem to see .krmignore differently. the live reader reads the whole tree from the root, so the root .krmignore covers subpackage files too. here each subpackage gets read on its own, and DirectSubpackages doesn't look at .krmignore, so it also walks into ignored dirs. I ran three cases through live.Load:
root .krmignore has values.yaml, file is at sub/values.yaml
base: OK, 2 objects
PR: blocked on values.yaml
root .krmignore has testdata/, which holds a fixture Kptfile and a values.yaml
base: OK, and live init, fn eval and fn source all work
PR: all four blocked on values.yaml
sub/.krmignore has values.yaml
base: unknown resource types: //
PR: unknown resource types: //
the first two used to work. the last one still ends in the same unknown resource types: // as base, because the scan honours the subpackage .krmignore but the flat live reader doesn't. maybe the flat callers could scan with the same reader setup, and only use the per-package walk to find out whose Kptfile opted in?
| var b strings.Builder | ||
| fmt.Fprintf(&b, "input resource list must contain only KRM resources (%d file(s)):", len(blocking)) | ||
| for _, f := range blocking { | ||
| fmt.Fprintf(&b, "\n %s: %s", f.RelPath, f.Reason) |
There was a problem hiding this comment.
small one. f.RelPath is relative to the owning subpackage, so the message gets confusing when two subpackages have the same file name:
# bare dir with two subpackages (each has a Kptfile): apps/app1/values.yaml, apps/app2/values.yaml
base: apps/app1/values.yaml: resource must have `apiVersion`
PR: values.yaml: resource must have `apiVersion`
values.yaml: resource must have `apiVersion`
the hint also says "this package's Kptfile" without saying which one. printing the path relative to the scan root would probably fix both.
| // FindNamespace re-reads the package itself with no skip hook, | ||
| // so it would hit the files we just excused. Use the loader's | ||
| // namespace directly instead, same as FindNamespace's own fallback. | ||
| namespace, _, err = c.Factory.ToRawKubeConfigLoader().Namespace() |
There was a problem hiding this comment.
when the annotation skips at least one file, this skips namespace detection and uses the kubeconfig namespace, so you get a different result than with .krmignore. all resources were in foo, plus a values.yaml:
values.yaml listed in .krmignore namespace: foo
annotation instead namespace: default
could FindNamespace, or a kpt copy of it, take the skip func instead?
|
|
||
| The non-KRM file is skipped in memory for that invocation only. | ||
| Nothing is written to disk because of this annotation: the file | ||
| itself is left completely untouched, and no `.krmignore` file is ever |
There was a problem hiding this comment.
I think live init still writes to files the annotation skipped. it happens in the attribution pass at the end of Run, at cmdliveinit.go:247. that line isn't part of the diff, so I'm commenting here. my package was annotated and had one file with a StorageBucket and a non-KRM doc, so the scan skipped the whole file:
[info] ignoring 1 non-KRM file(s) ... : mixed.yaml
diff after live init:
> annotations:
> cnrm.cloud.google.com/blueprint: 'krm-live'
this line says skipped files are left completely untouched. it still happens with main merged in. #4796 only stops writing files whose attribution didn't change, and here adding the CNRM annotation does change it. passing the skip func to the Attributor would probably do it.
| } else { | ||
| resources, resErr = p.LocalResources() | ||
| } | ||
| if resErr != nil { |
There was a problem hiding this comment.
when a file doesn't parse, the reader errors out here before Split sees it. so the annotation can't skip it, and the error stops at the first bad file:
# annotated package, b1.yaml is "not: [valid"
PR: Error: non-krm pre-scan: pkg.readResources: ... b1.yaml: MalformedYAMLError ...
# not annotated, b1.yaml and b2.yaml both broken, plus a values.yaml
PR: only b1.yaml is reported
base fails on these files too, except for live init --namespace from the other comment, so this part isn't a regression. but the docs say the annotation skips "any non-KRM files", so maybe count parse errors as non-KRM, or say in the docs that the file has to parse as YAML first?
Description
This PR comprised of 3 main changes but see #4824 for the full write-up of the problem
pkg/lib/krmvalidate), and uses it to give every affected command the same behavior for non-KRM files:kpt.dev/auto-ignore-non-krmKptfile annotation that skips non-KRM files automatically, in memory only (nothing written to disk, no.krmignoregenerated), for every affected command exceptlive migrate.The PR is split up as follows
Commands changed:
fn render,fn eval,fn source,pkg update,live apply,live status,live destroy,live init,live migrate,alpha live plan.pkg tree/pkg catare intentionally unchanged (read-only display tools, already tolerant by design).live migrategets the pre-scan but not the annotation: its legacy ConfigMap-based inventory reader has no way to skip a file in memory, so.krmignoreis the only way to exclude a non-KRM file for that command.Documentation is updated to match: a new annotation reference page, and a short note on each affected command's reference page and in the packages guide's
.krmignoresection.Motivation
Fixes #4790: along with other issues raised of
.krmignoreregarding, consistency throughout the code, bugs related to its performance and of course ease of use.AI Disclosure
Fixes
Fixes #4790
Testing
krmvalidate,kptops/render executor,runfn,cmdsource,pkg/lib/update,pkg/live(shared byapply/status/destroy/plan),live/init,live/migrate).go build ./...andgo vet ./...pass with no errors.go test ./...passes with no failures.livecommand was manually verified end-to-end against a real cluster (kind), confirming: without the annotation, a non-KRM file blocks the command before any cluster mutation; with the annotation, the file is skipped and left untouched on disk with no.krmignorewritten;live migratecontinues to block regardless of the annotation, as designed.fn render/fn eval/fn sourcewere manually verified against the compiled binary for the same before/after behavior.hugobuild (no brokenrelrefs) and the repo'smisspellcheck (make verifyindocumentation/).