ESO-323: Adds Bitwarden and cross-platform e2e suites with failure artifact collection#114
Open
bharath-b-rh wants to merge 2 commits intoopenshift:mainfrom
Open
ESO-323: Adds Bitwarden and cross-platform e2e suites with failure artifact collection#114bharath-b-rh wants to merge 2 commits intoopenshift:mainfrom
bharath-b-rh wants to merge 2 commits intoopenshift:mainfrom
Conversation
…tifact collection Signed-off-by: Bharath B <bhb@redhat.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@bharath-b-rh: This pull request references ESO-323 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target either version "4.22." or "openshift-4.22.", but it targets "eso-1.1" instead. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds comprehensive end-to-end testing: new e2e test suites for Bitwarden and cross-platform AWS flows, supporting test utilities (Bitwarden helpers, API job runner, AWS/Bitwarden resource factories), artifact dumping and cleanup tools, dynamic unstructured resource helpers, README docs, and minor test tooling tweaks. Changes
Sequence Diagram(s)sequenceDiagram
actor Test as E2E Test Suite
participant K8s as Kubernetes Cluster
participant BitSDK as Bitwarden SDK Server
participant ExtSecrets as ExternalSecrets Operator
Test->>K8s: Init clients & ensure ExternalSecretsConfig
Test->>K8s: Create operand namespace & TLS Secret
Test->>BitSDK: Probe /ready endpoint
Test->>K8s: Create Bitwarden ClusterSecretStore
Test->>K8s: Create PushSecret / ExternalSecret
ExtSecrets->>BitSDK: Request secret (API)
BitSDK-->>ExtSecrets: Respond with secret data
ExtSecrets->>K8s: Sync target Secret
Test->>K8s: Verify target Secret contents
Test->>K8s: Cleanup resources
Note over Test,K8s: On failures DumpE2EArtifacts collects logs, events, and resource YAMLs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": bin/kube-api-linter.so, plugin: not implemented Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bharath-b-rh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@bharath-b-rh: This pull request references ESO-323 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.22." or "openshift-4.22.", but it targets "eso-1.1" instead. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
test/utils/dynamic_resources.go (2)
143-147: Usescontext.Background()instead of stored context.
CreateFromUnstructuredReturnErrusescontext.Background()while the struct has acontextfield. For consistency and to support cancellation, consider usingd.context.♻️ Proposed fix
func (d DynamicResourceLoader) CreateFromUnstructuredReturnErr(unstructuredObj *unstructured.Unstructured, overrideNamespace string) error { dri := d.getResourceInterface(unstructuredObj, overrideNamespace) - _, err := dri.Create(context.Background(), unstructuredObj, metav1.CreateOptions{}) + _, err := dri.Create(d.context, unstructuredObj, metav1.CreateOptions{}) return err }Also apply to
DeleteFromUnstructuredat line 152.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/dynamic_resources.go` around lines 143 - 147, The CreateFromUnstructuredReturnErr function is calling context.Background() instead of using the loader's stored context; update CreateFromUnstructuredReturnErr to use d.context when calling dri.Create, and make the same change in DeleteFromUnstructured so both methods use the DynamicResourceLoader's context field (d.context) to allow cancellation and consistency with the struct's context.
157-172: Side effect:SetNamespacemutates the input object.
getResourceInterfacemodifiesunstructuredObj.SetNamespace(overrideNamespace)when override is provided. This could surprise callers if they reuse the object. Consider documenting this behavior or working on a copy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/dynamic_resources.go` around lines 157 - 172, The getResourceInterface function mutates the passed unstructuredObj via unstructuredObj.SetNamespace(overrideNamespace); to avoid surprising callers, make and use a deep copy of the object before any mutation: call unstructuredObj.DeepCopy() (or equivalent) into a local variable, use the original only to read GroupVersionKind (gvk) and discovery, then call SetNamespace and GetNamespace on the copy and pass the copy's namespace to DynamicClient.Resource(mapping.Resource).Namespace(...); ensure all mutations and namespace checks target the copy so the original unstructured.Unstructured remains unchanged.test/utils/bitwarden.go (2)
292-304: Using mutable:latesttag for curl image.
docker.io/curlimages/curl:latestis a mutable tag that could change between test runs, potentially causing inconsistent behavior or unexpected failures.♻️ Consider pinning to a specific version
- Image: "docker.io/curlimages/curl:latest", + Image: "docker.io/curlimages/curl:8.7.1",Or use a digest for full reproducibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/bitwarden.go` around lines 292 - 304, The container Image field for the curl container uses the mutable tag "docker.io/curlimages/curl:latest" which can change between runs; update the Image in the Container literal (the Container with Name "curl" in test/utils/bitwarden.go) to a specific, pinned version tag or preferably an immutable digest (e.g., curl:8.x or `@sha256`:...) to ensure reproducible tests.
185-198: Missing validation for empty token.
FetchBitwardenCredsFromSecretdoesn't verify that thetokenfield exists or is non-empty. An empty token would cause auth failures later with unclear errors.🛡️ Optional: add token validation
func FetchBitwardenCredsFromSecret(ctx context.Context, client kubernetes.Interface, secretName, secretNamespace string) (token, orgID, projectID string, err error) { secret, err := client.CoreV1().Secrets(secretNamespace).Get(ctx, secretName, metav1.GetOptions{}) if err != nil { return "", "", "", fmt.Errorf("get Bitwarden creds secret %s/%s: %w", secretNamespace, secretName, err) } token = string(secret.Data[TokenSecretKey]) + if token == "" { + return "", "", "", fmt.Errorf("Bitwarden creds secret %s/%s missing required key %q", secretNamespace, secretName, TokenSecretKey) + } if v, ok := secret.Data[BitwardenCredKeyOrgID]; ok { orgID = string(v) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/bitwarden.go` around lines 185 - 198, The function FetchBitwardenCredsFromSecret must validate that the token field exists and is non-empty: after reading token := string(secret.Data[TokenSecretKey]) check if token == "" and if so return an error (e.g., fmt.Errorf("missing Bitwarden token in secret %s/%s", secretNamespace, secretName)) instead of returning an empty token; keep the existing orgID/projectID extraction and return nil error only when token is present.test/utils/bitwarden_api_runner.go (2)
168-177: UUID value passed directly to shell command.The
uuidfromGetBitwardenSecretIDByKeyis interpolated into the shell script. While UUIDs should be safe (alphanumeric + hyphens), validating the format would add defense-in-depth.🛡️ Optional: validate UUID format
func DeleteBitwardenSecretByKey(ctx context.Context, client kubernetes.Interface, namespace, remoteKey string) { uuid, err := GetBitwardenSecretIDByKey(ctx, client, namespace, remoteKey) if err != nil || uuid == "" { return } + // Basic UUID format validation + if !regexp.MustCompile(`^[a-fA-F0-9-]+$`).MatchString(uuid) { + return + } baseURL := GetBitwardenSDKServerURL()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/bitwarden_api_runner.go` around lines 168 - 177, The DeleteBitwardenSecretByKey function currently interpolates the uuid returned from GetBitwardenSecretIDByKey directly into a shell command; add a validation step to ensure uuid matches a canonical UUID format (or parse using a UUID library) and return early (or log/error) if it is invalid before building the script, then use the validated value when calling RunBitwardenAPIJob; reference the DeleteBitwardenSecretByKey function, the uuid variable from GetBitwardenSecretIDByKey, and the RunBitwardenAPIJob invocation when making this change.
36-37: Using mutable:latesttag for test runner image.Same concern as noted in
bitwarden.go- consider pinning to a specific version for reproducibility.♻️ Pin to a specific version
- bitwardenAPITestRunnerImage = "docker.io/curlimages/curl:latest" + bitwardenAPITestRunnerImage = "docker.io/curlimages/curl:8.7.1"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/bitwarden_api_runner.go` around lines 36 - 37, The test runner is using a mutable tag; update the constant bitwardenAPITestRunnerImage in test/utils/bitwarden_api_runner.go to pin to a stable version or digest (e.g., a specific semver tag or `@sha256` digest) instead of "latest" so test runs are reproducible; ensure any related references (e.g., bitwardenCredMountPath usage) continue to work with the pinned image and update comments or docs if present to record the chosen version.test/utils/bitwarden_resources.go (1)
71-96: Consider handlingSetNestedFielderrors.The errors from
SetNestedFieldare ignored. While unlikely to fail with these literal map structures, capturing errors would aid debugging if the unstructured object shape changes.♻️ Optional improvement
func BitwardenExternalSecretByName(name, namespace, targetSecretName, storeName, remoteKey string) *unstructured.Unstructured { u := BitwardenExternalSecretBase(name, namespace, targetSecretName, storeName) - _ = unstructured.SetNestedField(u.Object, []interface{}{ + if err := unstructured.SetNestedField(u.Object, []interface{}{ map[string]interface{}{ "secretKey": "value", "remoteRef": map[string]interface{}{ "key": remoteKey, }, }, - }, "spec", "data") + }, "spec", "data"); err != nil { + // Log or handle error if needed in test context + } return u }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/bitwarden_resources.go` around lines 71 - 96, The calls to unstructured.SetNestedField in BitwardenExternalSecretByName and BitwardenExternalSecretByUUID ignore the returned error; capture the error value, and if non-nil handle it (for test helper utilities either panic or log.Fatalf) with a clear message that includes the function name and the error so issues building the unstructured object are visible; update both functions (BitwardenExternalSecretByName and BitwardenExternalSecretByUUID) to check the error returned from SetNestedField and fail loudly when it occurs.test/e2e/bitwarden_es_test.go (1)
149-165: Test ordering dependency via shared state.The
pushedRemoteKeyForUUIDTestvariable creates an implicit dependency between the twoItblocks. While theOrderedcontainer ensures execution order and line 391 documents this dependency, consider documenting this coupling more prominently in the variable declaration, or restructuring to make the UUID test fully self-contained.Optional: Add clarifying comment at declaration
- pushedRemoteKeyForUUIDTest string // set by first It, used by second It to look up secret UUID + // pushedRemoteKeyForUUIDTest is set by the first It block and consumed by the second. + // These tests must run in order (Ordered container) due to this shared state dependency. + pushedRemoteKeyForUUIDTest string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/bitwarden_es_test.go` around lines 149 - 165, The test relies on a shared variable pushedRemoteKeyForUUIDTest which creates an implicit coupling between the two It blocks inside the Ordered Describe; make this dependency explicit by either (A) adding a clear inline comment at the pushedRemoteKeyForUUIDTest declaration documenting that the first It sets it and the later It reads it (mentioning Ordered ensures execution order), or (B) refactor the UUID test to be self-contained (e.g., push and then lookup the remote key within the same It) so there is no cross-It state; update references to pushedRemoteKeyForUUIDTest and the two related It blocks accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/e2e_suite_test.go`:
- Around line 53-68: The getTestDir function incorrectly computes the local
_output path (e.g., filepath.Join(cwd, "..", "_output") yields test/_output when
cwd is test/e2e) and contains a redundant OPENSHIFT_CI block; change it to check
ARTIFACT_DIR only once and, if unset, compute the repo root by walking up from
os.Getwd() until you find a project marker (e.g., .git or go.mod) or reach
filesystem root, then return filepath.Join(repoRoot, "_output"); update
getTestDir to use os.Getwd(), iterate upward checking for .git or go.mod to
identify repoRoot, and remove the duplicate OPENSHIFT_CI branch so ARTIFACT_DIR
is respected in a single conditional.
In `@test/utils/bitwarden_api_runner.go`:
- Around line 112-116: The error message currently wraps err even when err is
nil, causing a confusing "%!w(<nil>)" output; update the
client.CoreV1().Pods(namespace).List call handling so you first check if err !=
nil and return fmt.Errorf("list pods for job %s: %w", jobName, err) only when
err is non-nil, and then separately handle the empty result (len(pods.Items) ==
0) with a clear error like fmt.Errorf("no pods found for job %s in namespace
%s", jobName, namespace) or similar; reference the pods and err variables and
the List call when making this change.
In `@test/utils/cleanup.go`:
- Around line 62-65: The code ignores the error from
extClient.ApiextensionsV1().CustomResourceDefinitions().List and directly ranges
over crdList.Items which can be nil; change the call to capture the error
(crdList, err :=
extClient.ApiextensionsV1().CustomResourceDefinitions().List(ctx,
metav1.ListOptions{LabelSelector: operandLabelSelector})), check err and handle
it (return or log and continue) and also defensively verify crdList != nil
before iterating over crdList.Items (in the cleanup.go function where crdList is
used).
- Around line 102-115: The delete loops for ValidatingWebhookConfigurations,
ClusterRoleBindings, and ClusterRoles risk nil-pointer panics because their List
results are unchecked; update the code around
clientset.AdmissionregistrationV1().ValidatingWebhookConfigurations().List
(webhooks), clientset.RbacV1().ClusterRoleBindings().List (crbs) and
clientset.RbacV1().ClusterRoles().List (crList) to check the returned error and
ensure the returned list is non-nil before iterating (e.g., if err != nil {
log/handle; } and if list != nil { for ... }). Also add the suggested imports
admissionregistrationv1 "k8s.io/api/admissionregistration/v1" and rbacv1
"k8s.io/api/rbac/v1" if needed for types or nil checks.
In `@test/utils/dynamic_resources.go`:
- Around line 149-155: The delete routine logs success unconditionally; move
error handling before the success log so we only print when the delete actually
succeeded: in DeleteFromUnstructured capture the error from dri.Delete, and if
err != nil call d.noErrorSkipNotExisting(err) and return (so any non-NotFound
error will be handled there) otherwise call d.t.Logf("Resource %s deleted\n",
unstructuredObj.GetName()); keep the same use of getResourceInterface, Delete,
noErrorSkipNotExisting and d.t.Logf names to locate the change.
---
Nitpick comments:
In `@test/e2e/bitwarden_es_test.go`:
- Around line 149-165: The test relies on a shared variable
pushedRemoteKeyForUUIDTest which creates an implicit coupling between the two It
blocks inside the Ordered Describe; make this dependency explicit by either (A)
adding a clear inline comment at the pushedRemoteKeyForUUIDTest declaration
documenting that the first It sets it and the later It reads it (mentioning
Ordered ensures execution order), or (B) refactor the UUID test to be
self-contained (e.g., push and then lookup the remote key within the same It) so
there is no cross-It state; update references to pushedRemoteKeyForUUIDTest and
the two related It blocks accordingly.
In `@test/utils/bitwarden_api_runner.go`:
- Around line 168-177: The DeleteBitwardenSecretByKey function currently
interpolates the uuid returned from GetBitwardenSecretIDByKey directly into a
shell command; add a validation step to ensure uuid matches a canonical UUID
format (or parse using a UUID library) and return early (or log/error) if it is
invalid before building the script, then use the validated value when calling
RunBitwardenAPIJob; reference the DeleteBitwardenSecretByKey function, the uuid
variable from GetBitwardenSecretIDByKey, and the RunBitwardenAPIJob invocation
when making this change.
- Around line 36-37: The test runner is using a mutable tag; update the constant
bitwardenAPITestRunnerImage in test/utils/bitwarden_api_runner.go to pin to a
stable version or digest (e.g., a specific semver tag or `@sha256` digest) instead
of "latest" so test runs are reproducible; ensure any related references (e.g.,
bitwardenCredMountPath usage) continue to work with the pinned image and update
comments or docs if present to record the chosen version.
In `@test/utils/bitwarden_resources.go`:
- Around line 71-96: The calls to unstructured.SetNestedField in
BitwardenExternalSecretByName and BitwardenExternalSecretByUUID ignore the
returned error; capture the error value, and if non-nil handle it (for test
helper utilities either panic or log.Fatalf) with a clear message that includes
the function name and the error so issues building the unstructured object are
visible; update both functions (BitwardenExternalSecretByName and
BitwardenExternalSecretByUUID) to check the error returned from SetNestedField
and fail loudly when it occurs.
In `@test/utils/bitwarden.go`:
- Around line 292-304: The container Image field for the curl container uses the
mutable tag "docker.io/curlimages/curl:latest" which can change between runs;
update the Image in the Container literal (the Container with Name "curl" in
test/utils/bitwarden.go) to a specific, pinned version tag or preferably an
immutable digest (e.g., curl:8.x or `@sha256`:...) to ensure reproducible tests.
- Around line 185-198: The function FetchBitwardenCredsFromSecret must validate
that the token field exists and is non-empty: after reading token :=
string(secret.Data[TokenSecretKey]) check if token == "" and if so return an
error (e.g., fmt.Errorf("missing Bitwarden token in secret %s/%s",
secretNamespace, secretName)) instead of returning an empty token; keep the
existing orgID/projectID extraction and return nil error only when token is
present.
In `@test/utils/dynamic_resources.go`:
- Around line 143-147: The CreateFromUnstructuredReturnErr function is calling
context.Background() instead of using the loader's stored context; update
CreateFromUnstructuredReturnErr to use d.context when calling dri.Create, and
make the same change in DeleteFromUnstructured so both methods use the
DynamicResourceLoader's context field (d.context) to allow cancellation and
consistency with the struct's context.
- Around line 157-172: The getResourceInterface function mutates the passed
unstructuredObj via unstructuredObj.SetNamespace(overrideNamespace); to avoid
surprising callers, make and use a deep copy of the object before any mutation:
call unstructuredObj.DeepCopy() (or equivalent) into a local variable, use the
original only to read GroupVersionKind (gvk) and discovery, then call
SetNamespace and GetNamespace on the copy and pass the copy's namespace to
DynamicClient.Resource(mapping.Resource).Namespace(...); ensure all mutations
and namespace checks target the copy so the original unstructured.Unstructured
remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9cb37430-7c2c-4c22-931e-496d3850620e
📒 Files selected for processing (17)
README.mdtest/README.mdtest/apis/README.mdtest/e2e/README.mdtest/e2e/bitwarden_api_test.gotest/e2e/bitwarden_es_test.gotest/e2e/e2e_suite_test.gotest/e2e/e2e_test.gotest/go.modtest/utils/artifact_dump.gotest/utils/aws_resources.gotest/utils/bitwarden.gotest/utils/bitwarden_api_runner.gotest/utils/bitwarden_resources.gotest/utils/cleanup.gotest/utils/conditions.gotest/utils/dynamic_resources.go
|
@bharath-b-rh: This pull request references ESO-323 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.22." or "openshift-4.22.", but it targets "eso-1.1" instead. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/utils/conditions.go (1)
250-271:⚠️ Potential issue | 🟠 MajorUse the context-aware Secrets Manager API method.
The function accepts
ctxbut passes it toDeleteSecret, which ignores it. Network calls can outlive the spec timeout and stall cleanup. UseDeleteSecretWithContextinstead:Suggested fix
- _, err = svc.DeleteSecret(&secretsmanager.DeleteSecretInput{ + _, err = svc.DeleteSecretWithContext(ctx, &secretsmanager.DeleteSecretInput{ SecretId: aws.String(awsSecretName), ForceDeleteWithoutRecovery: aws.Bool(true), // permanently delete without 7-day wait })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/conditions.go` around lines 250 - 271, The DeleteAWSSecretFromCredsSecret function is using svc.DeleteSecret which ignores the provided ctx; change the call to the context-aware method svc.DeleteSecretWithContext(ctx, ...) so the Secrets Manager request respects cancellation and timeouts, passing the same DeleteSecretInput you currently build and propagating the returned error (update the error message still referencing failed to delete AWS secret). Locate DeleteAWSSecretFromCredsSecret and replace the svc.DeleteSecret invocation with svc.DeleteSecretWithContext using the existing ctx variable.
🧹 Nitpick comments (2)
hack/govulncheck.sh (1)
23-27: All vulnerability IDs are valid and accessible.The additions of GO-2026-4601 and GO-2026-4602 to the KNOWN_VULNS_PATTERN follow the established documentation pattern and are properly justified. Both vulnerabilities exist in the Go vulnerability database and affect the operator code as documented.
Consider adding a tracking link (GitHub issue or Jira ticket) alongside these suppressions to ensure they are revisited once the downstream Go runtime fix is released and available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/govulncheck.sh` around lines 23 - 27, Add a short tracking reference next to the KNOWN_VULNS_PATTERN declaration so the suppressions GO-2026-4601 and GO-2026-4602 are revisited when the downstream Go runtime fix is available; update the comment above or on the same line as the KNOWN_VULNS_PATTERN variable to include a GitHub issue or Jira ticket ID (e.g., "tracking: github.com/org/repo/issues/1234") and a brief note about re-evaluating/removing these IDs once the Go fixes are released; ensure you reference the KNOWN_VULNS_PATTERN symbol so reviewers can easily find and confirm the tracking link.test/utils/dynamic_resources.go (1)
112-128: Consider refactoring to reduce duplication withdo()method.The REST mapping and namespace handling logic (lines 115-127) is nearly identical to lines 159-175 in the
do()method. Consider extracting a shared helper to reduce duplication and ensure consistent behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/dynamic_resources.go` around lines 112 - 128, The REST mapping and namespace handling duplicated between getResourceInterface and do() should be extracted into a shared helper (e.g., resolveResourceInterface or buildResourceInterface) on DynamicResourceLoader; move the logic that does restmapper.GetAPIGroupResources(d.KubeClient.Discovery()), restmapper.NewDiscoveryRESTMapper(gr), mapper.RESTMapping(gvk.GroupKind(), gvk.Version), scope check, overrideNamespace application to objCopy.SetNamespace and require.NotEmpty into that helper, and have both getResourceInterface and do() call it to obtain the dynamic.ResourceInterface (or the mapping + namespace) so behavior is centralized and duplication removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/bitwarden_es_test.go`:
- Around line 79-99: The existing-object branch for ExternalSecretsConfig only
waits for readiness and skips injecting the Bitwarden egress policy; update the
branch that handles the found esc (the runtimeClient.Get success path) to either
merge/apply the Bitwarden egress rule into the existing ExternalSecretsConfig
(e.g., read the file via
loadExternalSecretsConfigFromFileWithBitwardenNetworkPolicy and merge its
NetworkPolicy/egress into the retrieved esc, then runtimeClient.Update or Patch)
or return a clear error/skip before proceeding to the Bitwarden flows; apply the
same change to the duplicate logic at the other location mentioned (lines
189-204) so pre-existing objects get the required egress rule or fail fast with
an informative message.
- Around line 121-145: After rotating the TLS secret and updating
ExternalSecretsConfig inside the retry.RetryOnConflict block, ensure the
bitwarden-sdk-server is restarted so it picks up the new certs: after the
runtimeClient.Update(...) returns, delete or restart the bitwarden-sdk-server
pods (those matching bitwardenSDKServerPodPrefix in
utils.BitwardenOperandNamespace) — e.g. call a helper that deletes pods by
prefix or perform a rollout restart on the Deployment/StatefulSet — then wait
for them to be ready using utils.VerifyPodsReadyByPrefix and confirm
reachability with utils.WaitForBitwardenSDKServerReachableFromCluster; add this
restart logic immediately after the Update and before the existing
VerifyPodsReadyByPrefix calls.
In `@test/e2e/e2e_suite_test.go`:
- Around line 97-99: The suite timeout (suiteConfig.Timeout) is set to 90
minutes which exceeds the outer go test timeout and can cause the process to be
killed before Ginkgo finishes; change the value to be safely under the outer
test harness limit (for example use 50 * time.Minute or read a
TEST_SUITE_TIMEOUT env var and parse it), leaving suiteConfig.FailFast as false
so AfterSuite and reports can complete; update the assignment of
suiteConfig.Timeout in e2e_suite_test.go (replace the 90 * time.Minute literal)
to the chosen shorter timeout or env-driven value.
In `@test/utils/bitwarden_api_runner.go`:
- Around line 35-38: Update the bitwardenAPITestRunnerImage constant to pin the
curl image to a specific, stable tag or digest instead of ":latest"; locate the
constant bitwardenAPITestRunnerImage in test/utils/bitwarden_api_runner.go and
replace "docker.io/curlimages/curl:latest" with a fixed tag (e.g.,
"docker.io/curlimages/curl:8.5.0") or a content digest (sha256:...) to ensure
reproducible test runs; leave bitwardenCredMountPath unchanged.
In `@test/utils/cleanup.go`:
- Around line 64-132: The cleanup helper is deleting cluster-wide ESO resources
indiscriminately; change it to only remove suite-owned objects by (a) adding a
guard boolean or env/config flag (e.g., enableDestructiveCleanup) around the
entire helper so AfterSuite must opt-in, and (b) restrict all List/Delete calls
that currently use operandLabelSelector to combine that selector with a
suite-ownership label (e.g., addSuiteLabel or suiteID label) instead of just
"app=external-secrets" — update uses of operandLabelSelector,
dynamicClient.Resource(...).List/Namespace(...).List,
clientset.AdmissionregistrationV1().ValidatingWebhookConfigurations().List,
clientset.RbacV1().ClusterRoleBindings().List, ClusterRoles().List and the
ExternalSecretsConfig delete to either be skipped when the opt-in flag is false
or to include the additional suite label in their metav1.ListOptions so only
suite-created resources are impacted.
In `@test/utils/conditions.go`:
- Around line 240-245: FetchAWSCredsFromSecret currently returns empty strings
when the secret is missing required keys, causing opaque AWS auth errors later;
update FetchAWSCredsFromSecret to validate that cred.Data contains both
awsCredKeyIdSecretKeyName and awsCredAccessKeySecretKeyName, and if either key
is absent return a descriptive non-nil error (e.g., via fmt.Errorf) instead of
empty strings so callers receive a clear secret-format error immediately.
In `@test/utils/dynamic_resources.go`:
- Around line 92-98: CreateFromUnstructuredReturnErr fails to apply
overrideNamespace to the actual unstructured object before creating it; call
unstructuredObj.SetNamespace(overrideNamespace) (or clear it when empty) after
obtaining the ResourceInterface in getResourceInterface and before calling
dri.Create in CreateFromUnstructuredReturnErr (mirroring the pattern used in
do()), and apply the same fix to CreateFromUnstructured so the object’s metadata
namespace matches the ResourceInterface’s namespace.
---
Outside diff comments:
In `@test/utils/conditions.go`:
- Around line 250-271: The DeleteAWSSecretFromCredsSecret function is using
svc.DeleteSecret which ignores the provided ctx; change the call to the
context-aware method svc.DeleteSecretWithContext(ctx, ...) so the Secrets
Manager request respects cancellation and timeouts, passing the same
DeleteSecretInput you currently build and propagating the returned error (update
the error message still referencing failed to delete AWS secret). Locate
DeleteAWSSecretFromCredsSecret and replace the svc.DeleteSecret invocation with
svc.DeleteSecretWithContext using the existing ctx variable.
---
Nitpick comments:
In `@hack/govulncheck.sh`:
- Around line 23-27: Add a short tracking reference next to the
KNOWN_VULNS_PATTERN declaration so the suppressions GO-2026-4601 and
GO-2026-4602 are revisited when the downstream Go runtime fix is available;
update the comment above or on the same line as the KNOWN_VULNS_PATTERN variable
to include a GitHub issue or Jira ticket ID (e.g., "tracking:
github.com/org/repo/issues/1234") and a brief note about re-evaluating/removing
these IDs once the Go fixes are released; ensure you reference the
KNOWN_VULNS_PATTERN symbol so reviewers can easily find and confirm the tracking
link.
In `@test/utils/dynamic_resources.go`:
- Around line 112-128: The REST mapping and namespace handling duplicated
between getResourceInterface and do() should be extracted into a shared helper
(e.g., resolveResourceInterface or buildResourceInterface) on
DynamicResourceLoader; move the logic that does
restmapper.GetAPIGroupResources(d.KubeClient.Discovery()),
restmapper.NewDiscoveryRESTMapper(gr), mapper.RESTMapping(gvk.GroupKind(),
gvk.Version), scope check, overrideNamespace application to objCopy.SetNamespace
and require.NotEmpty into that helper, and have both getResourceInterface and
do() call it to obtain the dynamic.ResourceInterface (or the mapping +
namespace) so behavior is centralized and duplication removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dac78b36-6ed1-467b-af6e-d938a3f1882c
📒 Files selected for processing (9)
Makefilehack/govulncheck.shtest/e2e/bitwarden_es_test.gotest/e2e/e2e_suite_test.gotest/utils/bitwarden.gotest/utils/bitwarden_api_runner.gotest/utils/cleanup.gotest/utils/conditions.gotest/utils/dynamic_resources.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/utils/bitwarden.go
Signed-off-by: Bharath B <bhb@redhat.com>
|
@bharath-b-rh: This pull request references ESO-323 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.22." or "openshift-4.22.", but it targets "eso-1.1" instead. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/e2e/bitwarden_es_test.go (1)
175-177: Consider usingGinkgoT()instead of&testing.T{}.Line 177 creates
DynamicResourceLoaderwith&testing.T{}which is a zero-value struct. If the loader usest.Errorf()or similar methods, they won't integrate with Ginkgo's failure reporting. Consider usingGinkgoT()which provides a*testing.T-compatible interface that works with Ginkgo.♻️ Proposed fix
- loader = utils.NewDynamicResourceLoader(ctx, &testing.T{}) + loader = utils.NewDynamicResourceLoader(ctx, GinkgoT())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/bitwarden_es_test.go` around lines 175 - 177, Replace the zero-value testing.T passed into NewDynamicResourceLoader with Ginkgo's test adaptor so Ginkgo failure reporting is used: locate the BeforeAll block where loader is constructed via utils.NewDynamicResourceLoader(ctx, &testing.T{}) and change the second argument to GinkgoT() (importing Ginkgo's function if needed) so the loader receives a *testing.T-compatible object from Ginkgo rather than a bare &testing.T{}.test/utils/bitwarden.go (1)
207-236: Note: Labels and annotations are not preserved when copying secrets.The function copies only
DataandTypefrom the source secret. If the source secret has labels or annotations that are needed in the destination, they won't be copied. This is likely intentional for test purposes but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/bitwarden.go` around lines 207 - 236, The CopySecretToNamespace function currently only copies Data and Type from the source secret; update it to also copy labels and annotations from the source secret into the destination object and when updating an existing secret. Specifically, in CopySecretToNamespace, set dest.ObjectMeta.Labels and dest.ObjectMeta.Annotations from secret.ObjectMeta.Labels/Annotations (or their shallow copies to avoid shared map mutation) before Create, and likewise update existing.ObjectMeta.Labels and existing.ObjectMeta.Annotations from the source before calling Update; avoid copying immutable meta fields like UID/resourceVersion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/utils/dynamic_resources.go`:
- Around line 112-126: getResourceInterface currently only sets namespace for
namespaced resources but doesn't clear metadata.namespace for cluster-scoped
ones, so ensure you clear any previously set namespace when mapping.Scope is
cluster-wide: in DynamicResourceLoader.getResourceInterface, after computing
mapping and checking if mapping.Scope.Name() != meta.RESTScopeNameNamespace,
call unstructuredObj.SetNamespace("") before returning
d.DynamicClient.Resource(mapping.Resource); leave the namespaced branch
(including require.NotEmpty and optional overrideNamespace logic) unchanged.
---
Nitpick comments:
In `@test/e2e/bitwarden_es_test.go`:
- Around line 175-177: Replace the zero-value testing.T passed into
NewDynamicResourceLoader with Ginkgo's test adaptor so Ginkgo failure reporting
is used: locate the BeforeAll block where loader is constructed via
utils.NewDynamicResourceLoader(ctx, &testing.T{}) and change the second argument
to GinkgoT() (importing Ginkgo's function if needed) so the loader receives a
*testing.T-compatible object from Ginkgo rather than a bare &testing.T{}.
In `@test/utils/bitwarden.go`:
- Around line 207-236: The CopySecretToNamespace function currently only copies
Data and Type from the source secret; update it to also copy labels and
annotations from the source secret into the destination object and when updating
an existing secret. Specifically, in CopySecretToNamespace, set
dest.ObjectMeta.Labels and dest.ObjectMeta.Annotations from
secret.ObjectMeta.Labels/Annotations (or their shallow copies to avoid shared
map mutation) before Create, and likewise update existing.ObjectMeta.Labels and
existing.ObjectMeta.Annotations from the source before calling Update; avoid
copying immutable meta fields like UID/resourceVersion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7481661b-0de4-43d8-95ed-ac62211f53e1
📒 Files selected for processing (9)
Makefilehack/govulncheck.shtest/e2e/bitwarden_es_test.gotest/e2e/e2e_suite_test.gotest/utils/bitwarden.gotest/utils/bitwarden_api_runner.gotest/utils/cleanup.gotest/utils/conditions.gotest/utils/dynamic_resources.go
🚧 Files skipped from review as they are similar to previous changes (3)
- Makefile
- test/utils/conditions.go
- hack/govulncheck.sh
Contributor
Author
|
no user facing changes, hence adding below labels |
openshift-ci
bot
added
the
docs-approved
|
@bharath-b-rh: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds new e2e test suites for the Bitwarden provider and for cross-platform (GCP cluster / AWS Secrets Manager), and introduces failure artifact collection and e2e documentation.
New test suites
Bitwarden
/ready,/live), auth (401/400), and Secrets API (ListSecrets, CreateSecret/Get/Update/Delete, error cases). Jobs run in the operand namespace with the shared egress network policy.Cross-platform
aws-credsinkube-system. Covers PushSecret and ExternalSecret with a fixed cred secret name/namespace.Failure artifact collection
<output-dir>/e2e-artifacts/failure-<timestamp>/:ARTIFACT_DIRin CI (e.g. OpenShift CI) or_output/locally. JUnit and JSON reports are written to the same directory.CleanupESOOperandAndRelatedto remove operand CR instances, ExternalSecretsConfig, webhooks, and the operand namespace so the cluster is left in a clean state.E2E documentation
test/e2e/README.mdand examplemake test-e2ewith label filter.bitwarden-credsandaws-credssecrets, failure artifacts, and running multiple suites.Utils
suiteClientset,suiteDynamicClient, andsuiteRuntimeClientare created once in BeforeSuite and reused by the main e2e describe and Bitwarden describes so client creation is shared and Bitwarden-only runs still work.DumpE2EArtifactsandCleanupESOOperandAndRelated;CreateFromUnstructured/DeleteFromUnstructuredfor dynamic resources.CurrentSpecReport().Failed().Notes
Refer comment for test output.
Summary by CodeRabbit
Documentation
Tests
Chores