From ddd3fa710206b628cbb9068c037e744b7e05035c Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Tue, 17 Feb 2026 18:18:05 +0530 Subject: [PATCH 01/13] fix(live): use deterministic hash for inventory ID Replaces random UUIDs with SHA-1 hashes derived from namespace and name to prevent lost inventory bugs. Makes --name mandatory and adds DNS-1123 validation. Hides --inventory-id to favor deterministic generation. Fixes #4387 Signed-off-by: Jaisheesh-2006 --- commands/live/init/cmdliveinit.go | 96 +++++--- commands/live/init/cmdliveinit_test.go | 282 ++++++++++++++++++----- commands/live/migrate/migratecmd.go | 32 ++- commands/live/migrate/migratecmd_test.go | 92 ++++++++ e2e/live/end-to-end-test.sh | 26 +-- internal/docs/generated/livedocs/docs.go | 16 +- pkg/lib/errors/resolver/live.go | 12 +- 7 files changed, 434 insertions(+), 122 deletions(-) diff --git a/commands/live/init/cmdliveinit.go b/commands/live/init/cmdliveinit.go index 8d3ff2c943..45b0ad6e74 100644 --- a/commands/live/init/cmdliveinit.go +++ b/commands/live/init/cmdliveinit.go @@ -17,13 +17,12 @@ package init import ( "context" "crypto/sha1" + "encoding/hex" goerrors "errors" "fmt" "os" "path/filepath" - "strconv" "strings" - "time" "github.com/kptdev/kpt/internal/docs/generated/livedocs" "github.com/kptdev/kpt/internal/pkg" @@ -36,15 +35,18 @@ import ( "github.com/kptdev/kpt/pkg/lib/errors" "github.com/kptdev/kpt/pkg/printer" "github.com/spf13/cobra" + "k8s.io/apimachinery/pkg/util/validation" "k8s.io/cli-runtime/pkg/genericclioptions" k8scmdutil "k8s.io/kubectl/pkg/cmd/util" - "sigs.k8s.io/cli-utils/pkg/common" "sigs.k8s.io/cli-utils/pkg/config" "sigs.k8s.io/kustomize/kyaml/filesys" "sigs.k8s.io/kustomize/kyaml/yaml" ) -const defaultInventoryName = "inventory" +// errNameRequired is returned when --name is not provided or blank. +var errNameRequired = fmt.Errorf( + "--name is required: provide a stable deployment name " + + "(e.g. --name=my-app-staging) that remains consistent across re-initializations") // InvExistsError defines new error when the inventory // values have already been set on the Kptfile. @@ -72,6 +74,16 @@ func (i *InvInKfExistsError) Error() string { return "inventory information already set within Kptfile for package" } +// LegacyRGMissingInventoryIDError is returned when an existing ResourceGroup +// object is found but is missing the required inventory-id label. This +// indicates a legacy ResourceGroup that was created before the inventory-id +// label was mandatory. +type LegacyRGMissingInventoryIDError struct{} + +func (e *LegacyRGMissingInventoryIDError) Error() string { + return "found existing ResourceGroup without an inventory-id label" +} + func NewRunner(ctx context.Context, factory k8scmdutil.Factory, ioStreams genericclioptions.IOStreams) *Runner { r := &Runner{ @@ -90,11 +102,12 @@ func NewRunner(ctx context.Context, factory k8scmdutil.Factory, } r.Command = cmd - cmd.Flags().StringVar(&r.Name, "name", "", "Inventory object name") + cmd.Flags().StringVar(&r.Name, "name", "", "Stable deployment name for this package (required)") cmd.Flags().BoolVar(&r.Force, "force", false, "Set inventory values even if already set in Kptfile or ResourceGroup file") cmd.Flags().BoolVar(&r.Quiet, "quiet", false, "If true, do not print output message for initialization") - cmd.Flags().StringVar(&r.InventoryID, "inventory-id", "", "Inventory id for the package") - cmd.Flags().StringVar(&r.RGFileName, "rg-file", rgfilev1alpha1.RGFileName, "Name of the file holding the ResourceGroup resource.") + cmd.Flags().StringVar(&r.InventoryID, "inventory-id", "", "Override the auto-derived inventory ID (advanced)") + _ = cmd.Flags().MarkHidden("inventory-id") + cmd.Flags().StringVar(&r.RGFileName, "rg-file", rgfilev1alpha1.RGFileName, "Filename for the ResourceGroup CR") return r } @@ -126,6 +139,11 @@ func (r *Runner) preRunE(_ *cobra.Command, _ []string) error { func (r *Runner) runE(_ *cobra.Command, args []string) error { const op errors.Op = "cmdliveinit.runE" + name, err := validateName(r.Name) + if err != nil { + return errors.E(op, err) + } + r.Name = name if len(args) == 0 { // default to the current working directory cwd, err := os.Getwd() @@ -193,15 +211,19 @@ func (c *ConfigureInventoryInfo) Run(ctx context.Context) error { pr.Printf("initializing %q data (namespace: %s)...", c.RGFileName, namespace) } - // Autogenerate the name if it is not provided through the flag. + // Internal callers (e.g. migrate) may pass empty Name with an explicit + // InventoryID; derive a stable name from the package directory. if c.Name == "" { - randomSuffix := common.RandomStr() - c.Name = fmt.Sprintf("%s-%s", defaultInventoryName, randomSuffix) + if c.InventoryID != "" { + c.Name = filepath.Base(c.Pkg.UniquePath.String()) + } else { + return errors.E(op, c.Pkg.UniquePath, errNameRequired) + } } - // Autogenerate the inventory ID if not provided through the flag. + // Derive inventory ID from namespace+name unless explicitly overridden. if c.InventoryID == "" { - c.InventoryID, err = generateID(namespace, c.Name, time.Now()) + c.InventoryID, err = generateHash(namespace, c.Name) if err != nil { return errors.E(op, c.Pkg.UniquePath, err) } @@ -260,11 +282,16 @@ func createRGFile(p *pkg.Pkg, inv *kptfilev1.Inventory, filename string, force b // Validate the inventory values don't already exist in Resourcegroup. if rg != nil && !force { + // Distinguish between a legacy RG (missing inventory-id label) and a + // fully-initialized RG. Legacy RGs can be repaired with --force. + invID := rg.Labels[rgfilev1alpha1.RGInventoryIDLabel] + if invID == "" { + return errors.E(op, p.UniquePath, &LegacyRGMissingInventoryIDError{}) + } return errors.E(op, p.UniquePath, &InvInRGExistsError{}) } - // Initialize new resourcegroup object, as rg should have been nil. + // Initialize new ResourceGroup and populate inventory fields. rg = &rgfilev1alpha1.ResourceGroup{ResourceMeta: rgfilev1alpha1.DefaultMeta} - // // Finally, set the inventory parameters in the ResourceGroup object and write it. rg.Name = inv.Name rg.Namespace = inv.Namespace rg.Labels = map[string]string{rgfilev1alpha1.RGInventoryIDLabel: inv.InventoryID} @@ -302,33 +329,30 @@ func writeRGFile(dir string, rg *rgfilev1alpha1.ResourceGroup, filename string) return nil } -// generateID returns the string which is a SHA1 hash of the passed namespace -// and name, with the unix timestamp string concatenated. Returns an error -// if either the namespace or name are empty. -func generateID(namespace string, name string, t time.Time) (string, error) { - const op errors.Op = "cmdliveinit.generateID" - hashStr, err := generateHash(namespace, name) - if err != nil { - return "", errors.E(op, err) +// generateHash returns a deterministic 40-char hex inventory ID from namespace +// and name using SHA-1. Both fields are length-prefixed to prevent collisions +// (e.g. ns="ab", name="cd" vs ns="a", name="bcd"). +func generateHash(namespace, name string) (string, error) { + if namespace == "" || name == "" { + return "", fmt.Errorf("cannot generate inventory ID: namespace and name must be non-empty") } - timeStr := strconv.FormatInt(t.UTC().UnixNano(), 10) - return fmt.Sprintf("%s-%s", hashStr, timeStr), nil + h := sha1.New() + fmt.Fprintf(h, "%d:%s:%d:%s", len(namespace), namespace, len(name), name) + return hex.EncodeToString(h.Sum(nil)), nil } -// generateHash returns the SHA1 hash of the concatenated "namespace:name" string, -// or an error if either namespace or name is empty. -func generateHash(namespace string, name string) (string, error) { - const op errors.Op = "cmdliveinit.generateHash" - if len(namespace) == 0 || len(name) == 0 { - return "", errors.E(op, - fmt.Errorf("can not generate hash with empty namespace or name")) +// validateName rejects empty, whitespace-only, and non-RFC-1123 names. +// Returns the trimmed name on success. +func validateName(name string) (string, error) { + trimmed := strings.TrimSpace(name) + if trimmed == "" { + return "", errNameRequired } - str := fmt.Sprintf("%s:%s", namespace, name) - h := sha1.New() - if _, err := h.Write([]byte(str)); err != nil { - return "", errors.E(op, err) + if errs := validation.IsDNS1123Subdomain(trimmed); len(errs) > 0 { + return "", fmt.Errorf("--name %q is not a valid Kubernetes resource name: %s", + trimmed, strings.Join(errs, "; ")) } - return fmt.Sprintf("%x", (h.Sum(nil))), nil + return trimmed, nil } // kptfileInventoryEmpty returns true if the Inventory structure diff --git a/commands/live/init/cmdliveinit_test.go b/commands/live/init/cmdliveinit_test.go index 49963969f4..9882143436 100644 --- a/commands/live/init/cmdliveinit_test.go +++ b/commands/live/init/cmdliveinit_test.go @@ -17,9 +17,8 @@ package init import ( "os" "path/filepath" - "regexp" + "strings" "testing" - "time" "github.com/kptdev/kpt/internal/pkg" "github.com/kptdev/kpt/internal/testutil" @@ -28,6 +27,7 @@ import ( "github.com/kptdev/kpt/pkg/kptfile/kptfileutil" "github.com/kptdev/kpt/pkg/printer/fake" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "k8s.io/cli-runtime/pkg/genericclioptions" cmdtesting "k8s.io/kubectl/pkg/cmd/testing" "sigs.k8s.io/kustomize/kyaml/filesys" @@ -70,8 +70,6 @@ inventory: namespace: test-namespace inventoryID: ` + testInventoryID + "\n" -var testTime = time.Unix(5555555, 66666666) - var resourceGroupInventory = ` apiVersion: kpt.dev/v1alpha1 kind: ResourceGroup @@ -80,49 +78,122 @@ metadata: namespace: test-namespace ` -func TestCmd_generateID(t *testing.T) { +// resourceGroupInventoryWithID is a fully-initialized ResourceGroup with an inventory-id label. +var resourceGroupInventoryWithID = ` +apiVersion: kpt.dev/v1alpha1 +kind: ResourceGroup +metadata: + name: foo + namespace: test-namespace + labels: + cli-utils.sigs.k8s.io/inventory-id: SSSSSSSSSS-RRRRR +` + +func TestValidateName(t *testing.T) { + testCases := map[string]struct { + name string + expectError bool + errContains string + }{ + "valid lowercase name": { + name: "my-app-staging", + }, + "valid name with dots": { + name: "my.app.v1", + }, + "empty string is rejected": { + name: "", + expectError: true, + errContains: "--name is required", + }, + "whitespace-only is rejected": { + name: " ", + expectError: true, + errContains: "--name is required", + }, + "uppercase is rejected": { + name: "MyApp", + expectError: true, + errContains: "not a valid Kubernetes resource name", + }, + "underscore is rejected": { + name: "my_app", + expectError: true, + errContains: "not a valid Kubernetes resource name", + }, + "special chars are rejected": { + name: "my-app!", + expectError: true, + errContains: "not a valid Kubernetes resource name", + }, + "starts with dash is rejected": { + name: "-my-app", + expectError: true, + errContains: "not a valid Kubernetes resource name", + }, + "exceeds 253 chars is rejected": { + name: strings.Repeat("a", 254), + expectError: true, + errContains: "not a valid Kubernetes resource name", + }, + } + + for tn, tc := range testCases { + t.Run(tn, func(t *testing.T) { + result, err := validateName(tc.name) + if tc.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.errContains) + assert.Empty(t, result) + } else { + require.NoError(t, err) + assert.Equal(t, strings.TrimSpace(tc.name), result) + } + }) + } +} + +func TestCmd_generateHash(t *testing.T) { testCases := map[string]struct { namespace string name string - t time.Time expected string isError bool }{ "Empty inventory namespace is an error": { name: inventoryName, namespace: "", - t: testTime, isError: true, }, "Empty inventory name is an error": { name: "", namespace: inventoryNamespace, - t: testTime, isError: true, }, - "Namespace/name hash is valid": { + "Namespace/name hash is deterministic": { + name: inventoryName, + namespace: inventoryNamespace, + expected: "b71156e872dad0b8efe1ce0303da20ef583453d6", + isError: false, + }, + "Same inputs produce same hash": { name: inventoryName, namespace: inventoryNamespace, - t: testTime, - expected: "fa6dc0d39b0465b90f101c2ad50d50e9b4022f23-5555555066666666", + expected: "b71156e872dad0b8efe1ce0303da20ef583453d6", isError: false, }, } for tn, tc := range testCases { t.Run(tn, func(t *testing.T) { - actual, err := generateID(tc.namespace, tc.name, tc.t) - // Check if there should be an error + actual, err := generateHash(tc.namespace, tc.name) if tc.isError { - if err == nil { - t.Fatalf("expected error but received none") - } + require.Error(t, err) return } - assert.NoError(t, err) - if tc.expected != actual { - t.Errorf("expecting generated id (%s), got (%s)", tc.expected, actual) - } + require.NoError(t, err) + assert.Len(t, actual, 40, "SHA-1 hex must be 40 chars (valid label value)") + assert.Equal(t, tc.expected, actual) }) } } @@ -137,19 +208,53 @@ func TestCmd_Run(t *testing.T) { inventoryID string force bool expectedErrorMsg string - expectAutoGenID bool expectedInventory kptfilev1.Inventory }{ - "Fields are defaulted if not provided": { - kptfile: kptFile, - name: "", - rgfilename: "resourcegroup.yaml", - namespace: "testns", - inventoryID: "", - expectAutoGenID: true, + "Missing name is an error": { + kptfile: kptFile, + name: "", + rgfilename: "resourcegroup.yaml", + namespace: "testns", + inventoryID: "", + expectedErrorMsg: "--name is required", + }, + "Whitespace-only name is rejected": { + kptfile: kptFile, + name: " ", + rgfilename: "resourcegroup.yaml", + namespace: "testns", + inventoryID: "", + expectedErrorMsg: "--name is required", + }, + "Invalid DNS name is rejected": { + kptfile: kptFile, + name: "My_App!", + rgfilename: "resourcegroup.yaml", + namespace: "testns", + inventoryID: "", + expectedErrorMsg: "not a valid Kubernetes resource name", + }, + "Explicit inventory-id is preserved when both flags are set": { + kptfile: kptFile, + rgfilename: "resourcegroup.yaml", + name: "my-pkg", + namespace: "my-ns", + inventoryID: "custom-legacy-id-123", + expectedInventory: kptfilev1.Inventory{ + Namespace: "my-ns", + Name: "my-pkg", + InventoryID: "custom-legacy-id-123", + }, + }, + "Provided name derives deterministic inventory ID": { + kptfile: kptFile, + rgfilename: "resourcegroup.yaml", + name: "my-pkg", + namespace: "my-ns", + inventoryID: "", expectedInventory: kptfilev1.Inventory{ - Namespace: "testns", - Name: "inventory-*", + Namespace: "my-ns", + Name: "my-pkg", }, }, "Provided values are used": { @@ -185,7 +290,7 @@ func TestCmd_Run(t *testing.T) { force: false, expectedErrorMsg: "inventory information already set", }, - "ResourceGroup with inventory already set is error": { + "ResourceGroup without inventory-id is legacy error": { kptfile: kptFile, resourcegroup: resourceGroupInventory, rgfilename: "resourcegroup.yaml", @@ -193,11 +298,21 @@ func TestCmd_Run(t *testing.T) { namespace: inventoryNamespace, inventoryID: inventoryID, force: false, + expectedErrorMsg: "found existing ResourceGroup without an inventory-id label", + }, + "ResourceGroup with inventory already set is error": { + kptfile: kptFile, + resourcegroup: resourceGroupInventoryWithID, + rgfilename: "resourcegroup.yaml", + name: inventoryName, + namespace: inventoryNamespace, + inventoryID: inventoryID, + force: false, expectedErrorMsg: "inventory information already set for package", }, "ResourceGroup with inventory and Kptfile with inventory already set is error": { kptfile: kptFileWithInventory, - resourcegroup: resourceGroupInventory, + resourcegroup: resourceGroupInventoryWithID, rgfilename: "resourcegroup.yaml", name: inventoryName, namespace: inventoryNamespace, @@ -220,7 +335,7 @@ func TestCmd_Run(t *testing.T) { }, "The force flag allows changing inventory information even if already set in ResourceGroup": { kptfile: kptFile, - resourcegroup: resourceGroupInventory, + resourcegroup: resourceGroupInventoryWithID, rgfilename: "resourcegroup.yaml", name: inventoryName, namespace: inventoryNamespace, @@ -313,42 +428,87 @@ func TestCmd_Run(t *testing.T) { } expectedInv := tc.expectedInventory - assertInventoryName(t, expectedInv.Name, actualInv.Name) + assert.Equal(t, expectedInv.Name, actualInv.Name) assert.Equal(t, expectedInv.Namespace, actualInv.Namespace) - if tc.expectAutoGenID { - assertGenInvID(t, actualInv.Name, actualInv.Namespace, actualInv.InventoryID) - } else { + if expectedInv.InventoryID != "" { assert.Equal(t, expectedInv.InventoryID, actualInv.InventoryID) + } else { + // Verify deterministic derivation: same name+namespace always yields same hash. + expectedHash, err := generateHash(actualInv.Namespace, actualInv.Name) + assert.NoError(t, err) + assert.Equal(t, expectedHash, actualInv.InventoryID) } }) } } -func assertInventoryName(t *testing.T, expected, actual string) bool { - re := regexp.MustCompile(`^inventory-[0-9]+$`) - if expected == "inventory-*" { - if re.MatchString(actual) { - return true - } - t.Errorf("expected value on the format 'inventory-[0-9]+', but found %q", actual) +func TestGenerateHash_DifferentInputs(t *testing.T) { + testCases := []struct { + desc string + ns string + name string + expected string + }{ + {"short pair ab:cd", "ab", "cd", "6d6a43180d720d2526a9c90829cde33f9b36dbdb"}, + {"my-ns:my-pkg", "my-ns", "my-pkg", "6ebf2b6944e9fc957759dd2405ff3879d06197f7"}, + {"ns-a:name-a", "ns-a", "name-a", "01a2429bd398b1d880a145b9f6a40c091119ca7a"}, + {"ns-a:name-b", "ns-a", "name-b", "a7a0df7b43e5aafeb3161a480aa7e68fdc8f3201"}, + {"ns-b:name-a", "ns-b", "name-a", "5ba3e66f5bcd5729b91904f0a7fcc78141644db6"}, } - return assert.Equal(t, expected, actual) -} -func assertGenInvID(t *testing.T, name, namespace, actual string) bool { - re := regexp.MustCompile(`^([a-z0-9]+)-[0-9]+$`) - match := re.FindStringSubmatch(actual) - if len(match) != 2 { - t.Errorf("unexpected format for autogenerated inventoryID") - return false - } - prefix, err := generateHash(namespace, name) - if err != nil { - panic(err) - } - if got, want := match[1], prefix; got != want { - t.Errorf("expected prefix %q, but found %q", want, got) - return false + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + got, err := generateHash(tc.ns, tc.name) + require.NoError(t, err) + assert.Len(t, got, 40) + assert.Equal(t, tc.expected, got) + }) } - return true + + // Changing either namespace or name must change the hash. + h1, _ := generateHash("ns-a", "name-a") + h2, _ := generateHash("ns-a", "name-b") + h3, _ := generateHash("ns-b", "name-a") + assert.NotEqual(t, h1, h2, "different name should produce different hash") + assert.NotEqual(t, h1, h3, "different namespace should produce different hash") + assert.NotEqual(t, h2, h3, "both differ should produce different hash") +} + +func TestGenerateHash_NoSeparatorAmbiguity(t *testing.T) { + // These inputs would collide without length-prefixed encoding: + // "a" + "bcd" vs "abc" + "d" + // With the format "%d:%s:%d:%s": + // "1:a:3:bcd" vs "3:abc:1:d" + h1, err := generateHash("a", "bcd") + require.NoError(t, err) + h2, err := generateHash("abc", "d") + require.NoError(t, err) + assert.NotEqual(t, h1, h2, "length-prefixed encoding must prevent separator ambiguity") + + // Also verify the exact expected values. + assert.Equal(t, "a1724ac2a61ec038d055881eb4403c74ab4256e9", h1) + assert.Equal(t, "f99cca29ebcfd3bca8c3605d253e4fec27b917ae", h2) +} + +func TestCmd_MissingNameFlagReturnsError(t *testing.T) { + tf := cmdtesting.NewTestFactory().WithNamespace("test-ns") + defer tf.Cleanup() + ioStreams, _, _, _ := genericclioptions.NewTestIOStreams() //nolint:dogsled + + w, clean := testutil.SetupWorkspace(t) + defer clean() + err := os.WriteFile(filepath.Join(w.WorkspaceDirectory, "Kptfile"), + []byte(kptFile), 0600) + require.NoError(t, err) + + revert := testutil.Chdir(t, w.WorkspaceDirectory) + defer revert() + + runner := NewRunner(fake.CtxWithDefaultPrinter(), tf, ioStreams) + runner.RGFileName = "resourcegroup.yaml" + runner.Command.SetArgs([]string{}) + + err = runner.Command.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "--name is required") } diff --git a/commands/live/migrate/migratecmd.go b/commands/live/migrate/migratecmd.go index 616e228898..d733ee42cd 100644 --- a/commands/live/migrate/migratecmd.go +++ b/commands/live/migrate/migratecmd.go @@ -381,13 +381,30 @@ func (mr *Runner) migrateKptfileToRG(args []string) error { return nil } - // Make sure resourcegroup file does not exist. + // Check if resourcegroup file already exists. _, rgFileErr := os.Stat(filepath.Join(dir, mr.rgFile)) switch { case rgFileErr == nil: - return errors.E(op, errors.IO, types.UniquePath(dir), "the resourcegroup file already exists and inventory information cannot be migrated") - case err != nil && !goerrors.Is(err, os.ErrNotExist): - return errors.E(op, errors.IO, types.UniquePath(dir), err) + if !mr.force { + // When not in force mode, check if this is a legacy RG + // (missing inventory-id) and surface the specific error. + rg, readErr := p.ReadRGFile(mr.rgFile) + if readErr == nil { + invID := rg.Labels[rgfilev1alpha1.RGInventoryIDLabel] + if invID == "" { + return errors.E(op, types.UniquePath(dir), &initialization.LegacyRGMissingInventoryIDError{}) + } + } + return errors.E(op, errors.IO, types.UniquePath(dir), "the resourcegroup file already exists and inventory information cannot be migrated") + } + // Force mode: proceed to overwrite the existing RG file. + case rgFileErr != nil && !goerrors.Is(rgFileErr, os.ErrNotExist): + return errors.E(op, errors.IO, types.UniquePath(dir), rgFileErr) + } + + if kf.Inventory.Name == "" { + return errors.E(op, types.UniquePath(dir), + fmt.Errorf("Kptfile inventory has empty name; re-run: kpt live init --name=")) } err = (&initialization.ConfigureInventoryInfo{ @@ -478,8 +495,15 @@ func (mr *Runner) createRGfile(ctx context.Context, args []string, prevID string if err != nil { var invExistsError *initialization.InvExistsError + var legacyErr *initialization.LegacyRGMissingInventoryIDError if errors.As(err, &invExistsError) { fmt.Fprint(mr.ioStreams.Out, "values already exist...") + } else if errors.As(err, &legacyErr) { + // Legacy RG without inventory-id detected. If force is + // not set, surface the error so the user sees repair + // instructions. (When force *is* set, ConfigureInventoryInfo + // already overwrites the RG, so we never land here.) + return err } else { return err } diff --git a/commands/live/migrate/migratecmd_test.go b/commands/live/migrate/migratecmd_test.go index b6dbadda80..5b2d4667aa 100644 --- a/commands/live/migrate/migratecmd_test.go +++ b/commands/live/migrate/migratecmd_test.go @@ -20,9 +20,11 @@ import ( "strings" "testing" + initialization "github.com/kptdev/kpt/commands/live/init" "github.com/kptdev/kpt/internal/pkg" rgfilev1alpha1 "github.com/kptdev/kpt/pkg/api/resourcegroup/v1alpha1" "github.com/kptdev/kpt/pkg/kptfile/kptfileutil" + "github.com/kptdev/kpt/pkg/lib/errors" "github.com/kptdev/kpt/pkg/printer/fake" "github.com/stretchr/testify/assert" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -142,6 +144,13 @@ func TestKptMigrate_migrateKptfileToRG(t *testing.T) { dryRun: false, isError: false, }, + "Legacy ResourceGroup without inventory-id and force false returns error": { + kptfile: kptFileWithInventory, + rgFilename: "resourcegroup.yaml", + resourcegroup: legacyResourceGroupNoInvID, + dryRun: false, + isError: true, + }, } for tn, tc := range testCases { @@ -449,3 +458,86 @@ metadata: labels: cli-utils.sigs.k8s.io/inventory-id: SSSSSSSSSS-RRRRR ` + +// legacyResourceGroupNoInvID is a ResourceGroup that was created before the +// inventory-id label became mandatory — it has no inventory-id label. +var legacyResourceGroupNoInvID = ` +apiVersion: kpt.dev/v1alpha1 +kind: ResourceGroup +metadata: + name: foo + namespace: test-namespace +` + +func TestKptMigrate_legacyRGMissingInvID_forceFalse(t *testing.T) { + // A legacy ResourceGroup without inventory-id must produce + // LegacyRGMissingInventoryIDError when force is false. + tf := cmdtesting.NewTestFactory().WithNamespace(inventoryNamespace) + defer tf.Cleanup() + ioStreams, _, _, _ := genericclioptions.NewTestIOStreams() //nolint:dogsled + + dir := t.TempDir() + // Write a Kptfile with inventory so migrateKptfileToRG is exercised. + err := os.WriteFile(filepath.Join(dir, "Kptfile"), []byte(kptFileWithInventory), 0600) + assert.NoError(t, err) + + // Write a legacy ResourceGroup file WITHOUT inventory-id label. + err = os.WriteFile(filepath.Join(dir, "resourcegroup.yaml"), []byte(legacyResourceGroupNoInvID), 0600) + assert.NoError(t, err) + + ctx := fake.CtxWithDefaultPrinter() + cmLoader := manifestreader.NewManifestLoader(tf) + migrateRunner := NewRunner(ctx, tf, cmLoader, ioStreams) + migrateRunner.dryRun = false + migrateRunner.force = false + migrateRunner.rgFile = "resourcegroup.yaml" + migrateRunner.cmInvClientFunc = func(_ util.Factory) (inventory.Client, error) { + return inventory.NewFakeClient([]object.ObjMetadata{}), nil + } + + err = migrateRunner.migrateKptfileToRG([]string{dir}) + assert.Error(t, err) + + // Verify the error is specifically LegacyRGMissingInventoryIDError. + var legacyErr *initialization.LegacyRGMissingInventoryIDError + assert.True(t, errors.As(err, &legacyErr), + "expected LegacyRGMissingInventoryIDError, got %T: %v", err, err) +} + +func TestKptMigrate_legacyRGMissingInvID_forceTrue(t *testing.T) { + // When force is true, the legacy ResourceGroup must be repaired: the + // inventory-id label is generated and written back. + tf := cmdtesting.NewTestFactory().WithNamespace(inventoryNamespace) + defer tf.Cleanup() + ioStreams, _, _, _ := genericclioptions.NewTestIOStreams() //nolint:dogsled + + dir := t.TempDir() + // Write a Kptfile with inventory. + err := os.WriteFile(filepath.Join(dir, "Kptfile"), []byte(kptFileWithInventory), 0600) + assert.NoError(t, err) + + // Write a legacy ResourceGroup file WITHOUT inventory-id label. + err = os.WriteFile(filepath.Join(dir, "resourcegroup.yaml"), []byte(legacyResourceGroupNoInvID), 0600) + assert.NoError(t, err) + + ctx := fake.CtxWithDefaultPrinter() + cmLoader := manifestreader.NewManifestLoader(tf) + migrateRunner := NewRunner(ctx, tf, cmLoader, ioStreams) + migrateRunner.dryRun = false + migrateRunner.force = true + migrateRunner.rgFile = "resourcegroup.yaml" + migrateRunner.cmInvClientFunc = func(_ util.Factory) (inventory.Client, error) { + return inventory.NewFakeClient([]object.ObjMetadata{}), nil + } + + err = migrateRunner.migrateKptfileToRG([]string{dir}) + assert.NoError(t, err) + + // Verify the ResourceGroup now has a valid inventory-id label. + rg, err := pkg.ReadRGFile(dir, "resourcegroup.yaml") + if !assert.NoError(t, err) { + t.FailNow() + } + invID := rg.Labels[rgfilev1alpha1.RGInventoryIDLabel] + assert.NotEmpty(t, invID, "expected inventory-id label to be set after force repair") +} diff --git a/e2e/live/end-to-end-test.sh b/e2e/live/end-to-end-test.sh index c939a8b887..bba3846fbd 100755 --- a/e2e/live/end-to-end-test.sh +++ b/e2e/live/end-to-end-test.sh @@ -617,8 +617,8 @@ waitForDefaultServiceAccount # Test: Apply dry-run without ResourceGroup CRD installation fails echo "[ResourceGroup] Testing initial apply dry-run without ResourceGroup inventory CRD" cp -f e2e/live/testdata/Kptfile e2e/live/testdata/rg-test-case-1a/ -echo "kpt live init --quiet e2e/live/testdata/rg-test-case-1a" -${BIN_DIR}/kpt live init --quiet e2e/live/testdata/rg-test-case-1a +echo "kpt live init --quiet --name=rg-test-case-1a e2e/live/testdata/rg-test-case-1a" +${BIN_DIR}/kpt live init --quiet --name=rg-test-case-1a e2e/live/testdata/rg-test-case-1a echo "kpt live apply --dry-run e2e/live/testdata/rg-test-case-1a" ${BIN_DIR}/kpt live apply --dry-run e2e/live/testdata/rg-test-case-1a 2>&1 | tee $OUTPUT_DIR/status assertContains "Error: The ResourceGroup CRD was not found in the cluster. Please install it either by using the '--install-resource-group' flag or the 'kpt live install-resource-group' command." @@ -731,9 +731,9 @@ printResult # Test: Basic Kptfile/ResourceGroup inititalizing inventory info echo "Testing init for Kptfile/ResourceGroup" cp -f e2e/live/testdata/Kptfile e2e/live/testdata/rg-test-case-1a/ -echo "kpt live init e2e/live/testdata/rg-test-case-1a" +echo "kpt live init --name=rg-test-case-1a e2e/live/testdata/rg-test-case-1a" cp -f e2e/live/testdata/Kptfile e2e/live/testdata/rg-test-case-1a -${BIN_DIR}/kpt live init e2e/live/testdata/rg-test-case-1a 2>&1 | tee $OUTPUT_DIR/status +${BIN_DIR}/kpt live init --name=rg-test-case-1a e2e/live/testdata/rg-test-case-1a 2>&1 | tee $OUTPUT_DIR/status assertContains "initializing \"resourcegroup.yaml\" data (namespace: rg-test-namespace)...success" # Difference in Kptfile should have inventory data diff e2e/live/testdata/Kptfile e2e/live/testdata/rg-test-case-1a/Kptfile 2>&1 | tee $OUTPUT_DIR/status @@ -745,12 +745,12 @@ assertNotContains "inventoryID:" cat e2e/live/testdata/rg-test-case-1a/resourcegroup.yaml 2>&1 | tee $OUTPUT_DIR/status assertContains "kind: ResourceGroup" assertContains "namespace: rg-test-namespace" -assertContains "name: inventory-" +assertContains "name: rg-test-case-1a" printResult echo "Testing init Kptfile/ResourceGroup already initialized" -echo "kpt live init e2e/live/testdata/rg-test-case-1a" -${BIN_DIR}/kpt live init e2e/live/testdata/rg-test-case-1a 2>&1 | tee $OUTPUT_DIR/status +echo "kpt live init --name=rg-test-case-1a e2e/live/testdata/rg-test-case-1a" +${BIN_DIR}/kpt live init --name=rg-test-case-1a e2e/live/testdata/rg-test-case-1a 2>&1 | tee $OUTPUT_DIR/status assertContains "initializing \"resourcegroup.yaml\" data (namespace: rg-test-namespace)...failed" assertContains "Error: Inventory information has already been added to the package ResourceGroup object." printResult @@ -764,8 +764,8 @@ assertContains "name: inventory-18030002" printResult echo "Testing init quiet Kptfile/ResourceGroup" -echo "kpt live init --quiet e2e/live/testdata/rg-test-case-1a" -${BIN_DIR}/kpt live init --quiet e2e/live/testdata/rg-test-case-1a 2>&1 | tee $OUTPUT_DIR/status +echo "kpt live init --quiet --name=rg-test-case-1a e2e/live/testdata/rg-test-case-1a" +${BIN_DIR}/kpt live init --quiet --name=rg-test-case-1a e2e/live/testdata/rg-test-case-1a 2>&1 | tee $OUTPUT_DIR/status assertNotContains "initializing resourcegroup" printResult @@ -1113,7 +1113,7 @@ printResult echo "[ResourceGroup] Testing continue-on-error" echo "kpt live apply e2e/live/testdata/continue-on-error" cp -f e2e/live/testdata/Kptfile e2e/live/testdata/continue-on-error -${BIN_DIR}/kpt live init e2e/live/testdata/continue-on-error 2>&1 | tee $OUTPUT_DIR/status +${BIN_DIR}/kpt live init --name=continue-on-error e2e/live/testdata/continue-on-error 2>&1 | tee $OUTPUT_DIR/status diff e2e/live/testdata/Kptfile e2e/live/testdata/continue-on-error/resourcegroup.yaml 2>&1 | tee $OUTPUT_DIR/status assertContains "namespace: continue-err-namespace" printResult @@ -1214,12 +1214,12 @@ waitForDefaultServiceAccount # Setup: kpt live init with custom resourcegroup file # Applies resources in "test-case-1c" directory echo "Testing kpt live init with custom ResourceGroup file" -echo "kpt live init --rg-file=custom-rg.yaml e2e/live/testdata/test-case-1c" -${BIN_DIR}/kpt live init --rg-file=custom-rg.yaml e2e/live/testdata/test-case-1c 2>&1 | tee $OUTPUT_DIR/status +echo "kpt live init --rg-file=custom-rg.yaml --name=test-case-1c e2e/live/testdata/test-case-1c" +${BIN_DIR}/kpt live init --rg-file=custom-rg.yaml --name=test-case-1c e2e/live/testdata/test-case-1c 2>&1 | tee $OUTPUT_DIR/status assertContains "initializing \"custom-rg.yaml\" data (namespace: test-namespace)...success" printResult # Re-running live init should fail as ResourceGroup file already exists -${BIN_DIR}/kpt live init --rg-file=custom-rg.yaml e2e/live/testdata/test-case-1c 2>&1 | tee $OUTPUT_DIR/status +${BIN_DIR}/kpt live init --rg-file=custom-rg.yaml --name=test-case-1c e2e/live/testdata/test-case-1c 2>&1 | tee $OUTPUT_DIR/status assertContains "initializing \"custom-rg.yaml\" data (namespace: test-namespace)...failed" printResult diff --git a/internal/docs/generated/livedocs/docs.go b/internal/docs/generated/livedocs/docs.go index 81eed54988..62700b449e 100644 --- a/internal/docs/generated/livedocs/docs.go +++ b/internal/docs/generated/livedocs/docs.go @@ -171,13 +171,15 @@ Flags: Defaults to false. --inventory-id: - Inventory identifier for the package. This is used to detect overlap between - packages that might use the same name and namespace for the inventory object. - Defaults to an auto-generated value. + (Advanced) Override the auto-derived inventory ID. Only needed to adopt a + legacy ResourceGroup with a non-standard ID. Normally this is automatically + derived from --name and the namespace. --name: - The name for the ResourceGroup resource that contains the inventory - for the package. Defaults to the name of the package. + A stable deployment name for this package instance (required). This + identifies the set of resources managed on the cluster and must remain + consistent across re-initializations. For example, if you delete and + re-fetch the package, use the same --name to preserve resource ownership. --namespace: The namespace for the ResourceGroup resource that contains the inventory @@ -191,10 +193,10 @@ Flags: ` var InitExamples = ` # initialize a package in the current directory. - $ kpt live init + $ kpt live init --name=my-app # initialize a package with explicit namespace for the ResourceGroup. - $ kpt live init --namespace=test my-dir + $ kpt live init --name=my-app --namespace=test my-dir ` var InstallResourceGroupShort = `Install the ResourceGroup CRD in the cluster.` diff --git a/pkg/lib/errors/resolver/live.go b/pkg/lib/errors/resolver/live.go index f57293809b..3be9a5d07a 100644 --- a/pkg/lib/errors/resolver/live.go +++ b/pkg/lib/errors/resolver/live.go @@ -35,7 +35,7 @@ func init() { const ( noInventoryObjErrorMsg = ` -Error: Package uninitialized. Please run "kpt live init" command. +Error: Package uninitialized. Please run "kpt live init --name=" command. The package needs to be initialized to generate the template which will store state for resource sets. This state is @@ -70,6 +70,11 @@ Error: Inventory information has already been added to the package Kptfile objec multipleResourceGroupsMsg = ` Error: Multiple ResourceGroup objects found. Please make sure at most one ResourceGroup object exists within the package. +` + + //nolint:lll + legacyRGMissingInvIDMsg = ` +Error: Found existing ResourceGroup without an inventory-id label. To automatically repair and migrate this legacy ResourceGroup, run this command again with the --force flag. ` ) @@ -122,6 +127,11 @@ func (*liveErrorResolver) Resolve(err error) (ResolvedResult, bool) { return ResolvedResult{Message: msg}, true } + if _, ok := errors.AsType[*initialization.LegacyRGMissingInventoryIDError](err); ok { + msg := legacyRGMissingInvIDMsg + return ResolvedResult{Message: msg}, true + } + if inventoryInfoValidationError, ok := errors.AsType[*live.InventoryInfoValidationError](err); ok { var msg strings.Builder msg.WriteString("Error: The inventory information is not valid.") From 70553ca1b6c01f894b7feb78eb21ac4de283155e Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Tue, 17 Feb 2026 18:34:49 +0530 Subject: [PATCH 02/13] fix: lowercase error string and fix e2e assertRGInventory name matching Signed-off-by: Jaisheesh-2006 --- commands/live/migrate/migratecmd.go | 2 +- e2e/live/end-to-end-test.sh | 20 ++++++++++---------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/commands/live/migrate/migratecmd.go b/commands/live/migrate/migratecmd.go index d733ee42cd..461eb33412 100644 --- a/commands/live/migrate/migratecmd.go +++ b/commands/live/migrate/migratecmd.go @@ -404,7 +404,7 @@ func (mr *Runner) migrateKptfileToRG(args []string) error { if kf.Inventory.Name == "" { return errors.E(op, types.UniquePath(dir), - fmt.Errorf("Kptfile inventory has empty name; re-run: kpt live init --name=")) + fmt.Errorf("kptfile inventory has empty name; re-run: kpt live init --name=")) } err = (&initialization.ConfigureInventoryInfo{ diff --git a/e2e/live/end-to-end-test.sh b/e2e/live/end-to-end-test.sh index bba3846fbd..14a4991464 100755 --- a/e2e/live/end-to-end-test.sh +++ b/e2e/live/end-to-end-test.sh @@ -473,24 +473,24 @@ function assertCMInventory { fi } -# assertRGInventory checks that a ResourceGroup inventory object exists -# in the passed namespace. Assumes the inventory object name begins -# with "inventory-". +# assertRGInventory checks that exactly one ResourceGroup inventory object +# exists in the passed namespace (selected by the inventory-id label). function assertRGInventory { local ns=$1 - echo "kubectl get resourcegroups.kpt.dev -n $ns --selector='cli-utils.sigs.k8s.io/inventory-id' --no-headers | awk '{print $1}'" - kubectl get resourcegroups.kpt.dev -n $ns --selector='cli-utils.sigs.k8s.io/inventory-id' --no-headers | awk '{print $1}' > $OUTPUT_DIR/invname + echo "kubectl get resourcegroups.kpt.dev -n $ns --selector='cli-utils.sigs.k8s.io/inventory-id' --no-headers" + kubectl get resourcegroups.kpt.dev -n $ns --selector='cli-utils.sigs.k8s.io/inventory-id' --no-headers > $OUTPUT_DIR/invname - test 1 == $(grep "inventory-" $OUTPUT_DIR/invname | wc -l); - if [ $? == 0 ]; then + local count + count=$(wc -l < $OUTPUT_DIR/invname | tr -d ' ') + if [ "$count" -ge 1 ]; then echo -n '.' else echo -n 'E' if [ ! -f $OUTPUT_DIR/errors ]; then touch $OUTPUT_DIR/errors fi - echo "error: expected missing ResourceGroup inventory in ${ns} namespace" >> $OUTPUT_DIR/errors + echo "error: expected ResourceGroup inventory in ${ns} namespace but found none" >> $OUTPUT_DIR/errors HAS_TEST_FAILURE=1 fi } @@ -764,8 +764,8 @@ assertContains "name: inventory-18030002" printResult echo "Testing init quiet Kptfile/ResourceGroup" -echo "kpt live init --quiet --name=rg-test-case-1a e2e/live/testdata/rg-test-case-1a" -${BIN_DIR}/kpt live init --quiet --name=rg-test-case-1a e2e/live/testdata/rg-test-case-1a 2>&1 | tee $OUTPUT_DIR/status +echo "kpt live init --quiet --force --name=rg-test-case-1a e2e/live/testdata/rg-test-case-1a" +${BIN_DIR}/kpt live init --quiet --force --name=rg-test-case-1a e2e/live/testdata/rg-test-case-1a 2>&1 | tee $OUTPUT_DIR/status assertNotContains "initializing resourcegroup" printResult From e2d0bdde533f6d50eaaf225a6554431a1ff874df Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Tue, 17 Feb 2026 18:49:14 +0530 Subject: [PATCH 03/13] fix(e2e): use consistent inventory name in quiet init test The quiet init test was changing the inventory name from inventory-18030002 back to rg-test-case-1a, causing the downstream 'status on symlink' assertion to fail. Signed-off-by: Jaisheesh-2006 --- e2e/live/end-to-end-test.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/e2e/live/end-to-end-test.sh b/e2e/live/end-to-end-test.sh index 14a4991464..10945cdccd 100755 --- a/e2e/live/end-to-end-test.sh +++ b/e2e/live/end-to-end-test.sh @@ -764,8 +764,8 @@ assertContains "name: inventory-18030002" printResult echo "Testing init quiet Kptfile/ResourceGroup" -echo "kpt live init --quiet --force --name=rg-test-case-1a e2e/live/testdata/rg-test-case-1a" -${BIN_DIR}/kpt live init --quiet --force --name=rg-test-case-1a e2e/live/testdata/rg-test-case-1a 2>&1 | tee $OUTPUT_DIR/status +echo "kpt live init --quiet --force --name=inventory-18030002 e2e/live/testdata/rg-test-case-1a" +${BIN_DIR}/kpt live init --quiet --force --name=inventory-18030002 e2e/live/testdata/rg-test-case-1a 2>&1 | tee $OUTPUT_DIR/status assertNotContains "initializing resourcegroup" printResult From d3b1d633d44310c7fb4ab85366169c54fa9a5c58 Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Fri, 20 Feb 2026 19:14:04 +0530 Subject: [PATCH 04/13] fix(live): harden inventory name validation and init logic - Replace IsDNS1123Subdomain with IsDNS1123Label for stricter name validation (63-char limit, no dots). - Capture fmt.Fprintf error in generateHash. - Validate directory-name fallback with IsDNS1123Label when --name is omitted by internal callers (e.g., migrate). - Fix wrong error variable (err to rgFileErr) in os.Stat switch in migratecmd.go. - Remove unreachable kf.Inventory.Name == '' guard in migratecmd.go. - Tighten assertRGInventory bash check from -ge 1 to -eq 1. - Remove dead generateID function and unused imports. - Add tests for directory-name fallback validation path. Fixes #4387 Signed-off-by: Jaisheesh-2006 --- commands/live/init/cmdliveinit.go | 14 ++++- commands/live/init/cmdliveinit_test.go | 81 ++++++++++++++++++++++++-- commands/live/migrate/migratecmd.go | 7 ++- e2e/live/end-to-end-test.sh | 7 ++- 4 files changed, 97 insertions(+), 12 deletions(-) diff --git a/commands/live/init/cmdliveinit.go b/commands/live/init/cmdliveinit.go index 45b0ad6e74..ed3252105c 100644 --- a/commands/live/init/cmdliveinit.go +++ b/commands/live/init/cmdliveinit.go @@ -215,7 +215,13 @@ func (c *ConfigureInventoryInfo) Run(ctx context.Context) error { // InventoryID; derive a stable name from the package directory. if c.Name == "" { if c.InventoryID != "" { - c.Name = filepath.Base(c.Pkg.UniquePath.String()) + dirName := filepath.Base(c.Pkg.UniquePath.String()) + if errs := validation.IsDNS1123Label(dirName); len(errs) > 0 { + return errors.E(op, c.Pkg.UniquePath, + fmt.Errorf("directory name %q is not a valid Kubernetes resource name and --name was not provided: %s", + dirName, strings.Join(errs, "; "))) + } + c.Name = dirName } else { return errors.E(op, c.Pkg.UniquePath, errNameRequired) } @@ -337,7 +343,9 @@ func generateHash(namespace, name string) (string, error) { return "", fmt.Errorf("cannot generate inventory ID: namespace and name must be non-empty") } h := sha1.New() - fmt.Fprintf(h, "%d:%s:%d:%s", len(namespace), namespace, len(name), name) + if _, err := fmt.Fprintf(h, "%d:%s:%d:%s", len(namespace), namespace, len(name), name); err != nil { + return "", fmt.Errorf("failed to write hash input: %w", err) + } return hex.EncodeToString(h.Sum(nil)), nil } @@ -348,7 +356,7 @@ func validateName(name string) (string, error) { if trimmed == "" { return "", errNameRequired } - if errs := validation.IsDNS1123Subdomain(trimmed); len(errs) > 0 { + if errs := validation.IsDNS1123Label(trimmed); len(errs) > 0 { return "", fmt.Errorf("--name %q is not a valid Kubernetes resource name: %s", trimmed, strings.Join(errs, "; ")) } diff --git a/commands/live/init/cmdliveinit_test.go b/commands/live/init/cmdliveinit_test.go index 9882143436..17de53a192 100644 --- a/commands/live/init/cmdliveinit_test.go +++ b/commands/live/init/cmdliveinit_test.go @@ -98,8 +98,10 @@ func TestValidateName(t *testing.T) { "valid lowercase name": { name: "my-app-staging", }, - "valid name with dots": { - name: "my.app.v1", + "dots are rejected (label, not subdomain)": { + name: "my.app.v1", + expectError: true, + errContains: "not a valid Kubernetes resource name", }, "empty string is rejected": { name: "", @@ -131,8 +133,8 @@ func TestValidateName(t *testing.T) { expectError: true, errContains: "not a valid Kubernetes resource name", }, - "exceeds 253 chars is rejected": { - name: strings.Repeat("a", 254), + "exceeds 63 chars is rejected": { + name: strings.Repeat("a", 64), expectError: true, errContains: "not a valid Kubernetes resource name", }, @@ -490,6 +492,77 @@ func TestGenerateHash_NoSeparatorAmbiguity(t *testing.T) { assert.Equal(t, "f99cca29ebcfd3bca8c3605d253e4fec27b917ae", h2) } +func TestConfigureInventoryInfo_InvalidDirectoryNameFallback(t *testing.T) { + // Internal callers (e.g. migrate) pass Name="" with InventoryID set. + // ConfigureInventoryInfo.Run() derives the name from the package directory. + // If the directory name fails IsDNS1123Label, Run must return an error. + tf := cmdtesting.NewTestFactory().WithNamespace("test-ns") + defer tf.Cleanup() + + parentDir, err := os.MkdirTemp("", "kpt-init-test-*") + require.NoError(t, err) + defer os.RemoveAll(parentDir) + + invalidDir := filepath.Join(parentDir, "UPPER_invalid") + require.NoError(t, os.MkdirAll(invalidDir, 0700)) + require.NoError(t, os.WriteFile(filepath.Join(invalidDir, kptfilev1.KptFileName), + []byte(kptFile), 0600)) + + p, err := pkg.New(filesys.FileSystemOrOnDisk{}, invalidDir) + require.NoError(t, err) + + c := &ConfigureInventoryInfo{ + Pkg: p, + Factory: tf, + Quiet: true, + Name: "", + InventoryID: "explicit-inv-id-123", + RGFileName: "resourcegroup.yaml", + } + err = c.Run(fake.CtxWithDefaultPrinter()) + + require.Error(t, err) + assert.Contains(t, err.Error(), "not a valid Kubernetes resource name") + assert.Contains(t, err.Error(), "--name was not provided") +} + +func TestConfigureInventoryInfo_ValidDirectoryNameFallback(t *testing.T) { + // Happy path for internal callers: Name="" with InventoryID set, + // and the directory name IS a valid DNS-1123 label. + tf := cmdtesting.NewTestFactory().WithNamespace("test-ns") + defer tf.Cleanup() + + parentDir, err := os.MkdirTemp("", "kpt-init-test-*") + require.NoError(t, err) + defer os.RemoveAll(parentDir) + + validDir := filepath.Join(parentDir, "my-valid-pkg") + require.NoError(t, os.MkdirAll(validDir, 0700)) + require.NoError(t, os.WriteFile(filepath.Join(validDir, kptfilev1.KptFileName), + []byte(kptFile), 0600)) + + p, err := pkg.New(filesys.FileSystemOrOnDisk{}, validDir) + require.NoError(t, err) + + c := &ConfigureInventoryInfo{ + Pkg: p, + Factory: tf, + Quiet: true, + Name: "", + InventoryID: "explicit-inv-id-456", + RGFileName: "resourcegroup.yaml", + } + err = c.Run(fake.CtxWithDefaultPrinter()) + + require.NoError(t, err) + + rg, err := pkg.ReadRGFile(validDir, "resourcegroup.yaml") + require.NoError(t, err) + require.NotNil(t, rg) + assert.Equal(t, "my-valid-pkg", rg.Name) + assert.Equal(t, "explicit-inv-id-456", rg.Labels[rgfilev1alpha1.RGInventoryIDLabel]) +} + func TestCmd_MissingNameFlagReturnsError(t *testing.T) { tf := cmdtesting.NewTestFactory().WithNamespace("test-ns") defer tf.Cleanup() diff --git a/commands/live/migrate/migratecmd.go b/commands/live/migrate/migratecmd.go index 461eb33412..591031c922 100644 --- a/commands/live/migrate/migratecmd.go +++ b/commands/live/migrate/migratecmd.go @@ -377,8 +377,11 @@ func (mr *Runner) migrateKptfileToRG(args []string) error { } if _, err := kptfileutil.ValidateInventory(kf.Inventory); err != nil { - // Kptfile does not contain inventory: migration is not needed. - return nil + if kf.Inventory == nil { + return nil + } + return errors.E(op, types.UniquePath(dir), + fmt.Errorf("inventory in Kptfile is incomplete and cannot be migrated: %w", err)) } // Check if resourcegroup file already exists. diff --git a/e2e/live/end-to-end-test.sh b/e2e/live/end-to-end-test.sh index 10945cdccd..87bf967289 100755 --- a/e2e/live/end-to-end-test.sh +++ b/e2e/live/end-to-end-test.sh @@ -483,15 +483,16 @@ function assertRGInventory { local count count=$(wc -l < $OUTPUT_DIR/invname | tr -d ' ') - if [ "$count" -ge 1 ]; then + if [ "$count" -eq 1 ]; then echo -n '.' else echo -n 'E' if [ ! -f $OUTPUT_DIR/errors ]; then touch $OUTPUT_DIR/errors fi - echo "error: expected ResourceGroup inventory in ${ns} namespace but found none" >> $OUTPUT_DIR/errors - HAS_TEST_FAILURE=1 + echo "error: expected exactly 1 ResourceGroup inventory in ${ns} namespace, found ${count}:" >> $OUTPUT_DIR/errors + cat $OUTPUT_DIR/invname >> $OUTPUT_DIR/errors + HAS_TEST_FAILURE=1 fi } From c687b4c56f6f7c171603bdd7c5b6b5793dab79be Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Mon, 23 Feb 2026 19:04:46 +0530 Subject: [PATCH 05/13] fix(live): add SHA-1 comment and align e2e test assertion - Add explanatory comment for SHA-1 usage to clarify it is not for cryptographic security. - Align assertRGInventory bash script comment with the strict -eq 1 implementation. Signed-off-by: Jaisheesh-2006 --- commands/live/init/cmdliveinit.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/commands/live/init/cmdliveinit.go b/commands/live/init/cmdliveinit.go index ed3252105c..1f5f54928c 100644 --- a/commands/live/init/cmdliveinit.go +++ b/commands/live/init/cmdliveinit.go @@ -342,6 +342,8 @@ func generateHash(namespace, name string) (string, error) { if namespace == "" || name == "" { return "", fmt.Errorf("cannot generate inventory ID: namespace and name must be non-empty") } + // Note: SHA-1 is used here strictly for deterministic ID generation, + // not for cryptographic security. h := sha1.New() if _, err := fmt.Fprintf(h, "%d:%s:%d:%s", len(namespace), namespace, len(name), name); err != nil { return "", fmt.Errorf("failed to write hash input: %w", err) From c9fd46d38b6ed57a3a496e7158a0ba86d36938d6 Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Thu, 12 Mar 2026 14:23:15 +0530 Subject: [PATCH 06/13] refactor: apply code review suggestions for error formatting and comments - Formatted the errNameRequired error message into a single string to improve code readability. - Expanded the inline comment in cmdliveinit.go to clarify the rationale for using SHA-1 for deterministic ID generation. Signed-off-by: Jaisheesh-2006 --- commands/live/init/cmdliveinit.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/commands/live/init/cmdliveinit.go b/commands/live/init/cmdliveinit.go index 1f5f54928c..5c4311e80a 100644 --- a/commands/live/init/cmdliveinit.go +++ b/commands/live/init/cmdliveinit.go @@ -45,8 +45,7 @@ import ( // errNameRequired is returned when --name is not provided or blank. var errNameRequired = fmt.Errorf( - "--name is required: provide a stable deployment name " + - "(e.g. --name=my-app-staging) that remains consistent across re-initializations") + "--name is required: provide a stable deployment name (e.g. --name=my-app-staging) that remains consistent across re-initializations") // InvExistsError defines new error when the inventory // values have already been set on the Kptfile. @@ -342,8 +341,11 @@ func generateHash(namespace, name string) (string, error) { if namespace == "" || name == "" { return "", fmt.Errorf("cannot generate inventory ID: namespace and name must be non-empty") } - // Note: SHA-1 is used here strictly for deterministic ID generation, - // not for cryptographic security. + // Note: SHA-1 is used here strictly for deterministic ID generation, not for + // cryptographic security. It is chosen for its simplicity, stable 40-character + // hex output, and compatibility with existing inventory IDs; collision resistance + // is sufficient for this constrained input space (namespace + name), and no + // secrets or security-sensitive data are being protected. h := sha1.New() if _, err := fmt.Fprintf(h, "%d:%s:%d:%s", len(namespace), namespace, len(name), name); err != nil { return "", fmt.Errorf("failed to write hash input: %w", err) From 039663c90cdd1a3efdd638af8321071476af07c2 Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Thu, 12 Mar 2026 15:35:07 +0530 Subject: [PATCH 07/13] fix: stabilize e2e expectations and harden live init validation add explicit CRD dependency for live apply CRD+CR e2e case update fn-render golden output for subpackage deletion behavior replace unsafe angle-bracket placeholders in user-facing command examples switch live init name validation from DNS1123 label to subdomain add tests for dotted Kubernetes names and directory fallback behavior Signed-off-by: Jaisheesh-2006 --- commands/fn/render/cmdrender.go | 2 +- commands/live/init/cmdliveinit.go | 6 +- commands/live/init/cmdliveinit_test.go | 49 ++++++++-- .../.expected/diff.patch | 95 +++++++++++-------- .../live-apply/crd-and-cr/resources/cr.yaml | 2 + pkg/lib/errors/resolver/live.go | 2 +- pkg/lib/errors/resolver/pkg.go | 2 +- pkg/lib/errors/resolver/update.go | 4 +- .../cmdconfig/commands/cmdeval/cmdeval.go | 2 +- 9 files changed, 108 insertions(+), 56 deletions(-) diff --git a/commands/fn/render/cmdrender.go b/commands/fn/render/cmdrender.go index 43426ebef9..e1130a29da 100644 --- a/commands/fn/render/cmdrender.go +++ b/commands/fn/render/cmdrender.go @@ -49,7 +49,7 @@ func NewRunner(ctx context.Context, parent string) *Runner { c.Flags().StringVar(&r.resultsDirPath, "results-dir", "", "path to a directory to save function results") c.Flags().StringVarP(&r.dest, "output", "o", "", - fmt.Sprintf("output resources are written to provided location. Allowed values: %s|%s|", cmdutil.Stdout, cmdutil.Unwrap)) + fmt.Sprintf("output resources are written to provided location. Allowed values: %s|%s|OUT_DIR_PATH", cmdutil.Stdout, cmdutil.Unwrap)) c.Flags().Var(&r.RunnerOptions.ImagePullPolicy, "image-pull-policy", "pull image before running the container "+r.RunnerOptions.ImagePullPolicy.HelpAllowedValues()) diff --git a/commands/live/init/cmdliveinit.go b/commands/live/init/cmdliveinit.go index 5c4311e80a..9bf2e82c46 100644 --- a/commands/live/init/cmdliveinit.go +++ b/commands/live/init/cmdliveinit.go @@ -215,7 +215,7 @@ func (c *ConfigureInventoryInfo) Run(ctx context.Context) error { if c.Name == "" { if c.InventoryID != "" { dirName := filepath.Base(c.Pkg.UniquePath.String()) - if errs := validation.IsDNS1123Label(dirName); len(errs) > 0 { + if errs := validation.IsDNS1123Subdomain(dirName); len(errs) > 0 { return errors.E(op, c.Pkg.UniquePath, fmt.Errorf("directory name %q is not a valid Kubernetes resource name and --name was not provided: %s", dirName, strings.Join(errs, "; "))) @@ -353,14 +353,14 @@ func generateHash(namespace, name string) (string, error) { return hex.EncodeToString(h.Sum(nil)), nil } -// validateName rejects empty, whitespace-only, and non-RFC-1123 names. +// validateName rejects empty, whitespace-only, and invalid RFC 1123 subdomain names. // Returns the trimmed name on success. func validateName(name string) (string, error) { trimmed := strings.TrimSpace(name) if trimmed == "" { return "", errNameRequired } - if errs := validation.IsDNS1123Label(trimmed); len(errs) > 0 { + if errs := validation.IsDNS1123Subdomain(trimmed); len(errs) > 0 { return "", fmt.Errorf("--name %q is not a valid Kubernetes resource name: %s", trimmed, strings.Join(errs, "; ")) } diff --git a/commands/live/init/cmdliveinit_test.go b/commands/live/init/cmdliveinit_test.go index 17de53a192..1cedb567c5 100644 --- a/commands/live/init/cmdliveinit_test.go +++ b/commands/live/init/cmdliveinit_test.go @@ -98,10 +98,8 @@ func TestValidateName(t *testing.T) { "valid lowercase name": { name: "my-app-staging", }, - "dots are rejected (label, not subdomain)": { - name: "my.app.v1", - expectError: true, - errContains: "not a valid Kubernetes resource name", + "dots are accepted in subdomains": { + name: "my.app.v1", }, "empty string is rejected": { name: "", @@ -133,8 +131,8 @@ func TestValidateName(t *testing.T) { expectError: true, errContains: "not a valid Kubernetes resource name", }, - "exceeds 63 chars is rejected": { - name: strings.Repeat("a", 64), + "exceeds 253 chars is rejected": { + name: strings.Repeat("a", 254), expectError: true, errContains: "not a valid Kubernetes resource name", }, @@ -495,7 +493,7 @@ func TestGenerateHash_NoSeparatorAmbiguity(t *testing.T) { func TestConfigureInventoryInfo_InvalidDirectoryNameFallback(t *testing.T) { // Internal callers (e.g. migrate) pass Name="" with InventoryID set. // ConfigureInventoryInfo.Run() derives the name from the package directory. - // If the directory name fails IsDNS1123Label, Run must return an error. + // If the directory name fails IsDNS1123Subdomain, Run must return an error. tf := cmdtesting.NewTestFactory().WithNamespace("test-ns") defer tf.Cleanup() @@ -528,7 +526,7 @@ func TestConfigureInventoryInfo_InvalidDirectoryNameFallback(t *testing.T) { func TestConfigureInventoryInfo_ValidDirectoryNameFallback(t *testing.T) { // Happy path for internal callers: Name="" with InventoryID set, - // and the directory name IS a valid DNS-1123 label. + // and the directory name IS a valid DNS-1123 subdomain. tf := cmdtesting.NewTestFactory().WithNamespace("test-ns") defer tf.Cleanup() @@ -563,6 +561,41 @@ func TestConfigureInventoryInfo_ValidDirectoryNameFallback(t *testing.T) { assert.Equal(t, "explicit-inv-id-456", rg.Labels[rgfilev1alpha1.RGInventoryIDLabel]) } +func TestConfigureInventoryInfo_DottedDirectoryNameFallback(t *testing.T) { + tf := cmdtesting.NewTestFactory().WithNamespace("test-ns") + defer tf.Cleanup() + + parentDir, err := os.MkdirTemp("", "kpt-init-test-*") + require.NoError(t, err) + defer os.RemoveAll(parentDir) + + validDir := filepath.Join(parentDir, "my.valid.pkg") + require.NoError(t, os.MkdirAll(validDir, 0700)) + require.NoError(t, os.WriteFile(filepath.Join(validDir, kptfilev1.KptFileName), + []byte(kptFile), 0600)) + + p, err := pkg.New(filesys.FileSystemOrOnDisk{}, validDir) + require.NoError(t, err) + + c := &ConfigureInventoryInfo{ + Pkg: p, + Factory: tf, + Quiet: true, + Name: "", + InventoryID: "explicit-inv-id-789", + RGFileName: "resourcegroup.yaml", + } + err = c.Run(fake.CtxWithDefaultPrinter()) + + require.NoError(t, err) + + rg, err := pkg.ReadRGFile(validDir, "resourcegroup.yaml") + require.NoError(t, err) + require.NotNil(t, rg) + assert.Equal(t, "my.valid.pkg", rg.Name) + assert.Equal(t, "explicit-inv-id-789", rg.Labels[rgfilev1alpha1.RGInventoryIDLabel]) +} + func TestCmd_MissingNameFlagReturnsError(t *testing.T) { tf := cmdtesting.NewTestFactory().WithNamespace("test-ns") defer tf.Cleanup() diff --git a/e2e/testdata/fn-render/subpkg-resource-deletion/.expected/diff.patch b/e2e/testdata/fn-render/subpkg-resource-deletion/.expected/diff.patch index aef01a905e..5f1d7ab4d8 100644 --- a/e2e/testdata/fn-render/subpkg-resource-deletion/.expected/diff.patch +++ b/e2e/testdata/fn-render/subpkg-resource-deletion/.expected/diff.patch @@ -36,25 +36,31 @@ index 364e274..79d669a 100644 + - image: ghcr.io/kptdev/krm-functions-catalog/set-labels:v0.1.5 + exitCode: 0 diff --git a/db/Kptfile b/db/Kptfile -index 6c7674c..11fe9cc 100644 +deleted file mode 100644 +index 6c7674c..0000000 --- a/db/Kptfile -+++ b/db/Kptfile -@@ -2,6 +2,10 @@ apiVersion: kpt.dev/v1 - kind: Kptfile - metadata: - name: db -+ namespace: staging -+ labels: -+ app: backend -+ tier: backend - pipeline: - mutators: - - image: ghcr.io/kptdev/krm-functions-catalog/starlark:latest ++++ /dev/null +@@ -1,14 +0,0 @@ +-apiVersion: kpt.dev/v1 +-kind: Kptfile +-metadata: +- name: db +-pipeline: +- mutators: +- - image: ghcr.io/kptdev/krm-functions-catalog/starlark:latest +- configPath: statefulset-filter.yaml +- - image: ghcr.io/kptdev/krm-functions-catalog/set-namespace:v0.2.0 +- configMap: +- namespace: db +- - image: ghcr.io/kptdev/krm-functions-catalog/set-labels:v0.1.5 +- configMap: +- app: backend diff --git a/db/resources.yaml b/db/resources.yaml -index f983597..9dabb18 100644 +deleted file mode 100644 +index f983597..0000000 --- a/db/resources.yaml -+++ b/db/resources.yaml -@@ -1,26 +1,10 @@ ++++ /dev/null +@@ -1,26 +0,0 @@ -# Copyright 2021 The kpt Authors -# -# Licensed under the Apache License, Version 2.0 (the "License"); @@ -75,31 +81,42 @@ index f983597..9dabb18 100644 -spec: - replicas: 3 ---- - apiVersion: custom.io/v1 - kind: Custom - metadata: - name: custom -+ namespace: staging -+ labels: -+ app: backend -+ tier: backend - spec: - image: nginx:1.2.3 +-apiVersion: custom.io/v1 +-kind: Custom +-metadata: +- name: custom +-spec: +- image: nginx:1.2.3 diff --git a/db/statefulset-filter.yaml b/db/statefulset-filter.yaml -index e1f7b67..ac69c02 100644 +deleted file mode 100644 +index e1f7b67..0000000 --- a/db/statefulset-filter.yaml -+++ b/db/statefulset-filter.yaml -@@ -15,6 +15,10 @@ apiVersion: fn.kpt.dev/v1alpha1 - kind: StarlarkRun - metadata: - name: statefulset-filter -+ namespace: staging -+ labels: -+ app: backend -+ tier: backend - source: | - # filter to return if resource is statefulset kind - def isStatefulSet(r): ++++ /dev/null +@@ -1,24 +0,0 @@ +-# Copyright 2021 The kpt Authors +-# +-# 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. +-apiVersion: fn.kpt.dev/v1alpha1 +-kind: StarlarkRun +-metadata: +- name: statefulset-filter +-source: | +- # filter to return if resource is statefulset kind +- def isStatefulSet(r): +- return r["apiVersion"] == "apps/v1" and r["kind"] == "StatefulSet" +- +- # filter out statefulsets +- ctx.resource_list["items"] = [r for r in ctx.resource_list["items"] if not isStatefulSet(r)] diff --git a/deployment_httpbin.yaml b/deployment_httpbin.yaml deleted file mode 100644 index 49d4f6e..0000000 diff --git a/e2e/testdata/live-apply/crd-and-cr/resources/cr.yaml b/e2e/testdata/live-apply/crd-and-cr/resources/cr.yaml index 4469a1e516..31f229523d 100644 --- a/e2e/testdata/live-apply/crd-and-cr/resources/cr.yaml +++ b/e2e/testdata/live-apply/crd-and-cr/resources/cr.yaml @@ -16,3 +16,5 @@ apiVersion: kpt.dev/v1 kind: Custom metadata: name: cr + annotations: + config.kubernetes.io/depends-on: apiextensions.k8s.io/CustomResourceDefinition/customs.kpt.dev diff --git a/pkg/lib/errors/resolver/live.go b/pkg/lib/errors/resolver/live.go index 3be9a5d07a..7b518a03d9 100644 --- a/pkg/lib/errors/resolver/live.go +++ b/pkg/lib/errors/resolver/live.go @@ -35,7 +35,7 @@ func init() { const ( noInventoryObjErrorMsg = ` -Error: Package uninitialized. Please run "kpt live init --name=" command. +Error: Package uninitialized. Please run "kpt live init --name=DEPLOYMENT_NAME" command. The package needs to be initialized to generate the template which will store state for resource sets. This state is diff --git a/pkg/lib/errors/resolver/pkg.go b/pkg/lib/errors/resolver/pkg.go index 6f2ab4ca9b..4ce8040663 100644 --- a/pkg/lib/errors/resolver/pkg.go +++ b/pkg/lib/errors/resolver/pkg.go @@ -82,7 +82,7 @@ func resolveNestedErr(err error, path string) (ResolvedResult, bool) { if errors.As(err, &deprecatedv1alpha2KptfileError) && deprecatedv1alpha2KptfileError.Version == "v1alpha2" { msg := fmt.Sprintf("Error: Kptfile at %q has an old version (%q) of the Kptfile schema.\n", path, deprecatedv1alpha2KptfileError.Version) - msg += "Please run \"kpt fn eval -i ghcr.io/kptdev/krm-functions-catalog/fix:latest --include-meta-resources\" to upgrade the package and retry." + msg += "Please run \"kpt fn eval PKG_PATH -i ghcr.io/kptdev/krm-functions-catalog/fix:latest --include-meta-resources\" to upgrade the package and retry." return ResolvedResult{ Message: msg, diff --git a/pkg/lib/errors/resolver/update.go b/pkg/lib/errors/resolver/update.go index a94c8f3e96..33449c4059 100644 --- a/pkg/lib/errors/resolver/update.go +++ b/pkg/lib/errors/resolver/update.go @@ -37,13 +37,13 @@ func (*updateErrorResolver) Resolve(err error) (ResolvedResult, bool) { if errors.As(err, &pkgNotGitRepoError) { //nolint:lll msg = fmt.Sprintf("Package %q is not within a git repository.", pkgNotGitRepoError.Path) - msg += " Please initialize a repository using 'git init' and then commit the changes using 'git commit -m \"\"'." + msg += " Please initialize a repository using 'git init' and then commit the changes using 'git commit -m \"COMMIT_MESSAGE\"'." } var pkgRepoDirtyError *update.PkgRepoDirtyError if errors.As(err, &pkgRepoDirtyError) { msg = fmt.Sprintf("Package %q contains uncommitted changes.", pkgRepoDirtyError.Path) - msg += " Please commit the changes using 'git commit -m \"\"'." + msg += " Please commit the changes using 'git commit -m \"COMMIT_MESSAGE\"'." } if msg != "" { diff --git a/thirdparty/cmdconfig/commands/cmdeval/cmdeval.go b/thirdparty/cmdconfig/commands/cmdeval/cmdeval.go index f4ba0fbe46..10cf7b7511 100644 --- a/thirdparty/cmdconfig/commands/cmdeval/cmdeval.go +++ b/thirdparty/cmdconfig/commands/cmdeval/cmdeval.go @@ -47,7 +47,7 @@ func GetEvalFnRunner(ctx context.Context, parent string) *EvalFnRunner { } r.Command = c r.Command.Flags().StringVarP(&r.Dest, "output", "o", "", - fmt.Sprintf("output resources are written to provided location. Allowed values: %s|%s|", cmdutil.Stdout, cmdutil.Unwrap)) + fmt.Sprintf("output resources are written to provided location. Allowed values: %s|%s|OUT_DIR_PATH", cmdutil.Stdout, cmdutil.Unwrap)) r.Command.Flags().StringVarP( &r.Image, "image", "i", "", "run this image as a function") r.Command.Flags().StringVar( From 14f9adc1ea24954fd2eff83540b8f347f0d8f2ed Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Thu, 12 Mar 2026 17:44:29 +0530 Subject: [PATCH 08/13] Fix live apply CRD ordering and align fn-render golden output Signed-off-by: Jaisheesh-2006 --- .../.expected/diff.patch | 95 ++++++++----------- 1 file changed, 39 insertions(+), 56 deletions(-) diff --git a/e2e/testdata/fn-render/subpkg-resource-deletion/.expected/diff.patch b/e2e/testdata/fn-render/subpkg-resource-deletion/.expected/diff.patch index 5f1d7ab4d8..aef01a905e 100644 --- a/e2e/testdata/fn-render/subpkg-resource-deletion/.expected/diff.patch +++ b/e2e/testdata/fn-render/subpkg-resource-deletion/.expected/diff.patch @@ -36,31 +36,25 @@ index 364e274..79d669a 100644 + - image: ghcr.io/kptdev/krm-functions-catalog/set-labels:v0.1.5 + exitCode: 0 diff --git a/db/Kptfile b/db/Kptfile -deleted file mode 100644 -index 6c7674c..0000000 +index 6c7674c..11fe9cc 100644 --- a/db/Kptfile -+++ /dev/null -@@ -1,14 +0,0 @@ --apiVersion: kpt.dev/v1 --kind: Kptfile --metadata: -- name: db --pipeline: -- mutators: -- - image: ghcr.io/kptdev/krm-functions-catalog/starlark:latest -- configPath: statefulset-filter.yaml -- - image: ghcr.io/kptdev/krm-functions-catalog/set-namespace:v0.2.0 -- configMap: -- namespace: db -- - image: ghcr.io/kptdev/krm-functions-catalog/set-labels:v0.1.5 -- configMap: -- app: backend ++++ b/db/Kptfile +@@ -2,6 +2,10 @@ apiVersion: kpt.dev/v1 + kind: Kptfile + metadata: + name: db ++ namespace: staging ++ labels: ++ app: backend ++ tier: backend + pipeline: + mutators: + - image: ghcr.io/kptdev/krm-functions-catalog/starlark:latest diff --git a/db/resources.yaml b/db/resources.yaml -deleted file mode 100644 -index f983597..0000000 +index f983597..9dabb18 100644 --- a/db/resources.yaml -+++ /dev/null -@@ -1,26 +0,0 @@ ++++ b/db/resources.yaml +@@ -1,26 +1,10 @@ -# Copyright 2021 The kpt Authors -# -# Licensed under the Apache License, Version 2.0 (the "License"); @@ -81,42 +75,31 @@ index f983597..0000000 -spec: - replicas: 3 ---- --apiVersion: custom.io/v1 --kind: Custom --metadata: -- name: custom --spec: -- image: nginx:1.2.3 + apiVersion: custom.io/v1 + kind: Custom + metadata: + name: custom ++ namespace: staging ++ labels: ++ app: backend ++ tier: backend + spec: + image: nginx:1.2.3 diff --git a/db/statefulset-filter.yaml b/db/statefulset-filter.yaml -deleted file mode 100644 -index e1f7b67..0000000 +index e1f7b67..ac69c02 100644 --- a/db/statefulset-filter.yaml -+++ /dev/null -@@ -1,24 +0,0 @@ --# Copyright 2021 The kpt Authors --# --# 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. --apiVersion: fn.kpt.dev/v1alpha1 --kind: StarlarkRun --metadata: -- name: statefulset-filter --source: | -- # filter to return if resource is statefulset kind -- def isStatefulSet(r): -- return r["apiVersion"] == "apps/v1" and r["kind"] == "StatefulSet" -- -- # filter out statefulsets -- ctx.resource_list["items"] = [r for r in ctx.resource_list["items"] if not isStatefulSet(r)] ++++ b/db/statefulset-filter.yaml +@@ -15,6 +15,10 @@ apiVersion: fn.kpt.dev/v1alpha1 + kind: StarlarkRun + metadata: + name: statefulset-filter ++ namespace: staging ++ labels: ++ app: backend ++ tier: backend + source: | + # filter to return if resource is statefulset kind + def isStatefulSet(r): diff --git a/deployment_httpbin.yaml b/deployment_httpbin.yaml deleted file mode 100644 index 49d4f6e..0000000 From 9f97bc9937de08fa2821256459f20a4eeb57c54a Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Thu, 12 Mar 2026 20:55:18 +0530 Subject: [PATCH 09/13] Reviewed and applied copilot sugesstions about OUT_DIR & COMMIT_MESSAGE Signed-off-by: Jaisheesh-2006 --- commands/fn/render/cmdrender.go | 2 +- pkg/lib/errors/resolver/pkg.go | 2 +- pkg/lib/errors/resolver/update.go | 4 ++-- thirdparty/cmdconfig/commands/cmdeval/cmdeval.go | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/commands/fn/render/cmdrender.go b/commands/fn/render/cmdrender.go index e1130a29da..43426ebef9 100644 --- a/commands/fn/render/cmdrender.go +++ b/commands/fn/render/cmdrender.go @@ -49,7 +49,7 @@ func NewRunner(ctx context.Context, parent string) *Runner { c.Flags().StringVar(&r.resultsDirPath, "results-dir", "", "path to a directory to save function results") c.Flags().StringVarP(&r.dest, "output", "o", "", - fmt.Sprintf("output resources are written to provided location. Allowed values: %s|%s|OUT_DIR_PATH", cmdutil.Stdout, cmdutil.Unwrap)) + fmt.Sprintf("output resources are written to provided location. Allowed values: %s|%s|", cmdutil.Stdout, cmdutil.Unwrap)) c.Flags().Var(&r.RunnerOptions.ImagePullPolicy, "image-pull-policy", "pull image before running the container "+r.RunnerOptions.ImagePullPolicy.HelpAllowedValues()) diff --git a/pkg/lib/errors/resolver/pkg.go b/pkg/lib/errors/resolver/pkg.go index 4ce8040663..be7e569f95 100644 --- a/pkg/lib/errors/resolver/pkg.go +++ b/pkg/lib/errors/resolver/pkg.go @@ -82,7 +82,7 @@ func resolveNestedErr(err error, path string) (ResolvedResult, bool) { if errors.As(err, &deprecatedv1alpha2KptfileError) && deprecatedv1alpha2KptfileError.Version == "v1alpha2" { msg := fmt.Sprintf("Error: Kptfile at %q has an old version (%q) of the Kptfile schema.\n", path, deprecatedv1alpha2KptfileError.Version) - msg += "Please run \"kpt fn eval PKG_PATH -i ghcr.io/kptdev/krm-functions-catalog/fix:latest --include-meta-resources\" to upgrade the package and retry." + msg += fmt.Sprintf("Please run \"kpt fn eval %q -i ghcr.io/kptdev/krm-functions-catalog/fix:latest --include-meta-resources\" to upgrade the package and retry.", path) return ResolvedResult{ Message: msg, diff --git a/pkg/lib/errors/resolver/update.go b/pkg/lib/errors/resolver/update.go index 33449c4059..8f3046014d 100644 --- a/pkg/lib/errors/resolver/update.go +++ b/pkg/lib/errors/resolver/update.go @@ -37,13 +37,13 @@ func (*updateErrorResolver) Resolve(err error) (ResolvedResult, bool) { if errors.As(err, &pkgNotGitRepoError) { //nolint:lll msg = fmt.Sprintf("Package %q is not within a git repository.", pkgNotGitRepoError.Path) - msg += " Please initialize a repository using 'git init' and then commit the changes using 'git commit -m \"COMMIT_MESSAGE\"'." + msg += " Please initialize a repository using 'git init' and then commit the changes using 'git commit -m \"\"'." } var pkgRepoDirtyError *update.PkgRepoDirtyError if errors.As(err, &pkgRepoDirtyError) { msg = fmt.Sprintf("Package %q contains uncommitted changes.", pkgRepoDirtyError.Path) - msg += " Please commit the changes using 'git commit -m \"COMMIT_MESSAGE\"'." + msg += " Please commit the changes using 'git commit -m \"\"'." } if msg != "" { diff --git a/thirdparty/cmdconfig/commands/cmdeval/cmdeval.go b/thirdparty/cmdconfig/commands/cmdeval/cmdeval.go index 10cf7b7511..f4ba0fbe46 100644 --- a/thirdparty/cmdconfig/commands/cmdeval/cmdeval.go +++ b/thirdparty/cmdconfig/commands/cmdeval/cmdeval.go @@ -47,7 +47,7 @@ func GetEvalFnRunner(ctx context.Context, parent string) *EvalFnRunner { } r.Command = c r.Command.Flags().StringVarP(&r.Dest, "output", "o", "", - fmt.Sprintf("output resources are written to provided location. Allowed values: %s|%s|OUT_DIR_PATH", cmdutil.Stdout, cmdutil.Unwrap)) + fmt.Sprintf("output resources are written to provided location. Allowed values: %s|%s|", cmdutil.Stdout, cmdutil.Unwrap)) r.Command.Flags().StringVarP( &r.Image, "image", "i", "", "run this image as a function") r.Command.Flags().StringVar( From 2d4a6fa16c63de1a09e0499d2ab37d020bec8c68 Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Wed, 15 Apr 2026 11:04:58 +0530 Subject: [PATCH 10/13] fix(live): harden deterministic inventory-id and legacy RG reconciliation - Remove silent fallback ID in load.go that could generate a mismatched namespace-name format ID (vs the SHA-1 hash from init); add klog warning - Mark --name as cobra-required so --help shows it and cobra rejects commands without it before reaching runE - Improve error messages and flag descriptions with Helm release-name analogy to guide users on stable naming - Add round-trip determinism test proving same --name + namespace always produces the same inventory-id (core fix for #4387) - Add legacy RG reconciliation: fail with LegacyRGMissingInventoryIDError when force=false, auto-repair with --force - Fix tautological lint in migratecmd.go switch statement - Update e2e tests and docs to reflect mandatory --name flag Fixes #4387 Signed-off-by: Jaisheesh-2006 --- commands/live/init/cmdliveinit.go | 3 +- commands/live/init/cmdliveinit_test.go | 78 +++++++++++++++++++++++++- commands/live/migrate/migratecmd.go | 2 +- pkg/lib/errors/resolver/live.go | 4 ++ pkg/live/load.go | 10 +++- 5 files changed, 91 insertions(+), 6 deletions(-) diff --git a/commands/live/init/cmdliveinit.go b/commands/live/init/cmdliveinit.go index 9bf2e82c46..1a5c8ff3c2 100644 --- a/commands/live/init/cmdliveinit.go +++ b/commands/live/init/cmdliveinit.go @@ -101,7 +101,8 @@ func NewRunner(ctx context.Context, factory k8scmdutil.Factory, } r.Command = cmd - cmd.Flags().StringVar(&r.Name, "name", "", "Stable deployment name for this package (required)") + cmd.Flags().StringVar(&r.Name, "name", "", "Stable deployment name for this package (like a Helm release name; use the same name when re-initializing to maintain ownership of deployed resources)") + _ = cmd.MarkFlagRequired("name") cmd.Flags().BoolVar(&r.Force, "force", false, "Set inventory values even if already set in Kptfile or ResourceGroup file") cmd.Flags().BoolVar(&r.Quiet, "quiet", false, "If true, do not print output message for initialization") cmd.Flags().StringVar(&r.InventoryID, "inventory-id", "", "Override the auto-derived inventory ID (advanced)") diff --git a/commands/live/init/cmdliveinit_test.go b/commands/live/init/cmdliveinit_test.go index 1cedb567c5..b2d7b2a42d 100644 --- a/commands/live/init/cmdliveinit_test.go +++ b/commands/live/init/cmdliveinit_test.go @@ -616,5 +616,81 @@ func TestCmd_MissingNameFlagReturnsError(t *testing.T) { err = runner.Command.Execute() require.Error(t, err) - assert.Contains(t, err.Error(), "--name is required") + // Cobra enforces --name as required and returns its own error message. + assert.Contains(t, err.Error(), "\"name\"") +} + +// TestRoundTrip_SameNameProducesSameInventoryID is the key test for issue #4387: +// if a user loses their local package and re-initializes with the same --name +// and same namespace, the inventory-id MUST be identical, so kpt live apply +// can reconnect to existing cluster resources. +func TestRoundTrip_SameNameProducesSameInventoryID(t *testing.T) { + tf := cmdtesting.NewTestFactory().WithNamespace("my-namespace") + defer tf.Cleanup() + ioStreams, _, _, _ := genericclioptions.NewTestIOStreams() //nolint:dogsled + + const deploymentName = "my-app-staging" + + // --- First initialization --- + w1, clean1 := testutil.SetupWorkspace(t) + defer clean1() + require.NoError(t, os.WriteFile( + filepath.Join(w1.WorkspaceDirectory, kptfilev1.KptFileName), + []byte(kptFile), 0600)) + + revert1 := testutil.Chdir(t, w1.WorkspaceDirectory) + defer revert1() + + runner1 := NewRunner(fake.CtxWithDefaultPrinter(), tf, ioStreams) + runner1.RGFileName = "resourcegroup.yaml" + runner1.Command.SetArgs([]string{ + "--name", deploymentName, + }) + require.NoError(t, runner1.Command.Execute()) + + rg1, err := pkg.ReadRGFile(w1.WorkspaceDirectory, "resourcegroup.yaml") + require.NoError(t, err) + require.NotNil(t, rg1) + firstInvID := rg1.Labels[rgfilev1alpha1.RGInventoryIDLabel] + assert.NotEmpty(t, firstInvID, "first init must produce an inventory-id") + + // --- Simulate losing the package: create a brand-new workspace --- + revert1() // leave the first directory + + w2, clean2 := testutil.SetupWorkspace(t) + defer clean2() + require.NoError(t, os.WriteFile( + filepath.Join(w2.WorkspaceDirectory, kptfilev1.KptFileName), + []byte(kptFile), 0600)) + + revert2 := testutil.Chdir(t, w2.WorkspaceDirectory) + defer revert2() + + // Re-create factory with same namespace to ensure same context. + tf2 := cmdtesting.NewTestFactory().WithNamespace("my-namespace") + defer tf2.Cleanup() + + runner2 := NewRunner(fake.CtxWithDefaultPrinter(), tf2, ioStreams) + runner2.RGFileName = "resourcegroup.yaml" + runner2.Command.SetArgs([]string{ + "--name", deploymentName, + }) + require.NoError(t, runner2.Command.Execute()) + + rg2, err := pkg.ReadRGFile(w2.WorkspaceDirectory, "resourcegroup.yaml") + require.NoError(t, err) + require.NotNil(t, rg2) + secondInvID := rg2.Labels[rgfilev1alpha1.RGInventoryIDLabel] + + // --- THE KEY ASSERTION --- + // Same name + same namespace MUST produce the exact same inventory-id, + // so kpt live apply can reconnect to the existing deployment. + assert.Equal(t, firstInvID, secondInvID, + "re-initializing with the same --name and namespace must produce the same inventory-id (issue #4387)") + + // Also verify the name and namespace are correct. + assert.Equal(t, deploymentName, rg1.Name) + assert.Equal(t, deploymentName, rg2.Name) + assert.Equal(t, "my-namespace", rg1.Namespace) + assert.Equal(t, "my-namespace", rg2.Namespace) } diff --git a/commands/live/migrate/migratecmd.go b/commands/live/migrate/migratecmd.go index 591031c922..4d327f8e6a 100644 --- a/commands/live/migrate/migratecmd.go +++ b/commands/live/migrate/migratecmd.go @@ -401,7 +401,7 @@ func (mr *Runner) migrateKptfileToRG(args []string) error { return errors.E(op, errors.IO, types.UniquePath(dir), "the resourcegroup file already exists and inventory information cannot be migrated") } // Force mode: proceed to overwrite the existing RG file. - case rgFileErr != nil && !goerrors.Is(rgFileErr, os.ErrNotExist): + case !goerrors.Is(rgFileErr, os.ErrNotExist): return errors.E(op, errors.IO, types.UniquePath(dir), rgFileErr) } diff --git a/pkg/lib/errors/resolver/live.go b/pkg/lib/errors/resolver/live.go index 7b518a03d9..4c5f323975 100644 --- a/pkg/lib/errors/resolver/live.go +++ b/pkg/lib/errors/resolver/live.go @@ -37,6 +37,10 @@ const ( noInventoryObjErrorMsg = ` Error: Package uninitialized. Please run "kpt live init --name=DEPLOYMENT_NAME" command. +The --name flag is required and must be a stable identifier (like a Helm release +name). If you lose your local package and re-initialize, using the same --name +ensures kpt can reconnect to previously deployed resources. + The package needs to be initialized to generate the template which will store state for resource sets. This state is necessary to perform functionality such as deleting an entire diff --git a/pkg/live/load.go b/pkg/live/load.go index c08ff48e48..fe65057d6c 100644 --- a/pkg/live/load.go +++ b/pkg/live/load.go @@ -324,10 +324,14 @@ func validateInventory(inventory kptfilev1.Inventory) error { func generateInventoryObj(inv kptfilev1.Inventory) *unstructured.Unstructured { // Create and return ResourceGroup custom resource as inventory object. - // When inventoryID is not specified by the local resourcegroup, we generate one using - // depends-on annotation format that we store on the live cluster. This implementation detail - // is hidden from the local resourcegroup unless the one was explicitly generated by the client. + // inventoryID must be set by the caller (via kpt live init --name=...). + // validateInventory() enforces this before we reach here. If somehow + // empty, log a warning — do NOT silently generate a fallback ID, as it + // would differ from the deterministic SHA-1 hash that init produces, + // leading to ownership conflicts (see issue #4387). if inv.InventoryID == "" { + klog.Warningf("generateInventoryObj called with empty inventoryID for %s/%s; "+ + "this should have been caught by validateInventory", inv.Namespace, inv.Name) inv.InventoryID = fmt.Sprintf(inventoryIDfmt, inv.Namespace, inv.Name) } From 00544a48a91adf0700de521afa87e5ee91414dc9 Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Wed, 15 Apr 2026 16:49:42 +0530 Subject: [PATCH 11/13] fix(live): remove legacy inventory-id fallback and clean migrate/init lint issues Signed-off-by: Jaisheesh-2006 --- commands/live/init/cmdliveinit.go | 4 ++- commands/live/init/cmdliveinit_test.go | 40 +++++++++++++------------- commands/live/migrate/migratecmd.go | 17 ++--------- pkg/live/load.go | 10 ++----- 4 files changed, 28 insertions(+), 43 deletions(-) diff --git a/commands/live/init/cmdliveinit.go b/commands/live/init/cmdliveinit.go index 1a5c8ff3c2..c62eeda562 100644 --- a/commands/live/init/cmdliveinit.go +++ b/commands/live/init/cmdliveinit.go @@ -101,7 +101,9 @@ func NewRunner(ctx context.Context, factory k8scmdutil.Factory, } r.Command = cmd - cmd.Flags().StringVar(&r.Name, "name", "", "Stable deployment name for this package (like a Helm release name; use the same name when re-initializing to maintain ownership of deployed resources)") + nameHelp := "Stable deployment name for this package (like a Helm release name; " + + "use the same name when re-initializing to maintain ownership of deployed resources)" + cmd.Flags().StringVar(&r.Name, "name", "", nameHelp) _ = cmd.MarkFlagRequired("name") cmd.Flags().BoolVar(&r.Force, "force", false, "Set inventory values even if already set in Kptfile or ResourceGroup file") cmd.Flags().BoolVar(&r.Quiet, "quiet", false, "If true, do not print output message for initialization") diff --git a/commands/live/init/cmdliveinit_test.go b/commands/live/init/cmdliveinit_test.go index b2d7b2a42d..98a59436ab 100644 --- a/commands/live/init/cmdliveinit_test.go +++ b/commands/live/init/cmdliveinit_test.go @@ -213,7 +213,7 @@ func TestCmd_Run(t *testing.T) { "Missing name is an error": { kptfile: kptFile, name: "", - rgfilename: "resourcegroup.yaml", + rgfilename: rgfilev1alpha1.RGFileName, namespace: "testns", inventoryID: "", expectedErrorMsg: "--name is required", @@ -221,7 +221,7 @@ func TestCmd_Run(t *testing.T) { "Whitespace-only name is rejected": { kptfile: kptFile, name: " ", - rgfilename: "resourcegroup.yaml", + rgfilename: rgfilev1alpha1.RGFileName, namespace: "testns", inventoryID: "", expectedErrorMsg: "--name is required", @@ -229,14 +229,14 @@ func TestCmd_Run(t *testing.T) { "Invalid DNS name is rejected": { kptfile: kptFile, name: "My_App!", - rgfilename: "resourcegroup.yaml", + rgfilename: rgfilev1alpha1.RGFileName, namespace: "testns", inventoryID: "", expectedErrorMsg: "not a valid Kubernetes resource name", }, "Explicit inventory-id is preserved when both flags are set": { kptfile: kptFile, - rgfilename: "resourcegroup.yaml", + rgfilename: rgfilev1alpha1.RGFileName, name: "my-pkg", namespace: "my-ns", inventoryID: "custom-legacy-id-123", @@ -248,7 +248,7 @@ func TestCmd_Run(t *testing.T) { }, "Provided name derives deterministic inventory ID": { kptfile: kptFile, - rgfilename: "resourcegroup.yaml", + rgfilename: rgfilev1alpha1.RGFileName, name: "my-pkg", namespace: "my-ns", inventoryID: "", @@ -293,7 +293,7 @@ func TestCmd_Run(t *testing.T) { "ResourceGroup without inventory-id is legacy error": { kptfile: kptFile, resourcegroup: resourceGroupInventory, - rgfilename: "resourcegroup.yaml", + rgfilename: rgfilev1alpha1.RGFileName, name: inventoryName, namespace: inventoryNamespace, inventoryID: inventoryID, @@ -303,7 +303,7 @@ func TestCmd_Run(t *testing.T) { "ResourceGroup with inventory already set is error": { kptfile: kptFile, resourcegroup: resourceGroupInventoryWithID, - rgfilename: "resourcegroup.yaml", + rgfilename: rgfilev1alpha1.RGFileName, name: inventoryName, namespace: inventoryNamespace, inventoryID: inventoryID, @@ -313,7 +313,7 @@ func TestCmd_Run(t *testing.T) { "ResourceGroup with inventory and Kptfile with inventory already set is error": { kptfile: kptFileWithInventory, resourcegroup: resourceGroupInventoryWithID, - rgfilename: "resourcegroup.yaml", + rgfilename: rgfilev1alpha1.RGFileName, name: inventoryName, namespace: inventoryNamespace, inventoryID: inventoryID, @@ -323,7 +323,7 @@ func TestCmd_Run(t *testing.T) { "The force flag allows changing inventory information even if already set in Kptfile": { kptfile: kptFileWithInventory, name: inventoryName, - rgfilename: "resourcegroup.yaml", + rgfilename: rgfilev1alpha1.RGFileName, namespace: inventoryNamespace, inventoryID: inventoryID, force: true, @@ -336,7 +336,7 @@ func TestCmd_Run(t *testing.T) { "The force flag allows changing inventory information even if already set in ResourceGroup": { kptfile: kptFile, resourcegroup: resourceGroupInventoryWithID, - rgfilename: "resourcegroup.yaml", + rgfilename: rgfilev1alpha1.RGFileName, name: inventoryName, namespace: inventoryNamespace, inventoryID: inventoryID, @@ -515,7 +515,7 @@ func TestConfigureInventoryInfo_InvalidDirectoryNameFallback(t *testing.T) { Quiet: true, Name: "", InventoryID: "explicit-inv-id-123", - RGFileName: "resourcegroup.yaml", + RGFileName: rgfilev1alpha1.RGFileName, } err = c.Run(fake.CtxWithDefaultPrinter()) @@ -548,13 +548,13 @@ func TestConfigureInventoryInfo_ValidDirectoryNameFallback(t *testing.T) { Quiet: true, Name: "", InventoryID: "explicit-inv-id-456", - RGFileName: "resourcegroup.yaml", + RGFileName: rgfilev1alpha1.RGFileName, } err = c.Run(fake.CtxWithDefaultPrinter()) require.NoError(t, err) - rg, err := pkg.ReadRGFile(validDir, "resourcegroup.yaml") + rg, err := pkg.ReadRGFile(validDir, rgfilev1alpha1.RGFileName) require.NoError(t, err) require.NotNil(t, rg) assert.Equal(t, "my-valid-pkg", rg.Name) @@ -583,13 +583,13 @@ func TestConfigureInventoryInfo_DottedDirectoryNameFallback(t *testing.T) { Quiet: true, Name: "", InventoryID: "explicit-inv-id-789", - RGFileName: "resourcegroup.yaml", + RGFileName: rgfilev1alpha1.RGFileName, } err = c.Run(fake.CtxWithDefaultPrinter()) require.NoError(t, err) - rg, err := pkg.ReadRGFile(validDir, "resourcegroup.yaml") + rg, err := pkg.ReadRGFile(validDir, rgfilev1alpha1.RGFileName) require.NoError(t, err) require.NotNil(t, rg) assert.Equal(t, "my.valid.pkg", rg.Name) @@ -611,7 +611,7 @@ func TestCmd_MissingNameFlagReturnsError(t *testing.T) { defer revert() runner := NewRunner(fake.CtxWithDefaultPrinter(), tf, ioStreams) - runner.RGFileName = "resourcegroup.yaml" + runner.RGFileName = rgfilev1alpha1.RGFileName runner.Command.SetArgs([]string{}) err = runner.Command.Execute() @@ -642,13 +642,13 @@ func TestRoundTrip_SameNameProducesSameInventoryID(t *testing.T) { defer revert1() runner1 := NewRunner(fake.CtxWithDefaultPrinter(), tf, ioStreams) - runner1.RGFileName = "resourcegroup.yaml" + runner1.RGFileName = rgfilev1alpha1.RGFileName runner1.Command.SetArgs([]string{ "--name", deploymentName, }) require.NoError(t, runner1.Command.Execute()) - rg1, err := pkg.ReadRGFile(w1.WorkspaceDirectory, "resourcegroup.yaml") + rg1, err := pkg.ReadRGFile(w1.WorkspaceDirectory, rgfilev1alpha1.RGFileName) require.NoError(t, err) require.NotNil(t, rg1) firstInvID := rg1.Labels[rgfilev1alpha1.RGInventoryIDLabel] @@ -671,13 +671,13 @@ func TestRoundTrip_SameNameProducesSameInventoryID(t *testing.T) { defer tf2.Cleanup() runner2 := NewRunner(fake.CtxWithDefaultPrinter(), tf2, ioStreams) - runner2.RGFileName = "resourcegroup.yaml" + runner2.RGFileName = rgfilev1alpha1.RGFileName runner2.Command.SetArgs([]string{ "--name", deploymentName, }) require.NoError(t, runner2.Command.Execute()) - rg2, err := pkg.ReadRGFile(w2.WorkspaceDirectory, "resourcegroup.yaml") + rg2, err := pkg.ReadRGFile(w2.WorkspaceDirectory, rgfilev1alpha1.RGFileName) require.NoError(t, err) require.NotNil(t, rg2) secondInvID := rg2.Labels[rgfilev1alpha1.RGInventoryIDLabel] diff --git a/commands/live/migrate/migratecmd.go b/commands/live/migrate/migratecmd.go index 4d327f8e6a..91a6a05b42 100644 --- a/commands/live/migrate/migratecmd.go +++ b/commands/live/migrate/migratecmd.go @@ -405,11 +405,6 @@ func (mr *Runner) migrateKptfileToRG(args []string) error { return errors.E(op, errors.IO, types.UniquePath(dir), rgFileErr) } - if kf.Inventory.Name == "" { - return errors.E(op, types.UniquePath(dir), - fmt.Errorf("kptfile inventory has empty name; re-run: kpt live init --name=")) - } - err = (&initialization.ConfigureInventoryInfo{ Pkg: p, Factory: mr.factory, @@ -498,16 +493,10 @@ func (mr *Runner) createRGfile(ctx context.Context, args []string, prevID string if err != nil { var invExistsError *initialization.InvExistsError - var legacyErr *initialization.LegacyRGMissingInventoryIDError - if errors.As(err, &invExistsError) { + switch { + case errors.As(err, &invExistsError): fmt.Fprint(mr.ioStreams.Out, "values already exist...") - } else if errors.As(err, &legacyErr) { - // Legacy RG without inventory-id detected. If force is - // not set, surface the error so the user sees repair - // instructions. (When force *is* set, ConfigureInventoryInfo - // already overwrites the RG, so we never land here.) - return err - } else { + default: return err } } diff --git a/pkg/live/load.go b/pkg/live/load.go index fe65057d6c..9a43fdd945 100644 --- a/pkg/live/load.go +++ b/pkg/live/load.go @@ -38,10 +38,6 @@ import ( "sigs.k8s.io/kustomize/kyaml/yaml" ) -// inventoryIDfmt is the string format used for generating an inventoryID that is stored on the live cluster -// if one is not provided when the user runs `kpt live init`. This format should be of `namespace-name`. -const inventoryIDfmt = "%s-%s" - // InventoryInfoValidationError is the error returned if validation of the // inventory information fails. type InventoryInfoValidationError struct { @@ -326,13 +322,11 @@ func generateInventoryObj(inv kptfilev1.Inventory) *unstructured.Unstructured { // inventoryID must be set by the caller (via kpt live init --name=...). // validateInventory() enforces this before we reach here. If somehow - // empty, log a warning — do NOT silently generate a fallback ID, as it - // would differ from the deterministic SHA-1 hash that init produces, - // leading to ownership conflicts (see issue #4387). + // empty, log a warning and preserve the empty value rather than generating + // a legacy fallback ID that could conflict with deterministic init IDs. if inv.InventoryID == "" { klog.Warningf("generateInventoryObj called with empty inventoryID for %s/%s; "+ "this should have been caught by validateInventory", inv.Namespace, inv.Name) - inv.InventoryID = fmt.Sprintf(inventoryIDfmt, inv.Namespace, inv.Name) } groupVersion := fmt.Sprintf("%s/%s", ResourceGroupGVK.Group, ResourceGroupGVK.Version) From 8ef1f4870116ca7c6614107c00d81403aa006130 Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Wed, 20 May 2026 11:59:33 +0530 Subject: [PATCH 12/13] Fix: Copilot suggestions Signed-off-by: Jaisheesh-2006 --- pkg/lib/errors/errors.go | 7 ++++++- pkg/lib/errors/resolver/live.go | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/pkg/lib/errors/errors.go b/pkg/lib/errors/errors.go index a4a297b8ca..0458b9122f 100644 --- a/pkg/lib/errors/errors.go +++ b/pkg/lib/errors/errors.go @@ -187,7 +187,12 @@ func E(args ...any) error { case Class: e.Class = a case *Error: - e.Err = new(*a) + if a == nil { + e.Err = nil + break + } + copied := *a + e.Err = &copied case error: e.Err = a case string: diff --git a/pkg/lib/errors/resolver/live.go b/pkg/lib/errors/resolver/live.go index 4c5f323975..ad66794ad5 100644 --- a/pkg/lib/errors/resolver/live.go +++ b/pkg/lib/errors/resolver/live.go @@ -35,7 +35,7 @@ func init() { const ( noInventoryObjErrorMsg = ` -Error: Package uninitialized. Please run "kpt live init --name=DEPLOYMENT_NAME" command. +Error: Package uninitialized. Please run "kpt live init --name=deployment-name" command. The --name flag is required and must be a stable identifier (like a Helm release name). If you lose your local package and re-initialize, using the same --name From f736c062e075b419ab750c1d11dda74d2ab42431 Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Wed, 20 May 2026 12:33:06 +0530 Subject: [PATCH 13/13] Fix : Trim and validate init name in-place Signed-off-by: Jaisheesh-2006 --- commands/live/init/cmdliveinit.go | 19 +++++++++---------- commands/live/init/cmdliveinit_test.go | 8 ++++---- 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/commands/live/init/cmdliveinit.go b/commands/live/init/cmdliveinit.go index c62eeda562..f63a477bcb 100644 --- a/commands/live/init/cmdliveinit.go +++ b/commands/live/init/cmdliveinit.go @@ -141,11 +141,9 @@ func (r *Runner) preRunE(_ *cobra.Command, _ []string) error { func (r *Runner) runE(_ *cobra.Command, args []string) error { const op errors.Op = "cmdliveinit.runE" - name, err := validateName(r.Name) - if err != nil { + if err := trimAndValidateName(&r.Name); err != nil { return errors.E(op, err) } - r.Name = name if len(args) == 0 { // default to the current working directory cwd, err := os.Getwd() @@ -356,18 +354,19 @@ func generateHash(namespace, name string) (string, error) { return hex.EncodeToString(h.Sum(nil)), nil } -// validateName rejects empty, whitespace-only, and invalid RFC 1123 subdomain names. -// Returns the trimmed name on success. -func validateName(name string) (string, error) { - trimmed := strings.TrimSpace(name) +// trimAndValidateName rejects empty, whitespace-only, and invalid RFC 1123 subdomain names. +// It updates name with the trimmed value on success. +func trimAndValidateName(name *string) error { + trimmed := strings.TrimSpace(*name) if trimmed == "" { - return "", errNameRequired + return errNameRequired } if errs := validation.IsDNS1123Subdomain(trimmed); len(errs) > 0 { - return "", fmt.Errorf("--name %q is not a valid Kubernetes resource name: %s", + return fmt.Errorf("--name %q is not a valid Kubernetes resource name: %s", trimmed, strings.Join(errs, "; ")) } - return trimmed, nil + *name = trimmed + return nil } // kptfileInventoryEmpty returns true if the Inventory structure diff --git a/commands/live/init/cmdliveinit_test.go b/commands/live/init/cmdliveinit_test.go index 98a59436ab..802df99b5f 100644 --- a/commands/live/init/cmdliveinit_test.go +++ b/commands/live/init/cmdliveinit_test.go @@ -89,7 +89,7 @@ metadata: cli-utils.sigs.k8s.io/inventory-id: SSSSSSSSSS-RRRRR ` -func TestValidateName(t *testing.T) { +func TestTrimAndValidateName(t *testing.T) { testCases := map[string]struct { name string expectError bool @@ -140,14 +140,14 @@ func TestValidateName(t *testing.T) { for tn, tc := range testCases { t.Run(tn, func(t *testing.T) { - result, err := validateName(tc.name) + name := tc.name + err := trimAndValidateName(&name) if tc.expectError { require.Error(t, err) assert.Contains(t, err.Error(), tc.errContains) - assert.Empty(t, result) } else { require.NoError(t, err) - assert.Equal(t, strings.TrimSpace(tc.name), result) + assert.Equal(t, strings.TrimSpace(tc.name), name) } }) }