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

optional auth method name header in http authentication #6799

Merged
merged 4 commits into from
May 11, 2021
Merged

optional auth method name header in http authentication #6799

merged 4 commits into from
May 11, 2021

Conversation

KannarFr
Copy link
Contributor

No description provided.

@KannarFr KannarFr closed this Apr 22, 2020
@KannarFr KannarFr reopened this Apr 22, 2020
sijie
sijie reviewed Apr 23, 2020
View reviewed changes
Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

the change looks good to me. If you can add a test case for it, that would be great!

...oker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationService.java Show resolved Hide resolved
@KannarFr
Copy link
Contributor Author

rebased

@sijie
Copy link
Member

sijie commented Apr 27, 2020

/pulsarbot run-failure-checks

@sijie sijie added area/security release/2.5.2 type/feature The PR added a new feature or issue requested a new feature labels Apr 27, 2020
@sijie sijie added this to the 2.6.0 milestone Apr 27, 2020
@sijie
Copy link
Member

sijie commented May 7, 2020

/pulsarbot run-failure-checks

3 similar comments
@sijie
Copy link
Member

sijie commented May 9, 2020

/pulsarbot run-failure-checks

@jiazhai
Copy link
Member

jiazhai commented May 11, 2020

/pulsarbot run-failure-checks

@jiazhai
Copy link
Member

jiazhai commented May 11, 2020

/pulsarbot run-failure-checks

@jiazhai
Copy link
Member

jiazhai commented May 11, 2020

@KannarFr Looks like cpp and java unit tests failure related with this change. would you please help fix them？
https://github.com/apache/pulsar/pull/6799/checks?check_run_id=662938527
https://github.com/apache/pulsar/pull/6799/checks?check_run_id=662938425

[ RUN      ] AuthPluginTest.testAuthFactoryAthenz
2020-05-11 11:02:14.200 INFO  AuthPluginTest:314 | PARAMS: {
        "tenantDomain": "pulsar.test2.tenant",
        "tenantService": "service",
        "providerDomain": "pulsar.test.provider",
        "privateKey": "file:../../pulsar-broker/src/test/resources/authentication/tls/client-key.pem",
        "ztsUrl": "http://localhost:9998"
    }
2020-05-11 11:02:14.208 ERROR ZTSClient:350 | Response failed for url http://localhost:9998/zts/v1/domain/pulsar.test.provider/token. Error Code 7
/pulsar/pulsar-client-cpp/tests/AuthPluginTest.cc:320: Failure

And this looks like more of an improvement, not a bug, would like to remove it from tag 2.5.2. If it is really needed, we could add it back.

@codelipenghui codelipenghui mentioned this pull request May 19, 2020
Closed
@sijie
Copy link
Member

sijie commented May 26, 2020

/pulsarbot run-failure-checks

@codelipenghui
Copy link
Contributor

@KannarFr Could you please rebase to the master branch?

1 similar comment
@codelipenghui
Copy link
Contributor

@KannarFr Could you please rebase to the master branch?

@KannarFr
Copy link
Contributor Author

KannarFr commented Jun 6, 2020

@codelipenghui done.

@codelipenghui
Copy link
Contributor

/pulsarbot run-failure-checks

1 similar comment
@codelipenghui
Copy link
Contributor

/pulsarbot run-failure-checks

@codelipenghui
Copy link
Contributor

@KannarFr The failed CI tests are related to this change, could you please take a look? Or we can move it to 2.6.1/2.7.0

@codelipenghui
Copy link
Contributor

move to 2.7.0 first.

@codelipenghui codelipenghui modified the milestones: 2.6.0, 2.7.0 Jun 8, 2020
@KannarFr KannarFr force-pushed the httpspecifyauthprovider branch 2 times, most recently from ffe527a to 3dfb3e2 Compare August 13, 2020 15:11
@codelipenghui codelipenghui modified the milestones: 2.7.0, 2.8.0 Nov 4, 2020
merlimat
merlimat reviewed May 4, 2021
View reviewed changes
Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

Change looks mostly good. Just few comments and we should have some tests to validate that the client is selecting the particular Auth method when the broker supports multiple of them.

...oker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationService.java Outdated Show resolved Hide resolved
...oker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationService.java Outdated Show resolved Hide resolved
pulsar-client/src/test/java/org/apache/pulsar/client/impl/schema/AvroSchemaTest.java Outdated Show resolved Hide resolved
@KannarFr
Copy link
Contributor Author

KannarFr commented May 4, 2021

@merlimat I added test 8694c4b.

@KannarFr
Copy link
Contributor Author

KannarFr commented May 6, 2021

flaky tests?

@KannarFr
Copy link
Contributor Author

Can we merge it?

eolivelli
eolivelli approved these changes May 11, 2021
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Lgtm

@eolivelli eolivelli merged commit 21c9c81 into apache:master May 11, 2021
eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request May 11, 2021
@KannarFr @eolivelli
optional auth method name header in http authentication (apache#6799)
4e5d423
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merlimat merlimat merlimat approved these changes

@sijie sijie sijie approved these changes

@jiazhai jiazhai jiazhai approved these changes

@eolivelli eolivelli eolivelli approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

Assignees

@KannarFr KannarFr

Labels
area/security type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Milestone
2.8.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@KannarFr @sijie @jiazhai @codelipenghui @merlimat @eolivelli