-
Notifications
You must be signed in to change notification settings - Fork 105
[WIP]Create separate Go module for OTE test extension and migrating OCP-25806 to component repo #563
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughRestructures the extended test extension into a separate Go module under Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Dense test orchestration with multi-step polling logic, significant structural reorganization across layers (build system, dependencies, module layout), heterogeneous file changes spanning tests, utilities, build configuration, and metadata, requiring careful validation of encryption rotation logic and dependency isolation. Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gangwgr The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/extended/apiserver/encryption.go (2)
92-117: Consider allowing test to pass even if cleanup fails.The defer functions restore the operator specs, but cleanup failures (lines 100, 116) cause the entire test to fail via
o.Expect(err).NotTo(o.HaveOccurred()). If the rotation itself succeeded but cleanup failed, you may want to log the cleanup error without failing the test, as the primary test objective (validating rotation) has been met.Consider this pattern instead:
defer func() { e2e.Logf("CLEANUP: Restoring openshiftapiserver/cluster's spec") _, err := operatorClient.OperatorV1().OpenShiftAPIServers().Patch(ctx, "cluster", types.JSONPatchType, restorePatch, metav1.PatchOptions{}) if err != nil { e2e.Logf("WARNING: Failed to restore openshiftapiserver: %v", err) - } else { - e2e.Logf("✓ Successfully restored openshiftapiserver/cluster") } - o.Expect(err).NotTo(o.HaveOccurred()) + // Log cleanup failure but don't fail the test if rotation succeeded }()
204-214: Consider more precise prefix format validation.The current assertions only check if the prefix contains the encryption type substring (lines 204, 207), but don't validate the complete prefix structure. This could allow false positives if the prefix is malformed but happens to contain the expected substring.
Consider adding regex validation for the expected prefix format:
+expectedPrefixPattern := fmt.Sprintf(`^k8s:enc:%s:v1:[^:]+:\d+$`, encryptionType) +prefixRegex := regexp.MustCompile(expectedPrefixPattern) + o.Expect(oasEncValPrefix2).Should(o.ContainSubstring(fmt.Sprintf("k8s:enc:%s:v1", encryptionType))) +o.Expect(prefixRegex.MatchString(oasEncValPrefix2)).To(o.BeTrue(), + fmt.Sprintf("OAS prefix should match expected format: %s", expectedPrefixPattern)) e2e.Logf("✓ OAS prefix contains expected format")test/extended/apiserver/util.go (1)
44-81: Shell command pipeline could be more robust.The command at line 44 chains multiple operations (
etcdctl,head,xargs) without validating intermediate results. If any step returns empty output or fails, the command might succeed but return unexpected results.Consider adding validation after execution:
result := strings.TrimSpace(stdout.String()) +if len(result) == 0 { + e2e.Logf(" ERROR: Empty result from etcdctl command") + return "", fmt.Errorf("etcdctl command returned empty result for key path: %s", keyPath) +} +if len(result) != 32 { + e2e.Logf(" WARNING: Expected 32 bytes but got %d bytes", len(result)) +} e2e.Logf(" Successfully retrieved encryption prefix (length: %d bytes)", len(result)) return result, nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (130)
go.sumis excluded by!**/*.sumvendor/k8s.io/component-base/logs/testinit/testinit.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/.import-restrictionsis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/README.mdis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/bugs.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/expect.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/flake_reporting_util.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/framework.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/get.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/ginkgologger.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/ginkgowrapper.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/internal/junit/junit.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/log.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/namespacedname.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/nodes_util.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/ports.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/provider.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/size.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/test_context.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/testfiles/.import-restrictionsis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/testfiles/testfiles.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/timeouts.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/util.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/README.mdis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/dra/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/dra/dra-test-driver-proxy.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/embed.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/flexvolume/attachable-with-long-mountis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/flexvolume/dummyis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/flexvolume/dummy-attachableis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/gpu/gce/nvidia-driver-installer.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/gpu/gce/nvidia-gpu-device-plugin.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/agnhost-primary-deployment.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/agnhost-primary-service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/agnhost-replica-deployment.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/agnhost-replica-service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/frontend-deployment.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/frontend-service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/legacy/frontend-controller.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/legacy/redis-master-controller.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/legacy/redis-slave-controller.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/redis-master-deployment.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/redis-master-service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/redis-slave-deployment.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/redis-slave-service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/agnhost-primary-controller.json.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/agnhost-primary-pod.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/agnhost-primary-service.jsonis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/busybox-cronjob.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/busybox-pod.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/httpd-deployment1.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/httpd-deployment2.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/httpd-deployment3.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/httpd-rc.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/pause-pod.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/pod-with-readiness-probe.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/podis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/rbd-storage-class.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/sample-device-plugin/sample-device-plugin-control-registration.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/sample-device-plugin/sample-device-plugin.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cassandra/controller.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cassandra/pdb.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cassandra/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cassandra/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cassandra/tester.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cockroachdb/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cockroachdb/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/etcd/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/etcd/pdb.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/etcd/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/etcd/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/etcd/tester.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-galera/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-galera/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-upgrade/configmap.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-upgrade/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-upgrade/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-upgrade/tester.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/nginx/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/nginx/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/redis/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/redis/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/zookeeper/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/zookeeper/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/any-volume-datasource/crd/hello-populator-crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/any-volume-datasource/crd/populator.storage.k8s.io_volumepopulators.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/any-volume-datasource/hello-populator-deploy.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/any-volume-datasource/volume-data-source-validator/rbac-data-source-validator.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/any-volume-datasource/volume-data-source-validator/setup-data-source-validator.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/controller-role.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-attacher/rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-health-monitor/external-health-monitor-controller/rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-provisioner/rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-resizer/rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/csi-snapshotter/rbac-csi-snapshotter.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/groupsnapshot.storage.k8s.io_volumegroupsnapshotclasses.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/groupsnapshot.storage.k8s.io_volumegroupsnapshotcontents.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/groupsnapshot.storage.k8s.io_volumegroupsnapshots.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/volume-group-snapshots/csi-hostpath-plugin.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/volume-group-snapshots/run_group_snapshot_e2e.shis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/gce-pd/controller_ss.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/gce-pd/csi-controller-rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/gce-pd/node_ds.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/README.mdis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/hostpath/csi-hostpath-driverinfo.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/hostpath/csi-hostpath-plugin.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/hostpath/csi-hostpath-snapshotclass.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/hostpath/csi-hostpath-testing.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/hostpath/e2e-test-rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-driver-attacher.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-driver-resizer.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-driver-snapshotter.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-driver.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-driverinfo.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-proxy.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-storageclass.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/update-hostpath.shis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/utils/image/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/utils/image/csi_manifest.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/utils/image/manifest.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/utils/kubeconfig/kubeconfig.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/pod-security-admission/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/pod-security-admission/api/attributes.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/pod-security-admission/api/constants.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/pod-security-admission/api/doc.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/pod-security-admission/api/helpers.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (4)
cmd/openshift-apiserver-tests-ext/main.go(1 hunks)go.mod(1 hunks)test/extended/apiserver/encryption.go(1 hunks)test/extended/apiserver/util.go(1 hunks)
🔇 Additional comments (8)
go.mod (1)
186-186: LGTM! Standard Go modules pattern with replace directive.The v0.0.0 placeholder is correctly mapped to v0.33.3 via the replace directive at line 225, aligning with other Kubernetes v0.33.3 dependencies in this module.
cmd/openshift-apiserver-tests-ext/main.go (1)
25-25: LGTM! Proper test registration pattern.The blank import correctly registers the new apiserver encryption tests with the test extension framework, following Go conventions and the existing pattern.
test/extended/apiserver/encryption.go (4)
3-21: LGTM! Well-organized imports.All imports are necessary for the test implementation and follow standard Go conventions.
23-27: LGTM! Appropriate test markers.The test is correctly marked as [Serial][Slow][Disruptive], which is essential for encryption rotation tests that modify cluster-wide configuration and require sequential execution.
29-50: LGTM! Proper client initialization.Client setup follows best practices with appropriate error handling and helpful logging.
52-63: LGTM! Appropriate guard clause.The test correctly skips execution on clusters without encryption enabled, preventing false failures.
test/extended/apiserver/util.go (2)
85-116: LGTM! Regex-based key number extraction is sound.The function correctly extracts the highest encryption key number from secret names using regex matching. The dual-pattern approach (line 97-98) appropriately separates secret identification from number extraction.
148-154: Verify that checking only "secrets" is sufficient for migration completion.Line 151 only verifies that "secrets" appears in the migrated-resources annotation. The comment at line 150 mentions "all resources," suggesting there might be multiple resource types to migrate. Ensure this check aligns with the actual migration requirements for KAS.
If multiple resources need migration, consider checking for all expected resources:
if migratedResources, ok := annotations["encryption.apiserver.operator.openshift.io/migrated-resources"]; ok { e2e.Logf(" [Poll #%d] Migrated resources annotation found: %s", pollCount, migratedResources) - // When all resources are migrated, check if the expected resources are present - if strings.Contains(migratedResources, "secrets") { + // For KAS, verify that secrets are migrated (primary resource) + // For OAS, may need to check additional resources like routes, templates, etc. + expectedResource := "secrets" + if strings.Contains(migratedResources, expectedResource) { e2e.Logf(" [Poll #%d] ✓ MIGRATION COMPLETE! 'secrets' found in migrated resources", pollCount) return true, nil }
| // Only need to check kubeapiserver because kubeapiserver takes more time. | ||
| completed, err := waitEncryptionKeyMigration(ctx, kubeClient, newKASEncSecretName) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), fmt.Sprintf("saw all migrated-resources for %s", newKASEncSecretName)) | ||
| o.Expect(completed).Should(o.Equal(true)) | ||
| e2e.Logf("✓ Encryption migration completed successfully!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify that checking only KAS migration is sufficient.
Line 186 only waits for kubeapiserver encryption migration to complete, not openshiftapiserver. The comment states that KAS takes more time, but this assumption may not hold in all cluster configurations. If OAS migration completes later, the subsequent prefix verification (lines 193-214) could fail or show inconsistent state.
Consider validating both migrations or documenting why OAS migration completion can be safely assumed when KAS completes:
🏁 Script executed:
#!/bin/bash
# Search for any existing patterns in the codebase where both OAS and KAS migrations are checked
rg -nC3 'waitEncryptionKeyMigration.*openshift-apiserver' --type=go
rg -nC3 'waitEncryptionKeyMigration.*kube-apiserver' --type=goLength of output: 0
🏁 Script executed:
#!/bin/bash
# List all occurrences of waitEncryptionKeyMigration in Go code
rg -n 'waitEncryptionKeyMigration' --type=goLength of output: 392
🏁 Script executed:
#!/bin/bash
# Examine the waitEncryptionKeyMigration function definition
sed -n '118,150p' test/extended/apiserver/util.go
# Check the broader context in encryption.go around the wait call (lines 170-220)
sed -n '170,220p' test/extended/apiserver/encryption.goLength of output: 4409
🏁 Script executed:
#!/bin/bash
# Search for how newKASEncSecretName and potentially OAS secret names are defined
sed -n '1,185p' test/extended/apiserver/encryption.go | grep -n 'EncSecretName\|newKAS\|newOAS\|openshift-apiserver\|kube-apiserver'
# Check if there are separate secret names for OAS and KAS
rg -n 'EncSecretName' test/extended/apiserver/encryption.go -B2 -A2Length of output: 3657
🏁 Script executed:
#!/bin/bash
# Check if there is any documentation or coupling logic in the util.go file
# that explains why OAS migration doesn't need to be checked separately
sed -n '100,200p' test/extended/apiserver/util.go
# Search for any references to migration coupling or sequencing
rg -n 'OAS.*KAS|openshift-apiserver.*kube-apiserver|migrated-resources' test/extended/apiserver/ --type=go -B1 -A1Length of output: 4764
Await openshift-apiserver migration before verifying prefixes. The test only calls waitEncryptionKeyMigration for newKASEncSecretName (line 186); add a corresponding wait for newOASEncSecretName or document why checking KAS alone guarantees both migrations complete.
🤖 Prompt for AI Agents
In test/extended/apiserver/encryption.go around lines 185 to 189: the test only
waits for the kube-apiserver (newKASEncSecretName) migration, but the reviewer
requests awaiting the openshift-apiserver migration as well; add a second call
to waitEncryptionKeyMigration with newOASEncSecretName (mirror the error
handling and expectations used for KAS) before verifying prefixes, or
alternatively add an explicit comment explaining why waiting on KAS alone
guarantees OAS migration and update the test expectations accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/extended/apiserver/util.go (2)
40-42: Pick a Running etcd pod to avoid flakinessSelecting the first item may choose a non-Running pod/container. Prefer a Running pod; fail fast if none.
- podName := pods.Items[0].Name - e2e.Logf(" Using etcd pod: %s (total etcd pods: %d)", podName, len(pods.Items)) + var podName string + for _, p := range pods.Items { + if p.Status.Phase == corev1.PodRunning { + podName = p.Name + break + } + } + if podName == "" { + e2e.Logf(" ERROR: No running etcd pods found in openshift-etcd namespace") + return "", fmt.Errorf("no running etcd pods found") + } + e2e.Logf(" Using etcd pod: %s (total etcd pods: %d)", podName, len(pods.Items))
18-18: Use client-go ParameterCodec instead of kubectl’s schemeAvoid importing kubectl’s scheme in library/test helpers. Use k8s.io/client-go/kubernetes/scheme ParameterCodec.
- "k8s.io/kubectl/pkg/scheme" + k8sscheme "k8s.io/client-go/kubernetes/scheme"- }, scheme.ParameterCodec) + }, k8sscheme.ParameterCodec)Also applies to: 58-58
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (130)
go.sumis excluded by!**/*.sumvendor/k8s.io/component-base/logs/testinit/testinit.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/.import-restrictionsis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/README.mdis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/bugs.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/expect.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/flake_reporting_util.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/framework.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/get.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/ginkgologger.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/ginkgowrapper.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/internal/junit/junit.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/log.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/namespacedname.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/nodes_util.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/ports.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/provider.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/size.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/test_context.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/testfiles/.import-restrictionsis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/testfiles/testfiles.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/timeouts.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/util.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/README.mdis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/dra/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/dra/dra-test-driver-proxy.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/embed.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/flexvolume/attachable-with-long-mountis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/flexvolume/dummyis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/flexvolume/dummy-attachableis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/gpu/gce/nvidia-driver-installer.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/gpu/gce/nvidia-gpu-device-plugin.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/agnhost-primary-deployment.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/agnhost-primary-service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/agnhost-replica-deployment.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/agnhost-replica-service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/frontend-deployment.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/frontend-service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/legacy/frontend-controller.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/legacy/redis-master-controller.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/legacy/redis-slave-controller.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/redis-master-deployment.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/redis-master-service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/redis-slave-deployment.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/redis-slave-service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/agnhost-primary-controller.json.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/agnhost-primary-pod.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/agnhost-primary-service.jsonis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/busybox-cronjob.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/busybox-pod.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/httpd-deployment1.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/httpd-deployment2.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/httpd-deployment3.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/httpd-rc.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/pause-pod.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/pod-with-readiness-probe.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/podis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/rbd-storage-class.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/sample-device-plugin/sample-device-plugin-control-registration.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/sample-device-plugin/sample-device-plugin.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cassandra/controller.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cassandra/pdb.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cassandra/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cassandra/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cassandra/tester.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cockroachdb/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cockroachdb/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/etcd/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/etcd/pdb.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/etcd/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/etcd/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/etcd/tester.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-galera/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-galera/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-upgrade/configmap.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-upgrade/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-upgrade/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-upgrade/tester.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/nginx/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/nginx/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/redis/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/redis/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/zookeeper/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/zookeeper/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/any-volume-datasource/crd/hello-populator-crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/any-volume-datasource/crd/populator.storage.k8s.io_volumepopulators.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/any-volume-datasource/hello-populator-deploy.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/any-volume-datasource/volume-data-source-validator/rbac-data-source-validator.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/any-volume-datasource/volume-data-source-validator/setup-data-source-validator.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/controller-role.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-attacher/rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-health-monitor/external-health-monitor-controller/rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-provisioner/rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-resizer/rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/csi-snapshotter/rbac-csi-snapshotter.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/groupsnapshot.storage.k8s.io_volumegroupsnapshotclasses.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/groupsnapshot.storage.k8s.io_volumegroupsnapshotcontents.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/groupsnapshot.storage.k8s.io_volumegroupsnapshots.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/volume-group-snapshots/csi-hostpath-plugin.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/volume-group-snapshots/run_group_snapshot_e2e.shis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/gce-pd/controller_ss.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/gce-pd/csi-controller-rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/gce-pd/node_ds.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/README.mdis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/hostpath/csi-hostpath-driverinfo.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/hostpath/csi-hostpath-plugin.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/hostpath/csi-hostpath-snapshotclass.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/hostpath/csi-hostpath-testing.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/hostpath/e2e-test-rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-driver-attacher.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-driver-resizer.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-driver-snapshotter.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-driver.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-driverinfo.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-proxy.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-storageclass.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/update-hostpath.shis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/utils/image/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/utils/image/csi_manifest.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/utils/image/manifest.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/utils/kubeconfig/kubeconfig.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/pod-security-admission/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/pod-security-admission/api/attributes.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/pod-security-admission/api/constants.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/pod-security-admission/api/doc.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/pod-security-admission/api/helpers.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (4)
cmd/openshift-apiserver-tests-ext/main.go(1 hunks)go.mod(1 hunks)test/extended/apiserver/encryption.go(1 hunks)test/extended/apiserver/util.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/extended/apiserver/encryption.go
- go.mod
🔇 Additional comments (2)
cmd/openshift-apiserver-tests-ext/main.go (1)
25-26: LGTM: registering extended apiserver tests via blank importThis ensures the suite’s init hooks run. No issues.
test/extended/apiserver/util.go (1)
28-31: No repository references to ‘k8s-app=etcd’; verify that OpenShift etcd pods actually use this label before adding fallback logic.
| maxNumber := 0 | ||
| matchCount := 0 | ||
| re := regexp.MustCompile(pattern) | ||
| numberRe := regexp.MustCompile(`-(\d+)$`) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid panic on invalid regex pattern
regexp.MustCompile(pattern) will panic if pattern is invalid. Compile and return a proper error instead.
- maxNumber := 0
- matchCount := 0
- re := regexp.MustCompile(pattern)
+ maxNumber := 0
+ matchCount := 0
+ re, err := regexp.Compile(pattern)
+ if err != nil {
+ return 0, fmt.Errorf("invalid regex pattern %q: %w", pattern, err)
+ }
numberRe := regexp.MustCompile(`-(\d+)$`)📝 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.
| maxNumber := 0 | |
| matchCount := 0 | |
| re := regexp.MustCompile(pattern) | |
| numberRe := regexp.MustCompile(`-(\d+)$`) | |
| maxNumber := 0 | |
| matchCount := 0 | |
| re, err := regexp.Compile(pattern) | |
| if err != nil { | |
| return 0, fmt.Errorf("invalid regex pattern %q: %w", pattern, err) | |
| } | |
| numberRe := regexp.MustCompile(`-(\d+)$`) |
🤖 Prompt for AI Agents
In test/extended/apiserver/util.go around lines 95-99, replace
regexp.MustCompile(pattern) with regexp.Compile(pattern) and handle the returned
error instead of allowing a panic; update the function signature to return an
error (or propagate the error to the caller) so callers can handle invalid
regexes, and keep or leave numberRe as a MustCompile if it's constant and
guaranteed valid or similarly compile it with error handling if you prefer
consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/extended/apiserver/util.go (1)
95-99: Avoid panic on invalid regex pattern.As noted in the previous review,
regexp.MustCompile(pattern)will panic ifpatternis invalid. Useregexp.Compileand return a proper error instead.Apply this diff:
maxNumber := 0 matchCount := 0 - re := regexp.MustCompile(pattern) + re, err := regexp.Compile(pattern) + if err != nil { + return 0, fmt.Errorf("invalid regex pattern %q: %w", pattern, err) + } numberRe := regexp.MustCompile(`-(\d+)$`)
🧹 Nitpick comments (1)
test/extended/apiserver/util.go (1)
98-98: Consider consistency in regex compilation.For consistency with the error handling added for line 97, you could also use
regexp.CompilefornumberRe. However, since the pattern is a constant and guaranteed valid, usingMustCompileis acceptable here.If you prefer consistency:
- numberRe := regexp.MustCompile(`-(\d+)$`) + numberRe, _ := regexp.Compile(`-(\d+)$`) // constant pattern, error impossible
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (130)
go.sumis excluded by!**/*.sumvendor/k8s.io/component-base/logs/testinit/testinit.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/.import-restrictionsis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/README.mdis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/bugs.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/expect.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/flake_reporting_util.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/framework.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/get.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/ginkgologger.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/ginkgowrapper.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/internal/junit/junit.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/log.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/namespacedname.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/nodes_util.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/ports.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/provider.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/size.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/test_context.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/testfiles/.import-restrictionsis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/testfiles/testfiles.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/timeouts.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/util.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/README.mdis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/dra/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/dra/dra-test-driver-proxy.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/embed.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/flexvolume/attachable-with-long-mountis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/flexvolume/dummyis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/flexvolume/dummy-attachableis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/gpu/gce/nvidia-driver-installer.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/gpu/gce/nvidia-gpu-device-plugin.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/agnhost-primary-deployment.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/agnhost-primary-service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/agnhost-replica-deployment.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/agnhost-replica-service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/frontend-deployment.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/frontend-service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/legacy/frontend-controller.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/legacy/redis-master-controller.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/legacy/redis-slave-controller.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/redis-master-deployment.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/redis-master-service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/redis-slave-deployment.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/redis-slave-service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/agnhost-primary-controller.json.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/agnhost-primary-pod.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/agnhost-primary-service.jsonis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/busybox-cronjob.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/busybox-pod.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/httpd-deployment1.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/httpd-deployment2.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/httpd-deployment3.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/httpd-rc.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/pause-pod.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/pod-with-readiness-probe.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/podis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/rbd-storage-class.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/sample-device-plugin/sample-device-plugin-control-registration.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/sample-device-plugin/sample-device-plugin.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cassandra/controller.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cassandra/pdb.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cassandra/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cassandra/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cassandra/tester.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cockroachdb/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cockroachdb/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/etcd/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/etcd/pdb.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/etcd/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/etcd/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/etcd/tester.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-galera/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-galera/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-upgrade/configmap.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-upgrade/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-upgrade/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-upgrade/tester.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/nginx/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/nginx/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/redis/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/redis/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/zookeeper/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/zookeeper/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/any-volume-datasource/crd/hello-populator-crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/any-volume-datasource/crd/populator.storage.k8s.io_volumepopulators.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/any-volume-datasource/hello-populator-deploy.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/any-volume-datasource/volume-data-source-validator/rbac-data-source-validator.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/any-volume-datasource/volume-data-source-validator/setup-data-source-validator.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/controller-role.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-attacher/rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-health-monitor/external-health-monitor-controller/rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-provisioner/rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-resizer/rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/csi-snapshotter/rbac-csi-snapshotter.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/groupsnapshot.storage.k8s.io_volumegroupsnapshotclasses.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/groupsnapshot.storage.k8s.io_volumegroupsnapshotcontents.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/groupsnapshot.storage.k8s.io_volumegroupsnapshots.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/volume-group-snapshots/csi-hostpath-plugin.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/volume-group-snapshots/run_group_snapshot_e2e.shis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/gce-pd/controller_ss.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/gce-pd/csi-controller-rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/gce-pd/node_ds.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/README.mdis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/hostpath/csi-hostpath-driverinfo.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/hostpath/csi-hostpath-plugin.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/hostpath/csi-hostpath-snapshotclass.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/hostpath/csi-hostpath-testing.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/hostpath/e2e-test-rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-driver-attacher.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-driver-resizer.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-driver-snapshotter.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-driver.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-driverinfo.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-proxy.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-storageclass.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/update-hostpath.shis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/utils/image/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/utils/image/csi_manifest.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/utils/image/manifest.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/utils/kubeconfig/kubeconfig.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/pod-security-admission/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/pod-security-admission/api/attributes.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/pod-security-admission/api/constants.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/pod-security-admission/api/doc.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/pod-security-admission/api/helpers.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (4)
cmd/openshift-apiserver-tests-ext/main.go(1 hunks)go.mod(1 hunks)test/extended/apiserver/encryption.go(1 hunks)test/extended/apiserver/util.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- test/extended/apiserver/encryption.go
🔇 Additional comments (2)
cmd/openshift-apiserver-tests-ext/main.go (1)
25-25: LGTM!The side-effect import correctly registers the apiserver tests with the extension framework, following the established pattern.
test/extended/apiserver/util.go (1)
118-171: LGTM!The migration polling logic is well-structured with:
- Appropriate timeout (25 minutes) and polling interval (30 seconds) for integration tests
- Comprehensive logging for debugging
- Correct error handling and context propagation
- Clear success condition checking for the annotation
| e2e.Logf(" Using etcd pod: %s (total etcd pods: %d)", podName, len(pods.Items)) | ||
|
|
||
| // Execute etcdctl command to get the encryption prefix | ||
| command := fmt.Sprintf("etcdctl get %s --prefix --keys-only --limit=1 | head -1 | xargs -I {} etcdctl get {} --print-value-only | head -c 32", keyPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential command injection vulnerability.
The keyPath parameter is directly interpolated into a shell command without escaping or validation. If keyPath contains shell metacharacters (e.g., ;, |, $(), &&), arbitrary commands could be executed in the etcd container.
While this is test code and keyPath is likely controlled, defensive coding practices suggest validating or escaping the input.
Consider one of these mitigations:
Option 1: Validate keyPath format
func getEncryptionPrefix(ctx context.Context, config *rest.Config, kubeClient kubernetes.Interface, keyPath string) (string, error) {
e2e.Logf(" Getting encryption prefix for etcd key path: %s", keyPath)
+
+ // Validate keyPath format to prevent command injection
+ if strings.ContainsAny(keyPath, ";|&$`()<>") {
+ return "", fmt.Errorf("invalid keyPath contains shell metacharacters: %s", keyPath)
+ }
// Get first etcd podOption 2: Use shell escaping (requires shellquote library)
import "github.com/kballard/go-shellquote"
command := fmt.Sprintf("etcdctl get %s --prefix --keys-only --limit=1 | head -1 | xargs -I {} etcdctl get {} --print-value-only | head -c 32",
shellquote.Join(keyPath))🤖 Prompt for AI Agents
In test/extended/apiserver/util.go around line 44, the shell command
interpolates keyPath directly which allows command injection; fix by either
validating keyPath against a restrictive safe pattern (e.g., only allow
alphanumerics, slashes, dashes, underscores and return an error if it contains
any other characters) or by escaping it before interpolation using a
shell-quoting helper (add the github.com/kballard/go-shellquote import and call
shellquote.Join(keyPath) when building the command); ensure imports updated and
tests adjusted to handle validation errors if you choose validation.
5b75ac8 to
1f1363b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/extended/apiserver/encryption.go (1)
193-206: Still need to wait for OAS migration before verifying prefixes.We only block on
waitEncryptionKeyMigration(..., newKASEncSecretName). If the openshift-apiserver migration lags behind kube-apiserver, the OAS prefix assertions immediately below can read the pre-rotation value and fail the test. Please add a corresponding wait fornewOASEncSecretName(or otherwise prove OAS is done) before checking the prefixes—this was requested earlier and remains unresolved.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (130)
go.sumis excluded by!**/*.sumvendor/k8s.io/component-base/logs/testinit/testinit.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/.import-restrictionsis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/README.mdis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/bugs.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/expect.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/flake_reporting_util.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/framework.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/get.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/ginkgologger.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/ginkgowrapper.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/internal/junit/junit.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/log.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/namespacedname.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/nodes_util.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/ports.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/provider.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/size.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/test_context.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/testfiles/.import-restrictionsis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/testfiles/testfiles.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/timeouts.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/framework/util.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/README.mdis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/dra/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/dra/dra-test-driver-proxy.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/embed.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/flexvolume/attachable-with-long-mountis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/flexvolume/dummyis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/flexvolume/dummy-attachableis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/gpu/gce/nvidia-driver-installer.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/gpu/gce/nvidia-gpu-device-plugin.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/agnhost-primary-deployment.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/agnhost-primary-service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/agnhost-replica-deployment.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/agnhost-replica-service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/frontend-deployment.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/frontend-service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/legacy/frontend-controller.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/legacy/redis-master-controller.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/legacy/redis-slave-controller.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/redis-master-deployment.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/redis-master-service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/redis-slave-deployment.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/guestbook/redis-slave-service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/agnhost-primary-controller.json.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/agnhost-primary-pod.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/agnhost-primary-service.jsonis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/busybox-cronjob.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/busybox-pod.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/httpd-deployment1.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/httpd-deployment2.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/httpd-deployment3.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/httpd-rc.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/pause-pod.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/kubectl/pod-with-readiness-probe.yaml.inis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/podis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/rbd-storage-class.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/sample-device-plugin/sample-device-plugin-control-registration.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/sample-device-plugin/sample-device-plugin.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cassandra/controller.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cassandra/pdb.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cassandra/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cassandra/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cassandra/tester.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cockroachdb/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/cockroachdb/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/etcd/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/etcd/pdb.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/etcd/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/etcd/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/etcd/tester.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-galera/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-galera/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-upgrade/configmap.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-upgrade/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-upgrade/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/mysql-upgrade/tester.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/nginx/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/nginx/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/redis/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/redis/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/zookeeper/service.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/statefulset/zookeeper/statefulset.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/any-volume-datasource/crd/hello-populator-crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/any-volume-datasource/crd/populator.storage.k8s.io_volumepopulators.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/any-volume-datasource/hello-populator-deploy.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/any-volume-datasource/volume-data-source-validator/rbac-data-source-validator.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/any-volume-datasource/volume-data-source-validator/setup-data-source-validator.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/controller-role.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-attacher/rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-health-monitor/external-health-monitor-controller/rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-provisioner/rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-resizer/rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/csi-snapshotter/rbac-csi-snapshotter.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/groupsnapshot.storage.k8s.io_volumegroupsnapshotclasses.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/groupsnapshot.storage.k8s.io_volumegroupsnapshotcontents.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/groupsnapshot.storage.k8s.io_volumegroupsnapshots.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/volume-group-snapshots/csi-hostpath-plugin.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/external-snapshotter/volume-group-snapshots/run_group_snapshot_e2e.shis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/gce-pd/controller_ss.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/gce-pd/csi-controller-rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/gce-pd/node_ds.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/README.mdis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/hostpath/csi-hostpath-driverinfo.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/hostpath/csi-hostpath-plugin.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/hostpath/csi-hostpath-snapshotclass.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/hostpath/csi-hostpath-testing.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/hostpath/hostpath/e2e-test-rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-driver-attacher.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-driver-resizer.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-driver-snapshotter.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-driver.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-driverinfo.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-proxy.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-mock-rbac.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/mock/csi-storageclass.yamlis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/e2e/testing-manifests/storage-csi/update-hostpath.shis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/utils/image/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/utils/image/csi_manifest.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/utils/image/manifest.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/kubernetes/test/utils/kubeconfig/kubeconfig.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/pod-security-admission/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/pod-security-admission/api/attributes.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/pod-security-admission/api/constants.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/pod-security-admission/api/doc.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/pod-security-admission/api/helpers.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (4)
cmd/openshift-apiserver-tests-ext/main.go(1 hunks)go.mod(1 hunks)test/extended/apiserver/encryption.go(1 hunks)test/extended/apiserver/util.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/openshift-apiserver-tests-ext/main.go
- go.mod
| annotations := secret.GetAnnotations() | ||
| if migratedResources, ok := annotations["encryption.apiserver.operator.openshift.io/migrated-resources"]; ok { | ||
| e2e.Logf(" [Poll #%d] Migrated resources annotation found: %s", pollCount, migratedResources) | ||
| // When all resources are migrated, check if the expected resources are present | ||
| if strings.Contains(migratedResources, "secrets") { | ||
| e2e.Logf(" [Poll #%d] MIGRATION COMPLETE! 'secrets' found in migrated resources", pollCount) | ||
| return true, nil | ||
| } | ||
| e2e.Logf(" [Poll #%d] Migration in progress (waiting for 'secrets' in annotation)", pollCount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generalize migrated-resource check so it works for OAS too.
waitEncryptionKeyMigration currently succeeds only when the annotation contains "secrets". That’s correct for kube-apiserver but fails for openshift-apiserver, whose annotation lists different resources (e.g., routes.route.openshift.io). To unblock an OAS wait, parameterize the expected resource(s) or otherwise make the check configurable and update callers accordingly.
🤖 Prompt for AI Agents
In test/extended/apiserver/util.go around lines 148 to 156, the migration check
only looks for the literal string "secrets" in the migrated-resources annotation
which works for kube-apiserver but fails for openshift-apiserver; change the
function to accept one or more expected resource identifiers (e.g., a string or
[]string) and test the annotation for any of those values instead of the
hard-coded "secrets"; update all callers to pass the appropriate expected
resource(s) (for KAS pass "secrets", for OAS pass the OAS-specific resources
such as "routes.route.openshift.io" or a list), and preserve existing logging
and return behavior when a match is found or still pending.
|
/test e2e-aws-ovn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
♻️ Duplicate comments (3)
test/extended/tests-extension/apiserver/util.go (3)
148-156: Parameterize migrated-resources checkHardcoding "secrets" makes this helper KAS-specific. Accept expected resource(s) as input and match accordingly (e.g., routes.route.openshift.io for OAS).
-func waitEncryptionKeyMigration(ctx context.Context, kubeClient kubernetes.Interface, secretName string) (bool, error) { +func waitEncryptionKeyMigration(ctx context.Context, kubeClient kubernetes.Interface, secretName string, expected ...string) (bool, error) { ... - if migratedResources, ok := annotations["encryption.apiserver.operator.openshift.io/migrated-resources"]; ok { + if migratedResources, ok := annotations["encryption.apiserver.operator.openshift.io/migrated-resources"]; ok { ... - if strings.Contains(migratedResources, "secrets") { + for _, exp := range expected { + if strings.Contains(migratedResources, exp) { + ... + return true, nil + } + } ...Update callers to pass the expected resource(s).
43-56: Validate or escape keyPath to avoid command injectionkeyPath is interpolated into a shell pipeline. Validate against a safe charset or shell-escape before use.
- command := fmt.Sprintf("etcdctl get %s --prefix --keys-only --limit=1 | head -1 | xargs -I {} etcdctl get {} --print-value-only | head -c 32", keyPath) + // Allow only safe characters in etcd key prefixes. + if strings.ContainsAny(keyPath, ";|&$`()<>") { + return "", fmt.Errorf("invalid keyPath contains shell metacharacters: %q", keyPath) + } + command := fmt.Sprintf("etcdctl get %s --prefix --keys-only --limit=1 | head -1 | xargs -I {} etcdctl get {} --print-value-only | head -c 32", keyPath)
95-99: Avoid panic on invalid regex patternregexp.MustCompile(pattern) will panic on bad input. Compile and handle the error.
- re := regexp.MustCompile(pattern) + re, err := regexp.Compile(pattern) + if err != nil { + return 0, fmt.Errorf("invalid regex pattern %q: %w", pattern, err) + }
🧹 Nitpick comments (11)
images/Dockerfile.rhel (1)
6-6: Make gzip deterministic and smallerUse -n to omit mtime/owner for reproducible builds and -9 for max compression.
- && gzip test/extended/tests-extension/openshift-apiserver-tests-ext + && gzip -n -9 test/extended/tests-extension/openshift-apiserver-tests-exttest/extended/tests-extension/go.mod (1)
5-15: Align Ginkgo require with replace to avoid version skewYou require ginkgo v2.23.4 but replace it with the openshift fork at ~v2.6.1. This mismatch can confuse readers and tooling. Align the require to a version satisfied by the replace (e.g., 2.6.x) or add a comment explaining the intentional fork pin.
-require ( - github.com/onsi/ginkgo/v2 v2.23.4 +require ( + github.com/onsi/ginkgo/v2 v2.6.1 github.com/onsi/gomega v1.38.0 ... )Alternatively, add a note above the replace block that the fork intentionally downgrades API surface and is compatible with current usage.
Also applies to: 85-115
Makefile (3)
64-66: Reproducible builds: add -trimpathFor deterministic binaries, add -trimpath to strip local paths.
- go build -o $(TESTS_EXT_BINARY) -ldflags "$(TESTS_EXT_LDFLAGS)" $(TESTS_EXT_PACKAGE) + go build -trimpath -o $(TESTS_EXT_BINARY) -ldflags "$(TESTS_EXT_LDFLAGS)" $(TESTS_EXT_PACKAGE)
17-19: Unused variable: TESTS_EXT_OUTPUTEither use TESTS_EXT_OUTPUT in build/copy steps or drop it to avoid drift.
34-35: Typo in comment“builing the image an binding” → “building the image and binding”.
test/extended/tests-extension/README.md (1)
81-87: Fix unordered list indentation (markdownlint MD007)Indent list items by 2 spaces under the section.
- - Run `make tests-ext-update` to update test metadata + - Run `make tests-ext-update` to update test metadatatest/extended/tests-extension/apiserver/util.go (2)
28-31: Etcd pod selector may be too narrowSelector app=etcd may not match etcd pods on all OCP versions (often k8s-app=etcd). Consider broadening discovery or falling back when no pods are found.
- pods, err := kubeClient.CoreV1().Pods("openshift-etcd").List(ctx, metav1.ListOptions{ - LabelSelector: "app=etcd", - }) + pods, err := kubeClient.CoreV1().Pods("openshift-etcd").List(ctx, metav1.ListOptions{ + LabelSelector: "app=etcd,k8s-app=etcd", // best-effort; OR not supported, see fallback + }) + if err == nil && len(pods.Items) == 0 { + // Fallback without selector, then filter by container name + all, err2 := kubeClient.CoreV1().Pods("openshift-etcd").List(ctx, metav1.ListOptions{}) + if err2 == nil { + for _, p := range all.Items { + for _, c := range p.Spec.Containers { + if c.Name == "etcd" { + pods.Items = append(pods.Items, p) + break + } + } + } + } + }Also applies to: 40-42
186-205: Prefer typed client over string-searching JSONUse config client to List ClusterOperators and evaluate conditions, instead of raw GET + substring matches.
// Example sketch: cfgClient, _ := configclient.NewForConfig(kubeClient.RESTClient().(*rest.RESTClient).GetConfig()) cos, err := cfgClient.ConfigV1().ClusterOperators().List(cxt, metav1.ListOptions{}) if err != nil { return false, nil } for _, co := range cos.Items { if !isConditionTrue(co, "Available") || isConditionTrue(co, "Progressing") || isConditionTrue(co, "Degraded") { return false, nil } } return true, nilKeeps logic robust to JSON ordering/format changes.
Also applies to: 208-254
test/extended/tests-extension/cmd/openshift-apiserver-tests-ext/main.go (1)
97-100: Remove no-op BeforeAll or add intentThe empty “do stuff” hook adds overhead and confusion. Drop it or implement needed setup.
test/extended/tests-extension/apiserver/encryption.go (2)
136-145: Increase wait or detect “any newer” secret to reduce flakes120s may be tight on busy clusters. Either extend timeout or detect any secret with number > previous.
-errKey := wait.PollUntilContextTimeout(ctx, 5*time.Second, 120*time.Second, false, func(cxt context.Context) (bool, error) { +errKey := wait.PollUntilContextTimeout(ctx, 5*time.Second, 5*time.Minute, false, func(cxt context.Context) (bool, error) { ... })Or:
return hasSecretWithNumberGreaterThan(cxt, kubeClient, "openshift-apiserver", oasEncNumber) && hasSecretWithNumberGreaterThan(cxt, kubeClient, "openshift-kube-apiserver", kasEncNumber)
189-194: Pass expected resources to migration wait (after util change)Once waitEncryptionKeyMigration accepts expected resources, pass "secrets" explicitly (and consider also waiting for OAS resources like "routes.route.openshift.io" if needed).
-completed, err := waitEncryptionKeyMigration(ctx, kubeClient, newKASEncSecretName) +completed, err := waitEncryptionKeyMigration(ctx, kubeClient, newKASEncSecretName, "secrets")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
test/extended/tests-extension/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.gitignore(1 hunks)Makefile(3 hunks)go.mod(1 hunks)images/Dockerfile.rhel(1 hunks)test/extended/tests-extension/README.md(5 hunks)test/extended/tests-extension/apiserver/encryption.go(1 hunks)test/extended/tests-extension/apiserver/util.go(1 hunks)test/extended/tests-extension/cmd/openshift-apiserver-tests-ext/main.go(1 hunks)test/extended/tests-extension/go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
test/extended/tests-extension/README.md
62-62: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
86-86: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
87-87: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (4)
images/Dockerfile.rhel (1)
10-10: LGTM on new artifact pathCopy path aligns with TESTS_EXT_DIR relocation. No issues.
test/extended/tests-extension/README.md (1)
13-16: Paths updated: LGTMNew binary path and metadata location align with TESTS_EXT_DIR move.
Also applies to: 25-25, 51-51, 165-165
test/extended/tests-extension/cmd/openshift-apiserver-tests-ext/main.go (1)
23-26: LGTM: side-effect importsCorrectly registers extension packages after module relocation.
test/extended/tests-extension/apiserver/encryption.go (1)
26-234: Overall: solid flow and cleanupClient setup, patch/restore, prefix assertions, and logging look good. Assertions cover both format and change detection.
| if err != nil { | ||
| e2e.Logf(" ERROR: Cluster operators did not stabilize after %d polls: %v", pollCount, err) | ||
| // Log final status for debugging | ||
| e2e.Logf(" Attempting to get final cluster operator status for debugging...") | ||
| result, debugErr := kubeClient.CoreV1().RESTClient().Get(). | ||
| AbsPath("/apis/config.openshift.io/v1/clusteroperators"). | ||
| DoRaw(ctx) | ||
| if debugErr == nil { | ||
| e2e.Logf(" Final cluster operators status (truncated): %s", string(result)[:500]) | ||
| } | ||
| return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible panic when truncating debug output
string(result)[:500] panics if len(result) < 500. Guard the slice.
- if debugErr == nil {
- e2e.Logf(" Final cluster operators status (truncated): %s", string(result)[:500])
- }
+ if debugErr == nil {
+ out := string(result)
+ if len(out) > 500 {
+ out = out[:500]
+ }
+ e2e.Logf(" Final cluster operators status (truncated): %s", out)
+ }📝 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.
| if err != nil { | |
| e2e.Logf(" ERROR: Cluster operators did not stabilize after %d polls: %v", pollCount, err) | |
| // Log final status for debugging | |
| e2e.Logf(" Attempting to get final cluster operator status for debugging...") | |
| result, debugErr := kubeClient.CoreV1().RESTClient().Get(). | |
| AbsPath("/apis/config.openshift.io/v1/clusteroperators"). | |
| DoRaw(ctx) | |
| if debugErr == nil { | |
| e2e.Logf(" Final cluster operators status (truncated): %s", string(result)[:500]) | |
| } | |
| return err | |
| if err != nil { | |
| e2e.Logf(" ERROR: Cluster operators did not stabilize after %d polls: %v", pollCount, err) | |
| // Log final status for debugging | |
| e2e.Logf(" Attempting to get final cluster operator status for debugging...") | |
| result, debugErr := kubeClient.CoreV1().RESTClient().Get(). | |
| AbsPath("/apis/config.openshift.io/v1/clusteroperators"). | |
| DoRaw(ctx) | |
| if debugErr == nil { | |
| out := string(result) | |
| if len(out) > 500 { | |
| out = out[:500] | |
| } | |
| e2e.Logf(" Final cluster operators status (truncated): %s", out) | |
| } | |
| return err |
🤖 Prompt for AI Agents
In test/extended/tests-extension/apiserver/util.go around lines 256 to 266, the
call string(result)[:500] will panic when len(result) < 500; guard the slice by
computing a safe end index (e.g., end := 500; if len(result) < end { end =
len(result) }) and log string(result)[:end] instead, and ensure you still handle
debugErr != nil by logging the debugErr when the GET fails.
| ### Test Module Structure | ||
|
|
||
| This test extension uses a **separate Go module** to isolate test dependencies from the production operator code: | ||
|
|
||
| ``` | ||
| test/extended/tests-extension/ | ||
| ├── go.mod # Separate module with test dependencies | ||
| ├── go.sum | ||
| ├── cmd/openshift-apiserver-tests-ext/ | ||
| │ └── main.go # Test binary entry point | ||
| ├── apiserver/ # Test packages | ||
| │ ├── encryption.go | ||
| │ └── util.go | ||
| └── main.go # Additional test suites | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specify a language for fenced code block (markdownlint MD040)
Add a language hint; for directory trees, use "text".
-```
+```text
test/extended/tests-extension/
├── go.mod # Separate module with test dependencies
...
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
62-62: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In test/extended/tests-extension/README.md around lines 58 to 73, the fenced
code block showing the directory tree lacks a language hint which triggers
markdownlint MD040; update the opening fence to include a language (use "text"
for directory trees) so the block becomes ```text and leave the contents
unchanged to satisfy the lint rule.
</details>
<!-- fingerprinting:phantom:medusa:chinchilla -->
<!-- This is an auto-generated comment by CodeRabbit -->
15ec0e0 to
e0d38d6
Compare
d92ee9c to
0ef6082
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (6)
.gitignore(1 hunks)Makefile(2 hunks)images/Dockerfile.rhel(1 hunks)test/OWNERS(1 hunks)test/extended/tests-extension/.openshift-tests-extension/openshift_payload_openshift-apiserver.json(1 hunks)test/extended/tests-extension/Makefile(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
🪛 checkmake (0.2.2)
test/extended/tests-extension/Makefile
[warning] 70-70: Missing required phony target "all"
(minphony)
[warning] 70-70: Missing required phony target "test"
(minphony)
Makefile
[warning] 69-69: Missing required phony target "test"
(minphony)
🔇 Additional comments (6)
test/OWNERS (1)
1-15: LGTM! Verify identical reviewer/approver lists.The OWNERS file structure is correct. Note that the reviewers and approvers lists are identical—this may be intentional, but verify that this matches the team's governance model for the test directory.
Makefile (2)
17-18: LGTM! Clean delegation to subdirectory.The variables correctly point to the new test extension directory structure.
64-71: LGTM! Appropriate delegation pattern.Delegating the build and update targets to the subdirectory Makefile is the right approach for an isolated Go module, keeping the root Makefile clean and maintainable.
test/extended/tests-extension/.openshift-tests-extension/openshift_payload_openshift-apiserver.json (1)
1-22: Verify empty metadata objects are intentional.The test definitions include several empty objects (
labels,isolation,environmentSelector). Confirm whether these are intentional placeholders or need to be populated with actual configuration for the test execution environment.test/extended/tests-extension/Makefile (1)
40-43: Verify GO_COMPLIANCE_POLICY exemption is necessary.The build target sets
GO_COMPLIANCE_POLICY="exempt_all", which bypasses compliance checks. Confirm that this exemption is necessary for test binaries and aligns with project compliance requirements.images/Dockerfile.rhel (1)
6-10: LGTM! Artifact paths correctly updated.The gzip and COPY paths are correctly updated to reference the new test extension directory structure, maintaining consistency with the Makefile changes.
| .PHONY: clean-metadata | ||
| clean-metadata: #HELP Remove 'codeLocations' from metadata JSON | ||
| @echo "Cleaning metadata (removing codeLocations)..." | ||
| @for f in $(METADATA)/*.json; do \ | ||
| jq 'map(del(.codeLocations))' "$$f" > "$$f.tmp" && mv "$$f.tmp" "$$f"; \ | ||
| done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle empty metadata glob to avoid jq errors.
The loop over $(METADATA)/*.json will fail if no JSON files exist—the glob expands to a literal string when there are no matches, causing jq to fail. Add a guard to skip non-existent files.
Apply this diff to handle the empty glob case:
.PHONY: clean-metadata
clean-metadata: #HELP Remove 'codeLocations' from metadata JSON
@echo "Cleaning metadata (removing codeLocations)..."
- @for f in $(METADATA)/*.json; do \
+ @for f in $(METADATA)/*.json; do \
+ [ -e "$$f" ] || continue; \
jq 'map(del(.codeLocations))' "$$f" > "$$f.tmp" && mv "$$f.tmp" "$$f"; \
done📝 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.
| .PHONY: clean-metadata | |
| clean-metadata: #HELP Remove 'codeLocations' from metadata JSON | |
| @echo "Cleaning metadata (removing codeLocations)..." | |
| @for f in $(METADATA)/*.json; do \ | |
| jq 'map(del(.codeLocations))' "$$f" > "$$f.tmp" && mv "$$f.tmp" "$$f"; \ | |
| done | |
| .PHONY: clean-metadata | |
| clean-metadata: #HELP Remove 'codeLocations' from metadata JSON | |
| @echo "Cleaning metadata (removing codeLocations)..." | |
| @for f in $(METADATA)/*.json; do \ | |
| [ -e "$$f" ] || continue; \ | |
| jq 'map(del(.codeLocations))' "$$f" > "$$f.tmp" && mv "$$f.tmp" "$$f"; \ | |
| done |
🤖 Prompt for AI Agents
In test/extended/tests-extension/Makefile around lines 63 to 68, the for-loop
over $(METADATA)/*.json fails when the glob matches nothing because the literal
pattern is processed by jq; fix by adding a guard inside the loop to skip
non-existent files (e.g. test -f "$$f" || continue) before invoking jq so the
loop silently ignores the pattern when there are no JSON files.
This commit introduces dependency isolation by creating a separate Go module for the OpenShift API Server test extension at test/extended/tests-extension/. Key benefits: - Isolates test dependencies (ginkgo, gomega, openshift-tests-extension) from production operator code - Prevents dependency version conflicts between operator and test framework - Keeps production go.mod clean and focused on operator requirements - Follows the pattern used by other OpenShift operators The separate module includes: - go.mod with test-specific dependencies - go.sum with locked dependency versions - Same k8s.io version pinning as root module via replace directives Reference implementation: https://github.com/openshift/operator-framework-operator-controller/tree/main/openshift/tests-extension
Relocates all test-related code into the isolated test module: Test code moved: - test/extended/main.go → test/extended/tests-extension/main.go - test/extended/apiserver/* → test/extended/tests-extension/apiserver/* - cmd/openshift-apiserver-tests-ext → test/extended/tests-extension/cmd/ Metadata moved: - test/.openshift-tests-extension/ → test/extended/tests-extension/.openshift-tests-extension/ This consolidates all test extension components (code, binary entry point, and test metadata) into a single self-contained directory that uses its own go.mod for dependency management. The test binary and metadata now live alongside the test code in test/extended/tests-extension/, making the test module completely independent from the operator codebase.
Updates Makefile, Dockerfile, and .gitignore to work with the new test extension module location. Makefile changes: - Added TESTS_EXT_DIR and TESTS_EXT_OUTPUT variables - tests-ext-build now builds from test/extended/tests-extension/ - tests-ext-update runs from and updates metadata in tests-extension dir - Binary output: test/extended/tests-extension/openshift-apiserver-tests-ext Dockerfile changes: - Updated gzip path: test/extended/tests-extension/openshift-apiserver-tests-ext - Updated COPY path for .gz file to match new location .gitignore changes: - Updated to ignore binary at new path: /test/extended/tests-extension/openshift-apiserver-tests-ext The build process now: 1. cd into test/extended/tests-extension/ (where the separate go.mod is) 2. Build the binary using the isolated dependencies 3. Output binary stays within the test module directory 4. Docker build references the new paths for gzip and copy operations
Removes test-only dependencies from the production operator module now that they are isolated in test/extended/tests-extension/go.mod. Removed dependencies: - github.com/onsi/ginkgo/v2 v2.23.4 - github.com/onsi/gomega v1.38.0 - github.com/openshift-eng/openshift-tests-extension v0.0.0-20250804142706-7b3ab438a292 Removed replace directive: - github.com/onsi/ginkgo/v2 => github.com/openshift/onsi-ginkgo/v2 This achieves complete dependency isolation: - Production go.mod is now free of test framework dependencies - Test dependencies are managed separately in test/extended/tests-extension/go.mod - No risk of version conflicts between operator and test framework - Cleaner dependency tree for production builds The operator binary no longer carries any testing framework overhead.
Updates README.md to reflect the new isolated test module structure and document the dependency isolation benefits. Key documentation updates: - All commands now reference test/extended/tests-extension/ paths - Added section explaining the separate Go module structure - Documents dependency isolation benefits - Updated metadata file path references - Updated all test binary invocation examples New documentation includes: - Test Module Structure section explaining the directory layout - Benefits of dependency isolation (clean go.mod, no version conflicts) - Clear instructions for managing test dependencies separately - Updated paths for cmd/main.go references This ensures developers understand: - Why we have a separate module (dependency isolation) - Where to add new tests (test/extended/tests-extension/) - How to manage test dependencies (go mod tidy in tests-extension/) - Where the test binary and metadata live
Removes leftover indirect test dependencies after test module isolation. Run go mod tidy and go mod vendor to clean up: - github.com/go-task/slim-sprig/v3 (indirect dependency of ginkgo) - k8s.io/pod-security-admission indirect reference This ensures the root go.mod contains only production dependencies.
Remove k8s.io replace directives from test module to resolve version conflicts. The test module should not override k8s.io versions with replace directives as this creates inconsistent vendoring errors in CI builds. Kept only essential replaces: - github.com/onsi/ginkgo/v2 → openshift fork - k8s.io/pod-security-admission → v0.33.3 (required as it has v0.0.0 default) This allows the test module to naturally resolve dependencies without vendor directory conflicts.
- Updated all binary paths to use test/extended/tests-extension/bin/ - Added Makefile and bin/ directory to test module structure diagram - Clarified build output location in Makefile commands table - Updated all command examples to reference bin/ subdirectory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (6)
test/extended/tests-extension/README.md (1)
62-76: Add language to fenced code block (markdownlint MD040)Use “text” for the directory tree. This mirrors a prior comment.
-``` +```text test/extended/tests-extension/ ├── go.mod # Separate module with test dependencies ...</blockquote></details> <details> <summary>test/extended/tests-extension/apiserver/util.go (4)</summary><blockquote> `24-82`: **Past security concern still applies.** The command injection vulnerability flagged in the previous review (line 44) remains unaddressed. Ensure `keyPath` is validated or escaped before interpolation into the shell command to prevent potential arbitrary command execution. --- `85-116`: **Past panic risk still applies.** The use of `regexp.MustCompile(pattern)` at line 97 can panic if an invalid pattern is passed. The previous review correctly recommended switching to `regexp.Compile` with proper error handling. --- `119-172`: **Past generalization issue still applies.** The hard-coded check for `"secrets"` at line 152 won't work for openshift-apiserver resources. The previous review correctly noted this needs to be parameterized to support different resource types for OAS. --- `176-270`: **Past panic risk still applies.** The string truncation at line 264 (`string(result)[:500]`) will panic if `len(result) < 500`. The previous review correctly recommended guarding the slice operation. </blockquote></details> <details> <summary>test/extended/tests-extension/Makefile (1)</summary><blockquote> `63-68`: **Past glob handling issue still applies.** The previous review correctly identified that the for-loop will fail when no JSON files match the glob pattern. Adding a guard like `[ -e "$$f" ] || continue` is still needed. </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (12)</summary><blockquote> <details> <summary>test/extended/tests-extension/cmd/openshift-apiserver-tests-ext/main.go (3)</summary><blockquote> `24-25`: **Clarify side-effect import comment** Comment mentions “OAS operator tests”; these packages register the OpenShift API Server test extension. Update wording to avoid confusion. ```diff - // The import below is necessary to ensure that the OAS operator tests are registered with the extension. + // Side-effect imports register the OpenShift API Server test extension suites.
64-67: Prefer clean exit over panic for CLI init errorsUse stderr + non‑zero exit instead of panic to avoid stack dumps in user-facing tooling.
- if err != nil { - panic(fmt.Sprintf("couldn't build extension test specs from ginkgo: %+v", err.Error())) - } + if err != nil { + fmt.Fprintln(os.Stderr, "error: couldn't build extension test specs from ginkgo:", err) + os.Exit(2) + }
105-111: Polish Cobra root commandSet Use/Short and silence usage/errors for cleaner UX.
- root := &cobra.Command{ - Long: "OpenShift API Server Tests Extension", - } + root := &cobra.Command{ + Use: "openshift-apiserver-tests-ext", + Short: "Run OpenShift API Server tests via OTE", + Long: "OpenShift API Server Tests Extension", + SilenceUsage: true, + SilenceErrors: true, + }Makefile (2)
17-20: Remove unused root-level vars to reduce driftTESTS_EXT_OUTPUT and TESTS_EXT_PACKAGE aren’t referenced after delegating to the submodule. Drop them to avoid confusion.
TESTS_EXT_BINARY := openshift-apiserver-tests-ext -TESTS_EXT_DIR := ./test/extended/tests-extension -TESTS_EXT_OUTPUT := $(TESTS_EXT_DIR)/$(TESTS_EXT_BINARY) +TESTS_EXT_DIR := ./test/extended/tests-extension -TESTS_EXT_PACKAGE := ./cmd/openshift-apiserver-tests-ext
69-71: Add a minimal “test” target to appease checkmakeThe repo doesn’t need a test target here, but adding a no-op keeps linters quiet.
.PHONY: tests-ext-update tests-ext-update: $(MAKE) -C $(TESTS_EXT_DIR) build-update + +# Optional: satisfy checkmake's minphony rule +.PHONY: test +test: + @echo "No tests at root; see $(TESTS_EXT_DIR)"test/extended/tests-extension/go.mod (2)
5-16: Align ginkgo require with the replaced fork to avoid confusionYou require ginkgo v2.23.4 but replace it with an openshift fork at ~v2.6.1. Consider aligning the require to the replaced pseudo‑version (or drop the explicit version) so “go mod graph” and readers aren’t misled.
require ( - github.com/onsi/ginkgo/v2 v2.23.4 + github.com/onsi/ginkgo/v2 v2.6.1-0.20241205171354-8006f302fd12 github.com/onsi/gomega v1.38.0 ... )If other deps truly need ≥ v2.23 APIs, the replace target must be updated instead. Please confirm with a local
go build ./...inside test/extended/tests-extension.Also applies to: 85-88
3-4: Pin toolchain for reproducible buildsAdd a toolchain directive so CI/dev use a consistent Go toolchain.
go 1.24.0 +toolchain go1.24.0test/extended/tests-extension/README.md (2)
18-21: Remove duplicate headingThere are two “How to Run the Tests Locally” sections. Keep one.
-## How to Run the Tests Locally - -The tests can be run locally using the `openshift-apiserver-tests-ext` binary against an OpenShift cluster. +## Running locally + +The tests can be run using the `openshift-apiserver-tests-ext` binary against an OpenShift cluster.
88-91: Fix list indentation (markdownlint MD007)Adjust sub-list indent under “Before committing your changes:”.
- - Run `make tests-ext-update` to update test metadata - - Run `make verify` to check formatting, linting, and validation + - Run `make tests-ext-update` to update test metadata + - Run `make verify` to check formatting, linting, and validationtest/extended/tests-extension/apiserver/encryption.go (2)
127-145: Don’t assume secret names are exactly +1If rotation skips numbers or concurrent rotation occurs, hardcoded next names can flake. Poll on observed key numbers instead.
- newOASEncSecretName := "encryption-key-openshift-apiserver-" + strconv.Itoa(oasEncNumber+1) - newKASEncSecretName := "encryption-key-openshift-kube-apiserver-" + strconv.Itoa(kasEncNumber+1) + targetOAS := oasEncNumber + 1 + targetKAS := kasEncNumber + 1 ... - errKey := wait.PollUntilContextTimeout(ctx, 5*time.Second, 120*time.Second, false, func(cxt context.Context) (bool, error) { - _, err1 := kubeClient.CoreV1().Secrets("openshift-config-managed").Get(cxt, newOASEncSecretName, metav1.GetOptions{}) - _, err2 := kubeClient.CoreV1().Secrets("openshift-config-managed").Get(cxt, newKASEncSecretName, metav1.GetOptions{}) - if err1 != nil || err2 != nil { - e2e.Logf(" Still waiting for secrets... (OAS: %v, KAS: %v)", err1 != nil, err2 != nil) - return false, nil - } - e2e.Logf("Found both new encryption key secrets!") - return true, nil - }) + errKey := wait.PollUntilContextTimeout(ctx, 5*time.Second, 120*time.Second, false, func(cxt context.Context) (bool, error) { + currOAS, err1 := getEncryptionKeyNumber(cxt, kubeClient, `encryption-key-openshift-apiserver-\d+`) + currKAS, err2 := getEncryptionKeyNumber(cxt, kubeClient, `encryption-key-openshift-kube-apiserver-\d+`) + if err1 != nil || err2 != nil { + e2e.Logf(" Still waiting for key numbers... (errs: %v, %v)", err1, err2) + return false, nil + } + ready := currOAS >= targetOAS && currKAS >= targetKAS + if !ready { + e2e.Logf(" Observed OAS=%d KAS=%d; waiting for ≥ OAS=%d KAS=%d", currOAS, currKAS, targetOAS, targetKAS) + } + return ready, nil + })Also update the final expectation message accordingly.
24-25: Unnecessary GinkgoRecover in v2
defer g.GinkgoRecover()is not needed in a top-level Describe in Ginkgo v2.- defer g.GinkgoRecover() + // no-optest/extended/tests-extension/Makefile (1)
43-43: Clarify GO_COMPLIANCE_POLICY exemption.Setting
GO_COMPLIANCE_POLICY="exempt_all"bypasses compliance checks. While this may be intentional for test binaries, consider documenting why this exemption is necessary to help future maintainers understand the decision.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (271)
go.sumis excluded by!**/*.sumtest/extended/tests-extension/go.sumis excluded by!**/*.sumvendor/github.com/go-task/slim-sprig/v3/.editorconfigis excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/.gitattributesis excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/.gitignoreis excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/LICENSE.txtis excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/Taskfile.ymlis excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/crypto.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/date.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/defaults.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/dict.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/functions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/list.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/network.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/numeric.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/reflect.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/regex.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/strings.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/url.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/.gitignoreis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/CONTRIBUTING.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/RELEASING.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/config/deprecated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/core_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/core_dsl_patch.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/decorator_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/deprecated_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/formatter/colorable_others.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/formatter/colorable_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/formatter/formatter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/build/build_command.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/command/abort.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/command/command.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/command/program.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/generators/boostrap_templates.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/generators/bootstrap_command.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/generators/generate_command.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/generators/generate_templates.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/generators/generators_common.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/internal/compile.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/internal/gocovmerge.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/internal/profiles_and_reports.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/internal/run.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/internal/test_suite.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/internal/utils.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/internal/verify_version.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/labels/labels_command.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/main.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/outline/ginkgo.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/outline/import.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/outline/outline.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/outline/outline_command.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/run/run_command.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/unfocus/unfocus_command.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/delta.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/delta_tracker.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/dependencies.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/package_hash.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/package_hashes.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/suite.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/watch_command.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo_cli_dependencies.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo_t_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/counter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/failer.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/focus.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/global/init.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/group.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/interrupt_handler/interrupt_handler.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/interrupt_handler/sigquit_swallower_unix.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/interrupt_handler/sigquit_swallower_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/node.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/ordering.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/output_interceptor.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/output_interceptor_unix.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/output_interceptor_wasm.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/output_interceptor_win.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/parallel_support/client_server.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/parallel_support/http_client.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/parallel_support/http_server.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/parallel_support/rpc_client.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/parallel_support/rpc_server.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/parallel_support/server_handler.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/progress_report.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/progress_report_bsd.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/progress_report_unix.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/progress_report_wasm.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/progress_report_win.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/progress_reporter_manager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/report_entry.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/spec.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/spec_context.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/spec_patch.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/suite.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/suite_patch.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/testingtproxy/testing_t_proxy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/tree.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/writer.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/default_reporter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/deprecated_reporter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/json_report.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/junit_report.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/reporter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/teamcity_report.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/reporting_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/table_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/code_location.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/config.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/deprecated_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/deprecation_support.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/enum_support.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/errors.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/file_filter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/flags.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/label_filter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/report_entry.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/types_patch.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/version.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/.gitignoreis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/CONTRIBUTING.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/RELEASING.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/format/format.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/gomega_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/async_assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/duration_bundle.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/gomega.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/gutil/post_ioutil.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/gutil/using_ioutil.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/polling_signal_error.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/vetoptdesc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/and.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/assignable_to_type_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/attributes_slice.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_a_directory.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_a_regular_file.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_an_existing_file.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_closed_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_comparable_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_element_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_empty_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_equivalent_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_false_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_identical_to.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_key_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_nil_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_numerically_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_sent_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_temporally_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_true_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_zero_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/consist_of.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_element_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_elements_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_substring_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/equal_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_cap_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_each_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_exact_elements.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_existing_field_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_field.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_body_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_header_with_value_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_status_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_key_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_key_with_value_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_len_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_occurred_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_prefix_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_suffix_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_value.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/internal/miter/type_support_iter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/internal/miter/type_support_noiter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_error_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_json_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_regexp_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_xml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_yaml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/not.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/or.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/panic_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/receive_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/satisfy_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/semi_structured_data_support.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/succeed_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/bipartitegraph/bipartitegraph.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/bipartitegraph/bipartitegraphmatching.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/edge/edge.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/node/node.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/util/util.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/type_support.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/with_transform.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/types/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmd.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdimages/cmdimages.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdinfo/info.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdlist/list.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdrun/runsuite.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdrun/runtest.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdupdate/update.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/dbtime/time.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extension.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/environment.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/result.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/result_writer.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/spec.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/task.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/registry.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/flags/component.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/flags/concurrency.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/flags/environment.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/flags/names.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/flags/output.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/flags/suite.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/ginkgo/logging.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/ginkgo/parallel.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/ginkgo/util.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/junit/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/byte.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/empty.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/int.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/int32.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/int64.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/set.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/string.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/version/version.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/html/charset/charset.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/charmap/charmap.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/charmap/tables.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/htmlindex/htmlindex.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/htmlindex/map.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/htmlindex/tables.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/japanese/all.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/japanese/eucjp.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/japanese/iso2022jp.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/japanese/shiftjis.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/japanese/tables.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/korean/euckr.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/korean/tables.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/simplifiedchinese/all.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/simplifiedchinese/gbk.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/simplifiedchinese/hzgb2312.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/simplifiedchinese/tables.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/traditionalchinese/big5.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/traditionalchinese/tables.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/cover/profile.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ast/inspector/inspector.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ast/inspector/iter.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ast/inspector/typeof.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ast/inspector/walk.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/astutil/edge/edge.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (12)
.gitignore(1 hunks)Makefile(2 hunks)go.mod(0 hunks)images/Dockerfile.rhel(1 hunks)test/.openshift-tests-extension/openshift_payload_openshift-apiserver.json(0 hunks)test/extended/tests-extension/.openshift-tests-extension/openshift_payload_openshift-apiserver.json(1 hunks)test/extended/tests-extension/Makefile(1 hunks)test/extended/tests-extension/README.md(5 hunks)test/extended/tests-extension/apiserver/encryption.go(1 hunks)test/extended/tests-extension/apiserver/util.go(1 hunks)test/extended/tests-extension/cmd/openshift-apiserver-tests-ext/main.go(1 hunks)test/extended/tests-extension/go.mod(1 hunks)
💤 Files with no reviewable changes (2)
- go.mod
- test/.openshift-tests-extension/openshift_payload_openshift-apiserver.json
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- images/Dockerfile.rhel
🧰 Additional context used
🪛 checkmake (0.2.2)
test/extended/tests-extension/Makefile
[warning] 70-70: Missing required phony target "all"
(minphony)
[warning] 70-70: Missing required phony target "test"
(minphony)
Makefile
[warning] 69-69: Missing required phony target "test"
(minphony)
🪛 markdownlint-cli2 (0.18.1)
test/extended/tests-extension/README.md
62-62: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
89-89: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
90-90: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (1)
test/extended/tests-extension/.openshift-tests-extension/openshift_payload_openshift-apiserver.json (1)
12-21: Confirm lifecycle=blocking for a [Slow][Serial][Disruptive] testMarking disruptive serial tests as blocking can impact merge gating. Verify with TRT that this should be blocking versus informing.
| restorePatch := []byte(`[{"op":"replace","path":"/spec/unsupportedConfigOverrides","value":null}]`) | ||
| mergePatch := []byte(fmt.Sprintf(`{"spec":{"unsupportedConfigOverrides":{"encryption":{"reason":"force OAS rotation %s"}}}}`, t)) | ||
|
|
||
| g.By("3. Force encryption key rotation for both openshiftapiserver and kubeapiserver") | ||
| e2e.Logf("Preparing to force encryption key rotation with timestamp: %s", t) | ||
|
|
||
| // Patch OpenShift API Server | ||
| defer func() { | ||
| e2e.Logf("CLEANUP: Restoring openshiftapiserver/cluster's spec") | ||
| _, patchErr := operatorClient.OperatorV1().OpenShiftAPIServers().Patch(ctx, "cluster", types.JSONPatchType, restorePatch, metav1.PatchOptions{}) | ||
| if patchErr != nil { | ||
| e2e.Failf("Failed to restore openshiftapiserver: %v", patchErr) | ||
| } | ||
| e2e.Logf("Successfully restored openshiftapiserver/cluster") | ||
| }() | ||
| e2e.Logf("3.1) Patching openshiftapiserver to force encryption rotation...") | ||
| _, err = operatorClient.OperatorV1().OpenShiftAPIServers().Patch(ctx, "cluster", types.MergePatchType, mergePatch, metav1.PatchOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| e2e.Logf("Successfully patched openshiftapiserver/cluster") | ||
|
|
||
| // Patch Kube API Server | ||
| defer func() { | ||
| e2e.Logf("CLEANUP: Restoring kubeapiserver/cluster's spec") | ||
| _, patchErr := operatorClient.OperatorV1().KubeAPIServers().Patch(ctx, "cluster", types.JSONPatchType, restorePatch, metav1.PatchOptions{}) | ||
| if patchErr != nil { | ||
| e2e.Failf("Failed to restore kubeapiserver: %v", patchErr) | ||
| } | ||
| e2e.Logf("Successfully restored kubeapiserver/cluster") | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleanup is brittle: JSONPatch replace fails if path absent
Using JSONPatch op “replace” on /spec/unsupportedConfigOverrides errors when the field wasn’t present. Use a MergePatch setting the field to null and call Patch with MergePatchType for robustness.
- restorePatch := []byte(`[{"op":"replace","path":"/spec/unsupportedConfigOverrides","value":null}]`)
+ restorePatch := []byte(`{"spec":{"unsupportedConfigOverrides":null}}`)
...
- _, patchErr := operatorClient.OperatorV1().OpenShiftAPIServers().Patch(ctx, "cluster", types.JSONPatchType, restorePatch, metav1.PatchOptions{})
+ _, patchErr := operatorClient.OperatorV1().OpenShiftAPIServers().Patch(ctx, "cluster", types.MergePatchType, restorePatch, metav1.PatchOptions{})
...
- _, patchErr := operatorClient.OperatorV1().KubeAPIServers().Patch(ctx, "cluster", types.JSONPatchType, restorePatch, metav1.PatchOptions{})
+ _, patchErr := operatorClient.OperatorV1().KubeAPIServers().Patch(ctx, "cluster", types.MergePatchType, restorePatch, metav1.PatchOptions{})This reduces the chance of leaving the cluster mutated after test failures.
🤖 Prompt for AI Agents
In test/extended/tests-extension/apiserver/encryption.go around lines 85 to 113,
the deferred cleanup uses a JSONPatch "replace" which fails if
/spec/unsupportedConfigOverrides is absent; change the restorePatch to a
MergePatch payload like `{"spec":{"unsupportedConfigOverrides":null}}` (as
[]byte) and call Patch with types.MergePatchType (not types.JSONPatchType) for
both the OpenShiftAPIServers and KubeAPIServers restore calls so the field is
safely nulled whether or not it exists.
| .PHONY: verify-metadata #HELP To verify that the metadata was properly updated | ||
| verify-metadata: update-metadata | ||
| @if ! git diff --exit-code $(METADATA); then \ | ||
| echo "ERROR: Metadata is out of date. Please run 'make build-update' and commit the result."; \ | ||
| exit 1; \ | ||
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix absolute path in git diff check.
The verify-metadata target uses $(METADATA) in the git diff command, but METADATA is defined as $(shell pwd)/.openshift-tests-extension (line 15), which produces an absolute path. Git diff expects a relative path from the repository root, so this check will likely fail.
Apply this diff to use a relative path:
.PHONY: verify-metadata #HELP To verify that the metadata was properly updated
verify-metadata: update-metadata
- @if ! git diff --exit-code $(METADATA); then \
+ @if ! git diff --exit-code .openshift-tests-extension; then \
echo "ERROR: Metadata is out of date. Please run 'make build-update' and commit the result."; \
exit 1; \
fiCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 checkmake (0.2.2)
[warning] 70-70: Missing required phony target "all"
(minphony)
[warning] 70-70: Missing required phony target "test"
(minphony)
🤖 Prompt for AI Agents
In test/extended/tests-extension/Makefile around lines 70 to 75, the git diff in
the verify-metadata target uses the absolute $(METADATA) (defined as $(shell
pwd)/.openshift-tests-extension), which makes git diff fail; change the check to
use a repository-relative path by replacing $(METADATA) with the relative
filename (.openshift-tests-extension) or use a Make variable that strips the
absolute prefix such as REL_METADATA := $(notdir $(METADATA)) and then call git
diff --exit-code $(REL_METADATA) so the diff is run against a path relative to
the repo root.
|
@gangwgr: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| @@ -0,0 +1,235 @@ | |||
| package apiserver | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are already in the path of opneshift-apiserver component and no longer need this name apiserver alone
Create separate Go module for OTE test extension. Also migrated OCP-25806 to component repo
Commit 1: 43747ca - Create separate Go module for test extension
Commit 2: d33f57d - Move test code and metadata to tests-extension directory
Commit 3: 7732903 - Update build infrastructure for isolated test module
Commit 4: 7c0632e - Remove test dependencies from root go.mod
Commit 5: 3c67013 - Update documentation for test module structure
assisted by - Claude