[GitHub] rdhabalia commented on a change in pull request #1707: Fixed authentication flow via Pulsar Proxy
rdhabalia commented on a change in pull request #1707: Fixed authentication
flow via Pulsar Proxy
URL: https://github.com/apache/incubator-pulsar/pull/1707#discussion_r185667124
##
File path:
pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java
##
@@ -259,13 +295,34 @@ private boolean
verifyAuthenticationIfNeeded(CommandConnect connect) {
clientAuthRole =
service.getAuthenticationService().authenticate(authenticationData, authMethod);
LOG.info("[{}] Client successfully authenticated with {} role {}",
remoteAddress, authMethod,
clientAuthRole);
+if (service.getConfiguration().forwardAuthorizationCredentials()) {
+this.clientAuthData = authData;
+this.clientAuthMethod = authMethod;
+}
+createClient(clientConf, this.clientAuthData,
this.clientAuthMethod);
Review comment:
as name suggests. can we create and return client in `createClient()`:
`this.client = createClient(..)`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rdhabalia commented on a change in pull request #1707: Fixed authentication flow via Pulsar Proxy
rdhabalia commented on a change in pull request #1707: Fixed authentication
flow via Pulsar Proxy
URL: https://github.com/apache/incubator-pulsar/pull/1707#discussion_r185666025
##
File path:
pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyClientCnx.java
##
@@ -0,0 +1,63 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.proxy.server;
+
+import static org.apache.pulsar.client.impl.HttpClient.getPulsarClientVersion;
+
+import org.apache.pulsar.client.api.PulsarClientException;
+import org.apache.pulsar.client.impl.ClientCnx;
+import org.apache.pulsar.client.impl.conf.ClientConfigurationData;
+import org.apache.pulsar.common.api.Commands;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.channel.EventLoopGroup;
+
+public class ProxyClientCnx extends ClientCnx {
+
+ String clientAuthRole;
+ String clientAuthData;
+ String clientAuthMethod;
+
+ public ProxyClientCnx(ClientConfigurationData conf, EventLoopGroup
eventLoopGroup, String clientAuthRole,
+ String clientAuthData, String clientAuthMethod) {
+ super(conf, eventLoopGroup);
+ this.clientAuthRole = clientAuthRole;
+ this.clientAuthData = clientAuthData;
+ this.clientAuthMethod = clientAuthMethod;
+ }
+
+ @Override
+ protected ByteBuf newConnectCommand() throws PulsarClientException {
+ if (log.isDebugEnabled()) {
+log.info(
+"New Connection opened via ProxyClientCnx with params
clientAuthRole = {}, clientAuthData = {}, clientAuthMethod = {}",
+clientAuthRole, clientAuthData, clientAuthMethod);
+ }
+ String authData = "";
Review comment:
can we initialize with null?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rdhabalia commented on a change in pull request #1707: Fixed authentication flow via Pulsar Proxy
rdhabalia commented on a change in pull request #1707: Fixed authentication flow via Pulsar Proxy URL: https://github.com/apache/incubator-pulsar/pull/1707#discussion_r185668132 ## File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyService.java ## @@ -65,15 +60,10 @@ private ZooKeeperClientFactory zkClientFactory = null; private final EventLoopGroup acceptorGroup; -private final EventLoopGroup workerGroup; +final EventLoopGroup workerGroup; Review comment: can we keep it private? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rdhabalia commented on a change in pull request #1707: Fixed authentication flow via Pulsar Proxy
rdhabalia commented on a change in pull request #1707: Fixed authentication
flow via Pulsar Proxy
URL: https://github.com/apache/incubator-pulsar/pull/1707#discussion_r185666469
##
File path:
pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java
##
@@ -229,14 +245,39 @@ protected void handleLookup(CommandLookupTopic lookup) {
private void close() {
state = State.Closed;
ctx.close();
+try {
+client.close();
+} catch (PulsarClientException e) {
+LOG.error("Unable to clode pulsar client - {}", client);
Review comment:
type `close ` and add `e.getMessage()` in log
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rdhabalia commented on a change in pull request #1707: Fixed authentication flow via Pulsar Proxy
rdhabalia commented on a change in pull request #1707: Fixed authentication
flow via Pulsar Proxy
URL: https://github.com/apache/incubator-pulsar/pull/1707#discussion_r185667623
##
File path:
pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java
##
@@ -259,13 +295,34 @@ private boolean
verifyAuthenticationIfNeeded(CommandConnect connect) {
clientAuthRole =
service.getAuthenticationService().authenticate(authenticationData, authMethod);
LOG.info("[{}] Client successfully authenticated with {} role {}",
remoteAddress, authMethod,
clientAuthRole);
+if (service.getConfiguration().forwardAuthorizationCredentials()) {
+this.clientAuthData = authData;
+this.clientAuthMethod = authMethod;
+}
+createClient(clientConf, this.clientAuthData,
this.clientAuthMethod);
Review comment:
also we don't want to share proxy-to-broker connection when
`service.getConfiguration().forwardAuthorizationCredentials()` is enabled.
else, we can always share proxy-to-broker connection for lookup.
so, should we create a new client only in case of
`service.getConfiguration().forwardAuthorizationCredentials()=true` else we can
always share it.?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rdhabalia commented on a change in pull request #1707: Fixed authentication flow via Pulsar Proxy
rdhabalia commented on a change in pull request #1707: Fixed authentication
flow via Pulsar Proxy
URL: https://github.com/apache/incubator-pulsar/pull/1707#discussion_r185668092
##
File path:
pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyAuthenticationTest.java
##
@@ -0,0 +1,269 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.proxy.server;
+
+import static org.mockito.Mockito.spy;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.TimeUnit;
+import java.util.Set;
+
+import javax.naming.AuthenticationException;
+
+import org.apache.bookkeeper.test.PortManager;
+import org.apache.pulsar.broker.ServiceConfiguration;
+import org.apache.pulsar.broker.authentication.AuthenticationDataSource;
+import org.apache.pulsar.broker.authentication.AuthenticationProvider;
+import org.apache.pulsar.client.admin.PulsarAdmin;
+import org.apache.pulsar.client.api.Authentication;
+import org.apache.pulsar.client.api.AuthenticationDataProvider;
+import org.apache.pulsar.client.api.Consumer;
+import org.apache.pulsar.client.api.Message;
+import org.apache.pulsar.client.api.MessageId;
+import org.apache.pulsar.client.api.Producer;
+import org.apache.pulsar.client.api.ProducerConsumerBase;
+import org.apache.pulsar.client.api.PulsarClient;
+import org.apache.pulsar.client.api.PulsarClientException;
+import org.apache.pulsar.client.impl.ConsumerImpl;
+import org.apache.pulsar.common.policies.data.AuthAction;
+import org.apache.pulsar.common.util.FutureUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testng.Assert;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import com.google.common.collect.Sets;
+import com.google.gson.JsonElement;
+import com.google.gson.JsonObject;
+import com.google.gson.JsonParser;
+
+public class ProxyAuthenticationTest extends ProducerConsumerBase {
+ private static final Logger log =
LoggerFactory.getLogger(ProxyAuthenticationTest.class);
+
+ public static class BasicAuthenticationData implements
AuthenticationDataProvider {
+ private String authParam;
+
+ public BasicAuthenticationData(String authParam) {
+ this.authParam = authParam;
+ }
+
+ public boolean hasDataFromCommand() {
+ return true;
+ }
+
+ public String getCommandData() {
+ return authParam;
+ }
+
+ public boolean hasDataForHttp() {
+ return true;
+ }
+
+ @Override
+ public Set> getHttpHeaders() {
+ Map headers = new HashMap<>();
+ headers.put("BasicAuthentication", authParam);
+ return headers.entrySet();
+ }
+ }
+
+ public static class BasicAuthentication implements Authentication {
+
+ private String authParam;
+
+ @Override
+ public void close() throws IOException {
+ // noop
+ }
+
+ @Override
+ public String getAuthMethodName() {
+ return "BasicAuthentication";
+ }
+
+ @Override
+ public AuthenticationDataProvider getAuthData() throws
PulsarClientException {
+ try {
+ return new BasicAuthenticationData(authParam);
+ } catch (Exception e) {
+ throw new PulsarClientException(e);
+ }
+ }
+
+ @Override
+ public void configure(Map authParams) {
+ this.authParam = String.format("{\"entityType\":
\"%s\", \"expiryTime\": \"%s\"}",
+
[GitHub] rdhabalia commented on a change in pull request #1707: Fixed authentication flow via Pulsar Proxy
rdhabalia commented on a change in pull request #1707: Fixed authentication
flow via Pulsar Proxy
URL: https://github.com/apache/incubator-pulsar/pull/1707#discussion_r185668018
##
File path:
pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyAuthenticationTest.java
##
@@ -0,0 +1,269 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.proxy.server;
+
+import static org.mockito.Mockito.spy;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.TimeUnit;
+import java.util.Set;
+
+import javax.naming.AuthenticationException;
+
+import org.apache.bookkeeper.test.PortManager;
+import org.apache.pulsar.broker.ServiceConfiguration;
+import org.apache.pulsar.broker.authentication.AuthenticationDataSource;
+import org.apache.pulsar.broker.authentication.AuthenticationProvider;
+import org.apache.pulsar.client.admin.PulsarAdmin;
+import org.apache.pulsar.client.api.Authentication;
+import org.apache.pulsar.client.api.AuthenticationDataProvider;
+import org.apache.pulsar.client.api.Consumer;
+import org.apache.pulsar.client.api.Message;
+import org.apache.pulsar.client.api.MessageId;
+import org.apache.pulsar.client.api.Producer;
+import org.apache.pulsar.client.api.ProducerConsumerBase;
+import org.apache.pulsar.client.api.PulsarClient;
+import org.apache.pulsar.client.api.PulsarClientException;
+import org.apache.pulsar.client.impl.ConsumerImpl;
+import org.apache.pulsar.common.policies.data.AuthAction;
+import org.apache.pulsar.common.util.FutureUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testng.Assert;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import com.google.common.collect.Sets;
+import com.google.gson.JsonElement;
+import com.google.gson.JsonObject;
+import com.google.gson.JsonParser;
+
+public class ProxyAuthenticationTest extends ProducerConsumerBase {
+ private static final Logger log =
LoggerFactory.getLogger(ProxyAuthenticationTest.class);
+
+ public static class BasicAuthenticationData implements
AuthenticationDataProvider {
+ private String authParam;
+
+ public BasicAuthenticationData(String authParam) {
+ this.authParam = authParam;
+ }
+
+ public boolean hasDataFromCommand() {
+ return true;
+ }
+
+ public String getCommandData() {
+ return authParam;
+ }
+
+ public boolean hasDataForHttp() {
+ return true;
+ }
+
+ @Override
+ public Set> getHttpHeaders() {
+ Map headers = new HashMap<>();
+ headers.put("BasicAuthentication", authParam);
+ return headers.entrySet();
+ }
+ }
+
+ public static class BasicAuthentication implements Authentication {
+
+ private String authParam;
+
+ @Override
+ public void close() throws IOException {
+ // noop
+ }
+
+ @Override
+ public String getAuthMethodName() {
+ return "BasicAuthentication";
+ }
+
+ @Override
+ public AuthenticationDataProvider getAuthData() throws
PulsarClientException {
+ try {
+ return new BasicAuthenticationData(authParam);
+ } catch (Exception e) {
+ throw new PulsarClientException(e);
+ }
+ }
+
+ @Override
+ public void configure(Map authParams) {
+ this.authParam = String.format("{\"entityType\":
\"%s\", \"expiryTime\": \"%s\"}",
+
[GitHub] rdhabalia commented on a change in pull request #1707: Fixed authentication flow via Pulsar Proxy
rdhabalia commented on a change in pull request #1707: Fixed authentication
flow via Pulsar Proxy
URL: https://github.com/apache/incubator-pulsar/pull/1707#discussion_r185665960
##
File path:
pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyClientCnx.java
##
@@ -0,0 +1,63 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.proxy.server;
+
+import static org.apache.pulsar.client.impl.HttpClient.getPulsarClientVersion;
+
+import org.apache.pulsar.client.api.PulsarClientException;
+import org.apache.pulsar.client.impl.ClientCnx;
+import org.apache.pulsar.client.impl.conf.ClientConfigurationData;
+import org.apache.pulsar.common.api.Commands;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.channel.EventLoopGroup;
+
+public class ProxyClientCnx extends ClientCnx {
+
+ String clientAuthRole;
+ String clientAuthData;
+ String clientAuthMethod;
+
+ public ProxyClientCnx(ClientConfigurationData conf, EventLoopGroup
eventLoopGroup, String clientAuthRole,
+ String clientAuthData, String clientAuthMethod) {
+ super(conf, eventLoopGroup);
+ this.clientAuthRole = clientAuthRole;
+ this.clientAuthData = clientAuthData;
+ this.clientAuthMethod = clientAuthMethod;
+ }
+
+ @Override
+ protected ByteBuf newConnectCommand() throws PulsarClientException {
+ if (log.isDebugEnabled()) {
+log.info(
Review comment:
`log.debug(..)`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
