Fix 4950: User-defined port forwarding resources ignore the namespace flag #4987

anshlykov · 2020-11-02T17:19:13Z

Fixes: #4950
Description

User facing changes (remove if N/A)

codecov · 2020-11-02T17:28:45Z

Codecov Report

Merging #4987 into master will decrease coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4987      +/-   ##
==========================================
- Coverage   72.27%   72.18%   -0.09%     
==========================================
  Files         361      362       +1     
  Lines       12640    12718      +78     
==========================================
+ Hits         9135     9180      +45     
- Misses       2827     2856      +29     
- Partials      678      682       +4

Impacted Files	Coverage Δ
pkg/skaffold/schema/defaults/defaults.go	`87.34% <ø> (-1.75%)`	⬇️
...ffold/kubernetes/portforward/resource_forwarder.go	`86.00% <100.00%> (+3.50%)`	⬆️
pkg/skaffold/runner/build_deploy.go	`68.88% <100.00%> (ø)`
pkg/skaffold/runner/dev.go	`67.66% <100.00%> (ø)`
pkg/skaffold/initializer/build/resolve.go	`79.16% <0.00%> (-5.45%)`	⬇️
...g/skaffold/kubernetes/portforward/entry_manager.go	`92.00% <0.00%> (-3.84%)`	⬇️
...affold/kubernetes/portforward/kubectl_forwarder.go	`66.42% <0.00%> (-1.61%)`	⬇️
pkg/skaffold/docker/docker_init.go	`100.00% <0.00%> (ø)`
pkg/skaffold/build/buildpacks/types.go	`100.00% <0.00%> (ø)`
... and 7 more

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 cd2a38f...07da064. Read the comment docs.

tejal29 · 2020-11-02T18:27:53Z

pkg/skaffold/kubernetes/portforward/resource_forwarder_test.go


-		fakeForwarder := newTestForwarder()
-		entryManager := NewEntryManager(ioutil.Discard, fakeForwarder)
+			fakeForwarder := newTestForwarder()


unintentional tab.

Sorry I'm not sure about this. I have shifted the whole nested function body because of the loop above

oops. sorry i missed you ended up shifting.I thought the IDE replaced spaces by tabs.

tejal29

Changes look good to me except some nit.

tejal29 · 2020-11-02T18:32:35Z

Looks like an integration test failure here https://travis-ci.com/github/GoogleContainerTools/skaffold/jobs/424309654#L2210

anshlykov · 2020-11-02T20:24:20Z

thanks for the quick feedback!

Looks like an integration test failure here https://travis-ci.com/github/GoogleContainerTools/skaffold/jobs/424309654#L2210

The test failed because events may occur in a different order than I expected. I have split this test.

tejal29 · 2020-11-02T23:47:40Z

thanks for the quick feedback!

Looks like an integration test failure here https://travis-ci.com/github/GoogleContainerTools/skaffold/jobs/424309654#L2210

The test failed because events may occur in a different order than I expected. I have split this test.

Thanks for digging deeper into this!

tejal29 · 2020-11-03T00:24:21Z

TODO: manual verification.

tejal29 · 2020-11-03T02:18:23Z

Testing notes:

run skaffold dev --port-forward --namespace=test

Starting deploy...
 - deployment.apps/leeroy-web created
 - service/leeroy-app created
 - deployment.apps/leeroy-app created
Waiting for deployments to stabilize...

DEBU[0003] Running command: [kubectl --context minikube --namespace test port-forward --pod-running-timeout 1s --namespace test pod/leeroy-app-f8f6d6864-d6kl2 50053:50051] 
[leeroy-web] 2020/11/03 02:12:01 leeroy web server ready
[leeroy-app] 2020/11/03 02:12:01 leeroy app server ready
kubectl --context minikube --namespace test port-forward --pod-running-timeout 1s --namespace test pod/leeroy-app-f8f6d6864-d6kl2 50053:5005

Force multiple namespace by forcing leeroy-app to deploy in default namespace and test namespace.

diff --git a/examples/microservices/leeroy-app/kubernetes/deployment.yaml b/examples/microservices/leeroy-app/kubernetes/deployment.yaml
index f416632e9..8e23227f5 100644
--- a/examples/microservices/leeroy-app/kubernetes/deployment.yaml
+++ b/examples/microservices/leeroy-app/kubernetes/deployment.yaml
@@ -2,6 +2,7 @@ apiVersion: v1
 kind: Service
 metadata:
   name: leeroy-app
+  namespace: test
   labels:
     app: leeroy-app
 spec:
diff --git a/examples/microservices/leeroy-web/kubernetes/deployment.yaml b/examples/microservices/leeroy-web/kubernetes/deployment.yaml
index 1aa0c83fb..2adddd879 100644
--- a/examples/microservices/leeroy-web/kubernetes/deployment.yaml
+++ b/examples/microservices/leeroy-web/kubernetes/deployment.yaml
@@ -2,6 +2,7 @@ apiVersion: apps/v1
 kind: Deployment
 metadata:
   name: leeroy-web
+  namespace: default
   labels:
     app: leeroy-web
 spec:
(END)

I was expecting to see warning. "l"Skipping the port forwarding resource .." but did not.

➜  microservices git:(gh-4950) ✗ ../../out/skaffold dev --port-forward                 
Listing files to watch...
 - leeroy-web
 - leeroy-app
 - base
Generating tags...
 - leeroy-web -> leeroy-web:v1.16.0-3-g859506fd9-dirty
 - leeroy-app -> leeroy-app:v1.16.0-3-g859506fd9-dirty
 - base -> base:v1.16.0-3-g859506fd9
Checking cache...
 - leeroy-web: Found Locally
 - leeroy-app: Found. Tagging
 - base: Found Locally
Tags used in deployment:
 - leeroy-web -> leeroy-web:0db6d33d7f4ebbe382edd658469fec36541078ec410b8db8d3cac5c6ea221abe
 - leeroy-app -> leeroy-app:f6fc96236b1df222c8459d5233dbb38565ddba6dfb681131face2ce20c34cfae
 - base -> base:2a2453ad90bdf6c13da8609b87861756ebfba92505423efee54c2787032f58cd
Starting deploy...
WARN[0000] image [base] is not used by the deployment   
 - deployment.apps/leeroy-web created
 - service/leeroy-app created
 - deployment.apps/leeroy-app created
Waiting for deployments to stabilize...
 - deployment/leeroy-web is ready. [1/2 deployment(s) still pending]
 - deployment/leeroy-app is ready.
Deployments stabilized in 2.542367437s
Port forwarding deployment/leeroy-web in namespace , remote port 8080 -> address 127.0.0.1 port 9000
Port forwarding service/leeroy-app in namespace test, remote port 50051 -> address 127.0.0.1 port 50053

Will dig deeper tomorrow.

tejal29 · 2020-11-03T05:45:25Z

Looks like skaffold creates a forward manager instance with namespace from command line or kubectx before deploy.
We update the namespaces from the deployed resources only after deploy.

Applying the following diff fixes the issue.

+++ b/pkg/skaffold/runner/dev.go
@@ -212,10 +212,7 @@ func (r *SkaffoldRunner) Dev(ctx context.Context, out io.Writer, artifacts []*la
 
        logger := r.createLogger(out, bRes)
        defer logger.Stop()

-       forwarderManager := r.createForwarder(out)
-       defer forwarderManager.Stop()
-  
        debugContainerManager := r.createContainerManager()
        defer debugContainerManager.Stop()
 
@@ -228,6 +225,9 @@ func (r *SkaffoldRunner) Dev(ctx context.Context, out io.Writer, artifacts []*la
                return fmt.Errorf("exiting dev mode because first deploy failed: %w", err)
        }
 
+       forwarderManager := r.createForwarder(out)
+       defer forwarderManager.Stop()
+

We need to apply similar patch in build_deploy.go

anshlykov · 2020-11-03T14:23:45Z

Thanks! I have added these changes and am working on tests

update the port forward resource spec if the namespace is not specified

61be6ab

google-cla bot added the cla: yes label Nov 2, 2020

pull-request-size bot added the size/L label Nov 2, 2020

tejal29 reviewed Nov 2, 2020

View reviewed changes

fix IT

859506f

anshlykov marked this pull request as ready for review November 2, 2020 20:24

anshlykov requested a review from a team as a code owner November 2, 2020 20:24

anshlykov requested a review from gsquared94 November 2, 2020 20:24

tejal29 added the kokoro:run runs the kokoro jobs on a PR label Nov 3, 2020

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Nov 3, 2020

create forwarderManager after deploy

07da064

tejal29 added the kokoro:run runs the kokoro jobs on a PR label Nov 3, 2020

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Nov 3, 2020

tejal29 added the kokoro:run runs the kokoro jobs on a PR label Nov 3, 2020

tejal29 approved these changes Nov 3, 2020

View reviewed changes

tejal29 merged commit 884f1f8 into GoogleContainerTools:master Nov 3, 2020

anshlykov deleted the gh-4950 branch November 11, 2020 14:25

fmotrifork mentioned this pull request Nov 25, 2020

skaffold 1.17.0 fishworks/fish-food#1082

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 4950: User-defined port forwarding resources ignore the namespace flag #4987

Fix 4950: User-defined port forwarding resources ignore the namespace flag #4987

anshlykov commented Nov 2, 2020

codecov bot commented Nov 2, 2020 •

edited

Loading

tejal29 Nov 2, 2020

anshlykov Nov 2, 2020

tejal29 Nov 3, 2020

tejal29 left a comment

tejal29 commented Nov 2, 2020

anshlykov commented Nov 2, 2020

tejal29 commented Nov 2, 2020

tejal29 commented Nov 3, 2020

tejal29 commented Nov 3, 2020

tejal29 commented Nov 3, 2020 •

edited

Loading

anshlykov commented Nov 3, 2020

Fix 4950: User-defined port forwarding resources ignore the namespace flag #4987

Fix 4950: User-defined port forwarding resources ignore the namespace flag #4987

Conversation

anshlykov commented Nov 2, 2020

codecov bot commented Nov 2, 2020 • edited Loading

Codecov Report

tejal29 Nov 2, 2020

Choose a reason for hiding this comment

anshlykov Nov 2, 2020

Choose a reason for hiding this comment

tejal29 Nov 3, 2020

Choose a reason for hiding this comment

tejal29 left a comment

Choose a reason for hiding this comment

tejal29 commented Nov 2, 2020

anshlykov commented Nov 2, 2020

tejal29 commented Nov 2, 2020

tejal29 commented Nov 3, 2020

tejal29 commented Nov 3, 2020

tejal29 commented Nov 3, 2020 • edited Loading

anshlykov commented Nov 3, 2020

codecov bot commented Nov 2, 2020 •

edited

Loading

tejal29 commented Nov 3, 2020 •

edited

Loading