CCO-788 : e2e for removing kube-rbac-proxy container (CCO-788)#991
CCO-788 : e2e for removing kube-rbac-proxy container (CCO-788)#991miyadav wants to merge 2 commits intoopenshift:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdded a new Ginkgo test that performs end-to-end HTTPS metrics validation for the cloud-credential-operator: verifies container port and volume mounts, service and ServiceMonitor configuration, Prometheus target health, authenticated metrics access, unauthenticated rejection, and reconciliation evidence in logs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/extend/cloudcredential.go (2)
783-783: Consider filtering by container name instead of index.Using
containers[0]assumes the CCO container is always first. If another container (e.g., sidecar) is added before it, this test would break. Consider usingcontainers[?(@.name=="cloud-credential-operator")]similar to line 496.♻️ Suggested fix
- ports, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("deployment", "cloud-credential-operator", "-n", DefaultNamespace, "-o=jsonpath={.spec.template.spec.containers[0].ports}").Output() + ports, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("deployment", "cloud-credential-operator", "-n", DefaultNamespace, "-o=jsonpath={.spec.template.spec.containers[?(@.name==\"cloud-credential-operator\")].ports}").Output()Apply the same change to line 791 for volumeMounts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extend/cloudcredential.go` at line 783, The jsonpath used when calling oc.AsAdmin().WithoutNamespace().Run("get").Args(... "-o=jsonpath={.spec.template.spec.containers[0].ports}") assumes the CCO container is at index 0; change the selector to filter by container name (e.g. use containers[?(@.name=="cloud-credential-operator")].ports) so it targets the container by name, and make the analogous change for the volumeMounts call (replace containers[0].volumeMounts with containers[?(@.name=="cloud-credential-operator")].volumeMounts) to avoid fragility if sidecars are added.
828-833: Redundant metric check logic.Line 828 already asserts that
metricsOutputcontains"cco_". ThehasMetriccheck on lines 830-832 is redundant since it includesstrings.Contains(metricsOutput, "cco_")which will always be true if line 828 passes.♻️ Simplified check
g.By("Verify response includes CCO business metrics") - o.Expect(metricsOutput).To(o.ContainSubstring("cco_")) - // Check for common CCO metrics - hasMetric := strings.Contains(metricsOutput, "cco_credentials_mode") || - strings.Contains(metricsOutput, "cco_credentials_requests") || - strings.Contains(metricsOutput, "cco_") - o.Expect(hasMetric).To(o.BeTrue(), "Expected to find CCO metrics in response") + // Check for common CCO metrics prefixed with cco_ + o.Expect(metricsOutput).To(o.ContainSubstring("cco_"), "Expected to find CCO metrics in response")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extend/cloudcredential.go` around lines 828 - 833, The test contains a redundant assertion: metricsOutput is already checked for "cco_" via o.Expect(metricsOutput).To(o.ContainSubstring("cco_")), so the subsequent hasMetric boolean (which includes strings.Contains(metricsOutput, "cco_")) is unnecessary; either remove the initial broad Expect and keep the specific hasMetric checks, or better, remove the "cco_" check inside hasMetric and let hasMetric only test for specific metrics ("cco_credentials_mode" or "cco_credentials_requests") before asserting with o.Expect(hasMetric).To(o.BeTrue()). Update references to metricsOutput, hasMetric, and the Expect calls accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/extend/cloudcredential.go`:
- Line 783: The jsonpath used when calling
oc.AsAdmin().WithoutNamespace().Run("get").Args(...
"-o=jsonpath={.spec.template.spec.containers[0].ports}") assumes the CCO
container is at index 0; change the selector to filter by container name (e.g.
use containers[?(@.name=="cloud-credential-operator")].ports) so it targets the
container by name, and make the analogous change for the volumeMounts call
(replace containers[0].volumeMounts with
containers[?(@.name=="cloud-credential-operator")].volumeMounts) to avoid
fragility if sidecars are added.
- Around line 828-833: The test contains a redundant assertion: metricsOutput is
already checked for "cco_" via
o.Expect(metricsOutput).To(o.ContainSubstring("cco_")), so the subsequent
hasMetric boolean (which includes strings.Contains(metricsOutput, "cco_")) is
unnecessary; either remove the initial broad Expect and keep the specific
hasMetric checks, or better, remove the "cco_" check inside hasMetric and let
hasMetric only test for specific metrics ("cco_credentials_mode" or
"cco_credentials_requests") before asserting with
o.Expect(hasMetric).To(o.BeTrue()). Update references to metricsOutput,
hasMetric, and the Expect calls accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce49d981-a422-4850-8b9a-210d3e419c0a
📒 Files selected for processing (1)
test/extend/cloudcredential.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #991 +/- ##
=======================================
Coverage 46.28% 46.28%
=======================================
Files 98 98
Lines 12259 12259
=======================================
Hits 5674 5674
Misses 5935 5935
Partials 650 650 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/extend/cloudcredential.go (2)
821-830: Consider adding polling for initial metrics fetch to reduce flakiness.The direct
wgetcall may fail transiently if the metrics endpoint isn't immediately ready. This contrasts with the Prometheus target validation below (lines 833-884) which properly useswait.Poll. Consider wrapping this in a similar poll loop.♻️ Suggested approach
g.By("Validate metrics are reachable via HTTPS from prometheus pod") - metricsOutput, err := oc.AsAdmin().WithoutNamespace().Run("exec").Args("-n", OpenShiftMonitoringNamespace, "prometheus-k8s-0", "-c", "prometheus", "--", "sh", "-c", fmt.Sprintf("wget -qO- --no-check-certificate --header='Authorization: Bearer %s' https://cco-metrics.%s.svc:8443/metrics 2>/dev/null | head -10", token, DefaultNamespace)).Output() - o.Expect(err).NotTo(o.HaveOccurred()) - o.Expect(metricsOutput).NotTo(o.BeEmpty()) + var metricsOutput string + err = wait.Poll(5*time.Second, 1*time.Minute, func() (bool, error) { + output, execErr := oc.AsAdmin().WithoutNamespace().Run("exec").Args("-n", OpenShiftMonitoringNamespace, "prometheus-k8s-0", "-c", "prometheus", "--", "sh", "-c", fmt.Sprintf("wget -qO- --no-check-certificate --header='Authorization: Bearer %s' https://cco-metrics.%s.svc:8443/metrics 2>/dev/null | head -10", token, DefaultNamespace)).Output() + if execErr != nil || output == "" { + g.GinkgoT().Logf("Metrics endpoint not ready yet, retrying...") + return false, nil + } + metricsOutput = output + return true, nil + }) + assertWaitPollNoErr(err, "metrics endpoint not reachable") g.GinkgoT().Logf("Metrics endpoint response (first 10 lines): %s", metricsOutput)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extend/cloudcredential.go` around lines 821 - 830, The direct wget exec that populates metricsOutput can be flaky; wrap the oc.AsAdmin().WithoutNamespace().Run("exec") call (the command that sets metricsOutput) in a retry/poll loop (e.g., wait.Poll or similar) that retries the exec until metricsOutput contains the expected "cco_" substring or a timeout elapses; update the checks around metricsOutput and the o.Expect assertions to run after successful poll completion and log failures appropriately so the test only asserts once the endpoint is reachable and returns CCO metrics.
822-822: Minor: Hard-coded prometheus pod name could be fragile.The test uses
prometheus-k8s-0in multiple places. If this specific pod is temporarily unavailable (e.g., during rolling update), the test will fail. Consider dynamically selecting an available prometheus pod from the list obtained at line 800.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extend/cloudcredential.go` at line 822, The test currently hard-codes the Prometheus pod name in the oc exec call (the call building metricsOutput using oc.AsAdmin().WithoutNamespace().Run("exec").Args(..., "prometheus-k8s-0", ...)), which is fragile; change it to pick an available Prometheus pod dynamically from the previously obtained list of Prometheus pods (the variable holding the pod list from the earlier pod-listing call) instead of "prometheus-k8s-0": iterate the pod list or filter for Ready/Running status, select the first healthy pod name, and use that podName in the Args(...) for the exec call; if selection fails, surface a clear error so the test fails fast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/extend/cloudcredential.go`:
- Around line 821-830: The direct wget exec that populates metricsOutput can be
flaky; wrap the oc.AsAdmin().WithoutNamespace().Run("exec") call (the command
that sets metricsOutput) in a retry/poll loop (e.g., wait.Poll or similar) that
retries the exec until metricsOutput contains the expected "cco_" substring or a
timeout elapses; update the checks around metricsOutput and the o.Expect
assertions to run after successful poll completion and log failures
appropriately so the test only asserts once the endpoint is reachable and
returns CCO metrics.
- Line 822: The test currently hard-codes the Prometheus pod name in the oc exec
call (the call building metricsOutput using
oc.AsAdmin().WithoutNamespace().Run("exec").Args(..., "prometheus-k8s-0", ...)),
which is fragile; change it to pick an available Prometheus pod dynamically from
the previously obtained list of Prometheus pods (the variable holding the pod
list from the earlier pod-listing call) instead of "prometheus-k8s-0": iterate
the pod list or filter for Ready/Running status, select the first healthy pod
name, and use that podName in the Args(...) for the exec call; if selection
fails, surface a clear error so the test fails fast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bac3160e-60a2-4880-a12c-5269d3fc8047
📒 Files selected for processing (1)
test/extend/cloudcredential.go
|
/approve |
|
@jstuever: Overrode contexts on behalf of jstuever: ci/prow/security 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 kubernetes-sigs/prow repository. |
openshift-ci
bot
added
the
approved
|
@miyadav: 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: huangmingxia, jstuever, miyadav 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 |
miyadav
changed the title
miyadav
changed the title
|
@miyadav Hi, if it passes locally on your side, please add the verified label and then let's proceed with merging the PR. |
|
/verified by @miyadav |
openshift-ci-robot
added
the
verified
|
@miyadav: This PR has been marked as verified by 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. |
4 participants
@huangmingxia @jianping-shu PTAL when time permits , test case.
Manual execution
Upgrade validation is pending.
Co-authored: claudecode