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

Add oauth2 configuration (2/2) #172

Merged
merged 5 commits into from
Nov 9, 2023
Merged

Add oauth2 configuration (2/2) #172

merged 5 commits into from
Nov 9, 2023

Conversation

aprodhomme-dkt
Copy link
Contributor

This PR comes after having reviewed and merged the PR #171

It provides an optional configuration to use with API publishers that use OAUTH2 authentication but with their own form fields.
For example requesting an access token with the following form (eg: https://api.didomi.io/docs/#/sessions/post_sessions):

{
  "type": "api-key",
  "key": "my_client_id",
  "secret": "my_secret"
}

We would have the following configuration:


{
  "name": "http-sink-test",
  "config": {
    "connector.class": "io.aiven.kafka.connect.http.HttpSinkConnector",
    "topics.regex": "domain_a.*",
    "http.url": "http://httpmockserver:1080",
    "http.authorization.type": "oauth2",
    "http.headers.content.type":  "application/json",
    "oauth2.access.token.url":  "http://httpmockserver:1080/oauth2",
    "oauth2.client.authorization.mode": "URL",
    "oauth2.grant_type.key": "type",
    "oauth2.grant_type": " api-key",
    "oauth2.client.id":   "my_client_id",
    "oauth2.client.id.key":  "key",
    "oauth2.client.secret":  "my_secret",
    "oauth2.client.secret.key":  "secret",
    "key.converter": "org.apache.kafka.connect.storage.StringConverter",
    "value.converter": "org.apache.kafka.connect.json.JsonConverter"
  }
}

@aprodhomme-dkt aprodhomme-dkt requested review from a team as code owners June 30, 2023 13:10
@aprodhomme-dkt aprodhomme-dkt changed the title Add oauth2 configuration Add oauth2 configuration (2/2) Jun 30, 2023
Copy link
Contributor

@jeqo jeqo left a comment

Choose a reason for hiding this comment

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

Some comments on the main implementation. Will look into the tests a bit later.

Thanks @aprodhomme-dkt !

build.gradle Outdated Show resolved Hide resolved
docs/sink-connector-config-options.rst Outdated Show resolved Hide resolved
docs/sink-connector-config-options.rst Outdated Show resolved Hide resolved
gradle.properties Outdated Show resolved Hide resolved
docs/sink-connector-config-options.rst Outdated Show resolved Hide resolved
@aprodhomme-dkt aprodhomme-dkt requested a review from jeqo July 13, 2023 13:06
@aprodhomme-dkt aprodhomme-dkt requested a review from jeqo July 25, 2023 14:42
@jeqo
Copy link
Contributor

jeqo commented Aug 30, 2023

@aprodhomme-dkt could you rebase changes from main, so we can review commits only for this change.

ps. forgot to squash commits from #171, so main history got messed up a bit.

@aprodhomme-dkt
Copy link
Contributor Author

@aprodhomme-dkt could you rebase changes from main, so we can review commits only for this change.

ps. forgot to squash commits from #171, so main history got messed up a bit.

done.

@jeqo
Copy link
Contributor

jeqo commented Aug 30, 2023

Commits seem a bit strange still. It should only have the last 5 included in your PR, but there are 21.

@aprodhomme-dkt
Copy link
Contributor Author

Commits seem a bit strange still. It should only have the last 5 included in your PR, but there are 21.

indeed. I'll try to make it clearer. I might have to push force.

@aprodhomme-dkt
Copy link
Contributor Author

I think you're good to go now @jeqo ;)

Copy link
Contributor

@jeqo jeqo left a comment

Choose a reason for hiding this comment

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

@aprodhomme-dkt Looking good! just a few minor comments and validation improvement suggestions.

@aprodhomme-dkt aprodhomme-dkt requested a review from jeqo September 25, 2023 09:34
@jeqo
Copy link
Contributor

jeqo commented Oct 9, 2023

@aprodhomme-dkt sorry for the delay. Hope to look into this later this week. Thanks.

@jeqo
Copy link
Contributor

jeqo commented Oct 13, 2023

@aprodhomme-dkt thanks for looking into the suggestions! I think there's only one missing and it should be good to go.

@aprodhomme-dkt aprodhomme-dkt requested review from a team as code owners November 8, 2023 16:26
Copy link
Contributor

@jeqo jeqo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @aprodhomme-dkt for your contribution!

@jeqo jeqo merged commit 338df52 into Aiven-Open:main Nov 9, 2023
8 checks passed
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.

2 participants