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:[email protected]: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:[email protected]:9200";, "", "", ("user", "pass")), + # Explicit credentials take priority over URL-embedded ones + ( + "https://url_user:[email protected]: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:[email protected]:443";, None, 443), + # Explicit port overrides URL port + ("https://user:[email protected]: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:[email protected]: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:[email protected]:9200";, "", "", ("user", "pass")), + # Explicit credentials take priority over URL-embedded ones + ( + "https://url_user:[email protected]: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://[email protected]";, "", "", ("user", ""),), # Only password is present. ( "https://:[email protected]";, "", "", ("", "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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
