Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AIRFLOW-5444] Fix action_logging so that request.form for POST is logged #6064

Merged
merged 1 commit into from
Oct 4, 2019

Conversation

yuqian90
Copy link
Contributor

@yuqian90 yuqian90 commented Sep 9, 2019

  • Log request.form when request.method is POST
  • Add a test for action_logging

Make sure you have checked all steps below.

Jira

Description

  • Here are some details about my PR, including screenshots of any UI changes:
    When user performs actions such as "clear", "success" or "failed" on the WebUI, important parameters such as dag_id, task_id and execution_date are not logged by action_logging because the values are in request.form rather than request.args. This PR fixes the issue.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Added tests.www.test_views:TestDecorators to check for the scenario where request.method is GET and POST.

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

@feluelle feluelle added the area:webserver Webserver related Issues label Sep 9, 2019
@codecov-io
Copy link

codecov-io commented Sep 9, 2019

Codecov Report

Merging #6064 into master will decrease coverage by 0.39%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #6064     +/-   ##
=========================================
- Coverage   80.04%   79.65%   -0.4%     
=========================================
  Files         607      607             
  Lines       35033    35033             
=========================================
- Hits        28043    27904    -139     
- Misses       6990     7129    +139
Impacted Files Coverage Δ
airflow/www/decorators.py 74.5% <100%> (ø) ⬆️
airflow/operators/postgres_operator.py 0% <0%> (-100%) ⬇️
airflow/kubernetes/volume_mount.py 44.44% <0%> (-55.56%) ⬇️
airflow/kubernetes/volume.py 52.94% <0%> (-47.06%) ⬇️
airflow/kubernetes/pod_launcher.py 45.25% <0%> (-46.72%) ⬇️
airflow/kubernetes/kube_client.py 33.33% <0%> (-41.67%) ⬇️
...rflow/contrib/operators/kubernetes_pod_operator.py 70.14% <0%> (-28.36%) ⬇️
airflow/hooks/postgres_hook.py 94.73% <0%> (-1.76%) ⬇️
airflow/utils/sqlalchemy.py 91.52% <0%> (-1.7%) ⬇️
airflow/hooks/dbapi_hook.py 86.44% <0%> (-1.7%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2e06d0...c8c4a12. Read the comment docs.

Copy link
Member

@feluelle feluelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. But giving another one a chance to look it over. Maybe @ashb

airflow/www/decorators.py Outdated Show resolved Hide resolved
…gged

- Log request.values so both GET and POST are properly logged
- Add a test for action_logging
@yuqian90
Copy link
Contributor Author

@ashb @feluelle any further comments?

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, but it's just occurred to me that this might miss some actions entirely - ones that are using the Flask-AppBuilder model views etc.

@yuqian90
Copy link
Contributor Author

This is fine, but it's just occurred to me that this might miss some actions entirely - ones that are using the Flask-AppBuilder model views etc.

Thanks, @ashb. I looked at views.py again, most views inherit from AirflowModelView which is a subclass of flask_appbuilder.ModelView. Are those the views you think this action_logging decorator will miss? So I looked at some logs, I can confirm that actions such as Airflow.graph, Airflow.clear, Airflow.delete etc are all logged. So I believe the views using ModelView are not missed. Maybe I have misinterpreted what you meant? If that's the case pls point out.

Also, what's the path from here? Can one of the committers help merge this PR now it's approved?

@ashb
Copy link
Member

ashb commented Oct 4, 2019

I'll merge this PR as it fixes a bug.

Airflow.graph, Airflow.clear, Airflow.delete are not based on a ModelView. The Connection, XCom browse screens etc are the ones based on model views, and those are the ones that won't be included in the audit log. For now that is good as we don't want to include passwords in the audit logs

@ashb ashb merged commit e9ab9d6 into apache:master Oct 4, 2019
ashb pushed a commit that referenced this pull request Oct 7, 2019
…gged (#6064)

Log request.values so both GET and POST are properly logged

(cherry picked from commit e9ab9d6)
@yuqian90 yuqian90 deleted the AIRFLOW-5444 branch December 18, 2019 12:55
cong-zhu pushed a commit to cong-zhu/airflow that referenced this pull request Feb 6, 2020
…gged (apache#6064) (apache#379)

Log request.values so both GET and POST are properly logged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:webserver Webserver related Issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants