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

Enable only User Defined Port Forwarding #3563

Closed
jasperkuperus opened this issue Jan 22, 2020 · 18 comments · Fixed by #4053
Closed

Enable only User Defined Port Forwarding #3563

jasperkuperus opened this issue Jan 22, 2020 · 18 comments · Fixed by #4053
Assignees
Labels
area/portforward fixit help wanted We would love to have this done, but don't have the bandwidth, need help from contributors kind/bug Something isn't working priority/p2 May take a couple of releases

Comments

@jasperkuperus
Copy link

Sorry if this is a duplicate. There are quite some issues on port forwarding, but I didn't find an actual duplicate.

Expected behavior

In my specific scenario, I am using Redis with 2 slaves and 1 master. This results in three services:

service/redis-headless        ClusterIP      None    <none>        6379/TCP                     4m53s
service/redis-master          ClusterIP      <X>     <none>        6379/TCP                     4m53s
service/redis-slave           ClusterIP      <X>     <none>        6379/TCP                     4m53s

I define my user defined port forwards to make sure that 6379 is forwarded to the master, as I want to be able to write if I connect on the default redis port:

portForward:
  - resourceType: service
    resourceName: redis-master
    namespace: default
    port: 6379
    localPort: 6379

I would expect after running skaffold dev --port-forward, port 6379 always points to redis-master. But...

Actual behavior

Actual behavior is that first automatic port forwarding is triggered. That assigns the ports in random order, which might very well give port 6379 to redis-slave (which has no write access!), assigning a random port to redis-master. Therefore my user defined port forward is 'ignored'. As the order is random, sometimes the master does get port 6379, but I want to force that it's always master having that port.

Proposal

I would like to be able to use port forwarding and skip the automatic port forwarding. So only use my user defined port forwarding. Or turn around the order, first execute the user defined port forwarding, then automatically port forward what's left? AFAIK this is not yet possible or a workaround for this?

Information

  • Skaffold version: v1.0.1
  • Operating system: macOS Mojave
  • Contents of skaffold.yaml: Most important part already posted above.
@balopat balopat added kind/feature-request priority/p2 May take a couple of releases good first issue Good for newcomers priority/awaiting-more-evidence Lowest Priority. May be useful, but there is not yet enough supporting evidence. and removed priority/p2 May take a couple of releases good first issue Good for newcomers labels Jan 27, 2020
@balopat
Copy link
Contributor

balopat commented Jan 27, 2020

Hi @jasperkuperus, thanks for the issue!
Actually the userDefined resources do come first, but there is definitely a racey scenario here as by default all 3 services compete for 6379.

One way to workaround this might be to specify manually the conflict resolution:

  - resourceType: service
    resourceName: redis-master
    namespace: default
    port: 6379
    localPort: 6379
  - resourceType: service
    resourceName: redis-slave
    namespace: default
    port: 6379
    localPort: 6380
  - resourceType: service
    resourceName: redis-headless
    namespace: default
    port: 6379
    localPort: 6381

Would this work?

@jasperkuperus
Copy link
Author

@balopat Thanks for the response! But alas, this does not work for me. I did exactly this, ran skaffold dev --port-forward a few times and after a few tries ran into this:

Port forwarding service/redis-headless in namespace default, remote port 6379 -> local port 6379
Port forwarding service/redis-master in namespace default, remote port 6379 -> local port 6380
Port forwarding service/redis-slave in namespace default, remote port 6379 -> local port 6381

@foobarbecue
Copy link

foobarbecue commented Feb 6, 2020

I have the same problem. It makes user-defined port forwarding useless.

skaffold.yaml:

apiVersion: skaffold/v2alpha2
kind: Config
metadata:
  name: skaffold
build:
  artifacts:
  - image: lmmp-container
deploy:
  kubectl:
    manifests:
    - https://raw.githubusercontent.com/argoproj/argo/v2.5.0-rc8/manifests/install.yaml
    - k8s-*.yaml
portForward:
  - resourceType: service
    resourceName: argo-server
    address: 0.0.0.0
    namespace: argo
    port: 32019
    localPort: 2746

Result is sometimes:

Port forwarding service/argo-server in namespace argo, remote port 2746 -> address 127.0.0.1 port 2746
Port forwarding Service/argo-server in namespace argo, remote port 32019 -> address 0.0.0.0 port 2747

and other times:

Port forwarding service/argo-server in namespace argo, remote port 32019 -> address 0.0.0.0 port 32019
Port forwarding service/argo-server in namespace argo, remote port 2746 -> address 127.0.0.1 port 2746

argo-server is running on 2746. I'm suprised that Skaffold is coming up with 2747. I guess it just increments by one when a port is in use?

@maistrotoad
Copy link

I am also experiencing this issue. I got multiple services in my project, which all have 80 set as their port. I have configured Skaffold on my Ubuntu machine so it is allowed to port-forward ports below 1024 without sudo. What then seems to happen is a race condition for the first service to get deployed, this service will then get the port-forward to port 80, the ones coming after it get incremented ports like 81, 82, etc.

@ncthbrt
Copy link

ncthbrt commented Mar 8, 2020

I also have this problem. All services in our cluster are bound to 80 and 443, so all the services compete for the same ports. However what I really want is just for our ingress service to forward traffic from 80 and 443, so I defined this in the portForward section of skaffold.yaml. I expected manual port specification to take priority, so found the actual behaviour quite confusing.

Having to manually specify resolutions for services I don't need to expose externally is a little tedious and means that team members have to remember to do this every time they want to create a new service. Further having actually attempted this solution, it appears that it does not in fact work.

Having a flag to turn off automatic forwarding, or eliminating the race condition would solve this problem for me. Our system doesn't really work with arbitrary ports as we've got OpenID Connect servers running that for specification compliance have to specify redirect URLs in advance, so this is a very important feature for us.

As a workaround for the moment, I've resorted to using manual port forwarding

@ncthbrt
Copy link

ncthbrt commented Mar 15, 2020

@balopat You're the maintainer of this repository, so this is ultimately your (or the rest of the team's) judgement call, but I'd venture to suggest that this issue is actually a bug report disguised as a feature request.

@nkubala nkubala added kind/bug Something isn't working priority/p2 May take a couple of releases triage/discuss Items for discussion and removed kind/feature-request priority/awaiting-more-evidence Lowest Priority. May be useful, but there is not yet enough supporting evidence. labels Apr 7, 2020
@discreet
Copy link

discreet commented Apr 9, 2020

I'm deploying a Helm chart with Skaffold and trying to port forward to one of my services however the user defined port forward doesn't work...at all. Unless I specify --port-forward and use the automatic port forwarding for every service deployed Skaffold won't do any port forwarding. My skaffold.yaml file is below:

apiVersion: skaffold/v2beta1
kind: Config
metadata:
  name: appName
build:
  tagPolicy:
    envTemplate:
      template: "{{.IMAGE_NAME}}:{{.USER}}-latest"
  artifacts:
  - image: registry/project/app/development
    kaniko:
      buildArgs:
        GIT_PRIVATE_KEY: '{{.GIT_PRIVATE_KEY}}'
  cluster:
    pullSecretName: kaniko-pull
    dockerConfig:
      secretName: kaniko-push
    resources:
      requests:
        cpu: "500m"
        memory: "4Gi"
      limits:
        cpu: 1
        memory: "6Gi"
profiles:
  - name: latest
    deploy:
      helm:
        releases:
        - name:appName
          chartPath: helm/helmchart
          valuesFiles:
            - helm/helmchart/values.yaml
          values:
            surveyLoader.image: registry/project/app/development
          setValues:
            surveyLoader.imagePullSecrets: "{gitlab}"
    portForward:
      - name: proxy
        resourceType: service
        resourceName: proxy
        port: 13132
        localPort: 13132

@tstromberg tstromberg added help wanted We would love to have this done, but don't have the bandwidth, need help from contributors and removed triage/discuss Items for discussion labels Apr 20, 2020
@balopat balopat added priority/p1 High impact feature/bug. priority/p2 May take a couple of releases and removed priority/p2 May take a couple of releases priority/p1 High impact feature/bug. labels Apr 20, 2020
@nkubala nkubala added the fixit label Apr 24, 2020
@nkubala
Copy link
Contributor

nkubala commented Apr 24, 2020

I wonder if we want to change the default behavior of skaffold to always respect user port forwarding by default, but only automatically port forward when specified by the user. I suspect that when port forwarding is manually specified, auto port forwarding is almost never desired...so users that specified this would get the desired behavior of respecting their port forward config, and users that don't specify this still have the option to get auto port forwarding using --auto-port-forward or something similar.

@dgageot dgageot self-assigned this Apr 27, 2020
@dgageot
Copy link
Contributor

dgageot commented Apr 27, 2020

Here's what I think I'll do. Just want to be sure everyone is ok:

  • When skaffold runs without --port-forward, don't do any port forwarding.
  • When skaffold runs with --port-forward:
    • When skaffold.yaml contains at least one entry in portForward, only port forward those ports
    • Otherwise use the good old automatic port forwarding

@nkubala @jasperkuperus @foobarbecue @maistrotoad @ncthbrt @discreet would that be ok?

dgageot added a commit to dgageot/skaffold that referenced this issue Apr 27, 2020
@ncthbrt
Copy link

ncthbrt commented Apr 27, 2020

That'd be a massive improvement!

@nkubala
Copy link
Contributor

nkubala commented Apr 27, 2020

@dgageot that is definitely an improvement. however, I'm wondering if it would be a weird UX to have manual port forwarding defined in a skaffold.yaml, but then running skaffold dev with no flags the ports don't get forwarded. what about:

  • skaffold dev
    • (user defined ports) -> only user defined ports forwarded
    • (no user defined ports) -> no ports forwarded
  • skaffold dev --port-forward
    • (user defined ports) -> only user defined ports forwarded
    • (no user defined ports) -> good old auto port forwarding

the only weirdness here is that skaffold dev with or without --port-forward does the exact same thing if users have defined manual port forwarding in their skaffold.yaml. in this case we might want to issue a warning when running with --port-forward AND user defined ports that "automatic port forwarding is not supported when manual port forwarding is specified".

maybe this isn't better than what you have though?

@jasperkuperus
Copy link
Author

@dgageot That would already be a big help! And definitely works for me.

Personally, my intuition would be even a little different from what @nkubala proposes:

  • skaffold dev
    • (user defined ports) -> only user defined ports forwarded
    • (no user defined ports) -> no ports forwarded
  • skaffold dev --port-forward
    • (user defined ports) -> user defined ports take precedence, all other ports are then automatically forwarded
    • (no user defined ports) -> good old auto port forwarding

This way, if there are user defined ports, they are always used. And the flag is only for enabling auto port forwarding. And when they're used together, user defined ports should -always- be the first to be executed and the automatic ports may not override a user defined port. This way, I only have to create user defined ports for services that may result in a possible conflict (redis master/slave/slave, postgres/postgress headless, etc.) and further rely on the auto port forwarding.

@maistrotoad
Copy link

@dgageot If your proposal is easiest to implement, I can definitely work with that. The most important bit to me would be that user defined ports are respected in any case. Other aspects of it would be details that do not matter for my use case.

Maybe in a second iteration something akin to what @jasperkuperus is proposing would provide the most intuitive and feature-rich experience though.

@dgageot
Copy link
Contributor

dgageot commented Apr 29, 2020

While trying to implement what I said, I realized that there are cases where auto port forwarding in addition to manual port forwarding is pretty useful. We have a few integration tests that rely on the services being auto forwarded and a single deployment being user-defined forwarded.

So, I'll try a different strategy among the different proposals.

The one thing I don't really want is to have some port forwarding when --port-forward isn't set.

@dgageot
Copy link
Contributor

dgageot commented Apr 29, 2020

I've tried something much simpler :-)
I've kept the current behaviour in terms of which ports are port forwarded.
However, instead of starting the port forwarding in a separate go routine for each port, I starting a single go routine, for all the ports, in order. That way, the user-defined rules should take precedence over the automatic rules.

dgageot added a commit to dgageot/skaffold that referenced this issue Apr 29, 2020
First, user defined rules.
Then, automatic rules.

Fixes GoogleContainerTools#3563

Signed-off-by: David Gageot <[email protected]>
@dgageot
Copy link
Contributor

dgageot commented Apr 29, 2020

@nkubala @jasperkuperus @foobarbecue @maistrotoad @ncthbrt @discreet could you tell me if #4053 works for you and has good performance?

dgageot added a commit that referenced this issue Apr 30, 2020
First, user defined rules.
Then, automatic rules.

Fixes #3563

Signed-off-by: David Gageot <[email protected]>
@jasperkuperus
Copy link
Author

@dgageot Haven't had the time to test this yet. But if this PR indeed first applies the user defined port forwarding, then the automatic port forwarding, it'll work for me!

@foobarbecue
Copy link

Works for me! Note that I was running into problems because I capitalized the S in resourceType: Service ... then I was getting both auto and local port forwarding. This might have been my problem all along, actually...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/portforward fixit help wanted We would love to have this done, but don't have the bandwidth, need help from contributors kind/bug Something isn't working priority/p2 May take a couple of releases
Projects
None yet
9 participants