From e02ee1d6bb4683653cc9d813836beb65e7faef6e Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Wed, 6 Aug 2025 21:28:32 +1200 Subject: [PATCH 1/5] Use same quoting in isolinux.cfg as in grub.cfg Quoting is required in grub.cfg, and was added in MGMT-4693. This solves the problem that the '&' character in a URL is considered a word separator by Grub. However, having different kernel args between isolinux.cfg and grub.cfg is likely problematic for coreos-installer when editing kargs. Making them the same length also avoids the need to adjust padding in order to keep the same embed area length in each file (which is required, as kargs.json does not store per-file embed area sizes). isolinux.cfg does not use quoting, so the parameter will be passed to the kernel quoted. The kernel will only strip double quotes, not single quotes. In grub.cfg, using double quotes allows the expansion of variables with '$' and escaping with '\' (both of which are suppressed when using single quotes). '\' is not a valid character in a URL. '$' technically is, but is vanishingly rare. We will prohibit these characters so that we can use the same kargs in both files. Assisted-by: Cursor --- pkg/isoeditor/rhcos.go | 21 ++++++++++++++++++-- pkg/isoeditor/rhcos_test.go | 38 +++++++++++++++++++++++++++++++++---- 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/pkg/isoeditor/rhcos.go b/pkg/isoeditor/rhcos.go index adf330c3..da695ae8 100644 --- a/pkg/isoeditor/rhcos.go +++ b/pkg/isoeditor/rhcos.go @@ -5,6 +5,7 @@ import ( "os" "path/filepath" "regexp" + "strings" "github.com/openshift/assisted-image-service/internal/common" log "github.com/sirupsen/logrus" @@ -149,6 +150,9 @@ func embedInitrdPlaceholders(extractDir string) error { } func fixGrubConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool) error { + if err := validateRootFSURL(rootFSURL); err != nil { + return err + } availableGrubPaths := []string{"EFI/redhat/grub.cfg", "EFI/fedora/grub.cfg", "boot/grub/grub.cfg", "EFI/centos/grub.cfg"} var foundGrubPath string for _, pathSection := range availableGrubPaths { @@ -163,7 +167,7 @@ func fixGrubConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool) err } // Add the rootfs url - replacement := fmt.Sprintf("$1 $2 'coreos.live.rootfs_url=%s'", rootFSURL) + replacement := fmt.Sprintf("$1 $2 coreos.live.rootfs_url=\"%s\"", rootFSURL) if err := editFile(foundGrubPath, `(?m)^(\s+linux) (.+| )+$`, replacement); err != nil { return err } @@ -187,8 +191,21 @@ func fixGrubConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool) err return nil } +func validateRootFSURL(rootFSURL string) error { + if strings.Contains(rootFSURL, "$") { + return fmt.Errorf("invalid rootfs URL: contains invalid character '$'") + } + if strings.Contains(rootFSURL, "\\") { + return fmt.Errorf("invalid rootfs URL: contains invalid character '\\'") + } + return nil +} + func fixIsolinuxConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool) error { - replacement := fmt.Sprintf("$1 $2 coreos.live.rootfs_url=%s", rootFSURL) + if err := validateRootFSURL(rootFSURL); err != nil { + return err + } + replacement := fmt.Sprintf("$1 $2 coreos.live.rootfs_url=\"%s\"", rootFSURL) if err := editFile(filepath.Join(extractDir, "isolinux/isolinux.cfg"), `(?m)^(\s+append) (.+| )+$`, replacement); err != nil { return err } diff --git a/pkg/isoeditor/rhcos_test.go b/pkg/isoeditor/rhcos_test.go index db74f79e..1286ee9f 100644 --- a/pkg/isoeditor/rhcos_test.go +++ b/pkg/isoeditor/rhcos_test.go @@ -98,7 +98,7 @@ var _ = Context("with test files", func() { err := fixGrubConfig(testRootFSURL, filesDir, true) Expect(err).ToNot(HaveOccurred()) - newLine := " linux /images/pxeboot/vmlinuz random.trust_cpu=on rd.luks.options=discard ignition.firstboot ignition.platform.id=metal 'coreos.live.rootfs_url=%s'" + newLine := " linux /images/pxeboot/vmlinuz random.trust_cpu=on rd.luks.options=discard ignition.firstboot ignition.platform.id=metal coreos.live.rootfs_url=\"%s\"" grubCfg := fmt.Sprintf(newLine, testRootFSURL) validateFileContainsLine(filepath.Join(filesDir, "EFI/redhat/grub.cfg"), grubCfg) @@ -111,7 +111,7 @@ var _ = Context("with test files", func() { err := fixIsolinuxConfig(testRootFSURL, filesDir, true) Expect(err).ToNot(HaveOccurred()) - newLine := " append initrd=/images/pxeboot/initrd.img,/images/ignition.img,%s,%s random.trust_cpu=on rd.luks.options=discard ignition.firstboot ignition.platform.id=metal coreos.live.rootfs_url=%s" + newLine := " append initrd=/images/pxeboot/initrd.img,/images/ignition.img,%s,%s random.trust_cpu=on rd.luks.options=discard ignition.firstboot ignition.platform.id=metal coreos.live.rootfs_url=\"%s\"" isolinuxCfg := fmt.Sprintf(newLine, ramDiskImagePath, nmstateDiskImagePath, testRootFSURL) validateFileContainsLine(filepath.Join(filesDir, "isolinux/isolinux.cfg"), isolinuxCfg) }) @@ -122,7 +122,7 @@ var _ = Context("with test files", func() { err := fixGrubConfig(testRootFSURL, filesDir, false) Expect(err).ToNot(HaveOccurred()) - newLine := " linux /images/pxeboot/vmlinuz random.trust_cpu=on rd.luks.options=discard ignition.firstboot ignition.platform.id=metal 'coreos.live.rootfs_url=%s'" + newLine := " linux /images/pxeboot/vmlinuz random.trust_cpu=on rd.luks.options=discard ignition.firstboot ignition.platform.id=metal coreos.live.rootfs_url=\"%s\"" grubCfg := fmt.Sprintf(newLine, testRootFSURL) validateFileContainsLine(filepath.Join(filesDir, "EFI/redhat/grub.cfg"), grubCfg) @@ -135,10 +135,40 @@ var _ = Context("with test files", func() { err := fixIsolinuxConfig(testRootFSURL, filesDir, false) Expect(err).ToNot(HaveOccurred()) - newLine := " append initrd=/images/pxeboot/initrd.img,/images/ignition.img,%s random.trust_cpu=on rd.luks.options=discard ignition.firstboot ignition.platform.id=metal coreos.live.rootfs_url=%s" + newLine := " append initrd=/images/pxeboot/initrd.img,/images/ignition.img,%s random.trust_cpu=on rd.luks.options=discard ignition.firstboot ignition.platform.id=metal coreos.live.rootfs_url=\"%s\"" isolinuxCfg := fmt.Sprintf(newLine, ramDiskImagePath, testRootFSURL) validateFileContainsLine(filepath.Join(filesDir, "isolinux/isolinux.cfg"), isolinuxCfg) }) }) + + Context("URL validation", func() { + It("rejects URLs containing $ character", func() { + invalidURL := "https://example.com/test$invalid/rootfs.img" + err := fixGrubConfig(invalidURL, filesDir, false) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("invalid rootfs URL: contains invalid character '$'")) + + err = fixIsolinuxConfig(invalidURL, filesDir, false) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("invalid rootfs URL: contains invalid character '$'")) + }) + + It("rejects URLs containing \\ character", func() { + invalidURL := "https://example.com/test\\invalid/rootfs.img" + err := fixGrubConfig(invalidURL, filesDir, false) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("invalid rootfs URL: contains invalid character '\\'")) + + err = fixIsolinuxConfig(invalidURL, filesDir, false) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("invalid rootfs URL: contains invalid character '\\'")) + }) + + It("accepts valid URLs", func() { + validURL := "https://example.com/valid/rootfs.img" + err := validateRootFSURL(validURL) + Expect(err).ToNot(HaveOccurred()) + }) + }) }) }) From 2f362d7d11df91cf122a8813b623e992e5266582 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Wed, 10 Dec 2025 18:05:57 +1300 Subject: [PATCH 2/5] refactor: Edit minimal ISO kernel args in a single place Consolidate the duplicated logic for modifying kernel arguments into a single transformKernelArgs() function. This function: - Validates the rootfs URL - Removes the coreos.liveiso parameter - Adds the coreos.live.rootfs_url parameter at a specified position Both fixGrubConfig and fixIsolinuxConfig use this single implementation instead of each having their own inline logic. This eliminates code duplication and makes the transformation logic easier to understand and maintain. Assisted-by: Cursor Assisted-by: Claude Code --- pkg/isoeditor/rhcos.go | 68 +++++++++++++++++++++---------------- pkg/isoeditor/rhcos_test.go | 2 +- 2 files changed, 40 insertions(+), 30 deletions(-) diff --git a/pkg/isoeditor/rhcos.go b/pkg/isoeditor/rhcos.go index da695ae8..74ee1918 100644 --- a/pkg/isoeditor/rhcos.go +++ b/pkg/isoeditor/rhcos.go @@ -20,6 +20,32 @@ const ( RootfsImagePath = "images/pxeboot/rootfs.img" ) +// transformKernelArgs applies the standard kernel argument transformations: +// 1. Remove coreos.liveiso parameter +// 2. Add coreos.live.rootfs_url parameter at the specified insertion point +func transformKernelArgs(filePath string, linePattern string, rootFSURL string) error { + // Validate rootfs URL + if strings.Contains(rootFSURL, "$") { + return fmt.Errorf("invalid rootfs URL: contains invalid character '$'") + } + if strings.Contains(rootFSURL, "\\") { + return fmt.Errorf("invalid rootfs URL: contains invalid character '\\'") + } + + // Add the rootfs_url parameter + replacement := "$1 $2 coreos.live.rootfs_url=\"" + rootFSURL + "\"" + if err := editFile(filePath, linePattern, replacement); err != nil { + return err + } + + // Remove the coreos.liveiso parameter + if err := editFile(filePath, ` coreos\.liveiso=\S+`, ""); err != nil { + return err + } + + return nil +} + //go:generate mockgen -package=isoeditor -destination=mock_editor.go . Editor type Editor interface { CreateMinimalISOTemplate(fullISOPath, rootFSURL, arch, minimalISOPath, openshiftVersion, nmstatectlPath string) error @@ -150,9 +176,6 @@ func embedInitrdPlaceholders(extractDir string) error { } func fixGrubConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool) error { - if err := validateRootFSURL(rootFSURL); err != nil { - return err - } availableGrubPaths := []string{"EFI/redhat/grub.cfg", "EFI/fedora/grub.cfg", "boot/grub/grub.cfg", "EFI/centos/grub.cfg"} var foundGrubPath string for _, pathSection := range availableGrubPaths { @@ -166,14 +189,13 @@ func fixGrubConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool) err return fmt.Errorf("no grub.cfg found, possible paths are %v", availableGrubPaths) } - // Add the rootfs url - replacement := fmt.Sprintf("$1 $2 coreos.live.rootfs_url=\"%s\"", rootFSURL) - if err := editFile(foundGrubPath, `(?m)^(\s+linux) (.+| )+$`, replacement); err != nil { - return err - } + // Typical grub.cfg lines we're modifying: + // + // linux /images/pxeboot/vmlinuz rw coreos.liveiso=rhcos-9.6.20250523-0 ignition.firstboot ignition.platform.id=metal + // initrd /images/pxeboot/initrd.img /images/ignition.img - // Remove the coreos.liveiso parameter - if err := editFile(foundGrubPath, ` coreos.liveiso=\S+`, ""); err != nil { + // Apply standard kernel argument transformations (remove coreos.liveiso, add rootfs_url) + if err := transformKernelArgs(foundGrubPath, `(?m)^(\s+linux) (.+| )+$`, rootFSURL); err != nil { return err } @@ -191,26 +213,15 @@ func fixGrubConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool) err return nil } -func validateRootFSURL(rootFSURL string) error { - if strings.Contains(rootFSURL, "$") { - return fmt.Errorf("invalid rootfs URL: contains invalid character '$'") - } - if strings.Contains(rootFSURL, "\\") { - return fmt.Errorf("invalid rootfs URL: contains invalid character '\\'") - } - return nil -} - func fixIsolinuxConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool) error { - if err := validateRootFSURL(rootFSURL); err != nil { - return err - } - replacement := fmt.Sprintf("$1 $2 coreos.live.rootfs_url=\"%s\"", rootFSURL) - if err := editFile(filepath.Join(extractDir, "isolinux/isolinux.cfg"), `(?m)^(\s+append) (.+| )+$`, replacement); err != nil { - return err - } + isolinuxPath := filepath.Join(extractDir, "isolinux/isolinux.cfg") - if err := editFile(filepath.Join(extractDir, "isolinux/isolinux.cfg"), ` coreos.liveiso=\S+`, ""); err != nil { + // Typical isolinux.cfg line we're modifying: + // + // append initrd=/images/pxeboot/initrd.img,/images/ignition.img rw coreos.liveiso=rhcos-9.6.20250523-0 ignition.firstboot ignition.platform.id=metal + + // Apply standard kernel argument transformations (remove coreos.liveiso, add rootfs_url) + if err := transformKernelArgs(isolinuxPath, `(?m)^(\s+append) (.+| )+$`, rootFSURL); err != nil { return err } @@ -226,7 +237,6 @@ func fixIsolinuxConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool) return nil } - func editFile(fileName string, reString string, replacement string) error { content, err := os.ReadFile(fileName) if err != nil { diff --git a/pkg/isoeditor/rhcos_test.go b/pkg/isoeditor/rhcos_test.go index 1286ee9f..f5d3129b 100644 --- a/pkg/isoeditor/rhcos_test.go +++ b/pkg/isoeditor/rhcos_test.go @@ -166,7 +166,7 @@ var _ = Context("with test files", func() { It("accepts valid URLs", func() { validURL := "https://example.com/valid/rootfs.img" - err := validateRootFSURL(validURL) + err := fixGrubConfig(validURL, filesDir, false) Expect(err).ToNot(HaveOccurred()) }) }) From 27718819e6c9a9ea6855b17045dd1771d61b552f Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Wed, 10 Dec 2025 23:20:50 +1300 Subject: [PATCH 3/5] Narrow down locus of changes when editing minimal ISO files Introduce editString() function that uses named capture groups to precisely target content for replacement. The (?P) syntax makes it explicit what is being replaced in each regex operation. Convert transformKernelArgs, fixGrubConfig and fixIsolinuxConfig to use editString for their transformations. This provides better control over regex substitutions than full-string replacement. Assisted-by: Cursor Assisted-by: Claude Code --- pkg/isoeditor/rhcos.go | 114 ++++++++++++++++++++++++++++------------- 1 file changed, 78 insertions(+), 36 deletions(-) diff --git a/pkg/isoeditor/rhcos.go b/pkg/isoeditor/rhcos.go index 74ee1918..071e72f4 100644 --- a/pkg/isoeditor/rhcos.go +++ b/pkg/isoeditor/rhcos.go @@ -23,27 +23,31 @@ const ( // transformKernelArgs applies the standard kernel argument transformations: // 1. Remove coreos.liveiso parameter // 2. Add coreos.live.rootfs_url parameter at the specified insertion point -func transformKernelArgs(filePath string, linePattern string, rootFSURL string) error { +func transformKernelArgs(content string, insertionPattern string, rootFSURL string) (string, error) { // Validate rootfs URL if strings.Contains(rootFSURL, "$") { - return fmt.Errorf("invalid rootfs URL: contains invalid character '$'") + return "", fmt.Errorf("invalid rootfs URL: contains invalid character '$'") } if strings.Contains(rootFSURL, "\\") { - return fmt.Errorf("invalid rootfs URL: contains invalid character '\\'") + return "", fmt.Errorf("invalid rootfs URL: contains invalid character '\\'") } - // Add the rootfs_url parameter - replacement := "$1 $2 coreos.live.rootfs_url=\"" + rootFSURL + "\"" - if err := editFile(filePath, linePattern, replacement); err != nil { - return err - } + var err error // Remove the coreos.liveiso parameter - if err := editFile(filePath, ` coreos\.liveiso=\S+`, ""); err != nil { - return err + content, err = editString(content, `\b(?Pcoreos\.liveiso=\S+ ?)`, "") + if err != nil { + return "", err } - return nil + // Add the rootfs_url parameter at the specified insertion point + replacement := " coreos.live.rootfs_url=\"" + rootFSURL + "\"" + content, err = editString(content, insertionPattern, replacement) + if err != nil { + return "", err + } + + return content, nil } //go:generate mockgen -package=isoeditor -destination=mock_editor.go . Editor @@ -189,66 +193,104 @@ func fixGrubConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool) err return fmt.Errorf("no grub.cfg found, possible paths are %v", availableGrubPaths) } + // Read the file content + content, err := os.ReadFile(foundGrubPath) + if err != nil { + return err + } + contentStr := string(content) + // Typical grub.cfg lines we're modifying: // // linux /images/pxeboot/vmlinuz rw coreos.liveiso=rhcos-9.6.20250523-0 ignition.firstboot ignition.platform.id=metal // initrd /images/pxeboot/initrd.img /images/ignition.img // Apply standard kernel argument transformations (remove coreos.liveiso, add rootfs_url) - if err := transformKernelArgs(foundGrubPath, `(?m)^(\s+linux) (.+| )+$`, rootFSURL); err != nil { + contentStr, err = transformKernelArgs(contentStr, `(?m)^(\s+linux .+)(?P$)`, rootFSURL) + if err != nil { return err } - // Edit config to add custom ramdisk image to initrd + // Edit config to add custom ramdisk image to initrd - capture the end-of-line position to append our images + var initrdReplacement string if includeNmstateRamDisk { - if err := editFile(foundGrubPath, `(?m)^(\s+initrd) (.+| )+$`, fmt.Sprintf("$1 $2 %s %s", ramDiskImagePath, nmstateDiskImagePath)); err != nil { - return err - } + initrdReplacement = " " + ramDiskImagePath + " " + nmstateDiskImagePath } else { - if err := editFile(foundGrubPath, `(?m)^(\s+initrd) (.+| )+$`, fmt.Sprintf("$1 $2 %s", ramDiskImagePath)); err != nil { - return err - } + initrdReplacement = " " + ramDiskImagePath + } + contentStr, err = editString(contentStr, `(?m)^(\s+initrd .+)(?P$)`, initrdReplacement) + if err != nil { + return err } - return nil + // Write the modified content back to the file + return os.WriteFile(foundGrubPath, []byte(contentStr), 0600) } func fixIsolinuxConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool) error { isolinuxPath := filepath.Join(extractDir, "isolinux/isolinux.cfg") + // Read the file content + content, err := os.ReadFile(isolinuxPath) + if err != nil { + return err + } + contentStr := string(content) + // Typical isolinux.cfg line we're modifying: // // append initrd=/images/pxeboot/initrd.img,/images/ignition.img rw coreos.liveiso=rhcos-9.6.20250523-0 ignition.firstboot ignition.platform.id=metal // Apply standard kernel argument transformations (remove coreos.liveiso, add rootfs_url) - if err := transformKernelArgs(isolinuxPath, `(?m)^(\s+append) (.+| )+$`, rootFSURL); err != nil { + contentStr, err = transformKernelArgs(contentStr, `(?m)^(\s+append .+)(?P$)`, rootFSURL) + if err != nil { return err } + // Add ramdisk images to initrd specification - capture the position right after the initrd argument to append our images + var initrdReplacement string if includeNmstateRamDisk { - if err := editFile(filepath.Join(extractDir, "isolinux/isolinux.cfg"), `(?m)^(\s+append.*initrd=\S+) (.*)$`, fmt.Sprintf("${1},%s,%s ${2}", ramDiskImagePath, nmstateDiskImagePath)); err != nil { - return err - } + initrdReplacement = "," + ramDiskImagePath + "," + nmstateDiskImagePath } else { - if err := editFile(filepath.Join(extractDir, "isolinux/isolinux.cfg"), `(?m)^(\s+append.*initrd=\S+) (.*)$`, fmt.Sprintf("${1},%s ${2}", ramDiskImagePath)); err != nil { - return err - } + initrdReplacement = "," + ramDiskImagePath } - - return nil -} -func editFile(fileName string, reString string, replacement string) error { - content, err := os.ReadFile(fileName) + contentStr, err = editString(contentStr, `(?m)^(\s+append.*initrd=\S+)(?P)`, initrdReplacement) if err != nil { return err } + // Write the modified content back to the file + return os.WriteFile(isolinuxPath, []byte(contentStr), 0600) +} + +// editString applies a regex replacement to a string and returns the modified string +// It looks for a named capture group called "replace" and replaces only that content, using precise string manipulation +func editString(content string, reString string, replacement string) (string, error) { re := regexp.MustCompile(reString) - newContent := re.ReplaceAllString(string(content), replacement) - if err := os.WriteFile(fileName, []byte(newContent), 0600); err != nil { - return err + // Get the index of the "replace" named capture group + replaceIndex := re.SubexpIndex("replace") + if replaceIndex == -1 { + return "", fmt.Errorf("regex must have a named capture group called 'replace'") } - return nil + // Find the first match with subgroups + submatchIndexes := re.FindStringSubmatchIndex(content) + if submatchIndexes == nil { + // No match found + return content, nil + } + + // submatchIndexes contains [fullMatchStart, fullMatchEnd, group1Start, group1End, group2Start, group2End, ...] + if len(submatchIndexes) < (replaceIndex+1)*2 { + return "", fmt.Errorf("regex match does not contain the 'replace' capture group") + } + + replaceStart := submatchIndexes[replaceIndex*2] + replaceEnd := submatchIndexes[replaceIndex*2+1] + + // Replace only the "replace" capturing group + newContent := content[:replaceStart] + replacement + content[replaceEnd:] + + return newContent, nil } From 59446968b2b9d1e28fbea22e2fa73260b239336b Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Wed, 10 Dec 2025 23:52:53 +1300 Subject: [PATCH 4/5] Update default kargs in minimal ISO Read and update kargs.json metadata that coreos-installer uses to manage kernel arguments in the ISO. When we modify the default kernel arguments (removing coreos.liveiso and adding coreos.live.rootfs_url), update kargs.json to reflect these changes. This ensures coreos-installer can correctly edit kernel arguments after the ISO is created. Assisted-by: Cursor Assisted-by: Claude Code --- pkg/isoeditor/rhcos.go | 106 +++++++++++++++++++++++++++---- pkg/isoeditor/rhcos_test.go | 122 +++++++++++++++++++++++++++++++++--- 2 files changed, 206 insertions(+), 22 deletions(-) diff --git a/pkg/isoeditor/rhcos.go b/pkg/isoeditor/rhcos.go index 071e72f4..40f9f762 100644 --- a/pkg/isoeditor/rhcos.go +++ b/pkg/isoeditor/rhcos.go @@ -1,6 +1,7 @@ package isoeditor import ( + "encoding/json" "fmt" "os" "path/filepath" @@ -83,19 +84,11 @@ func CreateMinimalISO(extractDir, volumeID, rootFSURL, arch, minimalISOPath stri includeNmstateRamDisk = true } - if err := fixGrubConfig(rootFSURL, extractDir, includeNmstateRamDisk); err != nil { - log.WithError(err).Warnf("Failed to edit grub config") + if err := updateKargs(extractDir, rootFSURL, includeNmstateRamDisk, arch); err != nil { + log.WithError(err).Warnf("Failed to update kargs offsets and sizes") return err } - // ignore isolinux.cfg for ppc64le because it doesn't exist - if arch != "ppc64le" { - if err := fixIsolinuxConfig(rootFSURL, extractDir, includeNmstateRamDisk); err != nil { - log.WithError(err).Warnf("Failed to edit isolinux config") - return err - } - } - if err := Create(minimalISOPath, extractDir, volumeID); err != nil { return err } @@ -179,7 +172,8 @@ func embedInitrdPlaceholders(extractDir string) error { return nil } -func fixGrubConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool) error { +// fixGrubConfig modifies grub.cfg +func fixGrubConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool, kargs *kargsConfig) error { availableGrubPaths := []string{"EFI/redhat/grub.cfg", "EFI/fedora/grub.cfg", "boot/grub/grub.cfg", "EFI/centos/grub.cfg"} var foundGrubPath string for _, pathSection := range availableGrubPaths { @@ -227,8 +221,10 @@ func fixGrubConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool) err return os.WriteFile(foundGrubPath, []byte(contentStr), 0600) } -func fixIsolinuxConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool) error { - isolinuxPath := filepath.Join(extractDir, "isolinux/isolinux.cfg") +// fixIsolinuxConfig modifies isolinux.cfg +func fixIsolinuxConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool, kargs *kargsConfig) error { + relativeIsolinuxPath := strings.TrimPrefix(defaultIsolinuxFilePath, "/") + isolinuxPath := filepath.Join(extractDir, relativeIsolinuxPath) // Read the file content content, err := os.ReadFile(isolinuxPath) @@ -294,3 +290,87 @@ func editString(content string, reString string, replacement string) (string, er return newContent, nil } + +type kargsFileEntry struct { + End *string `json:"end"` + Offset *int64 `json:"offset"` + Pad *string `json:"pad"` + Path *string `json:"path"` +} + +type kargsConfig struct { + Default string `json:"default"` + Files []kargsFileEntry `json:"files"` + Size int64 `json:"size"` +} + +// updateDefaultKargs modifies the default kernel arguments to match bootloader modifications +// and applies the embed area size change directly to config.Size +func updateDefaultKargs(config *kargsConfig, rootFSURL string) error { + originalDefault := config.Default + + // Apply the same transformations we make to bootloader configs + // For default kargs, we append at the end (using $ insertion pattern) + var err error + config.Default, err = transformKernelArgs(config.Default, `(?P$)`, rootFSURL) + if err != nil { + return err + } + + // Calculate and apply the embed area size change from the default kargs transformation + embedSizeChange := int64(len(config.Default)) - int64(len(originalDefault)) + config.Size += embedSizeChange + + return nil +} + +// updateKargs reads kargs.json, applies bootloader config fixes, and updates kargs.json +func updateKargs(extractDir, rootFSURL string, includeNmstateRamDisk bool, arch string) error { + kargsPath := filepath.Join(extractDir, "coreos/kargs.json") + + var config *kargsConfig + + if _, err := os.Stat(kargsPath); !os.IsNotExist(err) { + kargsData, err := os.ReadFile(kargsPath) + if err != nil { + return fmt.Errorf("failed to read kargs.json: %v", err) + } + + var kargsStruct kargsConfig + if err := json.Unmarshal(kargsData, &kargsStruct); err != nil { + return fmt.Errorf("failed to unmarshal kargs.json: %v", err) + } + config = &kargsStruct + } + + // Apply bootloader config changes + if err := fixGrubConfig(rootFSURL, extractDir, includeNmstateRamDisk, config); err != nil { + return fmt.Errorf("failed to fix grub config: %v", err) + } + + // ignore isolinux.cfg for ppc64le because it doesn't exist + if arch != "ppc64le" { + if err := fixIsolinuxConfig(rootFSURL, extractDir, includeNmstateRamDisk, config); err != nil { + return fmt.Errorf("failed to fix isolinux config: %v", err) + } + } + + if config != nil { + // Update the default kernel arguments and apply embed area size change + if err := updateDefaultKargs(config, rootFSURL); err != nil { + return fmt.Errorf("failed to update default kargs: %v", err) + } + + updatedData, err := json.MarshalIndent(*config, "", " ") + if err != nil { + return fmt.Errorf("failed to marshal updated kargs.json: %v", err) + } + + if err := os.WriteFile(kargsPath, updatedData, 0600); err != nil { + return fmt.Errorf("failed to write updated kargs.json: %v", err) + } + } + + return nil +} + diff --git a/pkg/isoeditor/rhcos_test.go b/pkg/isoeditor/rhcos_test.go index f5d3129b..4e90813a 100644 --- a/pkg/isoeditor/rhcos_test.go +++ b/pkg/isoeditor/rhcos_test.go @@ -1,6 +1,7 @@ package isoeditor import ( + "encoding/json" "fmt" "os" "path/filepath" @@ -95,7 +96,8 @@ var _ = Context("with test files", func() { Describe("Fix Config", func() { Context("with including nmstate disk image", func() { It("fixGrubConfig alters the kernel parameters correctly", func() { - err := fixGrubConfig(testRootFSURL, filesDir, true) + // Pass nil for kargs since we're just testing file changes + err := fixGrubConfig(testRootFSURL, filesDir, true, nil) Expect(err).ToNot(HaveOccurred()) newLine := " linux /images/pxeboot/vmlinuz random.trust_cpu=on rd.luks.options=discard ignition.firstboot ignition.platform.id=metal coreos.live.rootfs_url=\"%s\"" @@ -108,7 +110,8 @@ var _ = Context("with test files", func() { }) It("fixIsolinuxConfig alters the kernel parameters correctly", func() { - err := fixIsolinuxConfig(testRootFSURL, filesDir, true) + // Pass nil for kargs since we're just testing file changes + err := fixIsolinuxConfig(testRootFSURL, filesDir, true, nil) Expect(err).ToNot(HaveOccurred()) newLine := " append initrd=/images/pxeboot/initrd.img,/images/ignition.img,%s,%s random.trust_cpu=on rd.luks.options=discard ignition.firstboot ignition.platform.id=metal coreos.live.rootfs_url=\"%s\"" @@ -119,7 +122,8 @@ var _ = Context("with test files", func() { Context("without including nmstate disk image", func() { It("fixGrubConfig alters the kernel parameters correctly", func() { - err := fixGrubConfig(testRootFSURL, filesDir, false) + // Pass nil for kargs since we're just testing file changes + err := fixGrubConfig(testRootFSURL, filesDir, false, nil) Expect(err).ToNot(HaveOccurred()) newLine := " linux /images/pxeboot/vmlinuz random.trust_cpu=on rd.luks.options=discard ignition.firstboot ignition.platform.id=metal coreos.live.rootfs_url=\"%s\"" @@ -132,7 +136,8 @@ var _ = Context("with test files", func() { }) It("fixIsolinuxConfig alters the kernel parameters correctly", func() { - err := fixIsolinuxConfig(testRootFSURL, filesDir, false) + // Pass nil for kargs since we're just testing file changes + err := fixIsolinuxConfig(testRootFSURL, filesDir, false, nil) Expect(err).ToNot(HaveOccurred()) newLine := " append initrd=/images/pxeboot/initrd.img,/images/ignition.img,%s random.trust_cpu=on rd.luks.options=discard ignition.firstboot ignition.platform.id=metal coreos.live.rootfs_url=\"%s\"" @@ -144,31 +149,130 @@ var _ = Context("with test files", func() { Context("URL validation", func() { It("rejects URLs containing $ character", func() { invalidURL := "https://example.com/test$invalid/rootfs.img" - err := fixGrubConfig(invalidURL, filesDir, false) + err := fixGrubConfig(invalidURL, filesDir, false, nil) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("invalid rootfs URL: contains invalid character '$'")) - err = fixIsolinuxConfig(invalidURL, filesDir, false) + err = fixIsolinuxConfig(invalidURL, filesDir, false, nil) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("invalid rootfs URL: contains invalid character '$'")) }) It("rejects URLs containing \\ character", func() { invalidURL := "https://example.com/test\\invalid/rootfs.img" - err := fixGrubConfig(invalidURL, filesDir, false) + err := fixGrubConfig(invalidURL, filesDir, false, nil) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("invalid rootfs URL: contains invalid character '\\'")) - err = fixIsolinuxConfig(invalidURL, filesDir, false) + err = fixIsolinuxConfig(invalidURL, filesDir, false, nil) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("invalid rootfs URL: contains invalid character '\\'")) }) It("accepts valid URLs", func() { validURL := "https://example.com/valid/rootfs.img" - err := fixGrubConfig(validURL, filesDir, false) + err := fixGrubConfig(validURL, filesDir, false, nil) Expect(err).ToNot(HaveOccurred()) }) }) + + Context("editString function", func() { + It("replaces content in named capture group", func() { + content := "line1\nline2\nline3" + newContent, err := editString(content, `(?Pline2)`, "modified") + Expect(err).ToNot(HaveOccurred()) + Expect(newContent).To(Equal("line1\nmodified\nline3")) + }) + + It("appends content when replace group is at end", func() { + content := "some text" + newContent, err := editString(content, `(some text)(?P$)`, " more") + Expect(err).ToNot(HaveOccurred()) + Expect(newContent).To(Equal("some text more")) + }) + + It("returns error if no replace group", func() { + content := "some text" + _, err := editString(content, `some text`, "replacement") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("must have a named capture group called 'replace'")) + }) + + It("returns original content if no match", func() { + content := "some text" + newContent, err := editString(content, `(?Pnomatch)`, "replacement") + Expect(err).ToNot(HaveOccurred()) + Expect(newContent).To(Equal(content)) + }) + }) + + Context("kargs.json handling", func() { + It("successfully round-trips kargs.json", func() { + // Create a temporary directory with a kargs.json file + tmpDir, err := os.MkdirTemp("", "kargs-test") + Expect(err).ToNot(HaveOccurred()) + defer os.RemoveAll(tmpDir) + + kargsDir := filepath.Join(tmpDir, "coreos") + err = os.MkdirAll(kargsDir, 0755) + Expect(err).ToNot(HaveOccurred()) + + // Create EFI/fedora and isolinux directories + err = os.MkdirAll(filepath.Join(tmpDir, "EFI/fedora"), 0755) + Expect(err).ToNot(HaveOccurred()) + err = os.MkdirAll(filepath.Join(tmpDir, "isolinux"), 0755) + Expect(err).ToNot(HaveOccurred()) + + // Create grub.cfg + grubConfig := "\tlinux /images/pxeboot/vmlinuz random.trust_cpu=on rd.luks.options=discard ignition.firstboot ignition.platform.id=metal coreos.liveiso=rhcos-416.94.202404301731-0\n\tinitrd /images/pxeboot/initrd.img /images/ignition.img\n" + err = os.WriteFile(filepath.Join(tmpDir, "EFI/fedora/grub.cfg"), []byte(grubConfig), 0600) + Expect(err).ToNot(HaveOccurred()) + + // Create isolinux.cfg + isolinuxConfig := " append initrd=/images/pxeboot/initrd.img,/images/ignition.img random.trust_cpu=on rd.luks.options=discard ignition.firstboot ignition.platform.id=metal coreos.liveiso=rhcos-416.94.202404301731-0\n" + err = os.WriteFile(filepath.Join(tmpDir, "isolinux/isolinux.cfg"), []byte(isolinuxConfig), 0600) + Expect(err).ToNot(HaveOccurred()) + + kargsPath := filepath.Join(kargsDir, "kargs.json") + originalJSON := `{ + "default": "random.trust_cpu=on rd.luks.options=discard ignition.firstboot ignition.platform.id=metal coreos.liveiso=rhcos-416.94.202404301731-0", + "files": [ + { + "path": "EFI/fedora/grub.cfg", + "offset": 1000, + "size": 2000 + } + ], + "size": 1024 +}` + err = os.WriteFile(kargsPath, []byte(originalJSON), 0600) + Expect(err).ToNot(HaveOccurred()) + + // Call updateKargs with a rootFS URL (no nmstate, x86_64 arch) + err = updateKargs(tmpDir, testRootFSURL, false, "x86_64") + Expect(err).ToNot(HaveOccurred()) + + // Read the file back and verify it's valid JSON + updatedData, err := os.ReadFile(kargsPath) + Expect(err).ToNot(HaveOccurred()) + + var config kargsConfig + err = json.Unmarshal(updatedData, &config) + Expect(err).ToNot(HaveOccurred()) + + // Verify the default kargs were updated: + // - coreos.liveiso parameter should be removed + // - coreos.live.rootfs_url should be added + Expect(config.Default).To(ContainSubstring("coreos.live.rootfs_url")) + Expect(config.Default).To(ContainSubstring(testRootFSURL)) + Expect(config.Default).ToNot(ContainSubstring("coreos.liveiso")) + + // Verify size was updated to reflect the change in default kargs length + // Removed: "coreos.liveiso=rhcos-416.94.202404301731-0 " (43 chars) + // Added: " coreos.live.rootfs_url=\"https://example.com/pub/openshift-v4/dependencies/rhcos/4.7/4.7.7/rhcos-live-rootfs.x86_64.img\"" (121 chars) + // Net change: 121 - 43 = 78 chars + Expect(config.Size).To(Equal(int64(1024 + 78))) + }) + }) }) }) From b139cee14eef10f38be543b220608d1554459e6c Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Wed, 10 Dec 2025 23:53:12 +1300 Subject: [PATCH 5/5] OCPBUGS-43501: Set correct kargs.json offsets in minimal ISO When modifying kernel arguments in grub.cfg or isolinux.cfg, track how these changes shift the position of the kargs embed area. Update the offsets in kargs.json so that coreos-installer can find the correct embed area after our modifications. Without this fix, adding initramfs images in isolinux.cfg can shift the kargs embed area, causing coreos-installer kargs edits to fail or corrupt the file. Assisted-by: Cursor Assisted-by: Claude Code --- pkg/isoeditor/rhcos.go | 79 ++++++++++++++++++++++++++++--------- pkg/isoeditor/rhcos_test.go | 73 ++++++++++++++++++++++++++++++++-- 2 files changed, 130 insertions(+), 22 deletions(-) diff --git a/pkg/isoeditor/rhcos.go b/pkg/isoeditor/rhcos.go index 40f9f762..d645f409 100644 --- a/pkg/isoeditor/rhcos.go +++ b/pkg/isoeditor/rhcos.go @@ -24,7 +24,7 @@ const ( // transformKernelArgs applies the standard kernel argument transformations: // 1. Remove coreos.liveiso parameter // 2. Add coreos.live.rootfs_url parameter at the specified insertion point -func transformKernelArgs(content string, insertionPattern string, rootFSURL string) (string, error) { +func transformKernelArgs(content string, insertionPattern string, rootFSURL string, fileEntry *kargsFileEntry) (string, error) { // Validate rootfs URL if strings.Contains(rootFSURL, "$") { return "", fmt.Errorf("invalid rootfs URL: contains invalid character '$'") @@ -36,14 +36,14 @@ func transformKernelArgs(content string, insertionPattern string, rootFSURL stri var err error // Remove the coreos.liveiso parameter - content, err = editString(content, `\b(?Pcoreos\.liveiso=\S+ ?)`, "") + content, err = editString(content, `\b(?Pcoreos\.liveiso=\S+ ?)`, "", fileEntry) if err != nil { return "", err } // Add the rootfs_url parameter at the specified insertion point replacement := " coreos.live.rootfs_url=\"" + rootFSURL + "\"" - content, err = editString(content, insertionPattern, replacement) + content, err = editString(content, insertionPattern, replacement, fileEntry) if err != nil { return "", err } @@ -172,14 +172,16 @@ func embedInitrdPlaceholders(extractDir string) error { return nil } -// fixGrubConfig modifies grub.cfg +// fixGrubConfig modifies grub.cfg and updates kargs config in place func fixGrubConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool, kargs *kargsConfig) error { availableGrubPaths := []string{"EFI/redhat/grub.cfg", "EFI/fedora/grub.cfg", "boot/grub/grub.cfg", "EFI/centos/grub.cfg"} var foundGrubPath string + var fileEntry *kargsFileEntry for _, pathSection := range availableGrubPaths { path := filepath.Join(extractDir, pathSection) if _, err := os.Stat(path); err == nil { foundGrubPath = path + fileEntry = kargs.FindFileByPath(pathSection) break } } @@ -200,7 +202,7 @@ func fixGrubConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool, kar // initrd /images/pxeboot/initrd.img /images/ignition.img // Apply standard kernel argument transformations (remove coreos.liveiso, add rootfs_url) - contentStr, err = transformKernelArgs(contentStr, `(?m)^(\s+linux .+)(?P$)`, rootFSURL) + contentStr, err = transformKernelArgs(contentStr, `(?m)^(\s+linux .+)(?P$)`, rootFSURL, fileEntry) if err != nil { return err } @@ -212,7 +214,7 @@ func fixGrubConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool, kar } else { initrdReplacement = " " + ramDiskImagePath } - contentStr, err = editString(contentStr, `(?m)^(\s+initrd .+)(?P$)`, initrdReplacement) + contentStr, err = editString(contentStr, `(?m)^(\s+initrd .+)(?P$)`, initrdReplacement, fileEntry) if err != nil { return err } @@ -221,7 +223,7 @@ func fixGrubConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool, kar return os.WriteFile(foundGrubPath, []byte(contentStr), 0600) } -// fixIsolinuxConfig modifies isolinux.cfg +// fixIsolinuxConfig modifies isolinux.cfg and updates kargs config in place func fixIsolinuxConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool, kargs *kargsConfig) error { relativeIsolinuxPath := strings.TrimPrefix(defaultIsolinuxFilePath, "/") isolinuxPath := filepath.Join(extractDir, relativeIsolinuxPath) @@ -237,8 +239,11 @@ func fixIsolinuxConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool, // // append initrd=/images/pxeboot/initrd.img,/images/ignition.img rw coreos.liveiso=rhcos-9.6.20250523-0 ignition.firstboot ignition.platform.id=metal + // Find the kargs file entry for this file + fileEntry := kargs.FindFileByPath(relativeIsolinuxPath) + // Apply standard kernel argument transformations (remove coreos.liveiso, add rootfs_url) - contentStr, err = transformKernelArgs(contentStr, `(?m)^(\s+append .+)(?P$)`, rootFSURL) + contentStr, err = transformKernelArgs(contentStr, `(?m)^(\s+append .+)(?P$)`, rootFSURL, fileEntry) if err != nil { return err } @@ -250,7 +255,7 @@ func fixIsolinuxConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool, } else { initrdReplacement = "," + ramDiskImagePath } - contentStr, err = editString(contentStr, `(?m)^(\s+append.*initrd=\S+)(?P)`, initrdReplacement) + contentStr, err = editString(contentStr, `(?m)^(\s+append.*initrd=\S+)(?P)`, initrdReplacement, fileEntry) if err != nil { return err } @@ -261,7 +266,7 @@ func fixIsolinuxConfig(rootFSURL, extractDir string, includeNmstateRamDisk bool, // editString applies a regex replacement to a string and returns the modified string // It looks for a named capture group called "replace" and replaces only that content, using precise string manipulation -func editString(content string, reString string, replacement string) (string, error) { +func editString(content string, reString string, replacement string, fileEntry *kargsFileEntry) (string, error) { re := regexp.MustCompile(reString) // Get the index of the "replace" named capture group @@ -288,14 +293,38 @@ func editString(content string, reString string, replacement string) (string, er // Replace only the "replace" capturing group newContent := content[:replaceStart] + replacement + content[replaceEnd:] + if content == newContent { + return content, nil + } + + if fileEntry == nil || fileEntry.Offset == nil { + return newContent, nil + } + + embedStart := *fileEntry.Offset + fileSizeChange := int64(len(newContent)) - int64(len(content)) + + replaceStartPos := int64(replaceStart) + replaceEndPos := int64(replaceEnd) + + // Add boundary crossing check to ensure no replacements span across the embed area start boundary + if replaceStartPos < embedStart && replaceEndPos > embedStart { + return "", fmt.Errorf("replacement spans across embed area boundary (replace: %d-%d, embed starts: %d)", replaceStartPos, replaceEndPos, embedStart) + } + + if replaceEndPos <= embedStart { + // Change is before embed area - affects offset + *fileEntry.Offset += fileSizeChange + } + return newContent, nil } type kargsFileEntry struct { - End *string `json:"end"` - Offset *int64 `json:"offset"` - Pad *string `json:"pad"` - Path *string `json:"path"` + End *string `json:"end,omitempty"` + Offset *int64 `json:"offset,omitempty"` + Pad *string `json:"pad,omitempty"` + Path *string `json:"path,omitempty"` } type kargsConfig struct { @@ -304,6 +333,20 @@ type kargsConfig struct { Size int64 `json:"size"` } +// FindFileByPath searches for a file entry by its path and returns a pointer to it. +// Returns nil if no file with the specified path is found. +func (k *kargsConfig) FindFileByPath(path string) *kargsFileEntry { + if k == nil { + return nil + } + for i := range k.Files { + if k.Files[i].Path != nil && *k.Files[i].Path == path { + return &k.Files[i] + } + } + return nil +} + // updateDefaultKargs modifies the default kernel arguments to match bootloader modifications // and applies the embed area size change directly to config.Size func updateDefaultKargs(config *kargsConfig, rootFSURL string) error { @@ -311,8 +354,9 @@ func updateDefaultKargs(config *kargsConfig, rootFSURL string) error { // Apply the same transformations we make to bootloader configs // For default kargs, we append at the end (using $ insertion pattern) + // Pass nil for fileEntry since we don't track offsets for the default string var err error - config.Default, err = transformKernelArgs(config.Default, `(?P$)`, rootFSURL) + config.Default, err = transformKernelArgs(config.Default, `(?P$)`, rootFSURL, nil) if err != nil { return err } @@ -324,7 +368,7 @@ func updateDefaultKargs(config *kargsConfig, rootFSURL string) error { return nil } -// updateKargs reads kargs.json, applies bootloader config fixes, and updates kargs.json +// updateKargs reads kargs.json, applies fixes with embed area awareness, and updates kargs.json func updateKargs(extractDir, rootFSURL string, includeNmstateRamDisk bool, arch string) error { kargsPath := filepath.Join(extractDir, "coreos/kargs.json") @@ -343,7 +387,7 @@ func updateKargs(extractDir, rootFSURL string, includeNmstateRamDisk bool, arch config = &kargsStruct } - // Apply bootloader config changes + // Apply bootloader config changes (without tracking size changes) if err := fixGrubConfig(rootFSURL, extractDir, includeNmstateRamDisk, config); err != nil { return fmt.Errorf("failed to fix grub config: %v", err) } @@ -373,4 +417,3 @@ func updateKargs(extractDir, rootFSURL string, includeNmstateRamDisk bool, arch return nil } - diff --git a/pkg/isoeditor/rhcos_test.go b/pkg/isoeditor/rhcos_test.go index 4e90813a..c418fd26 100644 --- a/pkg/isoeditor/rhcos_test.go +++ b/pkg/isoeditor/rhcos_test.go @@ -179,33 +179,98 @@ var _ = Context("with test files", func() { Context("editString function", func() { It("replaces content in named capture group", func() { content := "line1\nline2\nline3" - newContent, err := editString(content, `(?Pline2)`, "modified") + newContent, err := editString(content, `(?Pline2)`, "modified", nil) Expect(err).ToNot(HaveOccurred()) Expect(newContent).To(Equal("line1\nmodified\nline3")) }) It("appends content when replace group is at end", func() { content := "some text" - newContent, err := editString(content, `(some text)(?P$)`, " more") + newContent, err := editString(content, `(some text)(?P$)`, " more", nil) Expect(err).ToNot(HaveOccurred()) Expect(newContent).To(Equal("some text more")) }) It("returns error if no replace group", func() { content := "some text" - _, err := editString(content, `some text`, "replacement") + _, err := editString(content, `some text`, "replacement", nil) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("must have a named capture group called 'replace'")) }) It("returns original content if no match", func() { content := "some text" - newContent, err := editString(content, `(?Pnomatch)`, "replacement") + newContent, err := editString(content, `(?Pnomatch)`, "replacement", nil) Expect(err).ToNot(HaveOccurred()) Expect(newContent).To(Equal(content)) }) }) + Context("editString offset tracking", func() { + It("updates offset when replacement is before embed area", func() { + content := "prefix some text suffix" + offset := int64(13) // Points to "suffix" + fileEntry := &kargsFileEntry{Offset: &offset} + + // Replace "some" (before offset) with "LONGER" + newContent, err := editString(content, `(?Psome)`, "LONGER", fileEntry) + Expect(err).ToNot(HaveOccurred()) + Expect(newContent).To(Equal("prefix LONGER text suffix")) + // Offset should increase by 2 (6 - 4 = 2) + Expect(*fileEntry.Offset).To(Equal(int64(15))) + }) + + It("does not update offset when replacement is after embed area", func() { + content := "prefix some text suffix" + offset := int64(7) // Points to "some" + fileEntry := &kargsFileEntry{Offset: &offset} + + // Replace "suffix" (after offset) with "END" + newContent, err := editString(content, `(?Psuffix)`, "END", fileEntry) + Expect(err).ToNot(HaveOccurred()) + Expect(newContent).To(Equal("prefix some text END")) + // Offset should not change + Expect(*fileEntry.Offset).To(Equal(int64(7))) + }) + + It("updates offset when shrinking content before embed area", func() { + content := "prefix LONGER text suffix" + offset := int64(14) // Points to "text" + fileEntry := &kargsFileEntry{Offset: &offset} + + // Replace "LONGER" with "some" + newContent, err := editString(content, `(?PLONGER)`, "some", fileEntry) + Expect(err).ToNot(HaveOccurred()) + Expect(newContent).To(Equal("prefix some text suffix")) + // Offset should decrease by 2 (4 - 6 = -2) + Expect(*fileEntry.Offset).To(Equal(int64(12))) + }) + + It("returns error when replacement spans embed area boundary", func() { + content := "prefix some text suffix" + offset := int64(10) // Points to middle of "some text" + fileEntry := &kargsFileEntry{Offset: &offset} + + // Try to replace "some text" which spans across the offset + _, err := editString(content, `(?Psome text)`, "REPLACEMENT", fileEntry) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("replacement spans across embed area boundary")) + }) + + It("handles no change gracefully", func() { + content := "some text" + offset := int64(5) + fileEntry := &kargsFileEntry{Offset: &offset} + + // Replace with same content + newContent, err := editString(content, `(?Psome)`, "some", fileEntry) + Expect(err).ToNot(HaveOccurred()) + Expect(newContent).To(Equal(content)) + // Offset should not change + Expect(*fileEntry.Offset).To(Equal(int64(5))) + }) + }) + Context("kargs.json handling", func() { It("successfully round-trips kargs.json", func() { // Create a temporary directory with a kargs.json file