Add ability to change prefix in the Namer #74

bowei · 2017-11-07T18:38:36Z

This allows the multicluster code to share use of the Namer.
Add more unit testing
Coverage is now ~90% of namer code.

nikhiljindal · 2017-11-07T18:43:15Z

pkg/utils/namer.go

-	sslCertPrefix          = "k8s-ssl"
+	targetHTTPProxyPrefix  = "tp"
+	targetHTTPSProxyPrefix = "tps"
+	sslCertPrefix          = "ssl"
 	// TODO: this should really be "fr" and "frs".


Doesnt firewall rule use "fr"?

nikhiljindal · 2017-11-07T18:49:58Z

This is great! thx, lgtm

bowei · 2017-11-07T18:57:02Z

fixing the test failure related to instance groups

bowei · 2017-11-09T06:59:28Z

The second patch is a little large but it's basically making all of the unit tests and fakes use the Namer consistently.

nikhiljindal · 2017-11-09T07:14:37Z

pkg/controller/cluster_manager.go

@@ -36,16 +36,6 @@ const (
 	defaultPort            = 80
 	defaultHealthCheckPath = "/"

-	// A backend is created per nodePort, tagged with the nodeport.


Love this change!

nikhiljindal · 2017-11-09T07:14:59Z

pkg/controller/controller_test.go

@@ -50,7 +50,7 @@ var (

 // TODO: Use utils.Namer instead of this function.
 func defaultBackendName(clusterName string) string {
-	return fmt.Sprintf("%v-%v", backendPrefix, clusterName)
+	return fmt.Sprintf("%v-%v", "k8s-be", clusterName) // backendPrefix, clusterName) XXX


XXX is intentional? :)

Also dont like "k8s-be" being hardcoded here.
Remove this method altogether to use Namer?

yes, this needs to be fixed

nikhiljindal · 2017-11-09T07:18:22Z

pkg/healthchecks/healthchecks_test.go

@@ -54,7 +54,7 @@ func TestHealthCheckAdd(t *testing.T) {
 }

 func TestHealthCheckAddExisting(t *testing.T) {
-	namer := &utils.Namer{}
+	namer := utils.NewNamer("uid1", "fw1")


Maybe define a utils.NewTestNamer() rather than duplicating "uid1", "fw1" it in all these tests?

You can define it one of the fakes.go?

ping, what do you think of instantiating once and reusing in all?

nikhiljindal · 2017-11-09T07:18:48Z

pkg/healthchecks/healthchecks_test.go

@@ -202,7 +202,7 @@ func TestHealthCheckDeleteLegacy(t *testing.T) {
 }

 func TestAlphaHealthCheck(t *testing.T) {
-	namer := &utils.Namer{}
+	namer := utils.NewNamer("uid1", "fw1")


Do these really need a separate instance?

I see instances_test.go uses a single instance in all test cases. This uses separate. Lets be consistent.
Instantiate once and reuse it in all tests

nikhiljindal · 2017-11-09T07:20:03Z

pkg/loadbalancers/fakes.go

@@ -18,6 +18,8 @@ package loadbalancers

 import (
 	"fmt"
+	"github.com/golang/glog"
+	"testing"


No!!!!
This is reverting #69. Please fix

Can we move the method to the test or a different package?

Moving the method to test is fine by me. As long as non test code does not depend on testing.

Quite funny I think I exactly reverted the change.

nikhiljindal · 2017-11-09T07:23:50Z

pkg/loadbalancers/loadbalancers_test.go

 	lbInfo := &L7RuntimeInfo{
-		Name:      "test",


Do things break without this change?
Seems like you have fixed all the instances. We can keep things simple where a namer generated name is not required?

Things do break and the inconsistent use of names is quite problematic.

bowei · 2017-11-28T01:32:34Z

@nikhiljindal -- took care of most of the comments. Some of the test clean I'm going to put in a follow on, the change is getting somewhat large.

nikhiljindal · 2017-12-04T06:47:52Z

pkg/controller/controller_test.go

@@ -48,9 +48,8 @@ var (
 	testIPManager = testIP{}
 )

-// TODO: Use utils.Namer instead of this function.


Why remove the TODO? It still seems relevant.

It isn't actually a Namer resource (it is not a GCP resource). The comment is incorrect.

ok got it, thx!

nikhiljindal · 2017-12-04T06:48:17Z

pkg/controller/controller_test.go

 func defaultBackendName(clusterName string) string {
-	return fmt.Sprintf("%v-%v", backendPrefix, clusterName)
+	return fmt.Sprintf("%v-%v", "k8s-be", clusterName)


(repeat of previous comment). Do not hardcode this here and reuse namer method?

See comment above.

ok, resolved.

nikhiljindal · 2017-12-04T06:50:46Z

pkg/healthchecks/healthchecks_test.go

@@ -202,7 +202,7 @@ func TestHealthCheckDeleteLegacy(t *testing.T) {
 }

 func TestAlphaHealthCheck(t *testing.T) {
-	namer := &utils.Namer{}
+	namer := utils.NewNamer("uid1", "fw1")


I see instances_test.go uses a single instance in all test cases. This uses separate. Lets be consistent.
Instantiate once and reuse it in all tests

nikhiljindal · 2017-12-04T06:51:07Z

pkg/healthchecks/healthchecks_test.go

@@ -54,7 +54,7 @@ func TestHealthCheckAdd(t *testing.T) {
 }

 func TestHealthCheckAddExisting(t *testing.T) {
-	namer := &utils.Namer{}
+	namer := utils.NewNamer("uid1", "fw1")


ping, what do you think of instantiating once and reusing in all?

nikhiljindal · 2017-12-04T06:52:11Z

pkg/loadbalancers/fake/fakes.go

@@ -14,14 +14,18 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-package loadbalancers
+// Package fake contains fakes for loadbalancer.
+package fake


Keep package name and file name consistent. Call them both fakes?

nikhiljindal · 2017-12-04T06:56:14Z

pkg/loadbalancers/loadbalancers_test.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-package loadbalancers
+package loadbalancers_test


Sorry not sure why is this a separate package? Unit tests are supposed to be in the same package.

If you are really moving to a separate package, then also move this to a different directory with the package name.

I don't think there is actually a hard and fast rule about keeping the tests in the same package. (test via the front door). I hesitate to create a package just for the test.

Actually, let me see why I did that originally. Maybe it was a leftover from a different attempt to clean up

Ok, the problem is this:

fakes imports loadbalancers

the test depends on fakes.

import cycle.

Agreed that there is not a hard and fast rule, but it is preferred so that you dont have to expose private members outside the package, just for unit tests.

The problem here is that fakes imports testing, which is not usual.
All of this seems like a hack just to be able to pass t to CheckURLMap and I am not convinced why do you want to do that. Why not let CheckURLMap return an error and handle it appropriately in tests (as it does now). You wont need separate fake and loadbalancer_tests packages then (we can still keep the fake package if we want, but it wont necessarily be required). Reverting that change will make this PR a lot simpler and focused only on the namer change.
What do you think?

nikhiljindal · 2017-12-04T07:06:13Z

cc @G-Harmon @csbell

nikhiljindal · 2017-12-05T20:40:48Z

pkg/loadbalancers/fakes.go

@@ -38,46 +40,46 @@ func (t *testIP) ip() string {

 // Loadbalancer fakes

-// FakeLoadBalancers is a type that fakes out the loadbalancer interface.
+// LoadBalancers is a type that fakes out the loadbalancer interface.


Comment mismatches with code

nikhiljindal · 2017-12-05T20:41:32Z

pkg/loadbalancers/fakes.go

 type FakeLoadBalancers struct {
 	Fw    []*compute.ForwardingRule
 	Um    []*compute.UrlMap
 	Tp    []*compute.TargetHttpProxy
 	Tps   []*compute.TargetHttpsProxy
 	IP    []*compute.Address
 	Certs []*compute.SslCertificate
-	name  string
-	calls []string // list of calls that were made
+	Name  string


All these changes in this file seem like leftover from when the tests were not in same package and hence you had to export them. You dont need to export them anymore?

I'll change it back, but there shouldn't really be any issue with exporting stuff out of the fake...

nikhiljindal · 2017-12-05T21:49:13Z

Thanks for all the fixes!
/lgtm

nikhiljindal · 2017-12-05T21:50:16Z

unit test failures are legit:

pkg/loadbalancers/loadbalancers_test.go:415:24: f.Calls undefined (type *FakeLoadBalancers has no field or method Calls, but does have calls)
pkg/loadbalancers/loadbalancers_test.go:470:3: f.Name undefined (type *FakeLoadBalancers has no field or method Name, but does have name)

bowei · 2017-12-05T21:54:53Z

hmm -- strange, thought I fixed all of the call sites, fixing

- This allows the multicluster code to share use of the Namer. - Add more unit testing - Increases coverage of the namer code.

bowei · 2017-12-05T22:09:02Z

should be fixed now

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 7, 2017

bowei assigned nicksardo and nikhiljindal Nov 7, 2017

nikhiljindal reviewed Nov 7, 2017

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 9, 2017

nikhiljindal reviewed Nov 9, 2017

View reviewed changes

bowei force-pushed the namer branch from 3762614 to 49b1a35 Compare November 28, 2017 00:01

nikhiljindal reviewed Dec 4, 2017

View reviewed changes

bowei force-pushed the namer branch from cca24aa to e697c12 Compare December 5, 2017 00:11

nikhiljindal reviewed Dec 5, 2017

View reviewed changes

bowei force-pushed the namer branch 2 times, most recently from d1dcc42 to fc59532 Compare December 5, 2017 21:41

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 5, 2017

Add ability to change prefix in the Namer

c3178b4

- This allows the multicluster code to share use of the Namer. - Add more unit testing - Increases coverage of the namer code.

bowei force-pushed the namer branch from fc59532 to c3178b4 Compare December 5, 2017 22:08

bowei merged commit 2c86681 into kubernetes:master Dec 5, 2017

bowei deleted the namer branch December 5, 2017 22:23

Add ability to change prefix in the Namer #74

Add ability to change prefix in the Namer #74

Conversation

bowei commented Nov 7, 2017

Choose a reason for hiding this comment

nikhiljindal commented Nov 7, 2017

bowei commented Nov 7, 2017

bowei commented Nov 9, 2017

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

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

bowei commented Nov 28, 2017

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

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

nikhiljindal commented Dec 4, 2017

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

nikhiljindal commented Dec 5, 2017

nikhiljindal commented Dec 5, 2017

bowei commented Dec 5, 2017

bowei commented Dec 5, 2017