anmolxlight commented on code in PR #64749:
URL: https://github.com/apache/airflow/pull/64749#discussion_r3089222677
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/eks.py:
##########
@@ -82,20 +82,24 @@ class NodegroupStates(Enum):
# Load credentials from secure file using (POSIX-compliant dot
operator)
. {credentials_file}
+ # Redirect stderr to /dev/null to prevent Python warnings,
deprecation
+ # notices, or other log output from contaminating stdout. The token
+ # output must be the ONLY thing on stdout for bash parsing to work.
output=$({python_executable} -m
airflow.providers.amazon.aws.utils.eks_get_token \
- --cluster-name {eks_cluster_name} --sts-url '{sts_url}' {args}
2>&1)
+ --cluster-name {eks_cluster_name} --sts-url '{sts_url}' {args}
2>/dev/null)
Review Comment:
Good point, but I've kept stderr -> /dev/null. The stderr output from
eks_get_token (warnings, botocore debug messages) is not useful for users
debugging credential failures. The non-zero exit path preserves all stdout for
troubleshooting. See my reply to o-nikolas for the full reasoning.
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/eks.py:
##########
@@ -82,20 +82,24 @@ class NodegroupStates(Enum):
# Load credentials from secure file using (POSIX-compliant dot
operator)
. {credentials_file}
+ # Redirect stderr to /dev/null to prevent Python warnings,
deprecation
+ # notices, or other log output from contaminating stdout. The token
+ # output must be the ONLY thing on stdout for bash parsing to work.
output=$({python_executable} -m
airflow.providers.amazon.aws.utils.eks_get_token \
- --cluster-name {eks_cluster_name} --sts-url '{sts_url}' {args}
2>&1)
+ --cluster-name {eks_cluster_name} --sts-url '{sts_url}' {args}
2>/dev/null)
status=$?
# Clear environment variables after use (defense in depth)
unset AWS_ACCESS_KEY_ID AWS_SECRET_ACCESS_KEY AWS_SESSION_TOKEN
if [ "$status" -ne 0 ]; then
- printf '%s' "$output" >&2
+ printf 'eks_get_token failed with exit code %s' "$status" >&2
Review Comment:
Fixed! The new non-zero exit message is: `printf 'eks_get_token failed with
exit code %s. Output was: %s' "$status" "$output" >&2`. This includes the full
output for troubleshooting credential/STS issues. Resolved in the v2 PR:
apache/airflow#65335.
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/eks.py:
##########
@@ -104,6 +108,16 @@ class NodegroupStates(Enum):
token=${{last_line##*, token: }} # text after ", token: "
+ # Validate that token was extracted — empty token means parsing
failed
+ # or eks_get_token produced unexpected output. Without this check,
a
+ # malformed ExecCredential is sent to the API server, resulting in
a
+ # 401 with an empty user identity in the audit logs.
+ if [ -z "$token" ]; then
+ printf 'Failed to extract token from eks_get_token output. '
>&2
+ printf 'Output was: %s' "$output" >&2
Review Comment:
Critical catch — thank you! Fixed in v2 PR apache/airflow#65335. The
empty-token error branch now only prints `'Failed to extract token from
eks_get_token output.'` without the . No bearer token will leak into task logs.
##########
providers/amazon/tests/unit/amazon/aws/hooks/test_eks.py:
##########
@@ -1273,6 +1273,37 @@ def test_generate_config_file(self, mock_conn,
aws_conn_id, region_name, expecte
if expected_region_args:
assert expected_region_args in command_arg
+ def test_command_template_redirects_stderr(self):
+ """Verify COMMAND template redirects stderr to /dev/null to prevent
+ Python warnings/log output from contaminating stdout and breaking
+ bash token parsing. This is critical for cross-account AssumeRole
+ scenarios where the kubeconfig exec plugin must produce a clean
token."""
+ from airflow.providers.amazon.aws.hooks.eks import COMMAND
+
+ # Verify stderr is redirected to /dev/null, not merged with stdout
+ assert "2>/dev/null" in COMMAND, (
+ "COMMAND must redirect stderr to /dev/null to prevent output
contamination"
+ )
Review Comment:
Addressed in v2 PR apache/airflow#65335. The test now checks BOTH that
`2>&1` is absent (the core correctness requirement) AND that `2>/dev/null` is
present (the implementation). This is more robust — it catches the specific
problem while still asserting the current implementation.
##########
providers/amazon/tests/unit/amazon/aws/hooks/test_eks.py:
##########
@@ -1273,6 +1273,37 @@ def test_generate_config_file(self, mock_conn,
aws_conn_id, region_name, expecte
if expected_region_args:
assert expected_region_args in command_arg
+ def test_command_template_redirects_stderr(self):
+ """Verify COMMAND template redirects stderr to /dev/null to prevent
+ Python warnings/log output from contaminating stdout and breaking
+ bash token parsing. This is critical for cross-account AssumeRole
+ scenarios where the kubeconfig exec plugin must produce a clean
token."""
+ from airflow.providers.amazon.aws.hooks.eks import COMMAND
+
+ # Verify stderr is redirected to /dev/null, not merged with stdout
+ assert "2>/dev/null" in COMMAND, (
+ "COMMAND must redirect stderr to /dev/null to prevent output
contamination"
+ )
+ assert "2>&1" not in COMMAND, (
+ "COMMAND must not use 2>&1 — merging stderr with stdout breaks
bash token parsing"
+ )
+
+ def test_command_template_validates_token(self):
+ """Verify COMMAND template validates that the token was successfully
+ extracted before producing the ExecCredential JSON. Without this check,
+ a malformed ExecCredential with an empty token is sent to the API
server,
+ resulting in 401 Unauthorized with an empty user identity in audit
logs."""
+ from airflow.providers.amazon.aws.hooks.eks import COMMAND
+
+ # Verify token validation check exists
+ assert 'if [ -z "$token" ]' in COMMAND, (
+ "COMMAND must validate that token is non-empty before producing
ExecCredential JSON"
+ )
+ # Verify it exits with error on empty token
+ assert "exit 1" in COMMAND or 'exit "$' in COMMAND, (
+ "COMMAND must exit with error when token extraction fails"
Review Comment:
Fixed in v2 PR apache/airflow#65335. The test now uses a simple `assert
'exit 1' in COMMAND` — the `exit 1` only appears in the empty-token validation
block (the `exit "$status"` uses different quoting), so this is unambiguous.
--
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]
