OCPNODE-4031: Add criocredentialprovider tests#30821
Draft
QiWang19 wants to merge 2 commits intoopenshift:mainfrom
Draft
OCPNODE-4031: Add criocredentialprovider tests#30821QiWang19 wants to merge 2 commits intoopenshift:mainfrom
QiWang19 wants to merge 2 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
WalkthroughThis PR updates two OpenShift API and client-go dependencies in go.mod, exports two MCP-related helper functions in the imagepolicy test module by renaming them from private to public, and introduces a new comprehensive end-to-end test suite for CRIO credential provider configuration in OpenShift Node tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 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)
Comment |
openshift-ci-robot
added
the
jira/valid-reference
|
@QiWang19: This pull request references OCPNODE-4031 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. |
|
@QiWang19: This pull request references OCPNODE-4031 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. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: QiWang19 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
test/extended/node/criocredentialprovider.go (4)
163-184: Inconsistent error handling pattern.Functions like
cleanupCRIOCredentialProviderConfig,createIDMSResources,cleanupIDMSResources,createNamespaceRBAC, andcreateSecretreturnerrorbut always returnnilafter usingo.Expect()internally. Sinceo.Expect()fails the test on error, the return value is never meaningful.Consider either removing the return type (since these are test helpers that fail the test on error) or propagating the actual error for caller handling:
-func cleanupCRIOCredentialProviderConfig(oc *exutil.CLI) error { +func cleanupCRIOCredentialProviderConfig(oc *exutil.CLI) { // ... implementation ... o.Expect(err).NotTo(o.HaveOccurred(), "error cleaning up image config") // ... - return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/node/criocredentialprovider.go` around lines 163 - 184, The helper functions (e.g., cleanupCRIOCredentialProviderConfig, createIDMSResources, cleanupIDMSResources, createNamespaceRBAC, createSecret) declare and return error but call o.Expect(...) which fails the test and then always return nil, making the error return useless; either remove the error return from these helper signatures and callers (convert to void helpers that call o.Expect on failures) or stop using o.Expect inside them and instead return the encountered error (propagate the error up) so callers can handle it—pick one approach and apply consistently: update the function signatures (remove error) or replace o.Expect(...) with returning the error in functions like cleanupCRIOCredentialProviderConfig, and update all call sites accordingly.
14-21: Consider consolidating duplicate import aliases.The file imports the same packages with multiple aliases:
corev1andkapiv1both aliask8s.io/api/core/v1frameworkande2eboth aliask8s.io/kubernetes/test/e2e/framework♻️ Suggested cleanup
- corev1 "k8s.io/api/core/v1" - kapiv1 "k8s.io/api/core/v1" + corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/util/retry" - "k8s.io/kubernetes/test/e2e/framework" e2e "k8s.io/kubernetes/test/e2e/framework"Then update usages of
kapiv1tocorev1andframeworktoe2ethroughout the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/node/criocredentialprovider.go` around lines 14 - 21, There are duplicate import aliases: both `corev1` and `kapiv1` point to "k8s.io/api/core/v1", and `framework` and `e2e` point to "k8s.io/kubernetes/test/e2e/framework"; remove the redundant aliases (keep `corev1` and `e2e`) and update all references in this file (e.g., any uses of `kapiv1` and `framework` within functions like the CRI credential provider test helpers and test cases) to use `corev1` and `e2e` respectively so imports are consolidated and usages are consistent.
305-310: Consider using ServiceAccount kind for RBAC subject.The subject uses
Kind: rbacv1.UserKindwith a manually constructed service account name. The idiomatic approach is to useServiceAccountkind:♻️ Suggested improvement
Subjects: []rbacv1.Subject{ { - APIGroup: rbacv1.GroupName, - Kind: rbacv1.UserKind, - Name: "system:serviceaccount:" + f.Namespace.Name + ":default", + Kind: rbacv1.ServiceAccountKind, + Name: "default", + Namespace: f.Namespace.Name, }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/node/criocredentialprovider.go` around lines 305 - 310, The RBAC Subject is incorrectly using rbacv1.UserKind with a constructed service-account string; change the Subject to use Kind: rbacv1.ServiceAccountKind, clear APIGroup (empty string), put the namespace into Subject.Namespace (e.g. f.Namespace.Name) and use Name: "default" (the SA name) so the Subjects entry (the slice being built) becomes a proper ServiceAccount subject; update the block that constructs the Subject accordingly.
208-213: Redundant type conversions.Since
apicfgv1alpha1.MatchImageis a string type alias, the conversionsstring(apicfgv1alpha1.MatchImage(img))are redundant whenimgis already a string.♻️ Suggested simplification
for _, img := range expectedMatchImages { - o.Expect(out).To(o.ContainSubstring(string(apicfgv1alpha1.MatchImage(img))), "expected match image %s not found in CRIOCredentialProviderConfig on node %s", img, nodeName) + o.Expect(out).To(o.ContainSubstring(img), "expected match image %s not found in CRIOCredentialProviderConfig on node %s", img, nodeName) } for _, img := range excludedMatchImages { - o.Expect(out).NotTo(o.ContainSubstring(string(apicfgv1alpha1.MatchImage(img))), "excluded match image %s found in CRIOCredentialProviderConfig on node %s", img, nodeName) + o.Expect(out).NotTo(o.ContainSubstring(img), "excluded match image %s found in CRIOCredentialProviderConfig on node %s", img, nodeName) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/node/criocredentialprovider.go` around lines 208 - 213, The code is doing redundant conversions like string(apicfgv1alpha1.MatchImage(img)) even though img is already a string; update the two checks to use the plain img (or apicfgv1alpha1.MatchImage(img) without the extra string(...) wrapper) when calling o.ContainSubstring/NotTo(o.ContainSubstring) for variables expectedMatchImages and excludedMatchImages, keeping the rest of the assertion text and references to out, o.Expect and nodeName unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/node/criocredentialprovider.go`:
- Around line 65-66: The test call to
verifyWorkerNodeCRIOCredentialProviderConfig incorrectly passes hardcoded values
(nil, true) instead of the table-driven variables; change the invocation to pass
excludedMatchImages and expectCRIOProviderConfigEntry (i.e., restore the
commented-out form) so the test uses the parameterized values for
excludedMatchImages and expectCRIOProviderConfigEntry when calling
verifyWorkerNodeCRIOCredentialProviderConfig with oc, expectedMatchImages,
workerNodes[0], and credentialProviderConfigPath.
- Around line 346-357: Pod spec omits RestartPolicy, so the container that runs
"exit 0" will keep restarting; update the PodSpec literal where Spec:
kapiv1.PodSpec{... NodeName: nodeName,} to include RestartPolicy:
kapiv1.RestartPolicyNever (same behavior as imagepolicy.launchTestPod) next to
NodeName to ensure the pod does not restart the exited container; look for the
PodSpec that uses contName and image in this file and add the RestartPolicy
field.
- Around line 61-78: The test registers cleanup only in the conditional branch,
causing config changes from updateCRIOCredentialProviderConfig to persist; call
g.DeferCleanup(cleanupCRIOCredentialProviderConfig, oc) immediately after the
initial updateCRIOCredentialProviderConfig(oc, expectedMatchImages, false)
(i.e., unconditionally inside the DescribeTable body) so cleanup is always
registered; keep the existing conditional cleanup registration for the update
path if desired but ensure the initial update always has a DeferCleanup to avoid
test pollution.
- Around line 117-118: The call to e2epod.WaitForPodContainerToFail discards its
error and also uses "ImagePullBackOff" which is a pod condition reason, not a
container termination reason; update the test to capture and check the returned
error from launchTestPod and from e2epod.WaitForPodContainerToFail (or replace
the latter with e2epod.WaitForPodCondition / clif.ClientSet wait helper) to wait
for the pod to reach the ImagePullBackOff pod condition, e.g. call launchTestPod
and handle its error, then call e2epod.WaitForPodCondition (or equivalent) to
assert pod.Status.Reason == "ImagePullBackOff" and fail the test on any returned
error from those calls.
---
Nitpick comments:
In `@test/extended/node/criocredentialprovider.go`:
- Around line 163-184: The helper functions (e.g.,
cleanupCRIOCredentialProviderConfig, createIDMSResources, cleanupIDMSResources,
createNamespaceRBAC, createSecret) declare and return error but call
o.Expect(...) which fails the test and then always return nil, making the error
return useless; either remove the error return from these helper signatures and
callers (convert to void helpers that call o.Expect on failures) or stop using
o.Expect inside them and instead return the encountered error (propagate the
error up) so callers can handle it—pick one approach and apply consistently:
update the function signatures (remove error) or replace o.Expect(...) with
returning the error in functions like cleanupCRIOCredentialProviderConfig, and
update all call sites accordingly.
- Around line 14-21: There are duplicate import aliases: both `corev1` and
`kapiv1` point to "k8s.io/api/core/v1", and `framework` and `e2e` point to
"k8s.io/kubernetes/test/e2e/framework"; remove the redundant aliases (keep
`corev1` and `e2e`) and update all references in this file (e.g., any uses of
`kapiv1` and `framework` within functions like the CRI credential provider test
helpers and test cases) to use `corev1` and `e2e` respectively so imports are
consolidated and usages are consistent.
- Around line 305-310: The RBAC Subject is incorrectly using rbacv1.UserKind
with a constructed service-account string; change the Subject to use Kind:
rbacv1.ServiceAccountKind, clear APIGroup (empty string), put the namespace into
Subject.Namespace (e.g. f.Namespace.Name) and use Name: "default" (the SA name)
so the Subjects entry (the slice being built) becomes a proper ServiceAccount
subject; update the block that constructs the Subject accordingly.
- Around line 208-213: The code is doing redundant conversions like
string(apicfgv1alpha1.MatchImage(img)) even though img is already a string;
update the two checks to use the plain img (or apicfgv1alpha1.MatchImage(img)
without the extra string(...) wrapper) when calling
o.ContainSubstring/NotTo(o.ContainSubstring) for variables expectedMatchImages
and excludedMatchImages, keeping the rest of the assertion text and references
to out, o.Expect and nodeName unchanged.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (105)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.coderabbit.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/.golangci.go-validated.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/types_compatibilityrequirement.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/apps/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/zz_prerelease_lifecycle_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_apiserver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_insights.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_network.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_tlssecurityprofile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_backup.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_crio_credential_provider_config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_insights.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha2/types_insights.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha2/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/console/v1/types_console_sample.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/types_pacemakercluster.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/legacyfeaturegates.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/types_controlplanemachineset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machine/v1beta1/types_awsprovider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_machine.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_machineset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/types_osimagestream.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/types_machineconfiguration.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_network.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/types_clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operatoringress/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operatoringress/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operatoringress/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/acceptrisk.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/clusterversionstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/conditionalupdate.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/conditionalupdaterisk.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/oidcprovider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/tokenclaimvalidationcelrule.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/tokenclaimvalidationrule.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/tokenissuer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/tokenuservalidationrule.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/update.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/clustermonitoringspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/criocredentialproviderconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/criocredentialproviderconfigspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/criocredentialproviderconfigstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/prometheusoperatorconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/config_client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/criocredentialproviderconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/fake/fake_config_client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/fake/fake_criocredentialproviderconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/generated_expansion.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/config/v1alpha1/criocredentialproviderconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/config/v1alpha1/interface.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/generic.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/listers/config/v1alpha1/criocredentialproviderconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/listers/config/v1alpha1/expansion_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1beta1/machinesetstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1beta1/machinestatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterapiinstallercomponent.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterapiinstallercomponentimage.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterapiinstallerrevision.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterapispec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterapistatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/clientset/versioned/typed/operator/v1alpha1/clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (3)
go.modtest/extended/imagepolicy/imagepolicy.gotest/extended/node/criocredentialprovider.go
Comment on lines
+61
to
+78
| g.DescribeTable("criocredentialproviderconfig tests", | ||
| func(expectedMatchImages, updatedMatchImages, excludedMatchImages []string, expectCRIOProviderConfigEntry bool) { | ||
| updateCRIOCredentialProviderConfig(oc, expectedMatchImages, false) | ||
|
|
||
| // verifyWorkerNodeCRIOCredentialProviderConfig(oc, expectedMatchImages, excludedMatchImages, workerNodes[0], credentialProviderConfigPath, expectCRIOProviderConfigEntry) | ||
| verifyWorkerNodeCRIOCredentialProviderConfig(oc, expectedMatchImages, nil, workerNodes[0], credentialProviderConfigPath, true) | ||
|
|
||
| if updatedMatchImages != nil && expectCRIOProviderConfigEntry { | ||
| updateCRIOCredentialProviderConfig(oc, updatedMatchImages, false) | ||
| g.DeferCleanup(cleanupCRIOCredentialProviderConfig, oc) | ||
| verifyWorkerNodeCRIOCredentialProviderConfig(oc, updatedMatchImages, excludedMatchImages, workerNodes[0], credentialProviderConfigPath, true) | ||
| } | ||
|
|
||
| if !expectCRIOProviderConfigEntry { | ||
| cleanupCRIOCredentialProviderConfig(oc) | ||
| verifyWorkerNodeCRIOCredentialProviderConfig(oc, nil, nil, workerNodes[0], credentialProviderConfigPath, false) | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing cleanup registration for all test entries.
The first test entry ("pass update criocredentialproviderconfig...") modifies the config at line 63 but g.DeferCleanup is only registered conditionally inside the if block at line 70. This means the config won't be cleaned up after the first test entry, potentially causing test pollution.
🐛 Proposed fix
g.DescribeTable("criocredentialproviderconfig tests",
func(expectedMatchImages, updatedMatchImages, excludedMatchImages []string, expectCRIOProviderConfigEntry bool) {
updateCRIOCredentialProviderConfig(oc, expectedMatchImages, false)
+ g.DeferCleanup(cleanupCRIOCredentialProviderConfig, oc)
- // verifyWorkerNodeCRIOCredentialProviderConfig(oc, expectedMatchImages, excludedMatchImages, workerNodes[0], credentialProviderConfigPath, expectCRIOProviderConfigEntry)
- verifyWorkerNodeCRIOCredentialProviderConfig(oc, expectedMatchImages, nil, workerNodes[0], credentialProviderConfigPath, true)
+ verifyWorkerNodeCRIOCredentialProviderConfig(oc, expectedMatchImages, excludedMatchImages, workerNodes[0], credentialProviderConfigPath, expectCRIOProviderConfigEntry)
if updatedMatchImages != nil && expectCRIOProviderConfigEntry {
updateCRIOCredentialProviderConfig(oc, updatedMatchImages, false)
- g.DeferCleanup(cleanupCRIOCredentialProviderConfig, oc)
verifyWorkerNodeCRIOCredentialProviderConfig(oc, updatedMatchImages, excludedMatchImages, workerNodes[0], credentialProviderConfigPath, true)
}
- if !expectCRIOProviderConfigEntry {
- cleanupCRIOCredentialProviderConfig(oc)
- verifyWorkerNodeCRIOCredentialProviderConfig(oc, nil, nil, workerNodes[0], credentialProviderConfigPath, false)
- }
-
},📝 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.
Suggested change
| g.DescribeTable("criocredentialproviderconfig tests", | |
| func(expectedMatchImages, updatedMatchImages, excludedMatchImages []string, expectCRIOProviderConfigEntry bool) { | |
| updateCRIOCredentialProviderConfig(oc, expectedMatchImages, false) | |
| // verifyWorkerNodeCRIOCredentialProviderConfig(oc, expectedMatchImages, excludedMatchImages, workerNodes[0], credentialProviderConfigPath, expectCRIOProviderConfigEntry) | |
| verifyWorkerNodeCRIOCredentialProviderConfig(oc, expectedMatchImages, nil, workerNodes[0], credentialProviderConfigPath, true) | |
| if updatedMatchImages != nil && expectCRIOProviderConfigEntry { | |
| updateCRIOCredentialProviderConfig(oc, updatedMatchImages, false) | |
| g.DeferCleanup(cleanupCRIOCredentialProviderConfig, oc) | |
| verifyWorkerNodeCRIOCredentialProviderConfig(oc, updatedMatchImages, excludedMatchImages, workerNodes[0], credentialProviderConfigPath, true) | |
| } | |
| if !expectCRIOProviderConfigEntry { | |
| cleanupCRIOCredentialProviderConfig(oc) | |
| verifyWorkerNodeCRIOCredentialProviderConfig(oc, nil, nil, workerNodes[0], credentialProviderConfigPath, false) | |
| } | |
| g.DescribeTable("criocredentialproviderconfig tests", | |
| func(expectedMatchImages, updatedMatchImages, excludedMatchImages []string, expectCRIOProviderConfigEntry bool) { | |
| updateCRIOCredentialProviderConfig(oc, expectedMatchImages, false) | |
| g.DeferCleanup(cleanupCRIOCredentialProviderConfig, oc) | |
| verifyWorkerNodeCRIOCredentialProviderConfig(oc, expectedMatchImages, excludedMatchImages, workerNodes[0], credentialProviderConfigPath, expectCRIOProviderConfigEntry) | |
| if updatedMatchImages != nil && expectCRIOProviderConfigEntry { | |
| updateCRIOCredentialProviderConfig(oc, updatedMatchImages, false) | |
| verifyWorkerNodeCRIOCredentialProviderConfig(oc, updatedMatchImages, excludedMatchImages, workerNodes[0], credentialProviderConfigPath, true) | |
| } | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/node/criocredentialprovider.go` around lines 61 - 78, The test
registers cleanup only in the conditional branch, causing config changes from
updateCRIOCredentialProviderConfig to persist; call
g.DeferCleanup(cleanupCRIOCredentialProviderConfig, oc) immediately after the
initial updateCRIOCredentialProviderConfig(oc, expectedMatchImages, false)
(i.e., unconditionally inside the DescribeTable body) so cleanup is always
registered; keep the existing conditional cleanup registration for the update
path if desired but ensure the initial update always has a DeferCleanup to avoid
test pollution.
Comment on lines
+65
to
+66
| // verifyWorkerNodeCRIOCredentialProviderConfig(oc, expectedMatchImages, excludedMatchImages, workerNodes[0], credentialProviderConfigPath, expectCRIOProviderConfigEntry) | ||
| verifyWorkerNodeCRIOCredentialProviderConfig(oc, expectedMatchImages, nil, workerNodes[0], credentialProviderConfigPath, true) |
There was a problem hiding this comment.
Test logic is using hardcoded values instead of table parameters.
Line 65 is commented out and line 66 uses hardcoded nil and true instead of the table parameters excludedMatchImages and expectCRIOProviderConfigEntry. This defeats the purpose of the parameterized test.
🐛 Proposed fix
- // verifyWorkerNodeCRIOCredentialProviderConfig(oc, expectedMatchImages, excludedMatchImages, workerNodes[0], credentialProviderConfigPath, expectCRIOProviderConfigEntry)
- verifyWorkerNodeCRIOCredentialProviderConfig(oc, expectedMatchImages, nil, workerNodes[0], credentialProviderConfigPath, true)
+ verifyWorkerNodeCRIOCredentialProviderConfig(oc, expectedMatchImages, excludedMatchImages, workerNodes[0], credentialProviderConfigPath, expectCRIOProviderConfigEntry)📝 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.
Suggested change
| // verifyWorkerNodeCRIOCredentialProviderConfig(oc, expectedMatchImages, excludedMatchImages, workerNodes[0], credentialProviderConfigPath, expectCRIOProviderConfigEntry) | |
| verifyWorkerNodeCRIOCredentialProviderConfig(oc, expectedMatchImages, nil, workerNodes[0], credentialProviderConfigPath, true) | |
| verifyWorkerNodeCRIOCredentialProviderConfig(oc, expectedMatchImages, excludedMatchImages, workerNodes[0], credentialProviderConfigPath, expectCRIOProviderConfigEntry) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/node/criocredentialprovider.go` around lines 65 - 66, The test
call to verifyWorkerNodeCRIOCredentialProviderConfig incorrectly passes
hardcoded values (nil, true) instead of the table-driven variables; change the
invocation to pass excludedMatchImages and expectCRIOProviderConfigEntry (i.e.,
restore the commented-out form) so the test uses the parameterized values for
excludedMatchImages and expectCRIOProviderConfigEntry when calling
verifyWorkerNodeCRIOCredentialProviderConfig with oc, expectedMatchImages,
workerNodes[0], and credentialProviderConfigPath.
Comment on lines
+117
to
+118
| pod, _ := launchTestPod(context.Background(), clif, "dummy-pod", dummypodImage, workerNodes[0].Name) | ||
| e2epod.WaitForPodContainerToFail(context.Background(), clif.ClientSet, pod.Namespace, pod.Name, 0, "ImagePullBackOff", e2e.PodStartShortTimeout) |
There was a problem hiding this comment.
Error handling missing and potential incorrect wait condition.
The return value from WaitForPodContainerToFail is discarded. Additionally, ImagePullBackOff is a pod waiting reason, not typically a container termination reason that WaitForPodContainerToFail checks.
Consider using e2epod.WaitForPodCondition or similar to wait for the expected image pull failure state, and verify the error is handled:
- pod, _ := launchTestPod(context.Background(), clif, "dummy-pod", dummypodImage, workerNodes[0].Name)
- e2epod.WaitForPodContainerToFail(context.Background(), clif.ClientSet, pod.Namespace, pod.Name, 0, "ImagePullBackOff", e2e.PodStartShortTimeout)
+ pod, err := launchTestPod(context.Background(), clif, "dummy-pod", dummypodImage, workerNodes[0].Name)
+ o.Expect(err).NotTo(o.HaveOccurred())
+ g.DeferCleanup(func() { clif.ClientSet.CoreV1().Pods(pod.Namespace).Delete(context.Background(), pod.Name, metav1.DeleteOptions{}) })
+ // Verify image pull failure - consider using WaitForPodCondition for waiting reasons
+ err = e2epod.WaitForPodContainerToFail(context.Background(), clif.ClientSet, pod.Namespace, pod.Name, 0, "ImagePullBackOff", e2e.PodStartShortTimeout)
+ o.Expect(err).NotTo(o.HaveOccurred())#!/bin/bash
# Check WaitForPodContainerToFail signature and usage in e2e framework
ast-grep --pattern 'func WaitForPodContainerToFail($$$) $$$'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/node/criocredentialprovider.go` around lines 117 - 118, The
call to e2epod.WaitForPodContainerToFail discards its error and also uses
"ImagePullBackOff" which is a pod condition reason, not a container termination
reason; update the test to capture and check the returned error from
launchTestPod and from e2epod.WaitForPodContainerToFail (or replace the latter
with e2epod.WaitForPodCondition / clif.ClientSet wait helper) to wait for the
pod to reach the ImagePullBackOff pod condition, e.g. call launchTestPod and
handle its error, then call e2epod.WaitForPodCondition (or equivalent) to assert
pod.Status.Reason == "ImagePullBackOff" and fail the test on any returned error
from those calls.
Comment on lines
+346
to
+357
| Spec: kapiv1.PodSpec{ | ||
| Containers: []kapiv1.Container{ | ||
| { | ||
| Name: contName, | ||
| Image: image, | ||
| ImagePullPolicy: kapiv1.PullAlways, | ||
| Command: []string{"/bin/sh", "-c", "exit 0"}, | ||
| }, | ||
| }, | ||
| NodeName: nodeName, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Missing RestartPolicy in pod spec.
The pod spec is missing RestartPolicy. Without it, the default is Always, which would cause the container running exit 0 to restart continuously. Compare with imagepolicy.launchTestPod which sets RestartPolicy: kapiv1.RestartPolicyNever.
🐛 Proposed fix
Spec: kapiv1.PodSpec{
Containers: []kapiv1.Container{
{
Name: contName,
Image: image,
ImagePullPolicy: kapiv1.PullAlways,
Command: []string{"/bin/sh", "-c", "exit 0"},
},
},
- NodeName: nodeName,
+ NodeName: nodeName,
+ RestartPolicy: kapiv1.RestartPolicyNever,
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/node/criocredentialprovider.go` around lines 346 - 357, Pod
spec omits RestartPolicy, so the container that runs "exit 0" will keep
restarting; update the PodSpec literal where Spec: kapiv1.PodSpec{... NodeName:
nodeName,} to include RestartPolicy: kapiv1.RestartPolicyNever (same behavior as
imagepolicy.launchTestPod) next to NodeName to ensure the pod does not restart
the exited container; look for the PodSpec that uses contName and image in this
file and add the RestartPolicy field.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Chores
Tests