Add HTTPRoute (Gateway API) support to API server in Helm Chart#67675
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
|
|
Gentle ping 🙏 CI has been green for about a week now. Would really appreciate a review when someone has a moment — the change mirrors the chart's existing Ingress convention and is fully gated behind a disabled-by-default |
jscheffl
left a comment
There was a problem hiding this comment.
In general looks good to me. Content wise good and tests are added.
One thig that should be changed prior merge is that we must not add new deprecations. Can you please clean this?
Other reviewer opinions?
|
@somaz94 A few things need addressing before review — see our Pull Request quality criteria. No rush. Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
Backport failed to create: chart/v1-2x-test. View the failure log Run detailsNote: As of Merging PRs targeted for Airflow 3.X In matter of doubt please ask in #release-management Slack channel.
You can attempt to backport this manually by running: cherry_picker 742f9a7 chart/v1-2x-testThis should apply the commit to the chart/v1-2x-test branch and leave the commit in conflict state marking After you have resolved the conflicts, you can continue the backport process by running: cherry_picker --continueIf you don't have cherry-picker installed, see the installation guide. |
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
…in Helm Chart (#67675) (#68406) * Add HTTPRoute (Gateway API) support to API server in Helm Chart * Fix HTTPRoute helm tests by vendoring Gateway API CRD schema * Remove deprecated httpRoute.enabled toggle and fix static checks (cherry picked from commit 742f9a7) Co-authored-by: somaz <112675579+somaz94@users.noreply.github.com>
Miretpl
left a comment
There was a problem hiding this comment.
A bit late, but it would be worth it to consider some things before the upcoming release.
| httpRoute: | ||
| # Configs for the HTTPRoute of the API Server (Airflow 3+) | ||
| apiServer: |
There was a problem hiding this comment.
Shouldn't we invert this to be under apiServer like apiServer.httpRoute? I don't see really a reason why it would be a global setting 🤔
| ################################## | ||
| ## Airflow API Server HTTPRoute | ||
| ################################## | ||
| {{- if and .Values.apiServer.enabled .Values.httpRoute.apiServer.enabled }} |
There was a problem hiding this comment.
As it requires a custom resource installed in the cluster, could we detect if it is installed with a Helm Capabilities check?
| def test_should_pass_validation_with_just_httproute_enabled(self): | ||
| render_chart( | ||
| values={"httpRoute": {"apiServer": {"enabled": True}}}, | ||
| show_only=SHOW_ONLY, | ||
| ) |
There was a problem hiding this comment.
We should add some asserts here.
| assert jmespath.search("spec.parentRefs[*].name", docs[0]) == ["main-gateway", "main-gateway"] | ||
| assert jmespath.search("spec.parentRefs[1].sectionName", docs[0]) == "https" |
There was a problem hiding this comment.
| assert jmespath.search("spec.parentRefs[*].name", docs[0]) == ["main-gateway", "main-gateway"] | |
| assert jmespath.search("spec.parentRefs[1].sectionName", docs[0]) == "https" | |
| assert jmespath.search("spec.parentRefs, docs[0]) == [ | |
| {"name": "main-gateway", "namespace": "gateway-system"}, | |
| {"name": "main-gateway", "namespace": "gateway-system", "sectionName": "https"} | |
| ] |
this way we will be sure that the whole structure is correct.
similar comment to the other test cases too
| custom_rules = [ | ||
| { | ||
| "matches": [{"path": {"type": "PathPrefix", "value": "/admin"}}], | ||
| "backendRefs": [{"name": "external-admin", "port": 8443}], | ||
| }, | ||
| ] |
There was a problem hiding this comment.
This variable is not used despite one place, so it is rather not needed.
| @pytest.mark.parametrize( | ||
| ("api_server_value", "expected"), | ||
| [ | ||
| (None, False), | ||
| (False, False), | ||
| (True, True), | ||
| ], | ||
| ) | ||
| def test_httproute_created(self, api_server_value, expected): | ||
| values = {"httpRoute": {}} | ||
| if api_server_value is not None: | ||
| values["httpRoute"]["apiServer"] = {"enabled": api_server_value} | ||
| docs = render_chart(values=values, show_only=SHOW_ONLY) | ||
| assert bool(docs) is expected |
There was a problem hiding this comment.
I would separate it into two different test cases to remove the if logic from inside the test. If it fails, it will make it harder to debug the error.
| ################################## | ||
| ## Airflow API Server HTTPRoute | ||
| ################################## | ||
| {{- if and .Values.apiServer.enabled .Values.httpRoute.apiServer.enabled }} |
There was a problem hiding this comment.
Shouldn't this be an alternative to the Ingress? If yes, we should probably add some checks if both are not enabled at the same time, etc.
All good points that it seems I over-looked in review. @somaz94 can you make a follow-up PR? |
…he#67675) * Add HTTPRoute (Gateway API) support to API server in Helm Chart * Fix HTTPRoute helm tests by vendoring Gateway API CRD schema * Remove deprecated httpRoute.enabled toggle and fix static checks
…he#67675) * Add HTTPRoute (Gateway API) support to API server in Helm Chart * Fix HTTPRoute helm tests by vendoring Gateway API CRD schema * Remove deprecated httpRoute.enabled toggle and fix static checks
Adds an optional
HTTPRouteresource for the API server, gated byhttpRoute.apiServer.enabled.The chart only creates the route; the parent
Gatewayand Gateway API CRDs must be installedout of band. Mirrors the existing
ingress.apiServer.*shape so users can switch betweenIngressandHTTPRoutewithout rewiring the rest of the chart.This is a first slice — flower / statsd / pgbouncer can follow once this shape is agreed.
Validation:
helm templaterenders correctly with default rule, custom rules override, hostnames + parentRefs.helm templaterejects malformed values (e.g.parentRefs[*].namemissing) viavalues.schema.json.chart_schema.pypasses ("No problems").chart/tests/helm_tests/apiserver/test_httproute_apiserver.py.related: #61452
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 4.7) following the guidelines