apache / pulsar-client-python

Apache Pulsar Python client library
https://pulsar.apache.org/
Apache License 2.0
51 stars 40 forks source link

Support the base64 encoded credentials for OAuth2 authentication #106

Closed BewareMyPower closed 1 year ago

BewareMyPower commented 1 year ago

Fixes https://github.com/apache/pulsar-client-python/issues/101

Motivation

Currently the private_key field of the JSON passed to AuthenticationOauth2 only represents the path to the file, we need to support passing the base64 encoded JSON string.

Modifications

Since the C++ client supports reading the client_id and client_secret fields from the JSON, a _convert method, which converts the private_key field to the client_id and client_secret fields, is added to AuthenticationOauth2. Then, users can specify the base64 encoded value in the private_key field.

Since the current workflow already uses a tokenSecretKey config for JWT authentication and this secret key cannot be used to verify the OAuth2 access token, a docker-compose YAML file is added to set up another Pulsar standalone with OAuth2 authentication configured. oauth2_test.py is added to run against this separated cluster.

BewareMyPower commented 1 year ago

I'm curious why don't we make these changes to the c++ client?

Of course we can. But with this PR, we can have this feature without releasing a new version of the C++ client. If there is something wrong with the implementation of the C++ client, we have to raise a new release.

RobertIndie commented 1 year ago

From my understanding, it's a missing feature in the C++ client. Right? If so, I suggest we implement it in the C++ client even though we need to cut a new release.

BewareMyPower commented 1 year ago

If so, I suggest we implement it in the C++ client even though we need to cut a new release.

We can implement it in the C++ client. But we don't have to reuse the C++ implementation from the Python side because:

  1. It's not a critical path
  2. Implement it in the Python side is simple and intuitive.
  3. It's more friendly to Python developers because they don't need to look into the C++ code if they found something wrong. See https://github.com/apache/pulsar-client-python/issues/85#issuecomment-1418551427

And as I've said before:

If there is something wrong with the implementation of the C++ client, we have to raise a new release.

The Python client should only depends on the C++ client for basic Pulsar protocol implementations. Otherwise, many bug fixes would require the fix at the C++ side.

For example, ideally, the OAuth2 authentication provider could be implemented fully at the Python side. It can also avoid the issue like we have met before.

shibd commented 1 year ago

This PR of change is similar to the conversion of parameters, which is lightweight. So I think it can be implemented on the python side (it will be handed over to the user faster than the implementation on the cpp side)

If the CPP client has the same problem, we should create the corresponding issue in the CPP repo. After CPP support it, maybe we can remove it on the Python side as well.

BewareMyPower commented 1 year ago

@merlimat Could you share your thoughts? I'd like to hear more opinions about whether should we implement it first in Python client. Or whether should we have multiple implementations for different languages.

BewareMyPower commented 1 year ago

I will implement this feature in C++ client first. Mark it as drafted

shibd commented 1 year ago

Fix by apache/pulsar-client-cpp#249