address review feedback

This commit addresses the review feedback by making the following changes:

- use a more object-oriented approach for geoProximity handling
- change log levels to warnings instead of errors
- add more test cases for geoProximity

Author: prasadkatti

provider/aws/aws.go

@@ -69,7 +69,11 @@ const (
 	sameZoneAlias                                      = "same-zone"
 	// Currently supported up to 10 health checks or hosted zones.
 	// https://docs.aws.amazon.com/Route53/latest/APIReference/API_ListTagsForResources.html#API_ListTagsForResources_RequestSyntax
-	batchSize = 10
+	batchSize    = 10
+	minLatitude  = -90.0
+	maxLatitude  = 90.0
+	minLongitude = -180.0
+	maxLongitude = 180.0
 )
 
 // see elb: https://docs.aws.amazon.com/general/latest/gr/elb.html
@@ -235,6 +239,12 @@ type profiledZone struct {
 	zone    *route53types.HostedZone
 }
 
+type geoProximity struct {
+	location *route53types.GeoProximityLocation
+	endpoint *endpoint.Endpoint
+	isSet    bool
+}
+
 func (cs Route53Changes) Route53Changes() []route53types.Change {
 	var ret []route53types.Change
 	for _, c := range cs {
@@ -868,16 +878,17 @@ func (p *AWSProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) ([]*endpoi
 // if the endpoint is using geoproximity, set the bias to 0 if not set
 // this is needed to avoid unnecessary Upserts if the desired endpoint doesn't specify a bias
 func adjustGeoProximityLocationEndpoint(ep *endpoint.Endpoint) {
-	if ep.SetIdentifier != "" {
-		_, ok1 := ep.GetProviderSpecificProperty(providerSpecificGeoProximityLocationAWSRegion)
-		_, ok2 := ep.GetProviderSpecificProperty(providerSpecificGeoProximityLocationLocalZoneGroup)
-		_, ok3 := ep.GetProviderSpecificProperty(providerSpecificGeoProximityLocationCoordinates)
-
-		if ok1 || ok2 || ok3 {
-			// check if ep has bias property and if not, set it to 0
-			if _, ok := ep.GetProviderSpecificProperty(providerSpecificGeoProximityLocationBias); !ok {
-				ep.SetProviderSpecificProperty(providerSpecificGeoProximityLocationBias, "0")
-			}
+	if ep.SetIdentifier == "" {
+		return
+	}
+	_, ok1 := ep.GetProviderSpecificProperty(providerSpecificGeoProximityLocationAWSRegion)
+	_, ok2 := ep.GetProviderSpecificProperty(providerSpecificGeoProximityLocationLocalZoneGroup)
+	_, ok3 := ep.GetProviderSpecificProperty(providerSpecificGeoProximityLocationCoordinates)
+
+	if ok1 || ok2 || ok3 {
+		// check if ep has bias property and if not, set it to 0
+		if _, ok := ep.GetProviderSpecificProperty(providerSpecificGeoProximityLocationBias); !ok {
+			ep.SetProviderSpecificProperty(providerSpecificGeoProximityLocationBias, "0")
 		}
 	}
 }
@@ -985,78 +996,99 @@ func (p *AWSProvider) newChange(action route53types.ChangeAction, ep *endpoint.E
 	return change
 }
 
-func withChangeForGeoProximityEndpoint(change *Route53Change, ep *endpoint.Endpoint) {
-	geoproximity := &route53types.GeoProximityLocation{}
-	isGeoproximity := false
+func newGeoProximity(ep *endpoint.Endpoint) *geoProximity {
+	return &geoProximity{
+		location: &route53types.GeoProximityLocation{},
+		endpoint: ep,
+		isSet:    false,
+	}
+}
 
-	isGeoproximity = withGeoProximityAWSRegion(ep, geoproximity)
-	isGeoproximity = isGeoproximity || withGeoProximityCoordinates(ep, geoproximity)
-	isGeoproximity = isGeoproximity || withGeoProximityLocalZoneGroup(ep, geoproximity)
-	withGeoProximityBias(ep, geoproximity)
+func (gp *geoProximity) withAWSRegion() *geoProximity {
+	if prop, ok := gp.endpoint.GetProviderSpecificProperty(providerSpecificGeoProximityLocationAWSRegion); ok {
+		gp.location.AWSRegion = aws.String(prop)
+		gp.isSet = true
+	}
+	return gp
+}
 
-	if isGeoproximity {
-		change.ResourceRecordSet.GeoProximityLocation = geoproximity
+// add a method to set the local zone group for the geoproximity location
+func (gp *geoProximity) withLocalZoneGroup() *geoProximity {
+	if prop, ok := gp.endpoint.GetProviderSpecificProperty(providerSpecificGeoProximityLocationLocalZoneGroup); ok {
+		gp.location.LocalZoneGroup = aws.String(prop)
+		gp.isSet = true
 	}
+	return gp
 }
 
-func withGeoProximityAWSRegion(ep *endpoint.Endpoint, geoproximity *route53types.GeoProximityLocation) bool {
-	if prop, ok := ep.GetProviderSpecificProperty(providerSpecificGeoProximityLocationAWSRegion); ok {
-		geoproximity.AWSRegion = aws.String(prop)
-		return true
+// add a method to set the bias for the geoproximity location
+func (gp *geoProximity) withBias() *geoProximity {
+	if prop, ok := gp.endpoint.GetProviderSpecificProperty(providerSpecificGeoProximityLocationBias); ok {
+		bias, err := strconv.ParseInt(prop, 10, 32)
+		if err != nil {
+			log.Warnf("Failed parsing value of %s: %s: %v; using bias of 0", providerSpecificGeoProximityLocationBias, prop, err)
+			bias = 0
+		}
+		gp.location.Bias = aws.Int32(int32(bias))
+		gp.isSet = true
 	}
-	return false
+	return gp
 }
 
 // validateCoordinates checks if the given latitude and longitude are valid.
 func validateCoordinates(lat, long string) error {
 	latitude, err := strconv.ParseFloat(lat, 64)
-	if err != nil || latitude < -90 || latitude > 90 {
-		return errors.New("invalid latitude: must be a number between -90 and 90")
+	if err != nil || latitude < minLatitude || latitude > maxLatitude {
+		return fmt.Errorf("invalid latitude: must be a number between %f and %f", minLatitude, maxLatitude)
 	}
 
 	longitude, err := strconv.ParseFloat(long, 64)
-	if err != nil || longitude < -180 || longitude > 180 {
-		return errors.New("invalid longitude: must be a number between -180 and 180")
+	if err != nil || longitude < minLongitude || longitude > maxLongitude {
+		return fmt.Errorf("invalid longitude: must be a number between %f and %f", minLongitude, maxLongitude)
 	}
 
 	return nil
 }
 
-func withGeoProximityCoordinates(ep *endpoint.Endpoint, geoproximity *route53types.GeoProximityLocation) bool {
-	if prop, ok := ep.GetProviderSpecificProperty(providerSpecificGeoProximityLocationCoordinates); ok {
+func (gp *geoProximity) withCoordinates() *geoProximity {
+	if prop, ok := gp.endpoint.GetProviderSpecificProperty(providerSpecificGeoProximityLocationCoordinates); ok {
 		coordinates := strings.Split(prop, ",")
 		if len(coordinates) == 2 {
 			latitude := coordinates[0]
 			longitude := coordinates[1]
 			if err := validateCoordinates(latitude, longitude); err != nil {
-				log.Errorf("Invalid coordinates %s for name=%s setIdentifier=%s; %v", prop, ep.DNSName, ep.SetIdentifier, err)
-				return false
-			}
-			geoproximity.Coordinates = &route53types.Coordinates{
-				Latitude:  aws.String(latitude),
-				Longitude: aws.String(longitude),
+				log.Warnf("Invalid coordinates %s for name=%s setIdentifier=%s; %v", prop, gp.endpoint.DNSName, gp.endpoint.SetIdentifier, err)
+			} else {
+				gp.location.Coordinates = &route53types.Coordinates{
+					Latitude:  aws.String(latitude),
+					Longitude: aws.String(longitude),
+				}
+				gp.isSet = true
 			}
-			return true
 		} else {
-			log.Errorf("Invalid coordinates format for %s: %s; expected format 'latitude,longitude'", providerSpecificGeoProximityLocationCoordinates, prop)
+			log.Warnf("Invalid coordinates format for %s: %s; expected format 'latitude,longitude'", providerSpecificGeoProximityLocationCoordinates, prop)
 		}
 	}
-	return false
+	return gp
 }
 
-func withGeoProximityLocalZoneGroup(ep *endpoint.Endpoint, geoproximity *route53types.GeoProximityLocation) bool {
-	if prop, ok := ep.GetProviderSpecificProperty(providerSpecificGeoProximityLocationLocalZoneGroup); ok {
-		geoproximity.LocalZoneGroup = aws.String(prop)
-		return true
+func (gp *geoProximity) build() *route53types.GeoProximityLocation {
+	if gp.isSet {
+		return gp.location
 	}
-	return false
+	return nil
 }
 
-func withGeoProximityBias(ep *endpoint.Endpoint, geoproximity *route53types.GeoProximityLocation) {
-	if prop, ok := ep.GetProviderSpecificProperty(providerSpecificGeoProximityLocationBias); ok {
-		bias, _ := strconv.ParseInt(prop, 10, 32)
-		geoproximity.Bias = aws.Int32(int32(bias))
-	}
+func withChangeForGeoProximityEndpoint(change *Route53Change, ep *endpoint.Endpoint) {
+
+	geoProx := newGeoProximity(ep).
+		withAWSRegion().
+		withCoordinates().
+		withLocalZoneGroup().
+		withBias()
+
+	change.ResourceRecordSet.GeoProximityLocation = geoProx.build()
+
 }
 
 // searches for `changes` that are contained in `queue` and returns the `changes` separated by whether they were found in the queue (`foundChanges`) or not (`notFoundChanges`)