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.

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

Clean up C++ client curl configuration #16064

Merged
merged 1 commit into from
Jun 14, 2022

Conversation

michaeljmarshall
Copy link
Member

@michaeljmarshall michaeljmarshall commented Jun 14, 2022

Motivation

Clean up C++ client curl configuration.

Modifications

  • Remove some settings

Verifying this change

This is a minor change with well known scope. No additional tests are required.

  • doc-not-needed

@michaeljmarshall michaeljmarshall added component/client-c++ type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use release/2.8.4 release/2.10.2 release/2.9.4 labels Jun 14, 2022
@michaeljmarshall michaeljmarshall added this to the 2.11.0 milestone Jun 14, 2022
@michaeljmarshall michaeljmarshall self-assigned this Jun 14, 2022
@michaeljmarshall michaeljmarshall added the doc-not-needed Your PR changes do not impact docs label Jun 14, 2022
Copy link
Member

@dave2wave dave2wave left a comment

Choose a reason for hiding this comment

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

LGTM

@dave2wave dave2wave merged commit 48c575d into apache:master Jun 14, 2022
@merlimat merlimat added cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 labels Jun 15, 2022
@michaeljmarshall michaeljmarshall deleted the update-configuration branch June 15, 2022 02:37
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jun 15, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jul 5, 2022
michaeljmarshall added a commit that referenced this pull request Aug 25, 2022
michaeljmarshall added a commit to datastax/pulsar that referenced this pull request Sep 16, 2022
Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

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

Could you explain the motivation of this PR? The default values of these two configs are 1, which enables strict certificates check. I think it was zero are just to disable the check.

We observed an error like:

2022-10-20 02:42:38.545 ERROR [140011702527744] AuthOauth2:223 | Response failed for getting the well-known configuration <xxx>. Error Code 77: error setting certificate verify locations:
  CAfile: /etc/pki/tls/certs/ca-bundle.crt
  CApath: none

It seems to be related to this PR, see https://curl.se/libcurl/c/CURLOPT_SSL_VERIFYPEER.html

This trust is based on a chain of digital signatures, rooted in certification authority (CA) certificates you supply. curl uses a default bundle of CA certificates (the path for that is determined at build time)

/cc @freeznet @jiangpengcheng

@BewareMyPower
Copy link
Contributor

BewareMyPower commented Nov 2, 2022

I have confirmed it's caused by this PR and will revert it from branch-2.7 to 2.11. Though we should encourage users to use new separated pulsar-client-cpp and pulsar-client-python repos, I will confirm again for these new repos.

Reproduce

Checkout branch-2.9, run the following command.

$ pulsar-client-cpp/docker/build-wheels.sh 3.8 cp38-cp38 manylinux2014 x86_64

Then pulsar_client-2.9.3-cp38-cp38-linux_x86_64.whl will be generated under pulsar-client-cpp/python/dist. Then install the wheel in a ubuntu:20.04 container.

docker run -v $PWD:/pulsar -it ubuntu:20.04 /bin/bash
apt update -y
apt install -y python3 python3-pip
pip3 install /pulsar/pulsar-client-cpp/python/dist/pulsar_client-2.9.3-cp38-cp38-linux_x86_64.whl

Then run the following script:

#!/usr/bin/env python3
from pulsar import AuthenticationOauth2

auth = AuthenticationOauth2(auth_params_string='''{
    "type": "client_credentials",
    "issuer_url": "https://dev-kt-aa9ne.us.auth0.com",
    "client_id": "xxx",
    "client_secret": "xxx"
}''')

Output:

2022-11-02 09:28:19.003 ERROR [140425586124608] AuthOauth2:222 | Response failed for getting the well-known configuration https://dev-kt-aa9ne.us.auth0.com. Error Code 77: error setting certificate verify locations:
  CAfile: /etc/pki/tls/certs/ca-bundle.crt
  CApath: none

After I reverted this PR, it works.

image

/cc @michaeljmarshall @merlimat @dave2wave @freeznet @jiangpengcheng @mattisonchao

BewareMyPower added a commit that referenced this pull request Nov 2, 2022
BewareMyPower added a commit that referenced this pull request Nov 2, 2022
BewareMyPower added a commit that referenced this pull request Nov 2, 2022
BewareMyPower added a commit that referenced this pull request Nov 2, 2022
BewareMyPower added a commit that referenced this pull request Nov 2, 2022
@michaeljmarshall
Copy link
Member Author

@BewareMyPower - this should not have been reverted. Please see this stack overflow for help solving your environment's issue: https://stackoverflow.com/questions/3160909/how-do-i-deal-with-certificates-using-curl-while-trying-to-access-an-https-url.

BewareMyPower added a commit that referenced this pull request Nov 2, 2022
BewareMyPower added a commit that referenced this pull request Nov 2, 2022
BewareMyPower added a commit that referenced this pull request Nov 2, 2022
BewareMyPower added a commit that referenced this pull request Nov 2, 2022
BewareMyPower added a commit that referenced this pull request Nov 2, 2022
@BewareMyPower
Copy link
Contributor

They are restored now. I will confirm the solution soon.

@michaeljmarshall
Copy link
Member Author

Thanks @BewareMyPower.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked/branch-2.7 Archived: 2.7 is end of life cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 doc-not-needed Your PR changes do not impact docs release/2.7.5 release/2.8.4 release/2.9.3 release/2.10.2 type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants