BackendTrafficPolicy init + design doc by Alice-Lilith · Pull Request #1961 · envoyproxy/gateway

Alice-Lilith · 2023-10-13T02:41:49Z

Initializes the BackendTrafficPolicy custom resource and API.

I added some extra CRD validation to catch a couple things early instead of waiting to get to the runtime condition checks.
Here's some screenshots from when I was working on that so you can get an idea of what a user might see when trying to apply and fix an invalid resource. I'm happy to open a PR tweaking the existing ClientTrafficPolicy CRD to have similar validation if we're good with it.

Signed-off-by: Alice Wasko <alicewasko@datawire.io>

codecov · 2023-10-13T02:47:21Z

Codecov Report

Merging #1961 (5db9f43) into main (3827919) will increase coverage by 0.31%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##             main    #1961      +/-   ##
==========================================
+ Coverage   65.37%   65.69%   +0.31%     
==========================================
  Files          90       92       +2     
  Lines       13377    13743     +366     
==========================================
+ Hits         8745     9028     +283     
- Misses       4092     4166      +74     
- Partials      540      549       +9

Files	Coverage Δ
internal/gatewayapi/clienttrafficpolicy.go	`71.11% <100.00%> (ø)`
internal/gatewayapi/translator.go	`98.78% <100.00%> (+1.28%)`	⬆️
internal/message/types.go	`0.00% <ø> (ø)`
internal/status/backendtrafficpolicy.go	`0.00% <0.00%> (ø)`
internal/gatewayapi/runner/runner.go	`22.03% <0.00%> (-0.98%)`	⬇️
internal/gatewayapi/zz_generated.deepcopy.go	`0.00% <0.00%> (ø)`
internal/provider/kubernetes/controller.go	`53.13% <8.33%> (-1.00%)`	⬇️
internal/gatewayapi/resource.go	`62.00% <0.00%> (-1.27%)`	⬇️
internal/gatewayapi/backendtrafficpolicy.go	`78.96% <78.96%> (ø)`

... and 2 files with indirect coverage changes

zhaohuabing · 2023-10-13T03:11:25Z

Initializes the BackendTrafficPolicy custom resource and API.

I added some extra CRD validation to catch a couple things early instead of waiting to get to the runtime condition checks.

It's awesome! I need similar validation for SecurityPolicy as well.

zhaohuabing · 2023-10-13T04:23:00Z

internal/gatewayapi/backendtrafficpolicy.go

+			}
+
+			// Check if any policies that target xRoute resources are overriding this policy so we can set the status
+			if _, ok := overriddenPolicyTargets[*gatewayKey]; ok {


A policy targeting a xRoute does not always override a policy targeting a listener under the same gateway. For example, they may have different hostnames.

the sectionName of the parentReference is not being saved in L107 so we dont know for sure if this listener section is a parent of the xRoute which has a policy targeted to it

zhaohuabing · 2023-10-13T04:29:25Z

internal/gatewayapi/backendtrafficpolicy.go

+					message,
+				)
+			}
+


We should also check if there are existing policies at the route level.

I believe this is happening on L109

L109 overriddenPolicyTargets is only used to check conflicts with listeners at L152. Ideally, we should also check if there are existing policies at the route level that override the gateway-level(Policies targeting gateway without sectionname) policies.

arkodg · 2023-10-13T17:12:17Z

api/v1alpha1/backendtrafficpolicy_types.go

+)
+
+// +kubebuilder:object:root=true
+// +kubebuilder:resource:shortName=btpolicy


arkodg · 2023-10-13T17:18:05Z

api/v1alpha1/backendtrafficpolicy_types.go

+// spec defines the desired state of BackendTrafficPolicy.
+type BackendTrafficPolicySpec struct {
+
+	// +kubebuilder:validation:XValidation:rule="has(self.sectionName) ? self.kind == 'Gateway': true", message="sectionName can only be set when kind is 'Gateway'."


thanks for adding these !

raised #1964 to add validation tests

arkodg · 2023-10-13T17:30:39Z

api/v1alpha1/backendtrafficpolicy_types.go

+	// is being attached to.
+	// This Policy and the TargetRef MUST be in the same namespace
+	// for this Policy to have effect and be applied to the Gateway.
+	TargetRef gwapiv1a2.PolicyTargetReferenceWithSectionName `json:"targetRef"`


we should add a line here about policy hierarchy as well as policy override i.e.

Only one policy can be successfully attached to a specific target

Since multiple policies can be indirectly applied to the same target e.g. a policy targeting a Gateway as well as a policy targeting a xRoute attached to the same Gateway, the policy with the most specific target will always have higher precedence (in this case the policy targeting the xRoute) over the less specific one (the policy targeting the Gateway)

arkodg · 2023-10-13T17:35:34Z

internal/gatewayapi/backendtrafficpolicy.go

+				if parentRef.Namespace != nil {
+					namespace = string(*parentRef.Namespace)
+				}
+				overriddenPolicyTargets[types.NamespacedName{


can you explain this logic ?

nvm, looks like we are proactively making sure the parent gateways are overridden, a comment here would be great

arkodg · 2023-10-13T17:37:12Z

internal/gatewayapi/backendtrafficpolicy.go

+
+	// Process the policies targeting Gateways with a section name
+	for _, policy := range backendTrafficPolicies {
+		if policy.Spec.TargetRef.SectionName != nil {


in theory a route will never reach here because of the CEL validation in the CRD, but this maybe hard for the next dev looking at this to figure that out
can we add an extra checking in here

policy.Spec.TargetRef.Kind == KindGateway && .....

arkodg · 2023-10-13T18:03:54Z

internal/gatewayapi/backendtrafficpolicy.go

+		targetNs = ptr.To(gwv1b1.Namespace(policy.Namespace))
+	}
+
+	// Ensure Policy and target are in the same namespace


curious if this can be moved to CEL Validation, not a blocker, we can raise a GH issue to track this

arkodg · 2023-10-13T18:15:50Z

internal/gatewayapi/translator.go

+		for _, parentGW := range httpRoute.Spec.ParentRefs {
+			key := types.NamespacedName{Name: string(parentGW.Name)}
+			if parentGW.Namespace != nil {
+				key.Namespace = string(*parentGW.Namespace)


when parentGW.Namespace == nil , key.Namespace = httpRoute.Namespace and the same is true for all other route types

arkodg · 2023-10-13T18:58:49Z

internal/gatewayapi/translator.go

 	gateways := t.GetRelevantGateways(resources.Gateways)

+	// Get Routes belonging to our GatewayClass.
+	routes := t.GetRelevantRoutes(gateways,


I dont think this is needed, L160-L170 already compute the relevant routes today e.g.

gateway/internal/gatewayapi/route.go

Line 63 in 2ecc54d

return relevantHTTPRoutes

we just need to append it into a list and pass it over

arkodg · 2023-10-13T18:59:07Z

internal/gatewayapi/translator.go

 	return relevant
 }
+
+func (t *Translator) GetRelevantRoutes(relevantGateways []*GatewayContext,


as shared above, dont think this func is needed

arkodg · 2023-10-13T19:04:24Z

site/content/en/latest/design/backend-traffic-policy.md

+  name: default-ipv-policy
+  namespace: default
+spec:
+  protocols:


can we use a different feature as an example, maybe load balancing
ipv6 routing is tied to L3 which EG cannot really control, it can only make sure that if ipv6 endpoints exist, it can successfully fetch it and program it

arkodg · 2023-10-13T19:05:37Z

site/content/en/latest/design/backend-traffic-policy.md

+  - Policy A will be applied/attached to the specific Listener defined in the `targetRef.SectionName`
+  - Policy B will be applied to the remaining Listeners within the Gateway. Policy B will have an additional
+  status condition `Overridden=True`.
+


needs another line about policy hierarchy/precendance ( Route > Listener > Gateway)

arkodg · 2023-10-13T19:09:41Z

internal/gatewayapi/backendtrafficpolicy.go

+			targetedRoutes[routeKey] = true
+			route := routeMap[*target]
+			for _, parentRef := range GetParentReferences(route) {
+				namespace := ""


should default to namespace of the route

https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io%2fv1beta1.ParentReference

arkodg · 2023-10-13T19:18:15Z

this PR is close to the finish line, thanks for complementing the policy hierarchy complexity with test cases, added some comments, ptal

arkodg · 2023-10-13T19:48:23Z

also 2 more things needed in this PR is

a way to subscribe to the status

gateway/internal/provider/kubernetes/controller.go

Line 1263 in 2ecc54d

// ClientTrafficPolicy object status updater
a way to skip status write if the status in the server is the same

gateway/internal/status/status.go

Line 171 in 2ecc54d

// ClientTrafficPolicy

Takes PR envoyproxy#1961 forward Signed-off-by: Arko Dasgupta <arko@tetrate.io>

arkodg · 2023-10-23T23:53:23Z

closed in favor of #2053

* feat BackendTrafficPolicy Takes PR #1961 forward Signed-off-by: Arko Dasgupta <arko@tetrate.io> * lint Signed-off-by: Arko Dasgupta <arko@tetrate.io> * k8s status Signed-off-by: Arko Dasgupta <arko@tetrate.io> * k8s watch Signed-off-by: Arko Dasgupta <arko@tetrate.io> * Update site/content/en/latest/design/backend-traffic-policy.md Co-authored-by: zirain <zirain2009@gmail.com> Signed-off-by: Arko Dasgupta <arkodg@users.noreply.github.com> * Update site/content/en/latest/design/backend-traffic-policy.md Co-authored-by: zirain <zirain2009@gmail.com> Signed-off-by: Arko Dasgupta <arkodg@users.noreply.github.com> --------- Signed-off-by: Arko Dasgupta <arko@tetrate.io> Signed-off-by: Arko Dasgupta <arkodg@users.noreply.github.com> Co-authored-by: zirain <zirain2009@gmail.com>

init backendtrafficpolicy

5db9f43

Signed-off-by: Alice Wasko <alicewasko@datawire.io>

zhaohuabing reviewed Oct 13, 2023

View reviewed changes

zhaohuabing mentioned this pull request Oct 13, 2023

feat: init SecurityPolicy #1957

Merged

arkodg reviewed Oct 13, 2023

View reviewed changes

arkodg mentioned this pull request Oct 13, 2023

Add CEL validation tests for xPolicy resources #1964

Closed

arkodg reviewed Oct 13, 2023

View reviewed changes

arkodg mentioned this pull request Oct 14, 2023

Add CEL Validations for ClientTrafficPolicy #1965

Closed

arkodg pushed a commit to arkodg/gateway that referenced this pull request Oct 23, 2023

feat BackendTrafficPolicy

c0dcb3a

Takes PR envoyproxy#1961 forward Signed-off-by: Arko Dasgupta <arko@tetrate.io>

arkodg mentioned this pull request Oct 23, 2023

feat BackendTrafficPolicy #2053

Merged

arkodg closed this Oct 23, 2023

zirain deleted the alicewasko/backendpolicy-init branch March 7, 2024 14:09

Conversation

Alice-Lilith commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 13, 2023

Codecov Report

Uh oh!

zhaohuabing commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhaohuabing Oct 13, 2023 • 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!

zhaohuabing Oct 14, 2023 • 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!

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!

arkodg commented Oct 13, 2023

Uh oh!

arkodg commented Oct 13, 2023

Uh oh!

arkodg commented Oct 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Alice-Lilith commented Oct 13, 2023 •

edited

Loading

zhaohuabing commented Oct 13, 2023 •

edited

Loading

zhaohuabing Oct 13, 2023 •

edited

Loading

zhaohuabing Oct 14, 2023 •

edited

Loading