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

Correct the SchemaRegistry authentication for SASL_INHERIT #733

Merged
merged 7 commits into from
Mar 17, 2020
Merged

Correct the SchemaRegistry authentication for SASL_INHERIT #733

merged 7 commits into from
Mar 17, 2020

Conversation

abij
Copy link
Contributor

@abij abij commented Dec 4, 2019

We ran into an issue connecting the SchemaRegistry inside the Producer.

I looked into the problem, it turns out the Server(broker) can use multiple sasl.mechanisms (plural) and the clients (producer / consumer) sasl.mechanism (singular). There was an issue passing through this config, when the setting schema.registry.basic.auth.credentials.source is set to SASL_INHERIT.

I think there is no point in inheriting the credentials, since the SchemaRegistry has separate key/secret, but maybe there is a way to use the same credentials for both (SR + Cluster).

This PR will fix the SASL_INHERIT in SchemaRegistry.

I did add an example for confluent_cloud.py, but looks like the avro-cli.py has a working solution already, so I reverted this commit.

@ghost
Copy link

ghost commented Dec 4, 2019

It looks like @abij hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to prove it.

Appreciation of efforts,

clabot

@abij
Copy link
Contributor Author

abij commented Dec 4, 2019

[clabot:check]

@ghost
Copy link

ghost commented Dec 4, 2019

@confluentinc It looks like @abij just signed our Contributor License Agreement. 👍

Always at your service,

clabot

Copy link
Contributor

@rnpridgeon rnpridgeon left a comment

Choose a reason for hiding this comment

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

Good catch, that is annoying indeed.

We actually need to support both for a while as to not break backward compatibility. Let's add a deprecation warning forsasl.mechanisms while checking for both variations of the property.

confluent_kafka/avro/__init__.py Outdated Show resolved Hide resolved
confluent_kafka/avro/__init__.py Outdated Show resolved Hide resolved
@@ -135,8 +135,8 @@ def _configure_basic_auth(conf):
raise ValueError("schema.registry.basic.auth.credentials.source must be one of {}"
.format(VALID_AUTH_PROVIDERS))
if auth_provider == 'SASL_INHERIT':
if conf.pop('sasl.mechanism', '').upper() is ['GSSAPI']:
raise ValueError("SASL_INHERIT does not support SASL mechanisms GSSAPI")
if conf.pop('sasl.mechanism', '').upper() == 'GSSAPI':
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

@abij
Copy link
Contributor Author

abij commented Dec 17, 2019

I did add the plural fallback for mechanism(s), if you have a suggestion for the deprecation warning I could include this in the PR as-well.

@rnpridgeon
Copy link
Contributor

In retrospect deprecation since they are one and the same for the underlying producer/consumer clients.

Can you add a quick test to tests/avro/test_cached_client.py though. Use the singular variant with GSSAPI so we have slightly better coverage.

@andharris
Copy link

@abij are you planning on still pursuing getting this PR merged? I recently hit this issue and would like to help however possible. thanks!

@abij
Copy link
Contributor Author

abij commented Mar 16, 2020

Added a testcase to cover these lines as suggested by @rnpridgeon.

@edenhill edenhill merged commit 6b6fc68 into confluentinc:master Mar 17, 2020
@edenhill
Copy link
Contributor

Thank you for your contribution!

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.

4 participants