Separate out tests to cater for changes in Python 3.8.8 #14698

kaxil · 2021-03-10T15:05:01Z

python/cpython#24297 change was included in
Python 3.8.8 to fix a vulnerability (bpo-42967)

Depending on which Base Python Image is run in our CI, two of the tests
can fail or succeed.

Our Previous two attempts:

We might for a while get different base python version depending on the changes of a PR (whether or not it includes a change to dockerfiler).
a) when you have PR which do not have changes in the Dockerfile, they will use the older python version as base (for example Python 3.8.7)
b) when you have PR that touches the Dockerfile and have setup.py changes in master, it should pull Python 3.8.8 first.

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

kaxil · 2021-03-10T15:06:05Z

tests/www/test_views.py

I did not do:

sys.version_info <= (3, 8, 7)

for the following case:

❯ python Python 3.7.4 (default, Aug 13 2019, 15:17:50) [Clang 4.0.1 (tags/RELEASE_401/final)] :: Anaconda, Inc. on darwin Type "help", "copyright", "credits" or "license" for more information. >>> import sys >>> sys.version_info == (3, 7, 4, ) False >>> sys.version_info sys.version_info(major=3, minor=7, micro=4, releaselevel='final', serial=0) >>> sys.version_info == (3, 7, 4, 'final', 0) True

XD-DENG · 2021-03-10T17:13:18Z

tests/www/test_views.py

This line is missed to be updated?

ashb · 2021-03-10T18:22:29Z

Depending on which Base Python Image is run in our CI, two of the tests

You mean this isn't predictable?

python/cpython#24297 change was included in Python 3.8.8 to fix a vulnerability (bpo-42967) Depending on which Base Python Image is run in our CI, two of the tests can fail or succeed. Our Previous two attempts: - apache@061cd23 - apache@49952e7 We might for a while get different base python version depending on the changes of a PR (whether or not it includes a change to dockerfiler). a) when you have PR which do not have changes in the Dockerfile, they will use the older python version as base (for example Python 3.8.7) b) when you have PR that touches the Dockerfile and have setup.py changes in master, it should pull Python 3.8.8 first.

kaxil · 2021-03-10T22:00:37Z

Depending on which Base Python Image is run in our CI, two of the tests

You mean this isn't predictable?

Yup, from

airflow/scripts/ci/libraries/_push_pull_remove_images.sh

Lines 107 to 146 in 6851677

    
           # Pulls the base Python image. This image is used as base for CI and PROD images, depending on the parameters used: 
        
           # 
        
           # * if UPGRADE_TO_NEWER_DEPENDENCIES is noy false, then it pulls the latest Python image available first and 
        
           #     adds `org.opencontainers.image.source` label to it, so that it is linked to Airflow repository when 
        
           #     we push it to GHCR registry 
        
           # * If UPGRADE_TO_NEWER_DEPENDENCIES it pulls the Python base image from either GitHub registry or from DockerHub 
        
           #     depending on USE_GITHUB_REGISTRY variable. In case we pull specific build image (via suffix) 
        
           #     it will pull the right image using the specified suffix 
        
           function push_pull_remove_images::pull_base_python_image() { 
        
               echo 
        
               echo "Force pull base image ${AIRFLOW_PYTHON_BASE_IMAGE}. Upgrade to newer dependencies: ${UPGRADE_TO_NEWER_DEPENDENCIES}" 
        
               echo 
        
               if [[ -n ${DETECTED_TERMINAL=} ]]; then 
        
                   echo -n " 
        
           Docker pulling ${AIRFLOW_PYTHON_BASE_IMAGE}. Upgrade to newer dependencies ${UPGRADE_TO_NEWER_DEPENDENCIES} 
        
           " > "${DETECTED_TERMINAL}" 
        
               fi 
        
               if [[ "${UPGRADE_TO_NEWER_DEPENDENCIES}" != "false" ]]; then 
        
                   # Pull latest PYTHON_BASE_IMAGE, so that it is linked to the current project it is build in. 
        
                   # This is necessary in case we use Google Container registry - we always use the 
        
                   # Airflow version of the python image with the opencontainers label, so that GHCR can link it 
        
                   # to the Apache Airflow repository. 
        
                   docker pull "${PYTHON_BASE_IMAGE}" 
        
                   echo "FROM ${PYTHON_BASE_IMAGE}" | \ 
        
                       docker build \ 
        
                           --label "org.opencontainers.image.source=https://github.com/${GITHUB_REPOSITORY}" \ 
        
                           -t "${AIRFLOW_PYTHON_BASE_IMAGE}" - 
        
               else 
        
                   if [[ ${USE_GITHUB_REGISTRY} == "true" ]]; then 
        
                       PYTHON_TAG_SUFFIX="" 
        
                       if [[ ${GITHUB_REGISTRY_PULL_IMAGE_TAG} != "latest" ]]; then 
        
                           PYTHON_TAG_SUFFIX="-${GITHUB_REGISTRY_PULL_IMAGE_TAG}" 
        
                       fi 
        
                       push_pull_remove_images::pull_image_github_dockerhub "${AIRFLOW_PYTHON_BASE_IMAGE}" \ 
        
                           "${GITHUB_REGISTRY_PYTHON_BASE_IMAGE}${PYTHON_TAG_SUFFIX}" 
        
                   else 
        
                       docker pull "${AIRFLOW_PYTHON_BASE_IMAGE}" 
        
                   fi 
        
               fi 
        
           }

Jarek has explained and he is going to fix that in a follow-up PR

github-actions · 2021-03-10T23:14:11Z

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.

Turns out apache#14698 did not fix the issue as Master failed again. After digging a bit more I found that the CVE was fixed in all Python versions: 3.6.13, 3.7.10 & 3.8.8 The solution in this PR/commit checks the `parse_qsl` behavior with following tests: ``` ❯ docker run -it python:3.8-slim bash root@41120dfd035e:/# python Python 3.8.8 (default, Feb 19 2021, 18:07:06) >>> from urllib.parse import parse_qsl >>> parse_qsl(";a=b") [(';a', 'b')] >>> ``` ❯ docker run -it python:3.8.7-slim bash root@68e527725610:/# python Python 3.8.7 (default, Feb 9 2021, 08:21:15) >>> from urllib.parse import parse_qsl >>> parse_qsl(";a=b") [('a', 'b')] >>> ```

Turns out #14698 did not fix the issue as Master failed again. After digging a bit more I found that the CVE was fixed in all Python versions: 3.6.13, 3.7.10 & 3.8.8 The solution in this PR/commit checks the `parse_qsl` behavior with following tests: ``` ❯ docker run -it python:3.8-slim bash root@41120dfd035e:/# python Python 3.8.8 (default, Feb 19 2021, 18:07:06) >>> from urllib.parse import parse_qsl >>> parse_qsl(";a=b") [(';a', 'b')] >>> ``` ``` ❯ docker run -it python:3.8.7-slim bash root@68e527725610:/# python Python 3.8.7 (default, Feb 9 2021, 08:21:15) >>> from urllib.parse import parse_qsl >>> parse_qsl(";a=b") [('a', 'b')] >>> ```

potiuk · 2021-03-11T08:55:17Z

Nice! Thanks @kaxil !

Turns out #14698 did not fix the issue as Master failed again. After digging a bit more I found that the CVE was fixed in all Python versions: 3.6.13, 3.7.10 & 3.8.8 The solution in this PR/commit checks the `parse_qsl` behavior with following tests: ``` ❯ docker run -it python:3.8-slim bash root@41120dfd035e:/# python Python 3.8.8 (default, Feb 19 2021, 18:07:06) >>> from urllib.parse import parse_qsl >>> parse_qsl(";a=b") [(';a', 'b')] >>> ``` ``` ❯ docker run -it python:3.8.7-slim bash root@68e527725610:/# python Python 3.8.7 (default, Feb 9 2021, 08:21:15) >>> from urllib.parse import parse_qsl >>> parse_qsl(";a=b") [('a', 'b')] >>> ``` (cherry picked from commit 7bd9d47)

python/cpython#24297 change was included in Python 3.8.8 to fix a vulnerability (bpo-42967) Depending on which Base Python Image is run in our CI, two of the tests can fail or succeed. Our Previous two attempts: - 061cd23 - 49952e7 We might for a while get different base python version depending on the changes of a PR (whether or not it includes a change to dockerfiler). a) when you have PR which do not have changes in the Dockerfile, they will use the older python version as base (for example Python 3.8.7) b) when you have PR that touches the Dockerfile and have setup.py changes in master, it should pull Python 3.8.8 first. (cherry picked from commit ffe3bd2)

Turns out #14698 did not fix the issue as Master failed again. After digging a bit more I found that the CVE was fixed in all Python versions: 3.6.13, 3.7.10 & 3.8.8 The solution in this PR/commit checks the `parse_qsl` behavior with following tests: ``` ❯ docker run -it python:3.8-slim bash root@41120dfd035e:/# python Python 3.8.8 (default, Feb 19 2021, 18:07:06) >>> from urllib.parse import parse_qsl >>> parse_qsl(";a=b") [(';a', 'b')] >>> ``` ``` ❯ docker run -it python:3.8.7-slim bash root@68e527725610:/# python Python 3.8.7 (default, Feb 9 2021, 08:21:15) >>> from urllib.parse import parse_qsl >>> parse_qsl(";a=b") [('a', 'b')] >>> ``` (cherry picked from commit 7bd9d47)

python/cpython#24297 change was included in Python 3.8.8 to fix a vulnerability (bpo-42967) Depending on which Base Python Image is run in our CI, two of the tests can fail or succeed. Our Previous two attempts: - 061cd23 - 49952e7 We might for a while get different base python version depending on the changes of a PR (whether or not it includes a change to dockerfiler). a) when you have PR which do not have changes in the Dockerfile, they will use the older python version as base (for example Python 3.8.7) b) when you have PR that touches the Dockerfile and have setup.py changes in master, it should pull Python 3.8.8 first. (cherry picked from commit ffe3bd2)

Turns out #14698 did not fix the issue as Master failed again. After digging a bit more I found that the CVE was fixed in all Python versions: 3.6.13, 3.7.10 & 3.8.8 The solution in this PR/commit checks the `parse_qsl` behavior with following tests: ``` ❯ docker run -it python:3.8-slim bash root@41120dfd035e:/# python Python 3.8.8 (default, Feb 19 2021, 18:07:06) >>> from urllib.parse import parse_qsl >>> parse_qsl(";a=b") [(';a', 'b')] >>> ``` ``` ❯ docker run -it python:3.8.7-slim bash root@68e527725610:/# python Python 3.8.7 (default, Feb 9 2021, 08:21:15) >>> from urllib.parse import parse_qsl >>> parse_qsl(";a=b") [('a', 'b')] >>> ``` (cherry picked from commit 7bd9d47)

kaxil requested review from ashb and potiuk March 10, 2021 15:05

boring-cyborg bot added the area:webserver Webserver related Issues label Mar 10, 2021

kaxil commented Mar 10, 2021

View reviewed changes

kaxil requested a review from XD-DENG March 10, 2021 15:06

kaxil changed the title ~~Separate out tests to cater of changes in Python 3.8.8~~ Separate out tests to cater for changes in Python 3.8.8 Mar 10, 2021

XD-DENG reviewed Mar 10, 2021

View reviewed changes

tests/www/test_views.py Outdated

Copy link

Member

XD-DENG Mar 10, 2021

Choose a reason for hiding this comment

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

This line is missed to be updated?

kaxil force-pushed the fix-error branch from f0d289f to f656f98 Compare March 10, 2021 20:17

kaxil requested review from eladkal and ephraimbuddy March 10, 2021 23:00

vikramkoka approved these changes Mar 10, 2021

View reviewed changes

github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Mar 10, 2021

kaxil merged commit ffe3bd2 into apache:master Mar 10, 2021

kaxil deleted the fix-error branch March 10, 2021 23:15

kaxil mentioned this pull request Mar 11, 2021

Fix tests for all urllib versions with only '&' as separator #14710

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Separate out tests to cater for changes in Python 3.8.8 #14698

Separate out tests to cater for changes in Python 3.8.8 #14698

Uh oh!

kaxil commented Mar 10, 2021

Uh oh!

kaxil Mar 10, 2021

Uh oh!

XD-DENG Mar 10, 2021

Uh oh!

ashb commented Mar 10, 2021

Uh oh!

kaxil commented Mar 10, 2021

Uh oh!

github-actions bot commented Mar 10, 2021

Uh oh!

potiuk commented Mar 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Separate out tests to cater for changes in Python 3.8.8 #14698

Separate out tests to cater for changes in Python 3.8.8 #14698

Uh oh!

Conversation

kaxil commented Mar 10, 2021

Uh oh!

kaxil Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

XD-DENG Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

ashb commented Mar 10, 2021

Uh oh!

kaxil commented Mar 10, 2021

Uh oh!

github-actions bot commented Mar 10, 2021

Uh oh!

potiuk commented Mar 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants