TRT-2295: admit and extract non-payload OTE binaries#30863
TRT-2295: admit and extract non-payload OTE binaries#30863smg247 wants to merge 3 commits intoopenshift:mainfrom
Conversation
…mmand to install the TestExtension CRD and apply the matching CR
openshift-ci-robot
added
the
jira/valid-reference
|
@smg247: This pull request references TRT-2295 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughThis PR introduces extension admission support to OpenShift tests. It adds a new TestExtensionAdmission CRD (testextension.redhat.io/v1), a CLI command for managing extension admissions with CRD installation and resource creation, discovery logic for non-payload binary admissions, and infrastructure to classify and report unpermitted extensions as test failures. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Test Runner
participant Discovery as Non-Payload Discovery
participant Admission as TestExtensionAdmission API
participant Classification as Extension Classifier
participant Extraction as Binary Extractor
participant Reporter as Test Reporter
Runner->>Discovery: DiscoverNonPayloadBinaryAdmission()
Discovery->>Admission: List TestExtensionAdmission resources
Admission-->>Discovery: Return permit patterns
Discovery-->>Runner: Return []PermitPattern
Runner->>Discovery: discoverNonPayloadExtensions()
Discovery->>Discovery: List ImageStreamTags across namespaces
Discovery->>Classification: MatchesAnyPermit(namespace, imagestream, patterns)
Classification-->>Discovery: permitted=true/false
Discovery-->>Runner: Return permitted + unpermitted extensions
par Parallel Processing
Runner->>Extraction: Extract permitted extensions
Extraction-->>Runner: Return TestBinaries
and
Runner->>Reporter: createUnpermittedExtensionTests(unpermitted[])
Reporter-->>Runner: Return JUnit test cases
end
Runner->>Reporter: Append unpermitted test cases to results
Reporter-->>Runner: Complete test suite with failure reporting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
@smg247: This pull request references TRT-2295 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
pkg/test/extensions/non-payload-binary-admission.go (1)
119-126: Replace customisNotFound()with Kubernetes error helpers.This fallback decides whether to suppress API errors and continue in deny-all mode. Raw substring checks are fragile; message variations or unrelated errors containing "not found" will be misclassified. Use the standard Kubernetes helpers (
apierrors.IsNotFound()orkapierrs.IsNotFound()) already established throughout the codebase for discovery and dynamic-client errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/test/extensions/non-payload-binary-admission.go` around lines 119 - 126, The isNotFound(err error) helper currently does fragile substring checks; replace its body to use the Kubernetes standard error helper (e.g., apierrors.IsNotFound or kapierrs.IsNotFound) to determine NotFound errors and remove the manual strings.Contains checks; update imports to include the appropriate k8s.io/apimachinery/pkg/api/errors (or the project's kapierrs alias) and ensure all callers of isNotFound(func) continue to work with the same signature (isNotFound) so discovery/dynamic-client errors are detected reliably.pkg/test/ginkgo/cmd_runsuite.go (1)
180-209: LGTM - Flake pattern for visibility without blocking.The approach of creating both a pass and fail case for the same test name is an intentional "flake" pattern that makes unpermitted extensions visible in test results without causing the overall job to fail. This aligns with the comment on line 181 and is appropriate for rolling out this feature.
One minor formatting nit on line 196:
Cleaner string formatting
- msg := "extension binary not permitted by TestExtensionAdmission:\n" for _, u := range unpermitted { - msg = fmt.Sprintf("%s\n - %s/%s:%s (%s)\n", msg, u.Namespace, u.ImageStream, u.Tag, u.Component) + msg += fmt.Sprintf(" - %s/%s:%s (%s)\n", u.Namespace, u.ImageStream, u.Tag, u.Component)Or use
strings.Builderfor better performance with multiple appends.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/test/ginkgo/cmd_runsuite.go` around lines 180 - 209, The string building in createUnpermittedExtensionTests currently concatenates with fmt.Sprintf repeatedly (msg variable) which is less efficient and slightly messy; replace that with a strings.Builder (or use fmt.Fprintf to the builder) to construct the multi-line message inside the loop that iterates over unpermitted (for _, u := range unpermitted) and then set FailureOutput.Output to builder.String(), and keep the existing logrus.Warnf calls unchanged.pkg/cmd/openshift-tests/extension-admission/extension_admission_command.go (1)
165-171: File handle closed twice.The file is closed via
defer tmpFile.Close()at line 166, and then explicitly closed at line 171. While callingClose()twice on an*os.Fileis safe (subsequent calls return an error that's ignored here), the deferredos.Remove(tmpFile.Name())at line 165 will execute before the deferredClose()at line 166 due to LIFO ordering. This means the file is removed while still open (on some OSes this is fine, on Windows it may fail).Consider removing the deferred
Close()since you're explicitly closing before exec:Suggested fix
tmpFile, err := os.CreateTemp("", "testextensionadmission-*.yaml") if err != nil { return fmt.Errorf("failed to create temp file: %w", err) } defer os.Remove(tmpFile.Name()) - defer tmpFile.Close() if _, err := tmpFile.Write(yamlBytes); err != nil { + tmpFile.Close() return fmt.Errorf("failed to write YAML to temp file: %w", err) } - tmpFile.Close() + if err := tmpFile.Close(); err != nil { + return fmt.Errorf("failed to close temp file: %w", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/openshift-tests/extension-admission/extension_admission_command.go` around lines 165 - 171, The temp file is being closed twice (defer tmpFile.Close() and an explicit tmpFile.Close()), and defer os.Remove(tmpFile.Name()) will run before the deferred Close due to LIFO ordering which can cause remove-before-close issues on some OSes; remove the deferred tmpFile.Close() and keep the explicit tmpFile.Close() after writing (around the tmpFile.Write(yamlBytes) error handling) so the file is closed deterministically before or after os.Remove as intended; look for symbols tmpFile, tmpFile.Write, tmpFile.Close, os.Remove, and yamlBytes to update the defers accordingly.pkg/test/extensions/binary.go (1)
1040-1045: Consider adding a label selector to reduce API response size.Listing all
ImageStreamTagsacross all namespaces could return a large result set. If non-payload extensions are expected to have a specific label (e.g.,openshift-tests-extension=true), adding aLabelSelectorto theListOptionswould reduce the API response size and improve performance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/test/extensions/binary.go` around lines 1040 - 1045, The ImageStreamTags list call currently queries all namespaces via imageClient.ImageV1().ImageStreamTags("").List(ctx, metav1.ListOptions{}), which can be large; update the ListOptions to include a LabelSelector (e.g., "openshift-tests-extension=true") so the API returns only extension ImageStreamTags—modify the call that constructs metav1.ListOptions passed to ImageStreamTags("").List to set LabelSelector accordingly and ensure the label key matches the expected non-payload extension label.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/apis/testextension/v1/types.go`:
- Around line 7-18: Add the corresponding list runtime object for the CRD by
declaring a TestExtensionAdmissionList type that implements runtime.Object and
contains metav1.TypeMeta, metav1.ListMeta and an Items []TestExtensionAdmission
slice; keep the existing +genclient on TestExtensionAdmission and add the
appropriate deepcopy-gen/k8s tags (e.g.,
+k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object) on the list
type so the generated client/list/watch surfaces match the CRD's listKind.
In `@pkg/cmd/openshift-tests/extension-admission/testextensionadmission-crd.yaml`:
- Around line 29-36: Add the missing MinItems constraint and tighten the items
pattern so the CRD matches the matcher contract in
pkg/apis/testextension/v1/types.go: for the "permit" array add "minItems: 1" and
change the item's "pattern" so a segment is either a single literal "*" or a
whole name (no embedded asterisks) — i.e. enforce each side of the "/" to be
either "*" or a [A-Za-z0-9\-_]+ token; update the CRD "permit" definition to
reflect this and keep the description in sync with the matcher behavior.
In `@pkg/test/extensions/binary.go`:
- Line 1036: The function parameter name in discoverNonPayloadExtensions is
misspelled as permitPatters; rename it to permitPatterns in the function
signature and update every usage inside discoverNonPayloadExtensions (and any
local variables) to use permitPatterns, and also update any callers or
references in this file that pass or refer to permitPatters (including the
symbol used around the code block near where permitPatters is referenced) so the
identifier is consistent.
In `@pkg/test/extensions/provider.go`:
- Around line 217-228: The override environment-variable key generation for
binaryPathOverride is not sanitizing ':' characters (e.g.,
namespace/imagestream:tag), so exports with that override won't work in shells;
update the sanitizer used by binaryPathOverride (the code path invoked by the
call at binaryPathOverride(imageTag, binaryPath)) to replace ':' with a safe
character (e.g., '_') when constructing the env-var name, then return the
override so TestBinary receives a usable path; ensure the same sanitized form is
used wherever the file builds or reads the override env var.
- Around line 109-113: The fast-path returns a cached file without revalidating
its architecture; update the cache-check block in provider.go to call
checkCompatibleArchitecture(finalBinPath) before returning: if
checkCompatibleArchitecture(finalBinPath) reports compatible, log and return
finalBinPath, 0, nil as before; if it reports incompatible or an error, remove
the stale finalBinPath (os.Remove) and continue with normal download/creation
flow (do not return the invalid path). Reference finalBinPath, imageTag and the
helper checkCompatibleArchitecture when making this change.
- Around line 230-237: The non-payload cache directory is keyed only by imageTag
(safeNameForDir(imageTag)), which allows tag retargeting to reuse stale
binaries; change the key to use the image's immutable identity from imageRef
(e.g., its digest/ID) when building nonPayloadDir so the cache is tied to the
exact image, not the mutable tag; update the call site where nonPayloadDir is
constructed (provider.binPath, safeNameForDir(imageTag)) to derive a stable name
from imageRef (and fall back to tag only if no digest is available) and ensure
any logging or error messages still reference the new nonPayloadDir; keep
provider.extractBinary usage unchanged.
---
Nitpick comments:
In `@pkg/cmd/openshift-tests/extension-admission/extension_admission_command.go`:
- Around line 165-171: The temp file is being closed twice (defer
tmpFile.Close() and an explicit tmpFile.Close()), and defer
os.Remove(tmpFile.Name()) will run before the deferred Close due to LIFO
ordering which can cause remove-before-close issues on some OSes; remove the
deferred tmpFile.Close() and keep the explicit tmpFile.Close() after writing
(around the tmpFile.Write(yamlBytes) error handling) so the file is closed
deterministically before or after os.Remove as intended; look for symbols
tmpFile, tmpFile.Write, tmpFile.Close, os.Remove, and yamlBytes to update the
defers accordingly.
In `@pkg/test/extensions/binary.go`:
- Around line 1040-1045: The ImageStreamTags list call currently queries all
namespaces via imageClient.ImageV1().ImageStreamTags("").List(ctx,
metav1.ListOptions{}), which can be large; update the ListOptions to include a
LabelSelector (e.g., "openshift-tests-extension=true") so the API returns only
extension ImageStreamTags—modify the call that constructs metav1.ListOptions
passed to ImageStreamTags("").List to set LabelSelector accordingly and ensure
the label key matches the expected non-payload extension label.
In `@pkg/test/extensions/non-payload-binary-admission.go`:
- Around line 119-126: The isNotFound(err error) helper currently does fragile
substring checks; replace its body to use the Kubernetes standard error helper
(e.g., apierrors.IsNotFound or kapierrs.IsNotFound) to determine NotFound errors
and remove the manual strings.Contains checks; update imports to include the
appropriate k8s.io/apimachinery/pkg/api/errors (or the project's kapierrs alias)
and ensure all callers of isNotFound(func) continue to work with the same
signature (isNotFound) so discovery/dynamic-client errors are detected reliably.
In `@pkg/test/ginkgo/cmd_runsuite.go`:
- Around line 180-209: The string building in createUnpermittedExtensionTests
currently concatenates with fmt.Sprintf repeatedly (msg variable) which is less
efficient and slightly messy; replace that with a strings.Builder (or use
fmt.Fprintf to the builder) to construct the multi-line message inside the loop
that iterates over unpermitted (for _, u := range unpermitted) and then set
FailureOutput.Output to builder.String(), and keep the existing logrus.Warnf
calls unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9fa7a282-b3f1-4e14-9581-abc5f491e3c7
📒 Files selected for processing (13)
cmd/openshift-tests/openshift-tests.gopkg/apis/testextension/v1/doc.gopkg/apis/testextension/v1/register.gopkg/apis/testextension/v1/types.gopkg/cmd/openshift-tests/extension-admission/extension_admission_command.gopkg/cmd/openshift-tests/extension-admission/testextensionadmission-crd.yamlpkg/cmd/openshift-tests/images/images_command.gopkg/cmd/openshift-tests/list/extensions.gopkg/test/extensions/binary.gopkg/test/extensions/non-payload-binary-admission.gopkg/test/extensions/provider.gopkg/test/ginkgo/cmd_runsuite.gopkg/testsuites/standard_suites.go
pkg/cmd/openshift-tests/extension-admission/testextensionadmission-crd.yaml
Show resolved
Hide resolved
|
Scheduling required tests: |
|
/test e2e-aws-ovn-microshift |
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: c53a075
|
|
@smg247: This pull request references TRT-2295 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@smg247: This pull request references TRT-2295 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@smg247: This pull request references TRT-2295 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/test/extensions/provider.go (1)
152-155: Typo in error message: "compatability" → "compatibility".Suggested fix
// Verify the binary is compatible with our architecture if err := checkCompatibleArchitecture(extractedBinary); err != nil { - return "", 0, errors.WithMessage(err, "error checking binary architecture compatability") + return "", 0, errors.WithMessage(err, "error checking binary architecture compatibility") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/test/extensions/provider.go` around lines 152 - 155, The error message passed to errors.WithMessage in the checkCompatibleArchitecture call contains a typo ("compatability"); update the message string to "error checking binary architecture compatibility" where errors.WithMessage(err, "error checking binary architecture compatability") is used (refer to the checkCompatibleArchitecture invocation and extractedBinary variable) so the log reports the correct spelling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/test/extensions/provider.go`:
- Around line 152-155: The error message passed to errors.WithMessage in the
checkCompatibleArchitecture call contains a typo ("compatability"); update the
message string to "error checking binary architecture compatibility" where
errors.WithMessage(err, "error checking binary architecture compatability") is
used (refer to the checkCompatibleArchitecture invocation and extractedBinary
variable) so the log reports the correct spelling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 40b2afe2-203f-4bdb-a2a6-a05cb6216383
📒 Files selected for processing (7)
pkg/apis/testextension/v1/types.gopkg/cmd/openshift-tests/extension-admission/extension_admission_command.gopkg/cmd/openshift-tests/extension-admission/testextensionadmission-crd.yamlpkg/test/extensions/binary.gopkg/test/extensions/non-payload-binary-admission.gopkg/test/extensions/provider.gopkg/test/ginkgo/cmd_runsuite.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cmd/openshift-tests/extension-admission/extension_admission_command.go
|
Scheduling required tests: |
pkg/cmd/openshift-tests/extension-admission/extension_admission_command.go
Show resolved
Hide resolved
|
@smg247: This pull request references TRT-2295 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Looks great to me! /lgtm |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/cmd/openshift-tests/extension-admission/extension_admission_command.go (1)
192-196: Consider checking the error fromtmpFile.Close().For writes,
Close()can fail if there's an issue flushing buffered data to disk. While unlikely for small YAML files, it's good practice to check.♻️ Proposed fix
if _, err := tmpFile.Write(yamlBytes); err != nil { tmpFile.Close() return fmt.Errorf("failed to write YAML to temp file: %w", err) } - tmpFile.Close() + if err := tmpFile.Close(); err != nil { + return fmt.Errorf("failed to close temp file: %w", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/openshift-tests/extension-admission/extension_admission_command.go` around lines 192 - 196, The code writes YAML bytes to tmpFile using tmpFile.Write(yamlBytes) but currently ignores errors from tmpFile.Close(); update the function to check and handle Close() errors: after the Write error path ensure the tmpFile is closed and its Close() error is checked, and on the normal path replace the un-checked tmpFile.Close() with capturing its error (e.g., err = tmpFile.Close()) and return a wrapped fmt.Errorf if non-nil; reference symbols: tmpFile, Write, Close, yamlBytes.pkg/test/extensions/non_payload_binary_admission.go (1)
120-122: Consider removing the unnecessary wrapper function.
isNotFoundsimply delegates toapierrors.IsNotFound. Usingapierrors.IsNotFounddirectly would be clearer and remove the indirection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/test/extensions/non_payload_binary_admission.go` around lines 120 - 122, The isNotFound wrapper function simply forwards to apierrors.IsNotFound and should be removed to eliminate indirection; delete the isNotFound function definition and update all call sites to call apierrors.IsNotFound(err) directly (search for isNotFound usages in the file and replace them), ensuring imports remain correct and no references to isNotFound remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/cmd/openshift-tests/extension-admission/extension_admission_command.go`:
- Around line 192-196: The code writes YAML bytes to tmpFile using
tmpFile.Write(yamlBytes) but currently ignores errors from tmpFile.Close();
update the function to check and handle Close() errors: after the Write error
path ensure the tmpFile is closed and its Close() error is checked, and on the
normal path replace the un-checked tmpFile.Close() with capturing its error
(e.g., err = tmpFile.Close()) and return a wrapped fmt.Errorf if non-nil;
reference symbols: tmpFile, Write, Close, yamlBytes.
In `@pkg/test/extensions/non_payload_binary_admission.go`:
- Around line 120-122: The isNotFound wrapper function simply forwards to
apierrors.IsNotFound and should be removed to eliminate indirection; delete the
isNotFound function definition and update all call sites to call
apierrors.IsNotFound(err) directly (search for isNotFound usages in the file and
replace them), ensuring imports remain correct and no references to isNotFound
remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dbe04d8a-6074-40b4-a821-ca99680ff849
📒 Files selected for processing (2)
pkg/cmd/openshift-tests/extension-admission/extension_admission_command.gopkg/test/extensions/non_payload_binary_admission.go
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: smg247, stbenjam 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 |
|
Scheduling required tests: |
|
@smg247: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
4 participants
Implements the logic detailed in the enhancement to allow for discovery and extraction of out-of-payload OTE binaries.
Also adds a new subcommand to install the
TestExtensionCRD and apply the matching CRDev Testing workflow:
Summary by CodeRabbit
Release Notes