helm: add Kubeflow Dashboard chart - #3524
Conversation
|
Welcome to the Kubeflow Community Distribution Repository Thanks for opening your first PR. Your contribution means a lot to the Kubeflow community. Before making more PRs: Community Resources:
Thanks again for helping to improve Kubeflow. |
There was a problem hiding this comment.
Pull request overview
Adds a co-located Helm wrapper chart for the Kubeflow Dashboard application and wires it into the existing Helm↔Kustomize render-parity validation and synchronization flow.
Changes:
- Introduce
applications/dashboard/helmas a static, scenario-gated Helm wrapper chart rendering the current Dashboardoverlays/istio(“platform”) install. - Extend Helm/Kustomize comparison scripts to include the new
kubeflow-dashboardcomponent and scenario. - Update the Dashboard synchronization script to also refresh the Helm chart
appVersionand regenerated staticplatform.yamltemplate; update the comparison workflow to install Helm 4.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/helm_kustomize_compare.sh |
Adds kubeflow-dashboard component wiring for parity comparison. |
tests/helm_kustomize_compare.py |
Allows kubeflow-dashboard as a supported component in the comparison tool. |
tests/helm_kustomize_compare_all.sh |
Adds kubeflow-dashboard to the “all” comparison matrix. |
scripts/synchronize-dashboard-manifests.sh |
Extends upstream sync automation to also refresh Helm chart metadata and static template output. |
applications/dashboard/helm/values.yaml |
Defines default chart values (scenario selection). |
applications/dashboard/helm/templates/validate.yaml |
Adds scenario validation to fail fast on unsupported values. |
applications/dashboard/helm/templates/platform.yaml |
Provides the static Kustomize-parity rendered payload for the “platform” scenario. |
applications/dashboard/helm/README.md |
Documents install shape, caveats, and parity validation commands. |
applications/dashboard/helm/ci/values-platform.yaml |
CI fixture values for the “platform” scenario. |
applications/dashboard/helm/Chart.yaml |
Introduces the Dashboard wrapper chart metadata. |
.github/workflows/helm-kustomize-comparison.yml |
Triggers parity CI for Dashboard changes and installs Helm v4.2.2 via setup action. |
c3c85e9 to
bd02c7a
Compare
6f08852 to
f193623
Compare
juliusvonkohout
left a comment
There was a problem hiding this comment.
You are missing the synchronization script changes
|
The synchronization script now updates |
a76d633 to
d4a53a6
Compare
Render the three Deployments and the four ConfigMaps from hand-written templates so the values the Kustomize baseline already declares - the image transformers and the configMapGenerator inputs - can be set through values.yaml. The remaining 32 resources stay vendored and are read with .Files.Get, which Helm does not evaluate as a template. Replace the component classifier in the generator with an explicit list of hand-written resources and a split on CustomResourceDefinition, collapsing five payloads into two. The generator now fails when a hand-written resource is no longer rendered by Kustomize, which means its template has been orphaned. Reject installation outside the kubeflow namespace. Every resource declares that namespace, so a release installed elsewhere would record its metadata in one namespace while modifying another. Stable ConfigMap names replace the Kustomize content-hash suffix, so the consuming Deployments carry a checksum annotation to keep the rollout behaviour. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Ignore the Dashboard pod-template checksum annotations, which have no Kustomize counterpart because Kustomize triggers the same rollout through a content-hashed ConfigMap name. Stop stripping the hash suffix twice. normalize_manifest already removes it, so repeating the substitution in get_resource_key also removed a legitimate final name segment of exactly ten lowercase alphanumeric characters, and did so on one side only. dashboard-parameters is such a name. The Dashboard chart uses stable names, so its Helm output is no longer normalized, matching oauth2-proxy. Correct the Dashboard comparison namespace, which named kubeflow-system while every resource declares kubeflow. Print the differing fields when VERBOSE is set. The flag was accepted and never read, so a failure reported only a count. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
tests/kustomize_install.sh exports PATH only inside its own process, so the pinned Kustomize version never reached the comparison steps and the runner image version was used instead. Persist the directory through GITHUB_PATH in every job that installs Kustomize, as the Istio job already did. Run the Dashboard chart and generator tests in the enforcing job, and align the Dashboard job with the actions/setup-python version the other jobs already use. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
The selector is a prefix match over a literal dictionary, and the route this asserted is already exercised end to end by upstream_synchronization_idempotence_test. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Describe every value, the required namespace, the rollout behaviour, and why the custom resource definitions are rendered as templates with a retention policy rather than placed in crds/, where Helm would never upgrade them. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
924182a to
18f1528
Compare
Move the component-independent work - Kubernetes identity validation, duplicate detection, custom resource definition retention, document extraction, deterministic rendering and atomic replacement - into scripts/helm_manifest_generator.py. The Dashboard generator becomes a configuration object naming its Kustomize path, hand-written resources and extracted documents. Preserve the generated directory mode during atomic replacement. tempfile.mkdtemp creates the staging directory as 0700 and os.replace moved it into place mode and all, leaving the payloads unreadable to any other user or to a containerized packaging step. Git does not record directory modes, so a fresh checkout hid it. The Dashboard payloads regenerate byte-identical after the extraction. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
The three document values are strings containing a whole document. The helper accepted any truthy value and rendered it with Go's own formatting, so centralDashboard.links given a map produced the literal text "map[menuLinks:[]]", which Helm accepted and the application cannot parse. An empty map or a false value was treated as unset and silently replaced by the upstream default. Reject anything that is not a string, and decide "unset" from the value being an empty string rather than from truthiness. Add customResourceDefinitions.enabled. The two definitions are ordinary templates so that Helm can upgrade them, which also means --skip-crds does not apply to them. There was no supported way to install into a cluster where an administrator already owns them. The default keeps Kustomize parity. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
This restores paths removed in an earlier commit of this pull request, which argued the script never writes them. That missed the case that matters: an upstream change to one of the hand-written resources makes the parity comparison fail until a maintainer edits the template, and re-running synchronization then committed everything except that edit while reporting success. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Map scripts/helm_manifest_generator.py to the Dashboard synchronization script so a change to the engine regenerates the payloads it produces. Restore selector coverage as one table-driven test over the routes rather than the single-assertion file removed earlier. Deleting a route otherwise makes the idempotence job run no synchronization script for that path, so drift is never detected and nothing fails. Document the value contract, the custom resource definition lifecycle, and that a clean regeneration proves determinism rather than correctness. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Verified on a live single-node cluster with Helm 4.2.2. A plain helm upgrade stops with a server-side apply conflict, because the RBAC aggregation controller owns .rules on the two aggregated cluster roles and cert-manager's CA injector owns the webhook caBundle. Passing --force-conflicts succeeds and both controllers restore their fields immediately. Also note that an upgrade which fails on a conflict has already applied the objects processed before the failure. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
The test asserted that the routing table in tests/resynchronize-upstream-changes.py contains the rows this pull request added. That mirrors configuration rather than checking behaviour: it fails only when someone edits the table, and the fix is to edit the test. Four of the six routes are covered anyway. A stale dashboard payload fails the Helm and Kustomize comparison whether or not the idempotence job re-ran the synchronization script. The routing rows themselves are unchanged. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
The generated payloads are output, not source, but the README said so only in passing. State the pipeline, the three mechanisms that keep it honest - idempotent regeneration, parity as the drift detector, and declared exclusions that fail loudly - and what the manual surface actually is at a future release. Move the regeneration instructions out of the custom resource definition section, where they were unrelated to the surrounding text, and record that the login flow belongs to the dex and oauth2-proxy charts rather than this one. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
|
@juliusvonkohout ready for another look. On maintenance. The generated payloads are output, not source. Three mechanisms keep that honest, now written up in the chart README under How this chart is kept up to date: regeneration is idempotent and CI fails if the tree moves; parity compares the committed chart against a fresh On configurability. The full table is in the README. Every value maps to something the Kustomize baseline already declares, and every default equals what the overlay renders. On the login flow. That one is not this chart — it belongs to Also since the last review: the five-payload classifier is gone, the routing test you flagged as a duplication is removed, and the description has been rewritten because it still described the old design. One thing I found but did not change here, since it is out of scope for this pull request: parity runs twice per CI run for this component, once inside |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated no new comments.
Suppressed comments (1)
scripts/helm_manifest_generator.py:293
- Files created by
write_textretain the process umask. With a restrictive umask such as077, each generated payload has mode0600; the later permission adjustment covers directories only, so another user or container still cannot package the chart despite this function's stated guarantee. Set an explicit readable mode on every generated file after writing it.
destination.write_text(contents, encoding="utf-8", newline="\n")
Signed-off-by: Julius von Kohout <45896133+juliusvonkohout@users.noreply.github.com>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: juliusvonkohout 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 |
Summary of Changes
Adds the Kubeflow Dashboard Helm chart under
applications/dashboard/helm,co-located with the Kustomize component it wraps.
Kustomize stays the source of truth. The synchronization script builds the
platform overlay once and writes deterministic payloads under
manifests/, whichsmall templates load with
.Files.Get. Helm does not evaluate that content, soGo template delimiters inside upstream manifests survive verbatim.
Scope is one scenario,
platform, mapping toapplications/dashboard/overlays/istio.How it works
flowchart TD A["applications/dashboard/overlays/istio"] --> B["one kustomize build"] B --> C["scripts/generate-dashboard-helm-manifests.py"] C --> D["validate every resource<br/>apiVersion, kind, name, no duplicates"] D --> E["hand-written? exclude it"] D --> F["CustomResourceDefinition?<br/>platform-crds.yaml + resource-policy keep"] D --> G["everything else<br/>platform-resources.yaml"] E --> H["templates/*.yaml<br/>real Helm templates with values"] F --> I["templates/platform.yaml<br/>.Files.Get, not evaluated"] G --> I H --> J["rendered output equals kustomize build"] I --> JTwo generated payloads, three extracted documents, and seven resources rendered
from hand-written templates so the values Kustomize already declares can be set
through
values.yaml. Everything else is vendored verbatim.Configuration
Every value maps to something the Kustomize baseline already declares — an
images:transformer or aconfigMapGeneratorinput — and every default equalswhat
applications/dashboard/overlays/istiorenders. The full table is inthe chart README: image
registry and per-component repositories, the identity headers, the registration
flow and metrics toggles, the Profile Controller administrator and workload
identity, and three whole documents (
links,settings,namespaceLabels) thatinherit upstream byte for byte unless replaced.
Authentication is deliberately not configured here. The login flow belongs to the
dexandoauth2-proxycharts; this chart only consumes the identity they placein
identity.userIdHeader. Replacing that login flow with a real identityprovider is #3568.
Keeping it up to date
The payloads are output, not source, and three mechanisms keep that honest —
described in
How this chart is kept up to date:
script and fails if the tree changes, so a hand-edited payload is caught;
against a fresh
kustomize build, so an upstream change the chart does notfollow turns the build red;
in the generator, which refuses to run if one stops being produced by Kustomize
rather than silently dropping it.
At release n + 10 the payloads are whatever
kustomize buildproduced then, andthe only manual surface is those seven templates, each compared against Kustomize
on every run.
Changed since the last review
The five-payload component classifier was removed. Resources are classified
only as custom resource definitions or everything else, so the hand-written
surface no longer grows with upstream.
scripts/helm_manifest_generator.pywas extracted as the shared engine,reused by helm: add Kubeflow Notebooks chart #3525.
The three embedded documents are extracted verbatim into
manifests/documents/so a hand-written template can inherit them exactly while still allowing an
override.
The synchronization script updates
Chart.yamland regenerates the payloadsthrough the shared atomic writer, which addresses the earlier
"You are missing the synchronization script changes".
Custom resource definitions ship in
templates/withhelm.sh/resource-policy: keeprather than incrds/, because Helm neverupgrades anything in
crds/. The lifecycle table is in the README.The routing test has been dropped. It asserted that the routing table
contains the rows this pull request adds, which mirrors configuration rather
than checking behaviour. The routing rows themselves are unchanged.
For the record, this does leave one gap. If a route were deleted, the
idempotence job would run no synchronization script and pass. For the chart and
payload routes the Helm/Kustomize comparison still fails, because the committed
payload would no longer match a fresh
kustomize build. For the twogenerator-script routes it would not, since a change that only alters output
formatting leaves the comparison green. Happy to add a narrow guard for those
two if you would rather have it, though it belongs with the shared tooling
rather than in this chart's pull request.
Kustomize is now on
PATHfor the later steps in all four jobs that install it.Dependencies
Runtime installation order follows the foundation and wrapper charts: #3468,
#3470, #3478, #3479, #3480. Render parity for this pull request is self-contained.
#3567 touches
tests/helm_kustomize_compare.pyandtests/helm_kustomize_compare.sh. Whichever merges second needs a rebase.Related Issues
Part of the Project 5 Helm chart work.
Validation
python3 -m unittest tests/test_helm_manifest_generator.py python3 -m unittest tests/test_generate_dashboard_helm_manifests.py python3 tests/test_dashboard_helm_chart.py ./tests/helm_kustomize_compare_all.sh kubeflow-dashboard ./tests/helm_kustomize_compare_all.sh helm lint applications/dashboard/helm --namespace kubeflow KUBEFLOW_SYNCHRONIZE_NO_COMMIT=true ./scripts/synchronize-dashboard-manifests.sh git status --porcelain # empty, regeneration is idempotent git diff --checkHelm 4.2.2 in the workflows, verified locally against Helm 4.1.0 and
Kustomize v5.8.1.
Contributor Checklist