Merge pull request #812 from spencerhance/refactor-lb-features

Refactor loadbalancer features

pkg/healthchecks/healthchecks.go

@@ -164,7 +164,7 @@ func (h *HealthChecks) createILB(hc *HealthCheck) error {
 		return err
 	}
 
-	compositeType.Version = features.L7ILBVersion(features.HealthCheck)
+	compositeType.Version = features.L7ILBVersions().HealthCheck
 	compositeType.Region = key.Region
 	err = composite.CreateHealthCheck(cloud, key, compositeType)
 	if err != nil {
@@ -215,7 +215,7 @@ func (h *HealthChecks) updateILB(oldHC, newHC *HealthCheck) error {
 	key, err := composite.CreateKey(cloud, newHC.Name, features.L7ILBScope())
 
 	// Update fields
-	compositeType.Version = features.L7ILBVersion(features.HealthCheck)
+	compositeType.Version = features.L7ILBVersions().HealthCheck
 	compositeType.Region = key.Region
 	compositeType.HttpHealthCheck.Port = 0
 	compositeType.HttpHealthCheck.PortSpecification = oldHC.HttpHealthCheck.PortSpecification
@@ -289,7 +289,7 @@ func (h *HealthChecks) Delete(name string, scope meta.KeyType) error {
 			return err
 		}
 		// L7-ILB is the only use of regional right now
-		return composite.DeleteHealthCheck(cloud, key, features.L7ILBVersion(features.HealthCheck))
+		return composite.DeleteHealthCheck(cloud, key, features.L7ILBVersions().HealthCheck)
 	}
 
 	klog.V(2).Infof("Deleting health check %v", name)
@@ -306,7 +306,7 @@ func (h *HealthChecks) getILB(name string) (*HealthCheck, error) {
 		return nil, err
 	}
 	// L7-ILB is the only use of regional right now
-	hc, err := composite.GetHealthCheck(cloud, key, features.L7ILBVersion(features.HealthCheck))
+	hc, err := composite.GetHealthCheck(cloud, key, features.L7ILBVersions().HealthCheck)
 	if err != nil {
 		return nil, err
 	}
@@ -541,7 +541,7 @@ func (hc *HealthCheck) isHttp2() bool {
 // Use Beta API for NEG as PORT_SPECIFICATION is required, and HTTP2
 func (hc *HealthCheck) Version() meta.Version {
 	if hc.forILB {
-		return features.L7ILBVersion(features.HealthCheck)
+		return features.L7ILBVersions().HealthCheck
 	}
 	if hc.isHttp2() || hc.ForNEG {
 		return meta.VersionBeta

pkg/loadbalancers/certificates.go

@@ -20,7 +20,6 @@ import (
 	"crypto/sha256"
 	"fmt"
 	"k8s.io/ingress-gce/pkg/composite"
-	"k8s.io/ingress-gce/pkg/loadbalancers/features"
 	"net/http"
 	"strings"
 
@@ -104,7 +103,7 @@ func (l *L7) createSslCertificates(existingCerts []*composite.SslCertificate) ([
 			Name:        gcpCertName,
 			Certificate: ingCert,
 			PrivateKey:  ingKey,
-			Version:     l.Version(features.SslCertificate),
+			Version:     l.Versions().SslCertificate,
 		}
 		key, err := l.CreateKey(gcpCertName)
 		if err != nil {
@@ -145,7 +144,7 @@ func (l *L7) getSslCertificates(names []string) ([]*composite.SslCertificate, er
 		if err != nil {
 			return nil, err
 		}
-		cert, err := composite.GetSslCertificate(l.cloud, key, l.Version(features.SslCertificate))
+		cert, err := composite.GetSslCertificate(l.cloud, key, l.Versions().SslCertificate)
 		if err != nil {
 			failedCerts = append(failedCerts, name+": "+err.Error())
 			if utils.IsHTTPErrorCode(err, http.StatusNotFound) {
@@ -208,7 +207,7 @@ func (l *L7) getIngressManagedSslCerts() ([]*composite.SslCertificate, error) {
 	if err != nil {
 		return nil, err
 	}
-	version := l.Version(features.SslCertificate)
+	version := l.Versions().SslCertificate
 	certs, err := composite.ListSslCertificates(l.cloud, key, version)
 	if err != nil {
 		return nil, err
@@ -268,7 +267,7 @@ func (l *L7) deleteOldSSLCerts() {
 		}
 		klog.V(3).Infof("Cleaning up old SSL Certificate %s", cert.Name)
 		key, _ := l.CreateKey(cert.Name)
-		if certErr := utils.IgnoreHTTPNotFound(composite.DeleteSslCertificate(l.cloud, key, l.Version(features.SslCertificate))); certErr != nil {
+		if certErr := utils.IgnoreHTTPNotFound(composite.DeleteSslCertificate(l.cloud, key, l.Versions().SslCertificate)); certErr != nil {
 			klog.Errorf("Old cert %s delete failed - %v", cert.Name, certErr)
 		}
 	}

pkg/loadbalancers/features/features.go

@@ -24,63 +24,78 @@ import (
 	"k8s.io/ingress-gce/pkg/utils"
 )
 
-type LBResource string
-
 const (
-	FeatureL7ILB string = "L7ILB"
+	FeatureL7ILB = "L7ILB"
+)
 
-	// L7 Resources
-	UrlMap           LBResource = "UrlMap"
-	ForwardingRule   LBResource = "ForwardingRule"
-	TargetHttpProxy  LBResource = "TargetHttpProxy"
-	TargetHttpsProxy LBResource = "TargetHttpsProxy"
-	SslCertificate   LBResource = "SslCertificate"
+// ResourceVersions allows you to define all the versions required for each resource
+// for a feature.
+type ResourceVersions struct {
+	UrlMap           meta.Version
+	ForwardingRule   meta.Version
+	TargetHttpProxy  meta.Version
+	TargetHttpsProxy meta.Version
+	SslCertificate   meta.Version
 
 	// This is *only* used for backend annotations
 	// TODO(shance): find a way to remove these
-	BackendService LBResource = "BackendService"
-	HealthCheck    LBResource = "HealthCheck"
-)
+	BackendService meta.Version
+	HealthCheck    meta.Version
+}
 
 var (
 	// versionToFeatures stores the mapping from the resource to the required API
 	// version to feature names.  This is necessary since a feature could
 	// require using different versions for each resource.
-	resourceToVersionMap = map[LBResource]map[meta.Version][]string{
-		UrlMap: {
-			meta.VersionAlpha: {FeatureL7ILB},
-		},
-		ForwardingRule: {
-			meta.VersionAlpha: {FeatureL7ILB},
-		},
-		TargetHttpProxy: {
-			meta.VersionAlpha: {FeatureL7ILB},
-		},
-		TargetHttpsProxy: {
-			meta.VersionAlpha: {FeatureL7ILB},
-		},
-		SslCertificate: {
-			meta.VersionAlpha: {FeatureL7ILB},
-		},
-		// *Only used for backend annotations*
-		BackendService: {
-			meta.VersionAlpha: {FeatureL7ILB},
-		},
-		// TODO(shance): find a way to remove this
-		HealthCheck: {
-			meta.VersionAlpha: {FeatureL7ILB},
-		},
+	// must not be nil
+	featureToVersions = map[string]*ResourceVersions{
+		FeatureL7ILB: &l7IlbVersions,
 	}
 
 	// scopeToFeatures stores the mapping from the required resource type
 	// to feature names
 	// Only add features that have a hard scope requirement
 	// TODO: (shance) refactor scope to be per-resource
 	scopeToFeatures = map[meta.KeyType][]string{
-		meta.Regional: []string{FeatureL7ILB},
+		meta.Regional: {FeatureL7ILB},
+	}
+
+	// All of these fields must be filled in to allow L7ILBVersions() to work
+	l7IlbVersions = ResourceVersions{
+		UrlMap:           meta.VersionAlpha,
+		ForwardingRule:   meta.VersionAlpha,
+		TargetHttpProxy:  meta.VersionAlpha,
+		TargetHttpsProxy: meta.VersionAlpha,
+		SslCertificate:   meta.VersionAlpha,
+		BackendService:   meta.VersionAlpha,
+		HealthCheck:      meta.VersionAlpha,
 	}
 )
 
+func NewResourceVersions() *ResourceVersions {
+	return &ResourceVersions{
+		meta.VersionGA,
+		meta.VersionGA,
+		meta.VersionGA,
+		meta.VersionGA,
+		meta.VersionGA,
+		meta.VersionGA,
+		meta.VersionGA,
+	}
+}
+
+func (r *ResourceVersions) merge(other *ResourceVersions) *ResourceVersions {
+	return &ResourceVersions{
+		UrlMap:           mergeVersions(r.UrlMap, other.UrlMap),
+		ForwardingRule:   mergeVersions(r.ForwardingRule, other.ForwardingRule),
+		TargetHttpProxy:  mergeVersions(r.TargetHttpProxy, other.TargetHttpProxy),
+		TargetHttpsProxy: mergeVersions(r.TargetHttpsProxy, other.TargetHttpsProxy),
+		SslCertificate:   mergeVersions(r.SslCertificate, other.SslCertificate),
+		BackendService:   mergeVersions(r.BackendService, other.BackendService),
+		HealthCheck:      mergeVersions(r.HealthCheck, other.HealthCheck),
+	}
+}
+
 // featuresFromIngress returns the features enabled by an ingress
 func featuresFromIngress(ing *v1beta1.Ingress) []string {
 	var result []string
@@ -91,15 +106,37 @@ func featuresFromIngress(ing *v1beta1.Ingress) []string {
 }
 
 // versionFromFeatures returns the meta.Version required for a list of features
-func versionFromFeatures(strings []string, resource LBResource) meta.Version {
-	fs := sets.NewString(strings...)
-	if fs.HasAny(resourceToVersionMap[resource][meta.VersionAlpha]...) {
-		return meta.VersionAlpha
+func versionsFromFeatures(features []string) *ResourceVersions {
+	result := NewResourceVersions()
+
+	for _, feature := range features {
+		versions := featureToVersions[feature]
+		result = result.merge(versions)
 	}
-	if fs.HasAny(resourceToVersionMap[resource][meta.VersionBeta]...) {
-		return meta.VersionBeta
+
+	return result
+}
+
+func versionOrdinal(v meta.Version) int {
+	switch v {
+	case meta.VersionAlpha:
+		return 2
+	case meta.VersionBeta:
+		return 1
+	case "":
+		return -1
 	}
-	return meta.VersionGA
+
+	return 0
+}
+
+// mergeVersions returns the newer API (e.g. mergeVersions(alpha, GA) = alpha)
+func mergeVersions(a, b meta.Version) meta.Version {
+	if versionOrdinal(a) < versionOrdinal(b) {
+		return b
+	}
+
+	return a
 }
 
 // TODO: (shance) refactor scope to be per-resource
@@ -121,7 +158,7 @@ func ScopeFromIngress(ing *v1beta1.Ingress) meta.KeyType {
 	return scopeFromFeatures(featuresFromIngress(ing))
 }
 
-// VersionFromIngress returns the required meta.Version of features for an Ingress
-func VersionFromIngressForResource(ing *v1beta1.Ingress, resource LBResource) meta.Version {
-	return versionFromFeatures(featuresFromIngress(ing), resource)
+// VersionFromIngress returns a ResourceVersions struct containing all of the resources per version
+func VersionsFromIngress(ing *v1beta1.Ingress) *ResourceVersions {
+	return versionsFromFeatures(featuresFromIngress(ing))
 }