jwt_authn: RemoteJwks to support RetryPolicy config #16319 by alichnewsky · Pull Request #16924 · envoyproxy/envoy

alichnewsky · 2021-06-10T09:15:12Z

Signed-off-by: Anthony Lichnewsky alichnewsky@users.noreply.github.com

Commit Message: jwt_authn: RemoteJwks to support RetryPolicy config #16319 ( only for background fetches failing )
Additional Description: following up on comments and code pruned from background jwks refresh mechanism from #16298.
Risk Level: Low, default number of retries set to zero. truncated exponential backoff requires explicit configuration.
Testing: Only mock-based unit tests so far. Haven't been able to get it ( or the background jwks fetch option, for that matter ) built in esp-v2 so far.
Docs Changes:
Release Notes:
Platform Specific Features:
Fixes #16319

repokitteh-read-only · 2021-06-10T09:15:16Z

Hi @alichnewsky, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #16924 was opened by alichnewsky.

see: more, trace.

repokitteh-read-only · 2021-06-10T09:15:20Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #16924 was opened by alichnewsky.

see: more, trace.

alichnewsky · 2021-06-10T09:25:45Z

this is really code that @qiwzhang was already looking at.
I am not married to this implementation, my goal is to get the ball running.

The issue of missing retries was encountered during load tests on services proxied behind Google Cloud Platform's ESPv2

source/extensions/filters/http/jwt_authn/jwks_async_fetcher.cc

markdroth · 2021-06-10T20:45:28Z

/lgtm api

api/envoy/extensions/filters/http/jwt_authn/v3/config.proto

source/extensions/filters/http/common/jwks_fetcher.cc

source/extensions/filters/http/jwt_authn/filter_config.h

source/extensions/filters/http/jwt_authn/jwks_async_fetcher.cc

test/extensions/filters/http/common/jwks_fetcher_test.cc

test/extensions/filters/http/jwt_authn/jwks_async_fetcher_test.cc

lizan

/lgtm api
/wait for impl

source/extensions/filters/http/common/jwks_fetcher.cc

test/extensions/filters/http/common/jwks_fetcher_test.cc

test/extensions/filters/http/jwt_authn/jwks_async_fetcher_test.cc

qiwzhang

Look good. Thank you very much!

lizan

Sorry for being late on this.

While this shares same config from router's RetryPolicy, but we're not on same implementations. The HTTP Async Client specifies a NullRetryPolicy here, the optimal way should be allow setting a RetryPolicy in HTTP Async Client, instead of having another implementation and test.

Signed-off-by: Anthony Lichnewsky <alichnewsky@users.noreply.github.com>

alichnewsky · 2021-07-05T14:18:26Z

I force-pushed into this PR the new branch implementing this PR the way @lizan and @qiwzhang suggested.
It looks like it broke CI since the logs from the azure devops pipeline are consistent with the wrong code being checked out.
Most likely some relying on a cache ...

can you please advise?
should I just create another PR ?

api/envoy/extensions/filters/http/jwt_authn/v3/config.proto

source/extensions/filters/http/common/jwks_fetcher.cc

Signed-off-by: Anthony Lichnewsky <alichnewsky@users.noreply.github.com>

…amespace as per code style guidelines Signed-off-by: Anthony Lichnewsky <alichnewsky@users.noreply.github.com>

alichnewsky · 2021-07-08T19:25:39Z

I force-pushed into this PR the new branch implementing this PR the way @lizan and @qiwzhang suggested.
It looks like it broke CI since the logs from the azure devops pipeline are consistent with the wrong code being checked out.
Most likely some relying on a cache ...

can you please advise?
should I just create another PR ?

the last requested changes have been pushed on top of the same branch after the forced update, and the azure pipeline still fails for similar reasons ( most likely the ci scripts can't figure out where to rebase because a force-push occurred? )
clearly it complains about an error in a file I modified with a line number that would make sense only if the file didn't contain my modifications.

Please advise.

qiwzhang

LGTM. Thanks

lizan

Thanks! This LGTM with just one nit

source/common/http/async_client_impl.h

Signed-off-by: Anthony Lichnewsky <alichnewsky@users.noreply.github.com>

lizan · 2021-07-13T02:17:51Z

@alichnewsky The CI failure is real, can you try merge main and see if it fails locally? It might be another merged PR affect it.

…ies-via-async-client-route-retry-policy Signed-off-by: Anthony Lichnewsky <alichnewsky@users.noreply.github.com>

Signed-off-by: Anthony Lichnewsky <alichnewsky@users.noreply.github.com>

alichnewsky · 2021-07-13T15:31:24Z

@alichnewsky The CI failure is real, can you try merge main and see if it fails locally? It might be another merged PR affect it.

you are indeed correct. my bad. correction is trivial, but a some code outside the scope of this PR does not pass spellchecking, formatting, ?clang-tidy? pre-push checks.

alichnewsky · 2021-07-13T16:04:33Z

@alichnewsky The CI failure is real, can you try merge main and see if it fails locally? It might be another merged PR affect it.

you are indeed correct. my bad. correction is trivial, but a some code outside the scope of this PR does not pass spellchecking, formatting, ?clang-tidy? pre-push checks.

it may take me a couple of days to figure out how to fix these ...
stuff like this:

Checking format for test/extensions/filters/http/wasm/test_data/test_body_cpp.cc - ERROR: From ./test/extensions/filters/http/wasm/test_data/test_body_cpp.cc
ERROR: ./test/extensions/filters/http/wasm/test_data/test_body_cpp.cc:16: Don't use std::string_view or toStdStringView; use absl::string_view instead
ERROR: check format failed. run 'tools/code_format/check_format.py fix'
error: failed to push some refs to 'https://github.com/alichnewsky/envoy'

Signed-off-by: Anthony Lichnewsky <alichnewsky@users.noreply.github.com>

alichnewsky · 2021-07-14T15:53:37Z

at this point, I have pushed all the changes I intended to make, and the azure pipeline still fails ( various timeouts, after 3h).

I am not authorized to re-run the failed jobs... The only thing I can do is add a comment somewhere push that and rebuild everything again, which hardly sounds correct.

Please advise.

lizan · 2021-07-14T16:54:25Z

/retest

repokitteh-read-only · 2021-07-14T16:54:29Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16924 (comment) was created by @lizan.

see: more, trace.

alichnewsky · 2021-07-14T20:18:49Z

still the same issue it takes more than 3h just to pass the unit tests on mac.. so the tests get cancelled.
I wouldn't mind adding a timeout=300 parameter here, but I am not even sure that would do the trick.

Alternatively, I could just try to retest as you did every now and then and just hope some ci agent host gets less busy at some point ?

…nvoyproxy#16924) Commit Message: jwt_authn: RemoteJwks to support RetryPolicy config envoyproxy#16319 ( only for background fetches failing ) Additional Description: following up on comments and code pruned from background jwks refresh mechanism from envoyproxy#16912 Risk Level: Low, default number of retries set to zero. truncated exponential backoff requires explicit configuration. Testing: Only mock-based unit tests so far. Haven't been able to get it ( or the background jwks fetch option, for that matter ) built in [esp-v2](https://github.com/GoogleCloudPlatform/espv-2) so far. Docs Changes: Release Notes: Platform Specific Features: Fixes envoyproxy#16319 Signed-off-by: Anthony Lichnewsky <alichnewsky@users.noreply.github.com>

alichnewsky requested a review from lizan as a code owner June 10, 2021 09:15

repokitteh-read-only bot added the api label Jun 10, 2021

repokitteh-read-only bot assigned markdroth Jun 10, 2021

alichnewsky mentioned this pull request Jun 10, 2021

Jwks remote fetch failed ... not retried. GoogleCloudPlatform/esp-v2#533

Closed

qiwzhang reviewed Jun 10, 2021

View reviewed changes

source/extensions/filters/http/jwt_authn/jwks_async_fetcher.cc Outdated Show resolved Hide resolved

qiwzhang reviewed Jun 10, 2021

View reviewed changes

source/extensions/filters/http/jwt_authn/jwks_async_fetcher.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot removed the api label Jun 10, 2021

zuercher assigned qiwzhang Jun 11, 2021

qiwzhang reviewed Jun 15, 2021

View reviewed changes

api/envoy/extensions/filters/http/jwt_authn/v3/config.proto Show resolved Hide resolved

qiwzhang reviewed Jun 15, 2021

View reviewed changes

source/extensions/filters/http/common/jwks_fetcher.cc Show resolved Hide resolved

qiwzhang reviewed Jun 15, 2021

View reviewed changes

source/extensions/filters/http/common/jwks_fetcher.cc Outdated Show resolved Hide resolved

qiwzhang reviewed Jun 15, 2021

View reviewed changes

source/extensions/filters/http/common/jwks_fetcher.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the api label Jun 17, 2021

qiwzhang reviewed Jun 18, 2021

View reviewed changes

source/extensions/filters/http/common/jwks_fetcher.cc Outdated Show resolved Hide resolved

qiwzhang suggested changes Jun 18, 2021

View reviewed changes

alyssawilk assigned lizan Jun 21, 2021

lizan reviewed Jun 21, 2021

View reviewed changes

repokitteh-read-only bot added waiting and removed api waiting labels Jun 21, 2021

qiwzhang suggested changes Jun 24, 2021

View reviewed changes

qiwzhang reviewed Jun 24, 2021

View reviewed changes

test/extensions/filters/http/jwt_authn/jwks_async_fetcher_test.cc Outdated Show resolved Hide resolved

qiwzhang previously approved these changes Jun 24, 2021

View reviewed changes

lizan suggested changes Jun 30, 2021

View reviewed changes

lizan added the waiting label Jun 30, 2021

alichnewsky added 3 commits July 5, 2021 12:51

add test for various RemoteJwks configs with RetryPolicy

93eff0c

Signed-off-by: Anthony Lichnewsky <alichnewsky@users.noreply.github.com>

WIP: fix spelling to pass pre-push spell check verification

edcc5a9

Signed-off-by: Anthony Lichnewsky <alichnewsky@users.noreply.github.com>

WIP: changes to pass pre-push formatting validation

0d297b1

Signed-off-by: Anthony Lichnewsky <alichnewsky@users.noreply.github.com>

alichnewsky dismissed qiwzhang’s stale review via 0d297b1 July 5, 2021 13:10

alichnewsky force-pushed the 16319-jwks-async-fetcher-retries-signed-off branch from 6d19fcf to 0d297b1 Compare July 5, 2021 13:10

repokitteh-read-only bot removed the waiting label Jul 5, 2021

qiwzhang reviewed Jul 7, 2021

View reviewed changes

alichnewsky added 2 commits July 8, 2021 18:14

WIP: update proto documentation as per PR comments

b8d09f2

Signed-off-by: Anthony Lichnewsky <alichnewsky@users.noreply.github.com>

WIP: jwks_fetcher: move constants and static functions to anonymous n…

ddd29cd

…amespace as per code style guidelines Signed-off-by: Anthony Lichnewsky <alichnewsky@users.noreply.github.com>

repokitteh-read-only bot added the api label Jul 8, 2021

qiwzhang previously approved these changes Jul 8, 2021

View reviewed changes

lizan reviewed Jul 9, 2021

View reviewed changes

source/common/http/async_client_impl.h Outdated Show resolved Hide resolved

WIP: use std::make_unique<>() as per PR comment

b107e80

Signed-off-by: Anthony Lichnewsky <alichnewsky@users.noreply.github.com>

alichnewsky dismissed qiwzhang’s stale review via b107e80 July 9, 2021 10:28

alichnewsky added 2 commits July 13, 2021 10:04

Merge remote-tracking branch 'fork/main' into 16319-jwks-fetcher-retr…

96bc97d

…ies-via-async-client-route-retry-policy Signed-off-by: Anthony Lichnewsky <alichnewsky@users.noreply.github.com>

WIP: fix unit test after merge with recent upstream

7bde03b

Signed-off-by: Anthony Lichnewsky <alichnewsky@users.noreply.github.com>

WIP: fix clang-tidy errors

3bf9eae

Signed-off-by: Anthony Lichnewsky <alichnewsky@users.noreply.github.com>

lizan approved these changes Jul 14, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Jul 14, 2021

lizan merged commit b62dae2 into envoyproxy:main Jul 14, 2021

Conversation

alichnewsky commented Jun 10, 2021

Uh oh!

repokitteh-read-only bot commented Jun 10, 2021

Uh oh!

repokitteh-read-only bot commented Jun 10, 2021

Uh oh!

alichnewsky commented Jun 10, 2021

Uh oh!

Uh oh!

Uh oh!

markdroth commented Jun 10, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qiwzhang left a comment

Choose a reason for hiding this comment

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

alichnewsky commented Jul 5, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alichnewsky commented Jul 8, 2021

Uh oh!

qiwzhang left a comment

Choose a reason for hiding this comment

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lizan commented Jul 13, 2021

Uh oh!

alichnewsky commented Jul 13, 2021

Uh oh!

alichnewsky commented Jul 13, 2021

Uh oh!

alichnewsky commented Jul 14, 2021

Uh oh!

lizan commented Jul 14, 2021

Uh oh!

repokitteh-read-only bot commented Jul 14, 2021

Uh oh!

alichnewsky commented Jul 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

alichnewsky commented Jul 14, 2021 •

edited

Loading