From 1253afa11e3b730f4a49d2c023c502432694592e Mon Sep 17 00:00:00 2001 From: David Pait Date: Sat, 5 Aug 2023 00:45:11 -0400 Subject: [PATCH 1/8] add supoort for ingress backed istio gateways --- docs/tutorials/istio.md | 28 ++++++++++++++++++ source/istio_virtualservice.go | 52 ++++++++++++++++++++++++++++++++-- 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/docs/tutorials/istio.md b/docs/tutorials/istio.md index 0a519a46bc..0d667125ae 100644 --- a/docs/tutorials/istio.md +++ b/docs/tutorials/istio.md @@ -270,6 +270,34 @@ transfer-encoding: chunked **Note:** The `-H` flag in the original Istio tutorial is no longer necessary in the `curl` commands. +### Optional Gateway Annotation + +To support setups where an Ingress resource is used provision an external LB you can add the following annotation to your Gateway + +**Note:** The Ingress namespace can be omitted if its in the same namespace as the gateway + +```bash +$ cat < 0 { return } + ingressStr, found := gateway.Annotations[IstioIngressBackedGateway] + if found && ingressStr != "" { + targets, err = sc.targetsFromIngress(ctx, ingressStr, gateway) + return + } + services, err := sc.serviceInformer.Lister().Services(sc.namespace).List(labels.Everything()) if err != nil { log.Error(err) From 362b233833a81e2d8c533a2a920c8075dabd0bca Mon Sep 17 00:00:00 2001 From: David Pait Date: Tue, 8 Aug 2023 08:04:51 -0400 Subject: [PATCH 2/8] update istio gateway annotation name and docs --- docs/tutorials/istio.md | 2 +- source/istio_virtualservice.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/tutorials/istio.md b/docs/tutorials/istio.md index 0d667125ae..ed1945b5d7 100644 --- a/docs/tutorials/istio.md +++ b/docs/tutorials/istio.md @@ -284,7 +284,7 @@ metadata: name: httpbin-gateway namespace: istio-system annotations: - "external-dns.alpha.kubernetes.io/istio-ingress-backed-gateway": "$ingressNamespace/$ingressName" + "external-dns.alpha.kubernetes.io/ingress": "$ingressNamespace/$ingressName" spec: selector: istio: ingressgateway # use Istio default gateway implementation diff --git a/source/istio_virtualservice.go b/source/istio_virtualservice.go index a2b6f8b9bd..d55263d49e 100644 --- a/source/istio_virtualservice.go +++ b/source/istio_virtualservice.go @@ -44,7 +44,7 @@ const IstioMeshGateway = "mesh" // IstioIngressBackedGateway is used to determine if the gateway is implemented by an Ingress object // instead of a standard LoadBalancer service type -const IstioIngressBackedGateway = "external-dns.alpha.kubernetes.io/istio-ingress-backed-gateway" +const IstioIngressBackedGateway = "external-dns.alpha.kubernetes.io/ingress" // virtualServiceSource is an implementation of Source for Istio VirtualService objects. // The implementation uses the spec.hosts values for the hostnames. From cb2772c6aebd5a1ab0d31b29716773695c404aef Mon Sep 17 00:00:00 2001 From: David Pait Date: Tue, 8 Aug 2023 08:05:42 -0400 Subject: [PATCH 3/8] add istio gateway ingress annotation support to gateway source --- source/istio_gateway.go | 39 +++++++++++++++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/source/istio_gateway.go b/source/istio_gateway.go index 76c1094cf4..643b7b1a34 100644 --- a/source/istio_gateway.go +++ b/source/istio_gateway.go @@ -166,7 +166,7 @@ func (sc *gatewaySource) Endpoints(ctx context.Context) ([]*endpoint.Endpoint, e continue } - gwEndpoints, err := sc.endpointsFromGateway(gwHostnames, gateway) + gwEndpoints, err := sc.endpointsFromGateway(ctx, gwHostnames, gateway) if err != nil { return nil, err } @@ -232,12 +232,43 @@ func (sc *gatewaySource) setResourceLabel(gateway *networkingv1alpha3.Gateway, e } } -func (sc *gatewaySource) targetsFromGateway(gateway *networkingv1alpha3.Gateway) (targets endpoint.Targets, err error) { +func (sc *gatewaySource) targetsFromIngress(ctx context.Context, ingressStr string, gateway *networkingv1alpha3.Gateway) (targets endpoint.Targets, err error) { + namespace, name, err := parseIngress(ingressStr) + if err != nil { + log.Debugf("Failed parsing ingressStr %s of Gateway %s/%s", ingressStr, gateway.Namespace, gateway.Name) + return nil, err + } + if namespace == "" { + namespace = gateway.Namespace + } + + ingress, err := sc.kubeClient.NetworkingV1().Ingresses(namespace).Get(ctx, name, metav1.GetOptions{}) + if err != nil { + log.Error(err) + return + } + for _, lb := range ingress.Status.LoadBalancer.Ingress { + if lb.IP != "" { + targets = append(targets, lb.IP) + } else if lb.Hostname != "" { + targets = append(targets, lb.Hostname) + } + } + return +} + +func (sc *gatewaySource) targetsFromGateway(ctx context.Context, gateway *networkingv1alpha3.Gateway) (targets endpoint.Targets, err error) { targets = getTargetsFromTargetAnnotation(gateway.Annotations) if len(targets) > 0 { return } + ingressStr, found := gateway.Annotations[IstioIngressBackedGateway] + if found && ingressStr != "" { + targets, err = sc.targetsFromIngress(ctx, ingressStr, gateway) + return + } + services, err := sc.serviceInformer.Lister().Services(sc.namespace).List(labels.Everything()) if err != nil { log.Error(err) @@ -262,7 +293,7 @@ func (sc *gatewaySource) targetsFromGateway(gateway *networkingv1alpha3.Gateway) } // endpointsFromGatewayConfig extracts the endpoints from an Istio Gateway Config object -func (sc *gatewaySource) endpointsFromGateway(hostnames []string, gateway *networkingv1alpha3.Gateway) ([]*endpoint.Endpoint, error) { +func (sc *gatewaySource) endpointsFromGateway(ctx context.Context, hostnames []string, gateway *networkingv1alpha3.Gateway) ([]*endpoint.Endpoint, error) { var endpoints []*endpoint.Endpoint annotations := gateway.Annotations @@ -274,7 +305,7 @@ func (sc *gatewaySource) endpointsFromGateway(hostnames []string, gateway *netwo targets := getTargetsFromTargetAnnotation(annotations) if len(targets) == 0 { - targets, err = sc.targetsFromGateway(gateway) + targets, err = sc.targetsFromGateway(ctx, gateway) if err != nil { return nil, err } From 08b592d2fd749e1a1a8aea90737df527ce991070 Mon Sep 17 00:00:00 2001 From: David Pait Date: Tue, 8 Aug 2023 08:28:39 -0400 Subject: [PATCH 4/8] update istio gateway ingress source annotation constant --- source/istio_gateway.go | 2 +- source/istio_virtualservice.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/source/istio_gateway.go b/source/istio_gateway.go index 643b7b1a34..13861d72d7 100644 --- a/source/istio_gateway.go +++ b/source/istio_gateway.go @@ -263,7 +263,7 @@ func (sc *gatewaySource) targetsFromGateway(ctx context.Context, gateway *networ return } - ingressStr, found := gateway.Annotations[IstioIngressBackedGateway] + ingressStr, found := gateway.Annotations[IstioGatewayIngressSource] if found && ingressStr != "" { targets, err = sc.targetsFromIngress(ctx, ingressStr, gateway) return diff --git a/source/istio_virtualservice.go b/source/istio_virtualservice.go index d55263d49e..1143bf0fa8 100644 --- a/source/istio_virtualservice.go +++ b/source/istio_virtualservice.go @@ -42,9 +42,9 @@ import ( // IstioMeshGateway is the built in gateway for all sidecars const IstioMeshGateway = "mesh" -// IstioIngressBackedGateway is used to determine if the gateway is implemented by an Ingress object +// IstioGatewayIngressSource is the annotation used to determine if the gateway is implemented by an Ingress object // instead of a standard LoadBalancer service type -const IstioIngressBackedGateway = "external-dns.alpha.kubernetes.io/ingress" +const IstioGatewayIngressSource = "external-dns.alpha.kubernetes.io/ingress" // virtualServiceSource is an implementation of Source for Istio VirtualService objects. // The implementation uses the spec.hosts values for the hostnames. @@ -479,7 +479,7 @@ func (sc *virtualServiceSource) targetsFromGateway(ctx context.Context, gateway return } - ingressStr, found := gateway.Annotations[IstioIngressBackedGateway] + ingressStr, found := gateway.Annotations[IstioGatewayIngressSource] if found && ingressStr != "" { targets, err = sc.targetsFromIngress(ctx, ingressStr, gateway) return From f9600b7b35c26b310144146f4183299b492ea89a Mon Sep 17 00:00:00 2001 From: David Pait Date: Tue, 8 Aug 2023 11:47:40 -0400 Subject: [PATCH 5/8] move ingress annotation and parseIngress to gateway source --- source/istio_gateway.go | 17 +++++++++++++++++ source/istio_virtualservice.go | 17 ----------------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/source/istio_gateway.go b/source/istio_gateway.go index 13861d72d7..6ca6c9994e 100644 --- a/source/istio_gateway.go +++ b/source/istio_gateway.go @@ -38,6 +38,10 @@ import ( "sigs.k8s.io/external-dns/endpoint" ) +// IstioGatewayIngressSource is the annotation used to determine if the gateway is implemented by an Ingress object +// instead of a standard LoadBalancer service type +const IstioGatewayIngressSource = "external-dns.alpha.kubernetes.io/ingress" + // gatewaySource is an implementation of Source for Istio Gateway objects. // The gateway implementation uses the spec.servers.hosts values for the hostnames. // Use targetAnnotationKey to explicitly set Endpoint. @@ -232,6 +236,19 @@ func (sc *gatewaySource) setResourceLabel(gateway *networkingv1alpha3.Gateway, e } } +func parseIngress(ingress string) (namespace, name string, err error) { + parts := strings.Split(ingress, "/") + if len(parts) == 2 { + namespace, name = parts[0], parts[1] + } else if len(parts) == 1 { + name = parts[0] + } else { + err = fmt.Errorf("invalid ingress name (name or namespace/name) found '%v'", ingress) + } + + return +} + func (sc *gatewaySource) targetsFromIngress(ctx context.Context, ingressStr string, gateway *networkingv1alpha3.Gateway) (targets endpoint.Targets, err error) { namespace, name, err := parseIngress(ingressStr) if err != nil { diff --git a/source/istio_virtualservice.go b/source/istio_virtualservice.go index 1143bf0fa8..3e7f79a749 100644 --- a/source/istio_virtualservice.go +++ b/source/istio_virtualservice.go @@ -42,10 +42,6 @@ import ( // IstioMeshGateway is the built in gateway for all sidecars const IstioMeshGateway = "mesh" -// IstioGatewayIngressSource is the annotation used to determine if the gateway is implemented by an Ingress object -// instead of a standard LoadBalancer service type -const IstioGatewayIngressSource = "external-dns.alpha.kubernetes.io/ingress" - // virtualServiceSource is an implementation of Source for Istio VirtualService objects. // The implementation uses the spec.hosts values for the hostnames. // Use targetAnnotationKey to explicitly set Endpoint. @@ -435,19 +431,6 @@ func parseGateway(gateway string) (namespace, name string, err error) { return } -func parseIngress(ingress string) (namespace, name string, err error) { - parts := strings.Split(ingress, "/") - if len(parts) == 2 { - namespace, name = parts[0], parts[1] - } else if len(parts) == 1 { - name = parts[0] - } else { - err = fmt.Errorf("invalid ingress name (name or namespace/name) found '%v'", ingress) - } - - return -} - func (sc *virtualServiceSource) targetsFromIngress(ctx context.Context, ingressStr string, gateway *networkingv1alpha3.Gateway) (targets endpoint.Targets, err error) { namespace, name, err := parseIngress(ingressStr) if err != nil { From 0354d76ce0f90ea89ffd11acce1c2cd9762c33f9 Mon Sep 17 00:00:00 2001 From: David Pait Date: Tue, 8 Aug 2023 11:51:20 -0400 Subject: [PATCH 6/8] add unit tests for gateway ingress source annotation --- source/istio_gateway_test.go | 274 ++++++++++++++++++++- source/istio_virtualservice_test.go | 358 +++++++++++++++++++++++++++- 2 files changed, 621 insertions(+), 11 deletions(-) diff --git a/source/istio_gateway_test.go b/source/istio_gateway_test.go index 7ca79623ca..8fe065b974 100644 --- a/source/istio_gateway_test.go +++ b/source/istio_gateway_test.go @@ -20,6 +20,8 @@ import ( "context" "testing" + networkv1 "k8s.io/api/networking/v1" + "github.com/pkg/errors" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -41,6 +43,7 @@ type GatewaySuite struct { suite.Suite source Source lbServices []*v1.Service + ingresses []*networkv1.Ingress } func (suite *GatewaySuite) SetupTest() { @@ -68,6 +71,26 @@ func (suite *GatewaySuite) SetupTest() { suite.NoError(err, "should succeed") } + suite.ingresses = []*networkv1.Ingress{ + (fakeIngress{ + ips: []string{"2.2.2.2"}, + hostnames: []string{"v2"}, + namespace: "istio-system", + name: "istio-ingress", + }).Ingress(), + (fakeIngress{ + ips: []string{"3.3.3.3"}, + hostnames: []string{"v62"}, + namespace: "istio-system", + name: "istio-ingress2", + }).Ingress(), + } + + for _, ingress := range suite.ingresses { + _, err = fakeKubernetesClient.NetworkingV1().Ingresses(ingress.Namespace).Create(context.Background(), ingress, metav1.CreateOptions{}) + suite.NoError(err, "should succeed") + } + suite.source, err = NewIstioGatewaySource( context.TODO(), fakeKubernetesClient, @@ -167,6 +190,7 @@ func testEndpointsFromGatewayConfig(t *testing.T) { for _, ti := range []struct { title string lbServices []fakeIngressGatewayService + ingresses []fakeIngress config fakeGatewayConfig expected []*endpoint.Endpoint }{ @@ -230,6 +254,30 @@ func testEndpointsFromGatewayConfig(t *testing.T) { }, }, }, + { + title: "one rule.host one ingress.IP", + ingresses: []fakeIngress{ + { + name: "ingress1", + ips: []string{"8.8.8.8"}, + }, + }, + config: fakeGatewayConfig{ + annotations: map[string]string{ + IstioGatewayIngressSource: "ingress1", + }, + dnsnames: [][]string{ + {"foo.bar"}, + }, + }, + expected: []*endpoint.Endpoint{ + { + DNSName: "foo.bar", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"8.8.8.8"}, + }, + }, + }, { title: "one rule.host two lb.IP and two lb.Hostname", lbServices: []fakeIngressGatewayService{ @@ -256,6 +304,36 @@ func testEndpointsFromGatewayConfig(t *testing.T) { }, }, }, + { + title: "one rule.host two ingress.IP and two ingress.Hostname", + ingresses: []fakeIngress{ + { + name: "ingress1", + ips: []string{"8.8.8.8", "127.0.0.1"}, + hostnames: []string{"elb.com", "alb.com"}, + }, + }, + config: fakeGatewayConfig{ + annotations: map[string]string{ + IstioGatewayIngressSource: "ingress1", + }, + dnsnames: [][]string{ + {"foo.bar"}, + }, + }, + expected: []*endpoint.Endpoint{ + { + DNSName: "foo.bar", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"8.8.8.8", "127.0.0.1"}, + }, + { + DNSName: "foo.bar", + RecordType: endpoint.RecordTypeCNAME, + Targets: endpoint.Targets{"elb.com", "alb.com"}, + }, + }, + }, { title: "no rule.host", lbServices: []fakeIngressGatewayService{ @@ -284,6 +362,25 @@ func testEndpointsFromGatewayConfig(t *testing.T) { }, expected: []*endpoint.Endpoint{}, }, + { + title: "one empty rule.host with gateway ingress annotation", + ingresses: []fakeIngress{ + { + name: "ingress1", + ips: []string{"8.8.8.8", "127.0.0.1"}, + hostnames: []string{"elb.com", "alb.com"}, + }, + }, + config: fakeGatewayConfig{ + annotations: map[string]string{ + IstioGatewayIngressSource: "ingress1", + }, + dnsnames: [][]string{ + {""}, + }, + }, + expected: []*endpoint.Endpoint{}, + }, { title: "no targets", lbServices: []fakeIngressGatewayService{{}}, @@ -321,17 +418,47 @@ func testEndpointsFromGatewayConfig(t *testing.T) { }, }, }, + { + title: "one gateway, ingress in seperate namespace", + ingresses: []fakeIngress{ + { + hostnames: []string{"lb.com"}, + namespace: "istio-other2", + name: "ingress1", + }, + { + hostnames: []string{"lb2.com"}, + namespace: "istio-other", + name: "ingress2", + }, + }, + config: fakeGatewayConfig{ + annotations: map[string]string{ + IstioGatewayIngressSource: "istio-other2/ingress1", + }, + dnsnames: [][]string{ + {"foo.bar"}, // Kubernetes requires removal of trailing dot + }, + }, + expected: []*endpoint.Endpoint{ + { + DNSName: "foo.bar", + RecordType: endpoint.RecordTypeCNAME, + Targets: endpoint.Targets{"lb.com"}, + }, + }, + }, } { ti := ti t.Run(ti.title, func(t *testing.T) { t.Parallel() gatewayCfg := ti.config.Config() - if source, err := newTestGatewaySource(ti.lbServices); err != nil { + if source, err := newTestGatewaySource(ti.lbServices, ti.ingresses); err != nil { require.NoError(t, err) } else if hostnames, err := source.hostNamesFromGateway(gatewayCfg); err != nil { require.NoError(t, err) - } else if endpoints, err := source.endpointsFromGateway(hostnames, gatewayCfg); err != nil { + } else if endpoints, err := source.endpointsFromGateway(context.Background(), hostnames, gatewayCfg); err != nil { require.NoError(t, err) } else { validateEndpoints(t, endpoints, ti.expected) @@ -348,6 +475,7 @@ func testGatewayEndpoints(t *testing.T) { targetNamespace string annotationFilter string lbServices []fakeIngressGatewayService + ingresses []fakeIngress configItems []fakeGatewayConfig expected []*endpoint.Endpoint expectError bool @@ -477,6 +605,40 @@ func testGatewayEndpoints(t *testing.T) { }, }, }, + { + title: "one simple gateways on different namespace and a target namespace, one ingress service", + targetNamespace: "testing1", + ingresses: []fakeIngress{ + { + name: "ingress1", + ips: []string{"8.8.8.8"}, + hostnames: []string{"lb.com"}, + namespace: "testing2", + }, + }, + configItems: []fakeGatewayConfig{ + { + name: "fake1", + namespace: "testing1", + dnsnames: [][]string{{"example.org"}}, + annotations: map[string]string{ + IstioGatewayIngressSource: "testing2/ingress1", + }, + }, + }, + expected: []*endpoint.Endpoint{ + { + DNSName: "example.org", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"8.8.8.8"}, + }, + { + DNSName: "example.org", + RecordType: endpoint.RecordTypeCNAME, + Targets: endpoint.Targets{"lb.com"}, + }, + }, + }, { title: "valid matching annotation filter expression", targetNamespace: "", @@ -808,7 +970,8 @@ func testGatewayEndpoints(t *testing.T) { name: "fake3", namespace: "", annotations: map[string]string{ - targetAnnotationKey: "1.2.3.4", + IstioGatewayIngressSource: "not-real/ingress1", + targetAnnotationKey: "1.2.3.4", }, dnsnames: [][]string{{"example3.org"}}, }, @@ -930,6 +1093,45 @@ func testGatewayEndpoints(t *testing.T) { }, }, }, + { + title: "gateway rules with hostname, target and ingress annotation", + targetNamespace: "", + lbServices: []fakeIngressGatewayService{ + { + ips: []string{}, + }, + }, + ingresses: []fakeIngress{ + { + name: "ingress1", + ips: []string{}, + }, + }, + configItems: []fakeGatewayConfig{ + { + name: "fake1", + namespace: "", + annotations: map[string]string{ + IstioGatewayIngressSource: "ingress1", + hostnameAnnotationKey: "dns-through-hostname.com", + targetAnnotationKey: "gateway-target.com", + }, + dnsnames: [][]string{{"example.org"}}, + }, + }, + expected: []*endpoint.Endpoint{ + { + DNSName: "example.org", + Targets: endpoint.Targets{"gateway-target.com"}, + RecordType: endpoint.RecordTypeCNAME, + }, + { + DNSName: "dns-through-hostname.com", + Targets: endpoint.Targets{"gateway-target.com"}, + RecordType: endpoint.RecordTypeCNAME, + }, + }, + }, { title: "gateway rules with annotation and custom TTL", targetNamespace: "", @@ -1064,6 +1266,35 @@ func testGatewayEndpoints(t *testing.T) { expected: []*endpoint.Endpoint{}, fqdnTemplate: "{{.Name}}.ext-dns.test.com", }, + { + title: "Gateway with empty ingress annotation", + targetNamespace: "", + lbServices: []fakeIngressGatewayService{ + { + ips: []string{}, + hostnames: []string{}, + }, + }, + ingresses: []fakeIngress{ + { + name: "ingress1", + ips: []string{}, + hostnames: []string{}, + }, + }, + configItems: []fakeGatewayConfig{ + { + name: "fake1", + namespace: "", + annotations: map[string]string{ + IstioGatewayIngressSource: "", + }, + dnsnames: [][]string{}, + }, + }, + expected: []*endpoint.Endpoint{}, + fqdnTemplate: "{{.Name}}.ext-dns.test.com", + }, { title: "ignore hostname annotations", targetNamespace: "", @@ -1176,6 +1407,28 @@ func testGatewayEndpoints(t *testing.T) { }, }, }, + { + title: "gateways with ingress annotation; ingress not found", + targetNamespace: "", + ingresses: []fakeIngress{ + { + name: "ingress1", + ips: []string{"8.8.8.8"}, + }, + }, + configItems: []fakeGatewayConfig{ + { + name: "fake1", + namespace: "", + annotations: map[string]string{ + IstioGatewayIngressSource: "ingress2", + }, + dnsnames: [][]string{{"new.org"}}, + }, + }, + expected: []*endpoint.Endpoint{}, + expectError: true, + }, } { ti := ti t.Run(ti.title, func(t *testing.T) { @@ -1189,6 +1442,12 @@ func testGatewayEndpoints(t *testing.T) { require.NoError(t, err) } + for _, ing := range ti.ingresses { + ingress := ing.Ingress() + _, err := fakeKubernetesClient.NetworkingV1().Ingresses(ingress.Namespace).Create(context.Background(), ingress, metav1.CreateOptions{}) + require.NoError(t, err) + } + fakeIstioClient := istiofake.NewSimpleClientset() for _, config := range ti.configItems { gatewayCfg := config.Config() @@ -1221,7 +1480,7 @@ func testGatewayEndpoints(t *testing.T) { } // gateway specific helper functions -func newTestGatewaySource(loadBalancerList []fakeIngressGatewayService) (*gatewaySource, error) { +func newTestGatewaySource(loadBalancerList []fakeIngressGatewayService, ingressList []fakeIngress) (*gatewaySource, error) { fakeKubernetesClient := fake.NewSimpleClientset() fakeIstioClient := istiofake.NewSimpleClientset() @@ -1232,6 +1491,13 @@ func newTestGatewaySource(loadBalancerList []fakeIngressGatewayService) (*gatewa return nil, err } } + for _, ing := range ingressList { + ingress := ing.Ingress() + _, err := fakeKubernetesClient.NetworkingV1().Ingresses(ingress.Namespace).Create(context.Background(), ingress, metav1.CreateOptions{}) + if err != nil { + return nil, err + } + } src, err := NewIstioGatewaySource( context.TODO(), diff --git a/source/istio_virtualservice_test.go b/source/istio_virtualservice_test.go index 551be67061..d907da2eb5 100644 --- a/source/istio_virtualservice_test.go +++ b/source/istio_virtualservice_test.go @@ -31,6 +31,7 @@ import ( istiofake "istio.io/client-go/pkg/clientset/versioned/fake" fakenetworking3 "istio.io/client-go/pkg/clientset/versioned/typed/networking/v1alpha3/fake" v1 "k8s.io/api/core/v1" + networkv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/kubernetes/fake" @@ -46,6 +47,7 @@ type VirtualServiceSuite struct { suite.Suite source Source lbServices []*v1.Service + ingresses []*networkv1.Ingress gwconfig *networkingv1alpha3.Gateway vsconfig *networkingv1alpha3.VirtualService } @@ -75,6 +77,26 @@ func (suite *VirtualServiceSuite) SetupTest() { suite.NoError(err, "should succeed") } + suite.ingresses = []*networkv1.Ingress{ + (fakeIngress{ + ips: []string{"2.2.2.2"}, + hostnames: []string{"v2"}, + namespace: "istio-system", + name: "istio-ingress", + }).Ingress(), + (fakeIngress{ + ips: []string{"3.3.3.3"}, + hostnames: []string{"v62"}, + namespace: "istio-system", + name: "istio-ingress2", + }).Ingress(), + } + + for _, ingress := range suite.ingresses { + _, err = fakeKubernetesClient.NetworkingV1().Ingresses(ingress.Namespace).Create(context.Background(), ingress, metav1.CreateOptions{}) + suite.NoError(err, "should succeed") + } + suite.gwconfig = (fakeGatewayConfig{ name: "foo-gateway-with-targets", namespace: "istio-system", @@ -377,6 +399,7 @@ func testEndpointsFromVirtualServiceConfig(t *testing.T) { for _, ti := range []struct { title string lbServices []fakeIngressGatewayService + ingresses []fakeIngress gwconfig fakeGatewayConfig vsconfig fakeVirtualServiceConfig expected []*endpoint.Endpoint @@ -557,12 +580,76 @@ func testEndpointsFromVirtualServiceConfig(t *testing.T) { }, }, }, + { + title: "ingress gateway annotation same namespace", + ingresses: []fakeIngress{ + { + name: "ingress1", + hostnames: []string{"alb.com", "elb.com"}, + }, + { + name: "ingress2", + ips: []string{"127.0.0.1", "8.8.8.8"}, + }, + }, + gwconfig: fakeGatewayConfig{ + name: "mygw", + dnsnames: [][]string{{"*"}}, + annotations: map[string]string{ + IstioGatewayIngressSource: "ingress1", + }, + }, + vsconfig: fakeVirtualServiceConfig{ + gateways: []string{"mygw"}, + dnsnames: []string{"foo.bar"}, + }, + expected: []*endpoint.Endpoint{ + { + DNSName: "foo.bar", + RecordType: endpoint.RecordTypeCNAME, + Targets: endpoint.Targets{"alb.com", "elb.com"}, + }, + }, + }, + { + title: "ingress gateway annotation separate namespace", + ingresses: []fakeIngress{ + { + name: "ingress1", + namespace: "ingress", + hostnames: []string{"alb.com", "elb.com"}, + }, + { + name: "ingress2", + namespace: "ingress", + ips: []string{"127.0.0.1", "8.8.8.8"}, + }, + }, + gwconfig: fakeGatewayConfig{ + name: "mygw", + dnsnames: [][]string{{"*"}}, + annotations: map[string]string{ + IstioGatewayIngressSource: "ingress/ingress2", + }, + }, + vsconfig: fakeVirtualServiceConfig{ + gateways: []string{"mygw"}, + dnsnames: []string{"foo.bar"}, + }, + expected: []*endpoint.Endpoint{ + { + DNSName: "foo.bar", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"127.0.0.1", "8.8.8.8"}, + }, + }, + }, } { ti := ti t.Run(ti.title, func(t *testing.T) { t.Parallel() - if source, err := newTestVirtualServiceSource(ti.lbServices, []fakeGatewayConfig{ti.gwconfig}); err != nil { + if source, err := newTestVirtualServiceSource(ti.lbServices, ti.ingresses, []fakeGatewayConfig{ti.gwconfig}); err != nil { require.NoError(t, err) } else if endpoints, err := source.endpointsFromVirtualService(context.Background(), ti.vsconfig.Config()); err != nil { require.NoError(t, err) @@ -582,6 +669,7 @@ func testVirtualServiceEndpoints(t *testing.T) { targetNamespace string annotationFilter string lbServices []fakeIngressGatewayService + ingresses []fakeIngress gwConfigs []fakeGatewayConfig vsConfigs []fakeVirtualServiceConfig expected []*endpoint.Endpoint @@ -648,6 +736,78 @@ func testVirtualServiceEndpoints(t *testing.T) { }, }, }, + { + title: "two simple virtualservices with one gateway each, one ingress", + lbServices: []fakeIngressGatewayService{ + { + namespace: namespace, + ips: []string{"8.8.8.8"}, + hostnames: []string{"lb.com"}, + }, + }, + ingresses: []fakeIngress{ + { + name: "ingress1", + namespace: namespace, + ips: []string{"8.8.8.8"}, + hostnames: []string{"lb.com"}, + }, + }, + gwConfigs: []fakeGatewayConfig{ + { + name: "fake1", + namespace: namespace, + dnsnames: [][]string{{"example.org"}}, + annotations: map[string]string{ + IstioGatewayIngressSource: "ingress1", + }, + }, + { + name: "fake2", + namespace: namespace, + dnsnames: [][]string{{"new.org"}}, + annotations: map[string]string{ + IstioGatewayIngressSource: "ingress1", + }, + }, + }, + vsConfigs: []fakeVirtualServiceConfig{ + { + name: "vs1", + namespace: namespace, + gateways: []string{"fake1"}, + dnsnames: []string{"example.org"}, + }, + { + name: "vs2", + namespace: namespace, + gateways: []string{"fake2"}, + dnsnames: []string{"new.org"}, + }, + }, + expected: []*endpoint.Endpoint{ + { + DNSName: "example.org", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"8.8.8.8"}, + }, + { + DNSName: "example.org", + RecordType: endpoint.RecordTypeCNAME, + Targets: endpoint.Targets{"lb.com"}, + }, + { + DNSName: "new.org", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"8.8.8.8"}, + }, + { + DNSName: "new.org", + RecordType: endpoint.RecordTypeCNAME, + Targets: endpoint.Targets{"lb.com"}, + }, + }, + }, { title: "one virtualservice with two gateways, one ingressgateway loadbalancer service", lbServices: []fakeIngressGatewayService{ @@ -781,6 +941,54 @@ func testVirtualServiceEndpoints(t *testing.T) { }, }, }, + { + title: "two simple virtualservices with one gateway on different namespaces and a target namespace, one ingress", + targetNamespace: "testing1", + ingresses: []fakeIngress{ + { + name: "ingress1", + ips: []string{"8.8.8.8"}, + hostnames: []string{"lb.com"}, + namespace: "testing1", + }, + }, + gwConfigs: []fakeGatewayConfig{ + { + name: "fake1", + namespace: "testing1", + dnsnames: [][]string{{"*"}}, + annotations: map[string]string{ + IstioGatewayIngressSource: "ingress1", + }, + }, + }, + vsConfigs: []fakeVirtualServiceConfig{ + { + name: "vs1", + namespace: "testing1", + gateways: []string{"testing1/fake1"}, + dnsnames: []string{"example.org"}, + }, + { + name: "vs2", + namespace: "testing2", + gateways: []string{"testing1/fake1"}, + dnsnames: []string{"new.org"}, + }, + }, + expected: []*endpoint.Endpoint{ + { + DNSName: "example.org", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"8.8.8.8"}, + }, + { + DNSName: "example.org", + RecordType: endpoint.RecordTypeCNAME, + Targets: endpoint.Targets{"lb.com"}, + }, + }, + }, { title: "valid matching annotation filter expression", annotationFilter: "kubernetes.io/virtualservice.class in (alb, nginx)", @@ -851,6 +1059,36 @@ func testVirtualServiceEndpoints(t *testing.T) { expected: []*endpoint.Endpoint{}, expectError: true, }, + { + title: "gateway ingress annotation; ingress not found", + ingresses: []fakeIngress{ + { + name: "ingress1", + namespace: namespace, + ips: []string{"8.8.8.8"}, + }, + }, + gwConfigs: []fakeGatewayConfig{ + { + name: "fake1", + namespace: namespace, + dnsnames: [][]string{{"*"}}, + annotations: map[string]string{ + IstioGatewayIngressSource: "ingress2", + }, + }, + }, + vsConfigs: []fakeVirtualServiceConfig{ + { + name: "vs1", + namespace: namespace, + gateways: []string{"fake1"}, + dnsnames: []string{"example.org"}, + }, + }, + expected: []*endpoint.Endpoint{}, + expectError: true, + }, { title: "our controller type is dns-controller", lbServices: []fakeIngressGatewayService{ @@ -1120,6 +1358,59 @@ func testVirtualServiceEndpoints(t *testing.T) { }, }, }, + { + title: "virtualservice; gateway with target and ingress annotation", + lbServices: []fakeIngressGatewayService{ + { + ips: []string{"8.8.8.8"}, + namespace: namespace, + }, + }, + ingresses: []fakeIngress{ + { + name: "ingress1", + ips: []string{"1.1.1.1"}, + namespace: namespace, + }, + }, + gwConfigs: []fakeGatewayConfig{ + { + name: "fake1", + namespace: namespace, + dnsnames: [][]string{{"*"}}, + annotations: map[string]string{ + targetAnnotationKey: "gateway-target.com", + IstioGatewayIngressSource: "ingress1", + }, + }, + }, + vsConfigs: []fakeVirtualServiceConfig{ + { + name: "vs1", + namespace: namespace, + gateways: []string{"fake1"}, + dnsnames: []string{"example.org"}, + }, + { + name: "vs2", + namespace: namespace, + gateways: []string{"fake1"}, + dnsnames: []string{"example2.org"}, + }, + }, + expected: []*endpoint.Endpoint{ + { + DNSName: "example.org", + Targets: endpoint.Targets{"gateway-target.com"}, + RecordType: endpoint.RecordTypeCNAME, + }, + { + DNSName: "example2.org", + Targets: endpoint.Targets{"gateway-target.com"}, + RecordType: endpoint.RecordTypeCNAME, + }, + }, + }, { title: "virtualservice with hostname annotation", lbServices: []fakeIngressGatewayService{ @@ -1398,6 +1689,18 @@ func testVirtualServiceEndpoints(t *testing.T) { namespace: "testing2", }, }, + ingresses: []fakeIngress{ + { + name: "ingress1", + namespace: "testing1", + hostnames: []string{"target4.com"}, + }, + { + name: "ingress3", + namespace: "testing3", + hostnames: []string{"target5.com"}, + }, + }, gwConfigs: []fakeGatewayConfig{ { name: "fake1", @@ -1423,6 +1726,17 @@ func testVirtualServiceEndpoints(t *testing.T) { "app": "igw3", }, }, + { + name: "fake4", + namespace: "testing3", + dnsnames: [][]string{{"*.bax.com", "*.bar.com"}}, + selector: map[string]string{ + "app": "igw4", + }, + annotations: map[string]string{ + IstioGatewayIngressSource: "testing1/ingress1", + }, + }, }, vsConfigs: []fakeVirtualServiceConfig{ { @@ -1443,6 +1757,12 @@ func testVirtualServiceEndpoints(t *testing.T) { gateways: []string{"istio-system/fake1", "testing2/fake3"}, dnsnames: []string{"world.bax.com", "world.bak.com"}, }, + { + name: "vs4", + namespace: "testing1", + gateways: []string{"istio-system/fake1", "testing3/fake4"}, + dnsnames: []string{"foo.bax.com", "foo.bak.com"}, + }, }, expected: []*endpoint.Endpoint{ { @@ -1475,6 +1795,16 @@ func testVirtualServiceEndpoints(t *testing.T) { Targets: endpoint.Targets{"target1.com", "target3.com"}, RecordType: endpoint.RecordTypeCNAME, }, + { + DNSName: "foo.bak.com", + Targets: endpoint.Targets{"target1.com"}, + RecordType: endpoint.RecordTypeCNAME, + }, + { + DNSName: "foo.bax.com", + Targets: endpoint.Targets{"target1.com", "target4.com"}, + RecordType: endpoint.RecordTypeCNAME, + }, }, fqdnTemplate: "{{.Name}}.ext-dns.test.com", }, @@ -1501,6 +1831,12 @@ func testVirtualServiceEndpoints(t *testing.T) { require.NoError(t, err) } + for _, ing := range ti.ingresses { + ingress := ing.Ingress() + _, err := fakeKubernetesClient.NetworkingV1().Ingresses(ingress.Namespace).Create(context.Background(), ingress, metav1.CreateOptions{}) + require.NoError(t, err) + } + fakeIstioClient := istiofake.NewSimpleClientset() for _, gateway := range gateways { @@ -1569,7 +1905,7 @@ func testGatewaySelectorMatchesService(t *testing.T) { } } -func newTestVirtualServiceSource(loadBalancerList []fakeIngressGatewayService, gwList []fakeGatewayConfig) (*virtualServiceSource, error) { +func newTestVirtualServiceSource(loadBalancerList []fakeIngressGatewayService, ingressList []fakeIngress, gwList []fakeGatewayConfig) (*virtualServiceSource, error) { fakeKubernetesClient := fake.NewSimpleClientset() fakeIstioClient := istiofake.NewSimpleClientset() @@ -1581,6 +1917,14 @@ func newTestVirtualServiceSource(loadBalancerList []fakeIngressGatewayService, g } } + for _, ing := range ingressList { + ingress := ing.Ingress() + _, err := fakeKubernetesClient.NetworkingV1().Ingresses(ingress.Namespace).Create(context.Background(), ingress, metav1.CreateOptions{}) + if err != nil { + return nil, err + } + } + for _, gw := range gwList { gwObj := gw.Config() // use create instead of add @@ -1658,21 +2002,21 @@ func TestVirtualServiceSourceGetGateway(t *testing.T) { expectedErrStr string }{ {name: "EmptyGateway", fields: fields{ - virtualServiceSource: func() *virtualServiceSource { vs, _ := newTestVirtualServiceSource(nil, nil); return vs }(), + virtualServiceSource: func() *virtualServiceSource { vs, _ := newTestVirtualServiceSource(nil, nil, nil); return vs }(), }, args: args{ ctx: context.TODO(), gatewayStr: "", virtualService: nil, }, want: nil, expectedErrStr: ""}, {name: "MeshGateway", fields: fields{ - virtualServiceSource: func() *virtualServiceSource { vs, _ := newTestVirtualServiceSource(nil, nil); return vs }(), + virtualServiceSource: func() *virtualServiceSource { vs, _ := newTestVirtualServiceSource(nil, nil, nil); return vs }(), }, args: args{ ctx: context.TODO(), gatewayStr: IstioMeshGateway, virtualService: nil, }, want: nil, expectedErrStr: ""}, {name: "MissingGateway", fields: fields{ - virtualServiceSource: func() *virtualServiceSource { vs, _ := newTestVirtualServiceSource(nil, nil); return vs }(), + virtualServiceSource: func() *virtualServiceSource { vs, _ := newTestVirtualServiceSource(nil, nil, nil); return vs }(), }, args: args{ ctx: context.TODO(), gatewayStr: "doesnt/exist", @@ -1684,7 +2028,7 @@ func TestVirtualServiceSourceGetGateway(t *testing.T) { }, }, want: nil, expectedErrStr: ""}, {name: "InvalidGatewayStr", fields: fields{ - virtualServiceSource: func() *virtualServiceSource { vs, _ := newTestVirtualServiceSource(nil, nil); return vs }(), + virtualServiceSource: func() *virtualServiceSource { vs, _ := newTestVirtualServiceSource(nil, nil, nil); return vs }(), }, args: args{ ctx: context.TODO(), gatewayStr: "1/2/3/", @@ -1692,7 +2036,7 @@ func TestVirtualServiceSourceGetGateway(t *testing.T) { }, want: nil, expectedErrStr: "invalid gateway name (name or namespace/name) found '1/2/3/'"}, {name: "ExistingGateway", fields: fields{ virtualServiceSource: func() *virtualServiceSource { - vs, _ := newTestVirtualServiceSource(nil, []fakeGatewayConfig{{ + vs, _ := newTestVirtualServiceSource(nil, nil, []fakeGatewayConfig{{ namespace: "bar", name: "foo", }}) From bd989eeac4ce3ed7d2448bdcffc91a4c413dab18 Mon Sep 17 00:00:00 2001 From: David Pait Date: Wed, 9 Aug 2023 08:35:00 -0400 Subject: [PATCH 7/8] fix imports --- source/istio_gateway_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/istio_gateway_test.go b/source/istio_gateway_test.go index 8fe065b974..f8450c5e51 100644 --- a/source/istio_gateway_test.go +++ b/source/istio_gateway_test.go @@ -20,8 +20,6 @@ import ( "context" "testing" - networkv1 "k8s.io/api/networking/v1" - "github.com/pkg/errors" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -30,6 +28,7 @@ import ( networkingv1alpha3 "istio.io/client-go/pkg/apis/networking/v1alpha3" istiofake "istio.io/client-go/pkg/clientset/versioned/fake" v1 "k8s.io/api/core/v1" + networkv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/fake" From 1a5249af5e084ad3b9daaae6af2da6fbdcb81438 Mon Sep 17 00:00:00 2001 From: David Pait Date: Wed, 9 Aug 2023 08:41:50 -0400 Subject: [PATCH 8/8] fix go conventions and ehance error messages --- source/istio_gateway.go | 9 ++++----- source/istio_virtualservice.go | 7 +++---- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/source/istio_gateway.go b/source/istio_gateway.go index 6ca6c9994e..e3dd6d8d47 100644 --- a/source/istio_gateway.go +++ b/source/istio_gateway.go @@ -243,7 +243,7 @@ func parseIngress(ingress string) (namespace, name string, err error) { } else if len(parts) == 1 { name = parts[0] } else { - err = fmt.Errorf("invalid ingress name (name or namespace/name) found '%v'", ingress) + err = fmt.Errorf("invalid ingress name (name or namespace/name) found %q", ingress) } return @@ -252,8 +252,7 @@ func parseIngress(ingress string) (namespace, name string, err error) { func (sc *gatewaySource) targetsFromIngress(ctx context.Context, ingressStr string, gateway *networkingv1alpha3.Gateway) (targets endpoint.Targets, err error) { namespace, name, err := parseIngress(ingressStr) if err != nil { - log.Debugf("Failed parsing ingressStr %s of Gateway %s/%s", ingressStr, gateway.Namespace, gateway.Name) - return nil, err + return nil, fmt.Errorf("failed to parse Ingress annotation on Gateway (%s/%s): %w", gateway.Namespace, gateway.Name, err) } if namespace == "" { namespace = gateway.Namespace @@ -280,8 +279,8 @@ func (sc *gatewaySource) targetsFromGateway(ctx context.Context, gateway *networ return } - ingressStr, found := gateway.Annotations[IstioGatewayIngressSource] - if found && ingressStr != "" { + ingressStr, ok := gateway.Annotations[IstioGatewayIngressSource] + if ok && ingressStr != "" { targets, err = sc.targetsFromIngress(ctx, ingressStr, gateway) return } diff --git a/source/istio_virtualservice.go b/source/istio_virtualservice.go index 3e7f79a749..64d07b412e 100644 --- a/source/istio_virtualservice.go +++ b/source/istio_virtualservice.go @@ -434,8 +434,7 @@ func parseGateway(gateway string) (namespace, name string, err error) { func (sc *virtualServiceSource) targetsFromIngress(ctx context.Context, ingressStr string, gateway *networkingv1alpha3.Gateway) (targets endpoint.Targets, err error) { namespace, name, err := parseIngress(ingressStr) if err != nil { - log.Debugf("Failed parsing ingressStr %s of Gateway %s/%s", ingressStr, gateway.Namespace, gateway.Name) - return nil, err + return nil, fmt.Errorf("failed to parse Ingress annotation on Gateway (%s/%s): %w", gateway.Namespace, gateway.Name, err) } if namespace == "" { namespace = gateway.Namespace @@ -462,8 +461,8 @@ func (sc *virtualServiceSource) targetsFromGateway(ctx context.Context, gateway return } - ingressStr, found := gateway.Annotations[IstioGatewayIngressSource] - if found && ingressStr != "" { + ingressStr, ok := gateway.Annotations[IstioGatewayIngressSource] + if ok && ingressStr != "" { targets, err = sc.targetsFromIngress(ctx, ingressStr, gateway) return }