Add wildcard and glob pattern support for filesToSign in module signing#1741
Conversation
✅ Deploy Preview for openshift-kmm ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: TomerNewman The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Module as Module Resource
participant Webhook as Webhook
participant Controller as Reconciler
participant Template as Sign Dockerfile Template
participant SignImage as Sign Build Image
Module->>Webhook: submit/update spec (modprobe.dirName, filesToSign)
Webhook->>Webhook: validateFilesToSign(dirName)
alt validation fails
Webhook-->>Module: reject request (invalid filesToSign)
else validation passes
Webhook-->>Module: accept
Module->>Controller: reconciliation trigger
Controller->>Controller: read Modprobe.DirName, build TemplateData (DirName, filesToSign)
Controller->>Template: render Dockerfile using DirName and filesToSign
Template->>SignImage: COPY module dir (/opt{{DirName}}) and loop-sign files (globs allowed)
SignImage-->>Controller: produce signed artifacts
Controller->>Module: update status / complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/buildsign/resource/common_test.go (1)
387-401:⚠️ Potential issue | 🔴 CriticalFix test incompatibility: the new "should generate correct Dockerfile for signing" test expects a Pod but makeSignTemplate always returns a Build.
The test at line 567+ calls
makeSignTemplate(..., true)and asserts the returned object is av1.Pod, but the actual implementation at lines 223–273 ofcommon.goalways returnsbuildv1.Buildregardless of thepushImageparameter.The old test at line 387–401 correctly expects
buildv1.Buildand uses the old Dockerfile format, while the new test expectsv1.Podand defines a newer loop-based Dockerfile format. Either implement the Pod-based signing path inmakeSignTemplatewhenpushImageis true, or revert the new test to expectbuildv1.Buildand update the Dockerfile format accordingly.
🤖 Fix all issues with AI agents
In `@config/crd-hub/bases/kmm.sigs.x-k8s.io_modulebuildsignconfigs.yaml`:
- Around line 205-208: Update the description for the filesToSign array to fix
the grammar and explicitly state the relationship to Modprobe.DirName: change
the singular/plural mismatch (e.g., "Paths ... are" or make "Path ... is") and
add a sentence that all listed paths must be relative to the Modprobe.DirName
value from Modprobe.DirName (reference symbols: filesToSign and
Modprobe.DirName) since validation enforces this; keep the description concise
and mention that full paths or shell globs supported by the Ash shell are
allowed but resolved relative to Modprobe.DirName.
In `@config/crd-hub/bases/kmm.sigs.x-k8s.io_modules.yaml`:
- Around line 2734-2736: Fix the grammar in the description fields that read
"Full path to explicit files are required" (subject-verb disagreement) by
changing them to "Full paths to explicit files are required" (or "A full path to
each explicit file is required"); locate the two description entries that follow
"Paths inside the image for the kernel modules to sign." (the multi-line YAML
description blocks) and update both occurrences to use the corrected phrasing.
In `@internal/buildsign/resource/common_test.go`:
- Around line 321-334: Remove the dead top-level constant named `dockerfile`
(the unused Dockerfile string defined as `dockerfile = ...`) from the test file;
it is shadowed by a local `const dockerfile` inside the first `DescribeTable`
and never referenced elsewhere (the second `DescribeTable` reads the Dockerfile
from a pod annotation), so delete that unused `dockerfile` constant declaration
to eliminate dead code.
In `@internal/buildsign/resource/templates/Dockerfile.gotmpl`:
- Around line 5-9: The Dockerfile.gotmpl uses /opt{{ .DirName }} and /opt{{ . }}
which double-prefixes /opt when .DirName or entries in .FilesToSign are already
full paths, causing the [ -e "${file}" ] guard to skip signing; change the
template to copy into a dedicated staging root (e.g., use COPY --from=source {{
.DirName }} /signroot{{ .DirName }} and iterate over .FilesToSign as their full
paths under /signroot in the RUN loop) and update the RUN signing loop to
reference /signroot (not /opt) so sign-file is invoked on the actual staged
files, leaving .DirName and .FilesToSign as full-path inputs.
In `@internal/webhook/module.go`:
- Around line 226-239: In validateSignSection(sign *kmmv1beta1.Sign, dirName
string) ensure dirName is non-empty (return an error if it's empty when sign !=
nil), then normalize dirName and each filePath with path.Clean; for each cleaned
filePath also reject any that start with ".." or contain path traversal after
cleaning, and validate that the cleaned filePath is either exactly the cleaned
dirName or has the prefix cleanedDir + "/" (use strings.HasPrefix on the cleaned
values) before accepting; update error messages to reference the cleaned values.
🧹 Nitpick comments (1)
internal/webhook/module.go (1)
241-257: Consider validatingdirNameis non-empty before proceeding.
container.Modprobe.DirNameis read directly without checking if it's empty. If a user creates a Module without explicitly settingDirName, the kubebuilder default (/opt) should apply, but if the CRD default isn't applied (e.g., direct API calls),dirNamecould be empty. This ties into the concern above — either guard here or invalidateSignSection.
| description: |- | ||
| Paths inside the image for the kernel modules to sign. | ||
| Full path to explicit files are required or any globs supported by the `Ash` shell |
There was a problem hiding this comment.
Minor grammar issue in description: "path … are" → "paths … are".
"Full path to explicit files are required" has a subject-verb disagreement. Should be "Full paths to explicit files are required" (or "A full path to each explicit file is required").
The same issue is repeated in the second occurrence (lines 2902–2904).
Proposed fix (both occurrences)
description: |-
Paths inside the image for the kernel modules to sign.
- Full path to explicit files are required or any globs supported by the `Ash` shell
+ Full paths to explicit files are required, or any globs supported by the `Ash` shell📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| description: |- | |
| Paths inside the image for the kernel modules to sign. | |
| Full path to explicit files are required or any globs supported by the `Ash` shell | |
| description: |- | |
| Paths inside the image for the kernel modules to sign. | |
| Full paths to explicit files are required, or any globs supported by the `Ash` shell |
🤖 Prompt for AI Agents
In `@config/crd-hub/bases/kmm.sigs.x-k8s.io_modules.yaml` around lines 2734 -
2736, Fix the grammar in the description fields that read "Full path to explicit
files are required" (subject-verb disagreement) by changing them to "Full paths
to explicit files are required" (or "A full path to each explicit file is
required"); locate the two description entries that follow "Paths inside the
image for the kernel modules to sign." (the multi-line YAML description blocks)
and update both occurrences to use the corrected phrasing.
| COPY --from=source {{ .DirName }} /opt{{ .DirName }} | ||
| {{- range .FilesToSign }} | ||
| RUN for file in /opt{{ . }}; do \ | ||
| [ -e "${file}" ] && /usr/local/bin/sign-file sha256 /run/secrets/key/key.pem /run/secrets/cert/cert.pem "${file}"; \ | ||
| done |
There was a problem hiding this comment.
Avoid double-prefixing /opt, which can silently skip signing.
Line 5 and Line 7 prefix /opt to files already specified as full paths (e.g., /opt/lib/modules/... from the new docs and ci/e2e/module.yaml). With the default DirName=/opt, this yields /opt/opt/..., so the [ -e ] guard skips signing and modules remain unsigned.
A safe fix is to use a dedicated staging root (e.g., /signroot) and keep FilesToSign as full paths.
💡 Proposed fix (use a staging root to avoid double-prefixing)
FROM {{ .SignImage }} AS signimage
-COPY --from=source {{ .DirName }} /opt{{ .DirName }}
+COPY --from=source {{ .DirName }} /signroot{{ .DirName }}
{{- range .FilesToSign }}
-RUN for file in /opt{{ . }}; do \
+RUN for file in /signroot{{ . }}; do \
[ -e "${file}" ] && /usr/local/bin/sign-file sha256 /run/secrets/key/key.pem /run/secrets/cert/cert.pem "${file}"; \
done
{{- end }}
FROM source
-COPY --from=signimage /opt{{ .DirName }} {{ .DirName }}
+COPY --from=signimage /signroot{{ .DirName }} {{ .DirName }}Also applies to: 13-13
🤖 Prompt for AI Agents
In `@internal/buildsign/resource/templates/Dockerfile.gotmpl` around lines 5 - 9,
The Dockerfile.gotmpl uses /opt{{ .DirName }} and /opt{{ . }} which
double-prefixes /opt when .DirName or entries in .FilesToSign are already full
paths, causing the [ -e "${file}" ] guard to skip signing; change the template
to copy into a dedicated staging root (e.g., use COPY --from=source {{ .DirName
}} /signroot{{ .DirName }} and iterate over .FilesToSign as their full paths
under /signroot in the RUN loop) and update the RUN signing loop to reference
/signroot (not /opt) so sign-file is invoked on the actual staged files, leaving
.DirName and .FilesToSign as full-path inputs.
There was a problem hiding this comment.
COPY --from=source /opt /opt/opt- copies/optto/opt/opt- File at
/opt/lib/modules/.../foo.kobecomes/opt/opt/lib/modules/.../foo.ko for file in /opt/opt/lib/modules/.../foo.ko(matches the file location)COPY --from=signimage /opt/opt /opt- copies back
| func validateSignSection(sign *kmmv1beta1.Sign, dirName string) error { | ||
| if sign == nil { | ||
| return nil | ||
| } | ||
| if len(sign.FilesToSign) == 0 { | ||
| return fmt.Errorf("filesToSign is required when Sign is set") | ||
| } | ||
| for _, filePath := range sign.FilesToSign { | ||
| if !strings.HasPrefix(filePath, dirName+"/") { | ||
| return fmt.Errorf("filesToSign[%q] must be under dirName %q", filePath, dirName) | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual file to see the function in context
cat -n internal/webhook/module.go | head -250 | tail -50Repository: rh-ecosystem-edge/kernel-module-management
Length of output: 1802
🏁 Script executed:
# Check DirName usage and validation in the codebase
rg -n 'DirName' --type=go -C3 -g '!*_test.go' -g '!*vendor*'Repository: rh-ecosystem-edge/kernel-module-management
Length of output: 6933
🏁 Script executed:
# Search for where validateSignSection is called to understand how dirName is passed
rg -n 'validateSignSection' --type=go -C5 -g '!*_test.go'Repository: rh-ecosystem-edge/kernel-module-management
Length of output: 1694
Path traversal via .. segments and empty dirName weaken path validation.
Two security concerns with validateSignSection:
-
Path traversal: Paths containing
..segments (e.g.,/modules/../etc/passwd) pass the prefix check without being caught. Normalize paths withpath.Cleanbefore validation. -
Empty
dirName: AlthoughDirNamehas a kubebuilder default of/opt, nothing prevents it from being explicitly set to"". This reduces the prefix check tostrings.HasPrefix(filePath, "/"), allowing any absolute path. Validate thatdirNameis non-empty whenSignis configured.
Proposed fix
+import "path"
+
func validateSignSection(sign *kmmv1beta1.Sign, dirName string) error {
if sign == nil {
return nil
}
+ if dirName == "" {
+ return fmt.Errorf("dirName must be set when Sign is configured")
+ }
if len(sign.FilesToSign) == 0 {
return fmt.Errorf("filesToSign is required when Sign is set")
}
for _, filePath := range sign.FilesToSign {
- if !strings.HasPrefix(filePath, dirName+"/") {
+ cleanPath := path.Clean(filePath)
+ if !strings.HasPrefix(cleanPath, dirName+"/") {
return fmt.Errorf("filesToSign[%q] must be under dirName %q", filePath, dirName)
}
}
return nil
}🤖 Prompt for AI Agents
In `@internal/webhook/module.go` around lines 226 - 239, In
validateSignSection(sign *kmmv1beta1.Sign, dirName string) ensure dirName is
non-empty (return an error if it's empty when sign != nil), then normalize
dirName and each filePath with path.Clean; for each cleaned filePath also reject
any that start with ".." or contain path traversal after cleaning, and validate
that the cleaned filePath is either exactly the cleaned dirName or has the
prefix cleanedDir + "/" (use strings.HasPrefix on the cleaned values) before
accepting; update error messages to reference the cleaned values.
|
@coderabbitai[bot]: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
TomerNewman
force-pushed
the
add-wildcard-signing
branch
from
a6a70c2 to
b502833
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@api/v1beta1/module_types.go`:
- Around line 90-92: Update the comment above the FilesToSign field in
module_types.go to fix the grammar and clarify the two valid forms: change the
sentence "Full path to explicit files are required or any globs supported by the
`Ash` shell" to a clear wording such as "Full paths to explicit files are
required, or you may use any globs supported by the `Ash` shell." Keep the rest
of the comment about kernel module paths unchanged and ensure the JSON tag and
field name FilesToSign remain intact.
In `@internal/buildsign/resource/common_test.go`:
- Around line 610-631: Add a Ginkgo test Entry that covers glob/wildcard
patterns for filesToSign (e.g., an Entry titled "sign with glob pattern" that
passes []string{"/modules/*.ko"}) to verify template rendering, ensuring the
expected template lines include "for file in /opt/modules/*.ko; do" and the same
surrounding COPY and sign-file lines; update the table of Entry calls near the
existing "sign explicit paths" and "sign multiple paths" entries in
common_test.go so the new Entry exercises wildcard handling for the code that
processes filesToSign.
- Around line 597-600: The test incorrectly asserts that rm.makeSignTemplate
returns a *v1.Pod, but makeSignTemplate always returns a *buildv1.Build; update
the test around the call to rm.makeSignTemplate to assert the concrete type
*buildv1.Build (use a type assertion to *buildv1.Build and check ok) and then
inspect the Build's spec/fields that depend on the pushImage flag instead of
asserting Pod-specific fields.
🧹 Nitpick comments (2)
internal/buildsign/resource/templates/Dockerfile.gotmpl (1)
7-9: Unmatched glob silently skips signing with no warning.When a glob pattern in
FilesToSignmatches zero files, Ash keeps the literal pattern,[ -e "${file}" ]is false, and theRUNsucceeds silently. A user typo in a glob could leave all modules unsigned with no error in the build log.Consider adding an
else echo "WARNING: no files matched pattern" >&2branch (or failing the build) so users get feedback:Proposed improvement
-RUN for file in /opt{{ . }}; do \ - [ -e "${file}" ] && /usr/local/bin/sign-file sha256 /run/secrets/key/key.pem /run/secrets/cert/cert.pem "${file}"; \ - done +RUN matched=0; for file in /opt{{ . }}; do \ + if [ -e "${file}" ]; then \ + /usr/local/bin/sign-file sha256 /run/secrets/key/key.pem /run/secrets/cert/cert.pem "${file}"; \ + matched=$((matched+1)); \ + fi; \ + done; \ + if [ "$matched" -eq 0 ]; then echo "WARNING: no files matched pattern '{{ . }}'" >&2; exit 1; fiinternal/buildsign/resource/common_test.go (1)
602-608: Substring-based Dockerfile validation is a pragmatic approach but may be fragile.The test reads the Dockerfile from
actualPod.Annotations["dockerfile"]and validates viaContainSubstring. This is reasonable for verifying key fragments are present, but won't catch structural issues (e.g., commands in wrong order, missing newlines between stages). Consider adding at least one test that does a full exact-match comparison against the rendered template to catch regressions in template structure.
TomerNewman
force-pushed
the
add-wildcard-signing
branch
2 times, most recently
from
15797c1 to
512b6f0
Compare
|
/retest |
TomerNewman
force-pushed
the
add-wildcard-signing
branch
from
512b6f0 to
6efae5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@bundle/manifests/kmm.sigs.x-k8s.io_moduleimagesconfigs.yaml`:
- Around line 214-216: The description for the filesToSign field is
grammatically incorrect and ambiguous; update the text for clarity and correct
subject–verb agreement in the description string (the YAML key/field referenced
as filesToSign). Replace the sentence "Full path to explicit files are required
or any globs supported by the `Ash` shell" with a concise, correct alternative
such as "Provide full paths to files to sign, or use globs supported by the
`ash` shell," and ensure the surrounding description still explains that
explicit full paths or shell globs are accepted.
In `@config/crd/bases/kmm.sigs.x-k8s.io_modules.yaml`:
- Around line 2734-2736: The description text contains a subject-verb
disagreement: replace "Full path to explicit files are required" with a
grammatically correct form (e.g., "Full paths to explicit files are required" or
"A full path to each explicit file is required") in the YAML description field
that currently reads "Paths inside the image for the kernel modules to sign.
Full path to explicit files are required or any globs supported by the `Ash`
shell" — apply this same change for both identical occurrences of that
description string.
| description: |- | ||
| Paths inside the image for the kernel modules to sign. | ||
| Full path to explicit files are required or any globs supported by the `Ash` shell |
There was a problem hiding this comment.
Clarify the filesToSign description — grammar is awkward.
Line 216: "Full path to explicit files are required or any globs supported by the Ash shell" has a subject–verb disagreement ("path…are") and reads ambiguously. Consider something clearer:
📝 Suggested wording
filesToSign:
description: |-
Paths inside the image for the kernel modules to sign.
- Full path to explicit files are required or any globs supported by the `Ash` shell
+ Each entry must be either a full file path or a glob pattern supported by the Ash shell.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| description: |- | |
| Paths inside the image for the kernel modules to sign. | |
| Full path to explicit files are required or any globs supported by the `Ash` shell | |
| description: |- | |
| Paths inside the image for the kernel modules to sign. | |
| Each entry must be either a full file path or a glob pattern supported by the Ash shell. |
🤖 Prompt for AI Agents
In `@bundle/manifests/kmm.sigs.x-k8s.io_moduleimagesconfigs.yaml` around lines 214
- 216, The description for the filesToSign field is grammatically incorrect and
ambiguous; update the text for clarity and correct subject–verb agreement in the
description string (the YAML key/field referenced as filesToSign). Replace the
sentence "Full path to explicit files are required or any globs supported by the
`Ash` shell" with a concise, correct alternative such as "Provide full paths to
files to sign, or use globs supported by the `ash` shell," and ensure the
surrounding description still explains that explicit full paths or shell globs
are accepted.
6efae5c to
289bb65
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@config/crd-hub/bases/kmm.sigs.x-k8s.io_moduleimagesconfigs.yaml`:
- Around line 210-212: Update the description text under the moduleimagesconfigs
schema for kernel module signing: change "Full path to explicit files are
required" to "Full paths to explicit files are required" (i.e., pluralize "path"
to "paths") so the sentence agrees with "files"; edit the description field
associated with the kernel modules paths entry in
config/crd-hub/bases/kmm.sigs.x-k8s.io_moduleimagesconfigs.yaml accordingly.
In `@config/crd/bases/kmm.sigs.x-k8s.io_moduleimagesconfigs.yaml`:
- Around line 210-212: Update the description string in the CRD under the
moduleimagesconfigs schema where it currently reads "Full path to explicit files
are required..." to use correct pluralization: change "Full path" to "Full
paths" so the sentence reads "Full paths to explicit files are
required..."—locate the description block containing the sentence (the
multi-line YAML description for moduleimagesconfigs) and edit that phrase
accordingly.
In `@internal/buildsign/resource/common_test.go`:
- Around line 379-382: The test's filesToSign value uses ":" as a separator but
the template splits on "," causing a single colon-containing element and an
invalid Dockerfile path; update the test data in
internal/buildsign/resource/common_test.go by either (a) changing filesToSign to
use a comma-separated string (e.g.
"/modules/simple-kmod.ko,/modules/simple-procfs-kmod.ko") or (b) replace it with
an explicit []string literal so the template sees two entries; then update the
expected dockerfile constant to reflect two separate sign-loop entries (one per
path) matching the existing "sign multiple paths" pattern used elsewhere in the
file.
In `@internal/buildsign/resource/templates/Dockerfile.gotmpl`:
- Around line 8-13: The RUN loop in Dockerfile.gotmpl that iterates over
.FilesToSign uses "&&" after the test, causing the shell to return non-zero when
a glob matches nothing; change the loop body to explicitly test file existence
with an if/then/fi so that missing matches exit 0 but actual
/usr/local/bin/sign-file failures propagate non-zero. Specifically, update the
RUN block that uses "for file in /opt{{ . }}; do [ -e \"${file}\" ] &&
/usr/local/bin/sign-file sha256 /run/secrets/key/key /run/secrets/cert/cert
\"${file}\"; done" to use an if [ -e "${file}" ]; then /usr/local/bin/sign-file
...; fi pattern (still inside the same loop over .FilesToSign) so no-match
scenarios succeed and real signing errors fail the build.
🧹 Nitpick comments (2)
internal/buildsign/resource/templates/Dockerfile.gotmpl (1)
8-16: Consider adding a comment to explain the/optstaging prefix design.The template uses
/optas a fixed staging namespace for signing (line 8:COPY --from=source {{ .DirName }} /opt{{ .DirName }}). With the defaultDirName="/opt", this creates/opt/optduring the signing stage, andFilesToSignentries (which must start with/opt/per webhook validation) correctly resolve as/opt/opt/.... The design works correctly and is tested—e2e tests inci/e2e/module.yamlexercise the defaultDirName="/opt"case with paths like/opt/lib/modules/${KERNEL_FULL_VERSION}/kmm_ci_?.ko—but the double-nesting rationale is non-obvious and would benefit from a brief comment explaining why/optis used as the staging prefix.internal/buildsign/resource/common_test.go (1)
605-615: Consider whether multipleforloops (one per file) produce unnecessary Docker layers.The "sign multiple paths" entry at lines 610-611 confirms the template generates a separate
for file in …; doblock for each element inFilesToSign. Each becomes a separateRUNinstruction, creating an extra layer per file. For a small number of files this is fine, but with many explicit paths it adds image bloat.This is a template design concern rather than a test issue — just flagging it for awareness.
| description: |- | ||
| Paths inside the image for the kernel modules to sign. | ||
| Full path to explicit files are required or any globs supported by the `Ash` shell |
There was a problem hiding this comment.
Minor grammar: "Full path" → "Full paths".
The description reads "Full path to explicit files are required" — the subject-verb agreement suggests "Full paths" (plural) to match "files are required".
📝 Proposed fix
description: |-
Paths inside the image for the kernel modules to sign.
- Full path to explicit files are required or any globs supported by the `Ash` shell
+ Full paths to explicit files are required or any globs supported by the `Ash` shell📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| description: |- | |
| Paths inside the image for the kernel modules to sign. | |
| Full path to explicit files are required or any globs supported by the `Ash` shell | |
| description: |- | |
| Paths inside the image for the kernel modules to sign. | |
| Full paths to explicit files are required or any globs supported by the `Ash` shell |
🤖 Prompt for AI Agents
In `@config/crd-hub/bases/kmm.sigs.x-k8s.io_moduleimagesconfigs.yaml` around lines
210 - 212, Update the description text under the moduleimagesconfigs schema for
kernel module signing: change "Full path to explicit files are required" to
"Full paths to explicit files are required" (i.e., pluralize "path" to "paths")
so the sentence agrees with "files"; edit the description field associated with
the kernel modules paths entry in
config/crd-hub/bases/kmm.sigs.x-k8s.io_moduleimagesconfigs.yaml accordingly.
| description: |- | ||
| Paths inside the image for the kernel modules to sign. | ||
| Full path to explicit files are required or any globs supported by the `Ash` shell |
There was a problem hiding this comment.
Same grammar nit as the hub CRD: "Full path" → "Full paths".
📝 Proposed fix
description: |-
Paths inside the image for the kernel modules to sign.
- Full path to explicit files are required or any globs supported by the `Ash` shell
+ Full paths to explicit files are required or any globs supported by the `Ash` shell📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| description: |- | |
| Paths inside the image for the kernel modules to sign. | |
| Full path to explicit files are required or any globs supported by the `Ash` shell | |
| description: |- | |
| Paths inside the image for the kernel modules to sign. | |
| Full paths to explicit files are required or any globs supported by the `Ash` shell |
🤖 Prompt for AI Agents
In `@config/crd/bases/kmm.sigs.x-k8s.io_moduleimagesconfigs.yaml` around lines 210
- 212, Update the description string in the CRD under the moduleimagesconfigs
schema where it currently reads "Full path to explicit files are required..." to
use correct pluralization: change "Full path" to "Full paths" so the sentence
reads "Full paths to explicit files are required..."—locate the description
block containing the sentence (the multi-line YAML description for
moduleimagesconfigs) and edit that phrase accordingly.
| COPY --from=source {{ .DirName }} /opt{{ .DirName }} | ||
| {{- range .FilesToSign }} | ||
| RUN for file in /opt{{ . }}; do \ | ||
| [ -e "${file}" ] && /usr/local/bin/sign-file sha256 /run/secrets/key/key /run/secrets/cert/cert "${file}"; \ | ||
| done | ||
| {{- end }} |
There was a problem hiding this comment.
Glob matching zero files will fail the Docker build with a cryptic error.
When a glob pattern in FilesToSign matches no files, the Ash shell for loop iterates once with the literal unexpanded glob string. [ -e "${file}" ] returns exit code 1, and since && short-circuits, the loop body's exit code is 1. This causes the RUN instruction to fail the entire Docker build with no clear error message.
Use an if/then construct instead of && so that a no-match scenario exits cleanly (0), while still propagating actual sign-file failures:
🐛 Proposed fix
COPY --from=source {{ .DirName }} /opt{{ .DirName }}
{{- range .FilesToSign }}
-RUN for file in /opt{{ . }}; do \
- [ -e "${file}" ] && /usr/local/bin/sign-file sha256 /run/secrets/key/key /run/secrets/cert/cert "${file}"; \
+RUN for file in /opt{{ . }}; do \
+ if [ -e "${file}" ]; then /usr/local/bin/sign-file sha256 /run/secrets/key/key /run/secrets/cert/cert "${file}"; fi; \
done
{{- end }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| COPY --from=source {{ .DirName }} /opt{{ .DirName }} | |
| {{- range .FilesToSign }} | |
| RUN for file in /opt{{ . }}; do \ | |
| [ -e "${file}" ] && /usr/local/bin/sign-file sha256 /run/secrets/key/key /run/secrets/cert/cert "${file}"; \ | |
| done | |
| {{- end }} | |
| COPY --from=source {{ .DirName }} /opt{{ .DirName }} | |
| {{- range .FilesToSign }} | |
| RUN for file in /opt{{ . }}; do \ | |
| if [ -e "${file}" ]; then /usr/local/bin/sign-file sha256 /run/secrets/key/key /run/secrets/cert/cert "${file}"; fi; \ | |
| done | |
| {{- end }} |
🤖 Prompt for AI Agents
In `@internal/buildsign/resource/templates/Dockerfile.gotmpl` around lines 8 - 13,
The RUN loop in Dockerfile.gotmpl that iterates over .FilesToSign uses "&&"
after the test, causing the shell to return non-zero when a glob matches
nothing; change the loop body to explicitly test file existence with an
if/then/fi so that missing matches exit 0 but actual /usr/local/bin/sign-file
failures propagate non-zero. Specifically, update the RUN block that uses "for
file in /opt{{ . }}; do [ -e \"${file}\" ] && /usr/local/bin/sign-file sha256
/run/secrets/key/key /run/secrets/cert/cert \"${file}\"; done" to use an if [ -e
"${file}" ]; then /usr/local/bin/sign-file ...; fi pattern (still inside the
same loop over .FilesToSign) so no-match scenarios succeed and real signing
errors fail the build.
|
/hold In this project the sign image is based on ubi-minimal which uses bash, so we need to:
|
This commit adds support for wildcard and glob patterns in the filesToSign field when signing kernel modules. Previously, users had to specify the exact path to each .ko file they wanted to sign. Now, the user can specify full paths to explicit files (required as before), or any glob patterns supported by the Bash shell (UBI sign image). Additionally, this commit propagates DirName from the API into the sign image template, and adds the signed files to the validation webhook.
TomerNewman
force-pushed
the
add-wildcard-signing
branch
from
289bb65 to
91be085
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@bundle-hub/manifests/hub.kmm.sigs.x-k8s.io_managedclustermodules.yaml`:
- Around line 2762-2764: Update the description for the filesToSign field to fix
the grammar and clarity: change "Full path to explicit files are required or any
globs supported by the Bash shell" to a clearer sentence such as "Full paths to
explicit files are required, or any globs supported by the Bash shell may be
used." Apply the same wording to the identical description for filesToSign
elsewhere in the manifest so both occurrences (the description block under
filesToSign) match.
In `@bundle/manifests/kmm.sigs.x-k8s.io_modulebuildsignconfigs.yaml`:
- Around line 209-211: The description for filesToSign has a grammar and
punctuation issue: change "Full path to explicit files are required" to "Full
paths to explicit files are required." and ensure the sentence about globs ("any
globs supported by the Bash shell") ends with a trailing period; update the
description block for filesToSign accordingly.
In `@config/crd-hub/bases/hub.kmm.sigs.x-k8s.io_managedclustermodules.yaml`:
- Around line 2758-2760: Edit the description string for the filesToSign field:
change "Full path to explicit files are required or any globs supported by the
Bash shell" to "Full paths to explicit files are required, or any globs
supported by the Bash shell." and ensure you apply the same exact fix to the
second occurrence of the filesToSign description (both description blocks shown
in the diff).
| description: |- | ||
| Paths inside the image for the kernel modules to sign. | ||
| Full path to explicit files are required or any globs supported by the Bash shell |
There was a problem hiding this comment.
Minor grammar issue in the filesToSign description.
"Full path to explicit files are required" has a subject-verb disagreement and reads awkwardly. Consider:
📝 Suggested wording
description: |-
Paths inside the image for the kernel modules to sign.
- Full path to explicit files are required or any globs supported by the Bash shell
+ Full paths to explicit files, or any glob patterns supported by the Bash shell.The same fix applies to the identical description at Line 2930-2932.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| description: |- | |
| Paths inside the image for the kernel modules to sign. | |
| Full path to explicit files are required or any globs supported by the Bash shell | |
| description: |- | |
| Paths inside the image for the kernel modules to sign. | |
| Full paths to explicit files, or any glob patterns supported by the Bash shell. |
🤖 Prompt for AI Agents
In `@bundle-hub/manifests/hub.kmm.sigs.x-k8s.io_managedclustermodules.yaml` around
lines 2762 - 2764, Update the description for the filesToSign field to fix the
grammar and clarity: change "Full path to explicit files are required or any
globs supported by the Bash shell" to a clearer sentence such as "Full paths to
explicit files are required, or any globs supported by the Bash shell may be
used." Apply the same wording to the identical description for filesToSign
elsewhere in the manifest so both occurrences (the description block under
filesToSign) match.
| description: |- | ||
| Paths inside the image for the kernel modules to sign. | ||
| Full path to explicit files are required or any globs supported by the Bash shell |
There was a problem hiding this comment.
Minor grammar issue in filesToSign description.
"Full path to explicit files are required" → should be "Full paths to explicit files are required" for correct subject-verb agreement. Also, the sentence is missing a trailing period.
Proposed fix
description: |-
Paths inside the image for the kernel modules to sign.
- Full path to explicit files are required or any globs supported by the Bash shell
+ Full paths to explicit files are required, or any globs supported by the Bash shell.🤖 Prompt for AI Agents
In `@bundle/manifests/kmm.sigs.x-k8s.io_modulebuildsignconfigs.yaml` around lines
209 - 211, The description for filesToSign has a grammar and punctuation issue:
change "Full path to explicit files are required" to "Full paths to explicit
files are required." and ensure the sentence about globs ("any globs supported
by the Bash shell") ends with a trailing period; update the description block
for filesToSign accordingly.
| description: |- | ||
| Paths inside the image for the kernel modules to sign. | ||
| Full path to explicit files are required or any globs supported by the Bash shell |
There was a problem hiding this comment.
Minor grammar issues in the filesToSign description.
"Full path to explicit files are required" has a subject-verb disagreement — should be "Full paths … are required" (or "A full path … is required"). The sentence also lacks a trailing period. The same issue appears at Line 2928.
📝 Suggested fix (apply to both occurrences at lines 2758–2760 and 2926–2928)
description: |-
- Paths inside the image for the kernel modules to sign.
- Full path to explicit files are required or any globs supported by the Bash shell
+ Paths inside the image for the kernel modules to sign.
+ Full paths to explicit files are required, or any glob patterns supported by the Bash shell.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| description: |- | |
| Paths inside the image for the kernel modules to sign. | |
| Full path to explicit files are required or any globs supported by the Bash shell | |
| description: |- | |
| Paths inside the image for the kernel modules to sign. | |
| Full paths to explicit files are required, or any glob patterns supported by the Bash shell. |
🤖 Prompt for AI Agents
In `@config/crd-hub/bases/hub.kmm.sigs.x-k8s.io_managedclustermodules.yaml` around
lines 2758 - 2760, Edit the description string for the filesToSign field: change
"Full path to explicit files are required or any globs supported by the Bash
shell" to "Full paths to explicit files are required, or any globs supported by
the Bash shell." and ensure you apply the same exact fix to the second
occurrence of the filesToSign description (both description blocks shown in the
diff).
|
/retest |
openshift-merge-bot
Bot
merged commit f29727b
into
rh-ecosystem-edge:main
3 participants
This commit adds support for wildcard and glob patterns in the filesToSign field when signing kernel modules. Previously, users had to specify the exact path to each .ko file they wanted to sign.
Now, the user can specify full paths to explicit files (required as before), or any glob patterns supported by the Bash shell (UBI sign image).
Additionally, this commit propagates DirName from the API into the sign image template, and adds the signed files to the validation webhook.
fix #1740
/cc @yevgeny-shnaidman @ybettan
Summary by CodeRabbit
New Features
Documentation
Improvements
Tests