feat(extension manager): dynamically reload CA certificates by sapirpol · Pull Request #5613 · envoyproxy/gateway

sapirpol · 2025-03-25T20:36:46Z

What type of PR is this?
Dynamically reload CA certificates for GRPC connections on extension-manager.

What this PR does / why we need it:
This PR introduces dynamic reloading of CA certificates used by Envoy Gateway's extension manager when establishing GRPC connections. Previously, certificates were loaded once, making updates difficult without restarting the service. To resolve this, the PR uses grpc-go's advancedtls library, enabling dynamic certificate management via a custom callback that reloads the certificate.

Comparison of TLS Settings: Before vs. After Update

Setting	Before (Old Approach)	After (New Approach)
Min TLS Version	TLS 1.2	TLS 1.2
Max TLS Version	Highest available	Highest available
Ciphers	Used safe default cipher list with explicitly forbidden RFC 7540 ciphers grpc-go.	Uses safe default cipher list grpc-go. RSA key exchange, 3DES, and CBC cipher suites removed in Go 1.22 and Go 1.23.
Dynamic Certificate Loading	No	Yes (Using GetRootCertificates)

This change was manually tested.

Which issue(s) this PR fixes:

Fixes #5396

Release Notes: Yes

Signed-off-by: Sapir Pol <sapir.pol@sap.com>

guydc · 2025-03-26T14:36:17Z

cc @nareddyt

codecov · 2025-03-26T14:46:42Z

Codecov Report

Attention: Patch coverage is 60.00000% with 14 lines in your changes missing coverage. Please review.

Project coverage is 65.23%. Comparing base (096cb8d) to head (f9a3f16).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/extension/registry/extension_manager.go	60.00%	10 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5613      +/-   ##
==========================================
+ Coverage   65.19%   65.23%   +0.03%     
==========================================
  Files         214      214              
  Lines       34321    34339      +18     
==========================================
+ Hits        22377    22401      +24     
+ Misses      10591    10578      -13     
- Partials     1353     1360       +7

☔ 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.

guydc

LGTM

Signed-off-by: Sapir Pol <sapir.pol@sap.com>

Signed-off-by: Sapir Pol <30637290+sapirpol@users.noreply.github.com>

sapirpol · 2025-04-02T08:59:58Z

@nareddyt Could you please review?

nareddyt

Thanks for making this PR! A few minor comments on the production code. Though the test may need another look.

nareddyt · 2025-04-03T02:47:45Z

internal/extension/registry/extension_manager.go

+
+	caCertPEMBytes, ok := secret.Data[corev1.TLSCertKey]
+	if !ok {
+		return nil, errors.New("no TLS certificate found in CA secret")


Suggested change

return nil, errors.New("no TLS certificate found in CA secret")

return nil, fmt.Errorf("no CA certificate found in Kubernetes Secret %s in namespace %s", secret.GetName(), secret.GetNamespace())

This is parsing a CA certificate, not TLS certificate. The pre-existing naming of key corev1.TLSCertKey is incorrect unfortunately, but we need to support it for backwards compatibility.

Let's include secret name and namespace for debuggability.

I fixed the error message

nareddyt · 2025-04-03T02:56:05Z

internal/extension/registry/extension_manager.go

+
+func createGetRootCertificatesHandler(ctx context.Context, client k8scli.Client, ext *egv1a1.ExtensionManager, namespace string) func(*advancedtls.ConnectionInfo) (*advancedtls.RootCertificates, error) {
+	return func(params *advancedtls.ConnectionInfo) (*advancedtls.RootCertificates, error) {
+		cp, err := getCertPoolFromSecret(ctx, client, ext, namespace)


This function will run in the background in a goroutine inside advancedtls, correct?

If so, I suggest you don't pass in the parent context to this function, as someone may change the parent context to cancel or timeout in the future.

Instead, create a new context in this function ctx := context.Background()

nareddyt · 2025-04-03T03:01:49Z

internal/extension/registry/extension_manager_test.go

+		},
+		Type: corev1.SecretTypeTLS,
+		Data: map[string][]byte{
+			corev1.TLSCertKey:       certData,


Unfortunately this doesn't seem right. extension_manager.go line 316 is reading CA cert from corev1.TLSCertKey (yes it is incorrectly named, but that is a legacy behavior we need to support).

If so, I'm not entirely sure how this test is working (?) Because the CA cert is not in the correct place.

I checked the test, and it worked because both the CA and the server certificate were accepted — see: https://stackoverflow.com/questions/77084841/server-certificate-used-as-ca-cert-why-does-it-work/77084918#77084918

I've now updated the test to use a self-signed certificate, so the certificate acts as both the server cert and the CA.

Wow, did not know that. Thanks for the explanation!

I recommend modifying the test to only pass in the exact necessary data. This makes it clearer to readers that the current naming is incorrect.

Specifically, change the secret to just this, the test should still pass:

Data: map[string][]byte{ corev1.TLSCertKey: caCert, },

Otherwise the PR looks good.

Thanks! I updated the secret to include only the necessary data

Signed-off-by: Sapir Pol <sapir.pol@sap.com>

sapirpol · 2025-04-06T12:27:51Z

Thanks for making this PR! A few minor comments on the production code. Though the test may need another look.

Thanks! I’ve made the changes and replied back.

Signed-off-by: Sapir Pol <sapir.pol@sap.com>

sapirpol · 2025-04-10T07:13:59Z

@nareddyt Could you please re-review the PR and trigger the tests?

arkodg · 2025-04-12T13:54:34Z

Hey @sapirpol can you rebase the go.mod file

Signed-off-by: Sapir Pol <30637290+sapirpol@users.noreply.github.com>

sapirpol · 2025-04-16T08:36:19Z

Hi @arkodg, I rebased the branch. Could you please review the PR and retrigger the tests?

sapirpol added 2 commits March 24, 2025 16:51

fix: support CA certificate reload on extension manager

07975be

Signed-off-by: Sapir Pol <sapir.pol@sap.com>

add method

3347ed2

Signed-off-by: Sapir Pol <sapir.pol@sap.com>

sapirpol requested a review from a team as a code owner March 25, 2025 20:36

sapirpol and others added 3 commits March 25, 2025 22:36

Merge branch 'main' into certificate-reload

12922a8

fix comments

8f508b8

Signed-off-by: Sapir Pol <sapir.pol@sap.com>

Merge branch 'main' into certificate-reload

58d3ecc

guydc previously approved these changes Mar 26, 2025

View reviewed changes

add tests

064558c

Signed-off-by: Sapir Pol <sapir.pol@sap.com>

sapirpol dismissed guydc’s stale review via 064558c April 1, 2025 07:18

Merge branch 'main' into certificate-reload

ff0251b

Signed-off-by: Sapir Pol <30637290+sapirpol@users.noreply.github.com>

sapirpol requested a review from guydc April 2, 2025 08:56

Merge branch 'main' into certificate-reload

db82410

nareddyt reviewed Apr 3, 2025

View reviewed changes

sapirpol and others added 2 commits April 6, 2025 11:32

Merge branch 'main' into certificate-reload

2832915

cr fixes

2437573

Signed-off-by: Sapir Pol <sapir.pol@sap.com>

sapirpol requested a review from nareddyt April 6, 2025 12:28

sapirpol and others added 4 commits April 7, 2025 13:13

modify secret data

67658eb

Signed-off-by: Sapir Pol <sapir.pol@sap.com>

fix lint errors

5ff1c6d

Signed-off-by: Sapir Pol <sapir.pol@sap.com>

Merge branch 'main' into certificate-reload

c6baa82

Merge branch 'main' into certificate-reload

26bc142

guydc previously approved these changes Apr 10, 2025

View reviewed changes

nareddyt approved these changes Apr 12, 2025

View reviewed changes

Merge branch 'main' into certificate-reload

63bd5a4

Signed-off-by: Sapir Pol <30637290+sapirpol@users.noreply.github.com>

sapirpol dismissed guydc’s stale review via 63bd5a4 April 14, 2025 06:48

sapirpol added 2 commits April 15, 2025 15:44

Merge branch 'main' into certificate-reload

f006b75

Merge branch 'main' into certificate-reload

29d1997

Merge branch 'main' into certificate-reload

f9a3f16

arkodg approved these changes Apr 16, 2025

View reviewed changes

arkodg merged commit a4eeddc into envoyproxy:main Apr 16, 2025
24 of 25 checks passed

BrewTestBot mentioned this pull request May 14, 2025

egctl 1.4.0 Homebrew/homebrew-core#223391

Merged

	return nil, errors.New("no TLS certificate found in CA secret")
	return nil, fmt.Errorf("no CA certificate found in Kubernetes Secret %s in namespace %s", secret.GetName(), secret.GetNamespace())

Conversation

sapirpol commented Mar 25, 2025

Uh oh!

guydc commented Mar 26, 2025

Uh oh!

codecov bot commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

guydc left a comment

Choose a reason for hiding this comment

Uh oh!

sapirpol commented Apr 2, 2025

Uh oh!

nareddyt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sapirpol Apr 7, 2025 • 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!

sapirpol commented Apr 6, 2025

Uh oh!

sapirpol commented Apr 10, 2025

Uh oh!

arkodg commented Apr 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sapirpol commented Apr 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Mar 26, 2025 •

edited

Loading

sapirpol Apr 7, 2025 •

edited

Loading

arkodg commented Apr 12, 2025 •

edited

Loading