You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "rawwar (via GitHub)" <gi...@apache.org> on 2025/03/17 06:00:49 UTC
[PR] AIP-84 - add auth to dag report [airflow]
rawwar opened a new pull request, #47842:
URL: https://github.com/apache/airflow/pull/47842
related to https://github.com/apache/airflow/issues/42360
--
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
Re: [PR] AIP-84 - add auth to dag report [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun merged PR #47842:
URL: https://github.com/apache/airflow/pull/47842
--
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
Re: [PR] AIP-84 - add auth to dag report [airflow]
Posted by "rawwar (via GitHub)" <gi...@apache.org>.
rawwar commented on code in PR #47842:
URL: https://github.com/apache/airflow/pull/47842#discussion_r1998077131
##########
airflow/api_fastapi/core_api/routes/public/dag_report.py:
##########
@@ -41,16 +45,29 @@
status.HTTP_400_BAD_REQUEST,
]
),
+ dependencies=[Depends(requires_access_dag(method="GET"))],
)
def get_dag_reports(
subdir: str,
+ readable_dags_filter: ReadableDagsFilterDep,
):
"""Get DAG report."""
fullpath = os.path.normpath(subdir)
if not fullpath.startswith(settings.DAGS_FOLDER):
raise HTTPException(status.HTTP_400_BAD_REQUEST, "subdir should be subpath of DAGS_FOLDER settings")
dagbag = DagBag(fullpath)
+ filtered_dagbag_stats = []
+ if readable_dags_filter.value:
+ for each_file_load_stat in dagbag.dagbag_stats:
+ allowed = True
+ for each in eval(each_file_load_stat.dags):
+ if each not in readable_dags_filter.value:
+ allowed = False
+ break
+ if allowed:
+ filtered_dagbag_stats.append(each_file_load_stat)
+
Review Comment:
each file load stat also include number of tasks, dag id's. If a user does not have access to 1 dag out of N dags present in the file, we are not returning the report for that entire file.
The reason is that we are not sure how many tasks belong to the particular dag_id.
file stats are calculated using below code:
```
stats.append(
FileLoadStat(
file=filepath.replace(settings.DAGS_FOLDER, ""),
duration=file_parse_end_dttm - file_parse_start_dttm,
dag_num=len(found_dags),
task_num=sum(len(dag.tasks) for dag in found_dags),
dags=str([dag.dag_id for dag in found_dags]),
warning_num=len(self.captured_warnings.get(filepath, [])),
)
)
```
Unless, we pass dag_filter to the DagBag, its not easy for us to filter out tasks from the Dag's that user does not have access to.
Two ways. either we disallow the entire file if a user is forbidden to access even 1 out of N dags in the file. Or we can update DagBag to take dag's filter, and then filter out dag_id's, tasks from that specific dag_id.
--
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
Re: [PR] AIP-84 - add auth to dag report [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #47842:
URL: https://github.com/apache/airflow/pull/47842#discussion_r1998539556
##########
airflow/api_fastapi/core_api/routes/public/dag_report.py:
##########
@@ -41,16 +45,29 @@
status.HTTP_400_BAD_REQUEST,
]
),
+ dependencies=[Depends(requires_access_dag(method="GET"))],
)
def get_dag_reports(
subdir: str,
+ readable_dags_filter: ReadableDagsFilterDep,
):
"""Get DAG report."""
fullpath = os.path.normpath(subdir)
if not fullpath.startswith(settings.DAGS_FOLDER):
raise HTTPException(status.HTTP_400_BAD_REQUEST, "subdir should be subpath of DAGS_FOLDER settings")
dagbag = DagBag(fullpath)
+ filtered_dagbag_stats = []
+ if readable_dags_filter.value:
+ for each_file_load_stat in dagbag.dagbag_stats:
+ allowed = True
+ for each in eval(each_file_load_stat.dags):
+ if each not in readable_dags_filter.value:
+ allowed = False
+ break
+ if allowed:
+ filtered_dagbag_stats.append(each_file_load_stat)
+
Review Comment:
> we disallow the entire file if a user is forbidden to access even 1 out of N dags in the file
prefer this one
@pierrejeambrun WDYT?
--
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
Re: [PR] AIP-84 - add auth to dag report [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #47842:
URL: https://github.com/apache/airflow/pull/47842#discussion_r1998551132
##########
airflow/api_fastapi/core_api/routes/public/dag_report.py:
##########
@@ -41,16 +45,29 @@
status.HTTP_400_BAD_REQUEST,
]
),
+ dependencies=[Depends(requires_access_dag(method="GET"))],
)
def get_dag_reports(
subdir: str,
+ readable_dags_filter: ReadableDagsFilterDep,
):
"""Get DAG report."""
fullpath = os.path.normpath(subdir)
if not fullpath.startswith(settings.DAGS_FOLDER):
raise HTTPException(status.HTTP_400_BAD_REQUEST, "subdir should be subpath of DAGS_FOLDER settings")
dagbag = DagBag(fullpath)
+ filtered_dagbag_stats = []
+ if readable_dags_filter.value:
+ for each_file_load_stat in dagbag.dagbag_stats:
+ allowed = True
+ for each in eval(each_file_load_stat.dags):
+ if each not in readable_dags_filter.value:
+ allowed = False
+ break
+ if allowed:
+ filtered_dagbag_stats.append(each_file_load_stat)
+
Review Comment:
IMO, import error makes more sense to allow partial access as the user needs to fix it while dag report does not. 🤔 dag report also contains aggregation logic
--
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
Re: [PR] AIP-84 - add auth to dag report [airflow]
Posted by "rawwar (via GitHub)" <gi...@apache.org>.
rawwar commented on code in PR #47842:
URL: https://github.com/apache/airflow/pull/47842#discussion_r1998594862
##########
airflow/api_fastapi/core_api/routes/public/dag_report.py:
##########
@@ -41,16 +46,30 @@
status.HTTP_400_BAD_REQUEST,
]
),
+ dependencies=[Depends(requires_access_dag(method="GET"))],
)
def get_dag_reports(
subdir: str,
+ readable_dags_filter: ReadableDagsFilterDep,
):
"""Get DAG report."""
fullpath = os.path.normpath(subdir)
if not fullpath.startswith(settings.DAGS_FOLDER):
raise HTTPException(status.HTTP_400_BAD_REQUEST, "subdir should be subpath of DAGS_FOLDER settings")
+
dagbag = DagBag(fullpath)
+
+ readable_dag_ids: set[str] | None = readable_dags_filter.value
+ if readable_dag_ids:
+ filtered_dagbag_stats = [
+ file_load_stat
+ for file_load_stat in dagbag.dagbag_stats
+ if len(set(ast.literal_eval(file_load_stat.dags)) - readable_dag_ids) == 0
Review Comment:
+1
--
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
Re: [PR] AIP-84 - add auth to dag report [airflow]
Posted by "rawwar (via GitHub)" <gi...@apache.org>.
rawwar commented on code in PR #47842:
URL: https://github.com/apache/airflow/pull/47842#discussion_r1998232373
##########
airflow/api_fastapi/core_api/routes/public/dag_report.py:
##########
@@ -41,16 +45,29 @@
status.HTTP_400_BAD_REQUEST,
]
),
+ dependencies=[Depends(requires_access_dag(method="GET"))],
)
def get_dag_reports(
subdir: str,
+ readable_dags_filter: ReadableDagsFilterDep,
):
"""Get DAG report."""
fullpath = os.path.normpath(subdir)
if not fullpath.startswith(settings.DAGS_FOLDER):
raise HTTPException(status.HTTP_400_BAD_REQUEST, "subdir should be subpath of DAGS_FOLDER settings")
dagbag = DagBag(fullpath)
+ filtered_dagbag_stats = []
+ if readable_dags_filter.value:
+ for each_file_load_stat in dagbag.dagbag_stats:
+ allowed = True
+ for each in eval(each_file_load_stat.dags):
+ if each not in readable_dags_filter.value:
+ allowed = False
+ break
+ if allowed:
+ filtered_dagbag_stats.append(each_file_load_stat)
+
Review Comment:
alternatively, we can just remove the dag_id's, which the user does not have access to, from the list and let the task_num reflect the complete count.
--
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
Re: [PR] AIP-84 - add auth to dag report [airflow]
Posted by "rawwar (via GitHub)" <gi...@apache.org>.
rawwar commented on PR #47842:
URL: https://github.com/apache/airflow/pull/47842#issuecomment-2728336066
I found `ests_common.test_utils.db.parse_and_sync_to_db` method. Trying to use 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
Re: [PR] AIP-84 - add auth to dag report [airflow]
Posted by "rawwar (via GitHub)" <gi...@apache.org>.
rawwar commented on PR #47842:
URL: https://github.com/apache/airflow/pull/47842#issuecomment-2728262895
One issue here. So, DagBag will read all files in the DAG_DIR and also example dags. But, it does not sync to DB. Because of this, readable_dags_filter(which queries DagModel to fetch dag_id's) will be empty.
I am trying to figure out how to update DB with the example_dags + available dags in the test.
Also, code needs some refactoring. Which I'll do once I figure out above issue.
--
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
Re: [PR] AIP-84 - add auth to dag report [airflow]
Posted by "jason810496 (via GitHub)" <gi...@apache.org>.
jason810496 commented on code in PR #47842:
URL: https://github.com/apache/airflow/pull/47842#discussion_r1998546714
##########
airflow/api_fastapi/core_api/routes/public/dag_report.py:
##########
@@ -41,16 +45,29 @@
status.HTTP_400_BAD_REQUEST,
]
),
+ dependencies=[Depends(requires_access_dag(method="GET"))],
)
def get_dag_reports(
subdir: str,
+ readable_dags_filter: ReadableDagsFilterDep,
):
"""Get DAG report."""
fullpath = os.path.normpath(subdir)
if not fullpath.startswith(settings.DAGS_FOLDER):
raise HTTPException(status.HTTP_400_BAD_REQUEST, "subdir should be subpath of DAGS_FOLDER settings")
dagbag = DagBag(fullpath)
+ filtered_dagbag_stats = []
+ if readable_dags_filter.value:
+ for each_file_load_stat in dagbag.dagbag_stats:
+ allowed = True
+ for each in eval(each_file_load_stat.dags):
+ if each not in readable_dags_filter.value:
+ allowed = False
+ break
+ if allowed:
+ filtered_dagbag_stats.append(each_file_load_stat)
+
Review Comment:
The authorization scenario seems quite same to [AIP-84 | Add Auth for Import Error #47270
](https://github.com/apache/airflow/pull/47270). Not sure can we utilize the same logic ?
Since the there are Import Error endpoint in legacy API ( check from `v2-10-test` ), but I don't see Dag Report endpoint in legacy 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
Re: [PR] AIP-84 - add auth to dag report [airflow]
Posted by "jason810496 (via GitHub)" <gi...@apache.org>.
jason810496 commented on code in PR #47842:
URL: https://github.com/apache/airflow/pull/47842#discussion_r1998546714
##########
airflow/api_fastapi/core_api/routes/public/dag_report.py:
##########
@@ -41,16 +45,29 @@
status.HTTP_400_BAD_REQUEST,
]
),
+ dependencies=[Depends(requires_access_dag(method="GET"))],
)
def get_dag_reports(
subdir: str,
+ readable_dags_filter: ReadableDagsFilterDep,
):
"""Get DAG report."""
fullpath = os.path.normpath(subdir)
if not fullpath.startswith(settings.DAGS_FOLDER):
raise HTTPException(status.HTTP_400_BAD_REQUEST, "subdir should be subpath of DAGS_FOLDER settings")
dagbag = DagBag(fullpath)
+ filtered_dagbag_stats = []
+ if readable_dags_filter.value:
+ for each_file_load_stat in dagbag.dagbag_stats:
+ allowed = True
+ for each in eval(each_file_load_stat.dags):
+ if each not in readable_dags_filter.value:
+ allowed = False
+ break
+ if allowed:
+ filtered_dagbag_stats.append(each_file_load_stat)
+
Review Comment:
The authorization scenario seems quite same to [AIP-84 | Add Auth for Import Error #47270
](https://github.com/apache/airflow/pull/47270). Not sure can we utilize the same logic ?
--
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
Re: [PR] AIP-84 - add auth to dag report [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #47842:
URL: https://github.com/apache/airflow/pull/47842#discussion_r1998551132
##########
airflow/api_fastapi/core_api/routes/public/dag_report.py:
##########
@@ -41,16 +45,29 @@
status.HTTP_400_BAD_REQUEST,
]
),
+ dependencies=[Depends(requires_access_dag(method="GET"))],
)
def get_dag_reports(
subdir: str,
+ readable_dags_filter: ReadableDagsFilterDep,
):
"""Get DAG report."""
fullpath = os.path.normpath(subdir)
if not fullpath.startswith(settings.DAGS_FOLDER):
raise HTTPException(status.HTTP_400_BAD_REQUEST, "subdir should be subpath of DAGS_FOLDER settings")
dagbag = DagBag(fullpath)
+ filtered_dagbag_stats = []
+ if readable_dags_filter.value:
+ for each_file_load_stat in dagbag.dagbag_stats:
+ allowed = True
+ for each in eval(each_file_load_stat.dags):
+ if each not in readable_dags_filter.value:
+ allowed = False
+ break
+ if allowed:
+ filtered_dagbag_stats.append(each_file_load_stat)
+
Review Comment:
IMO, import error makes more sense to allow partial access as the user needs to fix it while dag report does not. 🤔
--
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
Re: [PR] AIP-84 - add auth to dag report [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #47842:
URL: https://github.com/apache/airflow/pull/47842#discussion_r1998565516
##########
airflow/api_fastapi/core_api/routes/public/dag_report.py:
##########
@@ -41,16 +46,30 @@
status.HTTP_400_BAD_REQUEST,
]
),
+ dependencies=[Depends(requires_access_dag(method="GET"))],
)
def get_dag_reports(
subdir: str,
+ readable_dags_filter: ReadableDagsFilterDep,
):
"""Get DAG report."""
fullpath = os.path.normpath(subdir)
if not fullpath.startswith(settings.DAGS_FOLDER):
raise HTTPException(status.HTTP_400_BAD_REQUEST, "subdir should be subpath of DAGS_FOLDER settings")
+
dagbag = DagBag(fullpath)
+
+ readable_dag_ids: set[str] | None = readable_dags_filter.value
+ if readable_dag_ids:
+ filtered_dagbag_stats = [
+ file_load_stat
+ for file_load_stat in dagbag.dagbag_stats
+ if len(set(ast.literal_eval(file_load_stat.dags)) - readable_dag_ids) == 0
Review Comment:
Not 100% sure whether it's a good idea to use `ast.literal_eval`. But we use it elsewhere in airflow and I think it should be better than `eval`
--
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