From ea0c6bcbfdc47528e713de2779abea4b3e54c49b Mon Sep 17 00:00:00 2001 From: Claire Li Date: Wed, 5 Nov 2014 22:38:06 -0800 Subject: [PATCH 1/3] Refactor findPort in service pkg --- pkg/service/endpoints_controller.go | 33 ++++++++++++++++++----------- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/pkg/service/endpoints_controller.go b/pkg/service/endpoints_controller.go index 93eddb6f893..8b943591df7 100644 --- a/pkg/service/endpoints_controller.go +++ b/pkg/service/endpoints_controller.go @@ -138,21 +138,30 @@ func endpointsEqual(e *api.Endpoints, endpoints []string) bool { // findPort locates the container port for the given manifest and portName. func findPort(manifest *api.ContainerManifest, portName util.IntOrString) (int, error) { - if ((portName.Kind == util.IntstrString && len(portName.StrVal) == 0) || - (portName.Kind == util.IntstrInt && portName.IntVal == 0)) && - len(manifest.Containers[0].Ports) > 0 { - return manifest.Containers[0].Ports[0].ContainerPort, nil + firstContianerPort := -1 + if len(manifest.Containers[0].Ports) > 0 { + firstContianerPort = manifest.Containers[0].Ports[0].ContainerPort } - if portName.Kind == util.IntstrInt { - return portName.IntVal, nil - } - name := portName.StrVal - for _, container := range manifest.Containers { - for _, port := range container.Ports { - if port.Name == name { - return port.ContainerPort, nil + + switch portName.Kind { + case util.IntstrString: + if len(portName.StrVal) == 0 && firstContianerPort != -1 { + return firstContianerPort, nil + } + name := portName.StrVal + for _, container := range manifest.Containers { + for _, port := range container.Ports { + if port.Name == name { + return port.ContainerPort, nil + } } } + case util.IntstrInt: + if portName.IntVal == 0 && firstContianerPort != -1 { + return firstContianerPort, nil + } + return portName.IntVal, nil } + return -1, fmt.Errorf("no suitable port for manifest: %s", manifest.ID) } From c5205870c0df63f0ae711245febbff08502cfad0 Mon Sep 17 00:00:00 2001 From: Claire Li Date: Wed, 5 Nov 2014 22:38:34 -0800 Subject: [PATCH 2/3] Refactor test for findPort in service pkg --- pkg/service/endpoints_controller.go | 14 ++-- pkg/service/endpoints_controller_test.go | 90 ++++++++++++++---------- 2 files changed, 59 insertions(+), 45 deletions(-) diff --git a/pkg/service/endpoints_controller.go b/pkg/service/endpoints_controller.go index 8b943591df7..75b597a4ad5 100644 --- a/pkg/service/endpoints_controller.go +++ b/pkg/service/endpoints_controller.go @@ -138,15 +138,15 @@ func endpointsEqual(e *api.Endpoints, endpoints []string) bool { // findPort locates the container port for the given manifest and portName. func findPort(manifest *api.ContainerManifest, portName util.IntOrString) (int, error) { - firstContianerPort := -1 + firstContainerPort := 0 if len(manifest.Containers[0].Ports) > 0 { - firstContianerPort = manifest.Containers[0].Ports[0].ContainerPort + firstContainerPort = manifest.Containers[0].Ports[0].ContainerPort } switch portName.Kind { case util.IntstrString: - if len(portName.StrVal) == 0 && firstContianerPort != -1 { - return firstContianerPort, nil + if len(portName.StrVal) == 0 && firstContainerPort != -1 { + return firstContainerPort, nil } name := portName.StrVal for _, container := range manifest.Containers { @@ -157,11 +157,11 @@ func findPort(manifest *api.ContainerManifest, portName util.IntOrString) (int, } } case util.IntstrInt: - if portName.IntVal == 0 && firstContianerPort != -1 { - return firstContianerPort, nil + if portName.IntVal == 0 && firstContainerPort != -1 { + return firstContainerPort, nil } return portName.IntVal, nil } - return -1, fmt.Errorf("no suitable port for manifest: %s", manifest.ID) + return 0, fmt.Errorf("no suitable port for manifest: %s", manifest.ID) } diff --git a/pkg/service/endpoints_controller_test.go b/pkg/service/endpoints_controller_test.go index 4833721304c..0850a38c495 100644 --- a/pkg/service/endpoints_controller_test.go +++ b/pkg/service/endpoints_controller_test.go @@ -79,45 +79,59 @@ func TestFindPort(t *testing.T) { }, }, } - port, err := findPort(&manifest, util.IntOrString{Kind: util.IntstrString, StrVal: "foo"}) - if err != nil { - t.Errorf("unexpected error: %v", err) + tests := []struct { + portName util.IntOrString + + wport int + werr bool + }{ + { + util.IntOrString{Kind: util.IntstrString, StrVal: "foo"}, + 8080, + false, + }, + { + util.IntOrString{Kind: util.IntstrString, StrVal: "bar"}, + 8000, + false, + }, + { + util.IntOrString{Kind: util.IntstrInt, IntVal: 8000}, + 8000, + false, + }, + { + util.IntOrString{Kind: util.IntstrInt, IntVal: 7000}, + 7000, + false, + }, + { + util.IntOrString{Kind: util.IntstrString, StrVal: "baz"}, + 0, + true, + }, + { + util.IntOrString{Kind: util.IntstrString, StrVal: ""}, + 8080, + false, + }, + { + util.IntOrString{}, + 8080, + false, + }, } - if port != 8080 { - t.Errorf("Expected 8080, Got %d", port) - } - port, err = findPort(&manifest, util.IntOrString{Kind: util.IntstrString, StrVal: "bar"}) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - if port != 8000 { - t.Errorf("Expected 8000, Got %d", port) - } - port, err = findPort(&manifest, util.IntOrString{Kind: util.IntstrInt, IntVal: 8000}) - if port != 8000 { - t.Errorf("Expected 8000, Got %d", port) - } - port, err = findPort(&manifest, util.IntOrString{Kind: util.IntstrInt, IntVal: 7000}) - if port != 7000 { - t.Errorf("Expected 7000, Got %d", port) - } - port, err = findPort(&manifest, util.IntOrString{Kind: util.IntstrString, StrVal: "baz"}) - if err == nil { - t.Error("unexpected non-error") - } - port, err = findPort(&manifest, util.IntOrString{Kind: util.IntstrString, StrVal: ""}) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - if port != 8080 { - t.Errorf("Expected 8080, Got %d", port) - } - port, err = findPort(&manifest, util.IntOrString{}) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - if port != 8080 { - t.Errorf("Expected 8080, Got %d", port) + for _, test := range tests { + port, err := findPort(&manifest, test.portName) + if port != test.wport { + t.Errorf("Expected port %d, Got %d", test.wport, port) + } + if err == nil && test.werr { + t.Errorf("unexpected non-error") + } + if err != nil && test.werr == false { + t.Errorf("unexpected error: %v", err) + } } } From 5a7c00754f676ceec713d9b8d3f0ca35e75327fa Mon Sep 17 00:00:00 2001 From: Claire Li Date: Wed, 5 Nov 2014 23:40:27 -0800 Subject: [PATCH 3/3] Fix findPort: correctly handle the empty ports case --- pkg/service/endpoints_controller.go | 14 ++++++++--- pkg/service/endpoints_controller_test.go | 31 ++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/pkg/service/endpoints_controller.go b/pkg/service/endpoints_controller.go index 75b597a4ad5..6fd0d963467 100644 --- a/pkg/service/endpoints_controller.go +++ b/pkg/service/endpoints_controller.go @@ -145,8 +145,11 @@ func findPort(manifest *api.ContainerManifest, portName util.IntOrString) (int, switch portName.Kind { case util.IntstrString: - if len(portName.StrVal) == 0 && firstContainerPort != -1 { - return firstContainerPort, nil + if len(portName.StrVal) == 0 { + if firstContainerPort != 0 { + return firstContainerPort, nil + } + break } name := portName.StrVal for _, container := range manifest.Containers { @@ -157,8 +160,11 @@ func findPort(manifest *api.ContainerManifest, portName util.IntOrString) (int, } } case util.IntstrInt: - if portName.IntVal == 0 && firstContainerPort != -1 { - return firstContainerPort, nil + if portName.IntVal == 0 { + if firstContainerPort != 0 { + return firstContainerPort, nil + } + break } return portName.IntVal, nil } diff --git a/pkg/service/endpoints_controller_test.go b/pkg/service/endpoints_controller_test.go index 0850a38c495..e456bd3c97a 100644 --- a/pkg/service/endpoints_controller_test.go +++ b/pkg/service/endpoints_controller_test.go @@ -79,50 +79,77 @@ func TestFindPort(t *testing.T) { }, }, } + emptyPortsManifest := api.ContainerManifest{ + Containers: []api.Container{ + { + Ports: []api.Port{}, + }, + }, + } tests := []struct { + manifest api.ContainerManifest portName util.IntOrString wport int werr bool }{ { + manifest, util.IntOrString{Kind: util.IntstrString, StrVal: "foo"}, 8080, false, }, { + manifest, util.IntOrString{Kind: util.IntstrString, StrVal: "bar"}, 8000, false, }, { + manifest, util.IntOrString{Kind: util.IntstrInt, IntVal: 8000}, 8000, false, }, { + manifest, util.IntOrString{Kind: util.IntstrInt, IntVal: 7000}, 7000, false, }, { + manifest, util.IntOrString{Kind: util.IntstrString, StrVal: "baz"}, 0, true, }, { + manifest, util.IntOrString{Kind: util.IntstrString, StrVal: ""}, 8080, false, }, { - util.IntOrString{}, + manifest, + util.IntOrString{Kind: util.IntstrInt, IntVal: 0}, 8080, false, }, + { + emptyPortsManifest, + util.IntOrString{Kind: util.IntstrString, StrVal: ""}, + 0, + true, + }, + { + emptyPortsManifest, + util.IntOrString{Kind: util.IntstrInt, IntVal: 0}, + 0, + true, + }, } for _, test := range tests { - port, err := findPort(&manifest, test.portName) + port, err := findPort(&test.manifest, test.portName) if port != test.wport { t.Errorf("Expected port %d, Got %d", test.wport, port) }