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/06/01 14:29:00 UTC
[GitHub] [airflow] bhavaniravi opened a new pull request, #24079: Add: #23880 :: Audit log for AirflowModelViews(Variables/Connection)
bhavaniravi opened a new pull request, #24079:
URL: https://github.com/apache/airflow/pull/24079
This includes connections, variables, pools, SLA, and xcom. pools
closes #23880
**Description**
1. Introduced event parameter to `action_logging` decorator to introduce custom naming. Without this the original `action_logging` wouldn't differentiate update between Connection/Variables/SLA
2. Added logging to the following actions
```
[
"list",
"show",
"add",
"edit",
"delete",
"download",
"action",
"action_post",
]
```
--
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 #24079: Add: #23880 :: Audit log for AirflowModelViews(Variables/Connection)
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24079:
URL: https://github.com/apache/airflow/pull/24079#discussion_r903456214
##########
airflow/api_connexion/endpoints/connection_endpoint.py:
##########
@@ -39,10 +39,12 @@
from airflow.security import permissions
from airflow.utils.session import NEW_SESSION, provide_session
from airflow.utils.strings import get_random_string
+from airflow.www.decorators import action_logging
@security.requires_access([(permissions.ACTION_CAN_DELETE, permissions.RESOURCE_CONNECTION)])
@provide_session
+@action_logging(event=f"connection.{permissions.ACTION_CAN_DELETE.replace('can_','')}")
Review Comment:
Instead of doing this all over the place, we could change `action_logging` to something like this:
```python
def action_logging(*, event: str = "", permission: str = "") -> Callable[[T], T]:
if permission.startswith("can_"):
permission = permission[4:]
if permission:
event = f"{event}.{permission}"
...
```
And use it like this
```python
@action_logging(event="connection", permission=permissions.ACTION_CAN_EDIT)
```
Does this sound like a good idea?
----
An alternative approach would be to introduce a helper for this:
```python
def action_event_from_permission(prefix: str, permission: str) -> str:
if permission.startswith("can_"):
permission = permission[4:]
if prefix:
return f"{prefix}.{permission}"
return permission
```
and do
```python
@action_logging(
event=action_event_from_permission(
prefix="connection",
permission=permissions.ACTION_CAN_DELETE,
),
)
```
--
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] kaxil commented on a diff in pull request #24079: Add: #23880 :: Audit log for AirflowModelViews(Variables/Connection)
Posted by GitBox <gi...@apache.org>.
kaxil commented on code in PR #24079:
URL: https://github.com/apache/airflow/pull/24079#discussion_r914642164
##########
tests/test_utils/www.py:
##########
@@ -45,3 +47,28 @@ def check_content_not_in_response(text, resp, resp_code=200):
assert line not in resp_html
else:
assert text not in resp_html
+
+
+def _check_last_log(session, dag_id, event, execution_date):
+ logs = (
+ session.query(
+ Log.dag_id,
+ Log.task_id,
+ Log.event,
+ Log.execution_date,
+ Log.owner,
+ Log.extra,
+ )
+ .filter(
+ Log.dag_id == dag_id,
+ Log.event == event,
+ Log.execution_date == execution_date,
+ )
+ .order_by(Log.dttm.desc())
+ .limit(5)
+ .all()
+ )
+ assert len(logs) >= 1
+ assert logs[0].extra
+ print(logs[0].event)
Review Comment:
Can be removed?
--
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] jyotsa09 commented on pull request #24079: Add: #23880 :: Audit log for AirflowModelViews(Variables/Connection)
Posted by GitBox <gi...@apache.org>.
jyotsa09 commented on PR #24079:
URL: https://github.com/apache/airflow/pull/24079#issuecomment-1327135692
Hi @bhavaniravi when I have created a connection with api, a blank entry logs generated in audit logs and when I have updated the connection then an entry with connection id is logged.
<img width="1352" alt="Screenshot 2022-11-25 at 1 34 10 PM" src="https://user-images.githubusercontent.com/88504849/203931655-18fc37d4-01b5-41ba-984e-2ba48d91f0f9.png">
--
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] kaxil commented on a diff in pull request #24079: Add: #23880 :: Audit log for AirflowModelViews(Variables/Connection)
Posted by GitBox <gi...@apache.org>.
kaxil commented on code in PR #24079:
URL: https://github.com/apache/airflow/pull/24079#discussion_r914642788
##########
tests/www/views/test_views_decorators.py:
##########
@@ -172,6 +149,20 @@ def test_action_logging_post(session, admin_client):
)
+def delete_variable(session, key):
+ session.query(Variable).filter(Variable.key == key).delete()
+ session.commit()
+
+
+def test_action_logging_variables_post(session, admin_client):
+ form = dict(key="random", value="random")
+ admin_client.post("/variable/add", data=form)
+ session.flush()
+ session.commit()
Review Comment:
flush isn't needed if you are running session.commit straight-after
--
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] bhavaniravi commented on pull request #24079: Add: #23880 :: Audit log for AirflowModelViews(Variables/Connection)
Posted by GitBox <gi...@apache.org>.
bhavaniravi commented on PR #24079:
URL: https://github.com/apache/airflow/pull/24079#issuecomment-1262928424
Sorry, Life took over. I will look into this again
--
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] bhavaniravi commented on a diff in pull request #24079: Add: #23880 :: Audit log for AirflowModelViews(Variables/Connection)
Posted by GitBox <gi...@apache.org>.
bhavaniravi commented on code in PR #24079:
URL: https://github.com/apache/airflow/pull/24079#discussion_r896609957
##########
airflow/www/views.py:
##########
@@ -3667,6 +3667,19 @@ class AirflowModelView(ModelView):
CustomSQLAInterface = wwwutils.CustomSQLAInterface
+ def __getattribute__(self, attr):
+ attribute = object.__getattribute__(self, attr)
+ if callable(attribute) and attr in [
+ "add",
+ "edit",
+ "delete",
+ "download",
+ "action",
+ "action_post",
+ ]:
+ return action_logging(event=f"{self.route_base.strip('/')}.{attr}")(attribute)
+ return attribute
Review Comment:
The problem with overriding is how to generate the event name at the decorator level. `{self.route_base.strip('/')}.{attr}` we won't have the `self` to recognize if the action is for variable or connection
--
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 #24079: Add: #23880 :: Audit log for AirflowModelViews(Variables/Connection)
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24079:
URL: https://github.com/apache/airflow/pull/24079#discussion_r903458774
##########
airflow/www/views.py:
##########
@@ -3667,6 +3667,19 @@ class AirflowModelView(ModelView):
CustomSQLAInterface = wwwutils.CustomSQLAInterface
+ def __getattribute__(self, attr):
+ attribute = object.__getattribute__(self, attr)
+ if callable(attribute) and attr in [
+ "add",
+ "edit",
+ "delete",
+ "download",
+ "action",
+ "action_post",
+ ]:
+ return action_logging(event=f"{self.route_base.strip('/')}.{attr}")(attribute)
+ return attribute
Review Comment:
Let’s add a docstring to explain this as well. We should also add something to the _class’s docstring_ since readers might not be able to pick up `__getattribute__`.
--
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 #24079: Add: #23880 :: Audit log for AirflowModelViews(Variables/Connection)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #24079:
URL: https://github.com/apache/airflow/pull/24079#issuecomment-1166826734
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] potiuk commented on pull request #24079: Add: #23880 :: Audit log for AirflowModelViews(Variables/Connection)
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24079:
URL: https://github.com/apache/airflow/pull/24079#issuecomment-1296459437
static tests Needs fixing
--
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 merged pull request #24079: Add: #23880 :: Audit log for AirflowModelViews(Variables/Connection)
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #24079:
URL: https://github.com/apache/airflow/pull/24079
--
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] bbovenzi commented on pull request #24079: Add: #23880 :: Audit log for AirflowModelViews(Variables/Connection)
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on PR #24079:
URL: https://github.com/apache/airflow/pull/24079#issuecomment-1146003064
CC: @Jorricks
--
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 commented on a diff in pull request #24079: Add: #23880 :: Audit log for AirflowModelViews(Variables/Connection)
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on code in PR #24079:
URL: https://github.com/apache/airflow/pull/24079#discussion_r912711945
##########
airflow/www/decorators.py:
##########
@@ -35,51 +35,56 @@
logger = logging.getLogger(__name__)
-def action_logging(f: T) -> T:
+def action_logging(func=None, event=None):
Review Comment:
```suggestion
def action_logging(func: Optional[T]=None, event: Optional[str]=None) -> T:
```
Seems we need typing here
--
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 pull request #24079: Add: #23880 :: Audit log for AirflowModelViews(Variables/Connection)
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24079:
URL: https://github.com/apache/airflow/pull/24079#issuecomment-1304900055
I re-run it to account for random errors.
--
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] Jorricks commented on pull request #24079: Add: #23880 :: Audit log for AirflowModelViews(Variables/Connection)
Posted by GitBox <gi...@apache.org>.
Jorricks commented on PR #24079:
URL: https://github.com/apache/airflow/pull/24079#issuecomment-1146598467
I quickly scanned it but I'm missing the API integration. Could you please also integrate this with the API?
--
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 #24079: Add: #23880 :: Audit log for AirflowModelViews(Variables/Connection)
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24079:
URL: https://github.com/apache/airflow/pull/24079#discussion_r889862081
##########
airflow/www/views.py:
##########
@@ -3667,6 +3667,19 @@ class AirflowModelView(ModelView):
CustomSQLAInterface = wwwutils.CustomSQLAInterface
+ def __getattribute__(self, attr):
+ attribute = object.__getattribute__(self, attr)
+ if callable(attribute) and attr in [
+ "add",
+ "edit",
+ "delete",
+ "download",
+ "action",
+ "action_post",
+ ]:
+ return action_logging(event=f"{self.route_base.strip('/')}.{attr}")(attribute)
+ return attribute
Review Comment:
If the name list is definite, can we simply override those methods, instead of doing this magic function?
--
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 pull request #24079: Add: #23880 :: Audit log for AirflowModelViews(Variables/Connection)
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24079:
URL: https://github.com/apache/airflow/pull/24079#issuecomment-1319383977
transient errors.
--
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] eladkal commented on pull request #24079: Add: #23880 :: Audit log for AirflowModelViews(Variables/Connection)
Posted by GitBox <gi...@apache.org>.
eladkal commented on PR #24079:
URL: https://github.com/apache/airflow/pull/24079#issuecomment-1215430806
Static checks failed
--
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