feat: adding support for ext auth and backend mtls by alexwo · Pull Request #3441 · envoyproxy/gateway

alexwo · 2024-05-20T18:01:23Z

What this PR does / why we need it:
This enables the use of a shared client certificate with Envoy proxies when accessing external services or backends.

Approach:
We introduce the capability to associate a TLS client certificate with the global configuration of an Envoy proxy. When configured, this client certificate will be used by the proxies when connecting to specified backends or external services.

Which issue(s) this PR fixes:
#2536

codecov · 2024-05-20T18:06:11Z

Codecov Report

Attention: Patch coverage is 67.36842% with 31 lines in your changes are missing coverage. Please review.

Project coverage is 67.18%. Comparing base (2880a55) to head (5edafcd).

Files	Patch %	Lines
internal/xds/translator/translator.go	15.00%	14 Missing and 3 partials ⚠️
internal/provider/kubernetes/controller.go	23.52%	13 Missing ⚠️
internal/gatewayapi/backendtlspolicy.go	97.91%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3441      +/-   ##
==========================================
+ Coverage   67.15%   67.18%   +0.02%     
==========================================
  Files         166      166              
  Lines       19463    19544      +81     
==========================================
+ Hits        13071    13131      +60     
- Misses       5447     5469      +22     
+ Partials      945      944       -1

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

alexwo · 2024-05-20T18:38:43Z

@alexwo Thanks for working on this!

Could you please add e2e tests to this PR directly? This is not a very large PR, incorporating these tests would not be a burden for reviewers.

Hi @zhaohuabing ,

Sure, I have added the e2e tests as part of this PR.

alexwo · 2024-05-21T08:34:35Z

/retest

alexwo · 2024-05-21T09:22:23Z

/retest

alexwo · 2024-05-21T10:19:32Z

/retest

zhaohuabing · 2024-05-21T16:55:49Z

internal/gatewayapi/backendtlspolicy.go

According to the API comment, should the namespace of the client secret be the same as the EnvoyProxy resource?

This secret should be located within the same namespace as the Envoy proxy resource that references it.

Suggested change

ns := ptr.Deref(resources.EnvoyProxy.Spec.BackendTLS.ClientCertificateRef.Namespace, "")

ns := ptr.Deref(resources.EnvoyProxy.Spec.BackendTLS.ClientCertificateRef.Namespace, "")

Yes, was actually unsure if we should restrict this, perhaps the we should just allow any namespace. For now i have removed this comment. let me know incase you think thats interesting.

IMO, It makes sense to create the secret in the same namespace because it's only used by the EnvoyProxy resource, and the other EG API does the similar.

Great, I had similar thoughts. I have just added it back, with a restriction.

internal/gatewayapi/backendtlspolicy.go

internal/provider/kubernetes/controller.go

alexwo · 2024-05-22T13:26:58Z

/retest

alexwo · 2024-05-22T14:33:01Z

/retest

alexwo · 2024-05-22T15:04:00Z

/retest

alexwo · 2024-05-22T15:38:50Z

/retest

alexwo · 2024-05-22T16:15:50Z

/retest

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

alexwo · 2024-05-23T07:19:46Z

/retest

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

alexwo · 2024-05-23T09:25:40Z

/retest

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

arkodg

LGTM thanks !

guydc

Thanks, this looks great overall, some comments around testing mostly. Can be fixed in a follow-up PR.

guydc · 2024-05-23T21:27:16Z

internal/provider/kubernetes/controller.go

 			}
 		}

+		// process envoy gateway secret refs


Should we also update predicates.go, indexers.go to make sure that a change in the secret triggers a reconcile and lookup is efficient?

guydc · 2024-05-23T21:28:16Z

test/e2e/tests/uptream_tls_settings.go

+	ConformanceTests = append(ConformanceTests, UpstreamTLSSettingsTest)
+}
+
+var UpstreamTLSSettingsTest = suite.ConformanceTest{


nit: maybe use Backend instead of Upstream for all names here??

guydc · 2024-05-23T21:31:22Z

test/e2e/testdata/upstream-tls.yaml

+      port: 443
+      targetPort: 8443
+---
+apiVersion: apps/v1


maybe use a different name for the deployment, secrets, etc. to make sure that this doesn't conflict with other tests from conformance/e2e suites

guydc · 2024-05-23T21:34:35Z

test/e2e/base/manifests.yaml

+      certificateRefs:
+      - group: ""
+        kind: Secret
+        name: backend-tls-certificate


Do we really need this? We can have HTTP => HTTPS upgrade in backend TLS tests..

guydc · 2024-05-23T21:38:37Z

test/e2e/tests/uptream_tls_settings.go

+				MaxVersion: ptr.To(v1alpha1.TLSv12),
+				Ciphers:    []string{"TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384"},
+			}
+			err = UpdateProxyConfig(suite.Client, proxyNN, config)


Can we maybe create multiple EP resources as part of the test data and just change the reference from the GWC?

guydc · 2024-05-23T21:45:26Z

test/e2e/tests/uptream_tls_settings.go

+				t.Error(err)
+			}
+			expectedRes.TLS.Version = "TLSv1.2"
+			expectedRes.TLS.CipherSuite = "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384"


maybe another yaml here for the expected result?

guydc · 2024-05-23T21:53:56Z

test/e2e/tests/uptream_tls_settings.go

+	TLS       TLSInfo             `json:"tls"`
+}
+
+type TLSInfo struct {


Can we somehow reuse the gateway-api test util structs by extending them here?

guydc · 2024-05-23T21:55:51Z

test/e2e/tests/uptream_tls_settings.go

+	return hasAllFieldsAndValuesRecursive(reflect.ValueOf(obj1), reflect.ValueOf(obj2))
+}
+
+func hasAllFieldsAndValuesRecursive(v1, v2 reflect.Value) (bool, error) {


can we use go-cmp here to make these comparisons?

guydc · 2024-05-23T21:57:37Z

internal/provider/kubernetes/controller.go

+			gatewayapi.KindEnvoyProxy,
+			*certRef); err != nil {
+			r.log.Error(err,
+				"failed to process TLS SecretRef for gateway",


for GatewayClass

guydc · 2024-05-23T22:52:42Z

internal/gatewayapi/backendtlspolicy.go

+	return tlsBundle, policy
+}
+
+func (t *Translator) applyEnvoyProxyBackendTLSSetting(policy *gwapiv1a3.BackendTLSPolicy, tlsBundle *ir.TLSUpstreamConfig, resources *Resources, parent gwapiv1a2.ParentReference) *ir.TLSUpstreamConfig {


nit: maybe rename tlsBundle to tlsConfig here? It used to be just a cert bundle, but now contains much more settings,

guydc · 2024-05-24T00:10:51Z

@alexwo - can you maybe make some of the suggested test improvements in a follow-up PR?

alexwo · 2024-05-24T05:25:52Z

@alexwo - can you maybe make some of the suggested test improvements in a follow-up PR?

yes sounds good! , thanks for the quick review & feedback.

alexwo requested a review from a team as a code owner May 20, 2024 18:01

zhaohuabing reviewed May 21, 2024

View reviewed changes

internal/provider/kubernetes/controller.go Outdated Show resolved Hide resolved

alexwo added 3 commits May 23, 2024 08:26

upstream mtls for eg

0259628

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

adding tests

84ad8a9

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

update gens

a919c19

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

fixing test

4039143

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

alexwo added 2 commits May 23, 2024 12:13

updating

cc87d65

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

fix lint

5edafcd

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

arkodg approved these changes May 23, 2024

View reviewed changes

arkodg requested review from a team and zhaohuabing May 23, 2024 22:57

guydc reviewed May 23, 2024

View reviewed changes

guydc approved these changes May 23, 2024

View reviewed changes

guydc merged commit e3439f6 into envoyproxy:main May 24, 2024

guydc mentioned this pull request May 24, 2024

chore: fix ci #3465

Merged

shawnh2 mentioned this pull request May 24, 2024

Envoy Client Certs for Ext Auth and Backend TLS #2536

Closed

alexwo deleted the upstream_mtls branch May 24, 2024 09:41

arkodg mentioned this pull request Jul 8, 2024

docs: Backend mTLS (Gateway->Backend) #3789

Closed

	ns := ptr.Deref(resources.EnvoyProxy.Spec.BackendTLS.ClientCertificateRef.Namespace, "")
	ns := ptr.Deref(resources.EnvoyProxy.Spec.BackendTLS.ClientCertificateRef.Namespace, "")

Conversation

alexwo commented May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alexwo commented May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexwo commented May 21, 2024

Uh oh!

alexwo commented May 21, 2024

Uh oh!

alexwo commented May 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexwo May 22, 2024 • 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!

zhaohuabing May 22, 2024 • 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!

alexwo May 23, 2024 • 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!

Uh oh!

Uh oh!

alexwo commented May 22, 2024

Uh oh!

alexwo commented May 22, 2024

Uh oh!

alexwo commented May 22, 2024

Uh oh!

alexwo commented May 22, 2024

Uh oh!

alexwo commented May 22, 2024

Uh oh!

alexwo commented May 23, 2024

Uh oh!

alexwo commented May 23, 2024

Uh oh!

arkodg left a comment

Choose a reason for hiding this comment

Uh oh!

guydc left a comment • 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!

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!

Choose a reason for hiding this comment

Uh oh!

guydc commented May 24, 2024

Uh oh!

alexwo commented May 24, 2024

Uh oh!

Reviewers

Assignees

alexwo commented May 20, 2024 •

edited

Loading

codecov bot commented May 20, 2024 •

edited

Loading

alexwo commented May 20, 2024 •

edited

Loading

alexwo May 22, 2024 •

edited

Loading

zhaohuabing May 22, 2024 •

edited

Loading

alexwo May 23, 2024 •

edited

Loading

guydc left a comment •

edited

Loading