Merge pull request #953 from skmatti/e2e-test-fix

Migrate Neg validator to use frontend namer factory

pkg/fuzz/default_validator_env.go

@@ -36,22 +36,31 @@ type DefaultValidatorEnv struct {
 	bc    *bcclient.Clientset
 	gce   cloud.Cloud
 	namer *namer.Namer
+	// feNamerFactory is frontend namer factory that creates frontend naming policy
+	// for given ingress/ load-balancer.
+	feNamerFactory namer.IngressFrontendNamerFactory
 }
 
 // NewDefaultValidatorEnv returns a new ValidatorEnv.
 func NewDefaultValidatorEnv(config *rest.Config, ns string, gce cloud.Cloud) (ValidatorEnv, error) {
 	ret := &DefaultValidatorEnv{ns: ns, gce: gce}
 	var err error
-	ret.k8s, err = kubernetes.NewForConfig(config)
-	if err != nil {
+	if ret.k8s, err = kubernetes.NewForConfig(config); err != nil {
+		return nil, err
+	}
+	if ret.bc, err = bcclient.NewForConfig(config); err != nil {
+		return nil, err
+	}
+	if ret.namer, err = app.NewStaticNamer(ret.k8s, "", ""); err != nil {
 		return nil, err
 	}
-	ret.bc, err = bcclient.NewForConfig(config)
+	// Get kube-system uid.
+	kubeSystemNS, err := ret.k8s.CoreV1().Namespaces().Get("kube-system", metav1.GetOptions{})
 	if err != nil {
 		return nil, err
 	}
-	ret.namer, err = app.NewStaticNamer(ret.k8s, "", "")
-	return ret, err
+	ret.feNamerFactory = namer.NewFrontendNamerFactory(ret.namer, kubeSystemNS.GetUID())
+	return ret, nil
 }
 
 // BackendConfigs implements ValidatorEnv.
@@ -86,6 +95,11 @@ func (e *DefaultValidatorEnv) Cloud() cloud.Cloud {
 }
 
 // Namer implements ValidatorEnv.
-func (e *DefaultValidatorEnv) Namer() *namer.Namer {
+func (e *DefaultValidatorEnv) BackendNamer() namer.BackendNamer {
 	return e.namer
 }
+
+// DefaultValidatorEnv implements ValidatorEnv.
+func (e *DefaultValidatorEnv) FrontendNamerFactory() namer.IngressFrontendNamerFactory {
+	return e.feNamerFactory
+}

pkg/fuzz/features/neg.go

@@ -23,18 +23,18 @@ package features
 import (
 	"context"
 	"fmt"
-	"k8s.io/klog"
 	"net/http"
 	"strconv"
 	"strings"
 
+	"k8s.io/klog"
+
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
 	"k8s.io/api/core/v1"
 	"k8s.io/api/networking/v1beta1"
 	"k8s.io/ingress-gce/pkg/annotations"
 	"k8s.io/ingress-gce/pkg/fuzz"
 	"k8s.io/ingress-gce/pkg/utils"
-	"k8s.io/ingress-gce/pkg/utils/common"
 )
 
 // NEG is a feature in GCP to support pod as Loadbalancer backends
@@ -89,19 +89,14 @@ func (v *negValidator) CheckResponse(host, path string, resp *http.Response, bod
 		return fuzz.CheckResponseContinue, err
 	}
 
-	key, err := common.KeyFunc(v.ing)
-	if err != nil {
-		return fuzz.CheckResponseContinue, err
-	}
-
-	urlMapName := v.env.Namer().UrlMap(v.env.Namer().LoadBalancer(key))
+	urlMapName := v.env.FrontendNamerFactory().Namer(v.ing).UrlMap()
 	if negEnabled {
 		if utils.IsGCEL7ILBIngress(v.ing) {
 			return fuzz.CheckResponseContinue, verifyNegRegionBackend(v.env, negName, negName, urlMapName, v.region)
 		}
 		return fuzz.CheckResponseContinue, verifyNegBackend(v.env, negName, urlMapName)
 	}
-	return fuzz.CheckResponseContinue, verifyIgBackend(v.env, v.env.Namer().IGBackend(int64(svcPort.NodePort)), urlMapName)
+	return fuzz.CheckResponseContinue, verifyIgBackend(v.env, v.env.BackendNamer().IGBackend(int64(svcPort.NodePort)), urlMapName)
 }
 
 // getNegNameForServicePort returns the NEG name for the service port if it exists.

pkg/fuzz/validator.go

@@ -26,9 +26,11 @@ import (
 	"time"
 
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud"
+	"github.com/google/go-cmp/cmp"
 	"k8s.io/api/core/v1"
 	"k8s.io/api/networking/v1beta1"
 	backendconfig "k8s.io/ingress-gce/pkg/apis/backendconfig/v1beta1"
+	"k8s.io/ingress-gce/pkg/utils/common"
 	"k8s.io/ingress-gce/pkg/utils/namer"
 	"k8s.io/klog"
 )
@@ -42,15 +44,17 @@ type ValidatorEnv interface {
 	BackendConfigs() (map[string]*backendconfig.BackendConfig, error)
 	Services() (map[string]*v1.Service, error)
 	Cloud() cloud.Cloud
-	Namer() *namer.Namer
+	BackendNamer() namer.BackendNamer
+	FrontendNamerFactory() namer.IngressFrontendNamerFactory
 }
 
 // MockValidatorEnv is an environment that is used for mock testing.
 type MockValidatorEnv struct {
-	BackendConfigsMap map[string]*backendconfig.BackendConfig
-	ServicesMap       map[string]*v1.Service
-	MockCloud         *cloud.MockGCE
-	IngressNamer      *namer.Namer
+	BackendConfigsMap    map[string]*backendconfig.BackendConfig
+	ServicesMap          map[string]*v1.Service
+	MockCloud            *cloud.MockGCE
+	IngressNamer         *namer.Namer
+	frontendNamerFactory namer.IngressFrontendNamerFactory
 }
 
 // BackendConfigs implements ValidatorEnv.
@@ -68,11 +72,16 @@ func (e *MockValidatorEnv) Cloud() cloud.Cloud {
 	return e.MockCloud
 }
 
-// Cloud implements ValidatorEnv.
-func (e *MockValidatorEnv) Namer() *namer.Namer {
+// Namer implements ValidatorEnv.
+func (e *MockValidatorEnv) BackendNamer() namer.BackendNamer {
 	return e.IngressNamer
 }
 
+// FrontendNamerFactory implements ValidatorEnv.
+func (e *MockValidatorEnv) FrontendNamerFactory() namer.IngressFrontendNamerFactory {
+	return e.frontendNamerFactory
+}
+
 // IngressValidatorAttributes are derived attributes governing how the Ingress
 // is validated. Features will use this structure to express changes to the
 // standard checks by modifying this struct.
@@ -190,8 +199,10 @@ func NewIngressValidator(env ValidatorEnv, ing *v1beta1.Ingress, features []Feat
 			return http.ErrUseLastResponse
 		},
 	}
+	frontendNamer := env.FrontendNamerFactory().Namer(ing)
 	return &IngressValidator{
 		ing:           ing,
+		frontendNamer: frontendNamer,
 		features:      fvs,
 		whiteboxTests: whiteboxTests,
 		attribs:       attribs,
@@ -203,6 +214,7 @@ func NewIngressValidator(env ValidatorEnv, ing *v1beta1.Ingress, features []Feat
 // is behaving correctly.
 type IngressValidator struct {
 	ing           *v1beta1.Ingress
+	frontendNamer namer.IngressFrontendNamer
 	features      []FeatureValidator
 	whiteboxTests []WhiteboxTest
 
@@ -234,6 +246,27 @@ func (v *IngressValidator) PerformWhiteboxTests(gclb *GCLB) error {
 	return nil
 }
 
+// FrontendNamingSchemeTest asserts that correct naming scheme is used.
+func (v *IngressValidator) FrontendNamingSchemeTest(gclb *GCLB) error {
+	// Do not need to validate naming scheme if ingress has no finalizer.
+	if !common.HasFinalizer(v.ing.ObjectMeta) {
+		return nil
+	}
+
+	// Verify that only one url map exists.
+	if l := len(gclb.URLMap); l != 1 {
+		return fmt.Errorf("expected 1 url map to exist but got %d", l)
+	}
+
+	// Verify that url map is created with correct naming scheme.
+	for key := range gclb.URLMap {
+		if diff := cmp.Diff(v.frontendNamer.UrlMap(), key.Name); diff != "" {
+			return fmt.Errorf("got diff for url map name (-want +got):\n%s", diff)
+		}
+	}
+	return nil
+}
+
 // Check runs all of the checks against the instantiated load balancer.
 func (v *IngressValidator) Check(ctx context.Context) *IngressResult {
 	klog.V(3).Infof("Check Ingress %s/%s attribs=%+v", v.ing.Namespace, v.ing.Name, v.attribs)

pkg/fuzz/validator_test.go

@@ -28,6 +28,9 @@ import (
 	"k8s.io/api/networking/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
+	"k8s.io/client-go/kubernetes/fake"
+	"k8s.io/ingress-gce/cmd/glbc/app"
+	"k8s.io/ingress-gce/pkg/utils/namer"
 	"k8s.io/klog"
 )
 
@@ -54,6 +57,11 @@ const (
 	mockValidatorSkipCheck
 )
 
+var (
+	mockNamer, _     = app.NewStaticNamer(fake.NewSimpleClientset(), "", "")
+	mockNamerFactory = namer.NewFrontendNamerFactory(mockNamer, "")
+)
+
 type mockFeature struct {
 	NullValidator
 	mode int
@@ -154,7 +162,7 @@ func TestNewIngressValidator(t *testing.T) {
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
-			_, err := NewIngressValidator(&MockValidatorEnv{}, tc.ing, tc.features, []WhiteboxTest{}, nil)
+			_, err := NewIngressValidator(&MockValidatorEnv{frontendNamerFactory: mockNamerFactory}, tc.ing, tc.features, []WhiteboxTest{}, nil)
 			gotErr := err != nil
 			if gotErr != tc.wantErr {
 				t.Errorf("NewIngressValidator() = %v; gotErr = %t, wantErr =%t", err, gotErr, tc.wantErr)
@@ -309,7 +317,7 @@ func TestValidatorCheck(t *testing.T) {
 			attribs := DefaultAttributes()
 			attribs.HTTPPort = ms.l.Addr().(*net.TCPAddr).Port
 			attribs.HTTPSPort = ms.ls.Addr().(*net.TCPAddr).Port
-			validator, err := NewIngressValidator(&MockValidatorEnv{}, tc.ing, []Feature{}, []WhiteboxTest{}, attribs)
+			validator, err := NewIngressValidator(&MockValidatorEnv{frontendNamerFactory: mockNamerFactory}, tc.ing, []Feature{}, []WhiteboxTest{}, attribs)
 			if err != nil {
 				t.Fatalf("NewIngressValidator(...) = _, %v; want _, nil", err)
 			}
@@ -384,7 +392,7 @@ func TestValidatorCheckFeature(t *testing.T) {
 			attribs.HTTPPort = ms.l.Addr().(*net.TCPAddr).Port
 			attribs.HTTPSPort = ms.ls.Addr().(*net.TCPAddr).Port
 
-			validator, err := NewIngressValidator(&MockValidatorEnv{}, tc.ing, []Feature{tc.feature}, []WhiteboxTest{}, attribs)
+			validator, err := NewIngressValidator(&MockValidatorEnv{frontendNamerFactory: mockNamerFactory}, tc.ing, []Feature{tc.feature}, []WhiteboxTest{}, attribs)
 			if gotErr := err != nil; gotErr != tc.wantNewValidatorErr {
 				t.Errorf("NewIngressValidator(...) = _, %v; gotErr = %t, want %t", err, gotErr, tc.wantNewValidatorErr)
 			}