You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/04/05 15:28:37 UTC
[GitHub] [airflow] tirkarthi opened a new pull request, #22754: Fix secrets rendered in UI when task is not executed.
tirkarthi opened a new pull request, #22754:
URL: https://github.com/apache/airflow/pull/22754
When the task instance is executed it fetches variables accessed in template and during variable access calls `mask_secret` add filter for secret variables. It also creates new `RenderedTaskInstanceFields` where the masked values are stored by calling `redact`. Subsequent rendering of executed tasks use `RenderedTaskInstanceFields` and redacted values.
When the task instance is not executed and when we try to render then `RenderedTaskInstanceFields` is not created and rendering template calls `mask_secret` to add filters but the output itself is not masked. So we have to use `RenderedTaskInstanceFields` where initialization does template rendering and also masks secrets.
closes: #22738
related: #22738
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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
[GitHub] [airflow] uranusjr commented on a diff in pull request #22754: Fix secrets rendered in UI when task is not executed.
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #22754:
URL: https://github.com/apache/airflow/pull/22754#discussion_r843610043
##########
airflow/models/taskinstance.py:
##########
@@ -2069,15 +2069,27 @@ def get_rendered_template_fields(self, session: Session = NEW_SESSION) -> None:
setattr(self.task, field_name, rendered_value)
self.task = task
return
+
try:
- self.render_templates()
+ # Task was never executed. Initialize RenderedTaskInstanceFields
+ # to render template and mask secrets. Set MASK_SECRETS_IN_LOGS
+ # to True to enable masking similar to task run.
+ original_value = settings.MASK_SECRETS_IN_LOGS
+ settings.MASK_SECRETS_IN_LOGS = True
Review Comment:
What code does this value affect? This monkeypatching behaviour looks too hacky from a quick glance.
--
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
[GitHub] [airflow] ephraimbuddy merged pull request #22754: Fix secrets rendered in UI when task is not executed.
Posted by GitBox <gi...@apache.org>.
ephraimbuddy merged PR #22754:
URL: https://github.com/apache/airflow/pull/22754
--
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
[GitHub] [airflow] tirkarthi commented on a diff in pull request #22754: Fix secrets rendered in UI when task is not executed.
Posted by GitBox <gi...@apache.org>.
tirkarthi commented on code in PR #22754:
URL: https://github.com/apache/airflow/pull/22754#discussion_r858261361
##########
airflow/models/taskinstance.py:
##########
@@ -2069,15 +2069,27 @@ def get_rendered_template_fields(self, session: Session = NEW_SESSION) -> None:
setattr(self.task, field_name, rendered_value)
self.task = task
return
+
try:
- self.render_templates()
+ # Task was never executed. Initialize RenderedTaskInstanceFields
+ # to render template and mask secrets. Set MASK_SECRETS_IN_LOGS
+ # to True to enable masking similar to task run.
+ original_value = settings.MASK_SECRETS_IN_LOGS
+ settings.MASK_SECRETS_IN_LOGS = True
Review Comment:
static check should be fixed now and rebased to latest main branch. Sorry, I didn't have docker setup for running pre-commit while starting PR.
Regarding moving the `MASK_SECRETS_IN_LOGS` check to `filter` method in `SecretsMasker` I saw some test errors on the change. This change also set and reset the variable in finally block on per request basis like task command in cli that does it per invocation.
--
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
[GitHub] [airflow] tirkarthi commented on pull request #22754: Fix secrets rendered in UI when task is not executed.
Posted by GitBox <gi...@apache.org>.
tirkarthi commented on PR #22754:
URL: https://github.com/apache/airflow/pull/22754#issuecomment-1089094001
I have set `MASK_SECRETS_IN_LOGS` to be True and reverting to original value in finally clause since this seems to be the approach in line with `airflow tasks run`.
--
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
[GitHub] [airflow] ashb commented on a diff in pull request #22754: Fix secrets rendered in UI when task is not executed.
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #22754:
URL: https://github.com/apache/airflow/pull/22754#discussion_r843760613
##########
airflow/models/taskinstance.py:
##########
@@ -2069,15 +2069,27 @@ def get_rendered_template_fields(self, session: Session = NEW_SESSION) -> None:
setattr(self.task, field_name, rendered_value)
self.task = task
return
+
try:
- self.render_templates()
+ # Task was never executed. Initialize RenderedTaskInstanceFields
+ # to render template and mask secrets. Set MASK_SECRETS_IN_LOGS
+ # to True to enable masking similar to task run.
+ original_value = settings.MASK_SECRETS_IN_LOGS
+ settings.MASK_SECRETS_IN_LOGS = True
Review Comment:
```python
def mask_secret(secret: Union[str, dict, Iterable], name: Optional[str] = None) -> None:
"""
Mask a secret from appearing in the task logs.
If ``name`` is provided, then it will only be masked if the name matches
one of the configured "sensitive" names.
If ``secret`` is a dict or a iterable (excluding str) then it will be
recursively walked and keys with sensitive names will be hidden.
"""
# Delay import
from airflow import settings
# Filtering all log messages is not a free process, so we only do it when
# running tasks
if not settings.MASK_SECRETS_IN_LOGS or not secret:
return
_secrets_masker().add_mask(secret, name)
```
If that is False nothing is added to the masker.
I agree, we should change that behaviour so that instead we always add things to the mask list, and only have the filter do anything if that setting is True.
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #22754: Fix secrets rendered in UI when task is not executed.
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #22754:
URL: https://github.com/apache/airflow/pull/22754#discussion_r862381219
##########
airflow/models/taskinstance.py:
##########
@@ -2069,15 +2069,27 @@ def get_rendered_template_fields(self, session: Session = NEW_SESSION) -> None:
setattr(self.task, field_name, rendered_value)
self.task = task
return
+
try:
- self.render_templates()
+ # Task was never executed. Initialize RenderedTaskInstanceFields
+ # to render template and mask secrets. Set MASK_SECRETS_IN_LOGS
+ # to True to enable masking similar to task run.
+ original_value = settings.MASK_SECRETS_IN_LOGS
+ settings.MASK_SECRETS_IN_LOGS = True
Review Comment:
This one LGTM and fixes the not-nice behaviour @ashb @uranusjr - are you ok with that one 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
[GitHub] [airflow] github-actions[bot] commented on pull request #22754: Fix secrets rendered in UI when task is not executed.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22754:
URL: https://github.com/apache/airflow/pull/22754#issuecomment-1114027603
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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
[GitHub] [airflow] tirkarthi commented on a diff in pull request #22754: Fix secrets rendered in UI when task is not executed.
Posted by GitBox <gi...@apache.org>.
tirkarthi commented on code in PR #22754:
URL: https://github.com/apache/airflow/pull/22754#discussion_r843821720
##########
airflow/models/taskinstance.py:
##########
@@ -2069,15 +2069,27 @@ def get_rendered_template_fields(self, session: Session = NEW_SESSION) -> None:
setattr(self.task, field_name, rendered_value)
self.task = task
return
+
try:
- self.render_templates()
+ # Task was never executed. Initialize RenderedTaskInstanceFields
+ # to render template and mask secrets. Set MASK_SECRETS_IN_LOGS
+ # to True to enable masking similar to task run.
+ original_value = settings.MASK_SECRETS_IN_LOGS
+ settings.MASK_SECRETS_IN_LOGS = True
Review Comment:
I agree this is setting a flag that has the side_effect of masking it in template rendering which seems slightly unrelated. This setting is done in task run command and I tries to emulate this here. We can change signature to add a parameter like force but open to better approach. The suggestion to make this parameter to filter also seems good.
--
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
[GitHub] [airflow] uranusjr commented on pull request #22754: Fix secrets rendered in UI when task is not executed.
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #22754:
URL: https://github.com/apache/airflow/pull/22754#issuecomment-1116759560
Still doesn’t like the approach of this, but I guess it’s worthwhile having this in 2.3.1 instead of nothing.
Can someone open a follow-up issue to do this the “right way” instead? I can get on it later myself if no-one is up to it.
--
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
[GitHub] [airflow] tirkarthi commented on pull request #22754: Fix secrets rendered in UI when task is not executed.
Posted by GitBox <gi...@apache.org>.
tirkarthi commented on PR #22754:
URL: https://github.com/apache/airflow/pull/22754#issuecomment-1088945895
I guess similar to task run always setting `settings.MASK_SECRETS_IN_LOGS = True` we should also set here within context manager while rendering to ensure secrets are masked.
https://github.com/apache/airflow/blob/acb1a100bbf889dddef1702c95bd7262a578dfcc/airflow/cli/commands/task_command.py#L347
--
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
[GitHub] [airflow] tirkarthi commented on pull request #22754: Fix secrets rendered in UI when task is not executed.
Posted by GitBox <gi...@apache.org>.
tirkarthi commented on PR #22754:
URL: https://github.com/apache/airflow/pull/22754#issuecomment-1117378233
@uranusjr Agreed, it's not an elegant fix. I will open an issue for this after merge if someone doesn't have a better fix as part of further review.
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #22754: Fix secrets rendered in UI when task is not executed.
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #22754:
URL: https://github.com/apache/airflow/pull/22754#discussion_r858022270
##########
airflow/models/taskinstance.py:
##########
@@ -2069,15 +2069,27 @@ def get_rendered_template_fields(self, session: Session = NEW_SESSION) -> None:
setattr(self.task, field_name, rendered_value)
self.task = task
return
+
try:
- self.render_templates()
+ # Task was never executed. Initialize RenderedTaskInstanceFields
+ # to render template and mask secrets. Set MASK_SECRETS_IN_LOGS
+ # to True to enable masking similar to task run.
+ original_value = settings.MASK_SECRETS_IN_LOGS
+ settings.MASK_SECRETS_IN_LOGS = True
Review Comment:
Static check is failing.
--
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
[GitHub] [airflow] m1racoli commented on a diff in pull request #22754: Fix secrets rendered in UI when task is not executed.
Posted by GitBox <gi...@apache.org>.
m1racoli commented on code in PR #22754:
URL: https://github.com/apache/airflow/pull/22754#discussion_r855870387
##########
airflow/models/taskinstance.py:
##########
@@ -2069,15 +2069,27 @@ def get_rendered_template_fields(self, session: Session = NEW_SESSION) -> None:
setattr(self.task, field_name, rendered_value)
self.task = task
return
+
try:
- self.render_templates()
+ # Task was never executed. Initialize RenderedTaskInstanceFields
+ # to render template and mask secrets. Set MASK_SECRETS_IN_LOGS
+ # to True to enable masking similar to task run.
+ original_value = settings.MASK_SECRETS_IN_LOGS
+ settings.MASK_SECRETS_IN_LOGS = True
Review Comment:
Hey guys, thanks for taking the effort of fixing this. Do we know what's blocking us?
--
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