[Issue 10816][Proxy] Refresh client auth token by kkoderok · Pull Request #13339 · apache/pulsar

kkoderok · 2021-12-15T13:06:11Z

Fixes #10816

Motivation

See #10816

Modifications

Refresh client token on proxy (to lookup) in case when refresh token command have been received from broker.

Verifying this change

Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

org.apache.pulsar.proxy.server.ProxyWithJwtAuthorizationTest#testRefreshClientToken

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

no-need-doc
Bug fix

hezhangjian · 2021-12-15T14:17:07Z

@KKKoder I think you do too much checkstyle format in this PR. I would suggest to split them if needed

hezhangjian · 2021-12-15T14:46:49Z

@KKKoder let me open a PR for checkstyle fix

kkoderok · 2021-12-15T18:43:51Z

@KKKoder let me open a PR for checkstyle fix

Ok, do it.

hezhangjian · 2021-12-16T00:42:16Z

@KKKoder I have done in #13343, could you please pull master code, and undo the format, thanks :)

github-actions · 2022-02-26T01:50:06Z

The pr had no activity for 30 days, mark with Stale label.

hsaputra · 2022-03-29T02:03:21Z

+            .forEach(future -> future.complete(clientData));
+    }
+
+    private CompletableFuture<AuthData> getOrRefreshClientAuthData() {


Could you walk me through what this method suppose to do? Thanks

When client auth data expired and proxy configured with forward authentication data, then this method requests a new token. This method is thread safe, because one ProxyConnection has multiple ProxyClientCnx, and potentially at same time, they request
auth data.

Jason918 · 2022-03-29T02:28:54Z

        AuthData authData = authenticationDataProvider.authenticate(AuthData.INIT_AUTH_DATA);
-        return Commands.newConnect(authentication.getAuthMethodName(), authData, this.protocolVersion,
-                PulsarVersion.getVersion(), proxyToTargetBrokerAddress, null, null, null);
+        return CompletableFuture.completedFuture(


What's the point of using CompletableFuture here ? It seems there is no async operations.

This method is overridden in the ProxyClientCnx class, which can use the asynchronous operation of obtaining authorization data from the client, so this method returns a CompletableFuture

I've done some changes, check them please.

eolivelli · 2022-03-29T08:09:31Z

        AuthData authData = authenticationDataProvider.authenticate(AuthData.INIT_AUTH_DATA);
-        return Commands.newConnect(authentication.getAuthMethodName(), authData, this.protocolVersion,
-                PulsarVersion.getVersion(), proxyToTargetBrokerAddress, null, null, null);
+        return CompletableFuture.completedFuture(


eolivelli · 2022-03-29T08:10:47Z

+        try {
+            clientAuthDataSupplier.get()
+                .thenAccept(authData -> sendAuthResponse(authData, clientAuthMethod));
+        } catch (Exception e) {


how is it possible to have an Exception here ?
in case of failure we should deal with the error and send a response or close the connection

Agree. I will fix.

I've already fixed it, make a review please.

sursingh · 2022-03-31T16:19:16Z

+    @Test
+    void testRefreshClientToken() throws Exception {
+        log.info("-- Starting {} test --", methodName);
+
+        startProxy();
+        createAdminClient();
+
+        @SuppressWarnings("unchecked")
+        Supplier<String> tokenSupplier = Mockito.mock(Supplier.class);
+        when(tokenSupplier.get()).thenAnswer(answer -> createClientJwtToken(Duration.ofSeconds(1)));
+
+        PulsarClient proxyClient = PulsarClient.builder()
+                .serviceUrl(proxyService.getServiceUrl()).statsInterval(0, TimeUnit.SECONDS)
+                .authentication(AuthenticationFactory.token(tokenSupplier))
+                .operationTimeout(1000, TimeUnit.MILLISECONDS)
+                .build();
+
+        String namespaceName = "my-property/proxy-authorization/my-ns";
+        admin.clusters().createCluster("proxy-authorization", ClusterData.builder().serviceUrl(brokerUrl.toString()).build());
+        admin.tenants().createTenant("my-property",
+                new TenantInfoImpl(Sets.newHashSet("appid1", "appid2"), Sets.newHashSet("proxy-authorization")));
+        admin.namespaces().createNamespace(namespaceName);
+
+        admin.namespaces().grantPermissionOnNamespace(namespaceName, CLIENT_ROLE,
+                Sets.newHashSet(AuthAction.consume, AuthAction.produce));
+        log.info("-- Admin permissions {} ---", admin.namespaces().getPermissions(namespaceName));
+
+        Producer<byte[]> producer = proxyClient.newProducer(Schema.BYTES)
+                .topic("persistent://my-property/proxy-authorization/my-ns/my-topic1").create();
+
+        final int msgs = 10;
+        for (int i = 0; i < msgs; i++) {
+            String message = "my-message-" + i;
+            producer.send(message.getBytes());
+        }
+
+        //noinspection unchecked
+        clearInvocations(tokenSupplier);
+        Thread.sleep(3000);
+        verify(tokenSupplier, atLeastOnce()).get();


I don't think this test is correct. It is passing for us, even without the associated fix.

Ok. thanks. I'll check and fix the test

I've made another test, check please.

github-actions · 2022-05-30T02:15:16Z

The pr had no activity for 30 days, mark with Stale label.

nodece · 2022-08-11T09:17:43Z

Hi @kkoderok, thanks for your contribution! Can you still work on this PR? If you don't have time, I will work on this PR.

nodece · 2022-09-29T17:38:23Z

Closed by #17831.

github-actions Bot assigned kkoderok Dec 15, 2021

github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Dec 15, 2021

kkoderok force-pushed the fix-10816 branch from e4fcb1f to a67028d Compare December 15, 2021 18:46

kkoderok force-pushed the fix-10816 branch from a67028d to 5d133af Compare December 16, 2021 06:03

hezhangjian reviewed Dec 18, 2021

View reviewed changes

Comment thread pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyWithJwtAuthorizationTest.java Outdated

kkoderok force-pushed the fix-10816 branch from 5d133af to d71f941 Compare December 20, 2021 06:36

kkoderok force-pushed the fix-10816 branch from d71f941 to 2004716 Compare December 29, 2021 13:57

lhotari requested review from eolivelli and michaeljmarshall January 13, 2022 19:15

github-actions Bot added the lifecycle/stale label Feb 26, 2022

hsaputra reviewed Mar 29, 2022

View reviewed changes

Jason918 reviewed Mar 29, 2022

View reviewed changes

eolivelli requested changes Mar 29, 2022

View reviewed changes

sursingh reviewed Mar 31, 2022

View reviewed changes

kkoderok force-pushed the fix-10816 branch from 2004716 to 3934569 Compare April 4, 2022 15:21

[Issue 10816][Proxy] Refresh client auth token

486f891

kkoderok force-pushed the fix-10816 branch from 3934569 to 486f891 Compare April 4, 2022 15:24

github-actions Bot added the Stale label May 30, 2022

nodece self-requested a review August 11, 2022 09:14

nodece self-assigned this Aug 11, 2022

nodece closed this Sep 29, 2022

Uh oh!

Conversation

kkoderok commented Dec 15, 2021

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Uh oh!

hezhangjian commented Dec 15, 2021

Uh oh!

hezhangjian commented Dec 15, 2021

Uh oh!

kkoderok commented Dec 15, 2021

Uh oh!

hezhangjian commented Dec 16, 2021

Uh oh!

Uh oh!

github-actions Bot commented Feb 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkoderok Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 30, 2022

Uh oh!

nodece commented Aug 11, 2022

Uh oh!

nodece commented Sep 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kkoderok Apr 4, 2022 •

edited

Loading