feat: credential injector impl by zhaohuabing · Pull Request #30850 · envoyproxy/envoy

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>

wbpcode · 2024-03-28T06:42:25Z

will look at this PR tomorrow. It's huge. orz

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

wbpcode

Thanks for your great contribution. This is a super huge PR. Thanks again for all your time and paying for it.

Some initial important comments are added to this PR.

CODEOWNERS

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

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

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

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

source/extensions/injected_credentials/common/credential.h

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.

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 Outdated 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>

Merge branch 'main' into credential_injector_filter_impl

70b4941

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

wbpcode requested changes Mar 29, 2024

View reviewed changes

move credentials to http directory

6db727b

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

repokitteh-read-only bot added the api label Mar 31, 2024

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 adisuissa and wbpcode 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

zhaohuabing added 2 commits April 10, 2024 07:00

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>

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

1e7974c

…ilter_impl

wbpcode approved these changes Apr 10, 2024

View reviewed changes

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

Conversation

zhaohuabing commented Nov 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Nov 13, 2023

Uh oh!

This comment was marked as outdated.

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adisuissa commented Nov 21, 2023

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wbpcode commented Mar 28, 2024

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!

Uh oh!

zhaohuabing commented Mar 31, 2024

Uh oh!

zhaohuabing commented Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kyessenov commented Apr 2, 2024

Uh oh!

zhaohuabing commented Apr 2, 2024

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zhaohuabing commented Apr 8, 2024

Uh oh!

phlax commented Apr 8, 2024

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wbpcode Apr 9, 2024

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Apr 9, 2024

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

zhaohuabing commented Apr 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

zhaohuabing commented Nov 13, 2023 •

edited

Loading

zhaohuabing commented Apr 2, 2024 •

edited

Loading