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.

Sign up for GitHub

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

Don't look up services in all namespaces. #2651

Merged
merged 4 commits into from
Aug 15, 2019
Merged

Don't look up services in all namespaces. #2651

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3935711
Look up services in namespaces
tejal29 Aug 14, 2019
43ffde3
add tests
tejal29 Aug 14, 2019
edf02ff
add another test case for services with no ports exposeD
tejal29 Aug 14, 2019
14c87ac
fix lint
tejal29 Aug 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/skaffold/kubernetes/portforward/forwarder_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
44 changes: 25 additions & 19 deletions pkg/skaffold/kubernetes/portforward/resource_forwarder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
// services deployed by skaffold.
type ResourceForwarder struct {
EntryManager
namespaces []string
userDefinedResources []*latest.PortForwardResource
label string
}
Expand All @@ -40,12 +41,14 @@ 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
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,
}
Expand All @@ -54,7 +57,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")
}
Expand Down Expand Up @@ -100,27 +103,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) {
clientset, err := kubernetes.GetClientset()
func retrieveServiceResources(label string, namespaces []string) ([]*latest.PortForwardResource, error) {
clientset, err := kClientSet()
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
Expand Down
115 changes: 110 additions & 5 deletions pkg/skaffold/kubernetes/portforward/resource_forwarder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -116,11 +123,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
})

Expand Down Expand Up @@ -189,7 +196,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{},
Expand Down Expand Up @@ -232,11 +239,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
})

Expand All @@ -252,3 +259,101 @@ 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}}},
},
},
}, {
description: "services present but does not expose any port",
Copy link
Contributor

Choose a reason for hiding this comment

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

is this possible for a service?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you wont normally do it. But yes, you can not have any ports defined in the Spec and the service is created.

$ kubectl apply -f microservices/leeroy-app/kubernetes/
service/leeroy-app created
deployment.apps/leeroy-app created

$ git diff microservices/leeroy-app/kubernetes/
diff --git a/examples/microservices/leeroy-app/kubernetes/deployment.yaml b/examples/microservices/leeroy-app/kubernetes/deployment.yaml
index 56ef7152..46feab6d 100644
--- a/examples/microservices/leeroy-app/kubernetes/deployment.yaml
+++ b/examples/microservices/leeroy-app/kubernetes/deployment.yaml
@@ -6,9 +6,6 @@ metadata:
     app: leeroy-app
 spec:
   clusterIP: None
-  ports:
-    - port: 50051
-      name: leeroy-app
   selector:
     app: leeroy-app
 ---
tejaldesai@@examples (move_resolve_namespace)$

namespaces: []string{"test"},
services: []*v1.Service{
{
ObjectMeta: metav1.ObjectMeta{
Name: "svc1",
Namespace: "test",
Labels: map[string]string{
deploy.RunIDLabel: "9876-6789",
},
},
},
},
},
}

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)
})
}
}