Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
 |
|
Verdict: The specification covers all six requested metrics and keeps the approved dashboard layouts, unavailable-value rules, and seven-day retention limit. The cross-component contracts for control-plane gauges, BFF responses, adapter mapping, and widget presentation are clear. Repository policy checks pass. One blocking identifier defect remains:
Specification identifiers must be unique and stable. Keep the existing After those identifiers are corrected, the specification is ready for another review. |
|
Verdict: The specification covers all six requested dashboard signals, preserves the seven-day limit for the new P99 fields, references the approved mockups, and fixes the prior duplicate success-criteria IDs. The metric boundaries and unavailable-value behavior are generally clear. Blocking findings:
|
|
Verdict: The specification now fixes the prior identifier, p95 fallback, index, and user-flow findings. It covers all six requested dashboard signals, preserves the seven-day P99 limit, references the approved mockups, and defines explicit unavailable states. Two blocking contract defects remain:
This is the third specification rejection. The issue requires human review before another agent retry. |
|
Verdict: The specification now defines all six requested dashboard signals and keeps the approved mockup layouts and seven-day constraint. The corrected certificate contract deduplicates replicas per cluster and then takes the fleet minimum, so The full specification tree now agrees on API p95, provisioning P99, readiness, certificate health, OIDC client sync, and state accuracy. The approved mockup references remain intact. Repository policy, specification identifier checks, Non-blocking observation: implementation must preserve the specified incomplete-seed unavailable state and must add future resource kinds to both state-accuracy gauges atomically. |
kdoberst
changed the title
| } | ||
|
|
||
| func (r *SandboxAttentionReconciler) listCertificateMaterials(ctx context.Context, eligible map[string]struct{}) ([][]byte, error) { | ||
| secrets, err := r.client.CoreV1().Secrets("").List(ctx, metav1.ListOptions{}) |
There was a problem hiding this comment.
[Major] Limit the certificate population to reconciled gateway and console resources. This cluster-wide list accepts every TLS Secret in every eligible namespace, and the Route loop below accepts every TLS Route. Unrelated workload certificates can inflate the dashboard counts, and the same certificate can be counted twice when it is represented by both a Secret and a Route. OBS-CPL-10 and issue #807 limit this metric to gateway and console TLS resources that the control plane reconciles. Resolve the expected Secret/Route identities from the reconciled gateway model, ignore unrelated objects, deduplicate one logical certificate, and add coverage for unrelated and duplicate TLS objects. Confidence: High.
| ).catch(() => undefined), | ||
| ]); | ||
|
|
||
| return { |
There was a problem hiding this comment.
[Major] Return the certificate widget as unavailable when the evaluated population is empty. With zero certificate materials, the control plane publishes three zero counts and this mapper returns a successful total_certificates: 0; the adapter then creates a metric, so the card renders 0 / 0 instead of Metric unavailable. WEB-OPS-34 and issue #807 require the whole widget to be unavailable when no certificate can be evaluated. Reject a zero total here (or omit the metric before it reaches the UI), and add a test that covers the all-zero gauge population through BFF mapping and card rendering. Confidence: High.
|
|
||
| observations := gateway.BuildAttentionObservations(pods, gateway.IndexSandboxes(sandboxes), eligible) | ||
| r.recordUsage(ctx, eligible, sandboxes, observations, crListFailed) | ||
| if !crListFailed && r.publishReadiness != nil { |
There was a problem hiding this comment.
[Major] Fail closed when the readiness or certificate source cannot be read. On a Sandbox CR error, readiness publication is skipped; on a certificate-list error, certificate publication is skipped. Prometheus can continue returning the previous gauge samples during its lookback window, so both widgets can show an old healthy percentage or valid-certificate count while the current source is unavailable. This conflicts with the issue requirement that unavailable sources must not silently report 0, 100%, or a valid state. Export and query an explicit availability/freshness signal (or another fail-closed representation), and test a successful refresh followed by each source failure. Confidence: High.
kdoberst
left a comment
kdoberst
left a comment
There was a problem hiding this comment.
The implementation connects all six requested SLI areas across the control plane, BFF, and dashboard UI, preserves admin-only placement, and adds broad automated coverage. However, the certificate population and unavailable-state handling can report incorrect or stale healthy values, so this change is not ready to merge.
Major
-
components/control-plane/internal/reconciler/dashboard_health.go:131collects every TLS Secret and TLS Route in eligible namespaces instead of only the gateway and console certificate resources that the control plane reconciles. This can count unrelated workload certificates and can double-count one certificate that appears through more than one object. Limit collection to expected reconciled resource identities, deduplicate logical certificates, and test unrelated and duplicate objects. Confidence: High. -
components/web-console/bff/src/metrics-certificate-health.ts:43treats an all-zero certificate population as a valid response. The adapter then creates a metric and the UI renders0 / 0, but WEB-OPS-34 and issue #807 require the localized unavailable state when no certificate can be evaluated. Reject or omit the zero-total metric and cover the full BFF-to-card path. Confidence: High. -
components/control-plane/internal/reconciler/sandbox_attention.go:157skips readiness or certificate gauge publication when its current source fails. Prometheus can continue serving the previous samples during its lookback window, so the dashboard can show stale healthy values while the source is unavailable. Add an explicit availability or freshness signal, gate the BFF result on it, and test success followed by source failure. Confidence: High.
CI was still running for E2E / Kind and two Konflux enterprise-contract checks when reviewed. The completed repository, frontend, control-plane, dashboard, security, and web-console checks were green; OpenShift E2E was skipped. The PR is conflict-free and GitHub reports it as mergeable.
GitHub does not permit the PR author to submit a formal REQUEST_CHANGES review on their own PR. This COMMENT records the same blocking assessment.
Overall assessment: REQUEST_CHANGES
Findings Summary (ordered by severity, highest first):
- [Major] Certificate discovery includes unrelated and duplicate TLS objects - Spec Consistency (
dashboard_health.go:131) - [Major] Empty certificate populations render as
0 / 0instead of unavailable - Correctness (metrics-certificate-health.ts:43) - [Major] Source failures can leave stale readiness and certificate health visible - Availability (
sandbox_attention.go:157)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound handled for optional Route API |
Pass |
| No secrets in logs or responses | Pass |
| Low-cardinality metric labels | Pass |
| Missing sources fail closed | Fail |
| Reconciled certificate population is scoped correctly | Fail |
| Existing test assertion changes justified | Pass |
| Conventional commit messages | Pass |
| } | ||
| secretMaterials[secret.Namespace+"\x00"+secret.Name] = secret.Data[corev1.TLSCertKey] | ||
| } | ||
| for namespace := range eligible { |
There was a problem hiding this comment.
[Major] Certificate inventory currently expects all four Secret names in every gateway namespace and also adds the console Route. The clarification requires configuration-dependent Secret identities. Derive the expected Secret set from each gateway configuration, omit Route material from this inventory, and test default and opted-in gateways. Confidence: High (99%).
| return false, nil | ||
| } | ||
| } | ||
| mappers, err := c.listClientProtocolMappers(ctx, client.ID) |
There was a problem hiding this comment.
[Major] GatewayClientSynced compares protocol-mapper names only. listClientProtocolMappers discards protocol, protocolMapper, and config, so same-name mappers with a wrong audience, claim, token flags, mapper type, or role target still report synced. Compare complete mapper representations with the canonical desired gateway configuration and add drift tests. Confidence: High (99%).
| @@ -141,6 +160,23 @@ func (r *SandboxAttentionReconciler) reconcileOnce(ctx context.Context) { | |||
|
|
|||
| observations := gateway.BuildAttentionObservations(pods, gateway.IndexSandboxes(sandboxes), eligible) | |||
| r.recordUsage(ctx, eligible, sandboxes, observations, crListFailed) | |||
There was a problem hiding this comment.
[Major] The new readiness denominator relies on gateway_sandbox_total, but recordUsage/exportUsage records only namespaces still in the eligible set. When a gateway namespace disappears, its cumulative cluster/namespace series is never reset and can keep deleted sandboxes in this denominator indefinitely. Emit zero for departed namespaces or export a current observable snapshot, with a removal regression test. Confidence: High (95%).
kdoberst
left a comment
kdoberst
left a comment
There was a problem hiding this comment.
The retry fixes the four findings from the previous review and completes the requested control-plane, BFF, and dashboard paths. Three Major correctness findings still make the overall assessment REQUEST_CHANGES; GitHub does not permit the PR author to submit a formal request-changes review on their own PR.
Major
- Certificate inventory does not follow the configuration-dependent Secret contract. Confidence: High (99%).
- OIDC mapper sync checks names, not desired mapper configuration. Confidence: High (99%).
- Removed gateway namespaces leave stale sandbox-total gauge series in the denominator. Confidence: High (95%).
Findings Summary (ordered by severity, highest first):
- [Major] Certificate inventory unconditionally expects all four Secrets and includes a Route outside the clarified inventory - Spec Consistency (
dashboard_health.go:173,dashboard_health.go:185) - [Major] OIDC client sync accepts same-name protocol mappers with incorrect types or config - Correctness (
client.go:1185,client.go:1324) - [Major] Sandbox total does not clear series for namespaces that leave the eligible set - Observability Correctness (
sandbox_attention.go:162,sandbox_attention.go:207)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
| Errors wrapped with context | Pass |
| No secrets in metrics, logs, or responses | Pass |
| Metric populations match the specified resource set | Fail |
| Unavailable values do not become valid-looking values | Fail |
| } | ||
|
|
||
| func (r *SandboxAttentionReconciler) listCertificateMaterials(ctx context.Context, eligible map[string]struct{}) ([][]byte, error) { | ||
| certificates, err := r.dynamicClient.Resource(certificateGVR).Namespace("").List(ctx, metav1.ListOptions{}) |
There was a problem hiding this comment.
Major — confidence: high. The expected inventory is still derived from Certificate objects that already exist. If a configured gateway's Certificate CR is missing (or was never created), its expected Secret identity never reaches selectCertificateMaterials, so the missing certificate is not counted as unavailable. This is the same blind spot the previous finding asked to close by deriving expectations from gateway configuration. Build the expected namespace/Secret set from desired gateways and their certificate mode, then join that set to Secrets; add a test where the gateway expects a certificate but both its Certificate CR and Secret are absent.
| validCounts && keycloakStatus === "connected" | ||
| ? (100 * synced) / expected | ||
| : null, | ||
| synced_clients: synced === undefined ? null : Math.round(synced), |
There was a problem hiding this comment.
Major — confidence: high. Disconnected/unknown Keycloak observations still expose the last numeric synced and expected counts. The issue clarification requires client counts and accuracy to be null whenever Keycloak is not connected; observeOIDCHealth intentionally publishes partial/expected values with connected=false, so this path can show authoritative-looking counts after a failed inspection. Gate both count fields on keycloakStatus being connected (and return null otherwise), and cover a disconnected non-zero sample in the BFF test.
| observation_count, | ||
| p50_seconds, | ||
| p95_seconds, | ||
| ...(rangeDays === 7 |
There was a problem hiding this comment.
Major — confidence: high. The 30-day response drops target_seconds, while the clarified contract says to continue returning the static 300-second target at 30 days so the UI keeps the Target row. It also requires p99_seconds: null and daily_p99: null, whereas this code omits both keys. Return the target for both supported ranges and explicit nulls for unavailable new fields (including the 30-day P99 fields), then add a range=30 response test.
| ...(validSandboxes | ||
| ? { sandbox_readiness_percent: (100 * sandboxReady) / sandboxTotal } | ||
| : {}), | ||
| ...(validPods && validSandboxes |
There was a problem hiding this comment.
Moderate — confidence: high. The clarified formula requires each unready subtraction to be clamped at zero. Here a transient ready > total sample makes validPods or validSandboxes false and removes the combined value entirely, rather than computing max(total-ready, 0) for each source. Distinguish source availability from count skew, clamp the two terms for unready_resources, and add a scrape-skew test.
kdoberst
left a comment
kdoberst
left a comment
There was a problem hiding this comment.
Amber review result: REQUEST_CHANGES.\n\nThe latest commit resolves the previous three findings: certificate identities are deduplicated and Route material is removed, protocol mapper configuration is compared, and departed sandbox namespaces are zeroed. The PR is conflict-free and GitHub reports MERGEABLE/CLEAN. All required completed checks are green, including Kind E2E, control-plane, frontend, web-console, operational-dashboard, repository-policy, security, and Konflux build checks. OpenShift E2E is skipped.\n\nFour findings still block approval:\n\n1. Major — Certificate expectations are inferred from Certificate CRs that already exist. If an expected Certificate CR and Secret are both missing, that configured certificate disappears from the inventory instead of incrementing unavailable. Derive the expected Secret identities from gateway configuration, then join to observed Secrets.\n\n2. Major — OIDC synced/expected counts remain numeric when Keycloak is disconnected or unknown. The issue contract requires both counts and accuracy to be null whenever Keycloak is not connected.\n\n3. Major — The 30-day gateway-provision response omits target_seconds, p99_seconds, and daily_p99. It must retain target_seconds=300 and return explicit null P99 fields so the UI keeps the Target row while hiding unavailable P99 content.\n\n4. Moderate — Readiness count skew is treated as source unavailability. The clarified combined formula requires each total-minus-ready term to be clamped at zero.\n\nLine-specific comments include the affected locations and fixes.
Summary
Verification
go vet ./...git diff --checkCompletefor Operational, Reliability, and Provisioning at 1440x900 and 390x844Closes #807