Skip to content

docs: clarify ExtensionTLS type#5154

Merged
arkodg merged 2 commits intoenvoyproxy:mainfrom
nareddyt:patch-1
Mar 11, 2025
Merged

docs: clarify ExtensionTLS type#5154
arkodg merged 2 commits intoenvoyproxy:mainfrom
nareddyt:patch-1

Conversation

@nareddyt
Copy link
Contributor

@nareddyt nareddyt commented Jan 26, 2025

Current document says to mount a secret that has a TLS private key. This is incorrect - Envoy Gateway acting as a client should not receive any private key.

Envoy Gateway doesn't support mTLS when connecting to extension server, so there is no need for private key today. This is verified by reading the code. EG is only looking for tls.crt

certRef := ext.Service.TLS.CertificateRef
secret, secretNamespace, err := kubernetes.ValidateSecretObjectReference(ctx, client, &certRef, namespace)
if err != nil {
return nil, err
}
cp, err := parseCA(secret)
if err != nil {
return nil, fmt.Errorf("error parsing cert in Secret %s in namespace %s", string(certRef.Name), secretNamespace)
}
creds = credentials.NewClientTLSFromCert(cp, "")

Release Notes: No

@nareddyt nareddyt requested a review from a team as a code owner January 26, 2025 17:23
@guydc
Copy link
Contributor

guydc commented Jan 26, 2025

The implementation right now is a bit confusing. tls.crt should usually be a certificate to be used by the process as either a client or a server certificate, not as a trusted CA. ca.crt, also as seen in the referenced docs, is more appropriate here.

I propose that we adopt something similar to GW-API caCertificateRefs here to avoid the confusion.

Regarding the change at hand: I would avoid linking cert-manager here, and keep it more concise, like:

CertificateRef is a reference to a Kubernetes Secret with a CA certificate in a key named tls.crt. The CA certificate is used by Envoy Gateway the verify the server certificate presented by the extension server. At this time, Envoy Gateway does not support Client Certificate authentication of Envoy Gateway towards the extension server"

@nareddyt
Copy link
Contributor Author

Thanks for the quick review @guydc ! Overall agree with what you said. Updated docs to match current state

guydc
guydc previously approved these changes Jan 30, 2025
@codecov
Copy link

codecov bot commented Jan 30, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.31%. Comparing base (8b74d01) to head (9dbb757).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5154      +/-   ##
==========================================
+ Coverage   65.30%   65.31%   +0.01%     
==========================================
  Files         213      213              
  Lines       33915    33915              
==========================================
+ Hits        22147    22152       +5     
+ Misses      10438    10433       -5     
  Partials     1330     1330              

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

@zirain
Copy link
Member

zirain commented Feb 2, 2025

@nareddyt can you run make -k gen-check to make CI happy?

@nareddyt
Copy link
Contributor Author

Sorry for the delay @zirain . Can I get another +1 @guydc ?

@nareddyt nareddyt requested a review from guydc February 13, 2025 00:12
zirain
zirain previously approved these changes Feb 13, 2025
guydc
guydc previously approved these changes Feb 14, 2025
@guydc
Copy link
Contributor

guydc commented Feb 14, 2025

/retest

@nareddyt
Copy link
Contributor Author

nareddyt commented Feb 14, 2025

Error: api/v1alpha1/envoygateway_types.go:565:1: File is not properly formatted (gci)
	// CertificateRef is a reference to a Kubernetes Secret with a CA certificate in a key named `tls.crt`. 
^

tabs vs spaces? apparently the backtick characters are not allowed, wish the error message was nicer

@zirain
Copy link
Member

zirain commented Feb 14, 2025

api/v1alpha1/envoygateway_types.go:565:1: File is not properly formatted (gci)

Signed-off-by: Teju Nareddy <tejunareddy@gmail.com>
@nareddyt nareddyt dismissed stale reviews from zirain and guydc via 225048a March 10, 2025 03:15
@nareddyt nareddyt requested review from guydc and zirain March 10, 2025 03:16
@nareddyt
Copy link
Contributor Author

Hey all, sorry for the delay. Forgot this was not merged in... Fixed CI lint issue

@nareddyt
Copy link
Contributor Author

/retest

@nareddyt
Copy link
Contributor Author

/retest

1 similar comment
@nareddyt
Copy link
Contributor Author

/retest

@nareddyt
Copy link
Contributor Author

I think the test failure is unrelated

@arkodg arkodg merged commit 3cfe4c2 into envoyproxy:main Mar 11, 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.

4 participants