Skip to content

clarify requirement for port in forwardTo #685

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

clarify requirement for port in forwardTo #685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions apis/v1alpha2/httproute_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -685,10 +685,11 @@ type HTTPRouteForwardTo struct {

// Port specifies the destination port number to use for the
// backend referenced by the ServiceName or BackendRef field.
// If unspecified, the destination port in the request is used
// when forwarding to a backendRef or serviceName.
// Port is required when the request is being forwarded to a Kubernetes
// Service. Port is optional with BackendRef to enable use cases where
// the port should be read from the referrant resource.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like something we should enforce with webhook validation. What should we do if BackendRef points to a Service?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like something we should enforce with webhook validation.

Absolutely, please see #578 (comment). With this patch and the validation bit, the user experience should be good.

What should we do if BackendRef points to a Service?

I didn't consider that part because I wrongly assumed that was not allowed. Is there a good reason to allow such a thing in the first place? At this level of detail, I personally prefer having only one way to do something.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't consider that part because I wrongly assumed that was not allowed. Is there a good reason to allow such a thing in the first place?

One potential example - say we end up adding the ability to forward traffic to a Service in another namespace. We have 3 options:

  1. Add a backendRef.Namespace field, this supports everything if Service can be referenced by BackendRef
  2. Add a serviceNamespace field, this only supports Service
  3. Add both a serviceNamespace field and backendRef.Namespace field

I'm pretty sure we want to avoid 3, and 2 could eventually lead to 3.

At this level of detail, I personally prefer having only one way to do something.

This may be a good argument against the serviceName shortcut altogether. I need to think about it more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is devolving into a separate conversation on if we should have serviceName shortcut or not.
Let's take a look at the following two examples:

forwardTo:
- serviceName: foo
  port: 80
forwardTo:
- backendRef:
    kind: Service
    name: foo
  port: 80

The easy path is shorter but not significantly so. The two additional lines also help with creating awareness around this extension point. Given how many corner cases exist with this special case, I think it would be best if we drop serviceName special case from here and rest of the API as well.

@jpeach @bowei @robscott @danehans Would love to hear your take on this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be confusing if backendRef.kind defaulted to Service? That would make the difference even smaller:

forwardTo:
- serviceName: foo
  port: 80
forwardTo:
- backendRef:
    name: foo
    port: 80

Given that we've required port to be specified when the backend is a Service, there's no longer a simple serviceName only short form anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather take one extra line of YAML over solving the corner cases the shortcut is introducing.
Would love to hear from others.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @hbagdi on this one. I don't see the serviceName shortcut adding enough value to make the effort worthwhile.

Copy link
Contributor

@mark-church mark-church Jul 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are thinking about having backendRef default to Service then why not do the following:

forwardTo:
- backendName: foo
  port: 80
  kind: <defaults to Service>

This still provides the shortcut, provides only 1 way of doing things, and is also the minimal number of lines. I hate to be so focused on number of YAML lines, but forwardTo is also one of the most heavily used fields in the API so the verbosity will be multiplied.

Using a key like backendName is also nice because backend is a proper term for thing that's being pointed to. I have already heard people call it "the forwardTo" which feels a bit off.

//
// Support: Core
// Support: Core for Kubernetes Service
//
// +optional
Port *PortNumber `json:"port,omitempty"`
Expand Down
7 changes: 4 additions & 3 deletions apis/v1alpha2/shared_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,10 +137,11 @@ type RouteForwardTo struct {

// Port specifies the destination port number to use for the
// backend referenced by the ServiceName or BackendRef field.
// If unspecified, the destination port in the request is used
// when forwarding to a backendRef or serviceName.
// Port is required when the request is being forwarded to a Kubernetes
// Service. Port is optional with BackendRef to enable use cases where
// the port should be read from the referrant resource.
//
// Support: Core
// Support: Core for Kubernetes Service
//
// +optional
Port *PortNumber `json:"port,omitempty"`
Expand Down
8 changes: 5 additions & 3 deletions config/crd/bases/networking.x-k8s.io_httproutes.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 5 additions & 3 deletions config/crd/bases/networking.x-k8s.io_tcproutes.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 5 additions & 3 deletions config/crd/bases/networking.x-k8s.io_tlsroutes.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 5 additions & 3 deletions config/crd/bases/networking.x-k8s.io_udproutes.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.