Improve API server HTTPRoute config based on review feedback#68552
Improve API server HTTPRoute config based on review feedback#68552somaz94 wants to merge 4 commits into
Conversation
somaz94
requested review from
bugraoz93,
hussein-awala,
jedcunningham and
jscheffl
as code owners
potiuk
added
the
ready for maintainer review
| @@ -290,50 +290,6 @@ ingress: | |||
| # The Ingress Class for the PgBouncer Ingress | |||
There was a problem hiding this comment.
Could you add a comment next to the proper ingress section regarding this option too?
There was a problem hiding this comment.
Done — added a note on ingress.apiServer.enabled pointing to apiServer.httpRoute.enabled as the mutually-exclusive alternative.
| # Requires the Gateway API CRDs to be installed in the cluster | ||
| # (https://gateway-api.sigs.k8s.io/guides/#installing-gateway-api). | ||
| # The HTTPRoute references an externally managed Gateway via `parentRefs`. | ||
| httpRoute: |
There was a problem hiding this comment.
Could you add a short note regarding the ingress (exclusive options)?
There was a problem hiding this comment.
Done — added a "mutually exclusive with ingress.apiServer.enabled" note on the apiServer.httpRoute block.
| } | ||
| }, | ||
| "parentRefs": { | ||
| "description": "List of parent Gateway references this HTTPRoute attaches to. Required when enabled.", |
There was a problem hiding this comment.
If required when enabled, we could specify that minItems is 1, probably, and it should fail fast on wrong config
There was a problem hiding this comment.
Good point on failing fast. I looked at minItems: 1 on the schema, but parentRefs defaults to [] in values.yaml and Helm validates the merged values against the schema on every helm template/lint — so an unconditional minItems: 1 would fail the default (HTTPRoute-disabled) render for the whole chart, and this schema has no conditional (if/then) validation precedent. Instead I added a template guard that fails when httpRoute.enabled is true but parentRefs is empty (alongside the existing CRD / ingress-conflict guards), making parentRefs effectively required-when-enabled. Updated the render tests to supply a minimal parentRefs and added a negative test test_should_fail_when_parent_refs_empty. Happy to switch to a conditional schema instead if you'd prefer.
There was a problem hiding this comment.
If you set the default of parentRefs to ~ it work in the same way that additional logic is needed?
There was a problem hiding this comment.
Done — I set parentRefs default to ~ (null) and added minItems: 1 to the schema. This gives a two-layer guard:
- Schema catches the explicit-empty-list case fail-fast: with the default
~,minItems: 1only constrains actual arrays, soenabled: true+parentRefs: []now fails validation immediately, while the default disabled render (null) stays valid. - Template guard is kept for the
enabled: true+parentRefs: ~(null) case — JSON SchemaminItemscan't catch null, and there's noif/thenconditional precedent invalues.schema.jsonto express "non-null when enabled" purely in schema.
So the additional template logic stays minimal (just the null-while-enabled case); the empty-list case is now handled by the schema as you suggested.
|
|
||
| ``api_versions`` is passed to ``helm template --api-versions`` so that templates | ||
| gated on ``.Capabilities.APIVersions.Has`` (e.g. Gateway API CRDs) can be rendered | ||
| offline, where Helm cannot query a live cluster for installed API groups. |
There was a problem hiding this comment.
| ``api_versions`` is passed to ``helm template --api-versions`` so that templates | |
| gated on ``.Capabilities.APIVersions.Has`` (e.g. Gateway API CRDs) can be rendered | |
| offline, where Helm cannot query a live cluster for installed API groups. |
It is how this flag works, so I believe that we don't need specific documentation which copies official documentation
There was a problem hiding this comment.
Applied — dropped the api_versions docstring paragraph.
| {{- if .Values.ingress.apiServer.enabled }} | ||
| {{- fail "`apiServer.httpRoute.enabled` and `ingress.apiServer.enabled` are both set to `true`. HTTPRoute (Gateway API) is an alternative to the API server Ingress; enable only one of them." }} | ||
| {{- end }} |
There was a problem hiding this comment.
Could you add similar check for the ingress side too?
There was a problem hiding this comment.
Done — added the reciprocal guard in api-server-ingress.yaml so enabling both fails from the ingress side too. Updated the conflict test to assert on the shared "enable only one of them" phrase since either guard may fire first.
| } | ||
| }, | ||
| "parentRefs": { | ||
| "description": "List of parent Gateway references this HTTPRoute attaches to. Required when enabled.", |
There was a problem hiding this comment.
If you set the default of parentRefs to ~ it work in the same way that additional logic is needed?
3 participants
Follow-up to #67675 addressing post-merge review feedback from @Miretpl. The
original PR has not been released yet, so these refinements land before the
upcoming chart release as requested.
Changes:
httpRoute.apiServer.*toapiServer.httpRoute.*— it is an API-server setting, not a global one(values.yaml, values.schema.json, template, docs, tests).
apiServer.httpRoute.enabledis true but the Gateway API HTTPRoute CRD is not installed (via
.Capabilities.APIVersions.Has).ingress.apiServer.enabledandapiServer.httpRoute.enabledare set — HTTPRoute is an alternative to the API server Ingress.
spec.parentRefs/spec.rulesstructures, addasserts to the validation test, inline a single-use variable, and split the
parametrized creation test into separate cases (no inner
if).api_versionsparameter to therender_charthelper soCapabilities-gated templates render offline.
Validation:
helm templaterenders for enabled/disabled and fails as expected for both guardsrelated: #67675
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 4.8) following the guidelines