Skip to content
This repository has been archived by the owner on Dec 14, 2022. It is now read-only.

[ENHANCEMENT] enhancement for setting up auth parameters #380

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wuzhanpeng
Copy link
Contributor

Motivations

When I tried to create FlinkPulsarSource in the following way,

props.setProperty(PulsarOptions.AUTH_PLUGIN_CLASSNAME_KEY, "org.apache.pulsar.client.impl.auth.AuthenticationToken");
props.setProperty(PulsarOptions.AUTH_PARAMS_KEY, "token:abcdefghijklmn");

FlinkPulsarSource(adminUrl, clientConf, pulsarDeserializationSchema, props);

I found my flink job was failed and I can see some warnning logs in my broker like:

Failed to authenticate HTTP request: Authentication required

The reason for this problem is that only setting auth params in Properties will not take effect in the final initialization of PulsarClient.

Modifications

Add some extra checking for this issue.

@wuzhanpeng wuzhanpeng requested review from jianyun8023, syhily and a team as code owners July 31, 2021 04:27
@@ -53,4 +53,19 @@ public static ClientConfigurationData newClientConf(String serviceUrl, Propertie
return clientConf;
}

public static void setupAuthIfNeed(ClientConfigurationData conf, Properties properties) {
if (!StringUtils.isBlank(conf.getAuthPluginClassName()) && !StringUtils.isBlank(conf.getAuthParams())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic may not looks like right. We support both authParams and authMap. So you may need to check the authMap here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I add a check for authMap and PTAL~ @syhily

@syhily syhily added compute/data-processing type/enhancement Indicates an improvement to an existing feature labels Nov 4, 2021
@syhily syhily self-assigned this Nov 4, 2021
@syhily
Copy link
Contributor

syhily commented Nov 4, 2021

OK, LGTM. But this is only a hotfix we may perform a code cleanup and refactor these complex logic.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compute/data-processing type/enhancement Indicates an improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants