TRT-2295: admit and extract non-payload OTE binaries#30863
TRT-2295: admit and extract non-payload OTE binaries#30863smg247 wants to merge 1 commit 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 change introduces a new TestExtensionAdmission-based feature for controlling non-payload binary extraction in OpenShift tests. It adds a new API group, CRD, discovery mechanism, and CLI command to permit or restrict binary extraction from specific image streams, with unpermitted extensions reported as test failures. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: smg247 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 |
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
| // +genclient | ||
| // +genclient:nonNamespaced | ||
| // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object | ||
|
|
||
| // TestExtensionAdmission controls which ImageStreams are permitted to provide extension test binaries | ||
| type TestExtensionAdmission struct { | ||
| metav1.TypeMeta `json:",inline"` | ||
| metav1.ObjectMeta `json:"metadata,omitempty"` | ||
|
|
||
| Spec TestExtensionAdmissionSpec `json:"spec"` | ||
| Status TestExtensionAdmissionStatus `json:"status,omitempty"` | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether the API package already defines the expected list type.
rg -n -C2 '^\s*// \+genclient|^\s*type TestExtensionAdmission(List)? struct' pkg/apis/testextension/v1Repository: openshift/origin
Length of output: 774
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search the entire testextension v1 package for TestExtensionAdmissionList
echo "=== Checking for TestExtensionAdmissionList in v1 package ==="
rg -n 'TestExtensionAdmissionList' pkg/apis/testextension/v1/
echo -e "\n=== All files in testextension/v1 directory ==="
fd . pkg/apis/testextension/v1/ -type f
echo -e "\n=== Checking for any CRD definition files ==="
rg -n 'listKind.*TestExtensionAdmission' --type yaml --type json pkg/apis/testextension/ 2>/dev/null || echo "No CRD manifests found with listKind"Repository: openshift/origin
Length of output: 121
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List all files in the testextension/v1 package
echo "=== Files in testextension/v1 directory ==="
find pkg/apis/testextension/v1/ -type f 2>/dev/null || echo "Directory not found"
echo -e "\n=== Checking types.go for all type definitions ==="
rg '^\s*type\s+\w+\s*(struct|interface)' pkg/apis/testextension/v1/types.go || echo "No types found"
echo -e "\n=== Searching entire codebase for TestExtensionAdmissionList ==="
rg 'TestExtensionAdmissionList' . --type go 2>/dev/null | head -20 || echo "TestExtensionAdmissionList not found anywhere"
echo -e "\n=== Checking for +genclient annotations in the package ==="
rg '\+genclient' pkg/apis/testextension/v1/ || echo "No genclient annotations found"Repository: openshift/origin
Length of output: 666
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Content of register.go ==="
cat pkg/apis/testextension/v1/register.go
echo -e "\n=== Searching for CRD manifests mentioning TestExtensionAdmission ==="
find . -type f \( -name "*.yaml" -o -name "*.yml" -o -name "*.json" \) -exec grep -l "TestExtensionAdmission" {} \; 2>/dev/null | head -10 || echo "No CRD manifests found"Repository: openshift/origin
Length of output: 459
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== CRD manifest content ==="
cat ./pkg/cmd/openshift-tests/extension-admission/testextensionadmission-crd.yamlRepository: openshift/origin
Length of output: 1555
Add the list runtime object before keeping +genclient here.
The CRD declares listKind: TestExtensionAdmissionList, but the corresponding Go runtime object is missing from the types. With +genclient enabled on the singular type, this mismatch will break generated list/watch client surfaces.
Suggested fix
// +genclient
// +genclient:nonNamespaced
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
// TestExtensionAdmission controls which ImageStreams are permitted to provide extension test binaries
type TestExtensionAdmission struct {
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`
Spec TestExtensionAdmissionSpec `json:"spec"`
Status TestExtensionAdmissionStatus `json:"status,omitempty"`
}
+
+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+type TestExtensionAdmissionList struct {
+ metav1.TypeMeta `json:",inline"`
+ metav1.ListMeta `json:"metadata,omitempty"`
+ Items []TestExtensionAdmission `json:"items"`
+}📝 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.
| // +genclient | |
| // +genclient:nonNamespaced | |
| // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object | |
| // TestExtensionAdmission controls which ImageStreams are permitted to provide extension test binaries | |
| type TestExtensionAdmission struct { | |
| metav1.TypeMeta `json:",inline"` | |
| metav1.ObjectMeta `json:"metadata,omitempty"` | |
| Spec TestExtensionAdmissionSpec `json:"spec"` | |
| Status TestExtensionAdmissionStatus `json:"status,omitempty"` | |
| } | |
| // +genclient | |
| // +genclient:nonNamespaced | |
| // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object | |
| // TestExtensionAdmission controls which ImageStreams are permitted to provide extension test binaries | |
| type TestExtensionAdmission struct { | |
| metav1.TypeMeta `json:",inline"` | |
| metav1.ObjectMeta `json:"metadata,omitempty"` | |
| Spec TestExtensionAdmissionSpec `json:"spec"` | |
| Status TestExtensionAdmissionStatus `json:"status,omitempty"` | |
| } | |
| // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object | |
| type TestExtensionAdmissionList struct { | |
| metav1.TypeMeta `json:",inline"` | |
| metav1.ListMeta `json:"metadata,omitempty"` | |
| Items []TestExtensionAdmission `json:"items"` | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/apis/testextension/v1/types.go` around lines 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.
| permit: | ||
| type: array | ||
| description: List of permitted ImageStream patterns in format "namespace/imagestream". Supports wildcards like "openshift/*", "*/*", or specific "namespace/stream" | ||
| items: | ||
| type: string | ||
| pattern: '^[a-zA-Z0-9\*]([a-zA-Z0-9\-\_\*]*[a-zA-Z0-9\*])?/[a-zA-Z0-9\*]([a-zA-Z0-9\-\_\*]*[a-zA-Z0-9\*])?$' | ||
| required: | ||
| - permit |
There was a problem hiding this comment.
Tighten the CRD schema to the matcher’s actual contract.
This manifest currently accepts configurations the runtime will not honor: it omits the MinItems=1 constraint from pkg/apis/testextension/v1/types.go, and the regex allows embedded * forms even though matching only treats a whole segment * as wildcard. That means values like permit: [] or ns/foo* can be admitted and then degrade into deny-all / never-match behavior.
Possible fix
permit:
type: array
+ minItems: 1
description: List of permitted ImageStream patterns in format "namespace/imagestream". Supports wildcards like "openshift/*", "*/*", or specific "namespace/stream"
items:
type: string
- pattern: '^[a-zA-Z0-9\*]([a-zA-Z0-9\-\_\*]*[a-zA-Z0-9\*])?/[a-zA-Z0-9\*]([a-zA-Z0-9\-\_\*]*[a-zA-Z0-9\*])?$'
+ # Allow either an exact segment or a full-segment "*" wildcard.
+ pattern: '^(\\*|[^/*]+)/(\*|[^/*]+)$'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cmd/openshift-tests/extension-admission/testextensionadmission-crd.yaml`
around lines 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.
|
|
||
| // discoverNonPayloadExtensions lists ImageStreamTags with ComponentAnnotation in all namespaces, | ||
| // parses binary annotation and image ref, and splits into permitted vs unpermitted by the admission result. | ||
| func discoverNonPayloadExtensions(ctx context.Context, oc *exutil.CLI, permitPatters []PermitPattern) (permitted []nonPayloadSource, unpermitted []UnpermittedExtension, err error) { |
There was a problem hiding this comment.
Typo in parameter name: permitPatters → permitPatterns.
Suggested fix
-func discoverNonPayloadExtensions(ctx context.Context, oc *exutil.CLI, permitPatters []PermitPattern) (permitted []nonPayloadSource, unpermitted []UnpermittedExtension, err error) {
- if len(permitPatters) == 0 {
+func discoverNonPayloadExtensions(ctx context.Context, oc *exutil.CLI, permitPatterns []PermitPattern) (permitted []nonPayloadSource, unpermitted []UnpermittedExtension, err error) {
+ if len(permitPatterns) == 0 {Also update line 1079:
- if MatchesAnyPermit(namespace, streamName, permitPatters) {
+ if MatchesAnyPermit(namespace, streamName, permitPatterns) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/test/extensions/binary.go` at 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.
| // Check if the binary already exists in cache | ||
| if _, err := os.Stat(finalBinPath); err == nil { | ||
| logrus.Infof("Using existing binary %s for %s", finalBinPath, imageTag) | ||
| return finalBinPath, 0, nil | ||
| } |
There was a problem hiding this comment.
Revalidate cached binaries before returning them.
The fast-path at Line 110 returns any existing file before checkCompatibleArchitecture runs. A cached artifact created on another host architecture, or left behind after a failed validation, will be treated as valid and only fail later when executed.
Suggested change
- if _, err := os.Stat(finalBinPath); err == nil {
- logrus.Infof("Using existing binary %s for %s", finalBinPath, imageTag)
- return finalBinPath, 0, nil
- }
+ if _, err := os.Stat(finalBinPath); err == nil {
+ if err := checkCompatibleArchitecture(finalBinPath); err != nil {
+ _ = os.Remove(finalBinPath)
+ return "", 0, errors.WithMessage(err, "cached binary has incompatible architecture")
+ }
+ logrus.Infof("Using existing binary %s for %s", finalBinPath, imageTag)
+ return finalBinPath, 0, nil
+ }🤖 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 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.
| // Allow overriding image path to an already existing local path, mostly useful for development | ||
| if override := binaryPathOverride(imageTag, binaryPath); override != "" { | ||
| logrus.WithFields(logrus.Fields{ | ||
| "tag": imageTag, | ||
| "binary": binaryPath, | ||
| "override": override, | ||
| }).Info("Found override for this extension") | ||
| return &TestBinary{ | ||
| imageTag: imageTag, | ||
| binaryPath: override, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Sanitize : in the override env-var key for non-payload tags.
Line 218 now passes values like namespace/imagestream:tag into binaryPathOverride, but the sanitizer later in this file does not replace :. That means the new override path won't work with normal shell export syntax in common dev workflows.
Suggested change
- safeEnvVar := strings.NewReplacer("/", "_", "-", "_", ".", "_")
+ safeEnvVar := strings.NewReplacer("/", "_", "-", "_", ".", "_", ":", "_")🤖 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 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.
| // Create a subdirectory for non-payload binaries to keep them separate | ||
| nonPayloadDir := filepath.Join(provider.binPath, "non-payload", safeNameForDir(imageTag)) | ||
| if err := createBinPath(nonPayloadDir); err != nil { | ||
| return nil, errors.WithMessagef(err, "error creating cache path %s", nonPayloadDir) | ||
| } | ||
|
|
||
| // Verify the binary actually exists | ||
| fileInfo, err := os.Stat(extractedBinary) | ||
| // Extract the binary using common logic | ||
| extractedBinary, extractDuration, err := provider.extractBinary(imageRef, binaryPath, nonPayloadDir, imageTag) |
There was a problem hiding this comment.
Key the non-payload cache by immutable image identity.
Line 231 uses only imageTag for the cache directory. Because tags can be retargeted while this cache survives for days, a later run can silently reuse a binary from the old image even when imageRef has changed.
Suggested change
- nonPayloadDir := filepath.Join(provider.binPath, "non-payload", safeNameForDir(imageTag))
+ nonPayloadDir := filepath.Join(
+ provider.binPath,
+ "non-payload",
+ safeNameForDir(imageTag),
+ pullSpecToDirName(imageRef),
+ )📝 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.
| // Create a subdirectory for non-payload binaries to keep them separate | |
| nonPayloadDir := filepath.Join(provider.binPath, "non-payload", safeNameForDir(imageTag)) | |
| if err := createBinPath(nonPayloadDir); err != nil { | |
| return nil, errors.WithMessagef(err, "error creating cache path %s", nonPayloadDir) | |
| } | |
| // Verify the binary actually exists | |
| fileInfo, err := os.Stat(extractedBinary) | |
| // Extract the binary using common logic | |
| extractedBinary, extractDuration, err := provider.extractBinary(imageRef, binaryPath, nonPayloadDir, imageTag) | |
| // Create a subdirectory for non-payload binaries to keep them separate | |
| nonPayloadDir := filepath.Join( | |
| provider.binPath, | |
| "non-payload", | |
| safeNameForDir(imageTag), | |
| pullSpecToDirName(imageRef), | |
| ) | |
| if err := createBinPath(nonPayloadDir); err != nil { | |
| return nil, errors.WithMessagef(err, "error creating cache path %s", nonPayloadDir) | |
| } | |
| // Extract the binary using common logic | |
| extractedBinary, extractDuration, err := provider.extractBinary(imageRef, binaryPath, nonPayloadDir, imageTag) |
🤖 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 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.
|
Scheduling required tests: |
|
/test e2e-aws-ovn-microshift |
|
@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. |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: c53a075
New tests seen in this PR at sha: c53a075
|
3 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 CRSummary by CodeRabbit
Release Notes
New Features
extension-admissioncommand withinstall-crdandcreatesubcommands to manage extension permissionsRefactor