Fix trigger_kwargs encryption/decryption on db migration #38876

hussein-awala · 2024-04-09T18:25:35Z

This PR updates the trigger kwargs encryption condition to from_revision < trigger_kwargs_encryption_version <= current_version and the trigger kwargs decryption condition to to_revision < trigger_kwargs_encryption_version <= from_revision:

encryption: the previous revision should be lower than the revision that encrypts the kwargs and the new revision should be equal to or greater than this revision.
decryption: the new revision should be lower than the revision that encrypts the kwargs and the previous revision should be equal to or greater than this revision.

ephraimbuddy · 2024-04-14T21:53:10Z

Hi @hussein-awala , can you add more information to the commit message, I'm finding it difficult to understand what you worked on.

hussein-awala · 2024-04-15T21:06:51Z

Hi @hussein-awala , can you add more information to the commit message, I'm finding it difficult to understand what you worked on.

I updated the PR description, could you take a look?

airflow/utils/db.py

ephraimbuddy · 2024-04-16T07:40:22Z

tests/utils/test_db.py

        mock_alembic_upgrade.assert_called_once_with(mock.ANY, f"{from_revision}:{to_revision}", sql=True)

+    @pytest.mark.skipif(
+        conf.get_mandatory_value("database", "sql_alchemy_conn").lower().startswith("sqlite"),
+        reason="Offline migration not supported for SQLite.",


Why do we now have this?

The tests are not valid for SQLite , they should be skipped because of

airflow/airflow/utils/db.py

Line 1563 in 1769ed0

raise AirflowException("Offline migration not supported for SQLite.")

But before _get_current_revision was not used if show_sql_only=True.

I can refactor it if needed to revert the tests

airflow/utils/db.py

hussein-awala · 2024-04-17T23:19:22Z

tests/utils/test_db.py

        mock_alembic_upgrade.assert_called_once_with(mock.ANY, f"{from_revision}:{to_revision}", sql=True)

+    @pytest.mark.skipif(
+        conf.get_mandatory_value("database", "sql_alchemy_conn").lower().startswith("sqlite"),
+        reason="Offline migration not supported for SQLite.",


The tests are not valid for SQLite , they should be skipped because of

airflow/airflow/utils/db.py

Line 1563 in 1769ed0

raise AirflowException("Offline migration not supported for SQLite.")

But before _get_current_revision was not used if show_sql_only=True.

I can refactor it if needed to revert the tests

Co-authored-by: Ephraim Anierobi <splendidzigy24@gmail.com>

Lee-W · 2024-04-24T01:09:07Z

airflow/utils/db.py

+    current_version = _from_revision
+    trigger_kwargs_encryption_revision = _REVISION_HEADS_MAP["2.9.0"]
+    if (
+        _from_revision != trigger_kwargs_encryption_revision
+        and _revision_greater(config, trigger_kwargs_encryption_revision, _from_revision)
+        and _revision_greater(config, current_version, trigger_kwargs_encryption_revision)
    ):
+        # _from_revision < trigger_kwargs_encryption_version <= current_version


I'm a bit confused here. If current_vesrion = _from_revision, then when will _from_revision < trigger_kwargs_encryption_version <= current_version happen?

Same here, this comment looks quite wrong here.

Is there confusion between from_revision I wonder? Not sure which is intended here.

Taragolis · 2024-04-24T08:56:32Z

tests/utils/test_db.py

+    @pytest.mark.skipif(
+        conf.get_mandatory_value("database", "sql_alchemy_conn").lower().startswith("sqlite"),
+        reason="Offline migration not supported for SQLite.",
+    )


Maybe mark it for supported backend?

Suggested change

@pytest.mark.skipif(

conf.get_mandatory_value("database", "sql_alchemy_conn").lower().startswith("sqlite"),

reason="Offline migration not supported for SQLite.",

)

@pytest.mark.backend("postgres", "mysql")

dstandish · 2024-04-24T16:14:44Z

Should we consider yanking 2.9.0 once 2.9.1 is out with this fix?

eladkal · 2024-04-24T18:57:48Z

Should we consider yanking 2.9.0 once 2.9.1 is out with this fix?

I don't think so.
In my point of view if we wanted to yank then we should have merge this quickly and cut 2.9.1 immidiately as critical bug fix. I asked about this when issue was discoved but others thought we should wait for more more bug fixes as this was not categorized as critical.

jedcunningham · 2024-04-24T22:58:30Z

Ignoring that the conditional is wrong, I don't follow how this migration approach works for offline migration in the first place. Am I missing functionality somewhere that solves that scenario?

jedcunningham · 2024-04-24T23:10:07Z

Just confirmed my suspicions, it doesn't work for offline migrations:

[2024-04-24T23:08:17.337+0000] {triggerer_job_runner.py:341} ERROR - Exception when executing TriggererJobRunner._run_trigger_loop
Traceback (most recent call last):                                                                                                
  File "/home/airflow/.local/lib/python3.12/site-packages/airflow/jobs/triggerer_job_runner.py", line 339, in _execute            
    self._run_trigger_loop()                                                                                                      
  File "/home/airflow/.local/lib/python3.12/site-packages/airflow/jobs/triggerer_job_runner.py", line 362, in _run_trigger_loop   
    self.load_triggers()                                                                                                          
  File "/home/airflow/.local/lib/python3.12/site-packages/airflow/jobs/triggerer_job_runner.py", line 377, in load_triggers       
    self.trigger_runner.update_triggers(set(ids))                                                                                 
  File "/home/airflow/.local/lib/python3.12/site-packages/airflow/jobs/triggerer_job_runner.py", line 676, in update_triggers     
    new_trigger_instance = trigger_class(**new_trigger_orm.kwargs)                                                                
                                           ^^^^^^^^^^^^^^^^^^^^^^                                                                 
  File "/home/airflow/.local/lib/python3.12/site-packages/airflow/models/trigger.py", line 93, in kwargs                          
    return self._decrypt_kwargs(self.encrypted_kwargs)                                                                            
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^                                                                            
  File "/home/airflow/.local/lib/python3.12/site-packages/airflow/models/trigger.py", line 119, in _decrypt_kwargs                
    decrypted_kwargs = json.loads(get_fernet().decrypt(encrypted_kwargs.encode("utf-8")).decode("utf-8"))                         
                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^                                          
  File "/home/airflow/.local/lib/python3.12/site-packages/cryptography/fernet.py", line 211, in decrypt                           
    raise InvalidToken                                                                                                            
cryptography.fernet.InvalidToken

This was all that was spit out for that migration:

-- Running upgrade ee1467d4aa35 -> 1949afb29106

ALTER TABLE trigger ALTER COLUMN kwargs TYPE TEXT;

UPDATE alembic_version SET version_num='1949afb29106' WHERE alembic_version.version_num = 'ee1467d4aa35';

dstandish · 2024-04-25T00:00:35Z

Should we consider yanking 2.9.0 once 2.9.1 is out with this fix?

I don't think so. In my point of view if we wanted to yank then we should have merge this quickly and cut 2.9.1 immidiately as critical bug fix. I asked about this when issue was discoved but others thought we should wait for more more bug fixes as this was not categorized as critical.

To me, I’m not sure whether it meets the criteria of critical, but it’s pretty bad. If the user is using helm chart, then it will run the migration pod a lot, and if there are triggers in flight, this could happen and sorta bork the cluster IIUC.

jedcunningham · 2024-04-25T04:01:46Z

Currently downgrade also doesn't work when you have triggers - alembic tries to switch the column back to json before decrypting happens.

jedcunningham · 2024-04-25T05:34:06Z

I've opened #39246 as an alternative solution to this.

jedcunningham · 2024-04-25T19:22:09Z

Closing this as #39246 has been merged. Thanks for kicking off fixing this bug @hussein-awala!

Fix trigger_kwargs encryption/decryption on db migration

e39ed83

hussein-awala added the type:bug-fix Changelog: Bug Fixes label Apr 9, 2024

hussein-awala added this to the Airflow 2.9.1 milestone Apr 9, 2024

hussein-awala added the area:db-migrations PRs with DB migration label Apr 9, 2024

hussein-awala requested review from potiuk and ephraimbuddy April 9, 2024 18:25

eladkal approved these changes Apr 9, 2024

View reviewed changes

hussein-awala mentioned this pull request Apr 9, 2024

DB migrate throws na error on encrypt_trigger_kwargs #38836

Closed

2 tasks

hussein-awala added 2 commits April 9, 2024 21:58

fix unit tests

de4c044

skip generic offline migration tests for SQLite

5b3f285

ephraimbuddy reviewed Apr 16, 2024

View reviewed changes

airflow/utils/db.py Outdated Show resolved Hide resolved

ephraimbuddy reviewed Apr 16, 2024

View reviewed changes

airflow/utils/db.py Outdated Show resolved Hide resolved

ephraimbuddy reviewed Apr 16, 2024

View reviewed changes

hussein-awala commented Apr 17, 2024

View reviewed changes

hussein-awala and others added 2 commits April 18, 2024 01:19

Update airflow/utils/db.py

8ff70d2

Co-authored-by: Ephraim Anierobi <splendidzigy24@gmail.com>

Apply suggestions from code review

faacda6

Lee-W self-requested a review April 24, 2024 00:59

Lee-W reviewed Apr 24, 2024

View reviewed changes

Taragolis reviewed Apr 24, 2024

View reviewed changes

jedcunningham mentioned this pull request Apr 25, 2024

Fix trigger kwarg encryption migration #39246

Merged

jedcunningham closed this Apr 25, 2024

ephraimbuddy mentioned this pull request Apr 30, 2024

Status of testing of Apache Airflow 2.9.1rc2 #39326

Closed

56 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix trigger_kwargs encryption/decryption on db migration #38876

Fix trigger_kwargs encryption/decryption on db migration #38876

hussein-awala commented Apr 9, 2024 •

edited

Loading

ephraimbuddy commented Apr 14, 2024

hussein-awala commented Apr 15, 2024

ephraimbuddy Apr 16, 2024

hussein-awala Apr 17, 2024

hussein-awala Apr 17, 2024

Lee-W Apr 24, 2024

uranusjr Apr 24, 2024

uranusjr Apr 24, 2024

Taragolis Apr 24, 2024

dstandish commented Apr 24, 2024

eladkal commented Apr 24, 2024

jedcunningham commented Apr 24, 2024

jedcunningham commented Apr 24, 2024

dstandish commented Apr 25, 2024 •

edited

Loading

jedcunningham commented Apr 25, 2024

jedcunningham commented Apr 25, 2024

jedcunningham commented Apr 25, 2024

Fix trigger_kwargs encryption/decryption on db migration #38876

Fix trigger_kwargs encryption/decryption on db migration #38876

Conversation

hussein-awala commented Apr 9, 2024 • edited Loading

ephraimbuddy commented Apr 14, 2024

hussein-awala commented Apr 15, 2024

ephraimbuddy Apr 16, 2024

Choose a reason for hiding this comment

hussein-awala Apr 17, 2024

Choose a reason for hiding this comment

hussein-awala Apr 17, 2024

Choose a reason for hiding this comment

Lee-W Apr 24, 2024

Choose a reason for hiding this comment

uranusjr Apr 24, 2024

Choose a reason for hiding this comment

uranusjr Apr 24, 2024

Choose a reason for hiding this comment

Taragolis Apr 24, 2024

Choose a reason for hiding this comment

dstandish commented Apr 24, 2024

eladkal commented Apr 24, 2024

jedcunningham commented Apr 24, 2024

jedcunningham commented Apr 24, 2024

dstandish commented Apr 25, 2024 • edited Loading

jedcunningham commented Apr 25, 2024

jedcunningham commented Apr 25, 2024

jedcunningham commented Apr 25, 2024

hussein-awala commented Apr 9, 2024 •

edited

Loading

dstandish commented Apr 25, 2024 •

edited

Loading