From ad7abb9edd9eb9387f598f4d2d6a7e46a3385867 Mon Sep 17 00:00:00 2001 From: Spencer Hance Date: Thu, 1 Aug 2019 18:40:08 -0700 Subject: [PATCH] Refactor loadbalancers/features.go Features are now defined with structs instead of nested maps --- pkg/healthchecks/healthchecks.go | 10 +- pkg/loadbalancers/certificates.go | 9 +- pkg/loadbalancers/features/features.go | 131 +++++++++------ pkg/loadbalancers/features/features_test.go | 167 +++++++++++++++----- pkg/loadbalancers/features/l7ilb.go | 4 +- pkg/loadbalancers/forwarding_rules.go | 5 +- pkg/loadbalancers/l7.go | 21 +-- pkg/loadbalancers/loadbalancers_test.go | 15 +- pkg/loadbalancers/target_proxies.go | 7 +- pkg/loadbalancers/url_maps.go | 8 +- 10 files changed, 246 insertions(+), 131 deletions(-) diff --git a/pkg/healthchecks/healthchecks.go b/pkg/healthchecks/healthchecks.go index 87b59f09f4..78f98cd397 100644 --- a/pkg/healthchecks/healthchecks.go +++ b/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 diff --git a/pkg/loadbalancers/certificates.go b/pkg/loadbalancers/certificates.go index 622621f1d5..5ada02cdc3 100644 --- a/pkg/loadbalancers/certificates.go +++ b/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) } } diff --git a/pkg/loadbalancers/features/features.go b/pkg/loadbalancers/features/features.go index d8853ea4db..048d5e4775 100644 --- a/pkg/loadbalancers/features/features.go +++ b/pkg/loadbalancers/features/features.go @@ -24,52 +24,32 @@ 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 @@ -77,10 +57,45 @@ var ( // 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)) } diff --git a/pkg/loadbalancers/features/features_test.go b/pkg/loadbalancers/features/features_test.go index 371d32a7a7..d7053f73db 100644 --- a/pkg/loadbalancers/features/features_test.go +++ b/pkg/loadbalancers/features/features_test.go @@ -20,6 +20,7 @@ import ( "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" "k8s.io/api/networking/v1beta1" "k8s.io/apimachinery/pkg/apis/meta/v1" + "reflect" "testing" ) @@ -36,6 +37,31 @@ const ( ) var ( + fakeAlphaFeatureVersions = ResourceVersions{ + meta.VersionAlpha, + meta.VersionAlpha, + meta.VersionAlpha, + meta.VersionAlpha, + meta.VersionAlpha, + meta.VersionAlpha, + meta.VersionAlpha, + } + + fakeBetaFeatureVersions = ResourceVersions{ + meta.VersionBeta, + meta.VersionBeta, + meta.VersionBeta, + meta.VersionBeta, + meta.VersionBeta, + meta.VersionBeta, + meta.VersionBeta, + } + + // Empty fields are considered meta.VersionGA + fakeAlphaFeatureUrlMapOnlyVersions = ResourceVersions{ + UrlMap: meta.VersionAlpha, + } + emptyIng = v1beta1.Ingress{ ObjectMeta: v1.ObjectMeta{ Annotations: map[string]string{}, @@ -48,21 +74,11 @@ var ( meta.Zonal: {fakeZonalFeature}, } - makeEntry = func(ga, alpha, beta string) map[meta.Version][]string { - return map[meta.Version][]string{meta.VersionGA: {ga}, meta.VersionAlpha: {alpha}, meta.VersionBeta: {beta}} - } - - fakeResourceToVersionMap = map[LBResource]map[meta.Version][]string{ - UrlMap: { - meta.VersionGA: {fakeGaFeature}, - meta.VersionAlpha: {fakeAlphaFeature, fakeAlphaFeatureUrlMapOnly}, - meta.VersionBeta: {fakeBetaFeature}, - }, - ForwardingRule: makeEntry(fakeGaFeature, fakeAlphaFeature, fakeBetaFeature), - TargetHttpProxy: makeEntry(fakeGaFeature, fakeAlphaFeature, fakeBetaFeature), - TargetHttpsProxy: makeEntry(fakeGaFeature, fakeAlphaFeature, fakeBetaFeature), - SslCertificate: makeEntry(fakeGaFeature, fakeAlphaFeature, fakeBetaFeature), - BackendService: makeEntry(fakeGaFeature, fakeAlphaFeature, fakeBetaFeature), + fakeFeatureToVersions = map[string]*ResourceVersions{ + fakeGaFeature: NewResourceVersions(), + fakeAlphaFeature: &fakeAlphaFeatureVersions, + fakeBetaFeature: &fakeBetaFeatureVersions, + fakeAlphaFeatureUrlMapOnly: &fakeAlphaFeatureUrlMapOnlyVersions, } ) @@ -93,71 +109,138 @@ func TestScopeFromIngress(t *testing.T) { } } -func TestVersionFromFeatures(t *testing.T) { +func TestMergeVersions(t *testing.T) { + t.Parallel() testCases := []struct { - desc string - features []string - versionMap map[LBResource]meta.Version + desc string + cur meta.Version + new meta.Version + expected meta.Version }{ { - desc: "No Features", - versionMap: map[LBResource]meta.Version{ - UrlMap: meta.VersionGA, - ForwardingRule: meta.VersionGA, - TargetHttpProxy: meta.VersionGA, - TargetHttpsProxy: meta.VersionGA, - SslCertificate: meta.VersionGA, - BackendService: meta.VersionGA, - }, + desc: "cur empty", + new: meta.VersionAlpha, + expected: meta.VersionAlpha, + }, + { + desc: "new empty", + cur: meta.VersionAlpha, + expected: meta.VersionAlpha, + }, + { + desc: "Newer version is lower", + cur: meta.VersionBeta, + new: meta.VersionAlpha, + expected: meta.VersionAlpha, + }, + { + desc: "Newer version is higher", + cur: meta.VersionAlpha, + new: meta.VersionBeta, + expected: meta.VersionAlpha, + }, + { + desc: "Same version", + cur: meta.VersionGA, + new: meta.VersionGA, + expected: meta.VersionGA, + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + result := mergeVersions(tc.cur, tc.new) + + if result != tc.expected { + t.Fatalf("want %q, got %q", tc.expected, result) + } + }) + } +} + +// This test makes sure none of the fields are empty +func TestL7ILBVersions(t *testing.T) { + result := L7ILBVersions() + if result.UrlMap == "" || + result.BackendService == "" || + result.HealthCheck == "" || + result.SslCertificate == "" || + result.TargetHttpsProxy == "" || + result.TargetHttpProxy == "" || + result.ForwardingRule == "" { + t.Fatalf("L7ILBVersions returned an empty field") + } +} + +func TestVersionsFromFeatures(t *testing.T) { + testCases := []struct { + desc string + features []string + expected *ResourceVersions + }{ + { + desc: "No Features", + expected: NewResourceVersions(), }, { desc: "Alpha features", features: []string{fakeAlphaFeature}, - versionMap: map[LBResource]meta.Version{ + expected: &ResourceVersions{ UrlMap: meta.VersionAlpha, ForwardingRule: meta.VersionAlpha, TargetHttpProxy: meta.VersionAlpha, TargetHttpsProxy: meta.VersionAlpha, SslCertificate: meta.VersionAlpha, BackendService: meta.VersionAlpha, + HealthCheck: meta.VersionAlpha, }, }, { desc: "Beta features", features: []string{fakeBetaFeature}, - versionMap: map[LBResource]meta.Version{ + expected: &ResourceVersions{ UrlMap: meta.VersionBeta, ForwardingRule: meta.VersionBeta, TargetHttpProxy: meta.VersionBeta, TargetHttpsProxy: meta.VersionBeta, SslCertificate: meta.VersionBeta, BackendService: meta.VersionBeta, + HealthCheck: meta.VersionBeta, }, }, { desc: "Differing versions", features: []string{fakeGaFeature, fakeAlphaFeatureUrlMapOnly}, - versionMap: map[LBResource]meta.Version{ + expected: NewResourceVersions().merge(&ResourceVersions{UrlMap: meta.VersionAlpha}), + }, + { + desc: "Many features", + features: []string{ + fakeGaFeature, + fakeAlphaFeature, + fakeBetaFeature, + fakeAlphaFeatureUrlMapOnly, + }, + expected: &ResourceVersions{ UrlMap: meta.VersionAlpha, - ForwardingRule: meta.VersionGA, - TargetHttpProxy: meta.VersionGA, - TargetHttpsProxy: meta.VersionGA, - SslCertificate: meta.VersionGA, - BackendService: meta.VersionGA, + ForwardingRule: meta.VersionAlpha, + TargetHttpProxy: meta.VersionAlpha, + TargetHttpsProxy: meta.VersionAlpha, + SslCertificate: meta.VersionAlpha, + BackendService: meta.VersionAlpha, + HealthCheck: meta.VersionAlpha, }, }, } // Override features with fakes - resourceToVersionMap = fakeResourceToVersionMap + featureToVersions = fakeFeatureToVersions for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { + result := versionsFromFeatures(tc.features) - for resource, expectedVersion := range tc.versionMap { - result := versionFromFeatures(tc.features, resource) - if result != expectedVersion { - t.Fatalf("want %s, got %s", expectedVersion, result) - } + if !reflect.DeepEqual(result, tc.expected) { + t.Fatalf("want %v, got %v", tc.expected, result) } }) } diff --git a/pkg/loadbalancers/features/l7ilb.go b/pkg/loadbalancers/features/l7ilb.go index 7343ce4702..8612598e05 100644 --- a/pkg/loadbalancers/features/l7ilb.go +++ b/pkg/loadbalancers/features/l7ilb.go @@ -47,8 +47,8 @@ func ILBSubnetSourceRange(cloud *gce.Cloud, region string) (string, error) { } // L7ILBVersion is a helper to get the version of L7-ILB -func L7ILBVersion(resource LBResource) meta.Version { - return versionFromFeatures([]string{FeatureL7ILB}, resource) +func L7ILBVersions() *ResourceVersions { + return versionsFromFeatures([]string{FeatureL7ILB}) } // L7ILBScope is a helper to get the scope of L7-ILB diff --git a/pkg/loadbalancers/forwarding_rules.go b/pkg/loadbalancers/forwarding_rules.go index fe53381a9e..5ecb4a53a2 100644 --- a/pkg/loadbalancers/forwarding_rules.go +++ b/pkg/loadbalancers/forwarding_rules.go @@ -19,8 +19,6 @@ package loadbalancers import ( "fmt" "k8s.io/ingress-gce/pkg/composite" - "k8s.io/ingress-gce/pkg/loadbalancers/features" - "k8s.io/ingress-gce/pkg/utils" "k8s.io/klog" ) @@ -64,7 +62,7 @@ func (l *L7) checkForwardingRule(name, proxyLink, ip, portRange string) (fw *com if err != nil { return nil, err } - version := l.Version(features.ForwardingRule) + version := l.Versions().ForwardingRule fw, _ = composite.GetForwardingRule(l.cloud, key, version) if fw != nil && (ip != "" && fw.IPAddress != ip || fw.PortRange != portRange) { klog.Warningf("Recreating forwarding rule %v(%v), so it has %v(%v)", @@ -80,7 +78,6 @@ func (l *L7) checkForwardingRule(name, proxyLink, ip, portRange string) (fw *com if err != nil { return nil, err } - version := l.Version(features.ForwardingRule) rule := &composite.ForwardingRule{ Name: name, IPAddress: ip, diff --git a/pkg/loadbalancers/l7.go b/pkg/loadbalancers/l7.go index 267a0d2e2e..c7f99a3a1f 100644 --- a/pkg/loadbalancers/l7.go +++ b/pkg/loadbalancers/l7.go @@ -125,9 +125,9 @@ type L7 struct { scope meta.KeyType } -// Version() returns the required meta.Version for a specific resource -func (l *L7) Version(resource features.LBResource) meta.Version { - return features.VersionFromIngressForResource(&l.ingress, resource) +// Version() returns the struct listing the versions for every resource +func (l *L7) Versions() *features.ResourceVersions { + return features.VersionsFromIngress(&l.ingress) } // CreateKey creates a meta.Key for use with composite types @@ -226,13 +226,14 @@ func (l *L7) GetIP() string { func (l *L7) Cleanup() error { var key *meta.Key var err error + versions := l.Versions() fwName := l.namer.ForwardingRule(l.Name, utils.HTTPProtocol) klog.V(2).Infof("Deleting global forwarding rule %v", fwName) if key, err = l.CreateKey(fwName); err != nil { return err } - if err := utils.IgnoreHTTPNotFound(composite.DeleteForwardingRule(l.cloud, key, l.Version(features.ForwardingRule))); err != nil { + if err := utils.IgnoreHTTPNotFound(composite.DeleteForwardingRule(l.cloud, key, versions.ForwardingRule)); err != nil { return err } @@ -241,7 +242,7 @@ func (l *L7) Cleanup() error { if key, err = l.CreateKey(fwsName); err != nil { return err } - if err := utils.IgnoreHTTPNotFound(composite.DeleteForwardingRule(l.cloud, key, l.Version(features.ForwardingRule))); err != nil { + if err := utils.IgnoreHTTPNotFound(composite.DeleteForwardingRule(l.cloud, key, versions.ForwardingRule)); err != nil { return err } @@ -258,7 +259,7 @@ func (l *L7) Cleanup() error { if key, err = l.CreateKey(tpName); err != nil { return err } - if err := utils.IgnoreHTTPNotFound(composite.DeleteTargetHttpProxy(l.cloud, key, l.Version(features.TargetHttpProxy))); err != nil { + if err := utils.IgnoreHTTPNotFound(composite.DeleteTargetHttpProxy(l.cloud, key, versions.TargetHttpProxy)); err != nil { return err } @@ -267,7 +268,7 @@ func (l *L7) Cleanup() error { if key, err = l.CreateKey(tpsName); err != nil { return err } - if err := utils.IgnoreHTTPNotFound(composite.DeleteTargetHttpsProxy(l.cloud, key, l.Version(features.TargetHttpsProxy))); err != nil { + if err := utils.IgnoreHTTPNotFound(composite.DeleteTargetHttpsProxy(l.cloud, key, versions.TargetHttpsProxy)); err != nil { return err } @@ -284,7 +285,7 @@ func (l *L7) Cleanup() error { if key, err = l.CreateKey(cert.Name); err != nil { return err } - if err := utils.IgnoreHTTPNotFound(composite.DeleteSslCertificate(l.cloud, key, l.Version(features.SslCertificate))); err != nil { + if err := utils.IgnoreHTTPNotFound(composite.DeleteSslCertificate(l.cloud, key, versions.SslCertificate)); err != nil { klog.Errorf("Old cert delete failed - %v", err) certErr = err } @@ -300,7 +301,7 @@ func (l *L7) Cleanup() error { if key, err = l.CreateKey(umName); err != nil { return err } - if err := utils.IgnoreHTTPNotFound(composite.DeleteUrlMap(l.cloud, key, l.Version(features.UrlMap))); err != nil { + if err := utils.IgnoreHTTPNotFound(composite.DeleteUrlMap(l.cloud, key, versions.UrlMap)); err != nil { return err } @@ -318,7 +319,7 @@ func GetLBAnnotations(l7 *L7, existing map[string]string, backendSyncer backends } backendState := map[string]string{} for _, beName := range backends { - version := l7.Version(features.BackendService) + version := l7.Versions().BackendService state, err := backendSyncer.Status(beName, version, l7.scope) // Don't return error here since we want to keep syncing if err != nil { diff --git a/pkg/loadbalancers/loadbalancers_test.go b/pkg/loadbalancers/loadbalancers_test.go index 328dad5350..229824a3ae 100644 --- a/pkg/loadbalancers/loadbalancers_test.go +++ b/pkg/loadbalancers/loadbalancers_test.go @@ -36,7 +36,6 @@ import ( "k8s.io/ingress-gce/pkg/composite" "k8s.io/ingress-gce/pkg/events" "k8s.io/ingress-gce/pkg/instances" - "k8s.io/ingress-gce/pkg/loadbalancers/features" "k8s.io/ingress-gce/pkg/utils" "k8s.io/kubernetes/pkg/util/slice" "k8s.io/legacy-cloud-providers/gce" @@ -251,15 +250,16 @@ func TestCreateHTTPSLoadBalancer(t *testing.T) { func verifyHTTPSForwardingRuleAndProxyLinks(t *testing.T, j *testJig, l7 *L7) { t.Helper() lbName := j.namer.LoadBalancer(ingressName) + versions := l7.Versions() key, err := composite.CreateKey(j.fakeGCE, j.UMName(lbName), l7.scope) if err != nil { t.Fatal(err) } - um, err := composite.GetUrlMap(j.fakeGCE, key, l7.Version(features.UrlMap)) + um, err := composite.GetUrlMap(j.fakeGCE, key, versions.UrlMap) key.Name = j.TPName(lbName, true) - tps, err := composite.GetTargetHttpsProxy(j.fakeGCE, key, l7.Version(features.TargetHttpsProxy)) + tps, err := composite.GetTargetHttpsProxy(j.fakeGCE, key, versions.TargetHttpsProxy) if err != nil { t.Fatalf("j.fakeGCE.GetTargetHTTPSProxy(%q) = _, %v; want nil", j.TPName(lbName, true), err) } @@ -268,7 +268,7 @@ func verifyHTTPSForwardingRuleAndProxyLinks(t *testing.T, j *testJig, l7 *L7) { } key.Name = j.FWName(lbName, true) - fws, err := composite.GetForwardingRule(j.fakeGCE, key, l7.Version(features.ForwardingRule)) + fws, err := composite.GetForwardingRule(j.fakeGCE, key, versions.ForwardingRule) if err != nil { t.Fatalf("j.fakeGCE.GetGlobalForwardingRule(%q) = _, %v, want nil", j.FWName(lbName, true), err) } @@ -283,14 +283,15 @@ func verifyHTTPSForwardingRuleAndProxyLinks(t *testing.T, j *testJig, l7 *L7) { func verifyHTTPForwardingRuleAndProxyLinks(t *testing.T, j *testJig, l7 *L7) { t.Helper() lbName := j.namer.LoadBalancer(ingressName) + versions := l7.Versions() key, err := composite.CreateKey(j.fakeGCE, j.UMName(lbName), l7.scope) if err != nil { t.Fatal(err) } - um, err := composite.GetUrlMap(j.fakeGCE, key, l7.Version(features.UrlMap)) + um, err := composite.GetUrlMap(j.fakeGCE, key, versions.UrlMap) key.Name = j.TPName(lbName, false) - tps, err := composite.GetTargetHttpProxy(j.fakeGCE, key, l7.Version(features.TargetHttpProxy)) + tps, err := composite.GetTargetHttpProxy(j.fakeGCE, key, versions.TargetHttpProxy) if err != nil { t.Fatalf("j.fakeGCE.GetTargetHTTPProxy(%q) = _, %v; want nil", j.TPName(lbName, false), err) } @@ -298,7 +299,7 @@ func verifyHTTPForwardingRuleAndProxyLinks(t *testing.T, j *testJig, l7 *L7) { t.Fatalf("tp.UrlMap = %q, want %q", tps.UrlMap, um.SelfLink) } key.Name = j.FWName(lbName, false) - fws, err := composite.GetForwardingRule(j.fakeGCE, key, l7.Version(features.ForwardingRule)) + fws, err := composite.GetForwardingRule(j.fakeGCE, key, versions.ForwardingRule) if err != nil { t.Fatalf("j.fakeGCE.GetGlobalForwardingRule(%q) = _, %v, want nil", j.FWName(lbName, false), err) } diff --git a/pkg/loadbalancers/target_proxies.go b/pkg/loadbalancers/target_proxies.go index 1f40b6e76a..3af0277275 100644 --- a/pkg/loadbalancers/target_proxies.go +++ b/pkg/loadbalancers/target_proxies.go @@ -19,7 +19,6 @@ package loadbalancers import ( "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud" "k8s.io/ingress-gce/pkg/composite" - "k8s.io/ingress-gce/pkg/loadbalancers/features" "k8s.io/ingress-gce/pkg/utils" "k8s.io/klog" ) @@ -42,7 +41,7 @@ func (l *L7) checkProxy() (err error) { if err != nil { return err } - version := l.Version(features.TargetHttpProxy) + version := l.Versions().TargetHttpProxy proxy, _ := composite.GetTargetHttpProxy(l.cloud, key, version) if proxy == nil { klog.V(3).Infof("Creating new http proxy for urlmap %v", l.um.Name) @@ -106,7 +105,7 @@ func (l *L7) checkHttpsProxy() (err error) { if err != nil { return err } - version := l.Version(features.TargetHttpProxy) + version := l.Versions().TargetHttpProxy proxy, _ := composite.GetTargetHttpsProxy(l.cloud, key, version) description, err := l.description() if err != nil { @@ -183,7 +182,7 @@ func (l *L7) getSslCertLinkInUse() ([]string, error) { if err != nil { return nil, err } - proxy, err := composite.GetTargetHttpsProxy(l.cloud, key, l.Version(features.TargetHttpsProxy)) + proxy, err := composite.GetTargetHttpsProxy(l.cloud, key, l.Versions().TargetHttpsProxy) if err != nil { return nil, err } diff --git a/pkg/loadbalancers/url_maps.go b/pkg/loadbalancers/url_maps.go index e8cde1e21f..9120dd76cb 100644 --- a/pkg/loadbalancers/url_maps.go +++ b/pkg/loadbalancers/url_maps.go @@ -20,12 +20,10 @@ import ( "crypto/md5" "encoding/hex" "fmt" - "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" - "k8s.io/ingress-gce/pkg/composite" - "k8s.io/ingress-gce/pkg/loadbalancers/features" - "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud" + "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/ingress-gce/pkg/composite" "k8s.io/ingress-gce/pkg/utils" "k8s.io/klog" ) @@ -52,7 +50,7 @@ func (l *L7) ensureComputeURLMap() error { expectedMap := toCompositeURLMap(l.Name, l.runtimeInfo.UrlMap, l.namer, key) key.Name = expectedMap.Name - expectedMap.Version = l.Version(features.UrlMap) + expectedMap.Version = l.Versions().UrlMap currentMap, err := composite.GetUrlMap(l.cloud, key, expectedMap.Version) if utils.IgnoreHTTPNotFound(err) != nil { return err