You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "SameerMesiah97 (via GitHub)" <gi...@apache.org> on 2026/05/02 14:58:09 UTC
Re: [PR] Fix OpenSearch log handler to support single-URL opensearch secret from helm chart [airflow]
SameerMesiah97 commented on code in PR #66051:
URL: https://github.com/apache/airflow/pull/66051#discussion_r3176737976
##########
providers/opensearch/docs/changelog.rst:
##########
@@ -27,6 +27,21 @@
Changelog
---------
+The OpenSearch log handler now fully supports a single-URL deployment style where
+``AIRFLOW__OPENSEARCH__HOST`` (e.g. ``https://user:password@opensearch.example.com:443``) is the
+only variable that needs to be set. Two bugs prevented this from working:
+
+* **Credentials silently discarded** — when ``AIRFLOW__OPENSEARCH__USERNAME`` /
+ ``AIRFLOW__OPENSEARCH__PASSWORD`` were absent, the userinfo embedded in the host URL
+ (``user:password@``) was stripped before building the OpenSearch client but never passed as
+ ``http_auth``, causing 401 errors. Credentials are now extracted from the URL and used for
+ authentication when the dedicated config keys are empty.
+
+* **Port ignored** — ``AIRFLOW__OPENSEARCH__PORT`` defaulted to ``9200`` in the logging-config
+ template, so a port embedded in the host URL (e.g. ``:443``) was silently overridden. The
+ default is now ``None``; when no explicit port is configured, the port is taken from the host
+ URL, falling back to ``9200`` only when the URL carries no port either.
+
Review Comment:
The changelog entry seems accurate but this is too much detail. These are meant to be user-facing summaries rather than explanations of internal behavior. I would suggest shortening this to focus on the observable fixes (credentials + port handling) rather than implementation details. Perhaps this might be better:
```
Fix OpenSearch log handler to properly support single-URL configuration
with embedded credentials and port. Previously, credentials could be
ignored and ports overridden by defaults.
```
##########
providers/opensearch/src/airflow/providers/opensearch/log/os_task_handler.py:
##########
@@ -190,9 +190,13 @@ def _create_opensearch_client(
) -> OpenSearch:
parsed_url = urlparse(_format_url(host))
resolved_port = port if port is not None else (parsed_url.port or 9200)
+ # Fall back to credentials embedded in the host URL (e.g. https://user:pass@host:port)
+ # when explicit username/password are not provided.
+ effective_username = username or parsed_url.username or ""
+ effective_password = password or parsed_url.password or ""
return OpenSearch(
hosts=[{"host": parsed_url.hostname, "port": resolved_port, "scheme": parsed_url.scheme}],
- http_auth=(username, password),
+ http_auth=(effective_username, effective_password),
Review Comment:
what about cases where `effective_username` is "" and `effective_password` is present? Or vice versa? Perhaps you could do this:
```
http_auth = (
(effective_username, effective_password)
if effective_username
else None
)
```
My understanding is empty password + non-empty username is valid but the opposite or both being empty are not.
##########
providers/opensearch/tests/unit/opensearch/log/test_os_task_handler.py:
##########
@@ -531,6 +532,55 @@ def test_get_index_patterns_with_callable(self):
assert result == "callable_index_pattern"
+class TestCreateOpensearchClient:
+ @pytest.mark.parametrize(
+ ("host", "username", "password", "expected_auth"),
+ [
+ # Explicit credentials are used as-is
+ ("http://opensearch.example.com:9200", "admin", "secret", ("admin", "secret")),
+ # Credentials embedded in host URL are extracted when explicit credentials are empty
+ ("https://user:pass@opensearch.example.com:9200", "", "", ("user", "pass")),
+ # Explicit credentials take priority over URL-embedded ones
+ (
+ "https://url_user:url_pass@opensearch.example.com:9200",
+ "explicit",
+ "creds",
+ ("explicit", "creds"),
+ ),
+ # No credentials anywhere — empty strings passed through
+ ("http://opensearch.example.com:9200", "", "", ("", "")),
+ ],
+ )
+ def test_credentials(self, host, username, password, expected_auth):
+ with patch("airflow.providers.opensearch.log.os_task_handler.OpenSearch") as mock_os:
+ _create_opensearch_client(host, None, username, password, {})
+ assert mock_os.call_args.kwargs["http_auth"] == expected_auth
+
+ @pytest.mark.parametrize(
+ ("host", "explicit_port", "expected_port"),
+ [
+ # Port from URL when explicit port is None
+ ("https://user:pass@opensearch.example.com:443", None, 443),
+ # Explicit port overrides URL port
+ ("https://user:pass@opensearch.example.com:443", 9200, 9200),
+ # Default fallback when neither explicit nor URL port
+ ("https://opensearch.example.com", None, 9200),
+ ],
+ )
+ def test_port(self, host, explicit_port, expected_port):
+ with patch("airflow.providers.opensearch.log.os_task_handler.OpenSearch") as mock_os:
+ _create_opensearch_client(host, explicit_port, "", "", {})
+ hosts = mock_os.call_args.kwargs["hosts"]
+ assert hosts[0]["port"] == expected_port
+
+ def test_url_credentials_do_not_appear_in_host_list(self):
+ """Userinfo must be stripped from the host entry passed to the OpenSearch client."""
+ with patch("airflow.providers.opensearch.log.os_task_handler.OpenSearch") as mock_os:
+ _create_opensearch_client("https://user:pass@opensearch.example.com:9200", None, "", "", {})
+ hosts = mock_os.call_args.kwargs["hosts"]
+ assert hosts == [{"host": "opensearch.example.com", "port": 9200, "scheme": "https"}]
+
Review Comment:
If you decide to introduce hostname validation, you can add this test as well:
```
def test_invalid_hostname_raises(self):
with pytest.raises(ValueError):
_create_opensearch_client("http://:9200", None, "", "", {})
```
I would add a `match` as well to `pytest.raises` to assert the error message.
##########
providers/opensearch/src/airflow/providers/opensearch/log/os_task_handler.py:
##########
Review Comment:
_format_url handles scheme + netloc which is fine for most cases, but we rely on parsed_url.hostname downstream. Might be worth validating that explicitly to avoid cases where netloc exists but hostname is None (e.g. malformed userinfo).
##########
providers/opensearch/tests/unit/opensearch/log/test_os_task_handler.py:
##########
@@ -531,6 +532,55 @@ def test_get_index_patterns_with_callable(self):
assert result == "callable_index_pattern"
+class TestCreateOpensearchClient:
+ @pytest.mark.parametrize(
+ ("host", "username", "password", "expected_auth"),
+ [
+ # Explicit credentials are used as-is
+ ("http://opensearch.example.com:9200", "admin", "secret", ("admin", "secret")),
+ # Credentials embedded in host URL are extracted when explicit credentials are empty
+ ("https://user:pass@opensearch.example.com:9200", "", "", ("user", "pass")),
+ # Explicit credentials take priority over URL-embedded ones
+ (
+ "https://url_user:url_pass@opensearch.example.com:9200",
+ "explicit",
+ "creds",
+ ("explicit", "creds"),
+ ),
+ # No credentials anywhere — empty strings passed through
+ ("http://opensearch.example.com:9200", "", "", ("", "")),
+ ],
+ )
Review Comment:
Could you add these 2 cases here;
```
# Only username is present.
("https://user@opensearch.example.com", "", "", ("user", ""),),
# Only password is present.
( "https://:pass@opensearch.example.com", "", "", ("", "pass"),),
```
We need to handle partial credentials too.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org