diff --git a/pkg/controller/ingress-shim/sync.go b/pkg/controller/ingress-shim/sync.go index 4e6f60d4e..1b4dd7dbc 100644 --- a/pkg/controller/ingress-shim/sync.go +++ b/pkg/controller/ingress-shim/sync.go @@ -106,18 +106,13 @@ func (c *controller) Sync(ctx context.Context, ing *networkingv1beta1.Ingress) e } func (c *controller) validateIngress(ing *networkingv1beta1.Ingress) []error { + // check for duplicate values of networkingv1beta1.IngressTLS.SecretName 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)) - } - if tls.SecretName == "" { - errs = append(errs, fmt.Errorf("TLS entry %d for hosts %v must specify a secretName", i, tls.Hosts)) - } + for _, tls := range ing.Spec.TLS { + namedSecrets[tls.SecretName]++ } + // not doing this in the previous for-loop to avoid erroring more than once for the same SecretName for name, n := range namedSecrets { if n > 1 { errs = append(errs, fmt.Errorf("Duplicate TLS entry for secretName %q", name)) @@ -126,13 +121,33 @@ func (c *controller) validateIngress(ing *networkingv1beta1.Ingress) []error { return errs } +func validateIngressTLSBlock(tlsBlock networkingv1beta1.IngressTLS) []error { + // unlikely that _both_ SecretName and Hosts would be empty, but still returning []error for consistency + var errs []error + + if len(tlsBlock.Hosts) == 0 { + errs = append(errs, fmt.Errorf("secret %q for ingress TLS has no hosts specified", tlsBlock.SecretName)) + } + if tlsBlock.SecretName == "" { + errs = append(errs, fmt.Errorf("TLS entry for hosts %v must specify a secretName", tlsBlock.Hosts)) + } + return errs +} + func (c *controller) buildCertificates(ctx context.Context, ing *networkingv1beta1.Ingress, issuerName, issuerKind, issuerGroup string) (new, update []*cmapi.Certificate, _ error) { log := logs.FromContext(ctx) var newCrts []*cmapi.Certificate var updateCrts []*cmapi.Certificate - for _, tls := range ing.Spec.TLS { + for i, tls := range ing.Spec.TLS { + errs := validateIngressTLSBlock(tls) + // if this tls entry is invalid, record an error event on Ingress object and continue to the next tls entry + if len(errs) > 0 { + errMsg := utilerrors.NewAggregate(errs).Error() + c.recorder.Eventf(ing, corev1.EventTypeWarning, "BadConfig", fmt.Sprintf("TLS entry %d is invalid: %s", i, errMsg)) + continue + } existingCrt, err := c.certificateLister.Certificates(ing.Namespace).Get(tls.SecretName) if !apierrors.IsNotFound(err) && err != nil { return nil, nil, err diff --git a/pkg/controller/ingress-shim/sync_test.go b/pkg/controller/ingress-shim/sync_test.go index 791f40a98..2420b5125 100644 --- a/pkg/controller/ingress-shim/sync_test.go +++ b/pkg/controller/ingress-shim/sync_test.go @@ -513,11 +513,14 @@ func TestSync(t *testing.T) { }, }, { - 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`}, + Name: "should skip an invalid TLS entry (no TLS hosts specified)", + Issuer: acmeIssuer, + IssuerLister: []runtime.Object{acmeIssuer}, + Err: true, + ExpectedEvents: []string{ + `Warning BadConfig TLS entry 0 is invalid: secret "example-com-tls-invalid" for ingress TLS has no hosts specified`, + `Normal CreateCertificate Successfully created Certificate "example-com-tls"`, + }, Ingress: &networkingv1beta1.Ingress{ ObjectMeta: metav1.ObjectMeta{ Name: "ingress-name", @@ -529,18 +532,44 @@ func TestSync(t *testing.T) { }, Spec: networkingv1beta1.IngressSpec{ TLS: []networkingv1beta1.IngressTLS{ + { + SecretName: "example-com-tls-invalid", + }, { SecretName: "example-com-tls", + Hosts: []string{"example.com", "www.example.com"}, + }, + }, + }, + }, + ExpectedCreate: []*cmapi.Certificate{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "example-com-tls", + Namespace: gen.DefaultTestNamespace, + OwnerReferences: buildOwnerReferences("ingress-name", gen.DefaultTestNamespace), + }, + Spec: cmapi.CertificateSpec{ + DNSNames: []string{"example.com", "www.example.com"}, + SecretName: "example-com-tls", + IssuerRef: cmmeta.ObjectReference{ + Name: "issuer-name", + Kind: "Issuer", }, }, }, }, }, + { - 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`}, + Name: "should skip an invalid TLS entry (no TLS secret name specified)", + Issuer: acmeIssuer, + Err: true, + IssuerLister: []runtime.Object{acmeIssuer}, + ExpectedEvents: []string{ + `Warning BadConfig TLS entry 0 is invalid: TLS entry for hosts [example.com] must specify a secretName`, + `Normal CreateCertificate Successfully created Certificate "example-com-tls"`, + }, Ingress: &networkingv1beta1.Ingress{ ObjectMeta: metav1.ObjectMeta{ Name: "ingress-name", @@ -555,10 +584,30 @@ func TestSync(t *testing.T) { { Hosts: []string{"example.com"}, }, + { + Hosts: []string{"example.com", "www.example.com"}, + SecretName: "example-com-tls", + }, + }, + }, + }, + ExpectedCreate: []*cmapi.Certificate{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "example-com-tls", + Namespace: gen.DefaultTestNamespace, + OwnerReferences: buildOwnerReferences("ingress-name", gen.DefaultTestNamespace), + }, + Spec: cmapi.CertificateSpec{ + DNSNames: []string{"example.com", "www.example.com"}, + SecretName: "example-com-tls", + IssuerRef: cmmeta.ObjectReference{ + Name: "issuer-name", + Kind: "Issuer", + }, }, }, }, - IssuerLister: []runtime.Object{acmeIssuer}, }, { Name: "should error if the specified issuer is not found",