fix: remove multiple registered domain for TLS managed certificates (#…

…173) * fix: add tests for TLS plugin * fix: do not register the same host multiple times in TLS plugin
caddyserver · Nov 5, 2023 · c5abad0 · c5abad0
1 parent 6f6f91c
commit c5abad0
Show file tree

Hide file tree

Showing 2 changed files with 215 additions and 1 deletion.
diff --git a/internal/caddy/global/tls.go b/internal/caddy/global/tls.go
@@ -2,6 +2,7 @@ package global
 
 import (
 	"encoding/json"
+	"slices"
 
 	"github.com/caddyserver/ingress/internal/controller"
 	"github.com/caddyserver/ingress/pkg/converter"
@@ -31,7 +32,9 @@ func (p TLSPlugin) GlobalHandler(config *converter.Config, store *store.Store) e
 	for _, ing := range store.Ingresses {
 		for _, tlsRule := range ing.Spec.TLS {
 			for _, h := range tlsRule.Hosts {
-				hosts = append(hosts, h)
+				if !slices.Contains(hosts, h) {
+					hosts = append(hosts, h)
+				}
 			}
 		}
 	}

diff --git a/internal/caddy/global/tls_test.go b/internal/caddy/global/tls_test.go
@@ -0,0 +1,211 @@
+package global
+
+import (
+	"testing"
+
+	"github.com/caddyserver/ingress/pkg/converter"
+	"github.com/caddyserver/ingress/pkg/store"
+
+	networkingv1 "k8s.io/api/networking/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	types "k8s.io/apimachinery/pkg/types"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestIngressTlsSkipCertificates(t *testing.T) {
+	testCases := []struct {
+		desc                string
+		skippedCertsDomains []string
+		ingresses           []*networkingv1.Ingress
+	}{
+		{
+			desc:                "No ingress registered",
+			skippedCertsDomains: []string{},
+			ingresses:           []*networkingv1.Ingress{},
+		},
+		{
+			desc:                "One ingress registered with certificate with one domain",
+			skippedCertsDomains: []string{"domain1.tld"},
+			ingresses: []*networkingv1.Ingress{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						UID: types.UID("first"),
+					},
+					Spec: networkingv1.IngressSpec{
+						TLS: []networkingv1.IngressTLS{{
+							Hosts: []string{"domain1.tld"},
+						}},
+					},
+				},
+			},
+		},
+		{
+			desc:                "One ingress registered with certificate with multiple domains",
+			skippedCertsDomains: []string{"domain1.tld", "domain2.tld", "domain3.tld"},
+			ingresses: []*networkingv1.Ingress{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						UID: types.UID("first"),
+					},
+					Spec: networkingv1.IngressSpec{
+						TLS: []networkingv1.IngressTLS{{
+							Hosts: []string{"domain1.tld", "domain2.tld", "domain3.tld"},
+						}},
+					},
+				},
+			},
+		},
+		{
+			desc:                "Two ingress registered with certificate one domain each",
+			skippedCertsDomains: []string{"domain1.tld", "domain2.tld"},
+			ingresses: []*networkingv1.Ingress{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						UID: types.UID("first"),
+					},
+					Spec: networkingv1.IngressSpec{
+						TLS: []networkingv1.IngressTLS{{
+							Hosts: []string{"domain1.tld"},
+						}},
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						UID: types.UID("second"),
+					},
+					Spec: networkingv1.IngressSpec{
+						TLS: []networkingv1.IngressTLS{{
+							Hosts: []string{"domain2.tld"},
+						}},
+					},
+				},
+			},
+		},
+		{
+			desc:                "Two ingress registered with certificate the same domain",
+			skippedCertsDomains: []string{"domain1.tld"},
+			ingresses: []*networkingv1.Ingress{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						UID: types.UID("first"),
+					},
+					Spec: networkingv1.IngressSpec{
+						TLS: []networkingv1.IngressTLS{{
+							Hosts: []string{"domain1.tld"},
+						}},
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						UID: types.UID("second"),
+					},
+					Spec: networkingv1.IngressSpec{
+						TLS: []networkingv1.IngressTLS{{
+							Hosts: []string{"domain1.tld"},
+						}},
+					},
+				},
+			},
+		},
+		{
+			desc:                "Two ingress registered with certificate with multiple domains each",
+			skippedCertsDomains: []string{"domain1a.tld", "domain1b.tld", "domain2a.tld", "domain2b.tld"},
+			ingresses: []*networkingv1.Ingress{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						UID: types.UID("first"),
+					},
+					Spec: networkingv1.IngressSpec{
+						TLS: []networkingv1.IngressTLS{{
+							Hosts: []string{"domain1a.tld", "domain1b.tld"},
+						}},
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						UID: types.UID("second"),
+					},
+					Spec: networkingv1.IngressSpec{
+						TLS: []networkingv1.IngressTLS{{
+							Hosts: []string{"domain2a.tld", "domain2b.tld"},
+						}},
+					},
+				},
+			},
+		},
+		{
+			desc:                "Two ingress registered with certificate with multiple domains each and partial domain overlap",
+			skippedCertsDomains: []string{"domain1.tld", "domain2a.tld", "domain2b.tld"},
+			ingresses: []*networkingv1.Ingress{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						UID: types.UID("first"),
+					},
+					Spec: networkingv1.IngressSpec{
+						TLS: []networkingv1.IngressTLS{{
+							Hosts: []string{"domain1.tld", "domain2a.tld"},
+						}},
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						UID: types.UID("second"),
+					},
+					Spec: networkingv1.IngressSpec{
+						TLS: []networkingv1.IngressTLS{{
+							Hosts: []string{"domain1.tld", "domain2b.tld"},
+						}},
+					},
+				},
+			},
+		},
+		{
+			desc:                "One ingress registered without certificate",
+			skippedCertsDomains: []string{},
+			ingresses: []*networkingv1.Ingress{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						UID: types.UID("first"),
+					},
+					Spec: networkingv1.IngressSpec{},
+				},
+			},
+		},
+		{
+			desc:                "Two ingresses registered without certificate",
+			skippedCertsDomains: []string{},
+			ingresses: []*networkingv1.Ingress{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						UID: types.UID("first"),
+					},
+					Spec: networkingv1.IngressSpec{},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						UID: types.UID("second"),
+					},
+					Spec: networkingv1.IngressSpec{},
+				},
+			},
+		},
+	}
+
+	for _, tC := range testCases {
+		t.Run(tC.desc, func(t *testing.T) {
+			tp := TLSPlugin{}
+			c := converter.NewConfig()
+			s := store.NewStore(store.Options{}, &store.PodInfo{})
+
+			for _, ing := range tC.ingresses {
+				s.AddIngress(ing)
+			}
+
+			tp.GlobalHandler(c, s)
+
+			toSkip := c.GetHTTPServer().AutoHTTPS.SkipCerts
+			assert.ElementsMatch(t, toSkip, tC.skippedCertsDomains, "List of certificate to skip don't match expectation")
+		})
+	}
+}