Creating JwksFetcher interface and impl #4225
Conversation
| namespace HttpFilters { | ||
| namespace Common { | ||
|
|
||
| class JwksFetcher { |
There was a problem hiding this comment.
Can you add comment to describe its usage? especially if its instance can be re-used for multiple fetching
| /* | ||
| * Close/stop any inflight request. | ||
| */ | ||
| virtual void close() PURE; |
There was a problem hiding this comment.
Changed to cancel() in e6b6b0 as per review comment.
|
|
||
| class JwksFetcher { | ||
| public: | ||
| typedef std::unique_ptr<JwksFetcher> JwksFetcherPtr; |
There was a problem hiding this comment.
move xxxPtr definition to outside of class xxx
There was a problem hiding this comment.
Changed in e6b6b0 as per review comment.
| AuthenticatorPtr Authenticator::create(FilterConfigSharedPtr config) { | ||
| return std::make_unique<AuthenticatorImpl>(config); | ||
| AuthenticatorPtr Authenticator::create(FilterConfigSharedPtr config, | ||
| Common::JwksFetcher::JwksFetcherPtr& fetcher) { |
There was a problem hiding this comment.
should we use && since you are using std::move for fetcher
There was a problem hiding this comment.
Changed in e6b6b0 as per review comment.
| return std::make_unique<AuthenticatorImpl>(config); | ||
| AuthenticatorPtr Authenticator::create(FilterConfigSharedPtr config, | ||
| Common::JwksFetcher::JwksFetcherPtr& fetcher) { | ||
| return std::make_unique<AuthenticatorImpl>(config, fetcher); |
There was a problem hiding this comment.
should we call std::move for fetcher
There was a problem hiding this comment.
Changed in e6b6b0 as per review comment.
| @@ -46,8 +46,9 @@ FilterFactory::createFilterFactoryFromProtoTyped(const JwtAuthentication& proto_ | |||
| validateJwtConfig(proto_config); | |||
| auto filter_config = std::make_shared<FilterConfig>(proto_config, prefix, context); | |||
| return [filter_config](Http::FilterChainFactoryCallbacks& callbacks) -> void { | |||
| callbacks.addStreamDecoderFilter( | |||
| std::make_shared<Filter>(filter_config->stats(), Authenticator::create(filter_config))); | |||
| auto jwks_fetcher = Common::JwksFetcher::create(filter_config->cm()); | |||
There was a problem hiding this comment.
can we pass a lambda function as fetcher_factory so fetcher is only created when it is needed, not always for each request?
|
This has turned into a bit of a mess. I'll fix before more review. Apologies |
|
I've closed this PR as the history had become very messy. The failure on the CI and resultant DCO instructions around rebasing have not been kind. I'll create a new version. |
JwksFetcher wraps up HTTP acquisition of JWKS strings converting them into a concrete type on the way. JwksFetcher is reusable so can be used in a wider context. Tests updated and fixed where necessary. We are in the process of implementing a new Envoy filter based on the design presented here and wish to re-use existing logic in the jwt_authn filter. We've split out the logic we're interested in into a new class called JwksFetcher. Later PRs will re-use the split out logic an OpenID Connect filter. This is the second crack at this PR (see #4225 which went horribly wrong after following the DCO rebase guidelines). Risk Level: Medium Testing: Add replacement and additional unit tests for the logic that's been moved. Signed-off-by: Nick A. Smith <[email protected]>
JwksFetcher wraps up HTTP acquisition of JWKS strings converting them into a concrete type on the way.
JwksFetcher is reusable so can be used in a wider context.
Tests updated and fixed where necessary.
Signed-off-by: Nick A. Smith [email protected]
For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md
Description:
We are in the process of implementing a new Envoy filter based on the design presented here and wish to re-use existing logic in the jwt_authn filter. We've split out the logic we're interested in into a new class called JwksFetcher. Later PRs will re-use the split out logic an OpenID Connect filter.
Risk Level:
Medium
Testing:
Add replacement and additional unit tests for the logic that's been moved.
Docs Changes:
None
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]