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

nikhiljindal commented 6 years ago

Ref https://github.com/GoogleCloudPlatform/k8s-multicluster-ingress/issues/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 commented 6 years ago

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

G-Harmon commented 6 years ago

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 commented 6 years ago

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 https://github.com/GoogleCloudPlatform/k8s-multicluster-ingress/issues/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 commented 6 years ago

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

coveralls commented 6 years ago

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

G-Harmon commented 6 years ago

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 https://github.com/GoogleCloudPlatform/k8s-multicluster-ingress/issues/94 to handle this separately.

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

Comments from Reviewable

nikhiljindal commented 6 years ago

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…