KAFKA-8404: Add HttpHeader to RestClient HTTP Request and Connector REST API by haidangdam · Pull Request #6791 · apache/kafka

haidangdam · 2019-05-23T00:22:57Z

Description:
The ticket: https://issues.apache.org/jira/browse/KAFKA-8404
Add HttpHeader to Connector REST API.
Adding Authorization header to the Connector's Rest Client HTTP Request
Backporting to Kafka version 2.0

Testing:
Logic Test with Mocking.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

haidangdam · 2019-05-23T00:24:58Z

@rayokota @C0urante please review my PR. Thanks!

haidangdam · 2019-05-23T17:27:49Z

retest me

rayokota

@haidangdam , this looks good! Left a few minor comments.

haidangdam · 2019-05-23T18:30:15Z

@haidangdam , this looks good! Left a few minor comments.

@rayokota Hi. Thanks for the input. I have already pushed the changed version.

rayokota

@haidangdam , left a few more nits. Thanks!

rayokota · 2019-05-23T21:30:22Z

nit: add param description

rayokota · 2019-05-23T21:30:33Z

nit: add param description

haidangdam · 2019-05-23T21:48:56Z

@rhauch Hi, would you mind taking a look at this PR. Thanks

rhauch

Thanks for the fix, @haidangdam! Just a few small comments/suggestions, but otherwise looks good.

rhauch · 2019-05-23T22:45:53Z

Maybe instead of null we could use NO_HEADERS instead?

I'm not sure about this. Would you like to have HttpHeaders nullHeaders = null;

I would like to see:

connectorsResource.createConnector(FORWARD, NULL_HEADERS, bodyIn);

to make it much more clear what the second parameter is and that most of the tests are not specifying headers.

Done. Thanks for clearer instruction.

C0urante

Looks pretty good @haidangdam, just one question.

C0urante · 2019-05-23T23:01:55Z

If the REST API requires authorization, won't this request fail due to lack of headers?

Any answer to this?

Hi. Pattern will filter out this endpoint so that authorization will not fail.

Hi @haidangdam, you mean there is no Authorization for POST /connectors/xx/tasks, so anyone can change tasks configuration ??

Currently this endpoint should be secured through other means, such as mutual TLS.

rayokota

LGTM

C0urante · 2019-06-03T17:57:17Z

I don't think the backslashes in this regex string are necessary; can it be simplified to "/?connectors/[^/]+/tasks/?"?

Actually, why don't we initialize the final as a Pattern instance so that we don't have to parse the pattern with every request?

That's a good addition too. We should definitely clean up the regex if possible too, though.

yup. changed as well! Thanks for the suggestion

rhauch

Thanks for the update, @haidangdam. A few questions and suggestions.

rhauch · 2019-06-03T19:49:52Z

Any answer to this?

rhauch · 2019-06-03T19:54:31Z

I would like to see:

connectorsResource.createConnector(FORWARD, NULL_HEADERS, bodyIn);

to make it much more clear what the second parameter is and that most of the tests are not specifying headers.

rhauch · 2019-06-03T19:55:31Z

Actually, why don't we initialize the final as a Pattern instance so that we don't have to parse the pattern with every request?

C0urante

LGTM, thanks for the fix @haidangdam!

Ideally we'll follow up to lock down the putTaskConfig resource, but that's probably out of scope for this PR and, while incomplete, this is strictly an improvement over the state of authentication in the existing REST API code.

rhauch

LGTM. Thanks, @haidangdam!

…/tasks

Resolved conflict

…/tasks

rhauch · 2019-06-03T23:04:18Z

The JDK 11 build failed on a Stream test, and all Connect tests passed with these changes in that build. The Connect tests all passed in the JDK 8 build, also.

@rayokota, @haidangdam: let's rebase this PR branch and run the Connect system tests with it. The rebase is necessary to pick up some Connect system test fixes that were merged since this branch was last rebased.

…EST API (#6791) When Connect forwards a REST request from one worker to another, the Authorization header was not forwarded. This commit changes the Connect framework to add include the authorization header when forwarding requests to other workers. Author: Hai-Dang Dam <damquanghaidang@gmail.com> Reviewers: Robert Yokota <rayokota@gmail.com>, Randall Hauch <rhauch@gmail.com>

…EST API (#6791) When Connect forwards a REST request from one worker to another, the Authorization header was not forwarded. This commit changes the Connect framework to add include the authorization header when forwarding requests to other workers. Author: Hai-Dang Dam <damquanghaidang@gmail.com> Reviewers: Robert Yokota <rayokota@gmail.com>, Randall Hauch <rhauch@gmail.com> # Conflicts: # connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java # connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResourceTest.java

…EST API (#6791) When Connect forwards a REST request from one worker to another, the Authorization header was not forwarded. This commit changes the Connect framework to add include the authorization header when forwarding requests to other workers. Author: Hai-Dang Dam <damquanghaidang@gmail.com> Reviewers: Robert Yokota <rayokota@gmail.com>, Randall Hauch <rhauch@gmail.com> # Conflicts: # connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java # connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResourceTest.java # Conflicts: # connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java

* AK_REPO/2.3: KAFKA-8155: Add 2.2.0 release to system tests (apache#6597) KAFKA-8404: Add HttpHeader to RestClient HTTP Request and Connector REST API (apache#6791) KAFKA-8473: Adjust Connect system tests for incremental cooperative rebalancing (apache#6872) KAFKA-8475: Temporarily restore SslFactory.sslContext() helper KAFKA-8449: Restart tasks on reconfiguration under incremental cooperative rebalancing (apache#6850) KAFKA-8426; Fix for keeping the ConfigProvider configs consistent with KIP-297 (apache#6750) KAFKA-8425: Fix for correctly handling immutable maps (KIP-421 bug) (apache#6795) MINOR: Reordering the props modification with configs construction

…EST API (apache#6791) When Connect forwards a REST request from one worker to another, the Authorization header was not forwarded. This commit changes the Connect framework to add include the authorization header when forwarding requests to other workers. Author: Hai-Dang Dam <damquanghaidang@gmail.com> Reviewers: Robert Yokota <rayokota@gmail.com>, Randall Hauch <rhauch@gmail.com>

rayokota reviewed May 23, 2019

View reviewed changes

Comment thread connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestClient.java Outdated

Comment thread connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestClient.java Outdated

rayokota reviewed May 23, 2019

View reviewed changes

rhauch changed the title ~~Add HttpHeader to RestClient HTTP Request and Connector REST API~~ KAFKA-8404: Add HttpHeader to RestClient HTTP Request and Connector REST API May 23, 2019

rhauch reviewed May 23, 2019

View reviewed changes

C0urante reviewed May 23, 2019

View reviewed changes

haidangdam force-pushed the KAFKA-8404-add-headers-connectors-rest branch 2 times, most recently from de9032a to 78c867e Compare June 3, 2019 17:41

rayokota approved these changes Jun 3, 2019

View reviewed changes

C0urante reviewed Jun 3, 2019

View reviewed changes

rhauch reviewed Jun 3, 2019

View reviewed changes

C0urante approved these changes Jun 3, 2019

View reviewed changes

rhauch approved these changes Jun 3, 2019

View reviewed changes

Hai-Dang Dam added 14 commits June 3, 2019 16:01

Add HttpHeader to RestClient HTTP Request and Connector REST API

470fb6b

Clearer function name

6a6c094

KAFKA-8404: Not allow authentication with POST /connectors/{connName}…

36a8240

…/tasks

KAFKA-8404: Minor fix

f4b050a

Add HttpHeader to RestClient HTTP Request and Connector REST API

c7bb262

Clearer function name

17ad54e

KAFKA-8404: Not allow authentication with POST /connectors/{connName}…

ff412d2

…/tasks

KAFKA-8404: Minor fix

97c1e04

KAFKA-8404:Minor fix

4c6524c

Add HttpHeader to RestClient HTTP Request and Connector REST API

422d517

Resolved conflict

Clearer function name

fd15a03

KAFKA-8404: Not allow authentication with POST /connectors/{connName}…

68e6801

…/tasks

KAFKA-8404: Minor fix

b040f84

Resolve rebase conflict

a0cdf32

Hai-Dang Dam added 2 commits June 3, 2019 16:01

Minor change in String regex for task request pattern

4e2c989

Change checkstyle

d30d6ce

haidangdam force-pushed the KAFKA-8404-add-headers-connectors-rest branch from 0674512 to d30d6ce Compare June 3, 2019 23:02

rhauch merged commit 1a3fe9a into apache:trunk Jun 4, 2019

kkonstantine added the connect label Oct 16, 2020

Uh oh!

Conversation

haidangdam commented May 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

haidangdam commented May 23, 2019

Uh oh!

haidangdam commented May 23, 2019

Uh oh!

rayokota left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

haidangdam commented May 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rayokota left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haidangdam commented May 23, 2019

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

C0urante left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haidangdam Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rayokota left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

C0urante left a comment

Choose a reason for hiding this comment

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

haidangdam commented May 23, 2019 •

edited

Loading

haidangdam commented May 23, 2019 •

edited

Loading

haidangdam Jun 3, 2019 •

edited

Loading