From 39357118870a6dca2239d467950c72ab1909a16e Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Wed, 14 Aug 2019 14:24:20 -0700 Subject: [PATCH 1/4] Look up services in namespaces --- .../portforward/forwarder_manager.go | 2 +- .../portforward/resource_forwarder.go | 41 +++++++++++-------- .../portforward/resource_forwarder_test.go | 10 ++--- 3 files changed, 29 insertions(+), 24 deletions(-) diff --git a/pkg/skaffold/kubernetes/portforward/forwarder_manager.go b/pkg/skaffold/kubernetes/portforward/forwarder_manager.go index a80b7d191b6..d876f56d963 100644 --- a/pkg/skaffold/kubernetes/portforward/forwarder_manager.go +++ b/pkg/skaffold/kubernetes/portforward/forwarder_manager.go @@ -54,7 +54,7 @@ func NewForwarderManager(out io.Writer, cli *kubectl.CLI, podSelector kubernetes ForwarderManager := &ForwarderManager{ output: out, - Forwarders: []Forwarder{NewResourceForwarder(em, label, userDefined)}, + Forwarders: []Forwarder{NewResourceForwarder(em, namespaces, label, userDefined)}, } if opts.ForwardPods { diff --git a/pkg/skaffold/kubernetes/portforward/resource_forwarder.go b/pkg/skaffold/kubernetes/portforward/resource_forwarder.go index f7754ed2db7..263ac0340af 100644 --- a/pkg/skaffold/kubernetes/portforward/resource_forwarder.go +++ b/pkg/skaffold/kubernetes/portforward/resource_forwarder.go @@ -32,6 +32,7 @@ import ( // services deployed by skaffold. type ResourceForwarder struct { EntryManager + namespaces []string userDefinedResources []*latest.PortForwardResource label string } @@ -43,9 +44,10 @@ var ( ) // NewResourceForwarder returns a struct that tracks and port-forwards pods as they are created and modified -func NewResourceForwarder(em EntryManager, label string, userDefinedResources []*latest.PortForwardResource) *ResourceForwarder { +func NewResourceForwarder(em EntryManager, namespaces []string, label string, userDefinedResources []*latest.PortForwardResource) *ResourceForwarder { return &ResourceForwarder{ EntryManager: em, + namespaces: namespaces, userDefinedResources: userDefinedResources, label: label, } @@ -54,7 +56,7 @@ func NewResourceForwarder(em EntryManager, label string, userDefinedResources [] // Start gets a list of services deployed by skaffold as []latest.PortForwardResource and // forwards them. func (p *ResourceForwarder) Start(ctx context.Context) error { - serviceResources, err := retrieveServices(p.label) + serviceResources, err := retrieveServices(p.label, p.namespaces) if err != nil { return errors.Wrap(err, "retrieving services for automatic port forwarding") } @@ -100,27 +102,30 @@ func (p *ResourceForwarder) getCurrentEntry(resource latest.PortForwardResource) // retrieveServiceResources retrieves all services in the cluster matching the given label // as a list of PortForwardResources -func retrieveServiceResources(label string) ([]*latest.PortForwardResource, error) { +func retrieveServiceResources(label string, namespaces []string) ([]*latest.PortForwardResource, error) { clientset, err := kubernetes.GetClientset() if err != nil { return nil, errors.Wrap(err, "getting clientset") } - services, err := clientset.CoreV1().Services("").List(metav1.ListOptions{ - LabelSelector: label, - }) - if err != nil { - return nil, errors.Wrapf(err, "selecting services by label %s", label) - } + var resources []*latest.PortForwardResource - for _, s := range services.Items { - for _, p := range s.Spec.Ports { - resources = append(resources, &latest.PortForwardResource{ - Type: constants.Service, - Name: s.Name, - Namespace: s.Namespace, - Port: int(p.Port), - LocalPort: int(p.Port), - }) + for _, ns := range namespaces { + services, err := clientset.CoreV1().Services(ns).List(metav1.ListOptions{ + LabelSelector: label, + }) + if err != nil { + return nil, errors.Wrapf(err, "selecting services by label %s", label) + } + for _, s := range services.Items { + for _, p := range s.Spec.Ports { + resources = append(resources, &latest.PortForwardResource{ + Type: constants.Service, + Name: s.Name, + Namespace: s.Namespace, + Port: int(p.Port), + LocalPort: int(p.Port), + }) + } } } return resources, nil diff --git a/pkg/skaffold/kubernetes/portforward/resource_forwarder_test.go b/pkg/skaffold/kubernetes/portforward/resource_forwarder_test.go index 283635da82c..b82f558bd60 100644 --- a/pkg/skaffold/kubernetes/portforward/resource_forwarder_test.go +++ b/pkg/skaffold/kubernetes/portforward/resource_forwarder_test.go @@ -116,11 +116,11 @@ func TestStart(t *testing.T) { testutil.Run(t, test.description, func(t *testutil.T) { event.InitializeState(latest.BuildConfig{}) fakeForwarder := newTestForwarder() - rf := NewResourceForwarder(NewEntryManager(ioutil.Discard, nil), "", nil) + rf := NewResourceForwarder(NewEntryManager(ioutil.Discard, nil), []string{"test"}, "", nil) rf.EntryForwarder = fakeForwarder t.Override(&retrieveAvailablePort, mockRetrieveAvailablePort(map[int]struct{}{}, test.availablePorts)) - t.Override(&retrieveServices, func(string) ([]*latest.PortForwardResource, error) { + t.Override(&retrieveServices, func(string, []string) ([]*latest.PortForwardResource, error) { return test.resources, nil }) @@ -189,7 +189,7 @@ func TestGetCurrentEntryFunc(t *testing.T) { expectedEntry := test.expected expectedEntry.resource = test.resource - rf := NewResourceForwarder(NewEntryManager(ioutil.Discard, nil), "", nil) + rf := NewResourceForwarder(NewEntryManager(ioutil.Discard, nil), []string{"test"}, "", nil) rf.forwardedResources = forwardedResources{ resources: test.forwardedResources, lock: &sync.Mutex{}, @@ -232,11 +232,11 @@ func TestUserDefinedResources(t *testing.T) { testutil.Run(t, "one service and one user defined pod", func(t *testutil.T) { event.InitializeState(latest.BuildConfig{}) fakeForwarder := newTestForwarder() - rf := NewResourceForwarder(NewEntryManager(ioutil.Discard, nil), "", []*latest.PortForwardResource{pod}) + rf := NewResourceForwarder(NewEntryManager(ioutil.Discard, nil), []string{"test"}, "", []*latest.PortForwardResource{pod}) rf.EntryForwarder = fakeForwarder t.Override(&retrieveAvailablePort, mockRetrieveAvailablePort(map[int]struct{}{}, []int{8080, 9000})) - t.Override(&retrieveServices, func(string) ([]*latest.PortForwardResource, error) { + t.Override(&retrieveServices, func(string, []string) ([]*latest.PortForwardResource, error) { return []*latest.PortForwardResource{svc}, nil }) From 43ffde3b649e2f1e42699a6f7fad8437e5c07d02 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Wed, 14 Aug 2019 15:22:13 -0700 Subject: [PATCH 2/4] add tests --- .../portforward/resource_forwarder.go | 3 +- .../portforward/resource_forwarder_test.go | 91 +++++++++++++++++++ 2 files changed, 93 insertions(+), 1 deletion(-) diff --git a/pkg/skaffold/kubernetes/portforward/resource_forwarder.go b/pkg/skaffold/kubernetes/portforward/resource_forwarder.go index 263ac0340af..e958acccae1 100644 --- a/pkg/skaffold/kubernetes/portforward/resource_forwarder.go +++ b/pkg/skaffold/kubernetes/portforward/resource_forwarder.go @@ -41,6 +41,7 @@ var ( // For testing retrieveAvailablePort = util.GetAvailablePort retrieveServices = retrieveServiceResources + kClientSet = kubernetes.GetClientset ) // NewResourceForwarder returns a struct that tracks and port-forwards pods as they are created and modified @@ -103,7 +104,7 @@ func (p *ResourceForwarder) getCurrentEntry(resource latest.PortForwardResource) // retrieveServiceResources retrieves all services in the cluster matching the given label // as a list of PortForwardResources func retrieveServiceResources(label string, namespaces []string) ([]*latest.PortForwardResource, error) { - clientset, err := kubernetes.GetClientset() + clientset, err := kClientSet() if err != nil { return nil, errors.Wrap(err, "getting clientset") } diff --git a/pkg/skaffold/kubernetes/portforward/resource_forwarder_test.go b/pkg/skaffold/kubernetes/portforward/resource_forwarder_test.go index b82f558bd60..d4487eba0e6 100644 --- a/pkg/skaffold/kubernetes/portforward/resource_forwarder_test.go +++ b/pkg/skaffold/kubernetes/portforward/resource_forwarder_test.go @@ -18,19 +18,26 @@ package portforward import ( "context" + "fmt" "io/ioutil" "sync" "testing" "time" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy" "github.com/google/go-cmp/cmp" + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/kubernetes" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/event" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" "github.com/GoogleContainerTools/skaffold/testutil" + fakekubeclientset "k8s.io/client-go/kubernetes/fake" ) type testForwarder struct { @@ -252,3 +259,87 @@ func TestUserDefinedResources(t *testing.T) { } }) } + +func mockClient(m kubernetes.Interface) func() (kubernetes.Interface, error) { + return func() (kubernetes.Interface, error) { + return m, nil + } + +} + +func TestRetrieveServices(t *testing.T) { + tests := []struct { + description string + namespaces []string + services []*v1.Service + expected []*latest.PortForwardResource + }{ + { + description: "multiple services in multiple namespaces", + namespaces: []string{"test", "test1"}, + services: []*v1.Service{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "svc1", + Namespace: "test", + Labels: map[string]string{ + deploy.RunIDLabel: "9876-6789", + }, + }, + Spec: v1.ServiceSpec{Ports: []v1.ServicePort{{Port: 8080}}}, + }, { + ObjectMeta: metav1.ObjectMeta{ + Name: "svc2", + Namespace: "test1", + Labels: map[string]string{ + deploy.RunIDLabel: "9876-6789", + }, + }, + Spec: v1.ServiceSpec{Ports: []v1.ServicePort{{Port: 8081}}}, + }, + }, + expected: []*latest.PortForwardResource{{ + Type: constants.Service, + Name: "svc1", + Namespace: "test", + Port: 8080, + LocalPort: 8080, + }, { + Type: constants.Service, + Name: "svc2", + Namespace: "test1", + Port: 8081, + LocalPort: 8081, + }}, + }, { + description: "no services in given namespace", + namespaces: []string{"randon"}, + services: []*v1.Service{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "svc1", + Namespace: "test", + Labels: map[string]string{ + deploy.RunIDLabel: "9876-6789", + }, + }, + Spec: v1.ServiceSpec{Ports: []v1.ServicePort{{Port: 8080}}}, + }, + }, + }, + } + + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + objs := make([]runtime.Object, len(test.services)) + for i, s := range test.services { + objs[i] = s + } + client := fakekubeclientset.NewSimpleClientset(objs...) + t.Override(&kClientSet, mockClient(client)) + actual, err := retrieveServiceResources(fmt.Sprintf("%s=9876-6789", deploy.RunIDLabel), test.namespaces) + t.CheckNoError(err) + t.CheckDeepEqual(test.expected, actual) + }) + } +} From edf02ffdf73ae0901a664d690604c6e9232b030d Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Wed, 14 Aug 2019 15:24:44 -0700 Subject: [PATCH 3/4] add another test case for services with no ports exposeD --- .../portforward/resource_forwarder_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/pkg/skaffold/kubernetes/portforward/resource_forwarder_test.go b/pkg/skaffold/kubernetes/portforward/resource_forwarder_test.go index d4487eba0e6..648e5ebf757 100644 --- a/pkg/skaffold/kubernetes/portforward/resource_forwarder_test.go +++ b/pkg/skaffold/kubernetes/portforward/resource_forwarder_test.go @@ -326,6 +326,20 @@ func TestRetrieveServices(t *testing.T) { Spec: v1.ServiceSpec{Ports: []v1.ServicePort{{Port: 8080}}}, }, }, + }, { + description: "services present but does not expose any port", + namespaces: []string{"test"}, + services: []*v1.Service{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "svc1", + Namespace: "test", + Labels: map[string]string{ + deploy.RunIDLabel: "9876-6789", + }, + }, + }, + }, }, } From 14c87acea0477697855b0b4ea7c04f4c9b4bd4c0 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Wed, 14 Aug 2019 16:43:28 -0700 Subject: [PATCH 4/4] fix lint --- pkg/skaffold/kubernetes/portforward/resource_forwarder.go | 4 ++-- .../kubernetes/portforward/resource_forwarder_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/skaffold/kubernetes/portforward/resource_forwarder.go b/pkg/skaffold/kubernetes/portforward/resource_forwarder.go index e958acccae1..c5fbf7d1ec3 100644 --- a/pkg/skaffold/kubernetes/portforward/resource_forwarder.go +++ b/pkg/skaffold/kubernetes/portforward/resource_forwarder.go @@ -41,7 +41,7 @@ var ( // For testing retrieveAvailablePort = util.GetAvailablePort retrieveServices = retrieveServiceResources - kClientSet = kubernetes.GetClientset + getClientSet = kubernetes.GetClientset ) // NewResourceForwarder returns a struct that tracks and port-forwards pods as they are created and modified @@ -104,7 +104,7 @@ func (p *ResourceForwarder) getCurrentEntry(resource latest.PortForwardResource) // retrieveServiceResources retrieves all services in the cluster matching the given label // as a list of PortForwardResources func retrieveServiceResources(label string, namespaces []string) ([]*latest.PortForwardResource, error) { - clientset, err := kClientSet() + clientset, err := getClientSet() if err != nil { return nil, errors.Wrap(err, "getting clientset") } diff --git a/pkg/skaffold/kubernetes/portforward/resource_forwarder_test.go b/pkg/skaffold/kubernetes/portforward/resource_forwarder_test.go index 648e5ebf757..4d7db102a84 100644 --- a/pkg/skaffold/kubernetes/portforward/resource_forwarder_test.go +++ b/pkg/skaffold/kubernetes/portforward/resource_forwarder_test.go @@ -350,7 +350,7 @@ func TestRetrieveServices(t *testing.T) { objs[i] = s } client := fakekubeclientset.NewSimpleClientset(objs...) - t.Override(&kClientSet, mockClient(client)) + t.Override(&getClientSet, mockClient(client)) actual, err := retrieveServiceResources(fmt.Sprintf("%s=9876-6789", deploy.RunIDLabel), test.namespaces) t.CheckNoError(err) t.CheckDeepEqual(test.expected, actual)