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 sasl authentication #47

Merged
merged 3 commits into from Aug 19, 2021
Merged

support sasl authentication #47

merged 3 commits into from Aug 19, 2021

Conversation

qiqizjl
Copy link
Contributor

@qiqizjl qiqizjl commented Aug 14, 2021

支持了SASL鉴权

目前只实现了PLAIN方式鉴权。可扩展。

SASL目前是一个数组

$sasl = [
    "type"=> longlang\\phpkafka\\Sasl\\PlainSasl::class, // 这个就是确定通过哪个方式鉴权
    ... 剩下的信息是鉴权信息,不同的鉴权的类不一样
]

修改了单元测试。由于kafka的Docker镜像在1.0.0和2.x启动脚本不一样。导致无法环境变量无法兼容。现在kafka的启动脚本是从官方最新的master分支拷贝出来的

@Yurunsoft
Copy link
Member

SASL 应该不需要配置 type,我看了是通过 kafka 通信协议做的 auth

未来支持 ssl 的话,SASL SSL 应该是一样的实现

另外 PlainText、SASL 都应该做测试,而不是把测试都改成 SASL 的

@qiqizjl
Copy link
Contributor Author

qiqizjl commented Aug 14, 2021

SASL 应该不需要配置 type,我看了是通过 kafka 通信协议做的 auth

未来支持 ssl 的话,SASL SSL 应该是一样的实现

另外 PlainText、SASL 都应该做测试,而不是把测试都改成 SASL 的

TYPE的话 是为了区分加密规则。本身KAFKA支持好几种加密规则。所以type的用途是为了区分对应的加密规则

单测稍后补上

@qiqizjl
Copy link
Contributor Author

qiqizjl commented Aug 15, 2021

另外 是否要和官方JavaSDK一样 去引入协议的概念? 如果去适配SSL的话 需要支持关于SSL的配置。官方是基于通讯协议来识别是否需要SSL/SASL。比如说SASL+明文是SASL_PLAINTEXT。如果是SASL+SSL就是SASL_SSL
至于SASL的type其实是对标的Java中的sasl.enabled.mechanisms。只是人家根据枚举去使用对应的类,我只是将这个类放出来了罢了

@Yurunsoft
Copy link
Member

我觉得SSL和SASL应该分开,SSL是通信层面的,SASL是鉴权,本来就应该是毫无关系的,在 java 里放一起可能是历史遗留问题。

另外上面有几个 review 你解决一下,然后就可以合并了

@qiqizjl
Copy link
Contributor Author

qiqizjl commented Aug 17, 2021

TestCase应该已经解决了 还有其他的问题么? 确实没看到其他review的内容

return $this->sasl;
}

public function setSasl(array $sasl): void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setter 方法应该返回 self 类型

@@ -24,14 +24,22 @@ public static function getHost(): string

public static function getPort(): int
{
return (int) (getenv('KAFKA_PORT') ?: 9092);
return (int) (getenv('KAFKA_PORT') ?: 9090);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里最好不要改吧,kafka 默认端口就是 9092

@@ -42,6 +50,8 @@ public static function createKafkaClient(string $class = null): ClientInterface
public static function getControllerClient(): ClientInterface
{
$client = self::createKafkaClient();
//$client->getConfig()->setSa
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

注释需要去掉

@Yurunsoft
Copy link
Member

忘了提交了,现在应该能看到吧

@qiqizjl
Copy link
Contributor Author

qiqizjl commented Aug 18, 2021

已修改 文档也已经补充了

@Yurunsoft Yurunsoft merged commit 140fbb5 into swoole:master Aug 19, 2021
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.

None yet

2 participants