helm: add Istio wrapper chart#3478
Conversation
|
Welcome to the Kubeflow Manifests 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
Note
Copilot was unable to run its full agentic suite in this review.
This PR adds an Istio “wrapper” Helm chart (static manifests rendered via Files.Get) and extends the existing Helm-vs-Kustomize comparison tests to cover Istio scenarios.
Changes:
- Add
istioas a supported component in the helm/kustomize comparison scripts (including “all” runner + help text). - Add an experimental
experimental/helm/charts/istiochart with static, Kustomize-generated manifests and CI values presets. - Update manifest comparison keying to include namespaces for Istio resources with repeated names.
Reviewed changes
Copilot reviewed 24 out of 27 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/helm_kustomize_compare_all.sh | Adds istio scenarios to the “compare all” test runner and help text. |
| tests/helm_kustomize_compare.sh | Adds Istio component mapping and supports comparing multiple Kustomize build roots per scenario. |
| tests/helm_kustomize_compare.py | Adjusts resource keying and CLI validation to support istio. |
| experimental/helm/charts/istio/Chart.yaml | Introduces the Istio wrapper Helm chart metadata. |
| experimental/helm/charts/istio/README.md | Documents how to install and how CI values map to Kustomize roots. |
| experimental/helm/charts/istio/values.yaml | Defines default values and feature toggles for which static manifests to render. |
| experimental/helm/charts/istio/templates/_helpers.tpl | Implements kubeflow-istio.renderFile for static-manifest rendering with string substitutions. |
| experimental/helm/charts/istio/templates/crds.yaml | Conditionally renders the CRD manifest bundle. |
| experimental/helm/charts/istio/templates/namespace.yaml | Conditionally renders the Istio namespace manifest. |
| experimental/helm/charts/istio/templates/networkpolicies.yaml | Conditionally renders the Istio namespace NetworkPolicies bundle. |
| experimental/helm/charts/istio/templates/install-base.yaml | Conditionally renders the base Istio install bundle. |
| experimental/helm/charts/istio/templates/install-oauth2-proxy.yaml | Conditionally renders the oauth2-proxy overlay install bundle. |
| experimental/helm/charts/istio/templates/cluster-local-gateway.yaml | Conditionally renders the cluster-local-gateway bundle. |
| experimental/helm/charts/istio/templates/kubeflow-istio-resources.yaml | Conditionally renders the Kubeflow-specific Istio resources bundle. |
| experimental/helm/charts/istio/manifests/namespace.yaml | Adds static namespace manifest derived from Kustomize output. |
| experimental/helm/charts/istio/manifests/networkpolicies.yaml | Adds static network policies derived from Kustomize output. |
| experimental/helm/charts/istio/manifests/crds.yaml | Adds static CRDs bundle derived from Kustomize output. |
| experimental/helm/charts/istio/manifests/install-base.yaml | Adds large static base install bundle derived from Kustomize output. |
| experimental/helm/charts/istio/manifests/install-oauth2-proxy.yaml | Adds static oauth2-proxy overlay install bundle derived from Kustomize output. |
| experimental/helm/charts/istio/manifests/cluster-local-gateway.yaml | Adds static cluster-local-gateway bundle derived from Kustomize output. |
| experimental/helm/charts/istio/manifests/kubeflow-istio-resources.yaml | Adds static Kubeflow-specific Istio resources bundle derived from Kustomize output. |
| experimental/helm/charts/istio/ci/values-crds.yaml | Adds CI preset values for the CRDs-only step. |
| experimental/helm/charts/istio/ci/values-base.yaml | Adds CI preset values for base (no oauth2-proxy) parity. |
| experimental/helm/charts/istio/ci/values-oauth2-proxy.yaml | Adds CI preset values for oauth2-proxy overlay parity. |
| experimental/helm/charts/istio/ci/values-cluster-local-gateway.yaml | Adds CI preset values for cluster-local-gateway parity. |
| experimental/helm/charts/istio/ci/values-kubeflow-istio-resources.yaml | Adds CI preset values for Kubeflow Istio resources parity. |
| experimental/helm/charts/istio/ci/values-platform-full.yaml | Adds CI preset values for the full managed Istio slice. |
| for path in $KUSTOMIZE_PATH; do | ||
| kustomize build "$path" >> "$KUSTOMIZE_OUTPUT" | ||
| printf "\n---\n" >> "$KUSTOMIZE_OUTPUT" |
There was a problem hiding this comment.
Fixed by writing YAML separators only between rendered Kustomize roots.
| for path in $KUSTOMIZE_PATH; do | ||
| if [ ! -d "$path" ]; then | ||
| echo "ERROR: Kustomize path does not exist: $path" | ||
| exit 1 | ||
| fi | ||
| done |
There was a problem hiding this comment.
Fixed by storing Kustomize roots as newline-delimited values and iterating over a quoted array.
| {{- $content = replace "- kubeflow/*" (printf "- %s/*" ($root.Values.global.kubeflowNamespace | toString)) $content -}} | ||
| {{- if .oauth2 -}} | ||
| {{- $content = replace "service: oauth2-proxy.oauth2-proxy.svc.cluster.local" (printf "service: %s" ($root.Values.oauth2Proxy.service | toString)) $content -}} | ||
| {{- $content = replace " port: 80\n name: oauth2-proxy" (printf " port: %d\n name: oauth2-proxy" (int $root.Values.oauth2Proxy.port)) $content -}} |
There was a problem hiding this comment.
Fixed by replacing the exact multiline port match with a whitespace-tolerant regex replacement.
dbbba3a to
8c0de67
Compare
| {{- if .Values.crds.enabled }} | ||
| {{ include "kubeflow-istio.renderFile" (dict "root" . "path" "manifests/crds.yaml") }} | ||
| {{- end }} |
There was a problem hiding this comment.
Keeping CRDs gated in this chart for now to preserve the documented two-step install; a separate CRD chart can be evaluated as a follow-up if maintainers prefer that lifecycle.
| {{- $content := $root.Files.Get .path -}} | ||
| {{- $content = replace "istio-system" ($root.Values.global.istioNamespace | toString) $content -}} | ||
| {{- $content = replace "kube-system" ($root.Values.global.kubeSystemNamespace | toString) $content -}} | ||
| {{- $content = replace "namespace: kubeflow\n" (printf "namespace: %s\n" ($root.Values.global.kubeflowNamespace | toString)) $content -}} | ||
| {{- $content = replace "- kubeflow/*" (printf "- %s/*" ($root.Values.global.kubeflowNamespace | toString)) $content -}} |
There was a problem hiding this comment.
Leaving the helper structure as-is for this slice; substitutions are limited and covered by Helm/Kustomize comparison tests.
| {{- $content = replace "istio-system" ($root.Values.global.istioNamespace | toString) $content -}} | ||
| {{- $content = replace "kube-system" ($root.Values.global.kubeSystemNamespace | toString) $content -}} | ||
| {{- $content = replace "namespace: kubeflow\n" (printf "namespace: %s\n" ($root.Values.global.kubeflowNamespace | toString)) $content -}} |
There was a problem hiding this comment.
Added DNS-1123 validation for the namespace values before rendering.
| cluster-local-gateway: | ||
| # -- Render the Kubeflow cluster-local gateway resources. | ||
| enabled: false |
There was a problem hiding this comment.
Renamed the values key to clusterLocalGateway and updated the CI presets/templates.
| apiVersion: v1 | ||
| kind: ServiceAccount | ||
| metadata: |
There was a problem hiding this comment.
Added provenance headers to the generated Istio install manifests and kept regeneration commands in the chart README.
|
What about the GKE flavor? That is something that has to be confifurable. Think twice about which values we really need. |
b364b03 to
9984842
Compare
|
Short install/lifecycle note for this Istio chart: Istio has both CRDs and Istio custom resources such as Recommended install shape: helm install istio ./experimental/helm/charts/istio \
--namespace kubeflow-system \
--values ./experimental/helm/charts/istio/ci/values-crds.yaml \
--wait
helm upgrade istio ./experimental/helm/charts/istio \
--namespace kubeflow-system \
--values ./experimental/helm/charts/istio/ci/values-gke.yaml \
--waitThe second step can use one of the supported install profiles:
Already covered in this PR:
Remaining Kustomize variants not covered yet:
|
Added GKE flavor comment with |
| {{- $content = replace "istio-system" ($root.Values.global.istioNamespace | toString) $content -}} | ||
| {{- $content = replace "kube-system" ($root.Values.global.kubeSystemNamespace | toString) $content -}} | ||
| {{- $content = replace "namespace: kubeflow\n" (printf "namespace: %s\n" ($root.Values.global.kubeflowNamespace | toString)) $content -}} | ||
| {{- $content = replace "- kubeflow/*" (printf "- %s/*" ($root.Values.global.kubeflowNamespace | toString)) $content -}} |
| {{- if and .Values.install.enabled (eq .Values.profile "gke") }} | ||
| {{ include "kubeflow-istio.renderFile" (dict "root" . "path" "manifests/install-gke.yaml" "oauth2" true) }} | ||
| {{- end }} |
| spec: | ||
| ports: | ||
| - name: status-port | ||
| port: 15020 | ||
| targetPort: 15020 | ||
| - name: http2 | ||
| port: 80 | ||
| targetPort: 8080 |
| readinessProbe: | ||
| failureThreshold: 30 | ||
| httpGet: | ||
| path: /healthz/ready | ||
| port: 15021 | ||
| scheme: HTTP |
| {{ include "kubeflow-istio.validateNamespace" (dict "name" "global.istioNamespace" "value" .Values.global.istioNamespace) }} | ||
| {{ include "kubeflow-istio.validateNamespace" (dict "name" "global.kubeflowNamespace" "value" .Values.global.kubeflowNamespace) }} | ||
| {{ include "kubeflow-istio.validateNamespace" (dict "name" "global.kubeSystemNamespace" "value" .Values.global.kubeSystemNamespace) }} |
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
fd75836 to
a2454f9
Compare
|
Rebased on latest |
| {{- if .Values.crds.enabled }} | ||
| {{ include "kubeflow-istio.renderFile" (dict "root" . "path" "manifests/crds.yaml") }} | ||
| {{- end }} |
| {{- $content := $root.Files.Get .path -}} | ||
| {{- $content = replace "istio-system" ($root.Values.global.istioNamespace | toString) $content -}} | ||
| {{- $content = replace "kube-system" ($root.Values.global.kubeSystemNamespace | toString) $content -}} | ||
| {{- $content = regexReplaceAll "(?m)^([[:space:]]*namespace: )kubeflow$" $content (printf "${1}%s" ($root.Values.global.kubeflowNamespace | toString)) -}} |
| def should_compare_manifest(manifest: Dict, component: str, scenario: str) -> bool: | ||
| """Select the resource subset owned by a comparison scenario.""" | ||
| if component == "istio" and manifest.get("kind") == "Namespace": | ||
| return False | ||
|
|
||
| return True |
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Summary
Adds the first Istio Helm wrapper chart for the Project 5 platform-first Helm work under
common/istio/helm, co-located with the Kustomize baseline undercommon/istio.The chart is a static Kustomize-parity wrapper for the current Kubeflow Istio install. It follows the resources under
common/istioinstead of using the upstream Istio Helm charts directly, so the first slice stays aligned with the current Kustomize baseline.This branch is aligned with the current Istio
1.30.1manifests.Supported scenarios
crds:common/istio/istio-crds/basebase: Istio CRDs, namespace NetworkPolicies, CNI,istiod, and ingress gatewayoauth2-proxy: Kubeflow Istio install with oauth2-proxy external auth providergke: GKE-specific Istio CNI path behaviorcluster-local-gateway: internal gateway resources for Knative/KServe traffickubeflow-istio-resources: Kubeflow gateway and Istio aggregate RBACplatform-full: oauth2-proxy profile plus cluster-local gateway and Kubeflow Istio resourcesScope update
experimental/helm/charts/istiotocommon/istio/helm.common/*/helm/templates/**are not treated as raw YAML.Follow-up
Sync script extension for refreshing the static Helm payloads from the regenerated Kustomize baseline will follow separately after the chart location change, so this PR stays focused on co-location and parity proof.
Stacking note
This PR follows:
The chart does not render
Namespace/istio-system. That namespace is provided by the foundation chart in #3468. Helm release metadata and Istio control-plane workloads are stored inistio-system; Istio CNI resources still run inkube-system.Install shape
Istio is installed in two Helm steps because Istio custom resources cannot be created until the Istio CRDs exist.
For GKE, use
ci/values-gke.yamlin the second step.Validation