Skip to content

HTTPRoute forwardTo ports not required #578

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.

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
stevesloka opened this issue Mar 9, 2021 · 6 comments
Closed

HTTPRoute forwardTo ports not required #578

stevesloka opened this issue Mar 9, 2021 · 6 comments
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.
Milestone

Comments

@stevesloka
Copy link
Contributor

What happened:

In implementing HTTPRoutes, I had a question on how to handle forwardTo.Ports if unspecified. I was unsure of which port to use since that is currently an optional field.

The docs state:

If unspecified, the destination port in the request is used when forwarding to a backendRef or serviceName.

I asked a question on Slack (https://kubernetes.slack.com/archives/CR0H13KGA/p1615242865086900) and found out about this thread in Slack (https://kubernetes.slack.com/archives/CR0H13KGA/p1613674798102500?thread_ts=1613607883.098300&cid=CR0H13KGA) which already was asked. =)

My original question was this:

The docs say that if not specified, it is to use the port the request came on. But in the event that we have two ports, an insecure & secure, is the assumption that my backend app will listen on both? Or only the https port (assuming I have the 301 redirect in place)?

Chatting with @danehans the thought was to revert the port from being optional back to being required. This would allow someone to specify which port to use when forwarding traffic to an upstream all the time.

To clarify how I understand, by making the port required we can assume the following:

  1. If serviceName is provided, then port is used to forward to name:port
  2. If backedRef is provided (and not serviceName), then port is used to forward to backendRef:port
  3. If both serviceName + backendRef is supplied, then serviceName:port takes priority

My only question is that is it assumed the port from forwardTo would be used in conjunction with a backendRef (i.e. example 2 previously), or would that referenced object want to configure those bits in that custom resource? If the backedRef wants to handle all of that itself, then maybe we should leave this alone and just require controllers to validate that port is provided when a serviceName is used.

// cc @danehans @robscott @howardjohn

@stevesloka stevesloka added the kind/bug Categorizes issue or PR as related to a bug. label Mar 9, 2021
@danehans
Copy link
Contributor

Notes from the community meeting:

  • We need to decide if port should be required or option. As @howardjohn pointed out, how does an implementation know what port to use if unspecified?
  • This example should be updated to remove the service target ports and change the httproute port to 443.
  • As @Miciah mentioned, the port docs should be updated to state that port should be the port of a Service when serviceName is specified.

cc: @stevesloka @robscott

@hbagdi
Copy link
Contributor

hbagdi commented May 5, 2021

/assign

@hbagdi
Copy link
Contributor

hbagdi commented May 26, 2021

My proposal for path forward on this problem:

This example should be updated to remove the service target ports and change the httproute port to 443.

@danehans I'm not sure what you mean here, could you please rephrase?

@robscott
Copy link
Member

robscott commented Aug 9, 2021

@hbagdi @stevesloka can we close this issue now that GEP #718 clarified where port was required? We also have #753 to track implementing that validation in the webhook.

@hbagdi
Copy link
Contributor

hbagdi commented Aug 9, 2021

/close

@k8s-ci-robot
Copy link
Contributor

@hbagdi: Closing this issue.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

No branches or pull requests

5 participants