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

Add refresh authentication command in broker #9064

Merged
merged 12 commits into from
Jan 26, 2021

Conversation

zymap
Copy link
Member

@zymap zymap commented Dec 25, 2020


Motivation

Some authentication provider is using cached authentication data. Until
redoing the 'getAuthData' then it will provide the new auth data. So
I add a refresh command and do the refresh authentication data provider
when received the command.

Verify this change

  • Existent tests can pass.

---

(If this PR fixes a github issue, please add `Fixes #<xyz>`)

Fixes #<xyz>

(or if this PR is one task of a github issue, please add `Master Issue: #<xyz>`
to link to the master issue)

Master Issue: #<xyz>

*Motivation*

Describe here the context, and why you're making that change.
What is the problem you're trying to solve.

*Modifications*

Describe the modifications you've done.

*Verify this change*

Please pick either of following options.

- This change is a trivial rework / code cleanup without any test coverage.
- This change is already covered by existing tests, such as *(please describe tests)*.
- This change added tests and can be verified as follows:
  *(example:)*
  - *Added integration tests for end-to-end deployment*
  - *Extended integration test for recovery after broker failure*
@zymap zymap force-pushed the allow-broker-send-refresh-cmd branch from 8621434 to c469ca3 Compare December 28, 2020 07:15
@zymap zymap changed the title (WIP) Add refresh authentication command in broker Add refresh authentication command in broker Dec 28, 2020
@zymap
Copy link
Member Author

zymap commented Dec 29, 2020

/pulsarbot run-failure-checks

@zymap
Copy link
Member Author

zymap commented Dec 30, 2020

/pulsarbot run-failure-checks

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.

A couple of questions:

  • Don't we need to change the client authentication providers to support fresh authentication?
  • Can you please add tests and integration tests to ensure the change is implemented correctly?

@sijie sijie added this to the 2.8.0 milestone Dec 30, 2020
@zymap
Copy link
Member Author

zymap commented Jan 18, 2021

@sijie sorry for the delay. This PR's goal is to address the issue that pulsar will disconnect the client if the token expired.
Currently, if the token expired the broker will disconnect the client and make it reconnect to do the authentication again. Because we get the authentication data when a new connection opened and it will not change until it reconnected.
We handle the refresh command to let the client can replace the token but doesn't need to disconnect from the broker. The client-side still get a token from the method getAuthData which is abstracted in the AuthenticationDataProvider, user can handle the token generates logic at the method getAuthData.
/cc @jiazhai Correct me if I missed something.

I improve the expiration token test to include my case.

@zymap
Copy link
Member Author

zymap commented Jan 18, 2021

/pulsarbot run-failure-checks

2 similar comments
@zymap
Copy link
Member Author

zymap commented Jan 19, 2021

/pulsarbot run-failure-checks

@zymap
Copy link
Member Author

zymap commented Jan 19, 2021

/pulsarbot run-failure-checks

@zymap
Copy link
Member Author

zymap commented Jan 20, 2021

ping @sijie @jiazhai

@zymap
Copy link
Member Author

zymap commented Jan 26, 2021

/pulsarbot run-failure-checks

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.

@zymap well done!

@zymap zymap merged commit 373948e into apache:master Jan 26, 2021
@zymap zymap deleted the allow-broker-send-refresh-cmd branch January 26, 2021 10:11
codelipenghui pushed a commit that referenced this pull request Jan 26, 2021
* Add refresh authentication command in the broker
---

**Motivation**

Some authentication provider is using cached authentication data. Until
redoing the 'getAuthData' then it will provide the new auth data. So
I add a refresh command and do the refresh authentication data provider
when received the command.

**Verify this change**

Existent tests can pass.

(cherry picked from commit 373948e)
@codelipenghui codelipenghui added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Jan 26, 2021
codelipenghui pushed a commit that referenced this pull request Jan 26, 2021
* Add refresh authentication command in the broker
---

**Motivation**

Some authentication provider is using cached authentication data. Until
redoing the 'getAuthData' then it will provide the new auth data. So
I add a refresh command and do the refresh authentication data provider
when received the command.

**Verify this change**

Existent tests can pass.

(cherry picked from commit 373948e)
codelipenghui pushed a commit that referenced this pull request Jan 26, 2021
* Add refresh authentication command in the broker
---

**Motivation**

Some authentication provider is using cached authentication data. Until
redoing the 'getAuthData' then it will provide the new auth data. So
I add a refresh command and do the refresh authentication data provider
when received the command.

**Verify this change**

Existent tests can pass.

(cherry picked from commit 373948e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants