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.

Sign up for GitHub

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

Jump to bottom

pp: address several improvements to basic auth and the client cache #6626

Merged
merged 3 commits into from
Oct 14, 2022
Merged

pp: address several improvements to basic auth and the client cache #6626

merged 3 commits into from
Oct 14, 2022

Conversation

NyaliaLui
Copy link
Contributor

@NyaliaLui NyaliaLui commented Oct 4, 2022 •
edited
Loading

Cover letter

#6452 Introduced HTTP Basic Auth support and a kafka client cache to the Pandaproxy. There were, however, several nits left on that PR.

This PR addresses some of those comments and a few other minor points

Closes #6616
Closes #6607

Changes from force-push 8cb9180:

  • Rebased on dev to fetch new CI fixes
  • Use confluent 40101 instead of HTTP 401 for unauthorized errors
  • Added password change test to ducktape

Changes from force-push 2ae8c71 and b77eb0b:

  • Clean up cache fixture test
  • Change eviction window to 1s

Changes from force-push 4184278:

  • Wait on gate.close in kafka_client_cache unit tests

Follow-ups left to do are:
#6617
#6595

Backport Required

  • not a bug fix
  • issue does not exist in previous branches
  • papercut/not impactful enough to backport
  • v22.2.x
  • v22.1.x
  • v21.11.x

UX changes

  • none

Release notes

  • none

@mmedenjak mmedenjak added kind/enhance New feature or request area/pandaproxy REST interface for Kafka API area/security labels Oct 5, 2022
BenPope
BenPope reviewed Oct 5, 2022
View reviewed changes
Copy link
Member

@BenPope BenPope left a comment

Choose a reason for hiding this comment

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

Did you do any analysis on the status codes as to the compatibility with other implementations?

src/v/pandaproxy/error.cc Outdated Show resolved Hide resolved
src/v/pandaproxy/kafka_client_cache.cc Outdated Show resolved Hide resolved
src/v/pandaproxy/kafka_client_cache.h Outdated Show resolved Hide resolved
@NyaliaLui
Copy link
Contributor Author

Remaining follow-ups after this PR:
#6617
#6595

@NyaliaLui NyaliaLui requested a review from BenPope October 5, 2022 21:15
dotnwat
dotnwat reviewed Oct 5, 2022
View reviewed changes
src/v/pandaproxy/rest/proxy.cc Outdated Show resolved Hide resolved
src/v/pandaproxy/sharded_client_cache.cc Outdated Show resolved Hide resolved
src/v/pandaproxy/kafka_client_cache.cc Outdated Show resolved Hide resolved
BenPope
BenPope reviewed Oct 6, 2022
View reviewed changes
src/v/pandaproxy/rest/proxy.cc Outdated Show resolved Hide resolved
src/v/pandaproxy/kafka_client_cache.cc Outdated Show resolved Hide resolved
@NyaliaLui NyaliaLui mentioned this pull request Oct 6, 2022
Closed
6 tasks
@NyaliaLui
Copy link
Contributor Author

Did you do any analysis on the status codes as to the compatibility with other implementations?

I still need to do this analysis. I spent too much time on cache eviction.

@NyaliaLui NyaliaLui mentioned this pull request Oct 7, 2022
Merged
6 tasks
@NyaliaLui
pp/errors: handle every ss::httpd error
ef7db20 
Currently the Pandaproxy will handle the majority of HTTP errors as a
Kafka Bad Request error. Instead each HTTP error should have an
individual response.

Fixes redpanda-data#6616
@NyaliaLui
Copy link
Contributor Author

NyaliaLui commented Oct 13, 2022 •
edited
Loading

Did you do any analysis on the status codes as to the compatibility with other implementations?

I still need to do this analysis. I spent too much time on cache eviction.

I'm delaying this until after the release.

Remaining follow-ups after this PR: #6617 #6595

The above tickets as well as #6766 #6765 #6764 #6763 can wait until after the release

BenPope
BenPope reviewed Oct 13, 2022
View reviewed changes
Copy link
Member

@BenPope BenPope left a comment

Choose a reason for hiding this comment

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

Did you have any luck with the karapace tests? I can't image the new error codes would cause a regression, kafka_bad_request probably isn't retriable.

src/v/pandaproxy/test/kafka_client_cache.cc Outdated Show resolved Hide resolved
tests/rptest/tests/pandaproxy_test.py Show resolved Hide resolved
dotnwat
dotnwat reviewed Oct 14, 2022
View reviewed changes
Copy link
Member

@dotnwat dotnwat left a comment

Choose a reason for hiding this comment

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

looks good. just a question about the debounce logic

src/v/pandaproxy/kafka_client_cache.cc Outdated Show resolved Hide resolved
@NyaliaLui NyaliaLui mentioned this pull request Oct 14, 2022
Closed
@NyaliaLui
Copy link
Contributor Author

Did you have any luck with the karapace tests? I can't image the new error codes would cause a regression, kafka_bad_request probably isn't retriable.

@BenPope no luck with running the karapace tests yet

@NyaliaLui
pp: trigger eviction when a client is removed
8043da5 
Previously client.stop() was not called on the evicted client until
client garbage collection kicked in. This meant that resources were not
freed until some later time. This commit instead triggers the eviction
process when a client is removed from the cache.

Closes redpanda-data#6607
@NyaliaLui
pp/test: create password change test
4184278
@NyaliaLui NyaliaLui merged commit 4c9c0db into redpanda-data:dev Oct 14, 2022
@NyaliaLui NyaliaLui mentioned this pull request Nov 3, 2022
Merged
6 tasks
@NyaliaLui NyaliaLui deleted the basic-auth-improvements branch November 18, 2022 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dotnwat dotnwat dotnwat approved these changes

@BenPope BenPope Awaiting requested review from BenPope

Assignees
No one assigned
Labels
area/pandaproxy REST interface for Kafka API area/redpanda area/security kind/enhance New feature or request
Projects
None yet
Milestone
No milestone
4 participants
@NyaliaLui @dotnwat @BenPope @mmedenjak