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.

Sign up for GitHub

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

Jump to bottom

Prevent large objects from being stored in the RTIF #38094

Merged
merged 16 commits into from
Mar 25, 2024
Merged

Prevent large objects from being stored in the RTIF #38094

merged 16 commits into from
Mar 25, 2024

Conversation

ephraimbuddy
Copy link
Contributor

There's no control over the size of objects stored in the rendered taskinstance field. This PR adds control and enable users to be able to customize the size of data that can be stored in this field

closes: #28199

@ephraimbuddy ephraimbuddy force-pushed the limit-templated-field-size branch 2 times, most recently from bdd600e to 6a10c94 Compare March 14, 2024 17:23
@ephraimbuddy ephraimbuddy force-pushed the limit-templated-field-size branch 5 times, most recently from 678cb87 to ffc67d1 Compare March 21, 2024 07:23
@ephraimbuddy
Copy link
Contributor Author

ephraimbuddy commented Mar 21, 2024 •
edited
Loading

There's something wrong with MySQL around templated fields. cc @uranusjr .

Oh, seen another test failure that's not mysql

@ephraimbuddy
Copy link
Contributor Author

I will like to include this in the next beta/rc, please review @uranusjr @jedcunningham @potiuk

potiuk
potiuk approved these changes Mar 22, 2024
View reviewed changes
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

LGTM. with one nit - but up to you to address it.

jedcunningham
jedcunningham reviewed Mar 22, 2024
View reviewed changes
airflow/config_templates/config.yml Outdated Show resolved Hide resolved
airflow/serialization/helpers.py Outdated Show resolved Hide resolved
potiuk
potiuk reviewed Mar 22, 2024
View reviewed changes
Copy link
Member

@potiuk potiuk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice. The prefix is cool.

There is one worry with that U just wanted to mention it.

I understand that we are masking secreates before serialization ? Just wanted to amke sure sure of it - it looks like from the code , but I wanted to make sure we are not showing half-unmasked secrets accidentally when we truncate the secrets.

@ephraimbuddy
Copy link
Contributor Author

Nice. The prefix is cool.

There is one worry with that U just wanted to mention it.

I understand that we are masking secreates before serialization ? Just wanted to amke sure sure of it - it looks like from the code , but I wanted to make sure we are not showing half-unmasked secrets accidentally when we truncate the secrets.

The secrets are masked after serialization see:

airflow/airflow/models/renderedtifields.py

Line 127 in 6a225cc

self._redact()
. I have added a test and will also do manual testing

@ephraimbuddy
Copy link
Contributor Author

ephraimbuddy commented Mar 22, 2024 •
edited
Loading

Nice. The prefix is cool.
There is one worry with that U just wanted to mention it.
I understand that we are masking secreates before serialization ? Just wanted to amke sure sure of it - it looks like from the code , but I wanted to make sure we are not showing half-unmasked secrets accidentally when we truncate the secrets.

The secrets are masked after serialization see:

airflow/airflow/models/renderedtifields.py

Line 127 in 6a225cc

self._redact()

. I have added a test and will also do manual testing

It doesn't work so
Resolved

@ephraimbuddy ephraimbuddy force-pushed the limit-templated-field-size branch 2 times, most recently from 11843f5 to 987d980 Compare March 24, 2024 00:08
ephraimbuddy and others added 15 commits March 25, 2024 08:03
@ephraimbuddy
Prevent large objects from being stored in the RTIF
9d171d9 
There's no control over the size of objects stored in the rendered taskinstance
field. This PR adds control and enable users to be able to customize the size of
data that can be stored in this field

closes: apache#28199
@ephraimbuddy
fixup! Prevent large objects from being stored in the RTIF
0e7fd98
@ephraimbuddy
Use len and check the size of the serialized
68b639a
@ephraimbuddy
Add test to already existing test
feb23d3
@ephraimbuddy
Remove xcom db clearing
e64b5a4
@ephraimbuddy @jedcunningham
Update airflow/config_templates/config.yml
7f126de 
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
@ephraimbuddy
Apply review suggestions
37914ab
@ephraimbuddy
Add test for redacting values and add significant item
d2fe6ec
@ephraimbuddy
Prefix with Truncated line
7ed2217
@ephraimbuddy
Ensure secrets are masked
97b4826
@ephraimbuddy
fixup! Ensure secrets are masked
4f799e2
@ephraimbuddy
Check the template field length in separate branches for jsonable and…
6e7d30c 
… nonjsonable
@ephraimbuddy
add tests for rendered dataframe
dfb6d5f
@ephraimbuddy
Apply suggestions from code review
0358d7e
@ephraimbuddy
update code and tests
8aa3962
@ephraimbuddy ephraimbuddy merged commit 4207099 into apache:main Mar 25, 2024
46 checks passed
@ephraimbuddy ephraimbuddy deleted the limit-templated-field-size branch March 25, 2024 08:35
@ephraimbuddy ephraimbuddy added the type:new-feature Changelog: New Features label Mar 27, 2024
@travis-cook-sfdc
Copy link

When a field gets truncated, is that only for the purpose of rendering it the UI? Or will it also get truncated when it gets executed?

The release notes on this topic are not clear and I'm seeing the truncation occur in both serialized_dag and rendered_task_fields.

@tatiana tatiana mentioned this pull request Oct 29, 2024
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dirrao dirrao dirrao left review comments

@uranusjr uranusjr uranusjr approved these changes

@potiuk potiuk potiuk approved these changes

@kaxil kaxil Awaiting requested review from kaxil

@ashb ashb Awaiting requested review from ashb ashb is a code owner

@bolkedebruin bolkedebruin Awaiting requested review from bolkedebruin bolkedebruin is a code owner

@XD-DENG XD-DENG Awaiting requested review from XD-DENG XD-DENG is a code owner

@jedcunningham jedcunningham Awaiting requested review from jedcunningham

Assignees
No one assigned
Labels
area:serialization type:new-feature Changelog: New Features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Prevent large objects from being stored in RenderedTaskInstanceFields
6 participants
@ephraimbuddy @travis-cook-sfdc @potiuk @uranusjr @dirrao @jedcunningham