http: make credential injector filter a dual filter by jewertow · Pull Request #38398 · envoyproxy/envoy

jewertow · 2025-02-11T10:04:02Z

Commit Message: http: make credential injector filter a dual filter
Additional Description: This change allows users to inject credentials to HTTP CONNECT requests sent to the upstream tunnel proxy. Credentials can come from secrets (static and sds) for example for Basic auth, or from authorization servers in case of OAuth2.
Risk Level:
Testing: Integration test was implemented.
Docs Changes: TODO
Release Notes: done
Fixes #13809

This approach to injecting Proxy-Authorization was suggested by @alyssawilk here

This PR must be merged first: #38399.

Here you can see example configuration for credential injector upstream filter.

…actoryContext (#38399) Commit Message: factory context: extend ServerFactoryContext with getTransportSocketFactoryContext Additional Description: This change is necessary to allow upstream filters reading secrets from SDS. Risk Level: low Testing: - Docs Changes: Not needed as it is only an internal change. Release Notes: Not needed as it is only an internal change. Platform Specific Features: - This change is necessary to unblock: 1. injecting credentials to upstream requests #38398 2. reading image pull secret by upstream wasm filters #37635 --------- Signed-off-by: Jacek Ewertowski <[email protected]>

Signed-off-by: Jacek Ewertowski <[email protected]>

jewertow · 2025-02-13T11:11:24Z

I don't know why the test CredentialInjectorIntegrationTest/RetryOnClusterNotFound fails for some parameters. It works in my local environment, but fail in CI, and the error also does not seem to make any sense:

Error: 2-13 10:05:02.838][783][error][credential_injector] [source/extensions/http/injected_credentials/oauth2/token_provider.cc:79] asyncGetAccessToken: OAuth cluster not found. Retrying in 1 seconds.
./test/integration/server.h:452: Failure
Value of: TestUtility::waitForCounterEq(statStore(), name, value, time_system_, timeout, dispatcher)
  Actual: false (timed out waiting for http.config_test.credential_injector.oauth2.token_fetch_failed_on_cluster_not_found to be 2, current value 2)
Expected: true

2 is not equal to 2 - what? Do you have any idea?
cc: @vikaschoudhary16

Update:

The counter did not reach the value of 2 within 1 second, and the waitForCounterEq waits additionally 10 milliseconds and then asserts the actual and expected values. So it reached 2 after 2010 milliseconds.

Signed-off-by: Jacek Ewertowski <[email protected]>

vikaschoudhary16 · 2025-02-13T12:37:26Z

@jewertow ~~I merged this recently https://github.com/envoyproxy/envoy/pull/38274/files .. looks like your code is not latest?~~
timings are going off sometimes on the CI :(

jewertow · 2025-02-13T12:54:10Z

I've seen that PR, but this test was still failing in this PR.

  test_server_->waitForCounterEq(
      "http.config_test.credential_injector.oauth2.token_fetch_failed_on_cluster_not_found", 0,
      std::chrono::milliseconds(10));

This check was failing immediately, and I think this is risky to assume that the initial request should not fail within 20 milliseconds, because the cluster manager returns nullptr immediately for a cluster, which does not exists.

So I changed the logic of this test to wait at most 3 seconds for 2 retried requests. My understanding of this test is as follows: a filter is initialized during X milliseconds, and we have to observe a failure before the first retry, which should happen within 1 second, so it is 1s + X ms. We want to observe a retry, so we must wait for the second request, but it must happen before the third request (second retry), which means it must happen not later than 3 s + X ms.

I know it's complicated, and I guess you don't want to debug this problem anymore. What do you think about merging it as is and see in the ongoing weeks if that will become flaky? If it will start failing again, I will take care of debugging and if I will not fix the problem, we will revert this change before the release?

jewertow · 2025-02-13T12:55:33Z

I will also mention here that this PR may cause flaky results, so I will be responsible for fixing issues.

jewertow · 2025-02-13T15:02:18Z

@mattklein123 I think this is ready for review.

vikaschoudhary16 · 2025-02-13T23:24:30Z

make sense @jewertow

jewertow · 2025-02-14T11:33:51Z

I could also investigate enabling SimulatedTimeSystem in HttpProtocolIntegrationTest to fix this problem in the long term.

- Update the ENVOY_COMMIT and ENVOY_SHA in bazel/repositories.bzl to the latest Envoy's commit. - Update .bazelrc with changes from envoyproxy/envoy#38477 - Update source/client/process_impl.cc with API changes to envoyproxy/envoy@a1cdaed - Update tools/code_format/config.yaml to envoyproxy/envoy#38460 and envoyproxy/envoy#38398 Signed-off-by: Tom Zhang <[email protected]>

@yanavlasov

fix: envoyproxy/gateway#5496 (comment) We should use the initManager in the DualInfo because the Credential Injector can be used for both HCM filter and upstream filter. Using the initManger from the ServerFactoryContext for HCM filter causes the secret to be added to the server initManager when it's already in the initialized state. Change log should not be required as this fixes a bug introduced in [a RP](#38398) that just merged after v1.33.0 . @yanavlasov --------- Signed-off-by: Huabing (Robin) Zhao <[email protected]>

@yanavlasov

fix: envoyproxy/gateway#5496 (comment) We should use the initManager in the DualInfo because the Credential Injector can be used for both HCM filter and upstream filter. Using the initManger from the ServerFactoryContext for HCM filter causes the secret to be added to the server initManager when it's already in the initialized state. Change log should not be required as this fixes a bug introduced in [a RP](envoyproxy#38398) that just merged after v1.33.0 . @yanavlasov --------- Signed-off-by: Huabing (Robin) Zhao <[email protected]>

…actoryContext (envoyproxy#38399) Commit Message: factory context: extend ServerFactoryContext with getTransportSocketFactoryContext Additional Description: This change is necessary to allow upstream filters reading secrets from SDS. Risk Level: low Testing: - Docs Changes: Not needed as it is only an internal change. Release Notes: Not needed as it is only an internal change. Platform Specific Features: - This change is necessary to unblock: 1. injecting credentials to upstream requests envoyproxy#38398 2. reading image pull secret by upstream wasm filters envoyproxy#37635 --------- Signed-off-by: Jacek Ewertowski <[email protected]>

@alyssawilk

Commit Message: http: make credential injector filter a dual filter Additional Description: This change allows users to inject credentials to HTTP CONNECT requests sent to the upstream tunnel proxy. Credentials can come from secrets (static and sds) for example for Basic auth, or from authorization servers in case of OAuth2. Risk Level: Testing: Integration test was implemented. Docs Changes: TODO Release Notes: done Fixes envoyproxy#13809 This approach to injecting Proxy-Authorization was suggested by @alyssawilk [here](envoyproxy#13809 (comment)) This PR must be merged first: envoyproxy#38399. [Here](https://github.com/jewertow/envoy-playground/blob/master/inject-proxy-authorization/envoy.filters.http.upstream.credential_injector/envoy.yaml) you can see example configuration for credential injector upstream filter. --------- Signed-off-by: Jacek Ewertowski <[email protected]>

@yanavlasov

fix: envoyproxy/gateway#5496 (comment) We should use the initManager in the DualInfo because the Credential Injector can be used for both HCM filter and upstream filter. Using the initManger from the ServerFactoryContext for HCM filter causes the secret to be added to the server initManager when it's already in the initialized state. Change log should not be required as this fixes a bug introduced in [a RP](envoyproxy#38398) that just merged after v1.33.0 . @yanavlasov --------- Signed-off-by: Huabing (Robin) Zhao <[email protected]>

jewertow requested review from kyessenov and wbpcode as code owners February 11, 2025 10:04

This was referenced Feb 11, 2025

factory context: extend ServerFactoryContext with getTransportSocketFactoryContext #38399

Merged

tcp_proxy: add support for injecting credentials to upsteam requests #38297

Closed

jewertow force-pushed the credential-injector-upstream-filter branch from 92bea00 to 17d1568 Compare February 11, 2025 10:27

mattklein123 self-assigned this Feb 12, 2025

mattklein123 added the waiting label Feb 12, 2025

jewertow added 2 commits February 12, 2025 18:54

http: make credential injector filter a dual filter

4a709f3

Signed-off-by: Jacek Ewertowski <[email protected]>

Add integration test for injecting credential to upstream tunnel proxy

822d32e

Signed-off-by: Jacek Ewertowski <[email protected]>

jewertow force-pushed the credential-injector-upstream-filter branch from 17d1568 to 3172ce6 Compare February 12, 2025 20:12

repokitteh-read-only bot removed the waiting label Feb 12, 2025

jewertow added 4 commits February 12, 2025 22:49

Add a release note

a2310f2

Signed-off-by: Jacek Ewertowski <[email protected]>

Add missing status_upstream

6e7f9c4

Signed-off-by: Jacek Ewertowski <[email protected]>

Add credential injector to visibility_excludes

c29ac9e

Signed-off-by: Jacek Ewertowski <[email protected]>

Fix unit tests

2f43d09

Signed-off-by: Jacek Ewertowski <[email protected]>

jewertow force-pushed the credential-injector-upstream-filter branch from 3172ce6 to 2f43d09 Compare February 12, 2025 21:51

Fix flaky test

f6b3ddf

Signed-off-by: Jacek Ewertowski <[email protected]>

jewertow added 2 commits February 13, 2025 12:43

Try to fix flaky test

d6ab8f6

Signed-off-by: Jacek Ewertowski <[email protected]>

Try to fix flaky test

f967a4b

Signed-off-by: Jacek Ewertowski <[email protected]>

mattklein123 approved these changes Feb 15, 2025

View reviewed changes

mattklein123 merged commit 7269582 into envoyproxy:main Feb 15, 2025
24 checks passed

jewertow mentioned this pull request Feb 17, 2025

Support dynamic host rewrite and Proxy-Authorization Header injection via SDS in TCPProxy tunneling #13809

Closed

jewertow mentioned this pull request Feb 19, 2025

flake in IpVersionsAndGrpcTypes/CredentialInjectorIntegrationTest.RetryOnClusterNotFound/0 #38263

Closed

tomjzzhang mentioned this pull request Feb 24, 2025

Update Envoy to 6c11eac (Feb 24, 2025) envoyproxy/nighthawk#1304

Merged

This was referenced Mar 17, 2025

feat: impl credential injection envoyproxy/gateway#5496

Merged

fix: credential injector init manager #38798

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: make credential injector filter a dual filter#38398

http: make credential injector filter a dual filter#38398
mattklein123 merged 9 commits intoenvoyproxy:mainfrom
jewertow:credential-injector-upstream-filter

jewertow commented Feb 11, 2025 •

edited

Loading

Uh oh!

jewertow commented Feb 13, 2025 •

edited

Loading

Uh oh!

vikaschoudhary16 commented Feb 13, 2025 •

edited

Loading

Uh oh!

jewertow commented Feb 13, 2025

Uh oh!

jewertow commented Feb 13, 2025

Uh oh!

jewertow commented Feb 13, 2025

Uh oh!

vikaschoudhary16 commented Feb 13, 2025

Uh oh!

jewertow commented Feb 14, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

jewertow commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jewertow commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vikaschoudhary16 commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jewertow commented Feb 13, 2025

Uh oh!

jewertow commented Feb 13, 2025

Uh oh!

jewertow commented Feb 13, 2025

Uh oh!

vikaschoudhary16 commented Feb 13, 2025

Uh oh!

jewertow commented Feb 14, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jewertow commented Feb 11, 2025 •

edited

Loading

jewertow commented Feb 13, 2025 •

edited

Loading

vikaschoudhary16 commented Feb 13, 2025 •

edited

Loading