From 8fdfb983220ebfe823e85ee84c063f22745ae926 Mon Sep 17 00:00:00 2001 From: Satish Matti Date: Wed, 20 Nov 2019 19:43:20 -0800 Subject: [PATCH] Migrate Neg validator to use frontend namer factory --- pkg/fuzz/default_validator_env.go | 26 +++++++++++++---- pkg/fuzz/features/neg.go | 13 +++------ pkg/fuzz/validator.go | 47 ++++++++++++++++++++++++++----- pkg/fuzz/validator_test.go | 14 +++++++-- 4 files changed, 75 insertions(+), 25 deletions(-) diff --git a/pkg/fuzz/default_validator_env.go b/pkg/fuzz/default_validator_env.go index b99fd6803a..310799e0ca 100644 --- a/pkg/fuzz/default_validator_env.go +++ b/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 +} diff --git a/pkg/fuzz/features/neg.go b/pkg/fuzz/features/neg.go index 8dcbe3a73c..575ba5ae46 100644 --- a/pkg/fuzz/features/neg.go +++ b/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. diff --git a/pkg/fuzz/validator.go b/pkg/fuzz/validator.go index 7f0c80211f..a896637643 100644 --- a/pkg/fuzz/validator.go +++ b/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) diff --git a/pkg/fuzz/validator_test.go b/pkg/fuzz/validator_test.go index bf570f1563..6deea91101 100644 --- a/pkg/fuzz/validator_test.go +++ b/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) }