From 5ed2c550063a2890e58bdf611a347155ce76f03e Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Tue, 18 Feb 2020 18:14:09 +0000 Subject: [PATCH 1/2] ingress-shim: add unit tests for events Signed-off-by: James Munnelly --- pkg/controller/ingress-shim/sync_test.go | 33 +++++++++++++++++++----- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/pkg/controller/ingress-shim/sync_test.go b/pkg/controller/ingress-shim/sync_test.go index 2c7542557..aa0f7c898 100644 --- a/pkg/controller/ingress-shim/sync_test.go +++ b/pkg/controller/ingress-shim/sync_test.go @@ -97,6 +97,7 @@ func TestSync(t *testing.T) { ExpectedCreate []*cmapi.Certificate ExpectedUpdate []*cmapi.Certificate ExpectedDelete []*cmapi.Certificate + ExpectedEvents []string } tests := []testT{ { @@ -125,6 +126,7 @@ func TestSync(t *testing.T) { }, }, ClusterIssuerLister: []runtime.Object{acmeClusterIssuer}, + ExpectedEvents: []string{`Normal CreateCertificate Successfully created Certificate "example-com-tls"`}, ExpectedCreate: []*cmapi.Certificate{ { ObjectMeta: metav1.ObjectMeta{ @@ -176,6 +178,7 @@ func TestSync(t *testing.T) { }, }, ClusterIssuerLister: []runtime.Object{acmeClusterIssuer}, + ExpectedEvents: []string{`Normal CreateCertificate Successfully created Certificate "example-com-tls"`}, ExpectedCreate: []*cmapi.Certificate{ { ObjectMeta: metav1.ObjectMeta{ @@ -223,6 +226,7 @@ func TestSync(t *testing.T) { }, }, ClusterIssuerLister: []runtime.Object{acmeClusterIssuer}, + ExpectedEvents: []string{`Normal CreateCertificate Successfully created Certificate "example-com-tls"`}, ExpectedCreate: []*cmapi.Certificate{ { ObjectMeta: metav1.ObjectMeta{ @@ -264,6 +268,7 @@ func TestSync(t *testing.T) { }, }, ClusterIssuerLister: []runtime.Object{acmeClusterIssuer}, + ExpectedEvents: []string{`Normal CreateCertificate Successfully created Certificate "example-com-tls"`}, ExpectedCreate: []*cmapi.Certificate{ { ObjectMeta: metav1.ObjectMeta{ @@ -306,6 +311,7 @@ func TestSync(t *testing.T) { }, }, ClusterIssuerLister: []runtime.Object{acmeClusterIssuer}, + ExpectedEvents: []string{`Normal CreateCertificate Successfully created Certificate "example-com-tls"`}, ExpectedCreate: []*cmapi.Certificate{ { ObjectMeta: metav1.ObjectMeta{ @@ -351,6 +357,7 @@ func TestSync(t *testing.T) { }, }, ClusterIssuerLister: []runtime.Object{acmeClusterIssuer}, + ExpectedEvents: []string{`Normal CreateCertificate Successfully created Certificate "example-com-tls"`}, ExpectedCreate: []*cmapi.Certificate{ { ObjectMeta: metav1.ObjectMeta{ @@ -392,6 +399,7 @@ func TestSync(t *testing.T) { }, }, ClusterIssuerLister: []runtime.Object{acmeClusterIssuer}, + ExpectedEvents: []string{`Normal CreateCertificate Successfully created Certificate "example-com-tls"`}, ExpectedCreate: []*cmapi.Certificate{ { ObjectMeta: metav1.ObjectMeta{ @@ -435,6 +443,7 @@ func TestSync(t *testing.T) { }, }, }, + ExpectedEvents: []string{`Normal CreateCertificate Successfully created Certificate "example-com-tls"`}, ExpectedCreate: []*cmapi.Certificate{ { ObjectMeta: metav1.ObjectMeta{ @@ -455,10 +464,11 @@ func TestSync(t *testing.T) { }, }, { - Name: "should return an error when no TLS hosts are specified", - Issuer: acmeIssuer, - IssuerLister: []runtime.Object{acmeIssuer}, - Err: true, + Name: "should return an error when no TLS hosts are specified", + Issuer: acmeIssuer, + IssuerLister: []runtime.Object{acmeIssuer}, + Err: true, + ExpectedEvents: []string{`Warning BadConfig Secret "example-com-tls" for ingress TLS has no hosts specified`}, Ingress: &extv1beta1.Ingress{ ObjectMeta: metav1.ObjectMeta{ Name: "ingress-name", @@ -478,9 +488,10 @@ func TestSync(t *testing.T) { }, }, { - Name: "should return an error when no TLS secret name is specified", - Issuer: acmeIssuer, - Err: true, + Name: "should return an error when no TLS secret name is specified", + Issuer: acmeIssuer, + Err: true, + ExpectedEvents: []string{`Warning BadConfig TLS entry 0 for hosts [example.com] must specify a secretName`}, Ingress: &extv1beta1.Ingress{ ObjectMeta: metav1.ObjectMeta{ Name: "ingress-name", @@ -586,6 +597,7 @@ func TestSync(t *testing.T) { ), }, DefaultIssuerKind: "Issuer", + ExpectedEvents: []string{`Normal UpdateCertificate Successfully updated Certificate "existing-crt"`}, ExpectedUpdate: []*cmapi.Certificate{ { ObjectMeta: metav1.ObjectMeta{ @@ -650,6 +662,7 @@ func TestSync(t *testing.T) { }, }, }, + ExpectedEvents: []string{`Normal UpdateCertificate Successfully updated Certificate "cert-secret-name"`}, ExpectedUpdate: []*cmapi.Certificate{ { ObjectMeta: metav1.ObjectMeta{ @@ -784,6 +797,7 @@ func TestSync(t *testing.T) { }, }, }, + ExpectedEvents: []string{`Normal DeleteCertificate Successfully deleted unrequired Certificate "existing-crt"`}, ExpectedDelete: []*cmapi.Certificate{ { ObjectMeta: metav1.ObjectMeta{ @@ -845,6 +859,7 @@ func TestSync(t *testing.T) { }, }, }, + ExpectedEvents: []string{`Normal UpdateCertificate Successfully updated Certificate "example-com-tls"`}, ExpectedUpdate: []*cmapi.Certificate{ { ObjectMeta: metav1.ObjectMeta{ @@ -902,6 +917,7 @@ func TestSync(t *testing.T) { T: t, CertManagerObjects: allCMObjects, ExpectedActions: expectedActions, + ExpectedEvents: test.ExpectedEvents, } b.Init() defer b.Stop() @@ -927,6 +943,9 @@ func TestSync(t *testing.T) { t.Errorf("Expected no error, but got: %s", err) } + if err := b.AllEventsCalled(); err != nil { + t.Error(err) + } if err := b.AllReactorsCalled(); err != nil { t.Errorf("Not all expected reactors were called: %v", err) } From eccd7b3fafa3f7615c748c906f69078e7a5aa9b3 Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Tue, 18 Feb 2020 18:00:37 +0000 Subject: [PATCH 2/2] Only allow a single TLS entry per secret name in an Ingress resource Signed-off-by: James Munnelly --- pkg/controller/ingress-shim/sync.go | 7 ++++++ pkg/controller/ingress-shim/sync_test.go | 32 ++++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/pkg/controller/ingress-shim/sync.go b/pkg/controller/ingress-shim/sync.go index 5e93448cb..8f4fedb5f 100644 --- a/pkg/controller/ingress-shim/sync.go +++ b/pkg/controller/ingress-shim/sync.go @@ -109,7 +109,9 @@ func (c *controller) Sync(ctx context.Context, ing *extv1beta1.Ingress) error { func (c *controller) validateIngress(ing *extv1beta1.Ingress) []error { var errs []error + namedSecrets := make(map[string]int) for i, tls := range ing.Spec.TLS { + namedSecrets[tls.SecretName] += 1 // validate the ingress TLS block if len(tls.Hosts) == 0 { errs = append(errs, fmt.Errorf("Secret %q for ingress TLS has no hosts specified", tls.SecretName)) @@ -118,6 +120,11 @@ func (c *controller) validateIngress(ing *extv1beta1.Ingress) []error { errs = append(errs, fmt.Errorf("TLS entry %d for hosts %v must specify a secretName", i, tls.Hosts)) } } + for name, n := range namedSecrets { + if n > 1 { + errs = append(errs, fmt.Errorf("Duplicate TLS entry for secretName %q", name)) + } + } return errs } diff --git a/pkg/controller/ingress-shim/sync_test.go b/pkg/controller/ingress-shim/sync_test.go index aa0f7c898..41466b812 100644 --- a/pkg/controller/ingress-shim/sync_test.go +++ b/pkg/controller/ingress-shim/sync_test.go @@ -879,6 +879,38 @@ func TestSync(t *testing.T) { }, }, }, + { + Name: "if an ingress contains multiple tls entires that specify the same secretName, an error should be logged and no action taken", + Issuer: acmeIssuer, + IssuerLister: []runtime.Object{acmeIssuer}, + ExpectedEvents: []string{ + `Warning BadConfig Duplicate TLS entry for secretName "example-com-tls"`, + }, + Ingress: &extv1beta1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ingress-name", + Namespace: gen.DefaultTestNamespace, + Annotations: map[string]string{ + cmapi.IngressIssuerNameAnnotationKey: "issuer-name", + cmapi.IssuerKindAnnotationKey: "Issuer", + cmapi.IssuerGroupAnnotationKey: "cert-manager.io", + }, + UID: types.UID("ingress-name"), + }, + Spec: extv1beta1.IngressSpec{ + TLS: []extv1beta1.IngressTLS{ + { + Hosts: []string{"example.com"}, + SecretName: "example-com-tls", + }, + { + Hosts: []string{"notexample.com"}, + SecretName: "example-com-tls", + }, + }, + }, + }, + }, } testFn := func(test testT) func(t *testing.T) { return func(t *testing.T) {