potiuk commented on code in PR #72355:
URL: https://github.com/apache/airflow/pull/72355#discussion_r4015125842
##########
shared/configuration/src/airflow_shared/configuration/parser.py:
##########
@@ -1602,16 +1602,35 @@ def getjson(
self, section: str, key: str, fallback=None, **kwargs
) -> dict | list | str | int | float | None:
"""
- Return a config value parsed from a JSON string.
+ Return a config value parsed from a JSON string or JSON file.
- ``fallback`` is *not* JSON parsed but used verbatim when no config
value is given.
+ If no direct config value is given for ``key``, attempts to read from
a file path
+ specified by ``key + '_file'``.
+
+ ``fallback`` is *not* JSON parsed but used verbatim when no config
value or file is given.
"""
try:
data = self.get(section=section, key=key, fallback=None,
_extra_stacklevel=1, **kwargs)
except (NoSectionError, NoOptionError):
data = None
if data is None or data == "":
+ try:
Review Comment:
This is the placement the dev-list thread landed on differently. Quoting it:
> I don't think we should limit it to just `getjson` though as your PR
currently does. The feature should live at the higher level that `_cmd` and
`_secret` live with all the same checks and allowlisting that those abide by.
> There is no particular reason to limit it to .json only, I can clearly see
the case where you want to read string or boolean values from files named by
the variables.
Concretely: a `_get_file_option()` sibling of `_get_cmd_option()`
(`parser.py:1154`) and `_get_secret_option()` (`parser.py:1172`), wired into
the same `get()` lookup chain those two are resolved in (around
`parser.py:1244` / `parser.py:1270`). The suffix then returns the file's
contents as the raw string value, every typed accessor (`get`, `getboolean`,
`getint`, `getfloat`, `getjson`) picks it up for free, and `getjson` needs no
change at all.
Full thread: https://lists.apache.org/thread/fpzwkdjxl1trs4q4hqbgj5w7vwz0kvyv
##########
shared/configuration/src/airflow_shared/configuration/parser.py:
##########
@@ -1602,16 +1602,35 @@ def getjson(
self, section: str, key: str, fallback=None, **kwargs
) -> dict | list | str | int | float | None:
"""
- Return a config value parsed from a JSON string.
+ Return a config value parsed from a JSON string or JSON file.
- ``fallback`` is *not* JSON parsed but used verbatim when no config
value is given.
+ If no direct config value is given for ``key``, attempts to read from
a file path
+ specified by ``key + '_file'``.
+
+ ``fallback`` is *not* JSON parsed but used verbatim when no config
value or file is given.
"""
try:
data = self.get(section=section, key=key, fallback=None,
_extra_stacklevel=1, **kwargs)
except (NoSectionError, NoOptionError):
data = None
if data is None or data == "":
+ try:
+ file_path = self.get(
+ section=section, key=key + "_file", fallback=None,
_extra_stacklevel=1, **kwargs
+ )
+ except (NoSectionError, NoOptionError):
+ file_path = None
+
+ if file_path:
Review Comment:
This is the "all the same checks and allowlisting" half of the dev-list
request, and it is easy to miss, so calling it out on its own. Both siblings
open with the same guard:
```python
def _get_cmd_option(self, section: str, key: str):
"""Get config option from command execution."""
fallback_key = key + "_cmd"
if (section, key) in self.sensitive_config_values:
```
`sensitive_config_values` is not only an opt-in list — it also drives
masking (`parser.py:638`) and is what `as_dict()` keys its `include_cmds` /
`include_secret` expansion off. This branch consults no such set, so any
`[section] anykey_file` is opened and read, and the resolved value participates
in none of that machinery.
To be clear on severity: this is **not** a privilege-escalation issue.
Anyone who can write `airflow.cfg` or set `AIRFLOW__*` is a Deployment Manager
who already has full control, which the security model puts out of scope. It
matters for consistency and masking behaviour. Moving the feature into the
generic chain resolves it by construction.
##########
shared/configuration/src/airflow_shared/configuration/parser.py:
##########
@@ -1602,16 +1602,35 @@ def getjson(
self, section: str, key: str, fallback=None, **kwargs
) -> dict | list | str | int | float | None:
"""
- Return a config value parsed from a JSON string.
+ Return a config value parsed from a JSON string or JSON file.
- ``fallback`` is *not* JSON parsed but used verbatim when no config
value is given.
+ If no direct config value is given for ``key``, attempts to read from
a file path
+ specified by ``key + '_file'``.
+
+ ``fallback`` is *not* JSON parsed but used verbatim when no config
value or file is given.
"""
try:
data = self.get(section=section, key=key, fallback=None,
_extra_stacklevel=1, **kwargs)
except (NoSectionError, NoOptionError):
data = None
if data is None or data == "":
+ try:
+ file_path = self.get(
+ section=section, key=key + "_file", fallback=None,
_extra_stacklevel=1, **kwargs
+ )
+ except (NoSectionError, NoOptionError):
+ file_path = None
+
+ if file_path:
+ try:
+ with open(file_path) as f:
+ return json.load(f)
+ except Exception as e:
Review Comment:
`except Exception` is wider than it needs to be here — it also turns genuine
programming errors (a non-string path yielding `TypeError`, say) into an
`AirflowConfigException`. The string branch a few lines below narrows to
`JSONDecodeError`; `except (OSError, ValueError)` would match that convention
and still cover both file-IO and JSON-decode failures, since `JSONDecodeError`
subclasses `ValueError`.
##########
shared/configuration/tests/configuration/test_parser.py:
##########
@@ -338,6 +338,49 @@ def test_getjson_fallback(self, fallback):
assert test_conf.getjson("test", "json", fallback=fallback) == fallback
+ def test_getjson_from_file(self, tmp_path):
Review Comment:
These three cover the happy path, the env-var path and invalid JSON. The two
branches most likely to regress are not covered:
1. **Precedence** — both `key` and `key_file` set. The direct value should
win; that is the `data is None or data == ""` condition doing real work, and
nothing pins it down.
2. **Missing file** — `key_file` pointing at a path that does not exist.
Today that raises rather than returning `fallback`. That is a defensible
choice, but it is currently neither documented in the docstring nor tested, so
it is easy to change by accident.
--
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]
