CM-1043: Add IstioCSR e2e coverage and IstioCSR-ServiceMesh smoke tests - #427
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 a Ginkgo E2E suite and helpers to test IstioCSR reconciliation, degraded states, drift correction, and optional OpenShift Service Mesh gRPC certificate retrieval, plus Vault setup robustness, an ACME ingress helper, Makefile quoting, and a new e2e test plan. ChangesIstio-CSR E2E Test Suite
🎯 3 (Moderate) | ⏱️ ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
🤖 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/istio_csr_p0_test.go`:
- Around line 658-662: The test currently overwrites meshMemberNS.Labels with a
new map containing istioCSRP0MaistraMemberOfLabel, which drops any existing
labels; instead, before calling clientset.CoreV1().Namespaces().Update, merge
the new label into the existing meshMemberNS.Labels (create the map if nil) and
set meshMemberNS.Labels[istioCSRP0MaistraMemberOfLabel] = istioCPNamespace, then
call Update—this preserves other labels set by helpers/fixtures while applying
the Maistra label.
- Around line 371-374: The test currently sets acmev1.ACMEIssuer.Server to the
public LetsEncrypt endpoint, which creates an external dependency; change the
ACMEIssuer.Server value (the Server field on acmev1.ACMEIssuer in
istio_csr_p0_test.go) to a non-routable placeholder or local mock URL (e.g.
"https://example.invalid/directory" or the test harness mock endpoint) so the
negative-path e2e test does not attempt external network access; ensure any
related setup uses the mock URL so no real ACME calls are attempted.
🪄 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: 3455e985-372e-4c5f-8df8-1305f85b60a5
📒 Files selected for processing (2)
test/e2e/istio_csr_p0_test.gotest/e2e/utils_test.go
arun717
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@arun717: This pull request references CM-1043 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 sub-task 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: 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 `@Makefile`:
- Around line 191-193: Update the Makefile comment that shows how to skip OSM
smoke tests to use Ginkgo v2's negation syntax: replace the invalid example that
uses `isSubsetOf`/`Feature:!ServiceMesh` with the correct `--label-filter` form
such as `Platform:Generic && !Feature:ServiceMesh` (or grouped as
`Platform:Generic && !(Feature:ServiceMesh)`), and update the explanatory line
that currently states `Feature:!ServiceMesh is invalid Ginkgo syntax.` to
clarify that the `!` must prefix the whole label (e.g., `!Feature:ServiceMesh`),
referencing the existing comment text fragments `Platform:Generic &&
!(Feature:ServiceMesh)` and `Feature:!ServiceMesh` to locate and change them.
🪄 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: 2e0cbe82-fbaf-4324-bcf3-967cf0d251cb
📒 Files selected for processing (7)
Makefiletest/e2e/condition_matcher_test.gotest/e2e/issuer_acme_http01_test.gotest/e2e/istio_csr_p0_test.gotest/e2e/istio_csr_test.gotest/e2e/plans/pr-379/test-cases.mdtest/e2e/utils_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/istio_csr_p0_test.go
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/issuer_vault_test.go`:
- Line 467: The test currently splices oidcIssuer directly into a shell snippet
via vaultShellCmd which is then executed with execInPod("sh","-c",...), creating
a shell injection risk; change the call to avoid composing a shell string with
untrusted data by passing the issuer as an argv element instead (e.g. refactor
vaultShellCmd or add a new helper to accept variadic args and call execInPod
with the command and arguments directly), and update the other occurrence around
line 513 similarly so oidcIssuer is not interpolated into a shell command.
🪄 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: 1fe2f448-4ae8-4c9a-8ef5-ee88356092c2
📒 Files selected for processing (4)
Makefiletest/e2e/issuer_vault_test.gotest/e2e/istio_csr_p0_test.gotest/e2e/utils_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- test/e2e/istio_csr_p0_test.go
| "k8s.io/client-go/kubernetes" | ||
| ) | ||
|
|
||
| const ( |
There was a problem hiding this comment.
Should we move all the constants to a dedicated file?
There was a problem hiding this comment.
For this e2e package, what I observed was to:
Keep test-local constants close to the test file (like most in istio_csr_operand_test.go).
The current layout matches the rest of test/e2e.
Let me know if you still want these constants to be moved.
arun717
changed the title
arun717
changed the title
Add Feature:ServiceMesh coverage that installs OSSM v3 when enabled, validates root CA distribution, mesh workload gRPC signing, workload CertificateRequests, and cross-namespace envoy traffic. Co-authored-by: Cursor <cursoragent@cursor.com>
Use a generated test namespace so BeforeEach setup does not time out when OSSM smoke tests already created the real istio-system namespace. Co-authored-by: Cursor <cursoragent@cursor.com>
The operator accepts only one IstioCSR per cluster. OSSM smoke left an operand in istio-csr that blocked cert-manager-istio-csr deployment in istio-csr-e2e-* namespaces. Exclude Feature:ServiceMesh from the default CI label filter, clean up the OSSM operand after smoke tests, and add a clear timeout hint when a second instance is rejected. Co-authored-by: Cursor <cursoragent@cursor.com>
After moving grpc tests into istio-csr-e2e-* namespaces, the operand template still pointed issuer lookup at istio-system, so the operator never created cert-manager-istio-csr. Template the istio namespace, clean up leftover operands before the grpc suite, and improve timeout diagnostics. Co-authored-by: Cursor <cursoragent@cursor.com>
Extend parseGRPCurlLogEntry to unmarshal compact or multi-line JSON responses and include a truncated log excerpt in errors when parsing fails. Co-authored-by: Cursor <cursoragent@cursor.com>
816280e to
1f2a128
Compare
|
/lgtm |
openshift-ci
Bot
added
px-approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arun717, 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
|
@arun717: 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. |
|
Suggestion: @arun717 would you please squash the commits before merging (by adding a comment |
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
|
The PR adds only e2e test cases, and no user facing changes |
openshift-ci
Bot
added
the
qe-approved
4 participants
Adds end-to-end coverage for IstioCSR reconciler and operand behavior that was not covered by existing istio_csr_test.go specs, plus two opt-in OpenShift Service Mesh (OSM) smoke tests.
New file test/e2e/istio_csr_p0_test.go introduces 12 P0 scenarios under Feature:IstioCSR / Platform:Generic, covering:
Also adds an OSM smoke context (Feature:ServiceMesh) with two cases that auto-skip when no istiod control plane is present:
Supporting changes:
utils_test.go: waitForIstioCSRConditionMessage helper for degraded-condition assertions
istio_csr_test.go: traceability label on existing grpc smoke (CM-867-TC-010)
Test plan
Default e2e (no mesh required):
E2E_GINKGO_LABEL_FILTER='Feature:IstioCSR && Platform:Generic' make test-e2eRun P0 specs individually if needed:
E2E_GINKGO_LABEL_FILTER='ISTIOCSR-P0-001' make test-e2eOSM smoke (cluster with ready istiod required):
E2E_GINKGO_LABEL_FILTER='Feature:IstioCSR && Feature:ServiceMesh' make test-e2eConfirm existing IstioCSR grpc spec still passes:
E2E_GINKGO_LABEL_FILTER='CM-867-TC-010' make test-e2eVerify OSM smokes are skipped cleanly on clusters without Service Mesh / istiod
Notes
Summary by CodeRabbit
Tests
Chores
Documentation