-
Notifications
You must be signed in to change notification settings - Fork 256
Replace random generated "inventory ID" #4396
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Jaisheesh-2006
wants to merge
13
commits into
kptdev:main
Choose a base branch
from
Jaisheesh-2006:fix/4387-loose-inventory-id
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
ddd3fa7
fix(live): use deterministic hash for inventory ID
Jaisheesh-2006 70553ca
fix: lowercase error string and fix e2e assertRGInventory name matching
Jaisheesh-2006 e2d0bdd
fix(e2e): use consistent inventory name in quiet init test
Jaisheesh-2006 d3b1d63
fix(live): harden inventory name validation and init logic
Jaisheesh-2006 c687b4c
fix(live): add SHA-1 comment and align e2e test assertion
Jaisheesh-2006 c9fd46d
refactor: apply code review suggestions for error formatting and comm…
Jaisheesh-2006 039663c
fix: stabilize e2e expectations and harden live init validation
Jaisheesh-2006 14f9adc
Fix live apply CRD ordering and align fn-render golden output
Jaisheesh-2006 9f97bc9
Reviewed and applied copilot sugesstions about OUT_DIR & COMMIT_MESSAGE
Jaisheesh-2006 2d4a6fa
fix(live): harden deterministic inventory-id and legacy RG reconcilia…
Jaisheesh-2006 00544a4
fix(live): remove legacy inventory-id fallback and clean migrate/init…
Jaisheesh-2006 8ef1f48
Fix: Copilot suggestions
Jaisheesh-2006 f736c06
Fix : Trim and validate init name in-place
Jaisheesh-2006 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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,17 @@ 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 +73,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 +101,15 @@ func NewRunner(ctx context.Context, factory k8scmdutil.Factory, | |||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| r.Command = cmd | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| cmd.Flags().StringVar(&r.Name, "name", "", "Inventory object name") | ||||||||||||||||||||||||||
| 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") | ||||||||||||||||||||||||||
| 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 +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" | ||||||||||||||||||||||||||
| if err := trimAndValidateName(&r.Name); err != nil { | ||||||||||||||||||||||||||
| return errors.E(op, err) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| if len(args) == 0 { | ||||||||||||||||||||||||||
| // default to the current working directory | ||||||||||||||||||||||||||
| cwd, err := os.Getwd() | ||||||||||||||||||||||||||
|
|
@@ -193,15 +211,25 @@ 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 != "" { | ||||||||||||||||||||||||||
| dirName := filepath.Base(c.Pkg.UniquePath.String()) | ||||||||||||||||||||||||||
| 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, "; "))) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| c.Name = dirName | ||||||||||||||||||||||||||
|
Comment on lines
+218
to
+224
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||
| return errors.E(op, c.Pkg.UniquePath, errNameRequired) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Jaisheesh-2006 marked this conversation as resolved.
|
||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // 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 +288,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 +335,38 @@ 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") | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| // 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) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| timeStr := strconv.FormatInt(t.UTC().UnixNano(), 10) | ||||||||||||||||||||||||||
| return fmt.Sprintf("%s-%s", hashStr, timeStr), nil | ||||||||||||||||||||||||||
| return hex.EncodeToString(h.Sum(nil)), nil | ||||||||||||||||||||||||||
|
Jaisheesh-2006 marked this conversation as resolved.
|
||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // 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")) | ||||||||||||||||||||||||||
| // 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 | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||
| *name = trimmed | ||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // kptfileInventoryEmpty returns true if the Inventory structure | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.