ESO-424:Added e2e test cases from openshift test private. - #160
ESO-424:Added e2e test cases from openshift test private.#160siddhibhor-56 wants to merge 2 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds AWS and generator e2e coverage, resource and AWS API helpers, namespace termination handling, suite lifecycle checks, and a govulncheck known-vulnerability filter update. ChangesProvider end-to-end coverage
Vulnerability filter maintenance
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProviderE2ESuite
participant KubernetesAPI
participant ExternalSecretsOperator
participant AWSSecretsManager
participant AWSSSMParameterStore
ProviderE2ESuite->>KubernetesAPI: Create provider resources
ExternalSecretsOperator->>AWSSecretsManager: Read or write secret values
ExternalSecretsOperator->>AWSSSMParameterStore: Read or write parameters
ExternalSecretsOperator->>KubernetesAPI: Reconcile Kubernetes Secret
ProviderE2ESuite->>KubernetesAPI: Verify synchronized Secret
ProviderE2ESuite->>AWSSecretsManager: Verify round trip
ProviderE2ESuite->>AWSSSMParameterStore: Verify round trip
Suggested reviewers: 🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 passed)
✨ 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.12.2)Error: build linters: unable to load custom analyzer "kubeapilinter": bin/kube-api-linter.so, plugin: not implemented Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: siddhibhor-56 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/e2e/helpers_test.go (1)
75-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the typed phase constant over the string literal.
Comparing
ns.Status.Phaseagainst the literal"Terminating"works but is fragile to typos and refactors. Use thecorev1.NamespaceTerminatingconstant fromk8s.io/api/core/v1.♻️ Proposed change
- if ns.Status.Phase != "Terminating" { + if ns.Status.Phase != corev1.NamespaceTerminating { return nil }Ensure
corev1 "k8s.io/api/core/v1"is imported.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/helpers_test.go` at line 75, The namespace phase check in the helper test is using a fragile string literal instead of the typed Kubernetes constant. Update the comparison in the namespace status assertion to use corev1.NamespaceTerminating from k8s.io/api/core/v1, and ensure the test file imports corev1 so the phase check stays consistent with the API types.test/e2e/provider_e2e_test.go (1)
273-289: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
Fail()instead ofExpect(false).To(BeTrue())for the missing-condition case.
g.Expect(false).To(BeTrue(), ...)works but is less idiomatic;Fail("expected Ready condition with ownership error")(orStopTrying) reads more clearly when the conditions loop finds noReadycondition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/provider_e2e_test.go` around lines 273 - 289, The missing-condition fallback in the ownership-error check is using a non-idiomatic false expectation; update the `Eventually` block in `provider_e2e_test.go` so that the `Ready` condition search in the `conds` loop uses `Fail("expected Ready condition with ownership error")` (or `StopTrying`) when no matching condition is found, rather than `g.Expect(false).To(BeTrue(), ...)`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/provider_e2e_test.go`:
- Around line 91-101: Add meaningful failure messages to the suite-setup
assertions in provider_e2e_test.go so Ginkgo failures are easier to diagnose.
Update the key Expect(...).To(Succeed()) calls around namespace creation,
VerifyPodsReadyByPrefix, and ensureExternalSecretsConfigReady in the setup flow
to include short, specific messages. Apply the same pattern consistently to
other resource-creation and readiness checks in this file, using the surrounding
helpers and setup steps as anchors.
- Around line 592-593: The AWS credential copy step is discarding the result of
CopyAWSCredsToNamespace, which can hide setup failures and cause later
SecretStore/PushSecret checks to fail confusingly. Update the provider_e2e_test
flow to assert that CopyAWSCredsToNamespace succeeds, matching the earlier AWS
Parameter Store setup pattern, and keep the failure close to the credential-copy
step for clearer test diagnostics.
---
Nitpick comments:
In `@test/e2e/helpers_test.go`:
- Line 75: The namespace phase check in the helper test is using a fragile
string literal instead of the typed Kubernetes constant. Update the comparison
in the namespace status assertion to use corev1.NamespaceTerminating from
k8s.io/api/core/v1, and ensure the test file imports corev1 so the phase check
stays consistent with the API types.
In `@test/e2e/provider_e2e_test.go`:
- Around line 273-289: The missing-condition fallback in the ownership-error
check is using a non-idiomatic false expectation; update the `Eventually` block
in `provider_e2e_test.go` so that the `Ready` condition search in the `conds`
loop uses `Fail("expected Ready condition with ownership error")` (or
`StopTrying`) when no matching condition is found, rather than
`g.Expect(false).To(BeTrue(), ...)`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 70d9ba79-663d-4969-821c-d2d4ca8627c2
⛔ Files ignored due to path filters (76)
test/go.sumis excluded by!**/*.sumvendor/github.com/aws/aws-sdk-go/service/ssm/api.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/service.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/waiters.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/AUTHORSis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/compression.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/conn.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/join.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/json.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/mask.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/mask_safe.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/prepared.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/proxy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/server.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/CONTRIBUTING.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/MAINTAINERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/NOTICEis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/connection.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/handlers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/priority.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/PATENTSis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/dictionary.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/options.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/read.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/write.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/stream.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/flowrate/flowrate.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/flowrate/io.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/flowrate/util.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/internal/socks/client.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/internal/socks/socks.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/proxy/dial.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/proxy/direct.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/proxy/per_host.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/proxy/proxy.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/proxy/socks5.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/httpstream/spdy/connection.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/httpstream/spdy/upgrade.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/proxy/dial.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/proxy/doc.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/proxy/transport.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/proxy/upgradeaware.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/third_party/forked/golang/netutil/addr.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/doc.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/errorstream.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/fallback.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/reader.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/remotecommand.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/resize.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/spdy.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/v1.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/v2.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/v3.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/v4.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/v5.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/websocket.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/transport/spdy/spdy.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/transport/websocket/roundtripper.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/util/exec/exec.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (5)
test/e2e/helpers_test.gotest/e2e/provider_e2e_test.gotest/go.modtest/utils/aws_provider_resources.gotest/utils/generator_resources.go
| got, err := providerClientset.CoreV1().Namespaces().Create(ctx, namespace, metav1.CreateOptions{}) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| providerTestNamespace = got.GetName() | ||
|
|
||
| By("Waiting for operator pod to be ready") | ||
| Expect(utils.VerifyPodsReadyByPrefix(ctx, providerClientset, operatorNamespace, []string{ | ||
| operatorPodPrefix, | ||
| })).To(Succeed()) | ||
|
|
||
| By("Ensuring ExternalSecretsConfig cluster CR exists and is Ready") | ||
| Expect(ensureExternalSecretsConfigReady(ctx)).To(Succeed()) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add failure messages to key suite-setup assertions.
Several setup Expect(...).To(Succeed()) calls (e.g., namespace creation, operator/operand readiness, ensureExternalSecretsConfigReady) lack messages, making BeforeAll/BeforeEach failures harder to triage. Adding short messages like "failed to create provider test namespace" improves diagnosability. This pattern recurs throughout the file (e.g., resource creation and readiness waits).
As per coding guidelines: "Flag Ginkgo test assertions that lack meaningful failure messages."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/provider_e2e_test.go` around lines 91 - 101, Add meaningful failure
messages to the suite-setup assertions in provider_e2e_test.go so Ginkgo
failures are easier to diagnose. Update the key Expect(...).To(Succeed()) calls
around namespace creation, VerifyPodsReadyByPrefix, and
ensureExternalSecretsConfigReady in the setup flow to include short, specific
messages. Apply the same pattern consistently to other resource-creation and
readiness checks in this file, using the surrounding helpers and setup steps as
anchors.
Source: Coding guidelines
|
/retest |
1 similar comment
|
/retest |
|
/test-all |
|
/test all |
siddhibhor-56
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@siddhibhor-56: This pull request references ESO-424 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/helpers_test.go`:
- Around line 68-76: Update the namespace termination check in the helper around
the namespace Get call to treat any non-nil ns.DeletionTimestamp as terminating,
regardless of ns.Status.Phase. Preserve the existing nil return for namespaces
that are neither marked for deletion nor in the Terminating phase.
In `@test/e2e/provider_e2e_test.go`:
- Around line 125-131: Label the AWS-dependent specs in the “AWS Parameter
Store” and “AWS Secrets Manager” contexts, including the [OCP-81813] push spec,
with [Skipped:Disconnected]. Preserve their existing AWS labels and test
behavior while ensuring all specs that call AWS SSM or Secrets Manager endpoints
are excluded in disconnected environments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4eae5032-69ca-401f-ac67-2efbf9cb4922
⛔ Files ignored due to path filters (6)
vendor/github.com/aws/aws-sdk-go/service/ssm/api.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/service.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/waiters.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (4)
test/e2e/helpers_test.gotest/e2e/provider_e2e_test.gotest/utils/aws_provider_resources.gotest/utils/generator_resources.go
| ns, err := clientset.CoreV1().Namespaces().Get(ctx, namespace, metav1.GetOptions{}) | ||
| if k8serrors.IsNotFound(err) { | ||
| return nil | ||
| } | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if ns.Status.Phase != "Terminating" { | ||
| return nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,140p' test/e2e/helpers_test.go
printf '\n--- usages ---\n'
rg -n "Terminating|DeletionTimestamp|is.*terminating|namespace.*terminating|helpers_test.go" test/e2e -g '*.go'Repository: openshift/external-secrets-operator
Length of output: 5754
🏁 Script executed:
sed -n '2050,2110p' test/e2e/e2e_test.go
python3 - <<'PY'
from pathlib import Path
text = Path('test/e2e/helpers_test.go').read_text()
start = text.index('func waitForNamespaceTermination')
print(text[start:text.index('// resourceType defines', start)])
PYRepository: openshift/external-secrets-operator
Length of output: 3179
🏁 Script executed:
rg -n "ensureExternalSecretsConfigReady|waitForNamespaceTermination|OperandDefaultNamespace" test/e2e -g '*.go'
sed -n '1,90p' test/e2e/helpers_test.goRepository: openshift/external-secrets-operator
Length of output: 4188
🌐 Web query:
Kubernetes NamespaceStatus phase Terminating deletionTimestamp before phase changes namespace deletion semantics
💡 Result:
In Kubernetes, the interaction between the Namespace.Status.Phase and the metadata.deletionTimestamp is a core mechanism of the graceful deletion process [1][2]. When a DELETE request is issued for a namespace, the API server sets the metadata.deletionTimestamp to the current server time [1][3]. This timestamp acts as the primary indicator that a graceful deletion has been requested [3][4]. Upon observing this change, the namespace-controller updates the Namespace.Status.Phase from Active to Terminating [1][5]. The deletion process follows these semantics: 1. Request and Marking: The presence of a non-nil deletionTimestamp prevents the creation of new objects within the namespace [6]. The phase transition to Terminating signals to users and system components that the namespace is undergoing cleanup and is no longer available for new workloads [7][8]. 2. Resource Cleanup: The namespace-controller begins iterating through and deleting all content (resources) associated with the namespace [1][6]. 3. Finalization: The namespace itself contains a spec.finalizers list (typically including the kubernetes finalizer) [1][2]. The namespace cannot be removed from storage until this list is empty [3][4]. The namespace-controller is responsible for purging the namespace's contents and, upon successful cleanup, removing the finalizers [1][6]. 4. Permanent Removal: Once all finalizers are removed and the deletionTimestamp is set, the namespace-controller sends a final DELETE action to the API server to permanently remove the namespace object from etcd storage [1][2]. If a namespace remains stuck in the Terminating phase, it is typically because one or more finalizers have not been removed, often due to a controller failing to successfully clean up the associated resources or a manual error (e.g., specifying a non-existent finalizer) [9][3][8]. Because the deletionTimestamp is already set, the namespace is effectively "locked" into the deletion flow until the finalizers are cleared [3][4]. Top results: [1][5][6][9][3]
Citations:
- 1: https://github.com/kubernetes/design-proposals-archive/blob/main/architecture/namespaces.md
- 2: https://github.com/kubernetes/kubernetes/blob/release-1.2/docs/design/namespaces.md
- 3: https://kubernetes.io/docs/concepts/overview/working-with-objects/finalizers/
- 4: https://kubernetes.io/docs/reference/kubernetes-api/definitions/object-meta-v1-meta/
- 5: https://github.com/kubernetes/kubernetes/blob/v1.23.4/pkg/controller/namespace/deletion/namespaced_resources_deleter.go
- 6: https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/namespace/namespace_controller.go
- 7: https://kubernetes.io/docs/reference/kubernetes-api/cluster-resources/namespace-v1/
- 8: https://kubernetes.io/docs/tasks/administer-cluster/namespaces/
- 9: https://www.redhat.com/en/blog/troubleshooting-terminating-namespaces
Treat a non-nil deletionTimestamp as terminating. Lines 75-76 can return while the namespace is already marked for deletion but still reports Active, letting setup continue before teardown finishes and making later readiness checks flaky.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/helpers_test.go` around lines 68 - 76, Update the namespace
termination check in the helper around the namespace Get call to treat any
non-nil ns.DeletionTimestamp as terminating, regardless of ns.Status.Phase.
Preserve the existing nil return for namespaces that are neither marked for
deletion nor in the Terminating phase.
| Context("AWS Parameter Store", Label("Platform:AWS"), func() { | ||
| const awsCredsLocalName = "aws-creds" | ||
|
|
||
| BeforeAll(func() { | ||
| By("Copying AWS credentials into the test namespace") | ||
| Expect(utils.CopyAWSCredsToNamespace(ctx, providerClientset, providerTestNamespace, awsCredsLocalName)).To(Succeed()) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
AWS provider specs reach external AWS endpoints; add a disconnected guard.
The AWS Parameter Store / Secrets Manager contexts (and the [OCP-81813] push spec at Line 579) drive live calls to AWS SSM and Secrets Manager APIs, which are unreachable in disconnected/IPv6-only environments. Consider labeling these AWS-dependent specs with [Skipped:Disconnected] so they don't fail there.
As per coding guidelines: "flag tests that require connectivity to public internet hosts ... Tests failing in IPv6-only disconnected environments should be flagged unless already protected with [Skipped:Disconnected] label."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/provider_e2e_test.go` around lines 125 - 131, Label the
AWS-dependent specs in the “AWS Parameter Store” and “AWS Secrets Manager”
contexts, including the [OCP-81813] push spec, with [Skipped:Disconnected].
Preserve their existing AWS labels and test behavior while ensuring all specs
that call AWS SSM or Secrets Manager endpoints are excluded in disconnected
environments.
Source: Coding guidelines
de7000b to
6f9f10a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/provider_e2e_test.go`:
- Line 78: Update the provider setup around NewDynamicResourceLoader to pass the
active Ginkgo test handle instead of constructing &testing.T{}; ensure the
loader’s t.Logf and require calls use that real test context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: be6c882d-b25a-49df-ac55-384fb15ba621
📒 Files selected for processing (5)
hack/govulncheck.shtest/e2e/helpers_test.gotest/e2e/provider_e2e_test.gotest/utils/aws_provider_resources.gotest/utils/generator_resources.go
🚧 Files skipped from review as they are similar to previous changes (2)
- hack/govulncheck.sh
- test/e2e/helpers_test.go
| providerClientset = suiteClientset | ||
| providerDynamicClient = suiteDynamicClient | ||
| providerRuntimeClient = suiteRuntimeClient | ||
| providerLoader = utils.NewDynamicResourceLoader(ctx, &testing.T{}) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect NewDynamicResourceLoader and DeleteFromUnstructured usage of *testing.T
fd -e go -t f . test/utils | xargs rg -nP 'func NewDynamicResourceLoader|DeleteFromUnstructured|CreateFromUnstructured'
ast-grep run --pattern '$T.Fatal($$$)' --lang go test/utils
ast-grep run --pattern '$T.Fatalf($$$)' --lang go test/utilsRepository: openshift/external-secrets-operator
Length of output: 1372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- test/e2e/provider_e2e_test.go around line 78 ---'
sed -n '60,90p' test/e2e/provider_e2e_test.go | cat -n
echo
echo '--- test/utils/dynamic_resources.go around loader methods ---'
sed -n '1,220p' test/utils/dynamic_resources.go | cat -nRepository: openshift/external-secrets-operator
Length of output: 10138
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- locate NewClientsConfigForTest ---'
rg -n "func NewClientsConfigForTest|NewClientsConfigForTest\(" test -g '*.go'
echo
echo '--- inspect the implementation ---'
file=$(rg -l "func NewClientsConfigForTest" test -g '*.go' | head -n 1)
sed -n '1,220p' "$file" | cat -n
echo
echo '--- search for zero-value testing.T patterns ---'
rg -n "&testing\.T\{\}|testing\.T\{\}" -g '*.go' .Repository: openshift/external-secrets-operator
Length of output: 9412
Pass a real test handle to NewDynamicResourceLoader test/e2e/provider_e2e_test.go:78
DynamicResourceLoader uses t.Logf, require.NoError, and require.NotEmpty, so &testing.T{} is not a safe stand-in here. Use the active Ginkgo test handle or refactor the loader to return errors instead of depending on *testing.T.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/provider_e2e_test.go` at line 78, Update the provider setup around
NewDynamicResourceLoader to pass the active Ginkgo test handle instead of
constructing &testing.T{}; ensure the loader’s t.Logf and require calls use that
real test context.
|
/retest |
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/e2e/provider_e2e_test.go (1)
505-576: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit this spec: it asserts three independent behaviors.
ClusterGenerator password generation, regeneration-on-refresh, and namespaced
Passwordgenerator are separate behaviors; a failure in the first leaves the rest unverified. Consider threeItblocks with sharedBeforeEachsetup.As per coding guidelines: "Review Ginkgo test code for single responsibility: each test (It block) should test one specific behavior."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/provider_e2e_test.go` around lines 505 - 576, Split the combined spec into three independent It blocks covering ClusterGenerator password format, ClusterGenerator regeneration on refresh, and namespaced Password generator format. Extract the shared setup into BeforeEach or reusable setup helpers, while preserving the existing resource cleanup and assertions for each behavior.Source: Coding guidelines
hack/govulncheck.sh (1)
31-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep one vulnerability list and one pattern assignment.
GO-2026-5970remains documented even though it was removed from the effective filter, whileKNOWN_VULNS_PATTERNis assigned twice andGO-2026-5856is justified twice. Remove the stale block and retain a single authoritative comment/pattern pair.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/govulncheck.sh` around lines 31 - 35, In hack/govulncheck.sh, consolidate the duplicate GO-2026-5856 documentation and KNOWN_VULNS_PATTERN assignments into one authoritative comment/pattern pair. Remove the stale GO-2026-5970 entry and retain only the effective vulnerability list in KNOWN_VULNS_PATTERN.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/provider_e2e_test.go`:
- Around line 332-346: The decoding-strategy coverage in the ExternalSecret e2e
flow is tautological because it uses plaintext input. Update the test setup to
store a base64-encoded Secrets Manager value, then adjust the verification
around the ExternalSecret patch and synced Secret assertions to confirm Auto
produces the decoded plaintext; preserve or add the corresponding None assertion
that expects the encoded value.
In `@test/utils/aws_provider_resources.go`:
- Around line 368-386: Update CopyAWSCredsToNamespace to validate that src.Data
contains non-empty awsCredKeyIdSecretKeyName and awsCredAccessKeySecretKeyName
before constructing the destination Secret, returning a descriptive error when
either is missing. When creating the target secret, treat
apierrors.IsAlreadyExists(err) as success while still returning other creation
errors; add the apierrors import required for this check.
---
Nitpick comments:
In `@hack/govulncheck.sh`:
- Around line 31-35: In hack/govulncheck.sh, consolidate the duplicate
GO-2026-5856 documentation and KNOWN_VULNS_PATTERN assignments into one
authoritative comment/pattern pair. Remove the stale GO-2026-5970 entry and
retain only the effective vulnerability list in KNOWN_VULNS_PATTERN.
In `@test/e2e/provider_e2e_test.go`:
- Around line 505-576: Split the combined spec into three independent It blocks
covering ClusterGenerator password format, ClusterGenerator regeneration on
refresh, and namespaced Password generator format. Extract the shared setup into
BeforeEach or reusable setup helpers, while preserving the existing resource
cleanup and assertions for each behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4d82b6d8-7ae1-4813-be1d-0109a93c1af9
⛔ Files ignored due to path filters (6)
vendor/github.com/aws/aws-sdk-go/service/ssm/api.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/service.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/waiters.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (5)
hack/govulncheck.shtest/e2e/helpers_test.gotest/e2e/provider_e2e_test.gotest/utils/aws_provider_resources.gotest/utils/generator_resources.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/helpers_test.go
| By("Patching ExternalSecret to enable decodingStrategy Auto") | ||
| patch := []byte(`{"spec":{"dataFrom":[{"extract":{"key":"` + awsSecretName + `","decodingStrategy":"Auto"}}]}}`) | ||
| _, err := providerDynamicClient.Resource(externalSecretGVR).Namespace(providerTestNamespace).Patch( | ||
| ctx, esName, "application/merge-patch+json", patch, metav1.PatchOptions{}, | ||
| ) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| By("Verifying the decoded value") | ||
| Eventually(func(g Gomega) { | ||
| secret, err := providerClientset.CoreV1().Secrets(providerTestNamespace).Get(ctx, targetSecret, metav1.GetOptions{}) | ||
| g.Expect(err).NotTo(HaveOccurred()) | ||
| data := secret.Data[secretKey] | ||
| g.Expect(data).NotTo(BeEmpty()) | ||
| g.Expect(string(data)).To(Equal(initialData), "decoded value should match") | ||
| }, time.Minute, 5*time.Second).Should(Succeed()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Decoding-strategy assertion is tautological — the test never exercises decoding.
The remote secret value is plaintext ({"key":"value"}), so decodingStrategy: Auto is a no-op and Equal(initialData) passes regardless of whether decoding works. To actually validate strategies, store a base64-encoded value in Secrets Manager and assert the synced secret contains the decoded plaintext (and, for None, the still-encoded form).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/provider_e2e_test.go` around lines 332 - 346, The decoding-strategy
coverage in the ExternalSecret e2e flow is tautological because it uses
plaintext input. Update the test setup to store a base64-encoded Secrets Manager
value, then adjust the verification around the ExternalSecret patch and synced
Secret assertions to confirm Auto produces the decoded plaintext; preserve or
add the corresponding None assertion that expects the encoded value.
| func CopyAWSCredsToNamespace(ctx context.Context, k8sClient *kubernetes.Clientset, targetNamespace, targetSecretName string) error { | ||
| src, err := k8sClient.CoreV1().Secrets(awsCredNamespace).Get(ctx, awsCredSecretName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return fmt.Errorf("get source aws-creds secret: %w", err) | ||
| } | ||
|
|
||
| dst := &corev1.Secret{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: targetSecretName, | ||
| Namespace: targetNamespace, | ||
| }, | ||
| Data: map[string][]byte{ | ||
| awsCredKeyIdSecretKeyName: src.Data[awsCredKeyIdSecretKeyName], | ||
| awsCredAccessKeySecretKeyName: src.Data[awsCredAccessKeySecretKeyName], | ||
| }, | ||
| } | ||
| _, err = k8sClient.CoreV1().Secrets(targetNamespace).Create(ctx, dst, metav1.CreateOptions{}) | ||
| return err | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate source credential keys and tolerate an existing target secret.
If kube-system/aws-creds lacks the expected keys, this silently creates a secret with empty values and the failure surfaces much later as an opaque SecretStore/AWS auth error. Create also fails with AlreadyExists if a prior spec copied the same name.
🛡️ Proposed fix
+ keyID, ok := src.Data[awsCredKeyIdSecretKeyName]
+ if !ok || len(keyID) == 0 {
+ return fmt.Errorf("source secret %s/%s missing key %q", awsCredNamespace, awsCredSecretName, awsCredKeyIdSecretKeyName)
+ }
+ accessKey, ok := src.Data[awsCredAccessKeySecretKeyName]
+ if !ok || len(accessKey) == 0 {
+ return fmt.Errorf("source secret %s/%s missing key %q", awsCredNamespace, awsCredSecretName, awsCredAccessKeySecretKeyName)
+ }
+
dst := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: targetSecretName,
Namespace: targetNamespace,
},
Data: map[string][]byte{
- awsCredKeyIdSecretKeyName: src.Data[awsCredKeyIdSecretKeyName],
- awsCredAccessKeySecretKeyName: src.Data[awsCredAccessKeySecretKeyName],
+ awsCredKeyIdSecretKeyName: keyID,
+ awsCredAccessKeySecretKeyName: accessKey,
},
}
- _, err = k8sClient.CoreV1().Secrets(targetNamespace).Create(ctx, dst, metav1.CreateOptions{})
- return err
+ if _, err = k8sClient.CoreV1().Secrets(targetNamespace).Create(ctx, dst, metav1.CreateOptions{}); err != nil {
+ if !apierrors.IsAlreadyExists(err) {
+ return err
+ }
+ if _, err = k8sClient.CoreV1().Secrets(targetNamespace).Update(ctx, dst, metav1.UpdateOptions{}); err != nil {
+ return err
+ }
+ }
+ return nilRequires importing apierrors "k8s.io/apimachinery/pkg/api/errors".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/utils/aws_provider_resources.go` around lines 368 - 386, Update
CopyAWSCredsToNamespace to validate that src.Data contains non-empty
awsCredKeyIdSecretKeyName and awsCredAccessKeySecretKeyName before constructing
the destination Secret, returning a descriptive error when either is missing.
When creating the target secret, treat apierrors.IsAlreadyExists(err) as success
while still returning other creation errors; add the apierrors import required
for this check.
|
@siddhibhor-56: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Added test cases for :-
Summary by CodeRabbit
e2eprovider suite for AWS Parameter Store and Secrets Manager, covering secret sync, updates, decoding (Auto), ownership/adoption behavior, conflict handling, and PushSecret round-trips.