Skip to content

add missing conn_id to string representation of ObjectStoragePath - #39313

Merged
Lee-W merged 1 commit into
apache:mainfrom
astronomer:fix-conn-id-missed-in-ObjectStoragePath
Apr 30, 2024
Merged

Lee-W merged 1 commit into
apache:mainfrom
astronomer:fix-conn-id-missed-in-ObjectStoragePath

Conversation

@Lee-W

@Lee-W Lee-W commented Apr 29, 2024

Copy link
Copy Markdown
Member

As stated in apache-airflow-providers-common-io - Object Storage XCom Backend, we'll need to set xcom_objectstorage_path to something like s3://conn_id@mybucket/key to use customized xcom backend. As the string representation of ObjectStoragePath does not contain conn_id, this serialization will lost this information which cause this comparison to fail.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@Lee-W
Lee-W requested a review from bolkedebruin as a code owner April 29, 2024 15:32
Comment thread tests/io/test_path.py Outdated

@pankajastro pankajastro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@Lee-W
Lee-W force-pushed the fix-conn-id-missed-in-ObjectStoragePath branch from 5acc478 to cb98b09 Compare April 29, 2024 15:38
@Lee-W
Lee-W merged commit ddd3b5b into apache:main Apr 30, 2024
@Lee-W
Lee-W deleted the fix-conn-id-missed-in-ObjectStoragePath branch April 30, 2024 00:41
@TJaniF

TJaniF commented May 13, 2024 •

Copy link
Copy Markdown
Contributor

Hello @eladkal 🙂

Very sorry for the late poke about this but is there a chance to include a 1.3.2 release of the common io provider with this bugfix in the current wave of releases (or the next one, since the May 12th one already two votes)?
That would be awesome, as soon as this bugfix is in, I can get a tutorial about using the Object Storage XCom backend in the Astronomer docs merged 🙂.

cc: @pankajkoti

@Lee-W

Lee-W commented May 14, 2024 •

Copy link
Copy Markdown
Member Author

Hello @eladkal 🙂

Very sorry for the late poke about this but is there a chance to include a 1.3.2 release of the common io provider with this bugfix in the current wave of releases (or the next one, since the May 12th one already two votes)? That would be awesome, as soon as this bugfix is in, I can get a tutorial about using the Object Storage XCom backend in the Astronomer docs merged 🙂.

cc: @pankajkoti

If I'm not mistaken, we'll need to wait for the next airflow release (not provider release) to include this fix.

@eladkal

eladkal commented May 14, 2024

Copy link
Copy Markdown
Contributor

Hello @eladkal 🙂

Very sorry for the late poke about this but is there a chance to include a 1.3.2 release of the common io provider with this bugfix in the current wave of releases (or the next one, since the May 12th one already two votes)? That would be awesome, as soon as this bugfix is in, I can get a tutorial about using the Object Storage XCom backend in the Astronomer docs merged 🙂.

cc: @pankajkoti

This is Airflow core not providers.
It depends when @ephraimbuddy will cut Airflow 2.9.2

@eladkal eladkal added this to the Airflow 2.9.2 milestone May 14, 2024
@eladkal eladkal added the type:bug-fix Changelog: Bug Fixes label May 14, 2024
@TJaniF

TJaniF commented May 14, 2024

Copy link
Copy Markdown
Contributor

This is Airflow core not providers. It depends when @ephraimbuddy will cut Airflow 2.9.2

Oh! Apologies 🤦 . Thank you!

@DLT1412

DLT1412 commented Jun 21, 2024 •

Copy link
Copy Markdown
Contributor

Just a security concern, is it too risky if we add conn info to xcom value return to UI? IMO xcom values should not have any sensitive value relate to internal process can be misuse/leak when user reach that, and user still need to access xcom value to debug/check etc so we not remove can read on XComs permission out of user role.

@Lee-W

Lee-W commented Jun 26, 2024

Copy link
Copy Markdown
Member Author

As long as there's no credential, I think we're good. But it really depends on what you think sensitive values are 🤔 By users, do you mean DAG author or just DAG user? I'm not sure we should grant DAG user XCom permission. For the DAG author, it again depends on the team's policy.

@DLT1412

DLT1412 commented Jun 26, 2024

Copy link
Copy Markdown
Contributor

@Lee-W with user I mean DAG author. Credential not expose to user in this case but it's a system configuration then I think by default should not expose to anyone except Admin and Op role, with honor with flag webserver.expose_config.
Again, credential not expose but dag author can use this connection just by param pass when init any cloud provider hook/operator(by mistake or intended) and I think we should reduce risk here.

@simi

simi commented Aug 1, 2025

Copy link
Copy Markdown
Contributor

Hello @Lee-W, this seems to broke ability to use ObjectStoragePath directly with DuckDB and similar as explained at https://airflow.apache.org/docs/apache-airflow/stable/tutorial/objectstorage.html#analyzing-the-data-with-duckdb since including the conn_id makes invalid bucket name. Is it still needed since there are serialize/deserialize methods implemented for XCom?

@simi simi mentioned this pull request Aug 1, 2025
1 of 2 tasks
@simi

simi commented Aug 1, 2025

Copy link
Copy Markdown
Contributor

I'll open PR to fix. I can confirm conn_id is serialized properly outside the URI.

image

@Lee-W

Lee-W commented Aug 2, 2025

Copy link
Copy Markdown
Member Author

yep, it's possible the implementation has changed since 2 to 3. Feel free to send a PR 🙂 Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:bug-fix Changelog: Bug Fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants