feat(translator): implement backend API #3495

guydc · 2024-05-28T18:37:12Z

What this PR does / why we need it:
Implements backend API

Support translation of fqdn, ip and unix endpoints in IR and XDS
Support H2C upstream application protocol hint
Support HTTPRoute and EnvoyExtensionPolicy references to Backend
Extend BackendTLSPolicy translation tests for HTTPRoute and Backend use cases
Minor refactor of test manifests and translation to increase code reuse

Which issue(s) this PR fixes:
Fixes #2997
Relates to #36

codecov · 2024-05-28T18:42:35Z

Codecov Report

Attention: Patch coverage is 65.94595% with 126 lines in your changes missing coverage. Please review.

Project coverage is 68.14%. Comparing base (3712659) to head (baf0840).

Files	Patch %	Lines
internal/provider/kubernetes/controller.go	18.18%	38 Missing and 7 partials ⚠️
internal/gatewayapi/validate.go	80.21%	14 Missing and 4 partials ⚠️
internal/gatewayapi/status/backend.go	0.00%	13 Missing ⚠️
internal/provider/kubernetes/predicates.go	0.00%	11 Missing ⚠️
internal/provider/kubernetes/status_updater.go	0.00%	6 Missing ⚠️
internal/gatewayapi/envoyextensionpolicy.go	54.54%	4 Missing and 1 partial ⚠️
internal/ir/xds.go	58.33%	4 Missing and 1 partial ⚠️
internal/provider/kubernetes/helpers.go	0.00%	5 Missing ⚠️
internal/gatewayapi/backend.go	88.23%	2 Missing and 2 partials ⚠️
internal/gatewayapi/ext_service.go	80.00%	2 Missing and 2 partials ⚠️
... and 5 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3495      +/-   ##
==========================================
+ Coverage   68.12%   68.14%   +0.02%     
==========================================
  Files         165      167       +2     
  Lines       20122    20370     +248     
==========================================
+ Hits        13708    13882     +174     
- Misses       5416     5487      +71     
- Partials      998     1001       +3

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

Signed-off-by: Guy Daich <[email protected]>

guydc · 2024-05-28T22:58:38Z

/retest

arkodg · 2024-05-29T19:22:10Z

internal/gatewayapi/route.go

@@ -1176,6 +1178,17 @@ func (t *Translator) processDestination(backendRefContext BackendRefContext,
 			Endpoints:   endpoints,
 			AddressType: addrType,
 		}
+		// TODO: support mixed endpointslice address type for the same backendRef


can we move this out of switch to make it common amongst all backend types ?

The condition is slightly different for Backend vs. the native K8s resources (depending on EndpointRouting toggle) and so is the message. I'll split the validation and status update.

arkodg · 2024-05-29T19:37:24Z

internal/gatewayapi/status/backend.go

+	case true:
+		return newCondition(string(egv1a1.BackendReasonInvalid), metav1.ConditionTrue,
+			string(egv1a1.BackendConditionAccepted),
+			"The Backend was accepted Envoy Gateway", time.Now(), be.Generation)


Suggested change

"The Backend was accepted Envoy Gateway", time.Now(), be.Generation)

"The Backend was accepted by Envoy Gateway", time.Now(), be.Generation)

Suggested change

"The Backend was accepted Envoy Gateway", time.Now(), be.Generation)

"The Backend was accepted Envoy Gateway", time.Now(), be.Generation)

arkodg · 2024-05-29T19:37:52Z

internal/gatewayapi/status/backend.go

+	case true:
+		return newCondition(string(egv1a1.BackendReasonInvalid), metav1.ConditionTrue,
+			string(egv1a1.BackendConditionAccepted),
+			"The Backend was accepted Envoy Gateway", time.Now(), be.Generation)


do we need the "Envoy Gateway" suffix?

arkodg · 2024-05-29T19:51:11Z

internal/provider/kubernetes/controller.go

+	// Watch Backend CRUDs and process affected *Route objects.
+	if r.envoyGateway.ExtensionAPIs != nil && r.envoyGateway.ExtensionAPIs.EnableBackend {
+		backendPredicates := []predicate.TypedPredicate[*egv1a1.Backend]{
+			predicate.NewTypedPredicateFuncs[*egv1a1.Backend](func(be *egv1a1.Backend) bool {


ObservedGeneration predicate ? we haven't added the ObjectMeta yet so it won't work
the issue I see here is constant reconciling -

reconcile backend

translate backend and update status

translate again (may be stopped by watchable layer which checks for equality)

gateway/internal/gatewayapi/resource.go

Line 172 in 44330ef

return reflect.DeepEqual(c, y)

Can you elaborate on why TypedGenerationChangedPredicate won't work here?

nvm it will, took a look at the struct

gateway/api/v1alpha1/backend_types.go

Line 43 in 6f30397

metav1.ObjectMeta `json:"metadata,omitempty"`

and you've added ObjectMeta, so should work, lets add it

internal/provider/kubernetes/controller.go

Signed-off-by: Guy Daich <[email protected]>

guydc · 2024-05-29T23:45:43Z

/retest

arkodg

LGTM thanks for implementing this and making it easier to route to non k8s endpoints !

can we document the remaining items (disallowing localhost , other xRoute backend) as subtasks ?

Signed-off-by: Guy Daich <[email protected]>

guydc · 2024-05-31T13:40:14Z

/retest

Signed-off-by: Guy Daich <[email protected]>

liorokman

Looking at this carefully again, I think maybe it would make sense to rename the IPv4 field in BackendEndpoint to IP since using IPv6 addresses would probably already just work.

liorokman · 2024-05-31T20:35:38Z

internal/ir/xds.go

+		}
+	} else { // IP or FQDN
+		err := validation.IsDNS1123Subdomain(d.Host)
+		_, pErr := netip.ParseAddr(d.Host)


Even though the field is called "ipv4", I think IPv6 addresses would still work here. netip.ParseAddr has no issue with IPv6 addresses.

This is enforced with CEL at the moment:

gateway/api/v1alpha1/backend_types.go

Line 84 in 7f2038f

// +kubebuilder:validation:Pattern=`^((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$`

liorokman · 2024-05-31T20:37:05Z

internal/gatewayapi/validate.go

+			unsupportedRef = true
+		case bep.FQDN != nil && bep.FQDN.Hostname == "localhost":
+			unsupportedRef = true
+		case bep.IPv4 != nil && bep.IPv4.Address == "127.0.0.1":


Even though the field is called IPv4, there's nothing that really prevents IPv6 addresses from being specified here as far as I can tell.

Consider also rejecting the IPv6 localhost address ? ::1 ?

Also, localhost on Linux systems is not just 127.0.0.1. It's 127.0.0.1/8, which means that this check is wrong.

The correct way to check this would be to check if the provided address is contained in 127.0.0.0/8, not to string-compare to 127.0.0.1.

yea, good point, we just discussed this in slack. I'll update the validation to comply with the K8s one for EP Slices that checks for the entire range.

liorokman · 2024-05-31T20:38:33Z

internal/gatewayapi/validate.go

+		switch {
+		case bep.Unix != nil:
+			unsupportedRef = true
+		case bep.FQDN != nil && bep.FQDN.Hostname == "localhost":


If the intent is to prevent localhost from being the target, in case of an FQDN the better approach would be to try to resolve the hostname and see that it doesn't resolve to the localhost.

What would happen if somebody were to use an IP in the FQDN field of BackendEndpoint? I think it would just work, except that it would be possible to use 127.0.0.1 or ::1 here.

If the intent is to prevent localhost from being the target, in case of an FQDN the better approach would be to try to resolve the hostname and see that it doesn't resolve to the localhost.

Users can change the DNS record this after the resource is accepted. Then, it becomes a race with the next translation until the config is rejected. So, it's still possible that there's a time window where localhost routing is possible.

We discussed this concern here: #3063 (comment). I'm not sure that there really is a good solution for that. The compromise is to enforce validations equivalent to what K8s allows in EP slices, and make users aware of the risk...

What would happen if somebody were to use an IP in the FQDN field of BackendEndpoint?

There's a CEL validation that will probably reject IPv6 Addresses (as ":" is nto supported). But, IPv4 addresses are actually accepted by this validation. I'll add additional programatic validations.

gateway/api/v1alpha1/backend_types.go

Line 103 in 7f2038f

// +kubebuilder:validation:Pattern=`^(\*\.)?[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$`

guydc · 2024-05-31T21:25:00Z

/retest

shawnh2 · 2024-06-02T08:36:32Z

internal/provider/kubernetes/controller.go

a non-blocking comment:

gateway/internal/provider/kubernetes/status_updater.go

Line 196 in eaa685d

func isStatusEqual(objA, objB interface{}) bool {

and

gateway/internal/provider/kubernetes/status_updater.go

Line 311 in eaa685d

func kindOf(obj interface{}) string {

can also need a update.

good catch, thanks, done!

shawnh2 · 2024-06-02T08:44:41Z

internal/gatewayapi/route.go

+
+	for _, ap := range backend.Spec.AppProtocols {
+		if ap == v1alpha1.AppProtocolTypeH2C {
+			dstProtocol = ir.HTTP2


need a break here?

shawnh2 · 2024-06-02T08:47:19Z

internal/gatewayapi/route.go

 	}

 	ds.Weight = &weight
 	return ds
 }

+func validateDestinationSettings(destinationSettings *ir.DestinationSetting, endpointRoutingDisabled bool, kind *gwapiv1.Kind) (bool, string) {


instead of string, can we return an error here? then we can use error.Error() to retrieve the string of this error later.

Signed-off-by: Guy Daich <[email protected]>

guydc · 2024-06-03T21:23:14Z

IP:

API Changed from IPv4/IPv6 => IP for future use, as suggested
IPv6 still not supported and rejected in validation (CEL, Controller).
Validate that target is not in the entire loopback range, for both xRoute and non-xRoute

FQDN:

Fix CEL validation to not support wildcards
Add controller validation that FQDN is not an IP address and has at least two segments, for both xRoute and non-xRoute

Signed-off-by: Guy Daich <[email protected]>

zhaohuabing · 2024-06-04T00:32:00Z

This one is huge :-)

It would be easier to review and quicker to merge if it could be split into multiple PRs.

zhaohuabing · 2024-06-04T00:51:10Z

This one is huge :-)

It would be easier to review and quicker to merge if it were split into multiple PRs.

I don’t mean this one, as it’s already under review, but in the future, it might be helpful to split large PRs into multiple smaller ones for easier and quicker reviews.

shawnh2 · 2024-06-04T03:08:33Z

internal/gatewayapi/status/backend.go

+
+// computeBackendAcceptedCondition computes the Backend Accepted status condition.
+func computeBackendAcceptedCondition(be *egv1a1.Backend, accepted bool, msg string) metav1.Condition {
+	switch accepted {


why not use the simple if..else for this boolean ?

guydc force-pushed the feat-backend branch 2 times, most recently from fe6d28d to 9fe20b9 Compare May 28, 2024 19:47

implement backend

67106e1

Signed-off-by: Guy Daich <[email protected]>

guydc force-pushed the feat-backend branch from 9fe20b9 to 67106e1 Compare May 28, 2024 21:46

guydc added 2 commits May 28, 2024 16:47

Merge branch 'main' into feat-backend

1f0a58e

Merge branch 'main' into feat-backend

e8c68d5

guydc marked this pull request as ready for review May 28, 2024 23:03

guydc requested a review from a team as a code owner May 28, 2024 23:03

arkodg reviewed May 29, 2024

View reviewed changes

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

guydc added 2 commits May 29, 2024 17:40

fix code review comments

c537b52

Signed-off-by: Guy Daich <[email protected]>

Merge branch 'main' into feat-backend

ab368bd

Signed-off-by: Guy Daich <[email protected]>

Merge branch 'main' into feat-backend

d75fa46

arkodg previously approved these changes May 30, 2024

View reviewed changes

arkodg requested review from a team May 30, 2024 23:31

guydc mentioned this pull request May 31, 2024

Backend Implementation #3509

Open

17 tasks

Merge branch 'main' into feat-backend

bd38b55

Signed-off-by: Guy Daich <[email protected]>

guydc dismissed arkodg’s stale review via bd38b55 May 31, 2024 13:24

guydc requested a review from arkodg May 31, 2024 14:12

arkodg previously approved these changes May 31, 2024

View reviewed changes

Merge branch 'main' into feat-backend

6b2ffd3

Signed-off-by: Guy Daich <[email protected]>

guydc dismissed arkodg’s stale review via 6b2ffd3 May 31, 2024 20:21

liorokman reviewed May 31, 2024

View reviewed changes

guydc added the hold do not merge label May 31, 2024

shawnh2 reviewed Jun 2, 2024

View reviewed changes

guydc added 2 commits June 3, 2024 16:19

code review fixes

3218653

Signed-off-by: Guy Daich <[email protected]>

Merge branch 'main' into feat-backend

0964a15

fix gen

946bd01

Signed-off-by: Guy Daich <[email protected]>

guydc removed the hold do not merge label Jun 3, 2024

guydc added 2 commits June 3, 2024 18:13

Merge branch 'main' into feat-backend

651545e

fix gen

baf0840

Signed-off-by: Guy Daich <[email protected]>

guydc requested review from arkodg, shawnh2 and liorokman June 3, 2024 23:57

shawnh2 reviewed Jun 4, 2024

View reviewed changes

liorokman approved these changes Jun 4, 2024

View reviewed changes

arkodg approved these changes Jun 4, 2024

View reviewed changes

arkodg merged commit 09fd519 into envoyproxy:main Jun 4, 2024
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(translator): implement backend API #3495

feat(translator): implement backend API #3495

guydc commented May 28, 2024 •

edited

Loading

codecov bot commented May 28, 2024 •

edited

Loading

guydc commented May 28, 2024

arkodg May 29, 2024

guydc May 29, 2024 •

edited

Loading

arkodg May 29, 2024

arkodg May 29, 2024

arkodg May 29, 2024

arkodg May 29, 2024

guydc May 29, 2024

arkodg May 29, 2024

guydc commented May 29, 2024

arkodg left a comment

guydc commented May 31, 2024

liorokman left a comment

liorokman May 31, 2024

guydc May 31, 2024

liorokman May 31, 2024

liorokman May 31, 2024

guydc May 31, 2024

liorokman May 31, 2024

liorokman May 31, 2024 •

edited

Loading

guydc May 31, 2024 •

edited

Loading

guydc May 31, 2024

guydc commented May 31, 2024

shawnh2 Jun 2, 2024

guydc Jun 4, 2024

shawnh2 Jun 2, 2024

shawnh2 Jun 2, 2024 •

edited

Loading

guydc commented Jun 3, 2024

zhaohuabing commented Jun 4, 2024 •

edited

Loading

zhaohuabing commented Jun 4, 2024

shawnh2 Jun 4, 2024

	"The Backend was accepted Envoy Gateway", time.Now(), be.Generation)
	"The Backend was accepted by Envoy Gateway", time.Now(), be.Generation)

feat(translator): implement backend API #3495

feat(translator): implement backend API #3495

Conversation

guydc commented May 28, 2024 • edited Loading

codecov bot commented May 28, 2024 • edited Loading

Codecov Report

guydc commented May 28, 2024

Choose a reason for hiding this comment

guydc May 29, 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

guydc commented May 29, 2024

arkodg left a comment

Choose a reason for hiding this comment

guydc commented May 31, 2024

liorokman 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

liorokman May 31, 2024 • edited Loading

Choose a reason for hiding this comment

guydc May 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guydc commented May 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnh2 Jun 2, 2024 • edited Loading

Choose a reason for hiding this comment

guydc commented Jun 3, 2024

zhaohuabing commented Jun 4, 2024 • edited Loading

zhaohuabing commented Jun 4, 2024

Choose a reason for hiding this comment

guydc commented May 28, 2024 •

edited

Loading

codecov bot commented May 28, 2024 •

edited

Loading

guydc May 29, 2024 •

edited

Loading

liorokman May 31, 2024 •

edited

Loading

guydc May 31, 2024 •

edited

Loading

shawnh2 Jun 2, 2024 •

edited

Loading

zhaohuabing commented Jun 4, 2024 •

edited

Loading