Adding code to create ssl cert from secret and create https target proxy

[nikhiljindal]

Ref #46

This is a step towards adding HTTPS support to kubemci.

It has the following main changes:

• Adding a SSLCertSyncer that manages certs for a given ingress
• Updating TargetProxySyncer to manage https target proxies as well.

In ingress-gce, Users can specify certs in 2 ways:

• Using ingress.Spec.TLS.SecretName
• Using ingress.gcp.kubernetes.io/pre-shared-cert annotation.

SSLCertSyncer supports only the first for now. Will add support for the annotation in next PR.
Also note, that updating the secret will cause downtime for now, since it deletes and then recreates the ssl certificate.

cc @csbell @G-Harmon

---

This change is 

[coveralls]

Changes Unknown when pulling bbcc252 on nikhiljindal:https into ** on GoogleCloudPlatform:master**.

[G-Harmon]

looks good overall. One bigger comment (below) about how you handle diffs.

---

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

---

app/kubemci/pkg/gcp/sslcert/sslcertsyncer.go, line 65 at r1 (raw file):

}

func (s *SSLCertSyncer) ensureSecretSSLCert(lbName string, ing *v1beta1.Ingress, client kubeclient.Interface, forceUpdate bool) (string, error) {

Seems confusing to have 2 methods that only differ by the capitalization. call this one ensureSecretSslCertInternal?

---

app/kubemci/pkg/gcp/sslcert/sslcertsyncer.go, line 69 at r1 (raw file):

	desiredCert, err := s.desiredSSLCert(lbName, ing, client)
	if err != nil {
		return "", fmt.Errorf("error %s in computing desired ssl cert", err)

fmt.Println the error too

---

app/kubemci/pkg/gcp/sslcert/sslcertsyncer.go, line 152 at r1 (raw file):

	existingCert.SelfLink = ""

	return reflect.DeepEqual(existingCert, desiredCert)

print the diff if they don't match

---

app/kubemci/pkg/gcp/sslcert/sslcertsyncer.go, line 160 at r1 (raw file):

	cert, err := tlsLoader.Load(ing)
	if err != nil {
		return nil, fmt.Errorf("Error in fetching certs for ing %s/%s: %s", ing.Namespace, ing.Name, err)

fmt.Println the error too?

---

app/kubemci/pkg/gcp/sslcert/sslcertsyncer_test.go, line 37 at r1 (raw file):

	lbName := "lb-name"
	// Should create the ssl cert as expected.
	scp := ingresslb.NewFakeLoadBalancers("")

If my update-vendor/ PR goes in first, you'll need to add the ingress-gce namer here.

---

app/kubemci/pkg/gcp/targetproxy/fake_targetproxysyncer.go, line 24 at r1 (raw file):

	LBName   string
	UmLink   string
	CertLink string

maybe add a comment for CertLink.

---

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

		err = multierror.Append(err, httpErr)
	}
	fmt.Println("target HTTP proxy", httpName, "deleted successfully")

this needs to be an else now.

---

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

		err = multierror.Append(err, httpsErr)
	}
	fmt.Println("target HTTPS proxy", httpsName, "deleted successfully")

same here.

---

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

// Apart from name, UrlMap is the only field that can be different. We update that field directly.
err := s.tpp.SetUrlMapForTargetHttpsProxy(desiredHttpsProxy, &compute.UrlMap{SelfLink: desiredHttpsProxy.UrlMap})

I think this can be buggy:
If another field, like the Description, is different, then we won't converge onto the desired state. (I ran into this situation when I was changing the Description field in my local client. I was doing that when testing --force-update.)

---

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

glog.V(0).Infof

This should be higher than the V(2) right above it, i think.

---

Comments from Reviewable

[nikhiljindal]

Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

app/kubemci/pkg/gcp/sslcert/sslcertsyncer.go, line 65 at r1 (raw file):

Previously, G-Harmon wrote…

Seems confusing to have 2 methods that only differ by the capitalization. call this one ensureSecretSslCertInternal?

Not sure which 2 methods are you talking about.
There is EnsureSSLCert, ensurePreSharedCert and ensureSecretSSLCert.
Have renamed EnsurePreSharedCert to ensurePreSharedSSLCert to make them all consisten.

EnsureSSLCert is the public exported method that calls ensurePreSharedSSLCert or ensureSecretSSLCert depending on how the certs should be handled.

---

app/kubemci/pkg/gcp/sslcert/sslcertsyncer.go, line 69 at r1 (raw file):

Previously, G-Harmon wrote…

fmt.Println the error too

Done.

---

app/kubemci/pkg/gcp/sslcert/sslcertsyncer.go, line 152 at r1 (raw file):

Previously, G-Harmon wrote…

print the diff if they don't match

Dont want to print the secret cert :)

---

app/kubemci/pkg/gcp/sslcert/sslcertsyncer.go, line 160 at r1 (raw file):

Previously, G-Harmon wrote…

fmt.Println the error too?

Done.

---

app/kubemci/pkg/gcp/sslcert/sslcertsyncer_test.go, line 37 at r1 (raw file):

Previously, G-Harmon wrote…

If my update-vendor/ PR goes in first, you'll need to add the ingress-gce namer here.

Done.

---

app/kubemci/pkg/gcp/targetproxy/fake_targetproxysyncer.go, line 24 at r1 (raw file):

Previously, G-Harmon wrote…

maybe add a comment for CertLink.

Done.

---

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

Previously, G-Harmon wrote…

this needs to be an else now.

ah good catch. fixed

---

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

Previously, G-Harmon wrote…

same here.

Done.

---

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

Previously, G-Harmon wrote…

// Apart from name, UrlMap is the only field that can be different. We update that field directly.
err := s.tpp.SetUrlMapForTargetHttpsProxy(desiredHttpsProxy, &compute.UrlMap{SelfLink: desiredHttpsProxy.UrlMap})

I think this can be buggy:
If another field, like the Description, is different, then we won't converge onto the desired state. (I ran into this situation when I was changing the Description field in my local client. I was doing that when testing --force-update.)

aah yes good catch again :)
Yes this bug exists in http target proxy as well.
Filed #94 to handle this separately.

---

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

Previously, G-Harmon wrote…

glog.V(0).Infof

This should be higher than the V(2) right above it, i think.

right, done

---

Comments from Reviewable

[nikhiljindal]

Rebased to include #93 and updated as per comments.
PTAL

[coveralls]

Changes Unknown when pulling 67a52be on nikhiljindal:https into ** on GoogleCloudPlatform:master**.

[G-Harmon]

Reviewed 4 of 6 files at r2.
Review status: 9 of 11 files reviewed at latest revision, 3 unresolved discussions.

---

app/kubemci/pkg/gcp/sslcert/sslcertsyncer.go, line 65 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

Not sure which 2 methods are you talking about.
There is EnsureSSLCert, ensurePreSharedCert and ensureSecretSSLCert.
Have renamed EnsurePreSharedCert to ensurePreSharedSSLCert to make them all consisten.

EnsureSSLCert is the public exported method that calls ensurePreSharedSSLCert or ensureSecretSSLCert depending on how the certs should be handled.

Oops, I misread it. I missed 'secret' in ensureSecretSSLCert.

---

app/kubemci/pkg/gcp/sslcert/sslcertsyncer.go, line 152 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

Dont want to print the secret cert :)

oh! (is that obvious, or is it worth a comment?)

---

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

Previously, nikhiljindal (Nikhil Jindal) wrote…

aah yes good catch again :)
Yes this bug exists in http target proxy as well.
Filed #94 to handle this separately.

okay, that's fine. Can you reference that bug here in the code as well?

---

Comments from Reviewable

[nikhiljindal]

Review status: 9 of 11 files reviewed at latest revision, 3 unresolved discussions.

---

app/kubemci/pkg/gcp/sslcert/sslcertsyncer.go, line 152 at r1 (raw file):

Previously, G-Harmon wrote…

oh! (is that obvious, or is it worth a comment?)

Done, added a comment.

---

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

Previously, G-Harmon wrote…

okay, that's fine. Can you reference that bug here in the code as well?

Done.

---

Comments from Reviewable

[coveralls]

Changes Unknown when pulling d80971c on nikhiljindal:https into ** on GoogleCloudPlatform:master**.

[G-Harmon]

---

Review status: 7 of 11 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable