Adding code to create Target http proxies

[nikhiljindal]

This uses the URLMap created in #7 to create target proxies.

This PR contains logic to create HTTP proxies only. HTTPS proxies require additional logic of managing SSL certs. Will add support for that in a separate PR.

cc @bowei @nicksardo @csbell @G-Harmon @madhusudancs

---

This change is 

[G-Harmon]

LGTM
minor comments, except the one about the name prefixes!

---

Reviewed 11 of 11 files at r1.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

---

app/mci/pkg/gcp/namer/namer.go, line 26 at r1 (raw file):

"hc"
backendPrefix = "be"
urlMapPrefix = "um"
targetHttpProxyPrefix = "tp"
targetHttpsProxyPrefix = "tps"

We don't need "k8s-*" prefixes for our resources?

---

app/mci/pkg/gcp/targetproxy/targetproxysyncer.go, line 44 at r1 (raw file):

for the given ports.
s/ports/UrlMap/ ?

---

app/mci/pkg/gcp/targetproxy/targetproxysyncer.go, line 49 at r1 (raw file):

	fmt.Println("Ensuring target proxies")
	// TODO(nikhiljindal): Support creating https proxies.
	fmt.Println("Warning: We create http proxies only, even if https was requested.")

Is it trivial to throw an error if HTTPS was requested? Or will HTTPS be implemented soon enough that it's not worth it?

---

app/mci/pkg/gcp/targetproxy/targetproxysyncer.go, line 92 at r1 (raw file):

TargetProxy
Should this say UrlMap instead of TargetProxy?

---

app/mci/pkg/gcp/targetproxy/targetproxysyncer.go, line 92 at r1 (raw file):

that be different
grammar nit: 'that is different'

---

app/mci/pkg/gcp/targetproxy/targetproxysyncer_test.go, line 32 at r1 (raw file):

	namer := utilsnamer.NewNamer("mci", lbName)
	tpName := namer.TargetHttpProxyName()
	ums := NewTargetProxySyncer(namer, tpp)

ums? Should this be tps (target proxy syncer)?

---

app/mci/pkg/gcp/urlmap/urlmapsyncer.go, line 59 at r1 (raw file):

// EnsureURLMap ensures that the required url map exists.
// Does nothing if it exists already, else creates a new one.
func (s *URLMapSyncer) EnsureURLMap(lbName string, ing *v1beta1.Ingress, beMap backendservice.BackendServicesMap) (string, error) {

Do we comment at both the interface and the implementation? On my last google3/C++ project, we only commented the impl if there were notes to add about the impl. Usually there was no comment at all.

anyway, I ask because you updated the interface comment but not the impl comment.

---

app/mci/pkg/gcp/urlmap/urlmapsyncer_test.go, line 53 at r1 (raw file):

		t.Fatalf("expected NotFound error, got nil")
	}
	_, err := ums.EnsureURLMap(lbName, ing, beMap)

minor: check that the urlmap name is correct.

---

Comments from Reviewable

[nikhiljindal]

Review status: all files reviewed at latest revision, 8 unresolved discussions.

---

app/mci/pkg/gcp/namer/namer.go, line 26 at r1 (raw file):

Previously, G-Harmon wrote…

"hc"
backendPrefix = "be"
urlMapPrefix = "um"
targetHttpProxyPrefix = "tp"
targetHttpsProxyPrefix = "tps"

We don't need "k8s-*" prefixes for our resources?

No we add mci prefix instead.

---

app/mci/pkg/gcp/targetproxy/targetproxysyncer.go, line 49 at r1 (raw file):

Previously, G-Harmon wrote…

Is it trivial to throw an error if HTTPS was requested? Or will HTTPS be implemented soon enough that it's not worth it?

We need logic to detect if HTTPS was requested. I will try to implement HTTPS soon, but if not will atleast add HTTPS request detection logic soon enough so that we can generate an error and remove this generic scary warning.
ForwardingRule PR that I am working on now has the same problem.

Now that kubernetes/ingress-gce#58 is merged, that should make it easier to detect annotations to figure out if HTTPS was requested. We still need additional logic to load the certs.

---

app/mci/pkg/gcp/urlmap/urlmapsyncer.go, line 59 at r1 (raw file):

Previously, G-Harmon wrote…

Do we comment at both the interface and the implementation? On my last google3/C++ project, we only commented the impl if there were notes to add about the impl. Usually there was no comment at all.

anyway, I ask because you updated the interface comment but not the impl comment.

Yes that sounds good to me.
Will send a separate PR to remove comments on impls that are just a copy of comment from interface :)

---

Comments from Reviewable

[nikhiljindal]

Review status: all files reviewed at latest revision, 8 unresolved discussions.

---

app/mci/pkg/gcp/targetproxy/targetproxysyncer.go, line 44 at r1 (raw file):

Previously, G-Harmon wrote…

for the given ports.
s/ports/UrlMap/ ?

Done.

---

app/mci/pkg/gcp/targetproxy/targetproxysyncer.go, line 92 at r1 (raw file):

Previously, G-Harmon wrote…

TargetProxy
Should this say UrlMap instead of TargetProxy?

Right, done

---

app/mci/pkg/gcp/targetproxy/targetproxysyncer.go, line 92 at r1 (raw file):

Previously, G-Harmon wrote…

that be different
grammar nit: 'that is different'

Updated to "that can be different".

---

app/mci/pkg/gcp/targetproxy/targetproxysyncer_test.go, line 32 at r1 (raw file):

Previously, G-Harmon wrote…

ums? Should this be tps (target proxy syncer)?

Done.

---

app/mci/pkg/gcp/urlmap/urlmapsyncer_test.go, line 53 at r1 (raw file):

Previously, G-Harmon wrote…

minor: check that the urlmap name is correct.

Done.

---

Comments from Reviewable

[nikhiljindal]

Thanks for the review @G-Harmon
Updated code as per comments.

[csbell]

/lgtm pending Greg's

[G-Harmon]

---

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

app/mci/pkg/gcp/namer/namer.go, line 26 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

No we add mci prefix instead.

ah, right. I saw that when I ran mci again yesterday :)

---

app/mci/pkg/gcp/urlmap/urlmapsyncer.go, line 59 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

Yes that sounds good to me.
Will send a separate PR to remove comments on impls that are just a copy of comment from interface :)

okay!

---

Comments from Reviewable

[nikhiljindal]

Thanks @csbell and @G-Harmon