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

Fix support for basic auth with the schema registry #94

Merged
merged 11 commits into from
Jul 6, 2021

Conversation

robbavey
Copy link
Contributor

@robbavey robbavey commented Jun 23, 2021

This fixes an issue with supporting basic auth when using the schema registry

The plugin currently expects schema_registry_secret and schema_registry_key to
be set to authenticate with the schema registry. However, while this works during the
'register' phase, the authentication type basic.auth.credentials.source is not set
when creating the kafka consumer, leading to authentication failures when attempting
to deserialize the kafka payload.

This commit correctly sets the basic.auth.credentials.source to USER_INFO when
schema_registry_key and schema_registry_secret are set, and also enables the use
of a basic auth embedded in the schema_registry_url via username:password@url

This fixes an issue with supporting basic auth when using the schema registry

The plugin currently expects 'schema_registry_secret` and `schema_registry_key` to
be set to authenticate with the schema registry. However, while this works during the
'register' phase, the authentication type `basic.auth.credentials.source` is not set
when creating the kafka consumer, leading to authentication failures when attempting
to deserialize the kafka payload.

This commit correctly sets the `basic.auth.credentials.source` to `USER_INFO` when
`schema_registry_key` and `schema_registry_secret` are set, and also enables the use
of a basic auth embedded in the `schema_registry_url` via `username:password@url`
@robbavey
Copy link
Contributor Author

This fixes #95

@robbavey robbavey requested a review from andsel June 24, 2021 16:44
@robbavey robbavey force-pushed the basic_auth_schema branch 6 times, most recently from 0bc06b1 to 873b2dc Compare June 24, 2021 20:18
Copy link
Contributor

@andsel andsel left a comment

Choose a reason for hiding this comment

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

Very nice catch @robbavey.
I've left on a note on a bad backtick in CHANGELOG, plus a suggestion for rename the method responsible to shutdown the Schema Registry from test suites.

LGTM

CHANGELOG.md Outdated
@@ -1,3 +1,7 @@
## 10.7.7
- Fix: Correct the settings to allow basic auth to work properly, either by setting 'schema_registry_key/secret` or embedding username/password in the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Fix: Correct the settings to allow basic auth to work properly, either by setting 'schema_registry_key/secret` or embedding username/password in the
- Fix: Correct the settings to allow basic auth to work properly, either by setting `schema_registry_key/secret` or embedding username/password in the

@@ -292,37 +317,58 @@ def delete_remote_schema(schema_registry_client, subject_name)
expect( subjects ).to be_empty
end
end
end

def cleanup_schema_registry
Copy link
Contributor

Choose a reason for hiding this comment

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

cleanup_schema_registry reminds me something that's cleaned on the Schema Registry service instead of a "shutdown Schema Registry" action

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

3 participants