helm: add cert-manager wrapper chart#3470
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
Adds an experimental Helm wrapper chart for cert-manager and wires it into the Helm/Kustomize comparison tooling.
Changes:
- Introduces the cert-manager wrapper chart, values, templates, lock file, and README.
- Adds Kubeflow-specific optional resources for the ClusterIssuer and NetworkPolicies.
- Extends comparison scripts to cover cert-manager base and Kubeflow scenarios.
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
.gitignore |
Ignores generated Helm dependency archives. |
experimental/helm/charts/cert-manager/.helmignore |
Defines files excluded from chart packaging. |
experimental/helm/charts/cert-manager/Chart.lock |
Locks the Jetstack cert-manager chart dependency. |
experimental/helm/charts/cert-manager/Chart.yaml |
Defines the wrapper chart metadata and dependency. |
experimental/helm/charts/cert-manager/README.md |
Documents install and parity validation flow. |
experimental/helm/charts/cert-manager/ci/values-base.yaml |
Adds base comparison values. |
experimental/helm/charts/cert-manager/ci/values-kubeflow.yaml |
Adds Kubeflow comparison values. |
experimental/helm/charts/cert-manager/templates/kubeflow-resources.yaml |
Adds optional Kubeflow ClusterIssuer and NetworkPolicies. |
experimental/helm/charts/cert-manager/templates/namespace.yaml |
Adds the cert-manager namespace template. |
experimental/helm/charts/cert-manager/values.yaml |
Adds default wrapper chart values. |
tests/helm_kustomize_compare.py |
Allows cert-manager in manifest comparison. |
tests/helm_kustomize_compare.sh |
Adds cert-manager scenarios and rendering logic. |
tests/helm_kustomize_compare_all.sh |
Includes cert-manager in all-scenario comparison runs. |
|
|
||
| ```bash | ||
| helm install kubeflow-namespaces ./experimental/helm/charts/kubeflow-namespaces | ||
| helm install kubeflow-platform ./experimental/helm/charts/kubeflow-platform |
There was a problem hiding this comment.
Fixed in the README: kubeflow-platform and cert-manager now use --namespace kubeflow-system.
| namespace = sys.argv[5] if len(sys.argv) > 5 and not sys.argv[5].startswith('--') else "" | ||
|
|
||
| if component not in ["katib", "hub", "kserve-models-web-app"]: | ||
| if component not in ["katib", "hub", "kserve-models-web-app", "cert-manager"]: |
There was a problem hiding this comment.
Fixed by preserving the Kubeflow cert-manager overlay labels on the overlay-owned resources during comparison.
553f9c5 to
6b1d396
Compare
8cf2018 to
e1f141d
Compare
|
Please first update kustomize to the latest available version in a separate PR. |
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
e1f141d to
a945009
Compare
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
|
Addressed: cert-manager Kustomize manifests were synchronized to |
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
| - upstream cert-manager `v1.20.2` | ||
| - cert-manager CRDs | ||
| - optional `ClusterIssuer/kubeflow-self-signing-issuer` | ||
| - optional Kubeflow cert-manager NetworkPolicies |
There was a problem hiding this comment.
Here it should be
- optional kubeflow specific cert-manager NetworkPolicies.
There was a problem hiding this comment.
Updated the bullet to optional Kubeflow-specific cert-manager NetworkPolicies.
| - optional `ClusterIssuer/kubeflow-self-signing-issuer` | ||
| - optional Kubeflow cert-manager NetworkPolicies | ||
|
|
||
| In the Kubeflow platform install, apply the foundation charts first. The `kubeflow-namespaces` chart provides `Namespace/cert-manager`; this wrapper stores its Helm release metadata in that same workload namespace. |
There was a problem hiding this comment.
Specify the names of the foundation charts and then mention what they provide.
| Chart type | Chart |
|---|---|
| Bootstrap Foundation | kubeflow-namespaces |
| Platform Foundation | kubeflow-platform |
There was a problem hiding this comment.
Added a small table for kubeflow-namespaces and kubeflow-platform with what each foundation chart provides.
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
| helm install kubeflow-namespaces ./experimental/helm/charts/kubeflow-namespaces --namespace default | ||
| helm install kubeflow-platform ./experimental/helm/charts/kubeflow-platform --namespace kubeflow-system | ||
|
|
||
| helm install cert-manager ./experimental/helm/charts/cert-manager --namespace cert-manager --wait |
There was a problem hiding this comment.
Add a command to pull the dependencies first using Chart.lock
helm dep build ./experimental/helm/charts/cert-manager
There was a problem hiding this comment.
Added helm dep build ./experimental/helm/charts/cert-manager before the cert-manager install command.
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
|
/retest |
| --values ./common/cert-manager/helm/ci/values-kubeflow.yaml --wait | ||
| ``` | ||
|
|
||
| The install is split into base install plus upgrade because `ClusterIssuer` cannot be created until cert-manager CRDs are available. |
There was a problem hiding this comment.
| The install is split into base install plus upgrade because `ClusterIssuer` cannot be created until cert-manager CRDs are available. | |
| The installation is split into base install plus upgrade because `ClusterIssuer` cannot be created until cert-manager CRDs are available. |
There was a problem hiding this comment.
| ROOT_DIRETORY="$(dirname "$SCRIPT_DIRECTORY")" |
There was a problem hiding this comment.
| SCRIPT_DIRECTORY="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" |
| declare -a failed_components=() | ||
|
|
||
| for comp in katib hub kserve-models-web-app; do | ||
| for comp in katib hub kserve-models-web-app cert-manager; do |
There was a problem hiding this comment.
| for comp in katib hub kserve-models-web-app cert-manager; do | |
| for component in katib hub kserve-models-web-app cert-manager; do |
| @@ -12,6 +12,7 @@ declare -A COMPONENT_SCENARIOS=( | |||
| ["katib"]="standalone cert-manager external-db leader-election openshift standalone-postgres with-kubeflow" | |||
| ["hub"]="base overlay-postgres overlay-db controller-manager controller-rbac controller-default controller-prometheus controller-network-policy ui-base ui-standalone ui-integrated ui-istio istio csi" | |||
| ["kserve-models-web-app"]="base kubeflow" | |||
There was a problem hiding this comment.
| ["kserve-models-web-app"]="base kubeflow" | |
| ["kserve-models-web-application"]="base kubeflow" |
| else | ||
| echo "ERROR: Unknown component: $COMPONENT" | ||
| echo "Supported components: katib, hub, kserve-models-web-app, all" | ||
| echo "Supported components: katib, hub, kserve-models-web-app, cert-manager, all" |
There was a problem hiding this comment.
| echo "Supported components: katib, hub, kserve-models-web-app, cert-manager, all" | |
| echo "Supported components: katib, hub, kserve-models-web-application, cert-manager, all" |
There was a problem hiding this comment.
| echo "ERROR: Helm chart directory does not exist: $CHART_DIRECTORY" |
There was a problem hiding this comment.
| if [ -n "$HELM_VALUES_ARG" ] && [ ! -f "$HELM_VALUES_ARGUMENTS" ]; then |
There was a problem hiding this comment.
| echo "ERROR: Helm values file does not exist: $HELM_VALUES_ARGUMENTS" |
There was a problem hiding this comment.
| cd "$ROOT_DIRECTORY" |
There was a problem hiding this comment.
| cd "$ROOT_DIRECTORY" | |
| if [[ "$COMPONENT" == "kserve-models-web-application" ]]; then |
There was a problem hiding this comment.
no, you must use helm 4
|
the synchonization script fixes are missing. It must update both kustomize and helm |
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>
@danish9039 Any updates on this? or will be fixed in follow up pr? |
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Summary
Adds an experimental cert-manager Helm wrapper chart under
common/cert-manager/helm, co-located with the Kustomize baseline undercommon/cert-manager.The chart wraps the upstream Jetstack cert-manager Helm chart at
v1.20.2and adds Kubeflow-specific cert-manager integration resources.Scope
experimental/helm/charts/cert-managertocommon/cert-manager/helm.common/*/helm/templates/**are not treated as raw YAML.Chart.yamldependency version as bare semver ("1.20.2") while preservingappVersion: v1.20.2.Chart.lockafter the dependency version fix.tests/helm_kustomize_compare.shwith the co-located chart path and--include-crds.helm dependency buildso CI can resolve the cert-manager dependency from a clean runner.Follow-up
Sync script extension for appVersion/dependency updates will follow separately after the chart location change, so this PR stays focused on co-location and parity proof.
Stacking note
This PR is stacked on the foundation chart shape from #3468.
The wrapper does not render
Namespace/cert-manager. The namespace is provided bykubeflow-namespaces, and the Helm release metadata is stored in thecert-managerworkload namespace.This means the intended platform install order is:
Install shape
Validation
Also validated the cert-manager compare path with an empty Helm repository config to match clean CI runners.