Skip to content

[improve][cpp-client] Add basic authentication #15822

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
Merged
nodece merged 2 commits into from
Aug 5, 2022
Merged

[improve][cpp-client] Add basic authentication #15822

nodece merged 2 commits into from
Aug 5, 2022

Conversation

@nodece
Copy link
Member

@nodece nodece commented May 28, 2022 •
edited
Loading

Signed-off-by: Zixuan Liu nodeces@gmail.com

Motivation

The basic authentication is missed in cpp-client.

Modifications

  • Added the basic authentication implement
  • Added the basic authentication test

Documentation

@github-actions github-actions bot added the doc-required Your PR changes impact docs and you will update later. label May 28, 2022
@nodece nodece force-pushed the basic-authencation-cpp-client branch 6 times, most recently from 40ca481 to 795f94d Compare May 30, 2022 10:51
@nodece
Copy link
Member Author

nodece commented May 31, 2022

/pulsarbot rerun-failure-checks

@nodece nodece force-pushed the basic-authencation-cpp-client branch from 795f94d to 9f556d3 Compare June 6, 2022 02:48
@momo-jun momo-jun mentioned this pull request Jun 16, 2022
Closed
@nodece nodece force-pushed the basic-authencation-cpp-client branch from 9f556d3 to 58e6d1e Compare June 16, 2022 09:38
@BewareMyPower BewareMyPower added type/feature The PR added a new feature or issue requested a new feature component/client-c++ labels Jun 20, 2022
BewareMyPower
BewareMyPower reviewed Jun 20, 2022
View reviewed changes
BewareMyPower
BewareMyPower requested changes Jun 20, 2022
View reviewed changes
BewareMyPower
BewareMyPower reviewed Jun 20, 2022
View reviewed changes
BewareMyPower
BewareMyPower reviewed Jun 20, 2022
View reviewed changes
@nodece nodece force-pushed the basic-authencation-cpp-client branch 2 times, most recently from 17a8cad to 72689b7 Compare June 23, 2022 06:26
@nodece nodece requested a review from BewareMyPower June 29, 2022 02:26
@nodece
Copy link
Member Author

nodece commented Jun 29, 2022

@BewareMyPower Thanks for your review, your request changes has been fixed.

@BewareMyPower
Copy link
Contributor

BewareMyPower commented Jun 29, 2022

Please check the failed tests introduced by this PR:

     287 ms: ./main AuthPluginBasic.testNoAuth (try #1)
     309 ms: ./main AuthPluginBasic.testNoAuthWithHttp (try #1)
     257 ms: ./main AuthPluginBasic.testNoAuthWithHttp (try #2)
     288 ms: ./main AuthPluginBasic.testNoAuth (try #2)

@BewareMyPower
Copy link
Contributor

BewareMyPower commented Jun 29, 2022

It looks like these tests should fail with ResultAuthorizationError because the standalone is configured with an anonymous role.

2022-06-29T17:48:01,551+0800 [pulsar-io-15-2] WARN  org.apache.pulsar.broker.service.ServerCnx - Role anonymous is not authorized to perform operation LOOKUP on topic persistent://private/auth/test-basic
2022-06-29T17:48:01,551+0800 [pulsar-io-15-2] WARN  org.apache.pulsar.broker.service.ServerCnx - [/127.0.0.1:63718] Proxy Client is not authorized to Get Partition Metadata with role anonymous on topic persistent://private/auth/test-basic

I'm not sure whether the anonymous role can be cancelled for a specific namespace.

@nodece nodece force-pushed the basic-authencation-cpp-client branch from c5371c7 to 898df31 Compare June 29, 2022 09:51
@nodece nodece force-pushed the basic-authencation-cpp-client branch from 898df31 to b38d89f Compare June 29, 2022 11:13
@nodece nodece requested a review from codelipenghui June 29, 2022 15:56
@nodece
Copy link
Member Author

nodece commented Jun 30, 2022

/pulsarbot rerun-failure-checks

nodece added 2 commits July 12, 2022 22:28
@nodece
[improve][cpp-client] Add basic authentication
fd5b582 
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@nodece
Fix test
4addb12 
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@nodece nodece force-pushed the basic-authencation-cpp-client branch from b38d89f to 4addb12 Compare July 12, 2022 14:28
@nodece nodece merged commit 4840a98 into apache:master Aug 5, 2022
@codelipenghui codelipenghui added this to the 2.11.0 milestone Aug 5, 2022
@nodece nodece mentioned this pull request Aug 5, 2022
Merged
4 tasks
@github-actions github-actions bot added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-required Your PR changes impact docs and you will update later. labels Aug 5, 2022
codelipenghui pushed a commit that referenced this pull request Aug 5, 2022
@nodece @codelipenghui
[improve][cpp-client] Add basic authentication (#15822)
d82d629 
* [improve][cpp-client] Add basic authentication

Signed-off-by: Zixuan Liu <nodeces@gmail.com>

* Fix test

Signed-off-by: Zixuan Liu <nodeces@gmail.com>
(cherry picked from commit 4840a98)
@BewareMyPower
Copy link
Contributor

BewareMyPower commented Aug 5, 2022

It's a new feature, we should not cherry-pick it to release branches.

@nodece
Copy link
Member Author

nodece commented Aug 5, 2022

Brokers have long supported basic authentication, but clients have not, so I think we should cherry-pick.

@BewareMyPower
Copy link
Contributor

BewareMyPower commented Aug 5, 2022

Maybe you need to open a discussion in mail list.

BTW, @merlimat what's your opinion about cherry-picking new features of C++ client into release branches?

@nodece
Copy link
Member Author

nodece commented Aug 6, 2022

@BewareMyPower I have a question to ask if the c++-client will only have one latest version, this is different from Java-Client, which comes in multiple versions.

@BewareMyPower
Copy link
Contributor

BewareMyPower commented Aug 7, 2022

if the c++-client will only have one latest version

No. The version management is the same with Java client.

@BewareMyPower BewareMyPower mentioned this pull request Aug 9, 2022
Merged
5 tasks
Technoboy- pushed a commit to merlimat/pulsar that referenced this pull request Aug 16, 2022
@nodece @Technoboy-
[improve][cpp-client] Add basic authentication (apache#15822)
4201117 
* [improve][cpp-client] Add basic authentication

Signed-off-by: Zixuan Liu <nodeces@gmail.com>

* Fix test

Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@codelipenghui codelipenghui mentioned this pull request Sep 6, 2022
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BewareMyPower BewareMyPower BewareMyPower approved these changes

@gaoran10 gaoran10 Awaiting requested review from gaoran10

@lhotari lhotari Awaiting requested review from lhotari

@merlimat merlimat Awaiting requested review from merlimat

@rdhabalia rdhabalia Awaiting requested review from rdhabalia

@RobertIndie RobertIndie Awaiting requested review from RobertIndie

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

Assignees

@nodece nodece

Labels

doc-complete Your PR changes impact docs and the related docs have been already added. type/feature The PR added a new feature or issue requested a new feature

Projects

None yet

Milestone

2.11.0

Development

Successfully merging this pull request may close these issues.

3 participants

@nodece @BewareMyPower @codelipenghui