Skip to content
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

Mapping ExternalNames to custom ports #3231

Merged
merged 5 commits into from
May 17, 2018
Merged

Mapping ExternalNames to custom ports #3231

merged 5 commits into from
May 17, 2018

Conversation

gildas
Copy link
Contributor

@gildas gildas commented Apr 24, 2018

What does this PR do?

This fixes issue #1816

Basically, if the servicePort.Port is not 443 or 80, we add it to the url (not the name).
And if the servicePort.name is "https" we also use "https" as the protocol. That allows to proxy https traffic to any port.

Motivation

In my deployment, I need træfik to proxy to external servers on custom ports. I do not control these servers.

More

  • Added/updated tests
  • Added/updated documentation

@gildas
Copy link
Contributor Author

gildas commented Apr 25, 2018

Actually, there is an issue with this PR.

When defining the Kubernetes Service, each port must have a different name.
That means if the current PR, we could have only https-type port (outside of 443).

In other words, we cannot declare:

apiVersion: v1
kind: Service
metadata:
  name: myservice
spec:
  type: ExternalName
  externalName: myserver.acme.com
  ports:
    - name: https
      port: 8443
    - name: https
      port: 9090

Kubernetes will choke on the second port upon creation.

How about we use the following naming convention:

  ports:
    - name: https
      port: 8443
    - name: https-icws
      port: 9090

The code would instead of comparing port.Name to "https" would compare the beginning of port.Name to "https".

What do you guys think?

@emilevauge
Copy link
Member

WDYT @containous/kubernetes ?

@timoreimann timoreimann self-requested a review April 27, 2018 16:39
@dtomcej
Copy link
Contributor

dtomcej commented May 7, 2018

I can get behind checking to see if the port name has an https prefix. I can't think of any reason this should not be allowed.

👍

@gildas
Copy link
Contributor Author

gildas commented May 7, 2018

It would also be very easy to change the PR from (https://github.com/gildas/traefik/blob/83629789f5a7eed8f13ff0715983efd16ce73a30/provider/kubernetes/kubernetes.go#L273):

	if port.Port == 443 || port.Name == "https" 

to:

	if port.Port == 443 || strings.HasPrefix(port.Name, "https") 

Copy link
Contributor

@dtomcej dtomcej left a comment

Choose a reason for hiding this comment

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

LGTM
:shipit:

@dtomcej
Copy link
Contributor

dtomcej commented May 15, 2018

ping @timoreimann

@ldez ldez added this to the 1.7 milestone May 16, 2018
Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@m3co-code m3co-code left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the feature! 👍

@billimek
Copy link

billimek commented Jun 5, 2018

This is merged to master but still doesn't appear to be in any of the traefik releases. What is the typical path forward to get something like this included in a release?

@timoreimann
Copy link
Contributor

@billimek the milestone indicates that the PR will be part of Traefik 1.7. Thus, the next release will include the feature.

@ldez ldez changed the title Mapping ExternalNames to custom ports Mapping ExternalNames to custom ports Jun 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants