helm: add oauth2-proxy wrapper chart#3479
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.
Adds Helm/Kustomize parity coverage for the oauth2-proxy component and introduces an initial (static) Helm chart to render the existing Kustomize resources, while improving manifest comparison normalization.
Changes:
- Extend Helm/Kustomize comparison scripts to support
oauth2-proxyscenarios and “all” runs. - Add an
experimental/helm/charts/oauth2-proxychart (templates, CI values files, docs). - Refine Kustomize hash-suffix normalization logic used by the manifest diff tool.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/helm_kustomize_compare_all.sh | Adds oauth2-proxy scenarios to the “compare all” harness and help text. |
| tests/helm_kustomize_compare.sh | Adds oauth2-proxy chart/kustomize mappings and Helm templating path. |
| tests/helm_kustomize_compare.py | Adds a compiled regex for Kustomize hash suffix stripping and includes namespace in keys for oauth2-proxy. |
| experimental/helm/charts/oauth2-proxy/Chart.yaml | Introduces the new Helm chart metadata. |
| experimental/helm/charts/oauth2-proxy/README.md | Documents installation and comparison workflows for the new chart. |
| experimental/helm/charts/oauth2-proxy/values.yaml | Defines default chart values for namespaces, credentials, and feature toggles. |
| experimental/helm/charts/oauth2-proxy/templates/_helpers.tpl | Adds helper templates for namespaces and JWT rule rendering. |
| experimental/helm/charts/oauth2-proxy/templates/namespace.yaml | Renders the oauth2-proxy Namespace (optional). |
| experimental/helm/charts/oauth2-proxy/templates/oauth2-proxy.yaml | Renders ServiceAccount/ConfigMaps/Secret/Service/Deployment/VirtualService for oauth2-proxy. |
| experimental/helm/charts/oauth2-proxy/templates/networkpolicies.yaml | Renders namespace network policies (optional). |
| experimental/helm/charts/oauth2-proxy/templates/istio-external-auth.yaml | Renders Istio AuthorizationPolicy + RequestAuthentication (optional). |
| experimental/helm/charts/oauth2-proxy/templates/m2m.yaml | Renders m2m RequestAuthentication (optional). |
| experimental/helm/charts/oauth2-proxy/templates/cluster-jwks-proxy.yaml | Renders JWKS proxy deployment for kind-like clusters (optional). |
| experimental/helm/charts/oauth2-proxy/files/oauth2_proxy.cfg | Adds the oauth2-proxy config file embedded into a ConfigMap. |
| experimental/helm/charts/oauth2-proxy/files/kubeflow-logo.svg | Adds a logo asset embedded into a ConfigMap. |
| experimental/helm/charts/oauth2-proxy/ci/values-base.yaml | CI values mapping for common/oauth2-proxy/base. |
| experimental/helm/charts/oauth2-proxy/ci/values-m2m-dex-only.yaml | CI values mapping for m2m-dex-only overlay. |
| experimental/helm/charts/oauth2-proxy/ci/values-m2m-dex-and-kind.yaml | CI values mapping for m2m-dex-and-kind overlay. |
| experimental/helm/charts/oauth2-proxy/ci/values-m2m-dex-and-eks.yaml | CI values mapping for m2m-dex-and-eks overlay. |
| credentials: | ||
| # -- OAuth client ID used by oauth2-proxy. | ||
| clientID: kubeflow-oidc-authservice | ||
| # -- OAuth client secret used by oauth2-proxy. | ||
| clientSecret: pUBnBOY80SnXgjibTYM9ZWNzY2xreNGQok | ||
| # -- Cookie secret used by oauth2-proxy. | ||
| cookieSecret: 7d16fee92f8d11b8940b081b3f8b8acb |
There was a problem hiding this comment.
Moved the Kustomize parity secrets into CI values and left placeholders in chart defaults.
| apiVersion: v1 | ||
| data: | ||
| ALLOW_SELF_SIGNED_ISSUER: {{ .Values.parameters.allowSelfSignedIssuer | quote }} | ||
| ENABLE_M2M_TOKENS: {{ .Values.parameters.enableM2MTokens | quote }} |
There was a problem hiding this comment.
Kept this aligned with current Kustomize; ENABLE_M2M_TOKENS is the existing parameter used for --skip-jwt-bearer-tokens.
There was a problem hiding this comment.
Kept as Kustomize parity; behavior rename belongs in follow-up.
| valueFrom: | ||
| configMapKeyRef: | ||
| key: ENABLE_M2M_TOKENS | ||
| name: oauth2-proxy-parameters |
There was a problem hiding this comment.
Kept this aligned with current Kustomize; ENABLE_M2M_TOKENS is the existing parameter used for --skip-jwt-bearer-tokens.
There was a problem hiding this comment.
Kept as Kustomize parity; behavior rename belongs in follow-up.
| from typing import Dict, List, Tuple, Any | ||
| import re | ||
|
|
||
| KUSTOMIZE_HASH_SUFFIX = re.compile(r'-(?=[a-z0-9]{10}$)(?=[a-z0-9]*[0-9])[a-z0-9]{10}$') |
There was a problem hiding this comment.
Checked this against current manifests; removing the digit guard incorrectly strips stable names like oauth2-proxy-parameters, so I kept it.
| clusterJwksProxy: | ||
| # -- Render the cluster JWKS proxy used by kind-like clusters. | ||
| enabled: false | ||
| image: docker.io/bitnamisecure/kubectl:latest |
There was a problem hiding this comment.
Moved the latest image out of chart defaults; it is now only set in the CI parity values that match Kustomize.
| elif [[ "$COMPONENT" == "oauth2-proxy" ]]; then | ||
| helm template oauth2-proxy . \ | ||
| --namespace "$NAMESPACE" \ | ||
| --include-crds \ | ||
| --values "$HELM_VALUES_ARG" > "$HELM_OUTPUT" |
There was a problem hiding this comment.
Leaving this as a follow-up cleanup because this PR keeps the component mapping explicit for review.
5b2986d to
9cc1a98
Compare
|
Please first update kustomize to the latest available version in a separate PR. |
28a36ef to
3a11549
Compare
|
Addressed: oauth2-proxy Kustomize manifests were synchronized to |
| from typing import Dict, List, Tuple, Any | ||
| import re | ||
|
|
||
| KUSTOMIZE_HASH_SUFFIX = re.compile(r'-(?=[a-z0-9]{10}$)(?=[a-z0-9]*[0-9])[a-z0-9]{10}$') |
| if any(ref_pattern in path for ref_pattern in [ | ||
| 'secretKeyRef', 'configMapKeyRef', 'configMapRef', 'secret', 'volumes' | ||
| ]): | ||
| value = re.sub(r'-[a-z0-9]{10}$', '', value) | ||
| value = KUSTOMIZE_HASH_SUFFIX.sub('', value) | ||
| elif 'volumes' in path and key == 'secretName': | ||
| value = re.sub(r'-[a-z0-9]{10}$', '', value) | ||
| value = KUSTOMIZE_HASH_SUFFIX.sub('', value) | ||
| elif 'volumes' in path and 'configMap' in path: | ||
| value = re.sub(r'-[a-z0-9]{10}$', '', value) | ||
| value = KUSTOMIZE_HASH_SUFFIX.sub('', value) |
| def should_compare_manifest(manifest: Dict, component: str, scenario: str) -> bool: | ||
| """Select the resource subset owned by a comparison scenario.""" | ||
| if component == "oauth2-proxy" and manifest.get("kind") == "Namespace": | ||
| return False | ||
|
|
||
| return True |
| spec: | ||
| containers: | ||
| - args: | ||
| - --http-address=0.0.0.0:4180 | ||
| - --config=/etc/oauth2_proxy/oauth2_proxy.cfg |
There was a problem hiding this comment.
Already wired to serviceAccountName: oauth2-proxy.
| - name: OAUTH2_PROXY_SKIP_JWT_BEARER_TOKENS | ||
| valueFrom: | ||
| configMapKeyRef: | ||
| key: ENABLE_M2M_TOKENS | ||
| name: oauth2-proxy-parameters |
There was a problem hiding this comment.
Kept as Kustomize parity; behavior rename belongs in follow-up.
| credentials: | ||
| clientSecret: pUBnBOY80SnXgjibTYM9ZWNzY2xreNGQok | ||
| cookieSecret: 7d16fee92f8d11b8940b081b3f8b8acb |
| - --accept-hosts=.* | ||
| - --accept-paths=^(?:/openid/v1/jwks)|(?:/.well-known/openid-configuration)$ | ||
| - --reject-methods=^(POST|PUT|PATCH|DELETE|HEAD|OPTIONS|CONNECT|TRACE)$ | ||
| image: {{ .Values.clusterJwksProxy.image }} |
juliusvonkohout
left a comment
There was a problem hiding this comment.
Pleasemove it to common/istio/helm and exten the istio synchronization script to also update helm.
| cookieSecret: 7d16fee92f8d11b8940b081b3f8b8acb | ||
|
|
||
| istioExternalAuth: | ||
| enabled: false |
There was a problem hiding this comment.
do you mean authentication or authorization?
There was a problem hiding this comment.
Authorisation, corrected this
There was a problem hiding this comment.
what is in general the prupose of this file? what is the customer scenario ?
There was a problem hiding this comment.
Removed , it was just a parity fixture
|
[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 |
|
/retest |
9834909 to
75ab734
Compare
@juliusvonkohout Moved the charts to ommon/istio/helm , also extended the sync script as well |
| "oauth2-proxy") | ||
| CHART_DIRECTORY="$ROOT_DIRECTORY/common/oauth2-proxy/helm" | ||
| MANIFESTS_DIRECTORY="$ROOT_DIRECTORY/common/oauth2-proxy" | ||
|
|
||
| declare -A KUSTOMIZE_PATHS=( | ||
| ["m2m-dex-only"]="$MANIFESTS_DIRECTORY/overlays/m2m-dex-only" | ||
| ) | ||
|
|
||
| declare -A HELM_VALUES=( | ||
| ["m2m-dex-only"]="$CHART_DIRECTORY/ci/values-m2m-dex-only.yaml" | ||
| ) |
There was a problem hiding this comment.
Fixed by defaulting and documenting m2m-dex-only.
| MANIFESTS_DIRECTORY=$(dirname $SCRIPT_DIRECTORY) | ||
| DESTINATION_DIRECTORY=$MANIFESTS_DIRECTORY/common/${COMPONENT_NAME} | ||
| CHART_DIRECTORY="$DESTINATION_DIRECTORY/helm" | ||
| create_branch "$BRANCH_NAME" | ||
| if [[ "$OSTYPE" == "darwin"* ]]; then | ||
| sed -i "" "s|quay.io/oauth2-proxy/oauth2-proxy:.*|quay.io/oauth2-proxy/oauth2-proxy:${COMMIT}|g" \ | ||
| $DESTINATION_DIRECTORY/base/deployment.yaml | ||
| sed -i "" "s|^appVersion: .*|appVersion: ${COMMIT#v}|g" \ | ||
| "$CHART_DIRECTORY/Chart.yaml" | ||
| sed -i "" "s|quay.io/oauth2-proxy/oauth2-proxy:.*|quay.io/oauth2-proxy/oauth2-proxy:${COMMIT}|g" \ | ||
| "$CHART_DIRECTORY/templates/oauth2-proxy.yaml" | ||
| else | ||
| sed -i "s|quay.io/oauth2-proxy/oauth2-proxy:.*|quay.io/oauth2-proxy/oauth2-proxy:${COMMIT}|g" \ | ||
| $DESTINATION_DIRECTORY/base/deployment.yaml | ||
| sed -i "s|^appVersion: .*|appVersion: ${COMMIT#v}|g" \ | ||
| "$CHART_DIRECTORY/Chart.yaml" | ||
| sed -i "s|quay.io/oauth2-proxy/oauth2-proxy:.*|quay.io/oauth2-proxy/oauth2-proxy:${COMMIT}|g" \ | ||
| "$CHART_DIRECTORY/templates/oauth2-proxy.yaml" |
There was a problem hiding this comment.
Fixed path quoting in the sync script.
531a695 to
f9e0b6f
Compare
| if [[ -z "$COMPONENT" ]]; then | ||
| echo "ERROR: Component is required" | ||
| echo "Usage: $0 <component> <scenario>" | ||
| echo "Components: katib, hub, kserve-models-web-application, cert-manager" | ||
| echo "Components: katib, hub, kserve-models-web-application, cert-manager, oauth2-proxy" | ||
| exit 1 |
| --- | ||
| apiVersion: v1 | ||
| data: | ||
| client-id: {{ .Values.credentials.clientID | b64enc }} | ||
| client-secret: {{ .Values.credentials.clientSecret | b64enc }} | ||
| cookie-secret: {{ .Values.credentials.cookieSecret | b64enc }} | ||
| kind: Secret | ||
| metadata: | ||
| name: oauth2-proxy | ||
| namespace: oauth2-proxy | ||
| type: Opaque | ||
| --- |
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>
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>
df4c286 to
ff69be0
Compare
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Summary
Adds the oauth2-proxy Helm wrapper chart for the Project 5 platform-first Helm work under
common/oauth2-proxy/helm, co-located with the Kustomize baseline undercommon/oauth2-proxy.The chart is a Kustomize-parity wrapper for the current Kubeflow oauth2-proxy install. It follows
common/oauth2-proxyand keeps the first slice focused on the current Kubeflow authentication integration shape.Supported comparison scenario
m2m-dex-only:common/oauth2-proxy/overlays/m2m-dex-onlyThis is the meaningful current Kubeflow oauth2-proxy scenario used with Dex. Additional cluster-specific variants are deferred until they have a clear Helm integration-test scenario.
Scope update
experimental/helm/charts/oauth2-proxytocommon/oauth2-proxy/helm.common/*/helm/templates/**are not treated as raw YAML.tests/helm_kustomize_compare.shwith the co-located chart path and them2m-dex-onlyscenario.scripts/synchronize-oauth2-proxy-manifests.shso future image/appVersion bumps update the Helm chart surfaces too.serviceAccountName: oauth2-proxyin both Kustomize and Helm output,clusterJwksProxy.imagewhenclusterJwksProxy.enabled=true,secretNamenormalization under volumes,CI fixture note
The oauth2-proxy runtime requires a cookie secret length valid for AES. This PR restores the existing Kustomize fixture secret values and mirrors those same values in Helm CI values so the wrapper chart remains a parity implementation. Any broader cleanup of fixture-secret naming or semantics should be handled in a separate follow-up PR.
The
OAUTH2_PROXY_SKIP_JWT_BEARER_TOKENS/ENABLE_M2M_TOKENSnaming remains unchanged in this PR because the current Kustomize baseline uses that mapping. Changing it should be handled as an intentional auth-behavior decision across both Kustomize and Helm.Stacking note
This PR is part of the platform authentication chain and is not intended to be installed before the foundation charts and Istio auth provider are present.
Expected platform order:
The Istio chart must be installed with the oauth2-proxy extension provider, because this chart renders Istio external authentication resources that reference that provider.
The default Kubeflow login path is only complete once Dex is installed as well.
Namespace behavior
The chart does not render
Namespace/oauth2-proxy. That namespace is provided by the foundation chart in #3468, and the Helm release metadata is stored in theoauth2-proxyworkload namespace.Validation
Also rendered every
common/oauth2-proxy/helm/ci/values*.yamlfile locally.