Credential Injector Filter: OAuth2 client credential extension by vikaschoudhary16 · Pull Request #33702 · envoyproxy/envoy

vikaschoudhary16 · 2024-04-19T12:10:18Z

Commit Message:
This PR is follow up to #30850 and is adding oauth2(client credential grant) extension. At a high level, oauth2 extension has TokenProvider which runs on main thread to fetch token from oauth server and sets tls slot with the received token. All worker threads then access this token from tls to inject requestHeaders during decodeHeaders.

Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

vikaschoudhary16 · 2024-04-19T12:14:18Z

    #
    # Access loggers
    #
-


oh, I will fix unnecessary formatting.

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

repokitteh-read-only · 2024-04-24T10:45:17Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #33702 was synchronize by vikaschoudhary16.

see: more, trace.

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

vikaschoudhary16 · 2024-04-27T03:10:52Z

I will work on coverage and adding docs, meanwhile early feedback is much appreciated. @kyessenov @wbpcode

wbpcode · 2024-04-29T02:26:40Z

+  timer_ = dispatcher_->createTimer([this]() -> void { asyncGetAccessToken(); });
+  ENVOY_LOG(debug, "onGetAccessTokenSuccess: Token fetched successfully, expires in {} seconds.",
+            expires_in.count());
+  timer_->enableTimer(expires_in / 2);


I think the things about timer could be simper. You completely needn't create the timer again and again. You can only create one and enable/disable it according your requirement here.

wbpcode

This look great overall. Only a comment about the retry timer. Thanks for working on this.

wbpcode · 2024-04-29T02:27:22Z

/wait

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

wbpcode

Thanks for you quick response and update. Some comments are added. Please note the possible lifetime problem of the in-flight-request.

wbpcode · 2024-05-02T08:58:08Z

+    return absl::NotFoundError("Failed to get oauth2 token from token provider");
+  }
+
+  headers.setCopy(Envoy::Http::CustomHeaders::get().Authorization, token);


nit: setReferenceKey()

wbpcode · 2024-05-02T09:10:57Z

+    thread_local_cluster->httpAsyncClient().send(
+        std::move(msg), *this,
+        Envoy::Http::AsyncClient::RequestOptions().setTimeout(
+            std::chrono::milliseconds(PROTOBUF_GET_MS_REQUIRED(uri_, timeout))));


What will happen if the client was destructed because the xDS updating and then the a response is received from remote server?
I think you may need to store the in-flight-request here and use the result to cancel any pending request when the client is destructed.

thanks for pointing out. Addressed it.

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

phlax · 2024-05-03T08:43:35Z

/docs

repokitteh-read-only · 2024-05-03T08:43:40Z

Docs for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/33702/docs/index.html

The docs are (re-)rendered each time the CI envoy-presubmit (precheck docs) job completes.

🐱

Caused by: a #33702 (comment) was created by @phlax.

see: more, trace.

phlax

docs lgtm, thanks again @vikaschoudhary16 for working on this

handing over to @wbpcode and @adisuissa for signoff

wbpcode

Thanks for your hard work. The code is fine to me. Most comments are for tests.

wbpcode · 2024-05-06T12:00:59Z

+
+#include "test/mocks/event/mocks.h"
+#include "test/mocks/server/factory_context.h"
+#include "test/mocks/thread_local/mocks.h"
+#include "test/mocks/upstream/cluster_manager.h"
+
+#include "gtest/gtest.h"
+
+namespace Envoy {
+namespace Extensions {
+namespace Http {
+namespace InjectedCredentials {
+namespace OAuth2 {


I think all these oauth2 tests should be placed in the test/extensions/http/credential_injector/oauth2?

And seems the unit tests are pretty simple (although the integration tests are great)

adisuissa

/lgtm api

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

vikaschoudhary16 · 2024-05-09T02:28:42Z

Thanks a lot @wbpcode @adisuissa @phlax for guidance and feedback. really appreciate!!

Credential Injector Filter: Oauth2 client credential extension

0b65ec0

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

vikaschoudhary16 marked this pull request as draft April 19, 2024 12:10

vikaschoudhary16 commented Apr 19, 2024

View reviewed changes

vikaschoudhary16 added 2 commits April 23, 2024 13:13

Add tests

b024b40

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

format

dfdab41

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

vikaschoudhary16 force-pushed the client-cred-oauth2 branch from 446b6af to dfdab41 Compare April 24, 2024 09:33

proto update

5d4f9be

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

repokitteh-read-only Bot added the api label Apr 24, 2024

repokitteh-read-only Bot assigned adisuissa Apr 24, 2024

vikaschoudhary16 marked this pull request as ready for review April 24, 2024 10:55

vikaschoudhary16 requested a review from kyessenov as a code owner April 24, 2024 10:55

vikaschoudhary16 marked this pull request as draft April 24, 2024 11:32

vikaschoudhary16 added 8 commits April 24, 2024 18:24

doc and gcc fix

a77ede1

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

docs

e9d10ce

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

more tests and configuration knob for retry interval

a8c6b64

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

format

844d7f4

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

format

2fdd985

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

retrigger ci

cc9d30f

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

test

9999d38

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

test

d8d2470

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

vikaschoudhary16 marked this pull request as ready for review April 27, 2024 03:09

wbpcode reviewed Apr 29, 2024

View reviewed changes

repokitteh-read-only Bot added the waiting label Apr 29, 2024

docs update

537c671

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

repokitteh-read-only Bot removed the waiting label Apr 29, 2024

enable/disable instead recreating timer

4859865

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

wbpcode requested changes May 2, 2024

View reviewed changes

vikaschoudhary16 added 5 commits May 2, 2024 15:33

Address feedback: docs and request lifetime

7f298cf

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

title line

99af0c5

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

fix doc

9c36381

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

More tests

da8fb23

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

format

c8c50fe

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

vikaschoudhary16 requested review from adisuissa, phlax and wbpcode May 3, 2024 02:10

phlax previously approved these changes May 3, 2024

View reviewed changes

wbpcode reviewed May 6, 2024

View reviewed changes

adisuissa reviewed May 6, 2024

View reviewed changes

repokitteh-read-only Bot removed the api label May 6, 2024

Address feedback

9a09099

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

vikaschoudhary16 dismissed phlax’s stale review via 9a09099 May 7, 2024 05:54

vikaschoudhary16 added 2 commits May 7, 2024 12:00

revert accidental commit

958268c

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

Merge branch 'main' into client-cred-oauth2

fb25af4

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

vikaschoudhary16 requested a review from wbpcode May 7, 2024 08:16

vikaschoudhary16 added 4 commits May 9, 2024 04:17

Move extension tests to under http/extensions

faacc74

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

add teardown

a7dd4cf

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

tear down

3cc083f

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

asan

eb73004

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>

wbpcode approved these changes May 9, 2024

View reviewed changes

wbpcode enabled auto-merge (squash) May 9, 2024 01:38

wbpcode merged commit 1d180ac into envoyproxy:main May 9, 2024

phlax mentioned this pull request May 9, 2024

[WIP] configs: Remove (windows) exclude for credentials yaml config #33848

Closed

nitinparab mentioned this pull request Jan 28, 2025

Support Envoy to fetch token via SDS and make token consumable by filters and clusters. #6654

Open

Conversation

vikaschoudhary16 commented Apr 19, 2024

Uh oh!

vikaschoudhary16 Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only Bot commented Apr 24, 2024

Uh oh!

vikaschoudhary16 commented Apr 27, 2024

Uh oh!

wbpcode Apr 29, 2024

Choose a reason for hiding this comment

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Apr 29, 2024

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

wbpcode May 2, 2024

Choose a reason for hiding this comment

Uh oh!

wbpcode May 2, 2024 • 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!

vikaschoudhary16 May 2, 2024

Choose a reason for hiding this comment

Uh oh!

phlax commented May 3, 2024

Uh oh!

repokitteh-read-only Bot commented May 3, 2024

Uh oh!

phlax left a comment

Choose a reason for hiding this comment

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wbpcode May 6, 2024

Choose a reason for hiding this comment

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

vikaschoudhary16 commented May 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wbpcode May 2, 2024 •

edited

Loading