From 801888f0f00fc20cac333c0a1612a19ef53e074a Mon Sep 17 00:00:00 2001 From: Louis Taylor Date: Thu, 8 Mar 2018 18:03:26 +0000 Subject: [PATCH 1/6] Reconfigure certs when ingress config changes --- cmd/ingress-shim/controller/sync.go | 59 +++++++++++++++++------- cmd/ingress-shim/controller/sync_test.go | 2 +- 2 files changed, 43 insertions(+), 18 deletions(-) diff --git a/cmd/ingress-shim/controller/sync.go b/cmd/ingress-shim/controller/sync.go index cfb6939d0..9b6b4f156 100644 --- a/cmd/ingress-shim/controller/sync.go +++ b/cmd/ingress-shim/controller/sync.go @@ -3,6 +3,7 @@ package controller import ( "context" "fmt" + "reflect" "strconv" "github.com/golang/glog" @@ -43,12 +44,12 @@ func (c *Controller) Sync(ctx context.Context, ing *extv1beta1.Ingress) error { return nil } - crts, err := c.buildCertificates(ing) + newCrts, updateCrts, err := c.buildCertificates(ing) if err != nil { return err } - for _, crt := range crts { + for _, crt := range newCrts { _, err := c.CMClient.CertmanagerV1alpha1().Certificates(crt.Namespace).Create(crt) if err != nil { return err @@ -56,35 +57,38 @@ func (c *Controller) Sync(ctx context.Context, ing *extv1beta1.Ingress) error { c.Recorder.Eventf(ing, corev1.EventTypeNormal, "CreateCertificate", "Successfully created Certificate %q", crt.Name) } + for _, crt := range updateCrts { + _, err := c.CMClient.CertmanagerV1alpha1().Certificates(crt.Namespace).Update(crt) + if err != nil { + return err + } + c.Recorder.Eventf(ing, corev1.EventTypeNormal, "CreateCertificate", "Successfully updated Certificate %q", crt.Name) + } + return nil } -func (c *Controller) buildCertificates(ing *extv1beta1.Ingress) ([]*v1alpha1.Certificate, error) { +func (c *Controller) buildCertificates(ing *extv1beta1.Ingress) (new, update []*v1alpha1.Certificate, _ error) { issuerName, issuerKind := c.issuerForIngress(ing) issuer, err := c.getGenericIssuer(ing.Namespace, issuerName, issuerKind) if err != nil { - return nil, err + return nil, nil, err } - var crts []*v1alpha1.Certificate + var newCrts []*v1alpha1.Certificate + var updateCrts []*v1alpha1.Certificate for i, tls := range ing.Spec.TLS { // validate the ingress TLS block if len(tls.Hosts) == 0 { - return nil, fmt.Errorf("secret %q for ingress %q has no hosts specified", tls.SecretName, ing.Name) + return nil, nil, fmt.Errorf("secret %q for ingress %q has no hosts specified", tls.SecretName, ing.Name) } if tls.SecretName == "" { - return nil, fmt.Errorf("TLS entry %d for ingress %q must specify a secretName", i, ing.Name) + return nil, nil, fmt.Errorf("TLS entry %d for ingress %q must specify a secretName", i, ing.Name) } - // check if a Certificate for this TLS entry already exists, and if it - // does then skip this entry existingCrt, err := c.certificateLister.Certificates(ing.Namespace).Get(tls.SecretName) if !apierrors.IsNotFound(err) && err != nil { - return nil, err - } - if existingCrt != nil { - glog.Infof("Certificate %q for ingress %q already exists, not re-creating", tls.SecretName, ing.Name) - continue + return nil, nil, err } crt := &v1alpha1.Certificate{ @@ -102,13 +106,34 @@ func (c *Controller) buildCertificates(ing *extv1beta1.Ingress) ([]*v1alpha1.Cer }, }, } + err = c.setIssuerSpecificConfig(crt, issuer, ing, tls) if err != nil { - return nil, err + return nil, nil, err + } + + // check if a Certificate for this TLS entry already exists, and if it + // does then skip this entry + if existingCrt != nil { + glog.Infof("Certificate %q for ingress %q already exists", tls.SecretName, ing.Name) + + if reflect.DeepEqual(existingCrt.Spec, crt.Spec) && existingCrt.Name == crt.Name { + glog.Infof("Certificate %q for ingress %q is up to date", tls.SecretName, ing.Name) + continue + } + + updateCrt := existingCrt.DeepCopy() + + updateCrt.Spec.DNSNames = tls.Hosts + updateCrt.Spec.SecretName = tls.SecretName + updateCrt.Spec.IssuerRef.Name = issuerName + updateCrt.Spec.IssuerRef.Kind = issuerKind + updateCrts = append(updateCrts, updateCrt) + } else { + newCrts = append(newCrts, crt) } - crts = append(crts, crt) } - return crts, nil + return newCrts, updateCrts, nil } func (c *Controller) setIssuerSpecificConfig(crt *v1alpha1.Certificate, issuer v1alpha1.GenericIssuer, ing *extv1beta1.Ingress, tls extv1beta1.IngressTLS) error { diff --git a/cmd/ingress-shim/controller/sync_test.go b/cmd/ingress-shim/controller/sync_test.go index b25a7cd80..c545ece75 100644 --- a/cmd/ingress-shim/controller/sync_test.go +++ b/cmd/ingress-shim/controller/sync_test.go @@ -380,7 +380,7 @@ func TestBuildCertificates(t *testing.T) { DefaultIssuerKind: test.DefaultIssuerKind, }, } - crts, err := c.buildCertificates(test.Ingress) + crts, _, err := c.buildCertificates(test.Ingress) if err != nil && !test.Err { t.Errorf("Expected no error, but got: %s", err) } From 2e5619b1d529f5ec048e331db3d08c6faf5ab13d Mon Sep 17 00:00:00 2001 From: Louis Taylor Date: Mon, 12 Mar 2018 14:00:16 +0000 Subject: [PATCH 2/6] Replace reflect.DeepEqual with crtEqual --- cmd/ingress-shim/controller/sync.go | 36 ++++++++++++++++++++++-- cmd/ingress-shim/controller/sync_test.go | 3 +- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/cmd/ingress-shim/controller/sync.go b/cmd/ingress-shim/controller/sync.go index 9b6b4f156..c731741a7 100644 --- a/cmd/ingress-shim/controller/sync.go +++ b/cmd/ingress-shim/controller/sync.go @@ -3,7 +3,6 @@ package controller import ( "context" "fmt" - "reflect" "strconv" "github.com/golang/glog" @@ -62,7 +61,7 @@ func (c *Controller) Sync(ctx context.Context, ing *extv1beta1.Ingress) error { if err != nil { return err } - c.Recorder.Eventf(ing, corev1.EventTypeNormal, "CreateCertificate", "Successfully updated Certificate %q", crt.Name) + c.Recorder.Eventf(ing, corev1.EventTypeNormal, "UpdateCertificate", "Successfully updated Certificate %q", crt.Name) } return nil @@ -117,7 +116,7 @@ func (c *Controller) buildCertificates(ing *extv1beta1.Ingress) (new, update []* if existingCrt != nil { glog.Infof("Certificate %q for ingress %q already exists", tls.SecretName, ing.Name) - if reflect.DeepEqual(existingCrt.Spec, crt.Spec) && existingCrt.Name == crt.Name { + if crtEqual(existingCrt, crt) { glog.Infof("Certificate %q for ingress %q is up to date", tls.SecretName, ing.Name) continue } @@ -136,6 +135,37 @@ func (c *Controller) buildCertificates(ing *extv1beta1.Ingress) (new, update []* return newCrts, updateCrts, nil } +// crtEqual checks and returns true if two Certificates are equal +func crtEqual(a, b *v1alpha1.Certificate) bool { + if a.Name != b.Name { + return false + } + + if len(a.Spec.DNSNames) != len(b.Spec.DNSNames) { + return false + } + + for i := range a.Spec.DNSNames { + if a.Spec.DNSNames[i] != b.Spec.DNSNames[i] { + return false + } + } + + if a.Spec.SecretName != b.Spec.SecretName { + return false + } + + if a.Spec.IssuerRef.Name != b.Spec.IssuerRef.Name { + return false + } + + if a.Spec.IssuerRef.Kind != b.Spec.IssuerRef.Kind { + return false + } + + return true +} + func (c *Controller) setIssuerSpecificConfig(crt *v1alpha1.Certificate, issuer v1alpha1.GenericIssuer, ing *extv1beta1.Ingress, tls extv1beta1.IngressTLS) error { ingAnnotations := ing.Annotations if ingAnnotations == nil { diff --git a/cmd/ingress-shim/controller/sync_test.go b/cmd/ingress-shim/controller/sync_test.go index c545ece75..029caa1e2 100644 --- a/cmd/ingress-shim/controller/sync_test.go +++ b/cmd/ingress-shim/controller/sync_test.go @@ -339,7 +339,8 @@ func TestBuildCertificates(t *testing.T) { Name: "ingress-name", Namespace: "ingress-namespace", Annotations: map[string]string{ - issuerNameAnnotation: "issuer-name", + issuerNameAnnotation: "issuer-name", + acmeIssuerChallengeTypeAnnotation: "http01", }, }, Spec: extv1beta1.IngressSpec{ From 6eb1c6f9313855b6aaabe3c1f133ca652803f69e Mon Sep 17 00:00:00 2001 From: Louis Taylor Date: Mon, 12 Mar 2018 17:44:53 +0000 Subject: [PATCH 3/6] Split Expected into ExpectedCreate and ExpectedUpdate --- cmd/ingress-shim/controller/sync_test.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/cmd/ingress-shim/controller/sync_test.go b/cmd/ingress-shim/controller/sync_test.go index 029caa1e2..c8ce3a20b 100644 --- a/cmd/ingress-shim/controller/sync_test.go +++ b/cmd/ingress-shim/controller/sync_test.go @@ -69,7 +69,8 @@ func TestBuildCertificates(t *testing.T) { DefaultIssuerName string DefaultIssuerKind string Err bool - Expected []*v1alpha1.Certificate + ExpectedCreate []*v1alpha1.Certificate + ExpectedUpdate []*v1alpha1.Certificate } tests := []testT{ { @@ -93,7 +94,7 @@ func TestBuildCertificates(t *testing.T) { }, }, ClusterIssuerLister: []*v1alpha1.ClusterIssuer{buildACMEClusterIssuer("issuer-name")}, - Expected: []*v1alpha1.Certificate{ + ExpectedCreate: []*v1alpha1.Certificate{ { ObjectMeta: metav1.ObjectMeta{ Name: "example-com-tls", @@ -190,7 +191,7 @@ func TestBuildCertificates(t *testing.T) { }, }, ClusterIssuerLister: []*v1alpha1.ClusterIssuer{buildACMEClusterIssuer("issuer-name")}, - Expected: []*v1alpha1.Certificate{ + ExpectedCreate: []*v1alpha1.Certificate{ { ObjectMeta: metav1.ObjectMeta{ Name: "example-com-tls", @@ -259,7 +260,7 @@ func TestBuildCertificates(t *testing.T) { }, }, ClusterIssuerLister: []*v1alpha1.ClusterIssuer{buildClusterIssuer("issuer-name")}, - Expected: []*v1alpha1.Certificate{ + ExpectedCreate: []*v1alpha1.Certificate{ { ObjectMeta: metav1.ObjectMeta{ Name: "example-com-tls", @@ -385,8 +386,8 @@ func TestBuildCertificates(t *testing.T) { if err != nil && !test.Err { t.Errorf("Expected no error, but got: %s", err) } - if !reflect.DeepEqual(crts, test.Expected) { - t.Errorf("Expected %+v but got %+v", test.Expected, crts) + if !reflect.DeepEqual(crts, test.ExpectedCreate) { + t.Errorf("Expected %+v but got %+v", test.ExpectedCreate, crts) } } } From d7153ecc1e6aaaa755a30a99a45ef2581f4b0da2 Mon Sep 17 00:00:00 2001 From: Louis Taylor Date: Mon, 12 Mar 2018 18:36:21 +0000 Subject: [PATCH 4/6] Test updates --- cmd/ingress-shim/controller/sync_test.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/cmd/ingress-shim/controller/sync_test.go b/cmd/ingress-shim/controller/sync_test.go index c8ce3a20b..2115e274f 100644 --- a/cmd/ingress-shim/controller/sync_test.go +++ b/cmd/ingress-shim/controller/sync_test.go @@ -382,12 +382,16 @@ func TestBuildCertificates(t *testing.T) { DefaultIssuerKind: test.DefaultIssuerKind, }, } - crts, _, err := c.buildCertificates(test.Ingress) + createCrts, updateCrts, err := c.buildCertificates(test.Ingress) if err != nil && !test.Err { t.Errorf("Expected no error, but got: %s", err) } - if !reflect.DeepEqual(crts, test.ExpectedCreate) { - t.Errorf("Expected %+v but got %+v", test.ExpectedCreate, crts) + if !reflect.DeepEqual(createCrts, test.ExpectedCreate) { + t.Errorf("Expected to create %+v but got %+v", test.ExpectedCreate, createCrts) + } + + if !reflect.DeepEqual(updateCrts, test.ExpectedUpdate) { + t.Errorf("Expected to update %+v but got %+v", test.ExpectedUpdate, updateCrts) } } } From 7bb9048578e4a69744d07b670fec717179c9d1f6 Mon Sep 17 00:00:00 2001 From: Louis Taylor Date: Mon, 12 Mar 2018 18:36:39 +0000 Subject: [PATCH 5/6] Add update testcase --- cmd/ingress-shim/controller/sync_test.go | 56 +++++++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-) diff --git a/cmd/ingress-shim/controller/sync_test.go b/cmd/ingress-shim/controller/sync_test.go index 2115e274f..601987e63 100644 --- a/cmd/ingress-shim/controller/sync_test.go +++ b/cmd/ingress-shim/controller/sync_test.go @@ -334,7 +334,45 @@ func TestBuildCertificates(t *testing.T) { }, }, { - Name: "should not return any certificates if a Certificate already exists", + Name: "should not return any certificates if a correct Certificate already exists", + Ingress: &extv1beta1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ingress-name", + Namespace: "ingress-namespace", + Annotations: map[string]string{ + issuerNameAnnotation: "issuer-name", + acmeIssuerChallengeTypeAnnotation: "http01", + }, + }, + Spec: extv1beta1.IngressSpec{ + TLS: []extv1beta1.IngressTLS{ + { + Hosts: []string{"example.com"}, + SecretName: "existing-crt", + }, + }, + }, + }, + IssuerLister: []*v1alpha1.Issuer{buildACMEIssuer("issuer-name", "ingress-namespace")}, + CertificateLister: []*v1alpha1.Certificate{ + &v1alpha1.Certificate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "existing-crt", + Namespace: "ingress-namespace", + }, + Spec: v1alpha1.CertificateSpec{ + DNSNames: []string{"example.com"}, + SecretName: "existing-crt", + IssuerRef: v1alpha1.ObjectReference{ + Name: "issuer-name", + Kind: "Issuer", + }, + }, + }, + }, + }, + { + Name: "should update a certificate if an incorrect Certificate exists", Ingress: &extv1beta1.Ingress{ ObjectMeta: metav1.ObjectMeta{ Name: "ingress-name", @@ -355,6 +393,22 @@ func TestBuildCertificates(t *testing.T) { }, IssuerLister: []*v1alpha1.Issuer{buildACMEIssuer("issuer-name", "ingress-namespace")}, CertificateLister: []*v1alpha1.Certificate{buildCertificate("existing-crt", "ingress-namespace")}, + ExpectedUpdate: []*v1alpha1.Certificate{ + &v1alpha1.Certificate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "existing-crt", + Namespace: "ingress-namespace", + }, + Spec: v1alpha1.CertificateSpec{ + DNSNames: []string{"example.com"}, + SecretName: "existing-crt", + IssuerRef: v1alpha1.ObjectReference{ + Name: "issuer-name", + Kind: "Issuer", + }, + }, + }, + }, }, } testFn := func(test testT) func(t *testing.T) { From 1690408632c91e0e7c42629fda13e95ab660f4e9 Mon Sep 17 00:00:00 2001 From: Louis Taylor Date: Mon, 12 Mar 2018 18:38:26 +0000 Subject: [PATCH 6/6] crtEqual -> certNeedsUpdate --- cmd/ingress-shim/controller/sync.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/cmd/ingress-shim/controller/sync.go b/cmd/ingress-shim/controller/sync.go index c731741a7..4cd4e0cc7 100644 --- a/cmd/ingress-shim/controller/sync.go +++ b/cmd/ingress-shim/controller/sync.go @@ -116,7 +116,7 @@ func (c *Controller) buildCertificates(ing *extv1beta1.Ingress) (new, update []* if existingCrt != nil { glog.Infof("Certificate %q for ingress %q already exists", tls.SecretName, ing.Name) - if crtEqual(existingCrt, crt) { + if !certNeedsUpdate(existingCrt, crt) { glog.Infof("Certificate %q for ingress %q is up to date", tls.SecretName, ing.Name) continue } @@ -135,35 +135,35 @@ func (c *Controller) buildCertificates(ing *extv1beta1.Ingress) (new, update []* return newCrts, updateCrts, nil } -// crtEqual checks and returns true if two Certificates are equal -func crtEqual(a, b *v1alpha1.Certificate) bool { +// certNeedsUpdate checks and returns true if two Certificates are equal +func certNeedsUpdate(a, b *v1alpha1.Certificate) bool { if a.Name != b.Name { - return false + return true } if len(a.Spec.DNSNames) != len(b.Spec.DNSNames) { - return false + return true } for i := range a.Spec.DNSNames { if a.Spec.DNSNames[i] != b.Spec.DNSNames[i] { - return false + return true } } if a.Spec.SecretName != b.Spec.SecretName { - return false + return true } if a.Spec.IssuerRef.Name != b.Spec.IssuerRef.Name { - return false + return true } if a.Spec.IssuerRef.Kind != b.Spec.IssuerRef.Kind { - return false + return true } - return true + return false } func (c *Controller) setIssuerSpecificConfig(crt *v1alpha1.Certificate, issuer v1alpha1.GenericIssuer, ing *extv1beta1.Ingress, tls extv1beta1.IngressTLS) error {