Miretpl commented on code in PR #67675:
URL: https://github.com/apache/airflow/pull/67675#discussion_r3405346563
##########
chart/templates/api-server/api-server-httproute.yaml:
##########
@@ -0,0 +1,61 @@
+{{/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+*/}}
+
+##################################
+## Airflow API Server HTTPRoute
+##################################
+{{- if and .Values.apiServer.enabled .Values.httpRoute.apiServer.enabled }}
Review Comment:
As it requires a custom resource installed in the cluster, could we detect
if it is installed with a Helm `Capabilities` check?
##########
chart/tests/helm_tests/apiserver/test_httproute_apiserver.py:
##########
@@ -0,0 +1,207 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+import jmespath
+import pytest
+from chart_utils.helm_template_generator import render_chart
+
+SHOW_ONLY = ["templates/api-server/api-server-httproute.yaml"]
+
+
+class TestHTTPRouteAPIServer:
+ """Tests HTTPRoute API Server (Kubernetes Gateway API)."""
+
+ def test_should_pass_validation_with_just_httproute_enabled(self):
+ render_chart(
+ values={"httpRoute": {"apiServer": {"enabled": True}}},
+ show_only=SHOW_ONLY,
+ )
Review Comment:
We should add some asserts here.
##########
chart/tests/helm_tests/apiserver/test_httproute_apiserver.py:
##########
@@ -0,0 +1,207 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+import jmespath
+import pytest
+from chart_utils.helm_template_generator import render_chart
+
+SHOW_ONLY = ["templates/api-server/api-server-httproute.yaml"]
+
+
+class TestHTTPRouteAPIServer:
+ """Tests HTTPRoute API Server (Kubernetes Gateway API)."""
+
+ def test_should_pass_validation_with_just_httproute_enabled(self):
+ render_chart(
+ values={"httpRoute": {"apiServer": {"enabled": True}}},
+ show_only=SHOW_ONLY,
+ )
+
+ def test_should_set_api_version_and_kind(self):
+ docs = render_chart(
+ values={"httpRoute": {"apiServer": {"enabled": True}}},
+ show_only=SHOW_ONLY,
+ )
+ assert jmespath.search("apiVersion", docs[0]) ==
"gateway.networking.k8s.io/v1"
+ assert jmespath.search("kind", docs[0]) == "HTTPRoute"
+
+ def test_should_allow_more_than_one_annotation(self):
+ docs = render_chart(
+ values={
+ "httpRoute": {
+ "apiServer": {"enabled": True, "annotations": {"aa": "bb",
"cc": "dd"}},
+ },
+ },
+ show_only=SHOW_ONLY,
+ )
+ assert jmespath.search("metadata.annotations", docs[0]) == {"aa":
"bb", "cc": "dd"}
+
+ def test_should_add_extra_labels(self):
+ docs = render_chart(
+ values={
+ "httpRoute": {"apiServer": {"enabled": True, "labels":
{"custom": "value"}}},
+ },
+ show_only=SHOW_ONLY,
+ )
+ assert jmespath.search('metadata.labels."custom"', docs[0]) == "value"
+
+ def test_should_pass_parent_refs_through(self):
+ docs = render_chart(
+ values={
+ "httpRoute": {
+ "apiServer": {
+ "enabled": True,
+ "parentRefs": [
+ {"name": "main-gateway", "namespace":
"gateway-system"},
+ {"name": "main-gateway", "namespace":
"gateway-system", "sectionName": "https"},
+ ],
+ },
+ },
+ },
+ show_only=SHOW_ONLY,
+ )
+ assert jmespath.search("spec.parentRefs[*].name", docs[0]) ==
["main-gateway", "main-gateway"]
+ assert jmespath.search("spec.parentRefs[1].sectionName", docs[0]) ==
"https"
Review Comment:
```suggestion
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*
##########
chart/tests/helm_tests/apiserver/test_httproute_apiserver.py:
##########
@@ -0,0 +1,207 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+import jmespath
+import pytest
+from chart_utils.helm_template_generator import render_chart
+
+SHOW_ONLY = ["templates/api-server/api-server-httproute.yaml"]
+
+
+class TestHTTPRouteAPIServer:
+ """Tests HTTPRoute API Server (Kubernetes Gateway API)."""
+
+ def test_should_pass_validation_with_just_httproute_enabled(self):
+ render_chart(
+ values={"httpRoute": {"apiServer": {"enabled": True}}},
+ show_only=SHOW_ONLY,
+ )
+
+ def test_should_set_api_version_and_kind(self):
+ docs = render_chart(
+ values={"httpRoute": {"apiServer": {"enabled": True}}},
+ show_only=SHOW_ONLY,
+ )
+ assert jmespath.search("apiVersion", docs[0]) ==
"gateway.networking.k8s.io/v1"
+ assert jmespath.search("kind", docs[0]) == "HTTPRoute"
+
+ def test_should_allow_more_than_one_annotation(self):
+ docs = render_chart(
+ values={
+ "httpRoute": {
+ "apiServer": {"enabled": True, "annotations": {"aa": "bb",
"cc": "dd"}},
+ },
+ },
+ show_only=SHOW_ONLY,
+ )
+ assert jmespath.search("metadata.annotations", docs[0]) == {"aa":
"bb", "cc": "dd"}
+
+ def test_should_add_extra_labels(self):
+ docs = render_chart(
+ values={
+ "httpRoute": {"apiServer": {"enabled": True, "labels":
{"custom": "value"}}},
+ },
+ show_only=SHOW_ONLY,
+ )
+ assert jmespath.search('metadata.labels."custom"', docs[0]) == "value"
+
+ def test_should_pass_parent_refs_through(self):
+ docs = render_chart(
+ values={
+ "httpRoute": {
+ "apiServer": {
+ "enabled": True,
+ "parentRefs": [
+ {"name": "main-gateway", "namespace":
"gateway-system"},
+ {"name": "main-gateway", "namespace":
"gateway-system", "sectionName": "https"},
+ ],
+ },
+ },
+ },
+ show_only=SHOW_ONLY,
+ )
+ assert jmespath.search("spec.parentRefs[*].name", docs[0]) ==
["main-gateway", "main-gateway"]
+ assert jmespath.search("spec.parentRefs[1].sectionName", docs[0]) ==
"https"
+
+ def test_should_set_hostnames(self):
+ docs = render_chart(
+ values={
+ "httpRoute": {
+ "apiServer": {
+ "enabled": True,
+ "hostnames": ["airflow.example.com",
"airflow2.example.com"],
+ },
+ },
+ },
+ show_only=SHOW_ONLY,
+ )
+ assert jmespath.search("spec.hostnames", docs[0]) == [
+ "airflow.example.com",
+ "airflow2.example.com",
+ ]
+
+ def test_hostnames_should_be_templated(self):
+ docs = render_chart(
+ name="airflow",
+ values={
+ "httpRoute": {
+ "apiServer": {
+ "enabled": True,
+ "hostnames": ["{{ .Release.Name }}.example.com"],
+ },
+ },
+ },
+ show_only=SHOW_ONLY,
+ )
+ assert jmespath.search("spec.hostnames", docs[0]) ==
["airflow.example.com"]
+
+ def test_should_default_to_path_prefix_and_api_server_backend(self):
+ docs = render_chart(
+ name="my-release",
+ values={"httpRoute": {"apiServer": {"enabled": True}}},
+ show_only=SHOW_ONLY,
+ )
+ assert jmespath.search("spec.rules[0].matches[0].path.type", docs[0])
== "PathPrefix"
+ assert jmespath.search("spec.rules[0].matches[0].path.value", docs[0])
== "/"
+ assert jmespath.search("spec.rules[0].backendRefs[0].name", docs[0])
== "my-release-api-server"
+ assert jmespath.search("spec.rules[0].backendRefs[0].port", docs[0])
== 8080
+
+ def test_custom_path_and_path_type_should_apply(self):
+ docs = render_chart(
+ values={
+ "httpRoute": {
+ "apiServer": {"enabled": True, "path": "/api", "pathType":
"Exact"},
+ },
+ },
+ show_only=SHOW_ONLY,
+ )
+ assert jmespath.search("spec.rules[0].matches[0].path.type", docs[0])
== "Exact"
+ assert jmespath.search("spec.rules[0].matches[0].path.value", docs[0])
== "/api"
+
+ def test_custom_rules_override_default_rule(self):
+ custom_rules = [
+ {
+ "matches": [{"path": {"type": "PathPrefix", "value":
"/admin"}}],
+ "backendRefs": [{"name": "external-admin", "port": 8443}],
+ },
+ ]
+ docs = render_chart(
+ values={
+ "httpRoute": {
+ "apiServer": {"enabled": True, "rules": custom_rules},
+ },
+ },
+ show_only=SHOW_ONLY,
+ )
+ assert jmespath.search("spec.rules[0].matches[0].path.value", docs[0])
== "/admin"
+ assert jmespath.search("spec.rules[0].backendRefs[0].name", docs[0])
== "external-admin"
+ assert jmespath.search("spec.rules[0].backendRefs[0].port", docs[0])
== 8443
+
+ @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
Review 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.
##########
chart/values.yaml:
##########
@@ -331,6 +331,50 @@ ingress:
# The Ingress Class for the PgBouncer Ingress
ingressClassName: ""
+# Kubernetes Gateway API (HTTPRoute) configuration.
+# Requires the Gateway API CRDs to be installed in the cluster
+# (https://gateway-api.sigs.k8s.io/guides/#installing-gateway-api).
+# The HTTPRoute resources reference an externally managed Gateway via
`parentRefs`.
+httpRoute:
+ # Configs for the HTTPRoute of the API Server (Airflow 3+)
+ apiServer:
Review 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 🤔
##########
chart/templates/api-server/api-server-httproute.yaml:
##########
@@ -0,0 +1,61 @@
+{{/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+*/}}
+
+##################################
+## Airflow API Server HTTPRoute
+##################################
+{{- if and .Values.apiServer.enabled .Values.httpRoute.apiServer.enabled }}
Review 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.
##########
chart/tests/helm_tests/apiserver/test_httproute_apiserver.py:
##########
@@ -0,0 +1,207 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+import jmespath
+import pytest
+from chart_utils.helm_template_generator import render_chart
+
+SHOW_ONLY = ["templates/api-server/api-server-httproute.yaml"]
+
+
+class TestHTTPRouteAPIServer:
+ """Tests HTTPRoute API Server (Kubernetes Gateway API)."""
+
+ def test_should_pass_validation_with_just_httproute_enabled(self):
+ render_chart(
+ values={"httpRoute": {"apiServer": {"enabled": True}}},
+ show_only=SHOW_ONLY,
+ )
+
+ def test_should_set_api_version_and_kind(self):
+ docs = render_chart(
+ values={"httpRoute": {"apiServer": {"enabled": True}}},
+ show_only=SHOW_ONLY,
+ )
+ assert jmespath.search("apiVersion", docs[0]) ==
"gateway.networking.k8s.io/v1"
+ assert jmespath.search("kind", docs[0]) == "HTTPRoute"
+
+ def test_should_allow_more_than_one_annotation(self):
+ docs = render_chart(
+ values={
+ "httpRoute": {
+ "apiServer": {"enabled": True, "annotations": {"aa": "bb",
"cc": "dd"}},
+ },
+ },
+ show_only=SHOW_ONLY,
+ )
+ assert jmespath.search("metadata.annotations", docs[0]) == {"aa":
"bb", "cc": "dd"}
+
+ def test_should_add_extra_labels(self):
+ docs = render_chart(
+ values={
+ "httpRoute": {"apiServer": {"enabled": True, "labels":
{"custom": "value"}}},
+ },
+ show_only=SHOW_ONLY,
+ )
+ assert jmespath.search('metadata.labels."custom"', docs[0]) == "value"
+
+ def test_should_pass_parent_refs_through(self):
+ docs = render_chart(
+ values={
+ "httpRoute": {
+ "apiServer": {
+ "enabled": True,
+ "parentRefs": [
+ {"name": "main-gateway", "namespace":
"gateway-system"},
+ {"name": "main-gateway", "namespace":
"gateway-system", "sectionName": "https"},
+ ],
+ },
+ },
+ },
+ show_only=SHOW_ONLY,
+ )
+ assert jmespath.search("spec.parentRefs[*].name", docs[0]) ==
["main-gateway", "main-gateway"]
+ assert jmespath.search("spec.parentRefs[1].sectionName", docs[0]) ==
"https"
+
+ def test_should_set_hostnames(self):
+ docs = render_chart(
+ values={
+ "httpRoute": {
+ "apiServer": {
+ "enabled": True,
+ "hostnames": ["airflow.example.com",
"airflow2.example.com"],
+ },
+ },
+ },
+ show_only=SHOW_ONLY,
+ )
+ assert jmespath.search("spec.hostnames", docs[0]) == [
+ "airflow.example.com",
+ "airflow2.example.com",
+ ]
+
+ def test_hostnames_should_be_templated(self):
+ docs = render_chart(
+ name="airflow",
+ values={
+ "httpRoute": {
+ "apiServer": {
+ "enabled": True,
+ "hostnames": ["{{ .Release.Name }}.example.com"],
+ },
+ },
+ },
+ show_only=SHOW_ONLY,
+ )
+ assert jmespath.search("spec.hostnames", docs[0]) ==
["airflow.example.com"]
+
+ def test_should_default_to_path_prefix_and_api_server_backend(self):
+ docs = render_chart(
+ name="my-release",
+ values={"httpRoute": {"apiServer": {"enabled": True}}},
+ show_only=SHOW_ONLY,
+ )
+ assert jmespath.search("spec.rules[0].matches[0].path.type", docs[0])
== "PathPrefix"
+ assert jmespath.search("spec.rules[0].matches[0].path.value", docs[0])
== "/"
+ assert jmespath.search("spec.rules[0].backendRefs[0].name", docs[0])
== "my-release-api-server"
+ assert jmespath.search("spec.rules[0].backendRefs[0].port", docs[0])
== 8080
+
+ def test_custom_path_and_path_type_should_apply(self):
+ docs = render_chart(
+ values={
+ "httpRoute": {
+ "apiServer": {"enabled": True, "path": "/api", "pathType":
"Exact"},
+ },
+ },
+ show_only=SHOW_ONLY,
+ )
+ assert jmespath.search("spec.rules[0].matches[0].path.type", docs[0])
== "Exact"
+ assert jmespath.search("spec.rules[0].matches[0].path.value", docs[0])
== "/api"
+
+ def test_custom_rules_override_default_rule(self):
+ custom_rules = [
+ {
+ "matches": [{"path": {"type": "PathPrefix", "value":
"/admin"}}],
+ "backendRefs": [{"name": "external-admin", "port": 8443}],
+ },
+ ]
Review Comment:
This variable is not used despite one place, so it is rather not needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
