Repository navigation
fix(postgres-operator): block HelmRelease Ready until the cnpg webhook actually serves - #2482
Conversation
…k actually serves The cnpg admission webhook's controller pod passes readinessProbe as soon as the local HTTPS server binds to :9443. The HelmRelease marks itself Ready right after that via helm install --wait. But the Service cnpg-webhook-service needs its EndpointSlice populated and the data plane (kube-proxy / cilium) programmed before kube-apiserver can reach the webhook through the Service ClusterIP. That gap is short but not zero, and any HelmRelease that depends on postgres-operator (cozy-keycloak, tenant Postgres apps) can fire its own install inside the window and hit Internal error occurred: failed calling webhook "mcluster.cnpg.io": failed to call webhook: Post "https://cnpg-webhook-service.cozy-postgres-operator.svc:443/...": dial tcp <svc-ip>:443: connect: connection refused which fails the install of the downstream release. Seen on #2470 E2E run 24862782568. Add a post-install,post-upgrade Helm hook Job that blocks the release from reporting Ready until the webhook answers /readyz through the apiserver service proxy. Apiserver proxy routes the call over the same Service IP → EndpointSlice → pod path the admission webhook uses, so once it responds, the webhook admission path is also working. RBAC is minimal: a dedicated ServiceAccount with a ClusterRole that only grants get on services/proxy scoped to https:cnpg-webhook-service:webhook-server. The Job times out after 120s with 60 attempts at 2s intervals — longer than any data-plane programming delay seen on E2E, but bounded. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a Helm-chart readiness gating feature for the CNPG admission webhook: a new Helm template that creates RBAC and a post-install/post-upgrade Job to poll the webhook via the Service proxy, new configurable values, a comprehensive helm-unittest suite, and a Makefile Changes
Sequence Diagram(s)sequenceDiagram
participant Helm as Helm Installer
participant K8s as Kubernetes API
participant RBAC as RBAC Controller
participant JobCtrl as Job Controller
participant Webhook as CNPG Webhook Service
Helm->>K8s: Install/Upgrade → render & apply hook manifests
K8s->>RBAC: Create ServiceAccount, Role, RoleBinding
RBAC->>K8s: RBAC objects available
K8s->>JobCtrl: Create readiness Job
JobCtrl->>Webhook: kubectl GET /api/v1/namespaces/<ns>/services/<svc>/proxy/readyz (loop)
alt Webhook ready
Webhook-->>JobCtrl: 200 OK
JobCtrl->>K8s: Job completes successfully
else Not ready / retry
Webhook-->>JobCtrl: error / non-200
JobCtrl->>JobCtrl: sleep/backoff and retry until maxAttempts or activeDeadlineSeconds
JobCtrl->>K8s: Job fails with last error after timeout
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dosubot
Bot
added
the
size/XS
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust readiness gate for the postgres-operator Helm chart to resolve a bootstrap race condition. By executing a post-install Job that verifies the CNPG admission webhook's availability through the Kubernetes API server proxy, it ensures that dependent resources are not deployed until the webhook is fully functional. This prevents intermittent 'connection refused' errors that occur when Helm reports a release as ready before the webhook's data plane is fully programmed. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a post-install readiness gate for the Postgres operator using a Helm hook. This ensures the admission webhook is reachable via the apiserver proxy before the HelmRelease reports readiness, preventing bootstrap race conditions. I have reviewed the implementation and provided feedback regarding the use of namespaced RBAC resources and the inclusion of resource requests and limits for the readiness Job.
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: ClusterRole | ||
| metadata: | ||
| name: {{ .Release.Name }}-webhook-ready | ||
| annotations: | ||
| helm.sh/hook: post-install,post-upgrade | ||
| helm.sh/hook-weight: "0" | ||
| helm.sh/hook-delete-policy: before-hook-creation,hook-succeeded | ||
| rules: | ||
| - apiGroups: [""] | ||
| resources: ["services/proxy"] | ||
| resourceNames: [{{ $resourceName | quote }}] | ||
| verbs: ["get"] | ||
| --- | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: ClusterRoleBinding | ||
| metadata: | ||
| name: {{ .Release.Name }}-webhook-ready | ||
| annotations: | ||
| helm.sh/hook: post-install,post-upgrade | ||
| helm.sh/hook-weight: "0" | ||
| helm.sh/hook-delete-policy: before-hook-creation,hook-succeeded | ||
| roleRef: | ||
| apiGroup: rbac.authorization.k8s.io | ||
| kind: ClusterRole | ||
| name: {{ .Release.Name }}-webhook-ready | ||
| subjects: | ||
| - kind: ServiceAccount | ||
| name: {{ .Release.Name }}-webhook-ready | ||
| namespace: {{ .Release.Namespace }} |
There was a problem hiding this comment.
The RBAC resources for checking the service proxy are currently defined as a ClusterRole and ClusterRoleBinding. Since the services/proxy subresource is namespaced and the Job only needs to access the service within its own namespace, it is recommended to use a Role and RoleBinding instead. This adheres to the principle of least privilege and avoids creating global resources for a namespaced task. Note that if you apply this change, you should also update the corresponding unit tests in tests/webhook-ready-hook_test.yaml that check for the ClusterRole kind.
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: {{ .Release.Name }}-webhook-ready
namespace: {{ .Release.Namespace }}
annotations:
helm.sh/hook: post-install,post-upgrade
helm.sh/hook-weight: "0"
helm.sh/hook-delete-policy: before-hook-creation,hook-succeeded
rules:
- apiGroups: [""]
resources: ["services/proxy"]
resourceNames: [{{ $resourceName | quote }}]
verbs: ["get"]
---
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
name: {{ .Release.Name }}-webhook-ready
namespace: {{ .Release.Namespace }}
annotations:
helm.sh/hook: post-install,post-upgrade
helm.sh/hook-weight: "0"
helm.sh/hook-delete-policy: before-hook-creation,hook-succeeded
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: Role
name: {{ .Release.Name }}-webhook-ready
subjects:
- kind: ServiceAccount
name: {{ .Release.Name }}-webhook-ready
namespace: {{ .Release.Namespace }}There was a problem hiding this comment.
Good catch — switched the RBAC to a namespaced Role and RoleBinding in .Release.Namespace since the Job only needs services/proxy on the webhook Service, which lives in the release namespace. Updated the kind assertions in tests/webhook-ready-hook_test.yaml so helm-unittest still guards the shape. Fixed in 648ad82.
| securityContext: | ||
| allowPrivilegeEscalation: false | ||
| readOnlyRootFilesystem: true | ||
| capabilities: | ||
| drop: ["ALL"] |
There was a problem hiding this comment.
The wait container in the readiness Job is missing resource requests and limits. It is recommended to define them to ensure predictable scheduling and resource management, as per the project guidelines (line 99).
resources:
requests:
cpu: 10m
memory: 32Mi
limits:
cpu: 100m
memory: 64Mi
securityContext:
allowPrivilegeEscalation: false
readOnlyRootFilesystem: true
capabilities:
drop: ["ALL"]References
- Missing resource requests/limits on new workloads should be flagged. (link)
There was a problem hiding this comment.
Added requests (10m CPU, 32Mi memory) and limits (100m CPU, 64Mi memory) on the wait container — matches the footprint of the kubectl polling loop and moves the pod out of BestEffort so namespace quotas apply. Added a matching unittest assertion to keep the values in sync with the template. Fixed in fc057c0.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/system/postgres-operator/templates/webhook-ready-hook.yaml (1)
127-137: Nit: off-by-one in the timeout message.The first kubectl call is the
untiltest itself (beforeiis incremented), so by the timei > max_attemptsfires you've mademax_attempts + 1actual calls. The message reads"after ${max_attempts} attempts", i.e. 60 when 61 were made with defaults. Purely cosmetic — not a bug.♻️ One-line fix
- until last_err=$(kubectl get --raw "$proxy" 2>&1 >/dev/null); do - i=$((i + 1)) - if [ $i -gt $max_attempts ]; then + until last_err=$(kubectl get --raw "$proxy" 2>&1 >/dev/null); do + i=$((i + 1)) + if [ $i -ge $max_attempts ]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/postgres-operator/templates/webhook-ready-hook.yaml` around lines 127 - 137, The timeout message incorrectly reports ${max_attempts} attempts even though the loop makes one extra kubectl call; update the echo in the failure branch inside the until loop (where variables i, max_attempts, last_err, proxy, sleep_seconds are used) to report the actual number of attempts (use the current i value or compute max_attempts+1) and adjust the parenthetical sleep_seconds wording accordingly so the message accurately reflects how many kubectl calls were made and the per-attempt sleep duration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/system/postgres-operator/values.yaml`:
- Around line 25-30: The comment describing the interplay of backoffLimit and
activeDeadlineSeconds is inaccurate for the current defaults: update the comment
that references activeDeadlineSeconds/backoffLimit so it either uses numbers
that match the defaults (activeDeadlineSeconds = 60*2+60 = 180s) or explicitly
qualifies the example as requiring a larger maxAttempts/sleepSeconds override;
specifically edit the explanatory text around activeDeadlineSeconds,
backoffLimit, maxAttempts and sleepSeconds to either adjust the math to a 180s
total or add a parenthetical like "with a generous maxAttempts/sleepSeconds
override" so the scenario no longer conflicts with the default values.
---
Nitpick comments:
In `@packages/system/postgres-operator/templates/webhook-ready-hook.yaml`:
- Around line 127-137: The timeout message incorrectly reports ${max_attempts}
attempts even though the loop makes one extra kubectl call; update the echo in
the failure branch inside the until loop (where variables i, max_attempts,
last_err, proxy, sleep_seconds are used) to report the actual number of attempts
(use the current i value or compute max_attempts+1) and adjust the parenthetical
sleep_seconds wording accordingly so the message accurately reflects how many
kubectl calls were made and the per-attempt sleep duration.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cae48463-bdd5-4fba-bf13-dd9ec693a397
📒 Files selected for processing (4)
packages/system/postgres-operator/Makefilepackages/system/postgres-operator/templates/webhook-ready-hook.yamlpackages/system/postgres-operator/tests/webhook-ready-hook_test.yamlpackages/system/postgres-operator/values.yaml
…space Address review feedback from gemini-code-assist on packages/system/postgres-operator/templates/webhook-ready-hook.yaml:83. The Job targets the cnpg-webhook-service/services/proxy subresource, which is namespaced and lives in the release namespace. A namespaced Role and RoleBinding grant the exact permission needed without creating global RBAC for a namespaced probe, which is the principle of least privilege. Also update the kind assertions in tests/webhook-ready-hook_test.yaml so the unittest suite tracks the new Role/RoleBinding objects. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Address review feedback from gemini-code-assist on packages/system/postgres-operator/templates/webhook-ready-hook.yaml:115. The wait container had no resource requests or limits, so schedulers treated it as BestEffort and downstream quota enforcement had no signal. Set small requests (10m CPU, 32Mi memory) and conservative limits (100m CPU, 64Mi memory) matching the actual footprint of the kubectl polling loop. Add a matching unittest assertion so the values stay in sync if anyone touches the template. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
…nt math Address review feedback from coderabbitai on packages/system/postgres-operator/values.yaml:30. The old comment's 5-minute ImagePullBackOff scenario conflicted with the 180s activeDeadlineSeconds that the default maxAttempts/sleepSeconds resolve to, so the numbers could not both be taken at face value. Rewrite the comment to state the actual deadline math and frame the two gates as an AND with activeDeadlineSeconds being the shorter one under defaults, so readers understand why backoffLimit has little headroom without an accompanying maxAttempts/sleepSeconds bump. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
2 participants
What this PR does
Closes a bootstrap race where the
cnpg-webhook-serviceService gets its EndpointSlice populated and the data plane (kube-proxy / Cilium) programmed a second or two afterhelm install --waitoncozy-postgres-operatordeclares the HelmRelease Ready. Any HelmRelease thatdependsOn: postgres-operatorand creates apostgresql.cnpg.io/v1resource in that window (cozy-keycloak, tenant Postgres apps) has kube-apiserver's call tomcluster.cnpg.iofail withwhich fails the downstream release's install.
The fix is a post-install / post-upgrade Helm hook (ServiceAccount + ClusterRole + ClusterRoleBinding + Job) that probes
/readyzon the webhook through the apiserver service proxy. Apiserver's service proxy uses the same endpoint-resolution and apiserver-initiated pod dial as the admission webhook path, so once/readyzanswers through the proxy, the subsequent admission call will also succeed. Helm--waitblocks the install from completing until the Job exits 0, so the HelmRelease Ready condition does not lie anymore.Hardened per review:
services/proxyresourceName (https:cnpg-webhook-service:webhook-server) — the exact string the apiserver URL path parser expects for the Service proxy subresource.metadata.name/ports[0].namediverge from the literals in the hook, so a futuremake updatethat renames the service forces this template to be updated in the same change.clastix/kubectl:v1.32@sha256:…) with arenovate:annotation;imagePullPolicy: IfNotPresentwhen digest-pinned,Alwayswhen tag-only.activeDeadlineSecondsis derived frommaxAttempts × sleepSeconds + 60sso a values override raising retries does not get silently cut by a fixed deadline.backoffLimit: 2(configurable) so a transient pod-level failure (image pull rate limit, OOM, CNI hiccup) does not fail the whole HelmRelease.kubectl get --rawstderr so the operator can distinguish DNS / refused / 401 / TLS from the Job logs.21 helm-unittest assertions cover ordering, RBAC/URL parity, subchart drift, image policy, retry-loop bounds, securityContext, and the deadline-scaling invariant. Wired via
make testinpackages/system/postgres-operator/Makefile.First surfaced on #2470 E2E run 24862782568 where
cozy-keycloak/keycloakfailed with the exact signature above. Not tied to that PR's branch — this is independently applicable tomain.Release note
Summary by CodeRabbit
New Features
Tests
Chores