pierrejeambrun commented on code in PR #43841: URL: https://github.com/apache/airflow/pull/43841#discussion_r1837815805
########## airflow/api_fastapi/core_api/routes/public/config.py: ########## @@ -0,0 +1,121 @@ +# 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 + +from typing import Literal + +from fastapi import Header, HTTPException, status +from fastapi.responses import Response + +from airflow.api_fastapi.common.router import AirflowRouter +from airflow.api_fastapi.core_api.datamodels.config import ( + Config, + ConfigOption, + ConfigSection, +) +from airflow.api_fastapi.core_api.openapi.exceptions import create_openapi_http_exception_doc +from airflow.configuration import conf + + +def _check_expose_config() -> bool: + display_sensitive: bool | None = None + if conf.get("webserver", "expose_config").lower() == "non-sensitive-only": + expose_config = True + display_sensitive = False + else: + expose_config = conf.getboolean("webserver", "expose_config") + display_sensitive = True + + if not expose_config: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Your Airflow administrator chose not to expose the configuration, most likely for security reasons.", + ) + return display_sensitive + + +config_router = AirflowRouter(tags=["Config"], prefix="/config") + + +@config_router.get( + "/", + responses=create_openapi_http_exception_doc( + [status.HTTP_401_UNAUTHORIZED, status.HTTP_403_FORBIDDEN, status.HTTP_404_NOT_FOUND] + ), +) +def get_config( + section: str | None = None, + content_type: Literal["application/json", "text/plain"] = Header(...), Review Comment: Nice. Mimetype on fastapi are already defined in the `dag_source.py` file. What we need to do is extract from `dag_source.py` and this file the `mimetype literal` or enum, and put that in a common file, most likely `common/types.py`. As you will see we also need the wildcard mimetype `*/*` because some http client send that to mention that they accept everything. In that case we should most likely default to `application/json`. And then re-use that literal in both here and `dag_sources`. Lastly you can use the `Annotated` notation, which is more modern and `Annotated[MimeTypeLiteral, Header()]`. (Not sure if the ellipsis `...` is needed). And remove the `NOT_ACCEPTABLE` code path for the `dag_source`. (handled by pydantic validation now) ########## airflow/api_fastapi/core_api/routes/public/config.py: ########## @@ -0,0 +1,121 @@ +# 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 + +from typing import Literal + +from fastapi import Header, HTTPException, status +from fastapi.responses import Response + +from airflow.api_fastapi.common.router import AirflowRouter +from airflow.api_fastapi.core_api.datamodels.config import ( + Config, + ConfigOption, + ConfigSection, +) +from airflow.api_fastapi.core_api.openapi.exceptions import create_openapi_http_exception_doc +from airflow.configuration import conf + + +def _check_expose_config() -> bool: + display_sensitive: bool | None = None + if conf.get("webserver", "expose_config").lower() == "non-sensitive-only": + expose_config = True + display_sensitive = False + else: + expose_config = conf.getboolean("webserver", "expose_config") + display_sensitive = True + + if not expose_config: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Your Airflow administrator chose not to expose the configuration, most likely for security reasons.", + ) + return display_sensitive + + +config_router = AirflowRouter(tags=["Config"], prefix="/config") + + +@config_router.get( + "/", + responses=create_openapi_http_exception_doc( + [status.HTTP_401_UNAUTHORIZED, status.HTTP_403_FORBIDDEN, status.HTTP_404_NOT_FOUND] + ), +) +def get_config( + section: str | None = None, + content_type: Literal["application/json", "text/plain"] = Header(...), +) -> Response: + display_sensitive = _check_expose_config() + + if section and not conf.has_section(section): + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"Section {section} not found.", + ) + conf_dict = conf.as_dict(display_source=False, display_sensitive=display_sensitive) + + if section: + conf_section_value = conf_dict[section] + conf_dict.clear() + conf_dict[section] = conf_section_value + + config = Config( + sections=[ + ConfigSection( + name=section, options=[ConfigOption(key=key, value=value) for key, value in options.items()] + ) + for section, options in conf_dict.items() + ] + ) + + return Response( + content=config.model_dump_json() if content_type == "application/json" else config.text_format, + headers={"Content-Type": content_type}, + ) + + +@config_router.get( + "/section/{section}/option/{option}", + responses=create_openapi_http_exception_doc( + [status.HTTP_401_UNAUTHORIZED, status.HTTP_403_FORBIDDEN, status.HTTP_404_NOT_FOUND] + ), Review Comment: Same for docs. ########## airflow/api_fastapi/core_api/routes/public/config.py: ########## @@ -0,0 +1,121 @@ +# 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 + +from typing import Literal + +from fastapi import Header, HTTPException, status +from fastapi.responses import Response + +from airflow.api_fastapi.common.router import AirflowRouter +from airflow.api_fastapi.core_api.datamodels.config import ( + Config, + ConfigOption, + ConfigSection, +) +from airflow.api_fastapi.core_api.openapi.exceptions import create_openapi_http_exception_doc +from airflow.configuration import conf + + +def _check_expose_config() -> bool: + display_sensitive: bool | None = None + if conf.get("webserver", "expose_config").lower() == "non-sensitive-only": + expose_config = True + display_sensitive = False + else: + expose_config = conf.getboolean("webserver", "expose_config") + display_sensitive = True + + if not expose_config: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Your Airflow administrator chose not to expose the configuration, most likely for security reasons.", + ) + return display_sensitive + + +config_router = AirflowRouter(tags=["Config"], prefix="/config") + + +@config_router.get( + "/", + responses=create_openapi_http_exception_doc( + [status.HTTP_401_UNAUTHORIZED, status.HTTP_403_FORBIDDEN, status.HTTP_404_NOT_FOUND] + ), +) +def get_config( + section: str | None = None, + content_type: Literal["application/json", "text/plain"] = Header(...), Review Comment: Also as you will see in `dag_source`. We use `start_with` because sometimes the mimetype can be `application/json; charset=utf-8` and not an exact match with `application_json`, but i didn't look to much to it. I think some tests were failing otherwise. ########## tests/api_fastapi/core_api/routes/public/test_config.py: ########## @@ -0,0 +1,317 @@ +# 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 textwrap +from typing import Generator +from unittest.mock import patch + +import pytest + +from tests_common.test_utils.config import conf_vars + +HEADERS_JSON = {"Content-Type": "application/json"} +HEADERS_TEXT = {"Content-Type": "text/plain"} +HEADERS_INVALID = {"Content-Type": "invalid"} +SECTION_CORE = "core" +SECTION_SMTP = "smtp" +SECTION_DATABASE = "database" +SECTION_NOT_EXIST = "not_exist_section" +OPTION_KEY_PARALLELISM = "parallelism" +OPTION_KEY_SMTP_HOST = "smtp_host" +OPTION_KEY_SMTP_MAIL_FROM = "smtp_mail_from" +OPTION_KEY_SQL_ALCHEMY_CONN = "sql_alchemy_conn" +OPTION_VALUE_PARALLELISM = "1024" +OPTION_VALUE_SMTP_HOST = "smtp.example.com" +OPTION_VALUE_SMTP_MAIL_FROM = "[email protected]" +OPTION_VALUE_SQL_ALCHEMY_CONN = "mock_sql_alchemy_conn" +OPTION_NOT_EXIST = "not_exist_option" +OPTION_VALUE_SENSITIVE_HIDDEN = "< hidden >" + +MOCK_CONFIG_DICT = { + SECTION_CORE: { + OPTION_KEY_PARALLELISM: OPTION_VALUE_PARALLELISM, + }, + SECTION_SMTP: { + OPTION_KEY_SMTP_HOST: OPTION_VALUE_SMTP_HOST, + OPTION_KEY_SMTP_MAIL_FROM: OPTION_VALUE_SMTP_MAIL_FROM, + }, + SECTION_DATABASE: { + OPTION_KEY_SQL_ALCHEMY_CONN: OPTION_VALUE_SQL_ALCHEMY_CONN, + }, +} +MOCK_CONFIG_OVERRIDE = { + (SECTION_CORE, OPTION_KEY_PARALLELISM): OPTION_VALUE_PARALLELISM, + (SECTION_SMTP, OPTION_KEY_SMTP_HOST): OPTION_VALUE_SMTP_HOST, + (SECTION_SMTP, OPTION_KEY_SMTP_MAIL_FROM): OPTION_VALUE_SMTP_MAIL_FROM, +} + +AIRFLOW_CONFIG_ENABLE_EXPOSE_CONFIG = {("webserver", "expose_config"): "True"} +AIRFLOW_CONFIG_DISABLE_EXPOSE_CONFIG = {("webserver", "expose_config"): "False"} +FORBIDDEN_RESPONSE = { + "detail": "Your Airflow administrator chose not to expose the configuration, most likely for security reasons." +} + + +class TestConfigEndpoint: + @pytest.fixture(autouse=True) + def setup(self) -> Generator[None, None, None]: + with conf_vars(AIRFLOW_CONFIG_ENABLE_EXPOSE_CONFIG | MOCK_CONFIG_OVERRIDE): + # since the endpoint calls `conf_dict.clear()` to remove extra keys, + # use `new` instead of `return_value` to avoid side effects + with patch( + "airflow.api_fastapi.core_api.routes.public.config.conf.as_dict", + new=lambda **_: MOCK_CONFIG_DICT.copy(), + ): + yield + + +class TestGetConfig(TestConfigEndpoint): + @pytest.mark.parametrize( + "section, headers, expected_status_code, expected_response", + [ + ( + None, + HEADERS_JSON, + 200, + { + "sections": [ + { + "name": SECTION_CORE, + "options": [ + {"key": OPTION_KEY_PARALLELISM, "value": OPTION_VALUE_PARALLELISM}, + ], + }, + { + "name": SECTION_SMTP, + "options": [ + {"key": OPTION_KEY_SMTP_HOST, "value": OPTION_VALUE_SMTP_HOST}, + {"key": OPTION_KEY_SMTP_MAIL_FROM, "value": OPTION_VALUE_SMTP_MAIL_FROM}, + ], + }, + { + "name": SECTION_DATABASE, + "options": [ + {"key": OPTION_KEY_SQL_ALCHEMY_CONN, "value": OPTION_VALUE_SQL_ALCHEMY_CONN}, + ], + }, + ], + }, + ), + ( + None, + HEADERS_TEXT, + 200, + textwrap.dedent( + f"""\ + [{SECTION_CORE}] + {OPTION_KEY_PARALLELISM} = {OPTION_VALUE_PARALLELISM} + + [{SECTION_SMTP}] + {OPTION_KEY_SMTP_HOST} = {OPTION_VALUE_SMTP_HOST} + {OPTION_KEY_SMTP_MAIL_FROM} = {OPTION_VALUE_SMTP_MAIL_FROM} + + [{SECTION_DATABASE}] + {OPTION_KEY_SQL_ALCHEMY_CONN} = {OPTION_VALUE_SQL_ALCHEMY_CONN} + """ + ), + ), + ( + None, + HEADERS_INVALID, + 422, + { + "detail": [ + { + "type": "literal_error", + "loc": ["header", "content-type"], + "msg": "Input should be 'application/json' or 'text/plain'", + "input": "invalid", + "ctx": {"expected": "'application/json' or 'text/plain'"}, + } + ] + }, + ), + ( + SECTION_CORE, + HEADERS_JSON, + 200, + { + "sections": [ + { + "name": SECTION_CORE, + "options": [ + {"key": OPTION_KEY_PARALLELISM, "value": OPTION_VALUE_PARALLELISM}, + ], + }, + ], + }, + ), + ( + SECTION_SMTP, + HEADERS_TEXT, + 200, + textwrap.dedent( + f"""\ + [{SECTION_SMTP}] + {OPTION_KEY_SMTP_HOST} = {OPTION_VALUE_SMTP_HOST} + {OPTION_KEY_SMTP_MAIL_FROM} = {OPTION_VALUE_SMTP_MAIL_FROM} + """ + ), + ), + ( + SECTION_DATABASE, + HEADERS_JSON, + 200, + { + "sections": [ + { + "name": SECTION_DATABASE, + "options": [ + {"key": OPTION_KEY_SQL_ALCHEMY_CONN, "value": OPTION_VALUE_SQL_ALCHEMY_CONN}, + ], + }, + ], + }, + ), + (None, HEADERS_JSON, 403, FORBIDDEN_RESPONSE), + (SECTION_CORE, HEADERS_JSON, 403, FORBIDDEN_RESPONSE), + (SECTION_NOT_EXIST, HEADERS_JSON, 404, {"detail": f"Section {SECTION_NOT_EXIST} not found."}), + ], + ) + def test_get_config(self, test_client, section, headers, expected_status_code, expected_response): + query_params = {"section": section} if section else None + if expected_status_code == 403: + with conf_vars(AIRFLOW_CONFIG_DISABLE_EXPOSE_CONFIG): + response = test_client.get("/public/config/", headers=headers, params=query_params) + else: + response = test_client.get("/public/config/", headers=headers, params=query_params) Review Comment: I think we are missing a test with `non-sensitive-only` ? ########## airflow/api_fastapi/core_api/routes/public/config.py: ########## @@ -0,0 +1,121 @@ +# 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 + +from typing import Literal + +from fastapi import Header, HTTPException, status +from fastapi.responses import Response + +from airflow.api_fastapi.common.router import AirflowRouter +from airflow.api_fastapi.core_api.datamodels.config import ( + Config, + ConfigOption, + ConfigSection, +) +from airflow.api_fastapi.core_api.openapi.exceptions import create_openapi_http_exception_doc +from airflow.configuration import conf + + +def _check_expose_config() -> bool: + display_sensitive: bool | None = None + if conf.get("webserver", "expose_config").lower() == "non-sensitive-only": + expose_config = True + display_sensitive = False + else: + expose_config = conf.getboolean("webserver", "expose_config") + display_sensitive = True + + if not expose_config: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Your Airflow administrator chose not to expose the configuration, most likely for security reasons.", + ) + return display_sensitive + + +config_router = AirflowRouter(tags=["Config"], prefix="/config") + + +@config_router.get( + "/", + responses=create_openapi_http_exception_doc( + [status.HTTP_401_UNAUTHORIZED, status.HTTP_403_FORBIDDEN, status.HTTP_404_NOT_FOUND] + ), Review Comment: You should add example responses because there is no way for FastAPI to populate correctly the openapi spec and swagger based on that. You can take a look at how we do it in `get_dag_source`. (We combine `responses` for text/plain and `response_model` for application/json documentation) -- 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]
