Re: [PR] fix: EksPodOperator 401 with cross-account AssumeRole via aws_conn_id [airflow]
vincbeck merged PR #65335: URL: https://github.com/apache/airflow/pull/65335 -- 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]
Re: [PR] fix: EksPodOperator 401 with cross-account AssumeRole via aws_conn_id [airflow]
ferruzzi commented on PR #65335: URL: https://github.com/apache/airflow/pull/65335#issuecomment-4511247174 @vincbeck - can you re-review when you get a minute? -- 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]
Re: [PR] fix: EksPodOperator 401 with cross-account AssumeRole via aws_conn_id [airflow]
ferruzzi commented on code in PR #65335:
URL: https://github.com/apache/airflow/pull/65335#discussion_r3236061912
##
providers/amazon/src/airflow/providers/amazon/aws/hooks/eks.py:
##
@@ -82,20 +82,25 @@ 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 token 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. Output was:
%s' \
+"$status" "$output" >&2
Review Comment:
Seems like a solid test to me, thanks for confirming that.
--
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]
Re: [PR] fix: EksPodOperator 401 with cross-account AssumeRole via aws_conn_id [airflow]
anmolxlight commented on code in PR #65335:
URL: https://github.com/apache/airflow/pull/65335#discussion_r3235104377
##
providers/amazon/src/airflow/providers/amazon/aws/hooks/eks.py:
##
@@ -82,20 +82,25 @@ 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 token 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. Output was:
%s' \
+"$status" "$output" >&2
Review Comment:
Yes — I tested the shell behavior separately. `output=$(cmd
2>"$stderr_file")` keeps stdout token parsing clean, preserves the command exit
status, and still leaves stderr available for the failure message. I did not
run it against a live EKS cluster.
--
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]
Re: [PR] fix: EksPodOperator 401 with cross-account AssumeRole via aws_conn_id [airflow]
ferruzzi commented on code in PR #65335:
URL: https://github.com/apache/airflow/pull/65335#discussion_r3230064921
##
providers/amazon/src/airflow/providers/amazon/aws/hooks/eks.py:
##
@@ -82,20 +82,25 @@ 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 token 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. Output was:
%s' \
+"$status" "$output" >&2
Review Comment:
Cool. Did you happen to test this? I'm only like 90% sure that it is the
right fix. Piping output can get confusing, I just want to double check.
Edit: Especially since that was exactly my mistake in the first place
:sweat_smile: :
--
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]
Re: [PR] fix: EksPodOperator 401 with cross-account AssumeRole via aws_conn_id [airflow]
ferruzzi commented on code in PR #65335:
URL: https://github.com/apache/airflow/pull/65335#discussion_r3230064921
##
providers/amazon/src/airflow/providers/amazon/aws/hooks/eks.py:
##
@@ -82,20 +82,25 @@ 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 token 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. Output was:
%s' \
+"$status" "$output" >&2
Review Comment:
Cool. Did you happen to test this? I'm only like 90% sure that it is the
right fix. Piping output can get confusing, I just want to double check.
--
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]
Re: [PR] fix: EksPodOperator 401 with cross-account AssumeRole via aws_conn_id [airflow]
anmolxlight commented on code in PR #65335:
URL: https://github.com/apache/airflow/pull/65335#discussion_r3224083073
##
providers/amazon/src/airflow/providers/amazon/aws/hooks/eks.py:
##
@@ -82,20 +82,25 @@ 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 token 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:
Fixed. stderr now goes to a temporary file instead of /dev/null, so stdout
stays clean for token parsing on success while the captured stderr is emitted
if eks_get_token exits nonzero.
##
providers/amazon/src/airflow/providers/amazon/aws/hooks/eks.py:
##
@@ -82,20 +82,25 @@ 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 token 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. Output was:
%s' \
+"$status" "$output" >&2
Review Comment:
Adjusted this to avoid printing stdout on failure. Since stdout can contain
the token, the error path now reports only the exit code plus captured stderr
from the temp file.
--
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]
Re: [PR] fix: EksPodOperator 401 with cross-account AssumeRole via aws_conn_id [airflow]
ferruzzi commented on code in PR #65335:
URL: https://github.com/apache/airflow/pull/65335#discussion_r3222853635
##
providers/amazon/src/airflow/providers/amazon/aws/hooks/eks.py:
##
@@ -82,20 +82,25 @@ 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 token 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:
I may be missing something, but isn't this going to dump the error messages
if there is an error and result in the user seeing `eks_get_token failed with
exit code 1. Output was:` with no reason since it got tossed into `/dev/null`?
Maybe as a compromise should we write to a tempfile and read that file back on
error? That way it's a clean output in the happy case and we still get the
error messages?
##
providers/amazon/src/airflow/providers/amazon/aws/hooks/eks.py:
##
@@ -82,20 +82,25 @@ 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 token 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. Output was:
%s' \
+"$status" "$output" >&2
Review Comment:
If I am reading your description right, there is a concern that the token
may be printed. Could it be printed here as part of `$output`? Should this be
masked or scrubbed or something?
--
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]
Re: [PR] fix: EksPodOperator 401 with cross-account AssumeRole via aws_conn_id [airflow]
anmolxlight commented on PR #64749: URL: https://github.com/apache/airflow/pull/64749#issuecomment-4260494672 > In the future please keep the same PR, it helps reviewers to track the progress and context alright -- 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]
Re: [PR] fix: EksPodOperator 401 with cross-account AssumeRole via aws_conn_id [airflow]
vincbeck commented on PR #65335: URL: https://github.com/apache/airflow/pull/65335#issuecomment-4260346574 Static checks failure is very much related to this change -- 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]
Re: [PR] fix: EksPodOperator 401 with cross-account AssumeRole via aws_conn_id [airflow]
vincbeck commented on PR #64749: URL: https://github.com/apache/airflow/pull/64749#issuecomment-4260351660 In the future please keep the same PR, it helps reviewers to track the progress and context -- 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]
Re: [PR] fix: EksPodOperator 401 with cross-account AssumeRole via aws_conn_id [airflow]
anmolxlight commented on PR #65335: URL: https://github.com/apache/airflow/pull/65335#issuecomment-4256117249 The "CI image checks / Static checks" failure is a pre-existing CI infrastructure issue unrelated to these changes — the workflow cannot extract uv/prek versions from uv.lock (those packages aren't declared in the lockfile at expected versions). All other jobs (MyPy, unit tests, build checks) passed. Please re-run or advise on next steps. -- 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]
Re: [PR] fix: EksPodOperator 401 with cross-account AssumeRole via aws_conn_id [airflow]
anmolxlight commented on PR #64749: URL: https://github.com/apache/airflow/pull/64749#issuecomment-4255303243 Closing this PR — superseded by apache/airflow#65335 which addresses all review feedback including a security fix for potential EKS bearer token leakage in error logs. -- 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]
Re: [PR] fix: EksPodOperator 401 with cross-account AssumeRole via aws_conn_id [airflow]
anmolxlight closed pull request #64749: fix: EksPodOperator 401 with cross-account AssumeRole via aws_conn_id URL: https://github.com/apache/airflow/pull/64749 -- 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]
Re: [PR] fix: EksPodOperator 401 with cross-account AssumeRole via aws_conn_id [airflow]
anmolxlight commented on PR #64749: URL: https://github.com/apache/airflow/pull/64749#issuecomment-4255302397 Hi vincbeck! Yes, I've addressed all the open review comments. Here's what I found and fixed: **Genuine issues addressed:** 1. **Security (CRITICAL)**: Copilot caught that the empty-token error branch was printing $output which includes the EKS bearer token — a credential leak into task logs. Fixed by removing $output from that error message. 2. **Diagnostic regression**: Copilot noted the non-zero exit path lost $output. Fixed by restoring it in a clear error message. 3. **Test tightness**: Copilot's suggestions for more robust test assertions were valid — the v2 PR has improved assertions. 4. **o-nikolas**: The /dev/null vs /tmp concern — kept /dev/null intentionally (stderr we discard is not actionable; non-zero exit covers failures). **New PR**: apache/airflow#65335 — supersedes #64749 with all review feedback addressed. -- 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]
Re: [PR] fix: EksPodOperator 401 with cross-account AssumeRole via aws_conn_id [airflow]
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_nam
Re: [PR] fix: EksPodOperator 401 with cross-account AssumeRole via aws_conn_id [airflow]
anmolxlight commented on code in PR #64749:
URL: https://github.com/apache/airflow/pull/64749#discussion_r3089221461
##
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:
Thanks for the review! You're right that `/dev/null` loses some diagnostic
info, but I've kept it for two reasons:
1. The stderr output we'd discard (Python warnings, botocore debug messages,
deprecation notices) is not actionable for users in this context — it doesn't
help them understand what's gone wrong with their credential configuration.
2. The non-zero exit path now preserves all diagnostic output from the
command (``) in the error message, so credential/STS failures are still fully
troubleshootable.
The key insight is that the stderr contamination problem only affects the
**success** path (when eks_get_token outputs a valid token but with extra noise
on stderr that gets merged into stdout and breaks parsing). On the failure
path, we now capture and report everything.
--
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]
Re: [PR] fix: EksPodOperator 401 with cross-account AssumeRole via aws_conn_id [airflow]
vincbeck commented on PR #64749: URL: https://github.com/apache/airflow/pull/64749#issuecomment-4252778186 Thanks @anmolxlight for creating this fix. There are many open conversations, have you had any chance to look at them? -- 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]
Re: [PR] fix: EksPodOperator 401 with cross-account AssumeRole via aws_conn_id [airflow]
Copilot commented on code in PR #64749:
URL: https://github.com/apache/airflow/pull/64749#discussion_r3066481518
##
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:
Redirecting `eks_get_token` stderr to `/dev/null` discards useful error
output (stack traces, botocore messages) and makes kubeconfig exec failures
hard to debug. Since the original parsing issue was caused by merging stderr
into stdout (`2>&1`), consider removing the redirection entirely (let stderr
pass through) or capture stderr separately and only surface it on non-zero
exit, while keeping stdout clean for parsing.
##
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:
The empty-token branch prints the full `eks_get_token` output to stderr.
That output includes the EKS bearer token (see `eks_get_token.py` printing
`token: {access_token}`), so this will leak credentials into task logs when
parsing fails. Please redact the token before logging (or omit the output
entirely) to avoid exposing bearer tokens.
```suggestion
printf 'Failed to extract token from eks_get_token output.'
>&2
```
##
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:
`assert "exit 1" in COMMAND or 'exit "$' in COMMAND` can pass even if the
empty-token validation never exits, because `'exit "$'` matches the existing
`exit "$status"` earlier in the script. Tighten this assertion to specifically
verify that the empty-token block exits (e.g., by checking ordering relative to
`if [ -z "$token" ]` or matching the `exit 1` inside that block).
```suggestion
# Verify the empty-token validation block exits with an error
assert 'if [ -z "$tok
Re: [PR] fix: EksPodOperator 401 with cross-account AssumeRole via aws_conn_id [airflow]
o-nikolas commented on code in PR #64749:
URL: https://github.com/apache/airflow/pull/64749#discussion_r3048670970
##
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:
Should we not pipe stderr output above to a durable location (perhaps
something in /tmp) instead of `/dev/null` and then combine it with the stdout
here? The status code alone is not very helpful.
--
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]
Re: [PR] fix: EksPodOperator 401 with cross-account AssumeRole via aws_conn_id [airflow]
boring-cyborg[bot] commented on PR #64749: URL: https://github.com/apache/airflow/pull/64749#issuecomment-4189582288 Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst) Here are some useful points: - Pay attention to the quality of your code (ruff, mypy and type annotations). Our [prek-hooks]( https://github.com/apache/airflow/blob/main/contributing-docs/08_static_code_checks.rst#prerequisites-for-prek-hooks) will help you with that. - In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/airflow-core/docs/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it. - Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/dev/breeze/doc/README.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations. - Be patient and persistent. It might take some time to get a review or get the final approval from Committers. - Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack. - Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#coding-style-and-best-practices). - Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits. Apache Airflow is a community-driven project and together we are making it better 🚀. In case of doubts contact the developers at: Mailing List: [email protected] Slack: https://s.apache.org/airflow-slack -- 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]
