embed manifest in ImageBuild#117
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughReplaces external ConfigMap-based AIB manifest with inline Changes
Sequence DiagramsequenceDiagram
participant User as User/Client
participant API as ImageBuild API
participant Server as Build API Server
participant Controller as ImageBuild Controller
participant K8s as Kubernetes
participant Pipeline as PipelineRun
User->>API: Submit ImageBuild (spec.aib.manifest, manifestFileName, customDefs, aibExtraArgs)
API->>Server: buildAIBSpec(req, manifest, manifestFileName)
Server-->>API: AIBSpec {Manifest, ManifestFileName, CustomDefs, AIBExtraArgs}
Controller->>Controller: Reconcile ImageBuild
Controller->>K8s: createOrUpdateManifestConfigMap(imageBuild) (manifest file, customDefs, aibExtraArgs)
K8s-->>Controller: ConfigMap created/updated
Controller->>Pipeline: Create PipelineRun referencing manifest ConfigMap workspace
Pipeline->>K8s: Run build pods/jobs using manifest file from ConfigMap
K8s-->>Pipeline: Build job/pods execute (logs, results)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/buildapi/server.go (1)
1672-1711:⚠️ Potential issue | 🟡 Minor
getBuildTemplatediscardsCustomDefsandAIBExtraArgsfrom the stored build.
aibExtra(line 1678) is declared but never assigned, resulting in a nilAIBExtraArgsin the response. Similarly,CustomDefsis hardcoded tonil(line 1706). If the template endpoint is meant to produce a re-usable build request, both values should be populated from the stored spec.Proposed fix
- var aibExtra []string - var sourceFiles []string for _, line := range strings.Split(manifest, "\n") { ... writeJSON(c, http.StatusOK, BuildTemplateResponse{ BuildRequest: BuildRequest{ ... - CustomDefs: nil, - AIBExtraArgs: aibExtra, + CustomDefs: build.Spec.GetCustomDefs(), + AIBExtraArgs: build.Spec.GetAIBExtraArgs(), ... }, SourceFiles: sourceFiles, })
🤖 Fix all issues with AI agents
In `@cmd/caib/main.go`:
- Around line 1327-1344: Move the sanitizeBuildName function so it does not
split sanitizeFilename's doc comment (place sanitizeBuildName above the
sanitizeFilename doc block or elsewhere outside that comment), and replace the
inline regexp.MustCompile(`-{2,}`) inside sanitizeBuildName with a package-level
precompiled variable (e.g., declare var multiHyphenRe =
regexp.MustCompile(`-{2,}`) near the other package vars/consts) and use
multiHyphenRe.ReplaceAllString(...) in sanitizeBuildName so the regex is
compiled once.
In `@internal/controller/imagebuild/controller.go`:
- Line 816: The derived resource name generation can exceed Kubernetes' 253-char
limit when appending suffixes (e.g. configMapName := fmt.Sprintf("%s-manifest",
imageBuild.Name)); instead add a helper to produce safe derived names that
truncates imageBuild.Name to (253 - len(suffix)) chars and appends a short hash
of the full name to preserve uniqueness, then use that helper for all derived
names (configMapName, upload pod, build-*, etc.); update callers that currently
rely on raw imageBuild.Name and keep validateBuildName as-is or tighten it
later, but ensure configMapName and other formatted names are produced via the
new helper so final lengths never exceed 253.
- Around line 809-857: createOrUpdateManifestConfigMap writes the inline
manifest (imageBuild.Spec.GetManifest()) into a ConfigMap (named configMapName)
but Kubernetes/etcd enforces ~1MB object limits while the API MaxManifestSize
allows up to 10MB, so large manifests will cause opaque apiserver/etcd failures;
fix by detecting manifest size in createOrUpdateManifestConfigMap (or earlier
during validation) and if it exceeds a configurable CONFIGMAP_MAX_BYTES
threshold (≤ etcd limit) either 1) reject the request / lower the API
MaxManifestSize to that threshold, or 2) fall back to a different backing store
(e.g., persist to the existing PVC-based approach or another storage backend)
and update the ImageBuild to reference that storage instead; implement the check
against imageBuild.Spec.GetManifest() and branch to the alternate storage path
(or return a clear validation error) so ConfigMap creation of configMapName
never exceeds the etcd size limit.
🧹 Nitpick comments (5)
cmd/caib/main_test.go (1)
228-258: Consider adding an empty-string edge case.Good coverage of the sanitization rules. One missing scenario:
sanitizeBuildName("")returns"", which would produce a build name like"-20260215-120000"(leading hyphen — invalid RFC 1123). While callers currently derive the input fromfilepath.Base()(never empty for valid paths), an explicit test documents the boundary behavior.Suggested addition
{"already-valid-name", "already-valid-name"}, {"under_score.and.dots", "under-score-and-dots"}, + {"", ""}, + {"---", ""}, }api/v1alpha1/imagebuild_types_test.go (1)
154-202: Consider adding an empty-slice test case forGetAIBExtraArgs.
TestGetCustomDefsincludes a "returns empty slice when custom defs is empty" case, butTestGetAIBExtraArgsdoesn't have an analogous case. Adding one would keep parity and ensure the accessor correctly returns[]string{}vsnil.Proposed additional test case
{ name: "returns nil when extra args is nil", spec: ImageBuildSpec{ AIB: &AIBSpec{}, }, want: nil, }, + { + name: "returns empty slice when extra args is empty", + spec: ImageBuildSpec{ + AIB: &AIBSpec{ + AIBExtraArgs: []string{}, + }, + }, + want: []string{}, + }, }internal/buildapi/build_aib_spec_test.go (1)
131-155: Nil/empty CustomDefs and AIBExtraArgs are not verified in the "basic" and "empty manifest filename" cases.When
wantCustomDefsis nil (zero value), thelen(tt.wantCustomDefs) > 0guard skips verification entirely. IfbuildAIBSpecincorrectly populates these fields, the test won't catch it.Proposed fix: verify nil/empty cases too
- // Check custom defs - if len(tt.wantCustomDefs) > 0 { + // Check custom defs + if len(tt.wantCustomDefs) > 0 { if len(got.CustomDefs) != len(tt.wantCustomDefs) { t.Errorf("CustomDefs length = %d, want %d", len(got.CustomDefs), len(tt.wantCustomDefs)) } else { for i := range got.CustomDefs { if got.CustomDefs[i] != tt.wantCustomDefs[i] { t.Errorf("CustomDefs[%d] = %q, want %q", i, got.CustomDefs[i], tt.wantCustomDefs[i]) } } } + } else if len(got.CustomDefs) != 0 { + t.Errorf("CustomDefs should be empty, got %v", got.CustomDefs) } - // Check extra args - if len(tt.wantExtraArgs) > 0 { + // Check extra args + if len(tt.wantExtraArgs) > 0 { if len(got.AIBExtraArgs) != len(tt.wantExtraArgs) { t.Errorf("AIBExtraArgs length = %d, want %d", len(got.AIBExtraArgs), len(tt.wantExtraArgs)) } else { for i := range got.AIBExtraArgs { if got.AIBExtraArgs[i] != tt.wantExtraArgs[i] { t.Errorf("AIBExtraArgs[%d] = %q, want %q", i, got.AIBExtraArgs[i], tt.wantExtraArgs[i]) } } } + } else if len(got.AIBExtraArgs) != 0 { + t.Errorf("AIBExtraArgs should be empty, got %v", got.AIBExtraArgs) }config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yaml (1)
80-87: Consider addingmaxLengthvalidation on themanifestfield in the CRD.The build API enforces
MaxManifestSizeat the HTTP layer, but directkubectl applybypasses that check. A CRD-levelmaxLength(aligned with the ConfigMap 1 MB limit mentioned earlier) would provide defense-in-depth against oversized manifests submitted throughkubectl.api/v1alpha1/imagebuild_types.go (1)
86-91: Unnecessaryyamlstruct tags on Kubernetes API types.Standard Kubernetes API types use only
jsontags; the API machinery serializes exclusively via JSON. Theyaml:"..."tags onManifestandManifestFileNameare inert and may confuse future readers into thinking a YAML codec is in play.
| name = strings.ToLower(name) | ||
| var b strings.Builder | ||
| for _, r := range name { | ||
| if (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') || r == '-' { |
There was a problem hiding this comment.
The names are also limited to 63 chars max (which is a lot for a manifest), but since you cover everything already I would add this check as well,
including any suffixes that added later.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/caib/main.go`:
- Around line 1327-1344: sanitizeBuildName can return an empty string for inputs
made only of invalid characters; update sanitizeBuildName (and leave
multiHyphenRe usage) so that after trimming if result == "" it returns a safe
fallback token (e.g., "unnamed" or "default") to avoid producing names that
begin with a hyphen when later concatenated with timestamps—modify the function
to set and return that fallback when the sanitized result is empty.
In `@internal/controller/imagebuild/controller.go`:
- Around line 858-867: Replace the space-delimited join for AIB extra args with
newline-delimited output and update the consumer to read one argument per line:
in the image build controller where you set cm.Data["aib-extra-args.txt"] (using
imageBuild.Spec.GetAIBExtraArgs()), change strings.Join(extraArgs, " ") to
strings.Join(extraArgs, "\n"); then modify the shell script that parses
aib-extra-args.txt (the loop that currently does `for item in $line`) to instead
read each line as a single argument (one-arg-per-line parsing), and update any
unit/integration tests that assert on aib-extra-args.txt or build_image.sh
behavior to expect newline-separated args.
🧹 Nitpick comments (1)
internal/controller/imagebuild/controller_test.go (1)
192-281: Good test coverage forsafeDerivedName.The tests cover the important properties: length constraint, suffix preservation, determinism, and hash-based uniqueness.
Minor note: the "exact length boundary" test (line 208) has a baseName of ~52 chars which exceeds
maxBaseLength(45 for-manifestsuffix), so it actually tests truncation rather than an exact boundary. Consider adding a test case wherelen(baseName) == maxBaseLength(i.e., 45 chars) to verify the no-truncation/truncation boundary precisely.
bennyz
force-pushed
the
embed-manifest-imagebuild
branch
from
10e872c to
2276390
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/caib/main.go (1)
1042-1049:⚠️ Potential issue | 🟠 MajorMissing validation of user-provided
buildNameinrunDisk.
runBuild(lines 938-943) andrunBuildDev(lines 1453-1458) validate that a user-supplied--nameisn't entirely invalid characters, butrunDiskskips this check. A user can pass--name "!!!"which will be sent to the API as-is.Proposed fix
if buildName == "" { parts := strings.Split(containerRef, "/") imagePart := parts[len(parts)-1] imagePart = strings.Split(imagePart, ":")[0] // remove tag buildName = fmt.Sprintf("disk-%s-%s", sanitizeBuildName(imagePart), time.Now().Format("20060102-150405")) fmt.Printf("Auto-generated build name: %s\n", buildName) + } else { + if sanitizeBuildName(buildName) == "" { + fmt.Printf("Error: build name '%s' contains only invalid characters\n", buildName) + fmt.Println("Build names must contain at least one letter or number") + os.Exit(1) + } }
🧹 Nitpick comments (2)
internal/controller/imagebuild/controller_test.go (1)
192-281: Good test coverage forsafeDerivedName.Tests thoroughly cover length constraints, suffix preservation, determinism, and hash-based uniqueness for different long inputs. One minor note: the "exact length boundary" test case (baseName is ~52 chars) actually triggers truncation since
maxBaseLength = 63 - 9 - 9 = 45. The test still passes because the assertions only checklen <= 63and suffix presence, but the name is slightly misleading. Consider renaming it or adjusting the base name to 45 chars to truly test the boundary.internal/controller/imagebuild/controller.go (1)
46-60:safeDerivedNameis well-designed but has a latent panic if suffix is ≥ 55 chars.If
len(suffix) >= 55, thenmaxBaseLengthbecomes ≤ 0. Whenlen(baseName) > maxBaseLength(which is always true for any non-empty base),baseName[:maxBaseLength]will panic with a negative or zero slice index on a non-empty string, or produce an empty truncation.All current callers use short suffixes (max 11 chars for
"-upload-pod"), so this isn't exploitable today, but a guard clause would make this future-proof.🛡️ Proposed defensive guard
func safeDerivedName(baseName, suffix string) string { maxBaseLength := maxK8sNameLength - len(suffix) - 9 + if maxBaseLength < 1 { + // Suffix is too long; fall back to hash-only base + hash := sha256.Sum256([]byte(baseName)) + return fmt.Sprintf("%x%s", hash[:4], suffix) + } if len(baseName) <= maxBaseLength { return fmt.Sprintf("%s%s", baseName, suffix) }
bennyz
force-pushed
the
embed-manifest-imagebuild
branch
from
2276390 to
8b4fbbe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/buildapi/server.go`:
- Around line 1340-1353: Validate the manifestFileName in validateBuildRequest
using k8s.io/apimachinery/pkg/util/validation.IsConfigMapKey to ensure it
contains only characters safe for ConfigMap data keys; call
IsConfigMapKey(req.ManifestFileName) (or the field passed into buildAIBSpec) and
if it returns any errors, return a validation error from validateBuildRequest
(with a clear message mentioning manifestFileName) so the request is rejected
before buildAIBSpec/ConfigMap creation proceeds.
8b4fbbe to
c2f51e6
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/buildapi/server.go (1)
1722-1738:⚠️ Potential issue | 🟡 Minor
getBuildTemplateomitsCustomDefsandAIBExtraArgsfrom the response.This PR adds
CustomDefsandAIBExtraArgstoBuildRequest/AIBSpec, but the template response hardcodesCustomDefs: nil(line 1733) and uses an always-emptyaibExtraslice (line 1734). A "rebuild from template" flow would silently lose these fields.Suggested fix
writeJSON(c, http.StatusOK, BuildTemplateResponse{ BuildRequest: BuildRequest{ Name: build.Name, Manifest: manifest, ManifestFileName: manifestFileName, Distro: Distro(build.Spec.GetDistro()), Target: Target(build.Spec.GetTarget()), Architecture: Architecture(build.Spec.Architecture), ExportFormat: ExportFormat(build.Spec.GetExportFormat()), Mode: Mode(build.Spec.GetMode()), AutomotiveImageBuilder: build.Spec.GetAIBImage(), - CustomDefs: nil, - AIBExtraArgs: aibExtra, + CustomDefs: build.Spec.GetCustomDefs(), + AIBExtraArgs: build.Spec.GetAIBExtraArgs(), Compression: build.Spec.GetCompression(), }, SourceFiles: sourceFiles, })
bennyz
force-pushed
the
embed-manifest-imagebuild
branch
from
c2f51e6 to
c2f6e53
Compare
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/controller/imagebuild/controller.go`:
- Around line 851-853: The ConfigMap creation is hardcoding the key
"manifest.aib.yml" into cm.Data instead of using the configured
ManifestFileName; change the assignment so cm.Data uses the ManifestFileName
value as the key (e.g. cm.Data = map[string]string{
imgbuild.Spec.ManifestFileName: manifestContent } or the appropriate struct
field/name where ManifestFileName is stored) so the mounted file name matches
the configured ManifestFileName, and update the assertions in
manifest_configmap_test.go to expect the ConfigMap key to equal ManifestFileName
rather than the hardcoded string.
🧹 Nitpick comments (1)
internal/controller/imagebuild/controller.go (1)
46-60:safeDerivedNamecould panic ifsuffixlength exceeds 54 characters.If
len(suffix) > 54, thenmaxBaseLengthbecomes negative, andbaseName[:maxBaseLength]will panic with a slice-bounds-out-of-range error. Current callers are all safe (longest suffix is"-upload-pod"at 11 chars), but a defensive guard would prevent future surprises.Suggested guard
func safeDerivedName(baseName, suffix string) string { maxBaseLength := maxK8sNameLength - len(suffix) - 9 + if maxBaseLength < 1 { + // suffix too long; fall back to hash-only base + hash := sha256.Sum256([]byte(baseName)) + return fmt.Sprintf("%x%s", hash[:4], suffix) + } if len(baseName) <= maxBaseLength {
bennyz
force-pushed
the
embed-manifest-imagebuild
branch
from
c2f6e53 to
d01f4a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/controller/imagebuild/controller.go`:
- Around line 46-60: The safeDerivedName function can compute a negative
maxBaseLength and then panic when slicing baseName; fix it by adding a defensive
guard in safeDerivedName (which uses maxK8sNameLength, suffix, and the 9-char
hash overhead): if maxBaseLength <= 0, skip truncating baseName and instead
build a name using only the hash and suffix (or a truncated hash+suf) that still
respects maxK8sNameLength; otherwise proceed with the existing truncation logic.
Ensure the final returned string is never longer than maxK8sNameLength.
🧹 Nitpick comments (1)
internal/buildapi/server.go (1)
1083-1098: Minor redundancy in double-dash collapsing.The initial
strings.ReplaceAllon line 1093 is redundant — theforloop on lines 1094–1096 already handles the same replacement iteratively.Simplified version
- result := strings.ReplaceAll(b.String(), "--", "-") - for strings.Contains(result, "--") { + result := b.String() + for strings.Contains(result, "--") { result = strings.ReplaceAll(result, "--", "-") }
bennyz
force-pushed
the
embed-manifest-imagebuild
branch
from
d01f4a5 to
66584a6
Compare
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
to comply with RFC 1123 fixes centos-automotive-suite#103 Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/buildapi/server.go`:
- Around line 1069-1098: The code validates build names but does not sanitize
req.Name before storing it in the ImageBuild CR, so metadata.Name and later
safeDerivedName receive unsanitized values that can violate Kubernetes DNS label
rules; update validateBuildName to either replace req.Name with a sanitized
value or enforce DNS label rules in validateInput and then use the sanitized
result when setting ImageBuild.Metadata.Name and when calling safeDerivedName.
Specifically, reuse sanitizeBuildNameForValidation (or implement equivalent
DNS-label sanitization) to produce a lowercase alphanumeric-and-hyphen name that
starts/ends with an alphanumeric character, ensure validateBuildName returns or
applies that sanitized name, and assign that sanitized name to the ImageBuild
name field (instead of raw req.Name) before any further processing.
In `@internal/controller/imagebuild/controller.go`:
- Around line 46-69: The fallback branch in safeDerivedName may produce a name
starting with a hyphen because hashStr is "-<hex>", which violates Kubernetes
DNS label rules when maxBaseLength <= 0; modify that branch to strip the leading
hyphen from the hash (or generate the 8-char hex without a hyphen) and construct
name = hexPart + suffix, and if that still does not begin with an alphanumeric
character ensure you prefix a safe character (e.g., 'a') before truncating to
maxK8sNameLength; keep references to safeDerivedName, maxK8sNameLength, and
hashStr when making the change.
🧹 Nitpick comments (2)
cmd/caib/main.go (1)
933-943: Duplicated build-name validation logic across three commands.The same validation block (check
sanitizeBuildName(buildName) == "", print error,os.Exit(1)) is copy-pasted inrunBuild,runDisk, andrunBuildDev. Consider extracting a helper to reduce duplication and ensure consistent behavior if the logic evolves.Suggested helper
// validateBuildName checks a user-provided build name and exits if it // contains only invalid characters. func validateBuildName(name string) { if sanitizeBuildName(name) == "" { fmt.Printf("Error: build name '%s' contains only invalid characters\n", name) fmt.Println("Build names must contain at least one letter or number") os.Exit(1) } }Then each command simplifies to:
} else { - if sanitizeBuildName(buildName) == "" { - fmt.Printf("Error: build name '%s' contains only invalid characters\n", buildName) - fmt.Println("Build names must contain at least one letter or number") - os.Exit(1) - } + validateBuildName(buildName) }Also applies to: 1043-1054, 1453-1464
internal/controller/imagebuild/manifest_configmap_test.go (1)
24-227: Well-structured table-driven tests with good coverage.The tests cover the key scenarios (manifest content, default filename, custom defs, extra args, labels, owner references). Consider adding a test case where
AIBisnilto validate the error path (wantErr: true), since the controller would presumably fail in that scenario.
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
embed-manifest-imagebuild
branch
from
66584a6 to
5c2b041
Compare
2 participants
Summary by CodeRabbit
New Features
Documentation
Chores