Merge pull request #384 from MrHohn/remove-unused-version-caller

Patch NEG version into features.go and add more docs for features package
kubernetes · Jun 29, 2018 · 2c38ef2 · 2c38ef2
2 parents de6082b + 37b18c4
commit 2c38ef2
Show file tree

Hide file tree

Showing 5 changed files with 39 additions and 24 deletions.
diff --git a/pkg/backends/backends_test.go b/pkg/backends/backends_test.go
@@ -132,7 +132,7 @@ func TestBackendPoolAdd(t *testing.T) {
 				t.Fatalf("Port %v not added to instance group", sp)
 			}
 
-			hc, err := pool.healthChecker.Get(beName, sp.Version())
+			hc, err := pool.healthChecker.Get(beName, features.VersionFromServicePort(&sp))
 			if err != nil {
 				t.Fatalf("Unexpected err when querying fake healthchecker: %v", err)
 			}
@@ -201,7 +201,7 @@ func TestHealthCheckMigration(t *testing.T) {
 	pool.Ensure([]utils.ServicePort{p}, nil)
 
 	// Assert the proper health check was created
-	hc, _ := pool.healthChecker.Get(beName, p.Version())
+	hc, _ := pool.healthChecker.Get(beName, features.VersionFromServicePort(&p))
 	if hc == nil || hc.Protocol() != p.Protocol {
 		t.Fatalf("Expected %s health check, received %v: ", p.Protocol, hc)
 	}
@@ -235,7 +235,7 @@ func TestBackendPoolUpdateHTTPS(t *testing.T) {
 	}
 
 	// Assert the proper health check was created
-	hc, _ := pool.healthChecker.Get(beName, p.Version())
+	hc, _ := pool.healthChecker.Get(beName, features.VersionFromServicePort(&p))
 	if hc == nil || hc.Protocol() != p.Protocol {
 		t.Fatalf("Expected %s health check, received %v: ", p.Protocol, hc)
 	}
@@ -255,7 +255,7 @@ func TestBackendPoolUpdateHTTPS(t *testing.T) {
 	}
 
 	// Assert the proper health check was created
-	hc, _ = pool.healthChecker.Get(beName, p.Version())
+	hc, _ = pool.healthChecker.Get(beName, features.VersionFromServicePort(&p))
 	if hc == nil || hc.Protocol() != p.Protocol {
 		t.Fatalf("Expected %s health check, received %v: ", p.Protocol, hc)
 	}
@@ -280,7 +280,7 @@ func TestBackendPoolUpdateHTTP2(t *testing.T) {
 	}
 
 	// Assert the proper health check was created
-	hc, _ := pool.healthChecker.Get(beName, p.Version())
+	hc, _ := pool.healthChecker.Get(beName, features.VersionFromServicePort(&p))
 	if hc == nil || hc.Protocol() != p.Protocol {
 		t.Fatalf("Expected %s health check, received %v: ", p.Protocol, hc)
 	}
@@ -599,13 +599,19 @@ func TestBackendPoolSyncQuota(t *testing.T) {
 			quota := len(tc.newPorts)
 
 			// Add hooks to simulate quota changes & errors.
-			(fakeGCE.Compute().(*cloud.MockGCE)).MockBackendServices.InsertHook = func(ctx context.Context, key *meta.Key, be *compute.BackendService, m *cloud.MockBackendServices) (bool, error) {
+			insertFunc := func(ctx context.Context, key *meta.Key, beName string) (bool, error) {
 				if bsCreated+1 > quota {
-					return true, &googleapi.Error{Code: http.StatusForbidden, Body: be.Name}
+					return true, &googleapi.Error{Code: http.StatusForbidden, Body: beName}
 				}
 				bsCreated += 1
 				return false, nil
 			}
+			(fakeGCE.Compute().(*cloud.MockGCE)).MockBackendServices.InsertHook = func(ctx context.Context, key *meta.Key, be *compute.BackendService, m *cloud.MockBackendServices) (bool, error) {
+				return insertFunc(ctx, key, be.Name)
+			}
+			(fakeGCE.Compute().(*cloud.MockGCE)).MockBetaBackendServices.InsertHook = func(ctx context.Context, key *meta.Key, be *computebeta.BackendService, m *cloud.MockBetaBackendServices) (bool, error) {
+				return insertFunc(ctx, key, be.Name)
+			}
 			(fakeGCE.Compute().(*cloud.MockGCE)).MockBackendServices.DeleteHook = func(ctx context.Context, key *meta.Key, m *cloud.MockBackendServices) (bool, error) {
 				bsCreated -= 1
 				return false, nil

diff --git a/pkg/backends/features/doc.go b/pkg/backends/features/doc.go
@@ -16,5 +16,9 @@ limitations under the License.
 
 // This package contains the implementations of backend service
 // features.
-// TODO(mrhohn): Document what needs to happen when a feature is added.
+// For features that requrie non-GA compute API, please make sure to
+// update `versionToFeatures` and `featuresFromServicePort()` in
+// features.go (upon both feature addition and promotion). It will make
+// sure the controller interacts with compute service using the proper
+// API version.
 package features
diff --git a/pkg/backends/features/features.go b/pkg/backends/features/features.go
@@ -31,14 +31,16 @@ const (
 	FeatureHTTP2 = "HTTP2"
 	// FeatureSecurityPolicy defines the feature name of SecurityPolicy.
 	FeatureSecurityPolicy = "SecurityPolicy"
+	// FeatureNEG defines the feature name of NEG.
+	FeatureNEG = "NEG"
 )
 
 var (
 	// versionToFeatures stores the mapping from the required API
 	// version to feature names.
 	versionToFeatures = map[meta.Version][]string{
 		meta.VersionAlpha: []string{FeatureHTTP2},
-		meta.VersionBeta:  []string{FeatureSecurityPolicy},
+		meta.VersionBeta:  []string{FeatureSecurityPolicy, FeatureNEG},
 	}
 )
 
@@ -57,6 +59,9 @@ func featuresFromServicePort(sp *utils.ServicePort) []string {
 	if sp.BackendConfig != nil && sp.BackendConfig.Spec.SecurityPolicy != nil {
 		features = append(features, FeatureSecurityPolicy)
 	}
+	if sp.NEGEnabled {
+		features = append(features, FeatureNEG)
+	}
 	// Keep feature names sorted to be consistent.
 	sort.Strings(features)
 	return features

diff --git a/pkg/backends/features/features_test.go b/pkg/backends/features/features_test.go
@@ -69,6 +69,11 @@ var (
 			},
 		},
 	}
+
+	svcPortWithNEG = utils.ServicePort{
+		ID:         fakeSvcPortID,
+		NEGEnabled: true,
+	}
 )
 
 func TestFeaturesFromServicePort(t *testing.T) {
@@ -92,6 +97,11 @@ func TestFeaturesFromServicePort(t *testing.T) {
 			svcPort:          svcPortWithSecurityPolicy,
 			expectedFeatures: []string{"SecurityPolicy"},
 		},
+		{
+			desc:             "NEG",
+			svcPort:          svcPortWithNEG,
+			expectedFeatures: []string{"NEG"},
+		},
 		{
 			desc:             "HTTP2 + SecurityPolicy",
 			svcPort:          svcPortWithHTTP2SecurityPolicy,
@@ -128,6 +138,11 @@ func TestVersionFromFeatures(t *testing.T) {
 			features:        []string{FeatureSecurityPolicy},
 			expectedVersion: meta.VersionBeta,
 		},
+		{
+			desc:            "NEG",
+			features:        []string{FeatureNEG},
+			expectedVersion: meta.VersionBeta,
+		},
 		{
 			desc:            "HTTP2 + SecurityPolicy",
 			features:        []string{FeatureHTTP2, FeatureSecurityPolicy},

diff --git a/pkg/utils/serviceport.go b/pkg/utils/serviceport.go
@@ -20,7 +20,6 @@ import (
 	extensions "k8s.io/api/extensions/v1beta1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/intstr"
-	"k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud/meta"
 
 	"k8s.io/ingress-gce/pkg/annotations"
 	backendconfigv1beta1 "k8s.io/ingress-gce/pkg/apis/backendconfig/v1beta1"
@@ -54,20 +53,6 @@ func (sp ServicePort) GetDescription() Description {
 	}
 }
 
-// Version returns the support version of the ServicePort configuration
-// Alpha includes HTTP2
-// Beta includes NEG
-// The rest are v1
-func (sp ServicePort) Version() meta.Version {
-	if sp.Protocol == annotations.ProtocolHTTP2 {
-		return meta.VersionAlpha
-	}
-	if sp.NEGEnabled {
-		return meta.VersionBeta
-	}
-	return meta.VersionGA
-}
-
 // BackendName returns the name of the backend which would be used for this ServicePort.
 func (sp ServicePort) BackendName(namer *Namer) string {
 	if !sp.NEGEnabled {