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

Support manually in-place authentication token update #65

Merged
merged 8 commits into from
May 29, 2024
Merged

Support manually in-place authentication token update #65

merged 8 commits into from
May 29, 2024

Conversation

EAimTY
Copy link
Contributor

@EAimTY EAimTY commented Sep 26, 2023

As mentioned in #54, the lack of authentication token update mechenism may cause active lease and watch become unusable after the token expire. The solution gave in #45 (comment) does theoretically solved it. However, recreating the client means every LeaseKeeper & Watcher needs to be rebuild. This makes the implementation quite complex.

Since updating token automatically is somehow difficult due to tonic's design (#45), I think updating token manually may be a better solution.

The field token in AuthService is changed to an Arc<Mutex<Option<HeaderValue>>> to support in-place modification and sharing between multiple services. The cost of the Mutex is acceptable since it only involves a clone of a small HeaderValue in every request.

The token update method is temporarily Client::update_auth. I am not sure where to put it since it may not be correspond to any service abstraction.

@EAimTY
Copy link
Contributor Author

EAimTY commented Oct 3, 2023

I think there are 3 ways to add the reauthentication API:

  1. Add a dedicated struct AuthHandle as a field of Client. It should be able to be got by a method of Client, and be cloned like other fields. We can have methods like update_token and remove_token on it.
  2. Add update_token and remove_token to AuthClient. However, this feels weird since XxxClient should only contains certain methods of its rpc service.
  3. Keep the current design (and add remove_token) on Client. But since we need &mut Client everytime updating token, this may not be as flexible as the first solution.

IMO the first one is better since it could makes the reauthenticate easier without affecting other running tasks.

@davidli2010
Copy link
Contributor

davidli2010 commented Nov 19, 2023

I think there are 3 ways to add the reauthentication API:

1. Add a dedicated struct `AuthHandle` as a field of `Client`. It should be able to be got by a method of `Client`, and be cloned like other fields. We can have methods like `update_token` and `remove_token` on it.

2. Add `update_token` and `remove_token` to `AuthClient`. However, this feels weird since `XxxClient` should only contains certain methods of its rpc service.

3. Keep the current design (and add `remove_token`) on `Client`. But since we need `&mut Client` everytime updating token, this may not be as flexible as the first solution.

IMO the first one is better since it could makes the reauthenticate easier without affecting other running tasks.

I preferred the second. Add to AuthClient:

etcd_client::auth_client().update_token(name, password);

src/auth.rs Outdated Show resolved Hide resolved
@EAimTY
Copy link
Contributor Author

EAimTY commented Mar 30, 2024

I preferred the second. Add to AuthClient:

etcd_client::auth_client().update_token(name, password);

The methods in AuthClient correspond one-to-one with the service Auth in protobuf, and all functions involve changes to the cluster configuration.

Therefore, I prefer the first approach, where the method for updating the client token is placed in a separate handler. This ensures the correspondence between XxxClient and the corresponding RPC service

@EAimTY EAimTY requested a review from davidli2010 March 30, 2024 09:03
@davidli2010
Copy link
Contributor

The methods in AuthClient correspond one-to-one with the service Auth in protobuf, and all functions involve changes to the cluster configuration.

There are no strict restrictions that require AuthClient and rpc to have a one-to-one correspondence, AuthClient is responsible for functions related to authentication, and it feels a little strange to have both AuthClient and AuthHandle.

@EAimTY
Copy link
Contributor Author

EAimTY commented Apr 10, 2024

AuthClient is responsible for functions related to authentication

A more accurate description, in my opinion, would be that AuthClient is responsible for modifying and querying authentication information of the cluster. As for the token setting on the client side, I don't think it's appropriate to include them here

@davidli2010
Copy link
Contributor

AuthClient is responsible for functions related to authentication

A more accurate description, in my opinion, would be that AuthClient is responsible for modifying and querying authentication information of the cluster.

I don't think it's necessary to take the AuthClient's responsibilities too seriously. I don't want to restrict AuthClient to server-side settings and create separate objects for both client-side and server-side.

@EAimTY
Copy link
Contributor Author

EAimTY commented May 25, 2024

I have no idea why clippy fails 😅

@davidli2010
Copy link
Contributor

davidli2010 commented May 25, 2024

I have no idea why clippy fails 😅

Fixed in #80

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants