openshift route source: better use of route status

docs/tutorials/openshift.md

@@ -4,46 +4,34 @@ It is meant to supplement the other provider-specific setup tutorials.
 
 ### For OCP 4.x
 
-In OCP 4.x, if you have multiple ingress controllers then you must specify an ingress controller name or a router name(you can get it from the route's Status.Ingress.RouterName field).
+In OCP 4.x, if you have multiple [OpenShift ingress controllers](https://docs.openshift.com/container-platform/4.9/networking/ingress-operator.html) then you must specify an ingress controller name (also called router name), you can get it from the route's `status.ingress[*].routerName` field.
-If you don't specify an ingress controller's or router name when you have multiple ingresscontrollers in your environment then the route gets populated with multiple entries of router canonical hostnames which causes external dns to create a CNAME record with multiple router canonical hostnames pointing to the route host which is a violation of RFC 1912 and is not allowed by Cloud Providers which leads to failure of record creation.
+If you don't specify a router name when you have multiple ingress controllers in your cluster then the first router from the route's `status.ingress` will be used. Note that the router must have admitted the route in order to be selected.
-Once you specify the ingresscontroller or router name then that will be matched by the external-dns and the router canonical hostname corresponding to this routerName(which is present in route's Status.Ingress.RouterName field) is selected and a CNAME record of this route host pointing to this router canonical hostname is created.
+Once the router is known, ExternalDNS will use this router's canonical hostname as the target for the CNAME record.
-
-Your externaldns CR shall be created as per the following example.
-Replace names in the domain section and zone ID as per your environment.
-This is example is for AWS environment.
 
+Starting from OCP 4.10 you can use [ExternalDNS Operator](https://github.com/openshift/external-dns-operator) to manage ExternalDNS instances. Example of its custom resource for AWS provider:
 ```yaml
-
   apiVersion: externaldns.olm.openshift.io/v1alpha1
   kind: ExternalDNS
   metadata:
-    name: sample1
+    name: sample
   spec:
-    domains:
-      - filterType: Include
-        matchType: Exact
-        names: apps.miheer.externaldns
     provider:
       type: AWS
     source:
-      hostnameAnnotation: Allow
       openshiftRouteOptions:
         routerName: default
       type: OpenShiftRoute
     zones:
       - Z05387772BD5723IZFRX3
-
 ```
 
-This will create an externaldns pod with the following container args under spec in the external-dns namespace where `- --source=openshift-route` and `- --openshift-router-name=default` is added by the external-dns-operator.
+This will create an ExternalDNS POD with the following container args in `external-dns` namespace:
-
 ```
 spec:
   containers:
   - args:
-    - --domain-filter=apps.misalunk.externaldns
     - --metrics-address=127.0.0.1:7979
-    - --txt-owner-id=external-dns-sample1
+    - --txt-owner-id=external-dns-sample
     - --provider=aws
     - --source=openshift-route
     - --policy=sync
@@ -52,7 +40,6 @@ spec:
     - --zone-id-filter=Z05387772BD5723IZFRX3
     - --openshift-router-name=default
     - --txt-prefix=external-dns-
-
 ```
 
 ### For OCP 3.11 environment

source/openshift_route.go

@@ -27,6 +27,7 @@ import (
 	extInformers "github.com/openshift/client-go/route/informers/externalversions"
 	routeInformer "github.com/openshift/client-go/route/informers/externalversions/route/v1"
 	log "github.com/sirupsen/logrus"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/client-go/tools/cache"
@@ -180,7 +181,8 @@ func (ors *ocpRouteSource) endpointsFromTemplate(ocpRoute *routev1.Route) ([]*en
 
 	targets := getTargetsFromTargetAnnotation(ocpRoute.Annotations)
 	if len(targets) == 0 {
-		targets = ors.targetsFromOcpRouteStatus(ocpRoute.Status)
+		targetsFromRoute, _ := ors.getTargetsFromRouteStatus(ocpRoute.Status)
+		targets = targetsFromRoute
 	}
 
 	providerSpecific, setIdentifier := getProviderSpecificAnnotations(ocpRoute.Annotations)
@@ -238,14 +240,15 @@ func (ors *ocpRouteSource) endpointsFromOcpRoute(ocpRoute *routev1.Route, ignore
 	}
 
 	targets := getTargetsFromTargetAnnotation(ocpRoute.Annotations)
+	targetsFromRoute, host := ors.getTargetsFromRouteStatus(ocpRoute.Status)
 
 	if len(targets) == 0 {
-		targets = ors.targetsFromOcpRouteStatus(ocpRoute.Status)
+		targets = targetsFromRoute
 	}
 
 	providerSpecific, setIdentifier := getProviderSpecificAnnotations(ocpRoute.Annotations)
 
-	if host := ocpRoute.Spec.Host; host != "" {
+	if host != "" {
 		endpoints = append(endpoints, endpointsForHostname(host, targets, ttl, providerSpecific, setIdentifier)...)
 	}
 
@@ -259,18 +262,35 @@ func (ors *ocpRouteSource) endpointsFromOcpRoute(ocpRoute *routev1.Route, ignore
 	return endpoints
 }
 
-func (ors *ocpRouteSource) targetsFromOcpRouteStatus(status routev1.RouteStatus) endpoint.Targets {
+// getTargetsFromRouteStatus returns the router's canonical hostname and host
-	var targets endpoint.Targets
+// either for the given router if it admitted the route
+// or for the first (in the status list) router that admitted the route.
+func (ors *ocpRouteSource) getTargetsFromRouteStatus(status routev1.RouteStatus) (endpoint.Targets, string) {
 	for _, ing := range status.Ingress {
-		if len(ors.ocpRouterName) != 0 {
+		// if this Ingress didn't admit the route or it doesn't have the canonical hostname, then ignore it
-			if ing.RouterName == ors.ocpRouterName {
+		if ingressConditionStatus(&ing, routev1.RouteAdmitted) != corev1.ConditionTrue || ing.RouterCanonicalHostname == "" {
-				targets = append(targets, ing.RouterCanonicalHostname)
+			continue
-				return targets
+		}
-			}
+
-		} else if ing.RouterCanonicalHostname != "" {
+		// if the router name is specified for the Route source and it matches the route's ingress name, then return it
-			targets = append(targets, ing.RouterCanonicalHostname)
+		if ors.ocpRouterName != "" && ors.ocpRouterName == ing.RouterName {
-			return targets
+			return endpoint.Targets{ing.RouterCanonicalHostname}, ing.Host
+		}
+
+		// if the router name is not specified in the Route source then return the first ingress
+		if ors.ocpRouterName == "" {
+			return endpoint.Targets{ing.RouterCanonicalHostname}, ing.Host
 		}
 	}
-	return targets
+	return endpoint.Targets{}, ""
+}
+
+func ingressConditionStatus(ingress *routev1.RouteIngress, t routev1.RouteIngressConditionType) corev1.ConditionStatus {
+	for _, condition := range ingress.Conditions {
+		if t != condition.Type {
+			continue
+		}
+		return condition.Status
+	}
+	return corev1.ConditionUnknown
 }

source/openshift_route_test.go

@@ -27,6 +27,7 @@ import (
 
 	routev1 "github.com/openshift/api/route/v1"
 	fake "github.com/openshift/client-go/route/clientset/versioned/fake"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"sigs.k8s.io/external-dns/endpoint"
@@ -165,41 +166,35 @@ func testOcpRouteSourceNewOcpRouteSource(t *testing.T) {
 // testOcpRouteSourceEndpoints tests that various OCP routes generate the correct endpoints.
 func testOcpRouteSourceEndpoints(t *testing.T) {
 	for _, tc := range []struct {
-		title                    string
+		title         string
-		targetNamespace          string
+		ocpRoute      *routev1.Route
-		annotationFilter         string
+		expected      []*endpoint.Endpoint
-		fqdnTemplate             string
+		expectError   bool
-		ignoreHostnameAnnotation bool
+		labelFilter   string
-		ocpRoute                 *routev1.Route
+		ocpRouterName string
-		expected                 []*endpoint.Endpoint
-		expectError              bool
-		labelFilter              string
-		ocpRouterName            string
 	}{
 		{
-			title:                    "route with basic hostname and route status target",
+			title: "route with basic hostname and route status target",
-			targetNamespace:          "",
-			annotationFilter:         "",
-			fqdnTemplate:             "",
-			ignoreHostnameAnnotation: false,
 			ocpRoute: &routev1.Route{
-				Spec: routev1.RouteSpec{
-					Host: "my-domain.com",
-				},
 				ObjectMeta: metav1.ObjectMeta{
-					Namespace:   "default",
+					Namespace: "default",
-					Name:        "route-with-target",
+					Name:      "route-with-target",
-					Annotations: map[string]string{},
 				},
 				Status: routev1.RouteStatus{
 					Ingress: []routev1.RouteIngress{
 						{
+							Host:                    "my-domain.com",
 							RouterCanonicalHostname: "apps.my-domain.com",
+							Conditions: []routev1.RouteIngressCondition{
+								{
+									Type:   routev1.RouteAdmitted,
+									Status: corev1.ConditionTrue,
+								},
+							},
 						},
 					},
 				},
 			},
-			ocpRouterName: "",
 			expected: []*endpoint.Endpoint{
 				{
 					DNSName: "my-domain.com",
@@ -208,28 +203,26 @@ func testOcpRouteSourceEndpoints(t *testing.T) {
 					},
 				},
 			},
-			expectError: false,
 		},
 		{
-			title:                    "route with basic hostname and route status target with one RouterCanonicalHostname and one ocpRouterNames defined",
+			title: "route with basic hostname, route status target and ocpRouterName defined",
-			targetNamespace:          "",
-			annotationFilter:         "",
-			fqdnTemplate:             "",
-			ignoreHostnameAnnotation: false,
 			ocpRoute: &routev1.Route{
-				Spec: routev1.RouteSpec{
-					Host: "my-domain.com",
-				},
 				ObjectMeta: metav1.ObjectMeta{
-					Namespace:   "default",
+					Namespace: "default",
-					Name:        "route-with-target",
+					Name:      "route-with-target",
-					Annotations: map[string]string{},
 				},
 				Status: routev1.RouteStatus{
 					Ingress: []routev1.RouteIngress{
 						{
+							Host:                    "my-domain.com",
 							RouterName:              "default",
 							RouterCanonicalHostname: "router-default.my-domain.com",
+							Conditions: []routev1.RouteIngressCondition{
+								{
+									Type:   routev1.RouteAdmitted,
+									Status: corev1.ConditionTrue,
+								},
+							},
 						},
 					},
 				},
@@ -243,32 +236,37 @@ func testOcpRouteSourceEndpoints(t *testing.T) {
 					},
 				},
 			},
-			expectError: false,
 		},
 		{
-			title:                    "route with basic hostname and route status target with one RouterCanonicalHostname and one ocpRouterNames defined and two router canonical names",
+			title: "route with basic hostname, route status target, one ocpRouterName and two router canonical names",
-			targetNamespace:          "",
-			annotationFilter:         "",
-			fqdnTemplate:             "",
-			ignoreHostnameAnnotation: false,
 			ocpRoute: &routev1.Route{
-				Spec: routev1.RouteSpec{
-					Host: "my-domain.com",
-				},
 				ObjectMeta: metav1.ObjectMeta{
-					Namespace:   "default",
+					Namespace: "default",
-					Name:        "route-with-target",
+					Name:      "route-with-target",
-					Annotations: map[string]string{},
 				},
 				Status: routev1.RouteStatus{
 					Ingress: []routev1.RouteIngress{
 						{
+							Host:                    "my-domain.com",
 							RouterName:              "default",
 							RouterCanonicalHostname: "router-default.my-domain.com",
+							Conditions: []routev1.RouteIngressCondition{
+								{
+									Type:   routev1.RouteAdmitted,
+									Status: corev1.ConditionTrue,
+								},
+							},
 						},
 						{
+							Host:                    "my-domain.com",
 							RouterName:              "test",
 							RouterCanonicalHostname: "router-test.my-domain.com",
+							Conditions: []routev1.RouteIngressCondition{
+								{
+									Type:   routev1.RouteAdmitted,
+									Status: corev1.ConditionTrue,
+								},
+							},
 						},
 					},
 				},
@@ -282,53 +280,126 @@ func testOcpRouteSourceEndpoints(t *testing.T) {
 					},
 				},
 			},
-			expectError: false,
 		},
 		{
-			title:                    "route with basic hostname and route status target with one RouterCanonicalHostname and one ocpRouterName defined and two router canonical names",
+			title: "route not admitted by the given router",
-			targetNamespace:          "",
+			ocpRoute: &routev1.Route{
-			annotationFilter:         "",
+				ObjectMeta: metav1.ObjectMeta{
-			fqdnTemplate:             "",
+					Namespace: "default",
-			ignoreHostnameAnnotation: false,
+					Name:      "route-with-target",
+				},
+				Status: routev1.RouteStatus{
+					Ingress: []routev1.RouteIngress{
+						{
+							Host:                    "my-domain.com",
+							RouterName:              "default",
+							RouterCanonicalHostname: "router-default.my-domain.com",
+							Conditions: []routev1.RouteIngressCondition{
+								{
+									Type:   routev1.RouteAdmitted,
+									Status: corev1.ConditionTrue,
+								},
+							},
+						},
+						{
+							Host:                    "my-domain.com",
+							RouterName:              "test",
+							RouterCanonicalHostname: "router-test.my-domain.com",
+							Conditions: []routev1.RouteIngressCondition{
+								{
+									Type:   routev1.RouteAdmitted,
+									Status: corev1.ConditionFalse,
+								},
+							},
+						},
+					},
+				},
+			},
+			ocpRouterName: "test",
+			expected:      []*endpoint.Endpoint{},
+		},
+		{
+			title: "route not admitted by any router",
 			ocpRoute: &routev1.Route{
 				Spec: routev1.RouteSpec{
 					Host: "my-domain.com",
 				},
 				ObjectMeta: metav1.ObjectMeta{
-					Namespace:   "default",
+					Namespace: "default",
-					Name:        "route-with-target",
+					Name:      "route-with-target",
-					Annotations: map[string]string{},
 				},
 				Status: routev1.RouteStatus{
 					Ingress: []routev1.RouteIngress{
 						{
+							Host:                    "my-domain.com",
 							RouterName:              "default",
 							RouterCanonicalHostname: "router-default.my-domain.com",
+							Conditions: []routev1.RouteIngressCondition{
+								{
+									Type:   routev1.RouteAdmitted,
+									Status: corev1.ConditionFalse,
+								},
+							},
 						},
 						{
+							Host:                    "my-domain.com",
 							RouterName:              "test",
 							RouterCanonicalHostname: "router-test.my-domain.com",
+							Conditions: []routev1.RouteIngressCondition{
+								{
+									Type:   routev1.RouteAdmitted,
+									Status: corev1.ConditionFalse,
+								},
+							},
+						},
+					},
+				},
+			},
+			expected: []*endpoint.Endpoint{},
+		},
+		{
+			title: "route admitted by first appropriate router",
+			ocpRoute: &routev1.Route{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "default",
+					Name:      "route-with-target",
+				},
+				Status: routev1.RouteStatus{
+					Ingress: []routev1.RouteIngress{
+						{
+							Host:                    "my-domain.com",
+							RouterName:              "default",
+							RouterCanonicalHostname: "router-default.my-domain.com",
+							Conditions: []routev1.RouteIngressCondition{
+								{
+									Type:   routev1.RouteAdmitted,
+									Status: corev1.ConditionFalse,
+								},
+							},
+						},
+						{
+							Host:                    "my-domain.com",
+							RouterName:              "test",
+							RouterCanonicalHostname: "router-test.my-domain.com",
+							Conditions: []routev1.RouteIngressCondition{
+								{
+									Type:   routev1.RouteAdmitted,
+									Status: corev1.ConditionTrue,
+								},
+							},
 						},
 					},
 				},
 			},
-			ocpRouterName: "default",
 			expected: []*endpoint.Endpoint{
 				{
 					DNSName: "my-domain.com",
-					Targets: []string{
+					Targets: []string{"router-test.my-domain.com"},
-						"router-default.my-domain.com",
-					},
 				},
 			},
-			expectError: false,
 		},
 		{
-			title:                    "route with incorrect externalDNS controller annotation",
+			title: "route with incorrect externalDNS controller annotation",
-			targetNamespace:          "",
-			annotationFilter:         "",
-			fqdnTemplate:             "",
-			ignoreHostnameAnnotation: false,
 			ocpRoute: &routev1.Route{
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "default",
@@ -338,20 +409,11 @@ func testOcpRouteSourceEndpoints(t *testing.T) {
 					},
 				},
 			},
-			ocpRouterName: "",
+			expected: []*endpoint.Endpoint{},
-			expected:      []*endpoint.Endpoint{},
-			expectError:   false,
 		},
 		{
-			title:                    "route with basic hostname and annotation target",
+			title: "route with basic hostname and annotation target",
-			targetNamespace:          "",
-			annotationFilter:         "",
-			fqdnTemplate:             "",
-			ignoreHostnameAnnotation: false,
 			ocpRoute: &routev1.Route{
-				Spec: routev1.RouteSpec{
-					Host: "my-annotation-domain.com",
-				},
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "default",
 					Name:      "route-with-annotation-target",
@@ -359,8 +421,22 @@ func testOcpRouteSourceEndpoints(t *testing.T) {
 						"external-dns.alpha.kubernetes.io/target": "my.site.foo.com",
 					},
 				},
+				Status: routev1.RouteStatus{
+					Ingress: []routev1.RouteIngress{
+						{
+							Host:                    "my-annotation-domain.com",
+							RouterName:              "default",
+							RouterCanonicalHostname: "router-default.my-domain.com",
+							Conditions: []routev1.RouteIngressCondition{
+								{
+									Type:   routev1.RouteAdmitted,
+									Status: corev1.ConditionTrue,
+								},
+							},
+						},
+					},
+				},
 			},
-			ocpRouterName: "",
 			expected: []*endpoint.Endpoint{
 				{
 					DNSName: "my-annotation-domain.com",
@@ -369,17 +445,11 @@ func testOcpRouteSourceEndpoints(t *testing.T) {
 					},
 				},
 			},
-			expectError: false,
 		},
 		{
-			title:                    "route with matching labels",
+			title:       "route with matching labels",
-			labelFilter:              "app=web-external",
+			labelFilter: "app=web-external",
-			ignoreHostnameAnnotation: false,
 			ocpRoute: &routev1.Route{
-
-				Spec: routev1.RouteSpec{
-					Host: "my-annotation-domain.com",
-				},
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "default",
 					Name:      "route-with-matching-labels",
@@ -391,8 +461,22 @@ func testOcpRouteSourceEndpoints(t *testing.T) {
 						"name": "service-frontend",
 					},
 				},
+				Status: routev1.RouteStatus{
+					Ingress: []routev1.RouteIngress{
+						{
+							Host:                    "my-annotation-domain.com",
+							RouterName:              "default",
+							RouterCanonicalHostname: "router-default.my-domain.com",
+							Conditions: []routev1.RouteIngressCondition{
+								{
+									Type:   routev1.RouteAdmitted,
+									Status: corev1.ConditionTrue,
+								},
+							},
+						},
+					},
+				},
 			},
-			ocpRouterName: "",
 			expected: []*endpoint.Endpoint{
 				{
 					DNSName: "my-annotation-domain.com",
@@ -401,12 +485,10 @@ func testOcpRouteSourceEndpoints(t *testing.T) {
 					},
 				},
 			},
-			expectError: false,
 		},
 		{
-			title:                    "route without matching labels",
+			title:       "route without matching labels",
-			labelFilter:              "app=web-external",
+			labelFilter: "app=web-external",
-			ignoreHostnameAnnotation: false,
 			ocpRoute: &routev1.Route{
 
 				Spec: routev1.RouteSpec{
@@ -424,9 +506,7 @@ func testOcpRouteSourceEndpoints(t *testing.T) {
 					},
 				},
 			},
-			ocpRouterName: "",
+			expected: []*endpoint.Endpoint{},
-			expected:      []*endpoint.Endpoint{},
-			expectError:   false,
 		},
 	} {
 		tc := tc
@@ -451,7 +531,6 @@ func testOcpRouteSourceEndpoints(t *testing.T) {
 				labelSelector,
 				tc.ocpRouterName,
 			)
-
 			require.NoError(t, err)
 
 			res, err := source.Endpoints(context.Background())