Support gateway api BackendTLSPolicy #6119

flawedmatrix · 2024-01-24T01:04:56Z

The BackendTLSPolicy will allow a user to connect an httproute to a backend service with TLS.

Implements #5929

Only implements BackendTLSPolicy for HTTPRoute
Only allows Services for spec.targetRef
Allows ConfigMap or Secret for spec.TLS.CACertRefs
Adds backendtlspolicies to the ClusterRole for Contour
Adds configmaps to the ClusterRole for Contour
Controller reconciles on BackendTLSPolicy and ConfigMaps now
BackendTLSPolicy spec.targetRef can specify SectionName to be port name of a service to target a particular section of the service.

In the interest of keeping this size of this change relatively small, we opted not to implement all of the GEP (https://gateway-api.sigs.k8s.io/geps/gep-1897/)

Specifically, we have not yet implemented BackendTLSPolicy for TLSRoute or GRPCRoute, precedence handling for BackendTLSPolicy (described here: https://gateway-api.sigs.k8s.io/geps/gep-713/#conflict-resolution), displaying status conditions on the BackendTLSPolicy, among others we may have missed.

We have also opted not to implement wellKnownCACerts since it was an implementation-specific detail of the GEP (kubernetes-sigs/gateway-api#2726) and right now support for System certs was not supported anywhere in Contour.

flawedmatrix · 2024-01-24T01:05:11Z

cc @christianang

sunjayBhatia · 2024-01-24T18:30:58Z

cmd/contour/serve.go

@@ -639,6 +639,10 @@ func (s *Server) doServe() error {
 		s.log.WithError(err).WithField("resource", "secrets").Fatal("failed to create informer")
 	}

+	if err := s.informOnResource(&corev1.ConfigMap{}, handler); err != nil {


i think we could only start this informer if the backendtlspolicy feature is enabled, could augment the features map below to be map[string][]string so if a feature is disabled/enabled we can enable/disable any other required informers/controllers

Not sure I follow the change for features to go from map[string]struct{} to map[string][]string. What would go into the new string array value type here? I see the serve.go currently deletes from the map if a feature is disabled. Is that not enough?

the map key would be feature names, which currently are only resource kinds we inform on, and then the string list could be kinds that a particular feature/top level kind depends on, in this case backendtlspolicy also needs a configmap informer to work fully, so if you disable backendtlspolicy you dont need to have a configmap informer etc.

it might be overkill, tbh since we also have an if statement checking if backendtlspolicy is enabled, we can add the creation of the configmap informer in there

Ended up moving the informOnResource line down into the feature check for backendltspolicy.

sunjayBhatia · 2024-01-24T19:01:03Z

~~An additional follow up issue will be that the Gateway Provisioner will need to set up the appropriate RBAC for Contour instances it provisions so that BackendTLSPolicy can be used~~ nvm that looks like it is done here: https://github.com/projectcontour/contour/pull/6119/files#diff-ae09cc5859e3f74916f0d940647bd28dc085139bf071dad80c5c43c4ff64b51a

sunjayBhatia · 2024-01-24T19:07:42Z

internal/dag/cache_test.go

@@ -1035,6 +1091,48 @@ func TestKubernetesCacheInsert(t *testing.T) {
 			},
 			want: true,
 		},
+		"insert backendtlspolicy referenced by gateway-api HTTPRoute": {


nit

Suggested change

"insert backendtlspolicy referenced by gateway-api HTTPRoute": {

"insert backendtlspolicy targeting backend Service": {

internal/dag/gatewayapi_processor.go

sunjayBhatia · 2024-01-24T19:15:56Z

internal/dag/cache_test.go

+			},
+			want: false,
+		},
+		"insert certificate configmap referenced by BackendTLSPolicy": {


should be able to add a case for a Secret referenced by a BackendTLSPolicy (on the remove case as well)

Done. It should be noted that the secretRetriggerBuild function always returns true on ca certs, which is what is always provided to backendtlspolicies, so no changes to the implementation was done here, but testing it is still valuable just to provide intent in case the logic here changes.

sunjayBhatia · 2024-01-24T19:36:12Z

internal/dag/cache_test.go

+			want:      backendTLSPolicy("btp2", "backend-service-with-fallback", nil, ptr.To("https")),
+			wantFound: true,
+		},
+		"finds the BackendTLSPolicy matching namespace": {


i think there should be some consideration of what namespace the backendtlspolicy resides in as well, at least as an initial pass let's not allow policies that live in a different namespace from the Service (for the purposes of this test, the targetRef target namespace) they reference

so we may need to always construct a targetRef with a namespace which in the processor this would be the namespace of the HTTPRoute if not specified on the backendref, otherwise the ns of the backendref

Yeah logic here has some hidden complexity. I did a bit of a rewrite, now matching the namespace provided on the targetref to the backendtlspolicy namespace instead, then check that the backendtlspolicy namespace matches the spec.targetRef.namespace afterward. Also added some logic around default namespaces and empty namespaces e.g if spec.targetRef.namespace is empty that should be fine since that should be used for cross namespace-ing, which is unsupported. Added additional testing here as well, but could always use another look to check if anything is missed or unclear.

i think the updated tests etc. look good, might need another eye just in case but good in my book

sunjayBhatia · 2024-01-24T19:37:15Z

internal/dag/gatewayapi_processor.go

+				Group:     backendRefGroup,
+				Kind:      backendRefKind,
+				Name:      backendRef.Name,
+				Namespace: backendRef.Namespace,


this may not be specified if no cross-ns reference is present, so likely have to make this the ns of the HTTPRoute if empty

Good catch. Done.

sunjayBhatia · 2024-01-24T19:44:12Z

Looks like this file does not include BackendTLSPolicy: https://github.com/kubernetes-sigs/gateway-api/blob/v1.0.0/config/crd/experimental/kustomization.yaml 🙃

so we never automatically picked it up in our examples:

contour/hack/generate-gateway-yaml.sh

Line 17 in 9eb2838

    
           kubectl kustomize -o examples/gateway/00-crds.yaml "github.com/kubernetes-sigs/gateway-api/config/crd/experimental?ref=${GATEWAY_API_VERSION}"

sunjayBhatia · 2024-01-24T19:50:20Z

Looks like this file does not include BackendTLSPolicy: https://github.com/kubernetes-sigs/gateway-api/blob/v1.0.0/config/crd/experimental/kustomization.yaml 🙃

so we never automatically picked it up in our examples:

contour/hack/generate-gateway-yaml.sh

Line 17 in 9eb2838

kubectl kustomize -o examples/gateway/00-crds.yaml "github.com/kubernetes-sigs/gateway-api/config/crd/experimental?ref=${GATEWAY_API_VERSION}"

we could update this instead to pull from the release artifact: https://github.com/kubernetes-sigs/gateway-api/releases/download/v1.0.0/experimental-install.yaml

christianang · 2024-01-24T22:07:41Z

Looks like this file does not include BackendTLSPolicy: https://github.com/kubernetes-sigs/gateway-api/blob/v1.0.0/config/crd/experimental/kustomization.yaml 🙃
so we never automatically picked it up in our examples:

contour/hack/generate-gateway-yaml.sh

Line 17 in 9eb2838

kubectl kustomize -o examples/gateway/00-crds.yaml "github.com/kubernetes-sigs/gateway-api/config/crd/experimental?ref=${GATEWAY_API_VERSION}"

we could update this instead to pull from the release artifact: https://github.com/kubernetes-sigs/gateway-api/releases/download/v1.0.0/experimental-install.yaml

Yeah we noticed that. That is interesting that BackendTLSPolicy is in the released artifact. We figured it might make sense to just PR the kustomization.yaml, but I'm fine with switching to the released artifact if that makes more sense.

sunjayBhatia · 2024-01-24T22:09:15Z

Looks like this file does not include BackendTLSPolicy: https://github.com/kubernetes-sigs/gateway-api/blob/v1.0.0/config/crd/experimental/kustomization.yaml 🙃
so we never automatically picked it up in our examples:

contour/hack/generate-gateway-yaml.sh

Line 17 in 9eb2838

kubectl kustomize -o examples/gateway/00-crds.yaml "github.com/kubernetes-sigs/gateway-api/config/crd/experimental?ref=${GATEWAY_API_VERSION}"

we could update this instead to pull from the release artifact: https://github.com/kubernetes-sigs/gateway-api/releases/download/v1.0.0/experimental-install.yaml

Yeah we noticed that. That is interesting that BackendTLSPolicy is in the released artifact. We figured it might make sense to just PR the kustomization.yaml, but I'm fine with switching to the released artifact if that makes more sense.

yeah we'll have to probably switch to downloading the released artifact since the old tag won't be updated

made a PR to fix this here: kubernetes-sigs/gateway-api#2736

codecov · 2024-01-25T21:53:37Z

Codecov Report

Attention: 49 lines in your changes are missing coverage. Please review.

Comparison is base (29201bb) 78.52% compared to head (95bfe88) 78.58%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6119      +/-   ##
==========================================
+ Coverage   78.52%   78.58%   +0.06%     
==========================================
  Files         140      141       +1     
  Lines       19911    20187     +276     
==========================================
+ Hits        15635    15864     +229     
- Misses       3967     4012      +45     
- Partials      309      311       +2

Files	Coverage Δ
internal/dag/dag.go	`98.74% <100.00%> (+0.04%)`	⬆️
internal/dag/gatewayapi_processor.go	`93.95% <100.00%> (+0.23%)`	⬆️
internal/dag/httpproxy_processor.go	`91.53% <100.00%> (+0.01%)`	⬆️
internal/envoy/cluster.go	`100.00% <100.00%> (ø)`
internal/featuretests/v3/envoy.go	`99.08% <100.00%> (ø)`
internal/k8s/helpers.go	`55.05% <100.00%> (+2.11%)`	⬆️
internal/provisioner/objects/rbac/util/util.go	`100.00% <100.00%> (ø)`
internal/dag/cache.go	`96.89% <98.00%> (+0.22%)`	⬆️
internal/controller/backendtlspolicy.go	`26.66% <26.66%> (ø)`
cmd/contour/serve.go	`20.26% <7.40%> (-0.54%)`	⬇️

... and 6 files with indirect coverage changes

christianang · 2024-01-25T22:34:00Z

@sunjayBhatia made some changes from your feedback and got the checks passing. I think this is ready for another review.

sunjayBhatia

Besides some v minor things I think this is basically good to go. Before merging too would be good to get some issues created for all the outstanding things for follow-ups:

Supporting BackendTLS for GRPCRoute
Supporting BackendTLS for TLSRoute
Status Condition logic for BackendTLSPolicies
Interaction between the existing Service annotation for specifying backend protocol and BackendTLSPolicy
Handling conflict resolution if there are multiple Policies targeting a Service
e2e testing upstream TLS settings w/ Gateway API Routes (to cover the Gateway processor being wired up properly in serve.go)
look at cluster name generation to see if we need to include all of the ca cert secret names in the hashed name
and any other things that we can think of

test/e2e/gateway/backend_tls_policy_test.go

sunjayBhatia · 2024-01-26T20:38:56Z

internal/envoy/cluster.go

@@ -47,8 +47,12 @@ func Clustername(cluster *dag.Cluster) string {
 		buf += hc.Path
 	}
 	if uv := cluster.UpstreamValidation; uv != nil {
-		buf += uv.CACertificate.Object.ObjectMeta.Name
-		buf += uv.SubjectNames[0]
+		if len(uv.CACertificates) > 0 {


hm i wonder if we could run into some issues here with duplicate cluster names for when the same cluster is referenced in different places that have different backend tls settings, adding as a follow up

Broke out the follow issue to follow up: #6141

sunjayBhatia · 2024-01-26T20:46:59Z

internal/dag/cache.go

+func (kc *KubernetesCache) LookupBackendTLSPolicyByTargetRef(targetRef gatewayapi_v1alpha2.PolicyTargetReferenceWithSectionName) (*gatewayapi_v1alpha2.BackendTLSPolicy, bool) {
+	var fallbackBackendTLSPolicy *gatewayapi_v1alpha2.BackendTLSPolicy
+	for _, v := range kc.backendtlspolicies {
+		// Match the namespace in the targetRef to the BackendTLSPolicy namespace instead of the it's


Suggested change

// Match the namespace in the targetRef to the BackendTLSPolicy namespace instead of the it's

// Match the namespace in the targetRef to the BackendTLSPolicy namespace instead of it's

sunjayBhatia · 2024-01-26T20:57:47Z

internal/dag/cache_test.go

+			want:      backendTLSPolicy("btp2", "some-ns", "backend-service-with-ns", nil, nil),
+			wantFound: true,
+		},
+		"finds the BackendTLSPolicy in default namespace": {


realistically i dont think this can happen since we alway set the targetRef NS to the Route ns but worth having in case

sunjayBhatia · 2024-01-26T20:59:31Z

internal/dag/cache_test.go

+			want:      backendTLSPolicy("btp2", "backend-service-with-fallback", nil, ptr.To("https")),
+			wantFound: true,
+		},
+		"finds the BackendTLSPolicy matching namespace": {


i think the updated tests etc. look good, might need another eye just in case but good in my book

internal/dag/gatewayapi_processor.go

izturn · 2024-01-29T05:57:18Z

internal/dag/builder_test.go

+			objs: []any{
+				tlsService,
+				configMapCert1,
+				&gatewayapi_v1beta1.HTTPRoute{


Shall we define some helper functions to create the objects?, this files is too large now

That makes sense. Done.

christianang · 2024-01-29T19:45:47Z

Besides some v minor things I think this is basically good to go. Before merging too would be good to get some issues created for all the outstanding things for follow-ups:

Supporting BackendTLS for GRPCRoute

Supporting BackendTLS for TLSRoute

Status Condition logic for BackendTLSPolicies

Interaction between the existing Service annotation for specifying backend protocol and BackendTLSPolicy

Handling conflict resolution if there are multiple Policies targeting a Service

e2e testing upstream TLS settings w/ Gateway API Routes (to cover the Gateway processor being wired up properly in serve.go)

look at cluster name generation to see if we need to include all of the ca cert secret names in the hashed name

and any other things that we can think of

Created follow up issues for the above. The only other follow up item I could think of is cross namespace certificate secrets with ReferenceGrants.

Happy to add more issues as more follow up items come up.

christianang · 2024-01-30T00:51:51Z

I addressed the latest round of feedback.

sunjayBhatia

LGTM, just a merge conflict to resolve

cc @projectcontour/maintainers if you want to take a look as well

skriss

Couple initial comments, still working through DAG logic

cmd/contour/serve.go

christianang · 2024-01-31T19:59:38Z

Thanks for the review Steve. I've updated the PR to address your comments.

skriss · 2024-01-31T20:11:25Z

Oops, looks like TestIsEqualFallback was using ConfigMap as its "unsupported" kind, but it's now "supported" in IsObjectEqual 😄 https://github.com/projectcontour/contour/actions/runs/7731651205/job/21079863873?pr=6119#step:6:75

The BackendTLSPolicy will allow a user to connect an httproute to a backend service with TLS. - Only implements BackendTLSPolicy for HTTPRoute - Only allows Services for spec.targetRef - Allows ConfigMap or Secret for spec.TLS.CACertRefs - Adds backendtlspolicies to the ClusterRole for Contour - Adds configmaps to the ClusterRole for Contour - Controller reconciles on BackendTLSPolicy and ConfigMaps now - BackendTLSPolicy spec.targetRef can specify SectionName to be port name of a service to target a particular section of the service. Co-authored-by: Christian Ang <christian.ang@broadcom.com> Signed-off-by: Edwin Xie <edwin.xie@broadcom.com>

- to help reduce verbosity of builder test Signed-off-by: Christian Ang <christian.ang@broadcom.com>

christianang · 2024-01-31T23:29:41Z

Ah missed running those tests locally. I updated the tests to account for the ConfigMap changes.

skriss

LGTM, will leave in case @sunjayBhatia wanted to take another look

sunjayBhatia

New changes LGTM!

sunjayBhatia added the release-note/major A major change that needs more than a paragraph of explanation in the release notes. label Jan 24, 2024

sunjayBhatia reviewed Jan 24, 2024

View reviewed changes

internal/dag/gatewayapi_processor.go Show resolved Hide resolved

sunjayBhatia reviewed Jan 24, 2024

View reviewed changes

christianang force-pushed the gateway-api-backend-tls-policy branch from e04d511 to dfbf234 Compare January 24, 2024 19:37

christianang force-pushed the gateway-api-backend-tls-policy branch from dfbf234 to 8e8626b Compare January 25, 2024 00:48

flawedmatrix marked this pull request as ready for review January 25, 2024 18:16

flawedmatrix requested a review from a team as a code owner January 25, 2024 18:16

flawedmatrix requested review from skriss and sunjayBhatia and removed request for a team January 25, 2024 18:16

sunjayBhatia requested review from a team, izturn and clayton-gonsalves and removed request for a team January 25, 2024 18:17

christianang force-pushed the gateway-api-backend-tls-policy branch 3 times, most recently from 1189d20 to 31a5bb2 Compare January 25, 2024 21:50

sunjayBhatia reviewed Jan 26, 2024

View reviewed changes

izturn reviewed Jan 29, 2024

View reviewed changes

christianang force-pushed the gateway-api-backend-tls-policy branch 2 times, most recently from cd9c056 to 2ccc498 Compare January 30, 2024 00:48

sunjayBhatia approved these changes Jan 30, 2024

View reviewed changes

christianang force-pushed the gateway-api-backend-tls-policy branch from 2ccc498 to dc66472 Compare January 31, 2024 00:36

skriss requested changes Jan 31, 2024

View reviewed changes

cmd/contour/serve.go Outdated Show resolved Hide resolved

cmd/contour/serve.go Outdated Show resolved Hide resolved

cmd/contour/serve.go Show resolved Hide resolved

cmd/contour/serve.go Show resolved Hide resolved

skriss requested changes Jan 31, 2024

View reviewed changes

cmd/contour/serve.go Show resolved Hide resolved

christianang force-pushed the gateway-api-backend-tls-policy branch from dc66472 to 39f89e0 Compare January 31, 2024 19:57

flawedmatrix and others added 2 commits January 31, 2024 23:02

Extract out common makeHTTPRoute helper

95bfe88

- to help reduce verbosity of builder test Signed-off-by: Christian Ang <christian.ang@broadcom.com>

christianang force-pushed the gateway-api-backend-tls-policy branch from 39f89e0 to 95bfe88 Compare January 31, 2024 23:02

skriss approved these changes Feb 1, 2024

View reviewed changes

sunjayBhatia approved these changes Feb 1, 2024

View reviewed changes

sunjayBhatia merged commit 943d5e2 into projectcontour:main Feb 1, 2024
26 checks passed

skriss mentioned this pull request Feb 1, 2024

Gateway API: Implement BackendTLSPolicy #5929

Closed

	"insert backendtlspolicy referenced by gateway-api HTTPRoute": {
	"insert backendtlspolicy targeting backend Service": {

	// Match the namespace in the targetRef to the BackendTLSPolicy namespace instead of the it's
	// Match the namespace in the targetRef to the BackendTLSPolicy namespace instead of it's

Support gateway api BackendTLSPolicy #6119

Support gateway api BackendTLSPolicy #6119

Conversation

flawedmatrix commented Jan 24, 2024

flawedmatrix commented Jan 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunjayBhatia commented Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunjayBhatia commented Jan 24, 2024 • edited Loading

sunjayBhatia commented Jan 24, 2024

christianang commented Jan 24, 2024

sunjayBhatia commented Jan 24, 2024

codecov bot commented Jan 25, 2024 • edited Loading

Codecov Report

christianang commented Jan 25, 2024

sunjayBhatia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianang commented Jan 29, 2024

christianang commented Jan 30, 2024

sunjayBhatia left a comment

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

christianang commented Jan 31, 2024

skriss commented Jan 31, 2024

christianang commented Jan 31, 2024

skriss left a comment

Choose a reason for hiding this comment

sunjayBhatia left a comment

Choose a reason for hiding this comment

sunjayBhatia commented Jan 24, 2024 •

edited

Loading

sunjayBhatia commented Jan 24, 2024 •

edited

Loading

codecov bot commented Jan 25, 2024 •

edited

Loading