[design] revise port-forwarding behaviour #4832

briandealwis · 2020-09-29T19:35:21Z

Fixes: #4163
Related: #969 #1245 #1564 #2115

Description

This PR proposes to change the behaviour of Skaffold's --port-foward. In a nutshell:

skaffold dev defaults to enable the user-defined forwards in the skaffold.yaml
skaffold dev --port-forward enables user-define forwards and forwards services (similar to today)
skaffold dev --port-forwarding=xxx allows selecting specific combinations of forwards
skaffold debug defaults to enable the user-defined forwards in the skaffold.yaml and debugging ports enabled on containers
skaffold debug --port-forward enables user-define forwards and forwards debug ports and services but does not forward container ports
skaffold debug --port-forwarding=xxx allows selecting specific combinations of forwards

where xxx is one of:

user: user-defined port-forwards as defined in the skaffold.yaml
services: service ports are forwarded
pods: containerPorts defined on Pods and Kubernetes workload objects that have pod-specs
are forwarded (Deployment, ReplicaSet, StatefulSet, DaemonSet, Job, CronJob)
debug: an internal strategy to forward debugging-related ports on Pods as set up
from skaffold debug
off: no ports are ever forwarded

User facing changes (remove if N/A)
This change aims to be backwards compatible, particularly for the Cloud Code IDEs which use skaffold dev --port-foward
and skaffold debug --port-foward.

(I'll include the implementation as part of this PR.)

- `--port-forwarding` -> `--port-forward-modes` - created a table summarizing mode changes from v1.15.0 - clarified that `debug` was `skaffold debug`-related ports - added config settings to `skaffold.yaml` and global config

codecov · 2020-09-30T19:15:12Z

Codecov Report

Merging #4832 (3ed71dc) into master (f4bd638) will increase coverage by 0.03%.
The diff coverage is n/a.

❗ Current head 3ed71dc differs from pull request most recent head 3dc6da5. Consider uploading reports for the commit 3dc6da5 to get more accurate results

@@            Coverage Diff             @@
##           master    #4832      +/-   ##
==========================================
+ Coverage   71.72%   71.75%   +0.03%     
==========================================
  Files         353      355       +2     
  Lines       12099    12116      +17     
==========================================
+ Hits         8678     8694      +16     
- Misses       2775     2776       +1     
  Partials      646      646

Impacted Files	Coverage Δ
pkg/skaffold/kubernetes/manifest/manifests.go	`91.48% <0.00%> (-3.11%)`	⬇️
pkg/skaffold/schema/versions.go	`80.39% <0.00%> (ø)`
pkg/skaffold/schema/v2beta8/upgrade.go	`100.00% <0.00%> (ø)`
pkg/skaffold/schema/v2beta8/config.go	`100.00% <0.00%> (ø)`
cmd/skaffold/app/cmd/filter.go	`26.82% <0.00%> (+0.63%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4bd638...3dc6da5. Read the comment docs.

docs/design_proposals/port-forward.md

nkubala · 2020-09-30T19:36:03Z

docs/design_proposals/port-forward.md

+> a backward-compatible manner (still under investigation).  This document assumes it is
+> not possible.
+
+~Skaffold's `--port-forward` argument should be changed from a binary true/false option to~


remove since you have a similar note above?

Will remove once I've confirmed that this can't be done.

docs/design_proposals/port-forward.md

nkubala · 2020-09-30T19:38:31Z

docs/design_proposals/port-forward.md

+
+#### skaffold.yaml
+
+We will define a new configuration block `defaults` with a single array field `portForwardModes`:


is this meant to be extended in the future? seems strange to add a top-level defaults field if there's only port-related config in it. maybe we can add this in the portForward stanza instead?

I had been thinking that we could put other defaults in here, like default-repo (#2317), but most of our other settings are found under the appropriate area.

The portForward block is an array of definitions, and I didn't think of an obvious field name to tuck them under. definitions? resources?

portForward: defaultModes: ["..."] resources: - resourceType: service resourceName: example

docs/design_proposals/port-forward.md

Co-authored-by: Nick Kubala <nkubala@users.noreply.github.com>

gsquared94 · 2020-10-13T09:49:15Z

docs/design_proposals/port-forward.md

+
+#### Global config ($HOME/.skaffold/config)
+
+The global config will support a `portForwardModes` with a single array field.  This can be set as a


does persisting this in the global config mean that dev and debug will default to it in the absence of an explicit flag for every project? I'm not sure if that's a good thing. Doesn't seem like portForwardModes is a global (shared across projects sort of) variable like kubecontext or default repo.

it'd probably look better as a per-project default but I don't think we have that granularity yet in the global config.

It could also be set on a per-context basis within the ~/.skaffold/config. The global default is good for people who are adamant that they never ever want port-forwarding.

IMHO it's better to preserve the defaults for every new project. There's already a way to set it globally via the generated env variable SKAFFOLD_PORT_FORWARD_MODES. And all the other properties stored in global config inherently make sense persisted across projects. Is there any issue created by the adamant user?

#1564 has a few comments. At that time we forwarded all containers across all pods, so it could be confusing. With saner defaults, this shouldn't be an issue, but it is worth implementing here.

docs/design_proposals/port-forward.md

MarlonGamez · 2021-02-01T21:39:37Z

@briandealwis do you have any updates on the status of this? You mentioned here that you'll include the implementation in this PR, do you still want to keep with that idea? Or do you think we should get this proposal merged and follow up with the implementation

briandealwis · 2021-03-16T14:27:56Z

I removed the section on the configuration in skaffold.yaml and global config — I think we should consider a broader rethink of embedding configuration in the skaffold.yaml and ~/.skaffold/config, and include things like default-repo, testing, and perhaps more.

nkubala · 2021-03-17T01:26:40Z

just leaving thoughts about this here (they should probably go in a separate issue):

I think we should consider a broader rethink of embedding configuration in the skaffold.yaml and ~/.skaffold/config, and include things like default-repo, testing, and perhaps more.

the global config was meant to address two issues:

alleviate the pain of having to specify permanent options through CLI flags (e.g. insecure registries)
enable users to set environment- or host-specific configuration options (e.g. cluster/registry info, push options) without having to embed them in their skaffold.yaml, to allow them to keep sharing a skaffold.yaml across hosts or environments

this is one potential set of grounding principles for whether or not we support a particular field in the global config alongside the skaffold.yaml, but we don't really have anything documented around this. I think port forwarding is a good candidate as per the first issue here, but I'm fine leaving it out of this design until we have a more robust strategy.

briandealwis · 2021-03-17T15:22:48Z

I started to wonder how it would/should interact with different contexts and profiles (especially command-activated profiles) and decided it seemed too involved for now.

docs/design_proposals/port-forward.md

briandealwis added 2 commits September 29, 2020 15:20

Design proposal to revise port-forwarding behaviour

3810eb8

add pointer to corneliusweig's proposal

030f14f

briandealwis added !! do-not-merge !! kind/design discussion area/portforward area/debug area/dev labels Sep 29, 2020

briandealwis requested a review from a team as a code owner September 29, 2020 19:35

briandealwis requested a review from gsquared94 September 29, 2020 19:35

googlebot added the cla: yes label Sep 29, 2020

pull-request-size bot added the size/L label Sep 29, 2020

briandealwis added 2 commits September 30, 2020 15:00

Updates from design review meetings

1cac7f1

- `--port-forwarding` -> `--port-forward-modes` - created a table summarizing mode changes from v1.15.0 - clarified that `debug` was `skaffold debug`-related ports - added config settings to `skaffold.yaml` and global config

Fix the table

3ed71dc

nkubala reviewed Sep 30, 2020

View reviewed changes

Apply suggestions from code review

2c45f8a

Co-authored-by: Nick Kubala <nkubala@users.noreply.github.com>

gsquared94 reviewed Oct 13, 2020

View reviewed changes

docs/design_proposals/port-forward.md Show resolved Hide resolved

briandealwis added this to the v1.21.0-rc milestone Feb 16, 2021

tejal29 mentioned this pull request Feb 23, 2021

Feature request: Port forward for all ready resources #5318

Open

briandealwis mentioned this pull request Feb 24, 2021

Add support for no-option-value; surface per-option-defaults #5447

Merged

briandealwis mentioned this pull request Mar 14, 2021

Fix setting of defaults for flags #5548

Merged

drop defaults and global config -- need a comprehensive strategy

4f74bee

briandealwis removed the !! do-not-merge !! label Mar 16, 2021

Update status to under implementation

497ab40

briandealwis mentioned this pull request Mar 16, 2021

Revise port-forward behaviour #5554

Merged

nkubala approved these changes Mar 17, 2021

View reviewed changes

docs/design_proposals/port-forward.md Show resolved Hide resolved

Update port-forward.md

3dc6da5

briandealwis merged commit 4ddbaa2 into GoogleContainerTools:master Mar 17, 2021

briandealwis deleted the port-forward-dd branch March 17, 2021 18:03

gofish-bot mentioned this pull request Mar 19, 2021

skaffold 1.21.0 fishworks/fish-food#1297

Merged

briandealwis changed the title ~~Revise port-forwarding behaviour~~ [design] revise port-forwarding behaviour Mar 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[design] revise port-forwarding behaviour #4832

[design] revise port-forwarding behaviour #4832

briandealwis commented Sep 29, 2020 •

edited

codecov bot commented Sep 30, 2020 •

edited

nkubala Sep 30, 2020

briandealwis Oct 1, 2020

nkubala Sep 30, 2020

briandealwis Oct 1, 2020

gsquared94 Oct 13, 2020

gsquared94 Oct 13, 2020

briandealwis Oct 13, 2020

gsquared94 Oct 14, 2020

briandealwis Oct 14, 2020

MarlonGamez commented Feb 1, 2021

briandealwis commented Mar 16, 2021

nkubala commented Mar 17, 2021

briandealwis commented Mar 17, 2021


		#### skaffold.yaml

		We will define a new configuration block `defaults` with a single array field `portForwardModes`:


		#### Global config ($HOME/.skaffold/config)

		The global config will support a `portForwardModes` with a single array field. This can be set as a

[design] revise port-forwarding behaviour #4832

[design] revise port-forwarding behaviour #4832

Conversation

briandealwis commented Sep 29, 2020 • edited

codecov bot commented Sep 30, 2020 • edited

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarlonGamez commented Feb 1, 2021

briandealwis commented Mar 16, 2021

nkubala commented Mar 17, 2021

briandealwis commented Mar 17, 2021

briandealwis commented Sep 29, 2020 •

edited

codecov bot commented Sep 30, 2020 •

edited