-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[fix][proxy] Fix refresh client auth #17831
Conversation
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java
Outdated
Show resolved
Hide resolved
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java
Outdated
Show resolved
Hide resolved
pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyClientCnx.java
Outdated
Show resolved
Hide resolved
pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyClientCnx.java
Outdated
Show resolved
Hide resolved
pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyRefreshAuthTest.java
Outdated
Show resolved
Hide resolved
pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyRefreshAuthTest.java
Outdated
Show resolved
Hide resolved
pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyRefreshAuthTest.java
Outdated
Show resolved
Hide resolved
pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyRefreshAuthTest.java
Outdated
Show resolved
Hide resolved
pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyRefreshAuthTest.java
Outdated
Show resolved
Hide resolved
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java
Outdated
Show resolved
Hide resolved
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java
Outdated
Show resolved
Hide resolved
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java
Outdated
Show resolved
Hide resolved
pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyClientCnx.java
Outdated
Show resolved
Hide resolved
pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java
Outdated
Show resolved
Hide resolved
de3ab59
to
c2a83b8
Compare
@lin-zhao @codelipenghui Thank you all! These changes have been fixed, please help to review. |
eb1de4b
to
e5cb029
Compare
Signed-off-by: Zixuan Liu <[email protected]>
3ddeb0c
to
72654b3
Compare
Signed-off-by: Zixuan Liu <[email protected]>
+ "the proxy client {}", proxyConnection.ctx().channel(), ctx.channel()); | ||
} | ||
|
||
proxyConnection.ctx().writeAndFlush(Commands.newAuthChallenge(clientAuthMethod, AuthData.REFRESH_AUTH_DATA, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we can make some improvements to avoid sending auth challenge command to the client if the proxy just has done the auth challenge. Because all the brokers that interacted with client connection will send the auth challenge command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it could be improved, but I remember there's only one connection here.
* [fix][proxy] Fix refresh client auth Signed-off-by: Zixuan Liu <[email protected]> * Fix style Signed-off-by: Zixuan Liu <[email protected]> (cherry picked from commit c952f3c)
* [fix][proxy] Fix refresh client auth Signed-off-by: Zixuan Liu <[email protected]> * Fix style Signed-off-by: Zixuan Liu <[email protected]> (cherry picked from commit c952f3c) Signed-off-by: Zixuan Liu <[email protected]>
* [fix][proxy] Fix refresh client auth Signed-off-by: Zixuan Liu <[email protected]> * Fix style Signed-off-by: Zixuan Liu <[email protected]> (cherry picked from commit c952f3c) Signed-off-by: Zixuan Liu <[email protected]>
I think that this change should be reverted. This can cause corruption of the traffic between the client and the proxy. |
I just fix the ProxyLookupRequests connection, so don't break other connections. |
} | ||
|
||
try { | ||
AuthData clientData = AuthData.of(authResponse.getResponse().getAuthData()); | ||
doAuthentication(clientData); | ||
if (service.getConfiguration().isForwardAuthorizationCredentials() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An interesting consequence of this change is that the ProxyConnection
goes back into the Connecting
state if the authentication takes multiple steps.
Relates to: #17831 (comment) ### Motivation When the `ProxyConnection` handles a `Connect` command, that is the time to go to `Connecting` state. There is no other time that makes sense to switch to connecting. The current logic will go to connecting in certain re-authentication scenarios, but those are incorrect. By moving the state change to earlier in the logic, we make the state transition clearer and prevent corrupted state. ### Modifications * Remove `state = State.Connecting` from the `doAuthentication` method, which is called multiple times for various reasons * Add `state = State.Connecting` to the start of the `handleConnect` method. ### Verifying this change The existing tests will verify this change, and reading through the code makes it clear this is a correct change. ### Does this pull request potentially affect one of the following parts: Not a breaking change. ### Documentation - [x] `doc-not-needed` It would be nice to map out the state transitions for our connection classes. That is our of the scope of this small improvement. ### Matching PR in forked repository PR in forked repository: michaeljmarshall#21
Relates to: #17831 (comment) ### Motivation When the `ProxyConnection` handles a `Connect` command, that is the time to go to `Connecting` state. There is no other time that makes sense to switch to connecting. The current logic will go to connecting in certain re-authentication scenarios, but those are incorrect. By moving the state change to earlier in the logic, we make the state transition clearer and prevent corrupted state. ### Modifications * Remove `state = State.Connecting` from the `doAuthentication` method, which is called multiple times for various reasons * Add `state = State.Connecting` to the start of the `handleConnect` method. ### Verifying this change The existing tests will verify this change, and reading through the code makes it clear this is a correct change. ### Does this pull request potentially affect one of the following parts: Not a breaking change. ### Documentation - [x] `doc-not-needed` It would be nice to map out the state transitions for our connection classes. That is our of the scope of this small improvement. ### Matching PR in forked repository PR in forked repository: michaeljmarshall#21 (cherry picked from commit c8650ce)
Relates to: #17831 (comment) ### Motivation When the `ProxyConnection` handles a `Connect` command, that is the time to go to `Connecting` state. There is no other time that makes sense to switch to connecting. The current logic will go to connecting in certain re-authentication scenarios, but those are incorrect. By moving the state change to earlier in the logic, we make the state transition clearer and prevent corrupted state. ### Modifications * Remove `state = State.Connecting` from the `doAuthentication` method, which is called multiple times for various reasons * Add `state = State.Connecting` to the start of the `handleConnect` method. ### Verifying this change The existing tests will verify this change, and reading through the code makes it clear this is a correct change. ### Does this pull request potentially affect one of the following parts: Not a breaking change. ### Documentation - [x] `doc-not-needed` It would be nice to map out the state transitions for our connection classes. That is our of the scope of this small improvement. ### Matching PR in forked repository PR in forked repository: michaeljmarshall#21 (cherry picked from commit c8650ce)
Relates to: #17831 (comment) ### Motivation When the `ProxyConnection` handles a `Connect` command, that is the time to go to `Connecting` state. There is no other time that makes sense to switch to connecting. The current logic will go to connecting in certain re-authentication scenarios, but those are incorrect. By moving the state change to earlier in the logic, we make the state transition clearer and prevent corrupted state. ### Modifications * Remove `state = State.Connecting` from the `doAuthentication` method, which is called multiple times for various reasons * Add `state = State.Connecting` to the start of the `handleConnect` method. ### Verifying this change The existing tests will verify this change, and reading through the code makes it clear this is a correct change. ### Does this pull request potentially affect one of the following parts: Not a breaking change. ### Documentation - [x] `doc-not-needed` It would be nice to map out the state transitions for our connection classes. That is our of the scope of this small improvement. ### Matching PR in forked repository PR in forked repository: michaeljmarshall#21 (cherry picked from commit c8650ce)
Relates to: apache#17831 (comment) ### Motivation When the `ProxyConnection` handles a `Connect` command, that is the time to go to `Connecting` state. There is no other time that makes sense to switch to connecting. The current logic will go to connecting in certain re-authentication scenarios, but those are incorrect. By moving the state change to earlier in the logic, we make the state transition clearer and prevent corrupted state. ### Modifications * Remove `state = State.Connecting` from the `doAuthentication` method, which is called multiple times for various reasons * Add `state = State.Connecting` to the start of the `handleConnect` method. ### Verifying this change The existing tests will verify this change, and reading through the code makes it clear this is a correct change. ### Does this pull request potentially affect one of the following parts: Not a breaking change. ### Documentation - [x] `doc-not-needed` It would be nice to map out the state transitions for our connection classes. That is our of the scope of this small improvement. ### Matching PR in forked repository PR in forked repository: michaeljmarshall#21 (cherry picked from commit c8650ce) (cherry picked from commit f9e24f2)
* [fix][proxy] Fix refresh client auth Signed-off-by: Zixuan Liu <[email protected]> * Fix style Signed-off-by: Zixuan Liu <[email protected]> (cherry picked from commit c952f3c) Signed-off-by: Zixuan Liu <[email protected]> (cherry picked from commit 388938e)
Fixes: #10816 PIP: #19771 Supersedes: #19026 Depends on: #20062 ### Motivation The Pulsar Proxy does not properly handle authentication data refresh when in state `ProxyLookupRequests`. The consequence is described in #10816. Essentially, the problem is that the proxy caches stale authentication data and sends it to the broker leading to connection failures. #17831 attempted to fix the underlying problem, but it missed an important edge cases. Specifically, it missed the case that the `ConnectionPool` will have multiple connections when a lookup gets redirected. As such, the following problem exists (and is fixed by this PR): 1. Client opens connection to perform lookups. 2. Proxy connects to broker 1 to get the topic ownership info. 3. Time passes. 4. Client does an additional lookup, and this topic is on a newly created broker 2. In this case, the proxy opens a new connection with the stale client auth data. 5. Broker 2 rejects the connection because it fails with expired authentication. ### Modifications * Remove some of the implementation from #17831. This new implementation still allows a broker to challenge the client through the proxy, but notably, it limits the number of challenges sent to the client. Further, the proxy does not challenge the client when the auth data is not expired. * Introduce authentication refresh in the proxy so that the proxy challenges the client any time the auth data is expired. * Update the `ProxyClientCnx` to get the `clientAuthData` from the `ProxyConnection` to ensure that it gets new authentication data. * Add clock skew to the `AuthenticationProviderToken`. This is necessary to make some of the testing not flaky and it will also be necessary for users to configure in their clusters. ### Verifying this change The `ProxyRefreshAuthTest` covers the existing behavior and I expanded it to cover the edge case described above. Additionally, testing this part of the code will be much easier to test once we implement #19624. ### Documentation - [x] `doc-not-needed` ### Matching PR in forked repository PR in forked repository: the relevant tests pass locally, so I am going to skip the forked tests.
Fixes: apache#10816 PIP: apache#19771 Supersedes: apache#19026 Depends on: apache#20062 The Pulsar Proxy does not properly handle authentication data refresh when in state `ProxyLookupRequests`. The consequence is described in apache#10816. Essentially, the problem is that the proxy caches stale authentication data and sends it to the broker leading to connection failures. apache#17831 attempted to fix the underlying problem, but it missed an important edge cases. Specifically, it missed the case that the `ConnectionPool` will have multiple connections when a lookup gets redirected. As such, the following problem exists (and is fixed by this PR): 1. Client opens connection to perform lookups. 2. Proxy connects to broker 1 to get the topic ownership info. 3. Time passes. 4. Client does an additional lookup, and this topic is on a newly created broker 2. In this case, the proxy opens a new connection with the stale client auth data. 5. Broker 2 rejects the connection because it fails with expired authentication. * Remove some of the implementation from apache#17831. This new implementation still allows a broker to challenge the client through the proxy, but notably, it limits the number of challenges sent to the client. Further, the proxy does not challenge the client when the auth data is not expired. * Introduce authentication refresh in the proxy so that the proxy challenges the client any time the auth data is expired. * Update the `ProxyClientCnx` to get the `clientAuthData` from the `ProxyConnection` to ensure that it gets new authentication data. * Add clock skew to the `AuthenticationProviderToken`. This is necessary to make some of the testing not flaky and it will also be necessary for users to configure in their clusters. The `ProxyRefreshAuthTest` covers the existing behavior and I expanded it to cover the edge case described above. Additionally, testing this part of the code will be much easier to test once we implement apache#19624. - [x] `doc-not-needed` PR in forked repository: the relevant tests pass locally, so I am going to skip the forked tests. (cherry picked from commit 075b625)
Fixes #10816
Motivation
When using the proxy, the Pulsar cannot handle the authentication data on the proxy. The proxy has a client used for lookup operations, which passes the user's and proxy's authentication data to the broker. When the broker found the user's authentication data had expired, the broker sent the refresh request to the proxy, but the proxy could not handle refreshing this authentication data incorrectly.
When an error occurs, Pulsar disconnects all components, which will affect the Pulsar performance.
Fixed version:
Old version:
Solution:
We need to do forwarding based on the proxy. When the proxy's client receives the refresh authentication data request from the broker, the proxy's client sends the same request to the user's client, the user's client responds new authentication to the proxy, and the proxy sends the new authentication to all client.
Documentation
doc-required
(Your PR needs to update docs and you will update later)
doc-not-needed
(Please explain why)
doc
(Your PR contains doc changes)
doc-complete
(Docs have been already added)
Matching PR in forked repository
PR in forked repository: nodece#5