feat: credential injector impl #30850

zhaohuabing · 2023-11-13T17:07:50Z

Implementation for the credential injector HTTP filter.
This PR includes the implementation of the Credential Injector filter and the Generic extension type. The oauth2 extension type will be in a follow-up pR.

Related: #27769

Sponsor: @kyessenov kindly offered to be the sponsor of this new filter.

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

repokitteh-read-only · 2023-11-13T17:08:00Z

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: #30850 was opened by zhaohuabing.

see: more, trace.

adisuissa

Small API comment.
RE doc errors, I assume it is due to linking to "not-implemented-hide" protos.

api/envoy/extensions/filters/http/credential_injector/v3/credential_injector.proto

adisuissa · 2023-11-21T04:01:26Z

Assigning Kuat as code-owner.
/assign @kyessenov

adisuissa

Thanks!
Left a small suggestion on the API, but otherwise the API LGTM.

api/envoy/extensions/filters/http/credential_injector/v3/credential_injector.proto

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

zhaohuabing · 2024-03-31T03:30:32Z

/retest

zhaohuabing · 2024-04-02T02:32:26Z

After discussing with @wbpcode, we think the initialization process can be handled in individual credential extensions. This means the filter only needs to call the inject method.

The oauth2 credential extension/or other possible similar extensions manage the initialization of credentials on their own, just like how generic extension handles SDS secrets.
- It retrieves the credentials from the OAuth2 server through the client credential flow.
- The received credentials are stored in TLS (Thread Local Storage) for accessibility by worker threads.
During the decodeHeaders stage, the credential injector filter simply calls the inject method of the extensions to inject the credentials.

@kyessenov I believe you suggested a similar approach before?

If we're all in agreement with this approach, I'll proceed with updating the current implementation as outlined above.

kyessenov · 2024-04-02T02:55:49Z

Sounds reasonable. The details are easier to work out once you have an implementation for oauth2. All we really need is to be able to pause a request, and let the extension handle the job itself (e.g. no explicit thread management in this filter).

zhaohuabing · 2024-04-02T03:47:40Z

All we really need is to be able to pause a request, and let the extension handle the job itself (e.g. no explicit thread management in this filter).

I'm thinking just failing the request and letting the client decide the next step(fail, retry, etc.) if the extension fails to initialize the credentials, like what we have done with the generic extension(SDS).

By this way, we can decouple the filter and the extension's credential initialization/retry logic.

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

wbpcode

Thanks for your update. It's much simpler (and better) now. Fix the tests and CI, then it is ready for final review.

source/extensions/filters/http/credential_injector/credential_injector_filter.cc

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

zhaohuabing · 2024-04-08T11:05:49Z

/retest

phlax · 2024-04-08T11:19:03Z

no point retesting atm - test certs are currently broken (#33389)

…ilter_impl

wbpcode

Thanks for your update. It's near to there. Only some minor comments are added to the implementation details.

source/extensions/filters/http/credential_injector/credential_injector_filter.h

source/extensions/http/injected_credentials/generic/generic_impl.h

test/extensions/filters/http/credential_injector/filter_test.cc

wbpcode · 2024-04-09T01:50:00Z

test/extensions/filters/http/credential_injector/filter_test.cc

+class MockSecretReader : public Http::InjectedCredentials::Common::SecretReader {
+public:
+  MockSecretReader(const std::string& secret) : secret_(secret){};
+  const std::string& credential() const override { return secret_; }
+
+private:
+  const std::string secret_;
+};


nit: I think it would be better to mock the CredentialInjector rather than to mock the SecretReader. Because the filter actually only use the CredentialInjector and the SecretReader is only part of implementation of GenericCredentialInjector.

wbpcode · 2024-04-09T01:51:07Z

/wait

Signed-off-by: huabing zhao <zhaohuabing@gmail.com> wrap tests in anonymous namespace Signed-off-by: huabing zhao <zhaohuabing@gmail.com> address comments Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

…ilter_impl

wbpcode

LGTM. Thanks so much for your great works and paying of so long time.

zhaohuabing · 2024-04-10T12:19:17Z

Thank you @kyessenov @lizan @adisuissa @wbpcode for helping on reviews and all the great feedback. I really appreciate it.

* credential injector impl Signed-off-by: huabing zhao <zhaohuabing@gmail.com> --------- Signed-off-by: huabing zhao <zhaohuabing@gmail.com> Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> Co-authored-by: phlax <phlax@users.noreply.github.com> Co-authored-by: Adi (Suissa) Peleg <adip@google.com>

repokitteh-read-only bot added the api label Nov 13, 2023

repokitteh-read-only bot assigned adisuissa Nov 13, 2023

zhaohuabing changed the title ~~credential injector impl~~ feat: credential injector impl Nov 13, 2023

zhaohuabing marked this pull request as draft November 13, 2023 17:08

zhaohuabing force-pushed the credential_injector_filter_impl branch 7 times, most recently from 1350725 to 78d5e62 Compare November 15, 2023 05:12

This comment was marked as outdated.

Sign in to view

zhaohuabing marked this pull request as ready for review November 16, 2023 05:02

adisuissa reviewed Nov 17, 2023

View reviewed changes

api/envoy/extensions/filters/http/credential_injector/v3/credential_injector.proto Outdated Show resolved Hide resolved

zhaohuabing force-pushed the credential_injector_filter_impl branch 2 times, most recently from b829c04 to b93e805 Compare November 20, 2023 21:45

repokitteh-read-only bot assigned kyessenov Nov 21, 2023

zhaohuabing force-pushed the credential_injector_filter_impl branch from fff14f0 to 17e9c6a Compare November 24, 2023 02:58

adisuissa reviewed Nov 27, 2023

View reviewed changes

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

zhaohuabing force-pushed the credential_injector_filter_impl branch 5 times, most recently from 887b67b to 21934d7 Compare November 28, 2023 05:57

zhaohuabing requested a review from adisuissa November 28, 2023 06:09

zhaohuabing added 3 commits November 28, 2023 07:09

credential injector impl

294b39c

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

fail the request if failed to inject the credential

2eb1ae8

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

refactor

c10beb4

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

zhaohuabing added 3 commits March 31, 2024 00:49

Config::Utility::getFactory

c284f3e

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

PassThroughDecoderFilter

49ba37d

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

string view

944318b

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

zhaohuabing requested review from wbpcode and adisuissa March 31, 2024 03:20

zhaohuabing added 2 commits April 2, 2024 04:27

refactor

6345bb4

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

fix test

d213342

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

wbpcode reviewed Apr 4, 2024

View reviewed changes

source/extensions/filters/http/credential_injector/credential_injector_filter.cc Outdated Show resolved Hide resolved

address comments

a2c16c9

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

Merge remote-tracking branch 'origin/main' into credential_injector_f…

86edacb

…ilter_impl

wbpcode reviewed Apr 9, 2024

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 9, 2024

address comments

f208668

Signed-off-by: huabing zhao <zhaohuabing@gmail.com> wrap tests in anonymous namespace Signed-off-by: huabing zhao <zhaohuabing@gmail.com> address comments Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

repokitteh-read-only bot removed the waiting label Apr 10, 2024

zhaohuabing requested a review from wbpcode April 10, 2024 07:15

Merge remote-tracking branch 'origin/main' into credential_injector_f…

1e7974c

…ilter_impl

wbpcode approved these changes Apr 10, 2024

View reviewed changes

repokitteh-read-only bot removed the api label Apr 10, 2024

wbpcode merged commit 5f68428 into envoyproxy:main Apr 10, 2024
54 checks passed

ericvn mentioned this pull request Apr 10, 2024

Automator: update envoy@ in istio/proxy@master istio/proxy#5455

Closed

vikaschoudhary16 mentioned this pull request Apr 19, 2024

Credential Injector Filter: OAuth2 client credential extension #33702

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: credential injector impl #30850

feat: credential injector impl #30850

zhaohuabing commented Nov 13, 2023 •

edited

Loading

repokitteh-read-only bot commented Nov 13, 2023

This comment was marked as outdated.

adisuissa left a comment

adisuissa commented Nov 21, 2023

adisuissa left a comment

zhaohuabing commented Mar 31, 2024

zhaohuabing commented Apr 2, 2024 •

edited

Loading

kyessenov commented Apr 2, 2024

zhaohuabing commented Apr 2, 2024

wbpcode left a comment

zhaohuabing commented Apr 8, 2024

phlax commented Apr 8, 2024

wbpcode left a comment

wbpcode Apr 9, 2024

wbpcode commented Apr 9, 2024

wbpcode left a comment

zhaohuabing commented Apr 10, 2024

feat: credential injector impl #30850

feat: credential injector impl #30850

Conversation

zhaohuabing commented Nov 13, 2023 • edited Loading

repokitteh-read-only bot commented Nov 13, 2023

This comment was marked as outdated.

adisuissa left a comment

Choose a reason for hiding this comment

adisuissa commented Nov 21, 2023

adisuissa left a comment

Choose a reason for hiding this comment

zhaohuabing commented Mar 31, 2024

zhaohuabing commented Apr 2, 2024 • edited Loading

kyessenov commented Apr 2, 2024

zhaohuabing commented Apr 2, 2024

wbpcode left a comment

Choose a reason for hiding this comment

zhaohuabing commented Apr 8, 2024

phlax commented Apr 8, 2024

wbpcode left a comment

Choose a reason for hiding this comment

wbpcode Apr 9, 2024

Choose a reason for hiding this comment

wbpcode commented Apr 9, 2024

wbpcode left a comment

Choose a reason for hiding this comment

zhaohuabing commented Apr 10, 2024

zhaohuabing commented Nov 13, 2023 •

edited

Loading

zhaohuabing commented Apr 2, 2024 •

edited

Loading