Skip to content

chore: fix two bugs associated with istio ingress annotation processing #6439

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

Open
mwhittington21 wants to merge 1 commit into
base: master
Choose a base branch
from
+56 −30
Open

chore: fix two bugs associated with istio ingress annotation processing #6439

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
17 changes: 10 additions & 7 deletions source/istio_gateway.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,11 @@ import (
"sigs.k8s.io/external-dns/source/template"
)

// IstioGatewayIngressSource is the annotation used to determine if the gateway is implemented by an Ingress object
// instead of a standard LoadBalancer service type
// Using var instead of const because annotation keys can be customized
var IstioGatewayIngressSource = annotations.Ingress
// IstioGatewayIngressSource returns the annotation key used to determine if the gateway
// is implemented by an Ingress object instead of a standard LoadBalancer service type.
// This must be a function (not a package-level var) because the annotation prefix can
// be customized at runtime via --annotation-prefix / SetAnnotationPrefix.
func IstioGatewayIngressSource() string { return annotations.Ingress }
Copy link
Copy Markdown
Member

@ivankatliarchuk ivankatliarchuk May 15, 2026

Choose a reason for hiding this comment

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

Technically not clear why this wrapper is even required, we could just remove the wrapper, use annotations.Ingress directly, to satisfy split brain annotation.

The bug related to that feature #5923

We may going to have similar issues elsewhere.


// gatewaySource is an implementation of Source for Istio Gateway objects.
// The gateway implementation uses the spec.servers.hosts values for the hostnames.
Expand Down Expand Up @@ -152,7 +153,8 @@ func (sc *gatewaySource) Endpoints(_ context.Context) ([]*endpoint.Endpoint, err

gwEndpoints, err := sc.endpointsFromGateway(gwHostnames, gateway)
if err != nil {
return nil, err
log.Warnf("Could not generate endpoints for gateway '%s/%s': %v", gateway.Namespace, gateway.Name, err)
Copy link
Copy Markdown
Member

@ivankatliarchuk ivankatliarchuk May 15, 2026

Choose a reason for hiding this comment

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

Need other maintainers view on that one. From high level perspective it make sense, but very much on the edge. As there is is a behaviour change.

The PR's blanket warn+continue on endpointsFromGateway is API change for a specific reason - look at what targetsFromGateway can return errors from:

  endpointsFromGateway
    └─ targetsFromGateway
         ├─ targetsFromIngress        ← per-resource: bad annotation / ingress not found
         └─ EndpointTargetsFromServices ← SYSTEMIC: fails to list services in a namespace

EndpointTargetsFromServices failing means the informer/API is broken — that affects every gateway, not one. Swallowing that with warn+continue would silently skip all gateways each cycle with no escalation.

There's also a double-logging bug: targetsFromIngress already calls log.Error(err) before returning, and then the PR adds log.Warnf(...) at the caller - two log lines at two different levels for the same event.

The error handling belongs inside targetsFromIngress, not at the Endpoints() loop level. When the annotation references an ingress that doesn't exist, that's not an error — it means zero targets for that gateway (the same outcome as a service with no load balancer IPs, which already returns empty with no error).

This is not a call for action, but a probabal change

Both istio_gateway.go and istio_virtualservice.go have identical targetsFromIngress implementations. The fix is the same in both:

  ingress, err := sc.ingressInformer.Lister().Ingresses(namespace).Get(name)
  if err != nil {
      log.Error(err)          // remove this
      return nil, err         // replace with ↓
  }

  ingress, err := sc.ingressInformer.Lister().Ingresses(namespace).Get(name)
  if err != nil {
      log.Warnf("Could not get ingress %s/%s referenced by gateway %s/%s: %v", namespace, name, gateway.Namespace, gateway.Name, err)
      return endpoint.Targets{}, nil
  }

continue
}

// apply template if host is missing on gateway
Expand All @@ -167,7 +169,8 @@ func (sc *gatewaySource) Endpoints(_ context.Context) ([]*endpoint.Endpoint, err
},
)
if err != nil {
Copy link
Copy Markdown
Member

@ivankatliarchuk ivankatliarchuk May 15, 2026

Choose a reason for hiding this comment

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

This should be reverted.

A template error is a configuration error that will fail for every resource on every reconciliation cycle, producing a warning flood without ever halting or surfacing the root cause. Every other source propagates template errors up.

return nil, err
log.Warnf("Could not apply template for gateway '%s/%s': %v", gateway.Namespace, gateway.Name, err)
continue
}

if endpoint.HasNoEmptyEndpoints(gwEndpoints, types.IstioGateway, gateway) {
Expand Down Expand Up @@ -220,7 +223,7 @@ func (sc *gatewaySource) targetsFromGateway(gateway *networkingv1.Gateway) (endp
return targets, nil
}

ingressStr, ok := gateway.Annotations[IstioGatewayIngressSource]
ingressStr, ok := gateway.Annotations[IstioGatewayIngressSource()]
if ok && ingressStr != "" {
return sc.targetsFromIngress(ingressStr, gateway)
}
Expand Down
43 changes: 32 additions & 11 deletions source/istio_gateway_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,27 @@ import (
// This is a compile-time validation that gatewaySource is a Source.
var _ Source = &gatewaySource{}

// TestAnnotationPrefixPropagation verifies that IstioGatewayIngressSource() reflects
// changes made by SetAnnotationPrefix at runtime. This is a regression test for a bug
// where it was a package-level var evaluated at import time — before SetAnnotationPrefix
// was called — causing annotation lookups to use the wrong prefix and silently produce
// no endpoints, leading to record deletion under sync policy.
func TestAnnotationPrefixPropagation(t *testing.T) {
t.Cleanup(func() { annotations.SetAnnotationPrefix(annotations.DefaultAnnotationPrefix) })

assert.Equal(t, annotations.DefaultAnnotationPrefix+"ingress", IstioGatewayIngressSource())

// Simulate --annotation-prefix=custom.io/
annotations.SetAnnotationPrefix("custom.io/")
assert.Equal(t, "custom.io/ingress", IstioGatewayIngressSource(),
"IstioGatewayIngressSource() must reflect updated annotation prefix")

// Simulate changing it again — must not be cached from first call
annotations.SetAnnotationPrefix("another.io/")
assert.Equal(t, "another.io/ingress", IstioGatewayIngressSource(),
"IstioGatewayIngressSource() must track subsequent prefix changes")
}

type GatewaySuite struct {
suite.Suite
source Source
Expand Down Expand Up @@ -206,7 +227,7 @@ func testEndpointsFromGatewayConfig(t *testing.T) {
},
config: fakeGatewayConfig{
annotations: map[string]string{
IstioGatewayIngressSource: "ingress1",
IstioGatewayIngressSource(): "ingress1",
},
dnsnames: [][]string{
{"foo.bar"},
Expand Down Expand Up @@ -258,7 +279,7 @@ func testEndpointsFromGatewayConfig(t *testing.T) {
},
config: fakeGatewayConfig{
annotations: map[string]string{
IstioGatewayIngressSource: "ingress1",
IstioGatewayIngressSource(): "ingress1",
},
dnsnames: [][]string{
{"foo.bar"},
Expand Down Expand Up @@ -319,7 +340,7 @@ func testEndpointsFromGatewayConfig(t *testing.T) {
},
config: fakeGatewayConfig{
annotations: map[string]string{
IstioGatewayIngressSource: "ingress1",
IstioGatewayIngressSource(): "ingress1",
},
dnsnames: [][]string{
{""},
Expand Down Expand Up @@ -380,7 +401,7 @@ func testEndpointsFromGatewayConfig(t *testing.T) {
},
config: fakeGatewayConfig{
annotations: map[string]string{
IstioGatewayIngressSource: "istio-other2/ingress1",
IstioGatewayIngressSource(): "istio-other2/ingress1",
},
dnsnames: [][]string{
{"foo.bar"}, // Kubernetes requires removal of trailing dot
Expand Down Expand Up @@ -634,7 +655,7 @@ func testGatewayEndpoints(t *testing.T) {
namespace: "testing1",
dnsnames: [][]string{{"example.org"}},
annotations: map[string]string{
IstioGatewayIngressSource: "testing2/ingress1",
IstioGatewayIngressSource(): "testing2/ingress1",
},
},
},
Expand Down Expand Up @@ -1000,7 +1021,7 @@ func testGatewayEndpoints(t *testing.T) {
name: "fake3",
namespace: "",
annotations: map[string]string{
IstioGatewayIngressSource: "not-real/ingress1",
IstioGatewayIngressSource(): "not-real/ingress1",
annotations.TargetKey: "1.2.3.4",
},
dnsnames: [][]string{{"example3.org"}},
Expand Down Expand Up @@ -1142,7 +1163,7 @@ func testGatewayEndpoints(t *testing.T) {
name: "fake1",
namespace: "",
annotations: map[string]string{
IstioGatewayIngressSource: "ingress1",
IstioGatewayIngressSource(): "ingress1",
annotations.HostnameKey: "dns-through-hostname.com",
annotations.TargetKey: "gateway-target.com",
},
Expand Down Expand Up @@ -1317,7 +1338,7 @@ func testGatewayEndpoints(t *testing.T) {
name: "fake1",
namespace: "",
annotations: map[string]string{
IstioGatewayIngressSource: "",
IstioGatewayIngressSource(): "",
},
dnsnames: [][]string{},
},
Expand Down Expand Up @@ -1451,13 +1472,13 @@ func testGatewayEndpoints(t *testing.T) {
name: "fake1",
namespace: "",
annotations: map[string]string{
IstioGatewayIngressSource: "ingress2",
IstioGatewayIngressSource(): "ingress2",
},
dnsnames: [][]string{{"new.org"}},
},
},
expected: []*endpoint.Endpoint{},
expectError: true,
expectError: false, // bad annotation should be skipped gracefully, not crash the controller
},
} {

Expand Down Expand Up @@ -1775,7 +1796,7 @@ func TestSingleGatewayMultipleServicesPointingToSameLoadBalancer(t *testing.T) {
{
Hosts: []string{"example.org"},
Tls: &istionetworking.ServerTLSSettings{
ServerCertificate: IstioGatewayIngressSource,
ServerCertificate: IstioGatewayIngressSource(),
Mode: istionetworking.ServerTLSSettings_SIMPLE,
},
},
Expand Down
8 changes: 5 additions & 3 deletions source/istio_virtualservice.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,8 @@ func (sc *virtualServiceSource) Endpoints(ctx context.Context) ([]*endpoint.Endp

gwEndpoints, err := sc.endpointsFromVirtualService(ctx, vService)
if err != nil {
return nil, err
log.Warnf("Could not generate endpoints for VirtualService '%s/%s': %v", vService.Namespace, vService.Name, err)
continue
}

// apply template if host is missing on VirtualService
Expand All @@ -166,7 +167,8 @@ func (sc *virtualServiceSource) Endpoints(ctx context.Context) ([]*endpoint.Endp
func() ([]*endpoint.Endpoint, error) { return sc.endpointsFromTemplate(ctx, vService) },
)
if err != nil {
return nil, err
log.Warnf("Could not apply template for VirtualService '%s/%s': %v", vService.Namespace, vService.Name, err)
continue
}

if endpoint.HasNoEmptyEndpoints(gwEndpoints, types.IstioVirtualService, vService) {
Expand Down Expand Up @@ -403,7 +405,7 @@ func (sc *virtualServiceSource) targetsFromGateway(gateway *networkingv1.Gateway
return targets, nil
}

ingressStr, ok := gateway.Annotations[IstioGatewayIngressSource]
ingressStr, ok := gateway.Annotations[IstioGatewayIngressSource()]
if ok && ingressStr != "" {
return sc.targetsFromIngress(ingressStr, gateway)
}
Expand Down
18 changes: 9 additions & 9 deletions source/istio_virtualservice_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,7 @@ func testEndpointsFromVirtualServiceConfig(t *testing.T) {
name: "mygw",
dnsnames: [][]string{{"*"}},
annotations: map[string]string{
IstioGatewayIngressSource: "ingress1",
IstioGatewayIngressSource(): "ingress1",
},
},
vsconfig: fakeVirtualServiceConfig{
Expand Down Expand Up @@ -616,7 +616,7 @@ func testEndpointsFromVirtualServiceConfig(t *testing.T) {
name: "mygw",
dnsnames: [][]string{{"*"}},
annotations: map[string]string{
IstioGatewayIngressSource: "ingress/ingress2",
IstioGatewayIngressSource(): "ingress/ingress2",
},
},
vsconfig: fakeVirtualServiceConfig{
Expand Down Expand Up @@ -790,15 +790,15 @@ func testVirtualServiceEndpoints(t *testing.T) {
namespace: namespace,
dnsnames: [][]string{{"example.org"}},
annotations: map[string]string{
IstioGatewayIngressSource: "ingress1",
IstioGatewayIngressSource(): "ingress1",
},
},
{
name: "fake2",
namespace: namespace,
dnsnames: [][]string{{"new.org"}},
annotations: map[string]string{
IstioGatewayIngressSource: "ingress1",
IstioGatewayIngressSource(): "ingress1",
},
},
},
Expand Down Expand Up @@ -1063,7 +1063,7 @@ func testVirtualServiceEndpoints(t *testing.T) {
namespace: "testing1",
dnsnames: [][]string{{"*"}},
annotations: map[string]string{
IstioGatewayIngressSource: "ingress1",
IstioGatewayIngressSource(): "ingress1",
},
},
},
Expand Down Expand Up @@ -1297,7 +1297,7 @@ func testVirtualServiceEndpoints(t *testing.T) {
namespace: namespace,
dnsnames: [][]string{{"*"}},
annotations: map[string]string{
IstioGatewayIngressSource: "ingress2",
IstioGatewayIngressSource(): "ingress2",
},
},
},
Expand All @@ -1310,7 +1310,7 @@ func testVirtualServiceEndpoints(t *testing.T) {
},
},
expected: []*endpoint.Endpoint{},
expectError: true,
expectError: false, // bad annotation should be skipped gracefully, not crash the controller
},
{
title: "our controller type is dns-controller",
Expand Down Expand Up @@ -1603,7 +1603,7 @@ func testVirtualServiceEndpoints(t *testing.T) {
dnsnames: [][]string{{"*"}},
annotations: map[string]string{
annotations.TargetKey: "gateway-target.com",
IstioGatewayIngressSource: "ingress1",
IstioGatewayIngressSource(): "ingress1",
},
},
},
Expand Down Expand Up @@ -1957,7 +1957,7 @@ func testVirtualServiceEndpoints(t *testing.T) {
"app": "igw4",
},
annotations: map[string]string{
IstioGatewayIngressSource: "testing1/ingress1",
IstioGatewayIngressSource(): "testing1/ingress1",
},
},
},
Expand Down
Loading