GEP-718: consolidation of ServiceName in RouteForwardTo #719

hbagdi · 2021-07-13T23:08:34Z

What type of PR is this?

/kind gep
What this PR does / why we need it:
This adds a GEP for droping ServiceName field from RouteForwardTo(#718)

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2021-07-13T23:08:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hbagdi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [hbagdi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

site-src/geps/gep-718.md

bowei · 2021-07-15T04:34:15Z

site-src/geps/gep-718.md

+
+```go
+// RouteForwardTo defines how a Route should forward a request.
+type RouteForwardTo struct {


Is it possible to put an indicator as to if there are changes to the long comment text

like this?

// no change // blah blah blah << // no change

@bowei I added another summarized diff in the proposal for a clearer understanding of what is changing. PTAL.

bowei · 2021-07-15T04:56:15Z

/hold (in case of accidental merge)

This looks reasonable to me

LGTM

mark-church · 2021-07-15T11:49:20Z

site-src/geps/gep-718.md

+
+## Note on future consolidation
+
+It has been noted that `forwardTo` doesn't flow as good as `backendRefs`.


Question - why wait to make this consolidation? Every change will be harder in the future and v1alpha2 is the best milestone to make these changes. The forwardTo block has a big impact on the YAML size and new user experience. Even the most basic Route with no matches must use it.

I should have used the word future more carefully.
I'll be writing another small GEP to dedicate a discussion on this matter and then implement this and the future GEP together for v1a2. This is to reach consensus on unrelated areas sooner. Sort of small atomic decisions.

My preference is the structure shown here. I read it as "forward to target X, with parameters A,B,C". I don't think that collapsing improves the readability.

hbagdi · 2021-07-15T17:56:28Z

/cc @jpeach @youngnick

hbagdi · 2021-07-21T15:20:17Z

@robscott @bowei Ping for a review.

robscott · 2021-07-21T17:57:16Z

Deploy Preview: https://deploy-preview-719--kubernetes-sigs-gateway-api.netlify.app/geps/gep-718/

robscott · 2021-07-21T18:59:14Z

@hbagdi Thanks for writing this up! This generally LGTM and seems reasonable, but I think I'd rather explore the future consolidation in the same GEP, especially if this and the future consolidation would be part of the v1alpha2 release. I'm hesitant to commit to adding the extra yaml (backend.name vs serviceName) without exploring the other options currently listed as either future expansion or alternatives.

robscott · 2021-07-21T19:14:47Z

site-src/geps/gep-718.md

+following:
+
+```yaml
+backendRefs:


The more I've thought about this, I actually think this approach could make the most sense. It's just as concise as the existing API while also being less confusing. I'm a little concerned that there will not be a way to reference/apply policy to a specific item in this list, but that seems like an edge case to me. In most cases that could be resolved by applying policy either to the route rule or to the backend itself.

jpeach · 2021-07-22T03:09:26Z

site-src/geps/gep-718.md

+	//
+	// +optional
+    // +kubebuilder:default={kind: "service"}
+	BackendRef *LocalObjectReference `json:"backendRef,omitempty"`


Maybe TargetRef, if we are considering names :)

hbagdi · 2021-07-22T15:00:37Z

I think I'd rather explore the future consolidation in the same GEP, especially if this and the future consolidation would be part of the v1alpha2 release. I'm hesitant to commit to adding the extra yaml (backend.name vs serviceName) without exploring the other options currently listed as either future expansion or alternatives.

Got it. I'll update the GEP to include this. Expect an update early next week.

stevesloka · 2021-07-26T19:28:00Z

site-src/geps/gep-718.md

+```yaml
+...
+forwardTo:
+- backendRef:


Could we remove the backendRef field and just have those fields exist in the RouteForwardTo struct?

You'd end up with this:

forwardTo: - name: foo-service-v1 port: 80 weight: 80 - name: foo-service-canary port: 80 weight: 20 --- forwardTo: - name: foo-v1 kind: server group: networking.acme.io port: 80 weight: 80 - name: foo-v1-canary kind: server group: networking.acme.io port: 80 weight: 20

This is mentioned here (https://github.com/kubernetes-sigs/gateway-api/pull/719/files#diff-200562f21cd2e117d439d1a59d8b3c844217b91f3d6224f34222687e9a8fa5faR267) to move the port/weight fields into the backendRef struct.

I'm suggesting something similar but just the opposite, moving everything into the forwardTo.

@robscott @youngnick Do you know if this is a violation of the upstream API convention or not?
I lean towards an xRef containing only the reference (as API evolves, having such a standard makes the UX much better, envoy is a good example here) but happy to tweak this if that's helpful.

This is a good question. I can't find anything in the k8s API conventions about this. @lavalamp would it ever be acceptable to include concepts like port or weight in an object reference? We're trying to figure out if we can flatten our API a bit. Essentially translating this:

forwardTo: - backendRef: group: "" kind: Service name: foo port: 80 weight: 10

to something like this:

backendRefs: - group: "" kind: Service name: foo port: 80 weight: 10

I think additional fields should be fine. As long as they don't collide with any standard fields, or fields we're likely to add. I doubt object references will ever have "port" or "weight" fields so you should be good. @deads2k will probably agree.

hbagdi · 2021-07-28T18:13:03Z

@robscott @stevesloka @jpeach I've updated this GEP to include the larger RouteForwardTo rework. Please take another look.

robscott · 2021-07-28T18:20:55Z

site-src/geps/gep-718.md

+- Rename `RouteForwardTo.BackendRef` to `RouteForwardTo.TargetRef`
+- Rename `RouteForwardTo` to `Backend`


I personally think that forwardTo is better unless we're able to flatten everything into a backendRefs list. In this current proposal we end up with the same levels of nesting (forwardTo.backendRef becomes backends.targetRef). That feels like a slightly different way to accomplish the same thing and I'm not sure it justifies a breaking change.

I am very curious if we could have a list of backendRefs that also included concepts like weight and port. If not, I think I'd prefer to fallback to forwardTo.backendRef.

I am very curious if we could have a list of backendRefs that also included concepts like weight and port. If not, I think I'd prefer to fallback to forwardTo.backendRef.

I'm fine with flattening those out if we have answers to the concerned raised below in the doc (alternatives).

Read Daniel's comment after posting the above comment, let me update this GEP in that case.

@robscott updated the GEP.

hbagdi · 2021-07-29T16:35:08Z

site-src/geps/gep-718.md

+        type: Exact
+        values:
+          env: canary
+    backends:


Suggestions so far on this name:

backendRefs: refs doesn't add much value here

targetRefs: target feels a bit too generic here

Destination and/or source is almost always preferable to "target". Target is ambiguous because it could be talking about the source's target (which is the destination) or the rule's target, (which is often the source).

mark-church · 2021-08-02T20:25:53Z

I'll just add my support for this GEP:

The new structure is very straightforward and less indentation makes it easier to read.
"backend" as a noun will make language very clear when referring to it.
It still supports weights, namespaces, and if we add name policy references too.

Given that forwardTo's are probably the most repeated field within the Gateway API, having a concise and generic format will have a big impact.

hbagdi · 2021-08-02T20:31:00Z

It still supports weights, namespaces, and if we add name policy references too.

Interesting reference to PolicyReference there.
Since we have flattened out the fields, name is used to identify the backend resource (as in the API resource name).
So, if the same name is used for referencing policy as well, are there any gotchas?

robscott

Thanks for the work on this! Just a few nits and a suggested tweak to the name but otherwise LGTM.

site-src/geps/gep-718.md

mark-church · 2021-08-02T20:59:46Z

Ignore my comment @hbagdi I don't think per-backend policies are a very realistic use-case or are easily implementable. I was thinking per-match policies and I got it mixed up.

Co-authored-by: Rob Scott <[email protected]>

hbagdi · 2021-08-03T18:55:22Z

@robscott Updated.

robscott · 2021-08-03T19:04:00Z

Thanks @hbagdi!

/lgtm

robscott · 2021-08-03T19:10:35Z

I think this has broad enough feedback and consensus to remove the hold.

/hold cancel

k8s-ci-robot added kind/gep PRs related to Gateway Enhancement Proposal(GEP) cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 13, 2021

k8s-ci-robot requested review from danehans and robscott July 13, 2021 23:08

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 13, 2021

hbagdi force-pushed the gep/718 branch from 36c7c33 to 2331b62 Compare July 13, 2021 23:25

robscott reviewed Jul 13, 2021

View reviewed changes

site-src/geps/gep-718.md Show resolved Hide resolved

hbagdi mentioned this pull request Jul 13, 2021

clarify requirement for port in forwardTo #685

Closed

bowei reviewed Jul 15, 2021

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 15, 2021

mark-church reviewed Jul 15, 2021

View reviewed changes

k8s-ci-robot requested review from jpeach and youngnick July 15, 2021 17:56

hbagdi mentioned this pull request Jul 15, 2021

v1alpha2: Clarify HTTPRouteFilter conformance levels #655

Closed

robscott added this to the v1alpha2 milestone Jul 17, 2021

robscott reviewed Jul 21, 2021

View reviewed changes

jpeach reviewed Jul 22, 2021

View reviewed changes

stevesloka reviewed Jul 26, 2021

View reviewed changes

hbagdi force-pushed the gep/718 branch from e6f50bf to d1cad24 Compare July 28, 2021 18:11

hbagdi requested review from jpeach and robscott July 28, 2021 18:14

hbagdi requested a review from bowei July 28, 2021 18:14

robscott reviewed Jul 28, 2021

View reviewed changes

hbagdi force-pushed the gep/718 branch from d1cad24 to 8641f0c Compare July 29, 2021 16:25

hbagdi commented Jul 29, 2021

View reviewed changes

robscott reviewed Aug 2, 2021

View reviewed changes

hbagdi force-pushed the gep/718 branch from e5b80d9 to c2b959a Compare August 3, 2021 18:47

gep-718: rework RoueForwardTo

3ebe173

Co-authored-by: Rob Scott <[email protected]>

hbagdi force-pushed the gep/718 branch from c2b959a to 3ebe173 Compare August 3, 2021 18:54

k8s-ci-robot assigned robscott Aug 3, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 3, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 3, 2021

k8s-ci-robot merged commit 43eb1d9 into kubernetes-sigs:master Aug 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GEP-718: consolidation of ServiceName in RouteForwardTo #719

GEP-718: consolidation of ServiceName in RouteForwardTo #719

hbagdi commented Jul 13, 2021

k8s-ci-robot commented Jul 13, 2021

bowei Jul 15, 2021

hbagdi Jul 15, 2021

bowei commented Jul 15, 2021

mark-church Jul 15, 2021

hbagdi Jul 15, 2021 •

edited

Loading

jpeach Jul 22, 2021

hbagdi commented Jul 15, 2021

hbagdi commented Jul 21, 2021

robscott commented Jul 21, 2021

robscott commented Jul 21, 2021

robscott Jul 21, 2021

jpeach Jul 22, 2021

hbagdi commented Jul 22, 2021

stevesloka Jul 26, 2021

stevesloka Jul 26, 2021

hbagdi Jul 26, 2021

robscott Jul 28, 2021

lavalamp Jul 28, 2021

hbagdi commented Jul 28, 2021

robscott Jul 28, 2021

hbagdi Jul 29, 2021

hbagdi Jul 29, 2021

hbagdi Jul 29, 2021

hbagdi Jul 29, 2021

lavalamp Jul 29, 2021

mark-church commented Aug 2, 2021 •

edited

Loading

hbagdi commented Aug 2, 2021

robscott left a comment

mark-church commented Aug 2, 2021 •

edited

Loading

hbagdi commented Aug 3, 2021

robscott commented Aug 3, 2021

robscott commented Aug 3, 2021


		## Note on future consolidation

		It has been noted that `forwardTo` doesn't flow as good as `backendRefs`.

		- Rename `RouteForwardTo.BackendRef` to `RouteForwardTo.TargetRef`
		- Rename `RouteForwardTo` to `Backend`

GEP-718: consolidation of ServiceName in RouteForwardTo #719

GEP-718: consolidation of ServiceName in RouteForwardTo #719

Conversation

hbagdi commented Jul 13, 2021

k8s-ci-robot commented Jul 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowei commented Jul 15, 2021

Choose a reason for hiding this comment

hbagdi Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbagdi commented Jul 15, 2021

hbagdi commented Jul 21, 2021

robscott commented Jul 21, 2021

robscott commented Jul 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbagdi commented Jul 22, 2021

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

hbagdi commented Jul 28, 2021

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

mark-church commented Aug 2, 2021 • edited Loading

hbagdi commented Aug 2, 2021

robscott left a comment

Choose a reason for hiding this comment

mark-church commented Aug 2, 2021 • edited Loading

hbagdi commented Aug 3, 2021

robscott commented Aug 3, 2021

robscott commented Aug 3, 2021

hbagdi Jul 15, 2021 •

edited

Loading

mark-church commented Aug 2, 2021 •

edited

Loading

mark-church commented Aug 2, 2021 •

edited

Loading