Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(Traefik Proxy): missing IgnoreIngressRulesSpec check #5114

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+192 −28
Open

fix(Traefik Proxy): missing IgnoreIngressRulesSpec check #5114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion source/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ func BuildWithConfig(ctx context.Context, source string, p ClientGenerator, cfg
if err != nil {
return nil, err
}
return NewTraefikSource(ctx, dynamicClient, kubernetesClient, cfg.Namespace, cfg.AnnotationFilter, cfg.IgnoreHostnameAnnotation, cfg.TraefikDisableLegacy, cfg.TraefikDisableNew)
return NewTraefikSource(ctx, dynamicClient, kubernetesClient, cfg.Namespace, cfg.AnnotationFilter, cfg.IgnoreHostnameAnnotation, cfg.IgnoreIngressRulesSpec, cfg.TraefikDisableLegacy, cfg.TraefikDisableNew)
case "openshift-route":
ocpClient, err := p.OpenShiftClient()
if err != nil {
Expand Down
46 changes: 26 additions & 20 deletions source/traefik_proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ var (
type traefikSource struct {
annotationFilter string
ignoreHostnameAnnotation bool
ignoreIngressRouteSpec bool
dynamicKubeClient dynamic.Interface
ingressRouteInformer informers.GenericInformer
ingressRouteTcpInformer informers.GenericInformer
Expand All @@ -93,7 +94,7 @@ type traefikSource struct {
unstructuredConverter *unstructuredConverter
}

func NewTraefikSource(ctx context.Context, dynamicKubeClient dynamic.Interface, kubeClient kubernetes.Interface, namespace string, annotationFilter string, ignoreHostnameAnnotation bool, disableLegacy bool, disableNew bool) (Source, error) {
func NewTraefikSource(ctx context.Context, dynamicKubeClient dynamic.Interface, kubeClient kubernetes.Interface, namespace string, annotationFilter string, ignoreHostnameAnnotation bool, ignoreIngressRouteSpec bool, disableLegacy bool, disableNew bool) (Source, error) {
// Use shared informer to listen for add/update/delete of Host in the specified namespace.
// Set resync period to 0, to prevent processing when nothing has changed.
informerFactory := dynamicinformer.NewFilteredDynamicSharedInformerFactory(dynamicKubeClient, 0, namespace, nil)
Expand Down Expand Up @@ -157,6 +158,7 @@ func NewTraefikSource(ctx context.Context, dynamicKubeClient dynamic.Interface,
return &traefikSource{
annotationFilter: annotationFilter,
ignoreHostnameAnnotation: ignoreHostnameAnnotation,
ignoreIngressRouteSpec: ignoreIngressRouteSpec,
dynamicKubeClient: dynamicKubeClient,
ingressRouteInformer: ingressRouteInformer,
ingressRouteTcpInformer: ingressRouteTcpInformer,
Expand Down Expand Up @@ -679,17 +681,19 @@ func (ts *traefikSource) endpointsFromIngressRoute(ingressRoute *IngressRoute, t
}
}

for _, route := range ingressRoute.Spec.Routes {
match := route.Match
if !ts.ignoreIngressRouteSpec {
for _, route := range ingressRoute.Spec.Routes {
match := route.Match

for _, hostEntry := range traefikHostExtractor.FindAllString(match, -1) {
for _, host := range traefikValueProcessor.FindAllString(hostEntry, -1) {
host = strings.TrimPrefix(host, "`")
host = strings.TrimSuffix(host, "`")
for _, hostEntry := range traefikHostExtractor.FindAllString(match, -1) {
for _, host := range traefikValueProcessor.FindAllString(hostEntry, -1) {
host = strings.TrimPrefix(host, "`")
host = strings.TrimSuffix(host, "`")

// Checking for host = * is required, as Host(`*`) can be set
if host != "*" && host != "" {
endpoints = append(endpoints, endpointsForHostname(host, targets, ttl, providerSpecific, setIdentifier, resource)...)
// Checking for host = * is required, as Host(`*`) can be set
if host != "*" && host != "" {
endpoints = append(endpoints, endpointsForHostname(host, targets, ttl, providerSpecific, setIdentifier, resource)...)
}
Comment on lines +685 to +696
Copy link
Contributor

@ivankatliarchuk ivankatliarchuk Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks edentical to lines 723-735. Could we have a private method for that as well?

Most likely not part of this change, but I leave it to you to decide, if what I'll open a pull request.

There are triple nested looping

The time complexity of the selected code is O(n * m * k), where:

  • n is the number of routes in ingressRoute.Spec.Routes.
  • m is the number of matches found by traefikHostExtractor.FindAllString.
  • k is the number of hosts found by traefikValueProcessor.FindAllString.
    This is because the code iterates over each route, then for each route, it iterates over each match found, and for each match, it iterates over each host found.

it would be desirable to have something like that

var hosts []string
for _, route := range ingressRoute.Spec.Routes {
	match := route.Match
	h := traefikHostExtractor.FindAllString(match, -1)
	hosts = append(hosts, h...)
}

var processedHosts []string
for _, entry := range hosts {
	pHosts := traefikValueProcessor.FindAllString(entry, -1)
	processedHosts = append(processedHosts, pHosts...)
}
for _, host := range processedHosts {
	host = strings.Trim(host, "`")
	if host != "*" && host != "" {
		endpoints = append(endpoints, endpointsForHostname(host, targets, ttl, providerSpecific, setIdentifier, resource)...)
	}
}

aka time complexity of O(n + m + p) same as O(n) vs O(n * m * p)

}
}
}
Expand All @@ -715,18 +719,20 @@ func (ts *traefikSource) endpointsFromIngressRouteTCP(ingressRoute *IngressRoute
}
}

for _, route := range ingressRoute.Spec.Routes {
match := route.Match
if !ts.ignoreIngressRouteSpec {
for _, route := range ingressRoute.Spec.Routes {
match := route.Match

for _, hostEntry := range traefikHostExtractor.FindAllString(match, -1) {
for _, host := range traefikValueProcessor.FindAllString(hostEntry, -1) {
host = strings.TrimPrefix(host, "`")
host = strings.TrimSuffix(host, "`")
for _, hostEntry := range traefikHostExtractor.FindAllString(match, -1) {
for _, host := range traefikValueProcessor.FindAllString(hostEntry, -1) {
host = strings.TrimPrefix(host, "`")
host = strings.TrimSuffix(host, "`")

// Checking for host = * is required, as HostSNI(`*`) can be set
// in the case of TLS passthrough
if host != "*" && host != "" {
endpoints = append(endpoints, endpointsForHostname(host, targets, ttl, providerSpecific, setIdentifier, resource)...)
// Checking for host = * is required, as HostSNI(`*`) can be set
// in the case of TLS passthrough
if host != "*" && host != "" {
endpoints = append(endpoints, endpointsForHostname(host, targets, ttl, providerSpecific, setIdentifier, resource)...)
}
}
}
}
Expand Down
172 changes: 165 additions & 7 deletions source/traefik_proxy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ func TestTraefikProxyIngressRouteEndpoints(t *testing.T) {
title string
ingressRoute IngressRoute
ignoreHostnameAnnotation bool
ignoreIngressRouteSpec bool
expected []*endpoint.Endpoint
}{
{
Expand Down Expand Up @@ -323,6 +324,44 @@ func TestTraefikProxyIngressRouteEndpoints(t *testing.T) {
},
expected: nil,
},
{
title: "IngressRoute ignoring host rules",
ingressRoute: IngressRoute{
TypeMeta: metav1.TypeMeta{
APIVersion: ingressrouteGVR.GroupVersion().String(),
Kind: "IngressRoute",
},
ObjectMeta: metav1.ObjectMeta{
Name: "ingressroute-multi-host-annotations-match",
Namespace: defaultTraefikNamespace,
Annotations: map[string]string{
"external-dns.alpha.kubernetes.io/hostname": "f.example.com",
"external-dns.alpha.kubernetes.io/target": "target.domain.tld",
"kubernetes.io/ingress.class": "traefik",
},
},
Spec: traefikIngressRouteSpec{
Routes: []traefikRoute{
{
Match: "Host(`g.example.com`, `h.example.com`)",
},
},
},
},
ignoreIngressRouteSpec: true,
expected: []*endpoint.Endpoint{
{
DNSName: "f.example.com",
Targets: []string{"target.domain.tld"},
RecordType: endpoint.RecordTypeCNAME,
RecordTTL: 0,
Labels: endpoint.Labels{
"resource": "ingressroute/traefik/ingressroute-multi-host-annotations-match",
},
ProviderSpecific: endpoint.ProviderSpecific{},
},
},
},
} {
ti := ti
t.Run(ti.title, func(t *testing.T) {
Expand All @@ -349,7 +388,7 @@ func TestTraefikProxyIngressRouteEndpoints(t *testing.T) {
_, err = fakeDynamicClient.Resource(ingressrouteGVR).Namespace(defaultTraefikNamespace).Create(context.Background(), &ir, metav1.CreateOptions{})
assert.NoError(t, err)

source, err := NewTraefikSource(context.TODO(), fakeDynamicClient, fakeKubernetesClient, defaultTraefikNamespace, "kubernetes.io/ingress.class=traefik", ti.ignoreHostnameAnnotation, false, false)
source, err := NewTraefikSource(context.TODO(), fakeDynamicClient, fakeKubernetesClient, defaultTraefikNamespace, "kubernetes.io/ingress.class=traefik", ti.ignoreHostnameAnnotation, ti.ignoreIngressRouteSpec, false, false)
assert.NoError(t, err)
assert.NotNil(t, source)

Expand All @@ -373,6 +412,7 @@ func TestTraefikProxyIngressRouteTCPEndpoints(t *testing.T) {
title string
ingressRouteTCP IngressRouteTCP
ignoreHostnameAnnotation bool
ignoreIngressRouteSpec bool
expected []*endpoint.Endpoint
}{
{
Expand Down Expand Up @@ -617,6 +657,44 @@ func TestTraefikProxyIngressRouteTCPEndpoints(t *testing.T) {
},
expected: nil,
},
{
title: "IngressRouteTCP ignoring IngressRouteTCP",
ingressRouteTCP: IngressRouteTCP{
TypeMeta: metav1.TypeMeta{
APIVersion: ingressrouteTCPGVR.GroupVersion().String(),
Kind: "IngressRouteTCP",
},
ObjectMeta: metav1.ObjectMeta{
Name: "ingressroutetcp-multi-host-annotations-match",
Namespace: defaultTraefikNamespace,
Annotations: map[string]string{
"external-dns.alpha.kubernetes.io/hostname": "f.example.com",
"external-dns.alpha.kubernetes.io/target": "target.domain.tld",
"kubernetes.io/ingress.class": "traefik",
},
},
Spec: traefikIngressRouteTCPSpec{
Routes: []traefikRouteTCP{
{
Match: "HostSNI(`g.example.com`, `h.example.com`)",
},
},
},
},
ignoreIngressRouteSpec: true,
expected: []*endpoint.Endpoint{
{
DNSName: "f.example.com",
Targets: []string{"target.domain.tld"},
RecordType: endpoint.RecordTypeCNAME,
RecordTTL: 0,
Labels: endpoint.Labels{
"resource": "ingressroutetcp/traefik/ingressroutetcp-multi-host-annotations-match",
},
ProviderSpecific: endpoint.ProviderSpecific{},
},
},
},
} {
ti := ti
t.Run(ti.title, func(t *testing.T) {
Expand All @@ -643,7 +721,7 @@ func TestTraefikProxyIngressRouteTCPEndpoints(t *testing.T) {
_, err = fakeDynamicClient.Resource(ingressrouteTCPGVR).Namespace(defaultTraefikNamespace).Create(context.Background(), &ir, metav1.CreateOptions{})
assert.NoError(t, err)

source, err := NewTraefikSource(context.TODO(), fakeDynamicClient, fakeKubernetesClient, defaultTraefikNamespace, "kubernetes.io/ingress.class=traefik", ti.ignoreHostnameAnnotation, false, false)
source, err := NewTraefikSource(context.TODO(), fakeDynamicClient, fakeKubernetesClient, defaultTraefikNamespace, "kubernetes.io/ingress.class=traefik", ti.ignoreHostnameAnnotation, ti.ignoreIngressRouteSpec, false, false)
assert.NoError(t, err)
assert.NotNil(t, source)

Expand All @@ -667,6 +745,7 @@ func TestTraefikProxyIngressRouteUDPEndpoints(t *testing.T) {
title string
ingressRouteUDP IngressRouteUDP
ignoreHostnameAnnotation bool
ignoreIngressRouteSpec bool
expected []*endpoint.Endpoint
}{
{
Expand Down Expand Up @@ -785,7 +864,7 @@ func TestTraefikProxyIngressRouteUDPEndpoints(t *testing.T) {
_, err = fakeDynamicClient.Resource(ingressrouteUDPGVR).Namespace(defaultTraefikNamespace).Create(context.Background(), &ir, metav1.CreateOptions{})
assert.NoError(t, err)

source, err := NewTraefikSource(context.TODO(), fakeDynamicClient, fakeKubernetesClient, defaultTraefikNamespace, "kubernetes.io/ingress.class=traefik", ti.ignoreHostnameAnnotation, false, false)
source, err := NewTraefikSource(context.TODO(), fakeDynamicClient, fakeKubernetesClient, defaultTraefikNamespace, "kubernetes.io/ingress.class=traefik", ti.ignoreHostnameAnnotation, ti.ignoreIngressRouteSpec, false, false)
assert.NoError(t, err)
assert.NotNil(t, source)

Expand All @@ -809,6 +888,7 @@ func TestTraefikProxyOldIngressRouteEndpoints(t *testing.T) {
title string
ingressRoute IngressRoute
ignoreHostnameAnnotation bool
ignoreIngressRouteSpec bool
expected []*endpoint.Endpoint
}{
{
Expand Down Expand Up @@ -1089,6 +1169,44 @@ func TestTraefikProxyOldIngressRouteEndpoints(t *testing.T) {
},
expected: nil,
},
{
title: "IngressRoute ignoring IngressRouteSpec",
ingressRoute: IngressRoute{
TypeMeta: metav1.TypeMeta{
APIVersion: oldIngressrouteGVR.GroupVersion().String(),
Kind: "IngressRoute",
},
ObjectMeta: metav1.ObjectMeta{
Name: "ingressroute-multi-host-annotations-match",
Namespace: defaultTraefikNamespace,
Annotations: map[string]string{
"external-dns.alpha.kubernetes.io/hostname": "f.example.com",
"external-dns.alpha.kubernetes.io/target": "target.domain.tld",
"kubernetes.io/ingress.class": "traefik",
},
},
Spec: traefikIngressRouteSpec{
Routes: []traefikRoute{
{
Match: "Host(`g.example.com`, `h.example.com`)",
},
},
},
},
ignoreIngressRouteSpec: true,
expected: []*endpoint.Endpoint{
{
DNSName: "f.example.com",
Targets: []string{"target.domain.tld"},
RecordType: endpoint.RecordTypeCNAME,
RecordTTL: 0,
Labels: endpoint.Labels{
"resource": "ingressroute/traefik/ingressroute-multi-host-annotations-match",
},
ProviderSpecific: endpoint.ProviderSpecific{},
},
},
},
} {
ti := ti
t.Run(ti.title, func(t *testing.T) {
Expand All @@ -1115,7 +1233,7 @@ func TestTraefikProxyOldIngressRouteEndpoints(t *testing.T) {
_, err = fakeDynamicClient.Resource(oldIngressrouteGVR).Namespace(defaultTraefikNamespace).Create(context.Background(), &ir, metav1.CreateOptions{})
assert.NoError(t, err)

source, err := NewTraefikSource(context.TODO(), fakeDynamicClient, fakeKubernetesClient, defaultTraefikNamespace, "kubernetes.io/ingress.class=traefik", ti.ignoreHostnameAnnotation, false, false)
source, err := NewTraefikSource(context.TODO(), fakeDynamicClient, fakeKubernetesClient, defaultTraefikNamespace, "kubernetes.io/ingress.class=traefik", ti.ignoreHostnameAnnotation, ti.ignoreIngressRouteSpec, false, false)
assert.NoError(t, err)
assert.NotNil(t, source)

Expand All @@ -1139,6 +1257,7 @@ func TestTraefikProxyOldIngressRouteTCPEndpoints(t *testing.T) {
title string
ingressRouteTCP IngressRouteTCP
ignoreHostnameAnnotation bool
ignoreIngressRouteSpec bool
expected []*endpoint.Endpoint
}{
{
Expand Down Expand Up @@ -1382,6 +1501,43 @@ func TestTraefikProxyOldIngressRouteTCPEndpoints(t *testing.T) {
},
},
expected: nil,
}, {
title: "IngressRouteTCP ignoring IngressRouteSpec",
ingressRouteTCP: IngressRouteTCP{
TypeMeta: metav1.TypeMeta{
APIVersion: oldIngressrouteTCPGVR.GroupVersion().String(),
Kind: "IngressRouteTCP",
},
ObjectMeta: metav1.ObjectMeta{
Name: "ingressroutetcp-multi-host-annotations-match",
Namespace: defaultTraefikNamespace,
Annotations: map[string]string{
"external-dns.alpha.kubernetes.io/hostname": "f.example.com",
"external-dns.alpha.kubernetes.io/target": "target.domain.tld",
"kubernetes.io/ingress.class": "traefik",
},
},
Spec: traefikIngressRouteTCPSpec{
Routes: []traefikRouteTCP{
{
Match: "HostSNI(`g.example.com`, `h.example.com`)",
},
},
},
},
ignoreIngressRouteSpec: true,
expected: []*endpoint.Endpoint{
{
DNSName: "f.example.com",
Targets: []string{"target.domain.tld"},
RecordType: endpoint.RecordTypeCNAME,
RecordTTL: 0,
Labels: endpoint.Labels{
"resource": "ingressroutetcp/traefik/ingressroutetcp-multi-host-annotations-match",
},
ProviderSpecific: endpoint.ProviderSpecific{},
},
},
},
} {
ti := ti
Expand Down Expand Up @@ -1409,7 +1565,7 @@ func TestTraefikProxyOldIngressRouteTCPEndpoints(t *testing.T) {
_, err = fakeDynamicClient.Resource(oldIngressrouteTCPGVR).Namespace(defaultTraefikNamespace).Create(context.Background(), &ir, metav1.CreateOptions{})
assert.NoError(t, err)

source, err := NewTraefikSource(context.TODO(), fakeDynamicClient, fakeKubernetesClient, defaultTraefikNamespace, "kubernetes.io/ingress.class=traefik", ti.ignoreHostnameAnnotation, false, false)
source, err := NewTraefikSource(context.TODO(), fakeDynamicClient, fakeKubernetesClient, defaultTraefikNamespace, "kubernetes.io/ingress.class=traefik", ti.ignoreHostnameAnnotation, ti.ignoreIngressRouteSpec, false, false)
assert.NoError(t, err)
assert.NotNil(t, source)

Expand All @@ -1433,6 +1589,7 @@ func TestTraefikProxyOldIngressRouteUDPEndpoints(t *testing.T) {
title string
ingressRouteUDP IngressRouteUDP
ignoreHostnameAnnotation bool
ignoreIngressRouteSpec bool
expected []*endpoint.Endpoint
}{
{
Expand Down Expand Up @@ -1551,7 +1708,7 @@ func TestTraefikProxyOldIngressRouteUDPEndpoints(t *testing.T) {
_, err = fakeDynamicClient.Resource(oldIngressrouteUDPGVR).Namespace(defaultTraefikNamespace).Create(context.Background(), &ir, metav1.CreateOptions{})
assert.NoError(t, err)

source, err := NewTraefikSource(context.TODO(), fakeDynamicClient, fakeKubernetesClient, defaultTraefikNamespace, "kubernetes.io/ingress.class=traefik", ti.ignoreHostnameAnnotation, false, false)
source, err := NewTraefikSource(context.TODO(), fakeDynamicClient, fakeKubernetesClient, defaultTraefikNamespace, "kubernetes.io/ingress.class=traefik", ti.ignoreHostnameAnnotation, ti.ignoreIngressRouteSpec, false, false)
assert.NoError(t, err)
assert.NotNil(t, source)

Expand All @@ -1576,6 +1733,7 @@ func TestTraefikAPIGroupDisableFlags(t *testing.T) {
ingressRoute IngressRoute
gvr schema.GroupVersionResource
ignoreHostnameAnnotation bool
ignoreIngressRouteSpec bool
disableLegacy bool
disableNew bool
expected []*endpoint.Endpoint
Expand Down Expand Up @@ -1714,7 +1872,7 @@ func TestTraefikAPIGroupDisableFlags(t *testing.T) {
_, err = fakeDynamicClient.Resource(ti.gvr).Namespace(defaultTraefikNamespace).Create(context.Background(), &ir, metav1.CreateOptions{})
assert.NoError(t, err)

source, err := NewTraefikSource(context.TODO(), fakeDynamicClient, fakeKubernetesClient, defaultTraefikNamespace, "kubernetes.io/ingress.class=traefik", ti.ignoreHostnameAnnotation, ti.disableLegacy, ti.disableNew)
source, err := NewTraefikSource(context.TODO(), fakeDynamicClient, fakeKubernetesClient, defaultTraefikNamespace, "kubernetes.io/ingress.class=traefik", ti.ignoreHostnameAnnotation, ti.ignoreIngressRouteSpec, ti.disableLegacy, ti.disableNew)
assert.NoError(t, err)
assert.NotNil(t, source)

Expand Down
Loading