Skip to content

feat: impl credential injection#5496

Merged
zirain merged 13 commits intoenvoyproxy:mainfrom
zhaohuabing:impl-4757
Apr 14, 2025
Merged

feat: impl credential injection#5496
zirain merged 13 commits intoenvoyproxy:mainfrom
zhaohuabing:impl-4757

Conversation

@zhaohuabing
Copy link
Copy Markdown
Member

@zhaohuabing zhaohuabing commented Mar 14, 2025

This PR implements the credential injection HTTPRoute filter, which allows adding credentials from a k8s secret into the Authorization header. It can be useful for egress scenarios, where authentication is required when talking to a backend service.

Implements #4757

Release Notes: Yes

@zhaohuabing zhaohuabing requested a review from a team as a code owner March 14, 2025 00:28
@zhaohuabing zhaohuabing marked this pull request as draft March 14, 2025 00:28
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 14, 2025

Codecov Report

Attention: Patch coverage is 50.99010% with 99 lines in your changes missing coverage. Please review.

Project coverage is 65.21%. Comparing base (0e49fa6) to head (aa0ee4b).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
internal/xds/translator/credentialInjector.go 72.44% 18 Missing and 9 partials ⚠️
internal/provider/kubernetes/filters.go 4.00% 23 Missing and 1 partial ⚠️
internal/provider/kubernetes/predicates.go 0.00% 15 Missing and 1 partial ⚠️
internal/provider/kubernetes/indexers.go 6.25% 14 Missing and 1 partial ⚠️
internal/ir/xds.go 0.00% 8 Missing and 1 partial ⚠️
internal/gatewayapi/filters.go 72.41% 7 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (50.99%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5496      +/-   ##
==========================================
- Coverage   65.34%   65.21%   -0.13%     
==========================================
  Files         213      214       +1     
  Lines       34121    34321     +200     
==========================================
+ Hits        22295    22382      +87     
- Misses      10489    10589     +100     
- Partials     1337     1350      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@zhaohuabing zhaohuabing force-pushed the impl-4757 branch 5 times, most recently from 65b7fbb to 7c959b8 Compare March 14, 2025 07:52
@zhaohuabing zhaohuabing marked this pull request as ready for review March 18, 2025 02:43
@zhaohuabing
Copy link
Copy Markdown
Member Author

zhaohuabing commented Mar 18, 2025

This PR is ready for review and can be merged once this Envoy credential injector issue is resolved.

@zhaohuabing
Copy link
Copy Markdown
Member Author

@arkodg This one is ready.

jewertow pushed a commit to jewertow/envoy that referenced this pull request Apr 2, 2025
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]>
@zhaohuabing zhaohuabing requested review from a team, arkodg and zirain April 5, 2025 18:54
agrawroh pushed a commit to agrawroh/envoy that referenced this pull request Apr 9, 2025
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]>
zhaohuabing and others added 10 commits April 10, 2025 03:21
Signed-off-by: Huabing (Robin) Zhao <[email protected]>
Signed-off-by: Huabing (Robin) Zhao <[email protected]>
Signed-off-by: Huabing (Robin) Zhao <[email protected]>
Signed-off-by: Huabing (Robin) Zhao <[email protected]>
Signed-off-by: Huabing (Robin) Zhao <[email protected]>
Signed-off-by: Huabing (Robin) Zhao <[email protected]>
Signed-off-by: Huabing (Robin) Zhao <[email protected]>
Co-authored-by: Arko Dasgupta <[email protected]>
Signed-off-by: Huabing (Robin) Zhao <[email protected]>
Signed-off-by: Huabing (Robin) Zhao <[email protected]>
arkodg
arkodg previously approved these changes Apr 10, 2025
Copy link
Copy Markdown
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks !

Co-authored-by: Arko Dasgupta <[email protected]>
Signed-off-by: Huabing (Robin) Zhao <[email protected]>
@arkodg arkodg requested review from a team April 13, 2025 23:41
@zirain zirain merged commit 40fc25d into envoyproxy:main Apr 14, 2025
27 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants