Merge pull request #1575 from tariq1890/istio_dupl

remove some code duplication in gateway source

source/gateway.go

@@ -60,7 +60,7 @@ func NewIstioGatewaySource(
 	namespace string,
 	annotationFilter string,
 	fqdnTemplate string,
-	combineFqdnAnnotation bool,
+	combineFQDNAnnotation bool,
 	ignoreHostnameAnnotation bool,
 ) (Source, error) {
 	var (
@@ -108,7 +108,7 @@ func NewIstioGatewaySource(
 		namespace:                namespace,
 		annotationFilter:         annotationFilter,
 		fqdnTemplate:             tmpl,
-		combineFQDNAnnotation:    combineFqdnAnnotation,
+		combineFQDNAnnotation:    combineFQDNAnnotation,
 		ignoreHostnameAnnotation: ignoreHostnameAnnotation,
 		serviceInformer:          serviceInformer,
 	}, nil
@@ -128,7 +128,7 @@ func (sc *gatewaySource) Endpoints() ([]*endpoint.Endpoint, error) {
 		return nil, err
 	}
 
-	endpoints := []*endpoint.Endpoint{}
+	var endpoints []*endpoint.Endpoint
 
 	for _, gateway := range gateways {
 		// Check controller annotation to see if we are responsible.
@@ -139,30 +139,35 @@ func (sc *gatewaySource) Endpoints() ([]*endpoint.Endpoint, error) {
 			continue
 		}
 
-		gwEndpoints, err := sc.endpointsFromGatewayConfig(gateway)
+		gwHostnames, err := sc.hostNamesFromGateway(gateway)
 		if err != nil {
 			return nil, err
 		}
 
 		// apply template if host is missing on gateway
-		if (sc.combineFQDNAnnotation || len(gwEndpoints) == 0) && sc.fqdnTemplate != nil {
+		if (sc.combineFQDNAnnotation || len(gwHostnames) == 0) && sc.fqdnTemplate != nil {
-			iEndpoints, err := sc.endpointsFromTemplate2(gateway)
+			iHostnames, err := sc.hostNamesFromTemplate(gateway)
 			if err != nil {
 				return nil, err
 			}
 
 			if sc.combineFQDNAnnotation {
-				gwEndpoints = append(gwEndpoints, iEndpoints...)
+				gwHostnames = append(gwHostnames, iHostnames...)
 			} else {
-				gwEndpoints = iEndpoints
+				gwHostnames = iHostnames
 			}
 		}
 
-		if len(gwEndpoints) == 0 {
+		if len(gwHostnames) == 0 {
-			log.Debugf("No endpoints could be generated from gateway %s/%s", gateway.Namespace, gateway.Name)
+			log.Debugf("No hostnames could be generated from gateway %s/%s", gateway.Namespace, gateway.Name)
 			continue
 		}
 
+		gwEndpoints, err := sc.endpointsFromGateway(gwHostnames, gateway)
+		if err != nil {
+			return nil, err
+		}
+
 		log.Debugf("Endpoints generated from gateway: %s/%s: %v", gateway.Namespace, gateway.Name, gwEndpoints)
 		sc.setResourceLabel(gateway, gwEndpoints)
 		endpoints = append(endpoints, gwEndpoints...)
@@ -178,42 +183,6 @@ func (sc *gatewaySource) Endpoints() ([]*endpoint.Endpoint, error) {
 func (sc *gatewaySource) AddEventHandler(ctx context.Context, handler func()) {
 }
 
-func (sc *gatewaySource) endpointsFromTemplate2(gateway networkingv1alpha3.Gateway) ([]*endpoint.Endpoint, error) {
-	// Process the whole template string
-	var buf bytes.Buffer
-	err := sc.fqdnTemplate.Execute(&buf, gateway)
-	if err != nil {
-		return nil, fmt.Errorf("failed to apply template on istio config %v: %v", gateway, err)
-	}
-
-	hostnames := buf.String()
-
-	ttl, err := getTTLFromAnnotations(gateway.Annotations)
-	if err != nil {
-		log.Warn(err)
-	}
-
-	targets := getTargetsFromTargetAnnotation(gateway.Annotations)
-
-	if len(targets) == 0 {
-		targets, err = sc.targetsFromGatewayConfig(gateway)
-		if err != nil {
-			return nil, err
-		}
-	}
-
-	providerSpecific, setIdentifier := getProviderSpecificAnnotations(gateway.Annotations)
-
-	var endpoints []*endpoint.Endpoint
-	// splits the FQDN template and removes the trailing periods
-	hostnameList := strings.Split(strings.Replace(hostnames, " ", "", -1), ",")
-	for _, hostname := range hostnameList {
-		hostname = strings.TrimSuffix(hostname, ".")
-		endpoints = append(endpoints, endpointsForHostname(hostname, targets, ttl, providerSpecific, setIdentifier)...)
-	}
-	return endpoints, nil
-}
-
 // filterByAnnotations2 filters a list of configs by a given annotation selector.
 func (sc *gatewaySource) filterByAnnotations(gateways []networkingv1alpha3.Gateway) ([]networkingv1alpha3.Gateway, error) {
 	labelSelector, err := metav1.ParseToLabelSelector(sc.annotationFilter)
@@ -271,8 +240,7 @@ func (sc *gatewaySource) targetsFromGatewayConfig(gateway networkingv1alpha3.Gat
 		for _, lb := range service.Status.LoadBalancer.Ingress {
 			if lb.IP != "" {
 				targets = append(targets, lb.IP)
-			}
+			} else if lb.Hostname != "" {
-			if lb.Hostname != "" {
 				targets = append(targets, lb.Hostname)
 			}
 		}
@@ -282,15 +250,16 @@ func (sc *gatewaySource) targetsFromGatewayConfig(gateway networkingv1alpha3.Gat
 }
 
 // endpointsFromGatewayConfig extracts the endpoints from an Istio Gateway Config object
-func (sc *gatewaySource) endpointsFromGatewayConfig(gateway networkingv1alpha3.Gateway) ([]*endpoint.Endpoint, error) {
+func (sc *gatewaySource) endpointsFromGateway(hostnames []string, gateway networkingv1alpha3.Gateway) ([]*endpoint.Endpoint, error) {
 	var endpoints []*endpoint.Endpoint
 
-	ttl, err := getTTLFromAnnotations(gateway.Annotations)
+	annotations := gateway.Annotations
+	ttl, err := getTTLFromAnnotations(annotations)
 	if err != nil {
 		log.Warn(err)
 	}
 
-	targets := getTargetsFromTargetAnnotation(gateway.Annotations)
+	targets := getTargetsFromTargetAnnotation(annotations)
 
 	if len(targets) == 0 {
 		targets, err = sc.targetsFromGatewayConfig(gateway)
@@ -299,8 +268,23 @@ func (sc *gatewaySource) endpointsFromGatewayConfig(gateway networkingv1alpha3.G
 		}
 	}
 
-	providerSpecific, setIdentifier := getProviderSpecificAnnotations(gateway.Annotations)
+	providerSpecific, setIdentifier := getProviderSpecificAnnotations(annotations)
 
+	// Skip endpoints if we do not want entries from annotations
+	if !sc.ignoreHostnameAnnotation {
+		hostnames = append(hostnames, getHostnamesFromAnnotations(annotations)...)
+	}
+
+	for _, host := range hostnames {
+		endpoints = append(endpoints, endpointsForHostname(host, targets, ttl, providerSpecific, setIdentifier)...)
+	}
+
+	return endpoints, nil
+}
+
+func (sc *gatewaySource) hostNamesFromGateway(gateway networkingv1alpha3.Gateway) ([]string, error) {
+
+	var hostnames []string
 	for _, server := range gateway.Spec.Servers {
 		for _, host := range server.Hosts {
 			if host == "" {
@@ -315,17 +299,20 @@ func (sc *gatewaySource) endpointsFromGatewayConfig(gateway networkingv1alpha3.G
 				host = parts[1]
 			}
 
-			endpoints = append(endpoints, endpointsForHostname(host, targets, ttl, providerSpecific, setIdentifier)...)
+			hostnames = append(hostnames, host)
 		}
 	}
-
+	return hostnames, nil
-	// Skip endpoints if we do not want entries from annotations
+}
-	if !sc.ignoreHostnameAnnotation {
+
-		hostnameList := getHostnamesFromAnnotations(gateway.Annotations)
+func (sc *gatewaySource) hostNamesFromTemplate(gateway networkingv1alpha3.Gateway) ([]string, error) {
-		for _, hostname := range hostnameList {
+	// Process the whole template string
-			endpoints = append(endpoints, endpointsForHostname(hostname, targets, ttl, providerSpecific, setIdentifier)...)
+	var buf bytes.Buffer
-		}
+	err := sc.fqdnTemplate.Execute(&buf, gateway)
-	}
+	if err != nil {
-
+		return nil, fmt.Errorf("failed to apply template on istio gateway %v: %v", gateway, err)
-	return endpoints, nil
+	}
+
+	hostnames := strings.Split(strings.Replace(buf.String(), " ", "", -1), ",")
+	return hostnames, nil
 }

source/gateway_test.go

@@ -309,7 +309,9 @@ func testEndpointsFromGatewayConfig(t *testing.T) {
 			gatewayCfg := ti.config.Config()
 			if source, err := newTestGatewaySource(ti.lbServices); err != nil {
 				require.NoError(t, err)
-			} else if endpoints, err := source.endpointsFromGatewayConfig(*gatewayCfg); err != nil {
+			} else if hostnames, err := source.hostNamesFromGateway(gatewayCfg); err != nil {
+				require.NoError(t, err)
+			} else if endpoints, err := source.endpointsFromGateway(hostnames, gatewayCfg); err != nil {
 				require.NoError(t, err)
 			} else {
 				validateEndpoints(t, endpoints, ti.expected)
@@ -1083,7 +1085,7 @@ func testGatewayEndpoints(t *testing.T) {
 			fakeIstioClient := NewFakeConfigStore()
 			for _, config := range ti.configItems {
 				gatewayCfg := config.Config()
-				_, err := fakeIstioClient.NetworkingV1alpha3().Gateways(ti.targetNamespace).Create(gatewayCfg)
+				_, err := fakeIstioClient.NetworkingV1alpha3().Gateways(ti.targetNamespace).Create(&gatewayCfg)
 				require.NoError(t, err)
 			}
 
@@ -1186,7 +1188,7 @@ type fakeGatewayConfig struct {
 	selector    map[string]string
 }
 
-func (c fakeGatewayConfig) Config() *networkingv1alpha3.Gateway {
+func (c fakeGatewayConfig) Config() networkingv1alpha3.Gateway {
 	gw := networkingv1alpha3.Gateway{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:        c.name,
@@ -1208,7 +1210,7 @@ func (c fakeGatewayConfig) Config() *networkingv1alpha3.Gateway {
 
 	gw.Spec.Servers = servers
 
-	return &gw
+	return gw
 }
 
 func NewFakeConfigStore() istioclient.Interface {