Skip to content

KMM: Add filesToSign glob pattern signing tests#1331

Open
ybrodsky-rh wants to merge 3 commits intorh-ecosystem-edge:mainfrom
ybrodsky-rh:feat/kmm-filestosign-glob-tests
Open

KMM: Add filesToSign glob pattern signing tests#1331
ybrodsky-rh wants to merge 3 commits intorh-ecosystem-edge:mainfrom
ybrodsky-rh:feat/kmm-filestosign-glob-tests

Conversation

@ybrodsky-rh
Copy link
Copy Markdown
Collaborator

@ybrodsky-rh ybrodsky-rh commented Apr 14, 2026

Add 5 tests for validating KMM module signing with glob patterns in filesToSign:

  1. explicit path
  2. *.ko wildcard
  3. ? single-char glob
  4. [ab] character class
  5. custom dirName support.

Summary by CodeRabbit

  • Tests
    • Added comprehensive test coverage for kernel module signing using glob patterns (wildcards, character classes, explicit paths).
    • Added scenarios validating multiple-module signing and detecting unexpectedly unsigned modules, including tests that assert modules are not signed by a given signer.
    • Extended tests to cover custom module directory layouts and ensured namespace-related logs are included for troubleshooting.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 14, 2026

📝 Walkthrough

Walkthrough

Adds multi-module kernel-module Dockerfile templates, new test namespace constant, helper configmap functions, two signing-check helpers (MultiModuleSigned, ModuleNotSigned), and a Ginkgo test suite exercising filesToSign glob patterns including custom directory cases.

Changes

Cohort / File(s) Summary
Kernel Module Signing Validation
tests/hw-accel/kmm/internal/check/check.go
Added MultiModuleSigned(...) to verify signing for multiple modules via a single verification pod (aggregates per-module results) and ModuleNotSigned(...) to assert a given module is not signed by a specific signer. Both manage pod create/wait/exec/delete.
ConfigMap Helper Functions
tests/hw-accel/kmm/internal/define/configmap.go
Added MultiKoConfigMapContent() and MultiKoCustomDirConfigMapContent() returning Dockerfile content maps sourced from kmmparams constants.
Dockerfile Templates & Test Namespace
tests/hw-accel/kmm/internal/kmmparams/const.go
Added MultiKoContents and MultiKoCustomDirContents Dockerfile templates and FilesToSignGlobTestNamespace constant.
Reporter Namespaces
tests/hw-accel/kmm/modules/internal/tsparams/kmm-vars.go
Registered the new FilesToSignGlobTestNamespace in ReporterNamespacesToDump for log collection.
filesToSign Glob Pattern Test Suite
tests/hw-accel/kmm/modules/tests/filestosign-glob-test.go
Added a new ordered Ginkgo test suite exercising explicit paths, *.ko wildcard, ? single-character wildcard, and [ab] character-class globs; includes custom dirName context, resource setup/teardown, module deploy/delete, and signing assertions (including ModuleNotSigned checks).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~30 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly describes the main change: adding Ginkgo tests for KMM module signing with glob patterns, which matches the core content across all modified files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Add 5 Ginkgo tests validating KMM module signing with glob patterns
in filesToSign: explicit path, *.ko wildcard, ? single-char glob,
[ab] character class, and custom dirName support.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ybrodsky-rh ybrodsky-rh force-pushed the feat/kmm-filestosign-glob-tests branch from 0866a86 to 028d950 Compare April 14, 2026 13:58
Copy link
Copy Markdown
Collaborator

@cdvultur cdvultur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall looks good,. some notes on defer and the proper IDs

Comment thread tests/hw-accel/kmm/internal/check/check.go Outdated
Comment thread tests/hw-accel/kmm/modules/tests/filestosign-glob-test.go Outdated
Comment thread tests/hw-accel/kmm/modules/tests/filestosign-glob-test.go Outdated
Comment thread tests/hw-accel/kmm/modules/tests/filestosign-glob-test.go Outdated
Comment thread tests/hw-accel/kmm/modules/tests/filestosign-glob-test.go Outdated
Replace defer-based pod cleanup with inline delete to match existing
patterns. Update reportxml IDs to match assigned test case numbers.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ybrodsky-rh ybrodsky-rh requested a review from cdvultur April 19, 2026 10:10
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/hw-accel/kmm/internal/check/check.go`:
- Around line 251-255: The current code ignores errors from testPod.Delete(),
which can leave stale pods named "multi-sign-checker"/"unsigned-checker" and
break later tests; change both places that call testPod.Delete() to capture its
error (e.g., err := testPod.Delete()), and if non-nil return a wrapped/formatted
error (including the pod name or context) instead of discarding it, only
returning success when both deletion and the signing verification (errs)
succeed. Ensure you update both occurrences that call testPod.Delete() around
the signing verification blocks.

In `@tests/hw-accel/kmm/modules/tests/filestosign-glob-test.go`:
- Around line 431-435: The verification currently calls
check.MultiModuleSigned(APIClient, []string{"kmm_ci_a", "kmm_ci_b"}, signerCN,
nsName, image, "/custom") but omits the built module test_mod, so a regression
that fails to sign /custom/lib/modules/$KERNEL_FULL_VERSION/test_mod.ko would
not be caught; update the call to include "test_mod" (or otherwise assert the
wildcard /custom/lib/modules/$KERNEL_FULL_VERSION/*.ko contains test_mod.ko) so
check.MultiModuleSigned (or its equivalent assertion) verifies signing for
kmm_ci_a, kmm_ci_b and test_mod under /custom.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8be9c80e-42bb-47e6-8e80-65e9f6e72c5e

📥 Commits

Reviewing files that changed from the base of the PR and between fe63d23 and b04ea71.

📒 Files selected for processing (5)
  • tests/hw-accel/kmm/internal/check/check.go
  • tests/hw-accel/kmm/internal/define/configmap.go
  • tests/hw-accel/kmm/internal/kmmparams/const.go
  • tests/hw-accel/kmm/modules/internal/tsparams/kmm-vars.go
  • tests/hw-accel/kmm/modules/tests/filestosign-glob-test.go

Comment thread tests/hw-accel/kmm/internal/check/check.go Outdated
Comment thread tests/hw-accel/kmm/modules/tests/filestosign-glob-test.go
ggordaniRed
ggordaniRed previously approved these changes Apr 19, 2026
… verification

Handle delete errors in MultiModuleSigned and ModuleNotSigned instead of
discarding them. Add test_mod to the custom dirName verification since the
Dockerfile produces all three .ko files.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tests/hw-accel/kmm/internal/check/check.go (1)

264-267: Consider parameterizing the module root for parity with custom-dir checks.

ModuleNotSigned is fixed to /opt, while MultiModuleSigned accepts dirName. If custom-dir glob tests need negative assertions, this helper cannot be reused without duplicating path logic. A small wrapper can preserve the current default behavior.

♻️ Optional refactor to support custom module directories
 // ModuleNotSigned verifies that a module in an image is NOT signed with the given signer.
 func ModuleNotSigned(apiClient *clients.Settings, modName, signerCN, nsname, image string) error {
-	modulePath := fmt.Sprintf("modinfo /opt/lib/modules/*/%s.ko", modName)
+	return ModuleNotSignedInDir(apiClient, modName, signerCN, nsname, image, "/opt")
+}
+
+// ModuleNotSignedInDir verifies that a module in an image directory is NOT signed with the given signer.
+func ModuleNotSignedInDir(apiClient *clients.Settings, modName, signerCN, nsname, image, dirName string) error {
+	modulePath := fmt.Sprintf("modinfo %s/lib/modules/*/%s.ko", dirName, modName)
 	command := []string{"bash", "-c", modulePath}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/hw-accel/kmm/internal/check/check.go` around lines 264 - 267,
ModuleNotSigned currently hardcodes modulePath under /opt making it incompatible
with MultiModuleSigned's dirName; change ModuleNotSigned signature to accept a
moduleRoot (e.g., moduleRoot string) and build modulePath using that root
instead of "/opt", update internal use of modulePath and the command slice
accordingly, add a small wrapper ModuleNotSignedDefault (or keep old
ModuleNotSigned as a wrapper) that calls the new ModuleNotSigned with "/opt" to
preserve existing callers, and update any callers/tests to pass dirName where
custom dirs are needed (referencing ModuleNotSigned, MultiModuleSigned, and
dirName).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/hw-accel/kmm/internal/check/check.go`:
- Around line 264-267: ModuleNotSigned currently hardcodes modulePath under /opt
making it incompatible with MultiModuleSigned's dirName; change ModuleNotSigned
signature to accept a moduleRoot (e.g., moduleRoot string) and build modulePath
using that root instead of "/opt", update internal use of modulePath and the
command slice accordingly, add a small wrapper ModuleNotSignedDefault (or keep
old ModuleNotSigned as a wrapper) that calls the new ModuleNotSigned with "/opt"
to preserve existing callers, and update any callers/tests to pass dirName where
custom dirs are needed (referencing ModuleNotSigned, MultiModuleSigned, and
dirName).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: fb09727b-4049-4a80-8f88-74f0022b8664

📥 Commits

Reviewing files that changed from the base of the PR and between b04ea71 and b2b0337.

📒 Files selected for processing (2)
  • tests/hw-accel/kmm/internal/check/check.go
  • tests/hw-accel/kmm/modules/tests/filestosign-glob-test.go
✅ Files skipped from review due to trivial changes (1)
  • tests/hw-accel/kmm/modules/tests/filestosign-glob-test.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants