feat(event-ledger): add Helm chart for self-hosted stack - #762
feat(event-ledger): add Helm chart for self-hosted stack#762shelleyshen-0 wants to merge 18 commits into
Conversation
Adds deploy/helm/event-ledger, a new Helm chart for deploying event-ledger as an optional add-on in the self-managed stack. - Deploys into the nvcf namespace - ServiceAccount name matches the OpenBao JWT auth role (event-ledger) - App config mounted via ConfigMap at /etc/event-ledger/config.yaml, read via EVENT_LEDGER_CONFIG; sets deprecate-endpoints: true (v3-only), cloudevents disabled, policy provider pointed at api-keys-api - Vault Agent template renders Cassandra credentials from services/event-ledger/kv/data/cassandra/creds and a dummy policy-bearer-token (api-keys-api evaluator is open/unauthenticated) - jwk-set-url points at services/event-ledger/jwt/jwks so inbound SIS/NVCA/NVCF-API JWTs are verified against event-ledger's own engine
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds an NVCF-owned Helm chart for Event Ledger. It configures deployment, services, autoscaling, Vault/OpenBao credential injection, runtime settings, validation, and deployment notes. It also updates Cassandra and OpenBao migration image tags. ChangesEvent Ledger Helm chart
Migration image updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This PR adds Helm deployment configuration for the event-ledger service and updates related migration image references; no actionable merge-blocking risk remains at the current head after normal checks and review. Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Helm
participant Kubernetes
participant VaultAgent
participant EventLedger
Helm->>Kubernetes: Render Deployment and ConfigMaps
Kubernetes->>VaultAgent: Start sidecar with Vault annotations
VaultAgent->>VaultAgent: Render Cassandra credentials
VaultAgent->>EventLedger: Provide secrets.json
Kubernetes->>EventLedger: Mount config.yaml and expose ports
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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 `@deploy/helm/event-ledger/files/secrets.json.tmpl`:
- Around line 2-5: Update the username and password fields in the secrets
template to serialize their interpolated credential values with the Helm toJSON
function, removing the surrounding manual quotes. Leave the policy-bearer-token
value unchanged.
In `@deploy/helm/event-ledger/templates/NOTES.txt`:
- Around line 4-7: Replace the hardcoded event-ledger Service references in the
DNS record and port-forward command with the chart’s rendered Service name,
reusing the existing fullname/service naming template so fullnameOverride values
are honored consistently.
🪄 Autofix
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: Enterprise
Run ID: b73471e4-3c79-4c28-a163-af7022fffc3e
📒 Files selected for processing (14)
deploy/helm/cassandra/helm/values.yamldeploy/helm/event-ledger/Chart.yamldeploy/helm/event-ledger/files/secrets.json.tmpldeploy/helm/event-ledger/templates/NOTES.txtdeploy/helm/event-ledger/templates/_helpers.tpldeploy/helm/event-ledger/templates/configmap-app.yamldeploy/helm/event-ledger/templates/configmap-vault-agent-template.yamldeploy/helm/event-ledger/templates/deployment.yamldeploy/helm/event-ledger/templates/hpa.yamldeploy/helm/event-ledger/templates/service.yamldeploy/helm/event-ledger/templates/serviceaccount.yamldeploy/helm/event-ledger/templates/validation.yamldeploy/helm/event-ledger/values.yamldeploy/helm/openbao/helm/values.yaml
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/helm/event-ledger/templates/deployment.yaml (1)
16-113: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd automated chart coverage or document the exception.
The supplied PR material records manual cluster testing. It does not include automated chart tests or explain why tests are not applicable. Add repository-native rendering coverage for the Deployment, HPA, ServiceAccount, and validation paths. Record the native test command in the PR.
🤖 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 `@deploy/helm/event-ledger/templates/deployment.yaml` around lines 16 - 113, Add repository-native Helm chart rendering tests covering the Deployment, HPA, ServiceAccount, and validation paths for the nvcf-event-ledger chart, including assertions for the rendered resources and relevant configuration. If automated coverage cannot be added, document the exception and rationale; otherwise record the command used to run the native tests in the PR.Source: Coding guidelines
🤖 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 `@deploy/helm/event-ledger/templates/deployment.yaml`:
- Around line 30-32: Add a checksum annotation alongside checksum/config-app in
the deployment template, hashing the Vault Agent secrets.json.tmpl content so
changes to that template alter the pod template and trigger a rollout. Use the
chart’s existing template rendering mechanism and preserve the current
annotations.
- Line 24: Conditionally render spec.replicas in the deployment template only
when eventLedger.autoscaling.enabled is false, omitting it entirely when
autoscaling is enabled so the HPA controls the replica count.
---
Outside diff comments:
In `@deploy/helm/event-ledger/templates/deployment.yaml`:
- Around line 16-113: Add repository-native Helm chart rendering tests covering
the Deployment, HPA, ServiceAccount, and validation paths for the
nvcf-event-ledger chart, including assertions for the rendered resources and
relevant configuration. If automated coverage cannot be added, document the
exception and rationale; otherwise record the command used to run the native
tests in the PR.
🪄 Autofix
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: Enterprise
Run ID: 5a20e895-d9d1-4438-a3fc-37b772b89cc4
📒 Files selected for processing (14)
deploy/helm/cassandra/helm/values.yamldeploy/helm/event-ledger/Chart.yamldeploy/helm/event-ledger/files/secrets.json.tmpldeploy/helm/event-ledger/templates/NOTES.txtdeploy/helm/event-ledger/templates/_helpers.tpldeploy/helm/event-ledger/templates/configmap-app.yamldeploy/helm/event-ledger/templates/configmap-vault-agent-template.yamldeploy/helm/event-ledger/templates/deployment.yamldeploy/helm/event-ledger/templates/hpa.yamldeploy/helm/event-ledger/templates/service.yamldeploy/helm/event-ledger/templates/serviceaccount.yamldeploy/helm/event-ledger/templates/validation.yamldeploy/helm/event-ledger/values.yamldeploy/helm/openbao/helm/values.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- deploy/helm/event-ledger/files/secrets.json.tmpl
- deploy/helm/cassandra/helm/values.yaml
- deploy/helm/openbao/helm/values.yaml
- deploy/helm/event-ledger/Chart.yaml
- deploy/helm/event-ledger/templates/NOTES.txt
- deploy/helm/event-ledger/values.yaml
- deploy/helm/event-ledger/templates/_helpers.tpl
…to feat/event-ledger-helm-chart
…se toJSON for credentials
| { | ||
| "username": {{ .Data.data.username | toJSON }}, | ||
| "password": {{ .Data.data.password | toJSON }}, | ||
| "policy-bearer-token": "self-managed" |
There was a problem hiding this comment.
Is this needed currently? the api key services does not require auth.
There was a problem hiding this comment.
Hi Ada perhaps this should have been a point that i should have brought up in the MR for auth model change in event ledger before the monorepo migration. The service depend on the policy-bearer-token to determine the auth model (SSA vs static token) which is actually not correct it should be the file let me make the fix.
There was a problem hiding this comment.
I've updated it here in this PR: https://github.com/NVIDIA/nvcf/pull/885/changes
There was a problem hiding this comment.
discussed offline. recommend using config to gate it.
|
Hi for nvcf-openbao-migrations, it uses 0.18.0-test. any blocker? |
| # via the vault.hashicorp.com/auth-config-token-path annotation. | ||
| # The token's `aud` claim must match the JWT auth role's bound_audiences, | ||
| # which the migration sets to OPENBAO_SERVER_INTERNAL_URL. | ||
| volumes: |
There was a problem hiding this comment.
please refer this for SA token mount pattern.
https://gitlab-master.nvidia.com/nvcf/nvcf/-/merge_requests/532/diffs
I've updated it to 0.18.0. I think you were looking at the old version. Edit: Also update the PR description. Thanks. |
| # limitations under the License. | ||
|
|
||
| apiVersion: v2 | ||
| name: helm-event-ledger |
There was a problem hiding this comment.
repo convention is helm-nvcf-* - does this drop the nvcf portion intentionally?
|
|
||
| version: 0.0.0 # autoversioning enabled via release pipeline | ||
|
|
||
| appVersion: "1.0.0" |
There was a problem hiding this comment.
This diverges from the tag specified in values
| name: {{ include "nvcf-event-ledger.fullname" . }} | ||
| minReplicas: {{ .Values.eventLedger.autoscaling.minReplicas }} | ||
| maxReplicas: {{ .Values.eventLedger.autoscaling.maxReplicas }} | ||
| metrics: |
There was a problem hiding this comment.
This can render an empty metrics key if autoscaling is enabled and no metric target is set. This needs a guard to ensure valid output for that combination.
| path: /health | ||
| port: api-port | ||
|
|
||
| autoscaling: |
There was a problem hiding this comment.
min/max both set to 1 makes HPA ineffective. Consider alternative defaults.
|
|
||
| serviceAccount: | ||
| create: true | ||
| automount: true |
There was a problem hiding this comment.
Verify this is intended. The chart already projects a specific OpenBao-audience token in this values file. If the default SA token is not needed, set automount false.
There was a problem hiding this comment.
You are right it's not needed updating.
| podLabels: {} | ||
|
|
||
| podSecurityContext: {} | ||
| securityContext: {} |
There was a problem hiding this comment.
Aim for less permissive sec config by default.
At minimum I’d expect allowPrivilegeEscalation: false and capabilities.drop: ["ALL"]. runAsNonRoot if the image supports it. If the image does not, then file a follow up to make that possible.
|
No optional PodDisruptionBudget, NetworkPolicy, topologySpreadConstraints, or priorityClassName. If the convention is to not provide these, then follow that convention. But worth checking. |
|
The chart has a useful NOTES.txt, but no README and no values.schema.json. Nearby charts commonly have parent READMEs explaining prerequisites, image settings, Vault/OpenBao expectations, install examples, and operational notes. For this chart, docs should explicitly cover:
|
|
The current image helper does not support digest pinning. A more complete image helper would support:
and render one of:
Image tags are mutable by nature, digest pinning is preferable to mitigate potential supply chain issues. |
|
The render shape of the chart looks good as-is, the comments here will tighten it up considerably. |
Add chart-owned OpenBao token helpers and move the projected service-account token volume/mount out of public values lists into the workload template directly. - Add openbaoTokenVolumeName/MountPath/FileName/Path helpers - Add agent-service-account-token-volume-name annotation - Derive auth-config-token-path from helpers instead of a hardcoded literal - Render the openbao-token projected volume and mount in deployment.yaml - Rename volumes/volumeMounts extension points to extraVolumes/extraVolumeMounts - Move audience from inline volume definition to vault.audience value
…ca guard - Rename chart from helm-event-ledger to helm-nvcf-event-ledger to follow repo convention - Set appVersion to 0.10.2 to match the image tag in values.yaml - Omit spec.replicas when autoscaling is enabled so the HPA controls replica count
- Guard HPA metrics key so it is omitted when no metric target is configured - Set automount: false to disable unused default SA token (chart uses projected OpenBao token) - Set maxReplicas to 3 so HPA can actually scale when enabled - Add EVENT_LEDGER_SELF_MANAGED=true env var
Set allowPrivilegeEscalation: false, runAsNonRoot: true, and drop all Linux capabilities. The image runs as UID 65532 so runAsNonRoot is safe.
Not seeing digest pinning in the other repos. Is this something we are looking to add support for? If so can we do it in a followup? |
3 participants
Closes #168
Summary
deploy/helm/event-ledgerHelm chart for deploying the event-ledger service in the self-hosted NVCF stackcassandra.cassandra-system.svc.cluster.local)nvcf-cassandra-migrationsto0.16.0andnvcf-openbao-migrationsto0.18.0Test plan
2/2 Runningwith Cassandra connected and API server started on:808022_setup_event-ledgerran successfully via0.18.0-testimageevent_ledgerkeyspace created via0.16.0migrations imagenvcf-openbao-migrationstag once official0.18.0image is releasedvalues.yamlandappVersioninChart.yaml