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

Add missing load_default_certs() call. #350

Merged
merged 1 commit into from
Jan 27, 2021
Merged

Add missing load_default_certs() call. #350

merged 1 commit into from
Jan 27, 2021

Conversation

moisesguimaraes
Copy link
Contributor

Fixes: #349

@matusvalo
Copy link
Member

Thank you @moisesguimaraes for your PR. Can you also adjust integration tests with case covering this PR? Now they are not failing on master:

$ pytest -sv -E rabbitmq t/integration/test_rmq.py
Test session starts (platform: linux, Python 3.8.5, pytest 5.3.5, pytest-sugar 0.9.4)
cachedir: .pytest_cache
rootdir: /home/matus/dev/py-amqp, inifile: setup.cfg
plugins: rerunfailures-9.1.1, forked-1.3.0, sugar-0.9.4, xdist-2.1.0, cov-2.10.1, travis-fold-1.3.0, case-1.5.3
collecting ...
 t/integration/test_rmq.py::test_connect[plain] ✓                                                                                                                                                   5% ▌
 t/integration/test_rmq.py::test_connect[tls] ✓                                                                                                                                                    10% ▉
 t/integration/test_rmq.py::test_tls_connect_fails ✓                                                                                                                                               14% █▌
 t/integration/test_rmq.py::test_rabbitmq_operations.test_publish_consume[plain-basic_publish-False-True] ✓                                                                                        19% █▉
 t/integration/test_rmq.py::test_rabbitmq_operations.test_publish_consume[plain-basic_publish-True-True] ✓                                                                                         24% ██▍
 t/integration/test_rmq.py::test_rabbitmq_operations.test_publish_consume[plain-basic_publish-False-False] ✓                                                                                       29% ██▉
 t/integration/test_rmq.py::test_rabbitmq_operations.test_publish_consume[plain-basic_publish-True-False] ✓                                                                                        33% ███▍
 t/integration/test_rmq.py::test_rabbitmq_operations.test_publish_consume[plain-basic_publish_confirm-False-True] ✓                                                                                38% ███▊
 t/integration/test_rmq.py::test_rabbitmq_operations.test_publish_consume[plain-basic_publish_confirm-True-True] ✓                                                                                 43% ████▍
 t/integration/test_rmq.py::test_rabbitmq_operations.test_publish_consume[plain-basic_publish_confirm-False-False] ✓                                                                               48% ████▊
 t/integration/test_rmq.py::test_rabbitmq_operations.test_publish_consume[plain-basic_publish_confirm-True-False] ✓                                                                                52% █████▎
 t/integration/test_rmq.py::test_rabbitmq_operations.test_publish_consume[tls-basic_publish-False-True] ✓                                                                                          57% █████▊
 t/integration/test_rmq.py::test_rabbitmq_operations.test_publish_consume[tls-basic_publish-True-True] ✓                                                                                           62% ██████▎
 t/integration/test_rmq.py::test_rabbitmq_operations.test_publish_consume[tls-basic_publish-False-False] ✓                                                                                         67% ██████▋
 t/integration/test_rmq.py::test_rabbitmq_operations.test_publish_consume[tls-basic_publish-True-False] ✓                                                                                          71% ███████▎
 t/integration/test_rmq.py::test_rabbitmq_operations.test_publish_consume[tls-basic_publish_confirm-False-True] ✓                                                                                  76% ███████▋
 t/integration/test_rmq.py::test_rabbitmq_operations.test_publish_consume[tls-basic_publish_confirm-True-True] ✓                                                                                   81% ████████▏
 t/integration/test_rmq.py::test_rabbitmq_operations.test_publish_consume[tls-basic_publish_confirm-False-False] ✓                                                                                 86% ████████▋
 t/integration/test_rmq.py::test_rabbitmq_operations.test_publish_consume[tls-basic_publish_confirm-True-False] ✓                                                                                  90% █████████▏
 t/integration/test_rmq.py::test_rabbitmq_operations.test_publish_get[plain] ✓                                                                                                                     95% █████████▌
 t/integration/test_rmq.py::test_rabbitmq_operations.test_publish_get[tls] ✓                                                                                                                      100% ██████████

Results (0.54s):
      21 passed

moisesguimaraes
moisesguimaraes commented Jan 26, 2021
View reviewed changes
t/unit/test_transport.py
context.set_ciphers.assert_not_called()
context.verify_mode.assert_not_called()

context.load_default_certs.assert_called_with(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@matusvalo I have tests to cover the new code path already.

Copy link
Member

@matusvalo matusvalo Jan 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

yes I saw. But I was curious why integration test creating real tls connection to broker suit does not fail on master. They should fail right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They should only fail when you try to load system certs. I added new tests for this case.

@moisesguimaraes
Copy link
Contributor Author

Ok, added some integration tests.

@auvipy auvipy mentioned this pull request Jan 27, 2021
Closed
@auvipy
Copy link
Member

auvipy commented Jan 27, 2021

Ok, added some integration tests.

Our CI is not working, so please check on you repo if they are working fine.

auvipy
auvipy reviewed Jan 27, 2021
View reviewed changes
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

looks good to me but waiting for confirmation

@matusvalo
Copy link
Member

I will check it today.

@matusvalo
Copy link
Member

OK I have checked the PR and it looks great! Thank you @moisesguimaraes ! I also like the integration test. Thanks again!

@matusvalo matusvalo merged commit 0b8a832 into celery:master Jan 27, 2021
@auvipy
Copy link
Member

auvipy commented Jan 27, 2021

i will try to release tonite or tomorrw morning

athos-ribeiro pushed a commit to athos-ribeiro/cachito that referenced this pull request Feb 10, 2021
Pin py-amqp version
568d03a 
A recent py-ampq update is causing cachito workers to fail to get local
certificates:

```
ssl.SSLCertVerificationError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1123)
```

This has been reported upstream on
celery/py-amqp#349 and fixed on
celery/py-amqp#350.

However, even with the fix, provided by Fedora on
https://bodhi.fedoraproject.org/updates/FEDORA-2021-904397f5c4, Cachito
workers still fail to connect to rabbitmq (with the same error) when
enforcing SSL connections.

Let's pin py-amqp version to the latest version known to work with
Cachito so we do not need to halt development/deployments while we deal
with the amqp issue.

Signed-off-by: Athos Ribeiro <athos@redhat.com>
@athos-ribeiro athos-ribeiro mentioned this pull request Feb 10, 2021
Merged
athos-ribeiro pushed a commit to containerbuildsystem/cachito that referenced this pull request Feb 10, 2021
@athos-ribeiro
Pin py-amqp version
91ef95c 
A recent py-ampq update is causing cachito workers to fail to get local
certificates:

```
ssl.SSLCertVerificationError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1123)
```

This has been reported upstream on
celery/py-amqp#349 and fixed on
celery/py-amqp#350.

However, even with the fix, provided by Fedora on
https://bodhi.fedoraproject.org/updates/FEDORA-2021-904397f5c4, Cachito
workers still fail to connect to rabbitmq (with the same error) when
enforcing SSL connections.

Let's pin py-amqp version to the latest version known to work with
Cachito so we do not need to halt development/deployments while we deal
with the amqp issue.

Signed-off-by: Athos Ribeiro <athos@redhat.com>
softdevm pushed a commit to softdevm/cachito-app that referenced this pull request Sep 2, 2022
Pin py-amqp version
5184f1e 
A recent py-ampq update is causing cachito workers to fail to get local
certificates:

```
ssl.SSLCertVerificationError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1123)
```

This has been reported upstream on
celery/py-amqp#349 and fixed on
celery/py-amqp#350.

However, even with the fix, provided by Fedora on
https://bodhi.fedoraproject.org/updates/FEDORA-2021-904397f5c4, Cachito
workers still fail to connect to rabbitmq (with the same error) when
enforcing SSL connections.

Let's pin py-amqp version to the latest version known to work with
Cachito so we do not need to halt development/deployments while we deal
with the amqp issue.

Signed-off-by: Athos Ribeiro <athos@redhat.com>
softdev87 added a commit to softdev87/cachito-app that referenced this pull request Sep 8, 2022
@softdev87
Pin py-amqp version
be66446 
A recent py-ampq update is causing cachito workers to fail to get local
certificates:

```
ssl.SSLCertVerificationError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1123)
```

This has been reported upstream on
celery/py-amqp#349 and fixed on
celery/py-amqp#350.

However, even with the fix, provided by Fedora on
https://bodhi.fedoraproject.org/updates/FEDORA-2021-904397f5c4, Cachito
workers still fail to connect to rabbitmq (with the same error) when
enforcing SSL connections.

Let's pin py-amqp version to the latest version known to work with
Cachito so we do not need to halt development/deployments while we deal
with the amqp issue.

Signed-off-by: Athos Ribeiro <athos@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matusvalo matusvalo matusvalo left review comments

@auvipy auvipy auvipy left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5.0.3 Regression with SSL handshake
3 participants
@moisesguimaraes @matusvalo @auvipy